text:
|Sterling Silver
|3.59 grams (2.31 DWT)
|Other Details
Lifetime Diamond and Gemstone Warranty
We will replace any diamond, ruby, or sapphire with a diamond, ruby, or sapphire of comparable quality to your original gemstone purchased from Incendia Diamonds, which in normal use chips, cracks, or separates from its mounting and is lost for the purchaser's lifetime. This warranty is valid, provided the diamond, ruby, or sapphire mounting has been cleaned and checked at least once every six months by Incendia Diamonds and that such inspection has been verified in writing in the space provided in the Lifetime Diamond and Gemstone Warranty Certificate by an authorized representative of Incendia Diamonds.
The Lifetime Diamond Warranty is non-transferable and will be considered invalid should the purchaser have the ring worked on by any jeweler other than Incendia Diamonds. Your Lifetime Diamond and Gemstone Warranty will be honored when you bring your ring to our Incendia Diamonds location. Please be sure to bring the Lifetime Diamond and Gemstone Warranty Certificate and your sales receipt with you to take advantage of the free inspection and cleaning. Remember, you must have your diamond ring inspected at least twice per year (once every six months), perform any necessary repair, and have the inspection verified in writing to keep your Lifetime Diamond and Gemstone Warranty valid.
Online Purchases:
If you purchased your ring online, you must come into our Incendia Diamonds store within 30 days of your purchase to receive your Lifetime Diamond and Gemstone Warranty Certificate, and have your item inspected by Incendia Diamonds once every six months thereafter, in order for your warranty to stay current and valid. We will not accept any product that is mailed to us.
Please be Aware:
Incendia Diamonds does not warranty diamonds, rubies, or sapphires purchased elsewhere and then set into our rings. There are no warranties, expressed or implied, written or oral, other than as set forth herein. The warrantor shall not be liable for incidental or consequential damages.
The price for this item is as shown, though actual cost may vary depending on the metal chosen and customizations requested. Ring prices subject to change due to fluctuation of metal and diamond pricing.
Incendia Diamonds has something for everyone and for every budget. If a price is not listed with your selection, it may be due to volatility in the precious metals market or some other restrictions. Please contact one of our fine jewelry consultants for pricing based on your selection. You can call (858) 909-8224 or, of course, come in and see the selection in person.
We endeavor to make available all items appearing on our website. Occasionally, that may not be the case as demand for a particular item may spike abruptly. If you cannot find what you are looking for, it is out of stock, or on back order, please contact us and we will strive to find you your desired item expeditiously. And of course, many items can be special-ordered or custom-designed and created by our master jewelers on-site.
After your item has been packaged, it will be shipped using Overnight Delivery via UPS, FEDEX, USPS, or other nationally recognized carrier. Your order will be shipped to you fully insured. All orders over $100 will be shipped Overnight Delivery, FREE of charge. We urge all customers to inspect their package for damage or tampering before receiving or signing for receipt.
Shipping to an Alternate Address
In an effort to help protect our customers against fraud, we will not ship to an address other than the billing address of the credit card used for purchase. If you wish to ship to an alternate address, you will need to call your credit card company and add the alternate address to your account.
(See complete Shipping Policy)
30-Day Money Back Guarantee Online
At Incendia Diamonds, quality is our guarantee. We will meet and surpass your expectations. We have the highest standards in the industry for every setting, gemstone, and diamond we offer. If you are not completely satisfied with your Incendia Diamonds purchase, just return the unused, unaltered item in saleable condition within 30 days of your purchase date and we will refund the full purchase amount to you. It's our promise to you - in writing.
See our 30-Day Return Policy & Money Back Guarantee for full details and how to return online purchases.


text:
Do you have any questions?
Ask a Videoguys adviser about this product. More
Our knowledgeable sales staff is available Mon-Fri, 9am – 5pm ET to help answer any questions you may have regarding system recommendations, compatibility, features and more. Call us at 800-323-2325
Free Tech Support
Shop with confidence with us here to help. More
Videoguys technicians are the best in the industry and are available Mon-Fri, 9am – 5pm ET to help you get your system up and running. We can even help you get in touch with the manufacturer's Help Desk when needed. Call our tech directly at 516-759-1611
Satisfaction Guaranteed!
30-day money-back or exchange on all purchases. More
We have FREE Tech Support and a 30-day money-back guarantee on all products we sell. If we can't help you get your video editing and production purchase working, you can return it for a refund or we can exchange it for a replacement or apply the value of the product you returned toward the purchase of a different product. If you choose to exchange your purchase, we give you a fresh 30 days to make it work!

subset: commoncrawl


text:
Sold out.
Quik – Titled "Paris Throw Up". Paint Marker and Spray on Parisian transit Map. 2011. Measures 25.5 × 14.5 Inches.
'Quik' Lin Felton (°1958) started tagging during the 1970's in Hollis (Queens, New York), the neighbourhood were he grew up. He's one of the few New York graffiti artists who made it to the art galleries and museums in the 1980's. His work has developed far beyond the original tagging and although he preserved the spirit of the graffiti, he expanded at the same time their conceptual limits by introducing both social and personal topics into his paintings.
The 'Blues Paintings' can hardly be described as 'mere graffiti'. Through the themes as well as by the painting technique, Quik's graffiti become more than 'just' writing. In a subtle, yet ironic way, a well considered balance between statement and sentiment is obtained. Although the canvases at first sight appear like fundamental socio-political critiques, they are still strongly autobiographic. It's in this interaction between global and personal issues, that the artist as a Black American excels, by pushing the intends of his works beyond the ordinary manifest. As a matter of fact, in the way it deals with African-American life and history, the works of Lin Felton can be perceived as the complete antithesis of socio-political art. Through his works, the artist seems to wrap himself in a pleasant state of discomfort when playing with his ambiguous relation towards oppression. On the one hand, he sings the sorrow of the Black Man through his paint sprayers, but on the other hand he expresses a kind of deeply erotic pleasure by adopting an underdog position towards women. This sometimes confusing duality is not only visible at the conceptual level, but also through the painting technique. 'Quik' is well aware of his artistic predecessors and he regularly winks towards art history. Inevitably one recognises Jasper Johns 'Flag' or Roy Lichtenstein's paradigmatic way of representing women in strong black outlines. But despite these elements taken popular culture, Lin Felton can not be considered a Pop artist. Opposite to his well controlled and colourful, almost canonised figurations, stands a hand that paints with a big amount of bravado. Instead of bright colours, his pallet turns dark, and strong outlines and forms become very irregular. In this way, his interest in painting itself becomes more important than the actual representation, as is for example shown in 'Enter the King' (2007). The path chosen by the paint dominates the hand of the artist. The Pop imagery has to make place for a very expressionistic way of rendering, which even tends towards voodoo, as if the painter tries to enchant the spectator in 'Wall of Sleep' (2007). Or is it a well considered reference towards the American Abstract Expressionists? Either way, the combination of well known, joyful representations versus a very personal and obscure touch, creates a quite contradictory but pleasant sense of uncanniness, as illustrated by 'Come 'ere Lil' man' (2007). In this work, a naïvely painted middle-class man becomes the 'object of desire' of an almost witch-like dominatrix. This lustful but still obscure way of representing women throughout painting is a regularly returning subject, both in ancient and contemporary art".
But despite their art-historical references, the paintings of Lin Felton have their roots in the streets, where history and theory are subordinate to the representation of personality and style. If graffiti are colourful parasites on the urban image, then Lin Felton's 'Blues Paintings' are their offspring, feeding on contemporary art.


text:
"When I do get time, I like to discover the world. Discover, that's what the "D" in Vitamin D really stands for."
Related quotes
"Our lives a series of defining moments, strung together by passing time. Surrender fully to this moment, because it is not the moment itself that defines us, but how we choose to live in it."
"Live life fully while you're here. Experience everything. Take care of yourself and your friends. Have fun, be crazy, be weird. Go out and screw up! You're going to anyway, so you might as well enjoy the process. Take the opportunity to learn from your mistakes: find the cause of your problem and eliminate it. Don't try to be perfect; just be an excellent example of being human."
"To live in this single moment, this lightest breath, this softest touch, to be captured by this smallest, this briefest space of time, to be fully present, alive, consumed in this instant alone, this is truly living."


text:
Cheesy Jalapeño Popper Baked Stuffed Chicken
What happens when you combine Skinny Baked Jalapeño Poppers with chicken??
Oh yeah baby!! Cheesy stuffed chicken breast stuffed with diced jalapeño, cream cheese, cheddar jack cheese, scallions and bacon!
I basically deconstructed my jalapeño poppers and stuffed them into chicken and the results where mouth watering!! I used bread crumbs because I was out of panko, but I think seasoned panko would give these a wonderful crunch like I used on my Skinny Baked Jalapeño Poppers.
I made the serving size a generous two pieces, so a simple vegetable, fiesta lime rice, or a salad on the side would make this a complete meal. Trust me, you won't want to stop at just one!
And those of you on Pinterest, if you want some Skinny Cinco De Mayo inspiration, check out my Cinco De Mayo board!
Cheesy Jalapeño Popper Baked Stuffed Chicken
Servings: 4 • Serving Size: 2 stuffed breasts • Points +: 9 pts • Smart Points: 5
Calories: 371 • Fat: 17.5 g • Protein: 42.7 g • Carb: 11 g • Fiber: 1.5 g • Sugar: 0.6 g
Sodium: 355.8 mg (without salt)
- 2 slices center cut bacon, cooked and crumbled
- 3 jalapeños, chopped (remove seeds for milder)
- 3 oz 1/3 less fat cream cheese, softened
- 4 oz reduced fat shredded cheddar jack (Sargento)
- 2 tbsp chopped scallions
- 8 thin sliced skinless chicken breast cutlets, 3 oz each
- 1/2 cup Italian seasoned whole wheat breadcrumbs
- 1 1/2 juicy limes, juice of
- 1 tbsp olive oil
- salt and fresh pepper
- olive oil non-stick spray
Wash and dry chicken cutlets, season with salt and pepper. Preheat oven to 450°. Lightly spray a baking dish with non-stick spray.
Combine cream cheese, cheddar, scallions, jalapeño and bacon crumbles in a medium bowl.
Lay chicken cutlets on a working surface and spread 2 tbsp of cream cheese mixture on each cutlet. Loosely roll each one, secure the ends with toothpicks to prevent the cheese from oozing out.
Place breadcrumbs in a bowl; in a second bowl combine olive oil, lime juice, salt and pepper.
Dip chicken in lime-oil mixture, then in breadcrumbs and place seam side down on a baking dish. Repeat with the remaining chicken. When finished, lightly spray the top of the chicken with oil spray.
Bake 22-25 minutes, serve immediately.


text:
Understand What Your Benefits Are and How to Request Them
As a veteran, you may qualify for burial benefits through the U.S. Department of Veterans Affairs (VA), so it's important to understand what your benefits are and how to request them.
A recent survey shows that 60 percent of veterans mistakenly believe the VA pays funeral, cremation and cemetery expenses for all veterans.
Because we serve more veterans than any other funeral service provider, the funeral professionals of the Dignity Memorial® network discovered that many veterans don't know all the facts when they or their families come in to plan their funeral services. For example, a recent survey shows that 60 percent of veterans mistakenly believe the VA pays funeral, cremation and cemetery expenses for all veterans. In truth, reimbursement of funeral or cremation expenses is limited, and certain restrictions apply.
Six out of ten veterans also believe caskets are provided free of charge to all veterans. Actually, neither the VA nor the various branches of service provide a free casket for a deceased veteran unless death occurs while on active duty.
These and other misperceptions are common. That's why the Dignity Memorial network created the Veterans Planning Guide. The Veterans Planning Guide is a full-color, 56-page guide designed to educate veterans about their VA burial benefits as well as the benefits Dignity Memorial providers extend to members of two veterans service organizations.
The guide also contains useful information about military funeral honors, information on the importance of advance planning and several detachable forms, including applications to veterans service organizations and forms necessary to receive military medals, a U.S. flag, Presidential Memorial certificates, military records and other VA burial benefits.
To receive a free Veterans Planning Guide, contact a Dignity Memorial provider near you or call toll-free 866-508-5834. A Dignity Memorial funeral professional will gladly deliver and review the guide with you, so you can take advantage of all the VA burial benefits you earned through your service.
Find a Service Provider
The Dignity Memorial® network serves families through more than 2,000 service providers across North America.
Find a funeral home, cremation provider or cemetery near you.
Dignity Memorial Veterans Planning Guide
Learn more about veterans burial benefits with the Dignity Memorial network's free 56-page Veterans Planning Guide or:


text:
This is step 1 in the WSCCI/Symfony shift in our core routing logic. See this post for background. I will not repeat it here for brevity. (Rare for me, I know.)
Proposed resolution
This issue puts a Symfony2 HttpKernel-based routing model on top of our current bootstrap, effectively replacing menu_execute_active_handler(). It is not fully developed yet. There are still a lot of @todos in the code. The goal of this patch is to get the foundation in place so that we can build on top of it, hopefully in parallel.
Please read this blog series on the Symfony2 components before reviewing this patch. I know it's long (although it's actually a quick read), but reading that will vastly help understand the moving parts in this patch and save everyone time explaining things that are already documented.
Given that, what this patch does is as follows:
- Creates a Drupal kernel, called DrupalKernel, that extends the Symfony HttpKernel implementation. That lets us do our own setup, but leverage all of the existing workflow from Symfony itself.
Creates a Drupal-specific UrlMatcher class, extending Symfony's. This class is responsible for determining the correct Route (what Drupal 7 calls a router item) for a given request. At this point it is just a fairly trivial wrapper around the existing menu lookup system. This is a temporary measure; follow-up work will add a new, more robust routing component that can live along side this one for now, and then we can remove the old one entirely once everything is ported over.
- Registers a number of subscribers to the kernel. Subscribers are essentially a convenient way to bundle Listeners, which are more or less the Symfony equivalent of hooks. HttpKernel, which we are using here, has numerous event points to control various parts of the request process and allow others to hook into it. See the code for all of the nitty gritty
- drupal_page_footer() is now a kernel "terminate" event. This needs to get cleaned up later.
- We do not actually route on the path of the request. Drupal 7 manipulated the request path in-place, stored it into $_GET['q'], and then pretended that was what the browser asked for. Instead, we now have a request attribute, system_path, that we populate in the kernel "request" event. Currently we do four things to it, all ported from the legacy handling, from different listeners: urldcode() the path, handle language prefixing on the path, replace an empty request with the front page value, and url alias dereferencing. Although it wasn't the original intent, it means that we have essentially replaced hook_url_inbound_alter() entirely with "you can do that in the kernel request event if you want", and then ported all of our existing logic to that. As a side effect, it means that it will likely be possible to move url alias functionality out of core to a module if we want. I don't know if we want to do that, but the fact that we will be able to (after we refactor url generation, too) is really cool. :-)
- Any controller that is a function we assume to be a legacy controller and wrap in an anonymous function. That means we're not actually using any of Symfony's magic controller argument handling yet, but it means we needed to change almost nothing in the current routing system. That's a good trade off. Once the new routing system is in place we will leverage all of that fun new stuff.
- Returning MENU_NOT_FOUND or MENU_ACCESS_DENIED no longer works. The new correct way to handle those is to throw an exception, which the new ExceptionController will turn into the appropriate page response.
- The database system no longer throws PDOExceptions. Because of Symfony's FlattenException class, we cannot pass extra arbitrary public properties forward on the exception object to carry the query information. Instead, we ported the "the query that broke was" logic into the database layer directly, and took advantage of PHP 5.3's nested exception capability.
Note: Please *do not post new patches here*. The code is being worked on in the kernel branch of the WSCCI sandbox. New patches will be rolled from that. If you want to help writing code, contact Crell.
Remaining tasks
Follow up issues are being tagged kernel-followup. The following is a list that was started prior to that issue tag. Make sure these items are posted as issues and tagged appropriately.
- Integrate the kernel and its components with the dependency injection container.
- Replace current_path(), request_path(), and so forth with direct access to the request object, somehow. Or better, refactor enough code so that we don't need utility functions or the direct request object at all.
- Develop the new routing system to replace our current one, as discussed.
- Either extend or replace hook_menu() so that we can declare multiple router items at the same path for different mime types and HTTP methods. Since the router system and menu links system have no business being as intertwined as they are now, replacing hook_menu() entirely is a viable approach we will consider. (Possibly put it in CMI?)
- Replace the trivial access listener with something more flexible, powerful, and non-trivial. This should be considered as part of the previous two items.
- Revise the Ajax system to be more self-documenting and intuitive, and fit better with this new listener model.
- Eliminate our various "screw with the global request data" bootstrap routines, since we will not be screwing with it anymore.
- Lots of other things, but we'll get there. :-)
User interface changes
There shouldn't be any of note. This is all at the API level.
API changes
1) Delivery callbacks are gone in favor of kernel View events.
2) *Most* things are emulated fairly well at the moment, so most modules do not need to change. The big API changes will come in follow-ups. However, some more esoteric edge cases (returning NULL from a page handler, anywhere that we print and then call drupal_exit(), etc.) cannot be emulated so are being altered to simply return a response object.
PASSED: [[SimpleTest]]: [MySQL] 37,009 pass(es). View
PASSED: [[SimpleTest]]: [MySQL] 36,856 pass(es). View
|Dave Reid
PASSED: [[SimpleTest]]: [MySQL] 36,681 pass(es). View
|Dave Reid
PASSED: [[SimpleTest]]: [MySQL] 36,680 pass(es). View
PASSED: [[SimpleTest]]: [MySQL] 36,697 pass(es). View
Comment #3moshe weitzman CreditAttribution: moshe weitzman commented
+++ b/core/lib/Drupal/Core/DrupalApp.phpundefined
@@ -0,0 +1,106 @@
+class DrupalApp implements HttpKernelInterface {
Would it make sense to call this DrupalKernel?
+++ b/core/lib/Drupal/Core/DrupalApp.phpundefined
@@ -0,0 +1,106 @@
+ * @todo Make the listeners that get attached extensible, but without using
Thats quite a major todo IMO. I would like to see this resolved before commit. As I said, I want a 'fully expressed' commit. Thats different from fully implemented which is where the slog of conversion comes in.
+++ b/core/lib/Drupal/Core/EventSubscriber/AccessSubscriber.phpundefined
@@ -0,0 +1,54 @@
+ * Verifys that the current user can access the requested path.
Dreditor (Chrome) just choked on this big patch so switching to narrative style review ...
- Use $this->context instead of variable_get('site_frontpage', 'user');. I meant Config, not $this->context
- The query on menu_router currently returns multiple items and then menu_get_item() winnows down the list to the first one that has access. We chould consider breaking this up in a Symfony way where access check goes there. I know we plan to work on this later. I'm suggesting that we could do some refactoring in this patch and still keep current hook_menu() intact.
It would be nice to remove Drupal's delivery callback system in this patch.
Looks like we are adding a nation-state worth of Symfony code. It has dupes of existing code like Lock API (see lock method), Watchdog (HttpKernel\Log;), Timer API (Stopwatch), Browser object, etc. It reimplements much of Varnish in PHP. It is all high quality, but are we really simplifying Drupal? All this for Web Services and Panels Everywhere? Those are two projects we already have. I get that we want to do them better, but still; there are alternative paths to better. What I'm trying to say is that I don't know that I support this yet and my comments above should not be taken as 'These are Moshe's only objections'. Also, I have yet to review the flow in a debugger. Will do.
Comment #4EclipseGc CreditAttribution: EclipseGc commented
$this->context instead of variable_get('site_frontpage', 'user');
This is a CMI thing, so we can't really update that till CMI's landed in core. Should be trivial after that though I would think.
Comment #5webchick CreditAttribution: webchick commented
Comment #6bojanz CreditAttribution: bojanz commented
DrupalKernel sounds more sensible to me as well.
Not clear to me why we're adding one subscriber in getDispatcher() and the others in handle(). Is it because the ones in getDispatcher() will be taken from an assembled list later on?
+ * @todo Make the listeners that get attached extensible, but without using hooks.
CMI perhaps? I guess that's the plugin discovery discussion again, irrelevant at this moment.
What also caught my eye is that Drupal's UrlMatcher::match($pathinfo) needs a comment explaining $this->allow().
It is initialized at the top of the method, and evaluated at the end, but the actual modification happens in another method call (matchCollection(), which is defined in the parent class), which took me a few minutes to realize. And I'm still unclear about what it actually does.
The sha256 hash call also looked curious, later saw that it is done to make the path pass validation when passed as the name parameter. Maybe we can get away with using a faster function such as md5 (if it's actually faster at all, just a stray thought.)
As for the size of the added Symfony code, I'm not really worried about that. It's their code. If there are extra pieces we are not using, it's not the end of the world.
And since in the future we could use Composer to just pull in the Symfony dependencies and not keep it ourselves (unless we patched it and have it in our own repo, but still separate from the main repo), that would be even more of a non-issue.
This is a general theme we'll always run into with libraries, we had it with JQuery UI, we have it in Symfony.
Comment #7yched CreditAttribution: yched commented
add tag ?
Comment #8bojanz CreditAttribution: bojanz commented
What happens now with #1417320: Introduce the Request object?
It relates to #3 and has no matching changes in the current patch, so I didn't want to mark it as duplicate.
Comment #10sun CreditAttribution: sun commented
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module. View
Same as #1, but without all the Symfony libraries.
(see #1465584: Review external library dependencies in core)
Comment #12effulgentsia CreditAttribution: effulgentsia commented
Looks like some really good progress has been made, and I'm happy to see HttpKernel itself being used, not just the interface. Crell's in the middle of some work on removing the need for any hook_menu() changes from this initial patch, and once he merges that back into the "kernel" branch, I'll look into the AJAX bits to see if we can fully remove delivery callbacks as part of this patch.
Comment #14effulgentsia CreditAttribution: effulgentsia commented
+++ b/core/includes/common.inc
@@ -2184,31 +2184,20 @@ function url($path = NULL, array $options = array()) {
+ // If Clean URLs are not enabled, we need to prefix the script name onto
+ // the link.
+ // @todo: Make this dynamic based on the request object without using a global
+ // request object.
+ if (empty($GLOBALS['conf']['clean_url'])) {
+ $base .= 'index.php/';
+ }
- Why this (and surrounding) change to the url() function as part of this issue?
- Hard-coding 'index.php' instead of using $options['script'] is wrong.
- $GLOBALS['conf']['clean_url'] seems to be NULL after a fresh install (is that a new bug/feature in D8?) In any case, combined with the hard-coded index.php, that's what messes up Overlay. Removing this line (on a setup where clean URLs work) makes Overlay work.
Comment #15effulgentsia CreditAttribution: effulgentsia commented
Ok, ignore #14. I see now that this issue is changing non-clean URLs from http://example.com/?q=foo to http://example.com/index.php/foo. If we really want to do that, that will necessitate fixing several things both Overlay related and not. Specifically within overlay, the Drupal.overlay.getPath(), Drupal.overlay.eventhandlerOverrideLink(), Drupal.overlay.fragmentizeLink() functions, and maybe others. So I guess a question is: do we really need to make this change now, or can we leave non-clean URLs as http://example.com/?q=foo until a future issue?
"kernel" branch as-is has Overlay working just fine once you enable clean URLs, so this really does just boil down to what we want to do with non-clean URLs at this time.
Comment #17catch CreditAttribution: catch commented
With changing the paths is this will break existing links on all sites that don't have clean URLs - that's a small enough proportion of sites that I can't find myself caring about it, but if we're going to do that I think we could go further and just drop explicit support for it per #1183208: Remove variable_get('clean_url') and switch to index.php/path pattern for dirty URL support. Then all those sites that now have the wonders of mod_rewrite or the equivalent we could also redirect the old URLs for in a commented section of .htaccess.
I'm short of time at the moment but I'll try to take a look at the Kernel branch (or an updated patch here if it arrives).
Comment #19neclimdul CreditAttribution: neclimdul commented
not to mention those "oh gah clean urls decided not to work! Let me turn them off real quick and keep the site running while I fix it" moments.
Comment #21effulgentsia CreditAttribution: effulgentsia commented
See also #1474018: Remove all traces of old-style dirty URLs, $_GET['q'], and 'clean_url' variable. I recommend including that into the scope of this first patch, because if this new kernel is changing our approach to dirty URLs, I think we need to incorporate that all the way, including into our tests.
Comment #22effulgentsia CreditAttribution: effulgentsia commented
Comment #24webchick CreditAttribution: webchick commented
Just a note that #1465584: Review external library dependencies in core got committed, so the next patch doesn't need to include all of the Symfonic bits.
Comment #26.0(not verified) CreditAttribution: commented
Make note about file size and remove placeholder stuff.
Comment #40Damien Tournoud CreditAttribution: Damien Tournoud commented
|Needs work
|» Needs review
FAILED: [[SimpleTest]]: [MySQL] 34,276 pass(es), 1,237 fail(s), and 561 exception(s). View
Add */* as an HTML-type MIME type for the test bot.
Comment #42andypost CreditAttribution: andypost commented
We don't use md5 in core anymore because government standards
There's still some usage in core: password and filter, also some grep'ed within symfony code
Comment #43neclimdul CreditAttribution: neclimdul commented
@andypost the password code is legacy conversion and the filter is just some filler to obfuscate html comments so there's arguably no valid collision. The Symfony stuff is external code that we'll have to review but has no relevance to the fact that that is the reason we don't use md5 which is why I chose it for getting an automated unique value for the key.
I'd like to ask that we table this bike-shedding while there is some deep work being done on this patch and try to focus on what can get us to passing tests. That alone might remove that code and we've got the entire rest of the development cycle to argue the optimization of a hash function.
Comment #52deviantintegral CreditAttribution: deviantintegral commented
|Needs work
|» Needs review
FAILED: [[SimpleTest]]: [MySQL] 34,223 pass(es), 1,486 fail(s), and 645 exception(s). View
Comment #54Niklas Fiekas CreditAttribution: Niklas Fiekas commented
|Needs work
|» Needs review
FAILED: [[SimpleTest]]: [MySQL] 34,565 pass(es), 1,484 fail(s), and 442 exception(s). View
Here's a try to fix custom 403 and 404 pages.
- Symfony seams to expect a
/prefix to paths.
- Reusing the sub request hack from 404 for 403.
- Unseralize menu items, even if access denied.
Comment #55Niklas Fiekas CreditAttribution: Niklas Fiekas commented
|Needs review
|» Needs work
+++ b/core/lib/Drupal/Core/EventSubscriber/HtmlSubscriber.phpundefined
@@ -0,0 +1,185 @@
+ if ($path && $path != $system_path) {
Oh ... I realize that with the prefix, this comparision seams to be flawed.
Comment #56Niklas Fiekas CreditAttribution: Niklas Fiekas commented
|Needs work
|» Needs review
FAILED: [[SimpleTest]]: [MySQL] 34,718 pass(es), 1,305 fail(s), and 379 exception(s). View
Fixing some of the menu callbacks that assume they get the trailing path as another parameter.
Comment #59Niklas Fiekas CreditAttribution: Niklas Fiekas commented
Ah, ok, thank you. I'll ping you.
Comment #61RobLoach CreditAttribution: RobLoach commented
|Needs work
|» Needs review
Comment #62EclipseGc CreditAttribution: EclipseGc commented
He said it would fail, so he didn't even bother, but w/e it's queued.
Comment #65chx CreditAttribution: chx commented
As you are keeping menu_get_item I do not need to comment on this.
Comment #67nielsvm CreditAttribution: nielsvm commented
Comment #68Wim Leers CreditAttribution: Wim Leers commented
Coding style issues
- Very minor: you've used two spaces (instead of one) after every sentence in a multi-sentence comment.
- Very minor: sometimes
@todo, sometimes
if(in_array('text/html', $acceptable_content_types) || in_array('*/*', $acceptable_content_types)) { Missing space after
- unnecessary
\n after
public function onKernelRequestAccessCheck(GetResponseEvent $event) {,
public function onKernelRequestPathResolve(GetResponseEvent $event) {,
public function onView(GetResponseEvent $event) {,
public function execute(FlattenException $exception, Request $request) {,
public function match($pathinfo) {
- "drupal" should be upper case at
+ * Get a drupal menu item.
- missing trailing period at
+ * The path being looked up by
- missing docs at
protected function convertDrupalItem($router_item) {
- why:
- * Implements hook_menu().
+ * Implements hook_menu()
- s/reuqest/request/ at
+ * Returns the current global reuqest object.
- s/extract/determine at
+ * The controller resolver that will extract the controller from a Request.
Code issues
+ return $route;
+ return new Route($router_item['href'], $route);
- file_transfer($uri, $headers);
+ return new StreamedResponse(function() use ($uri) {
+ $scheme = file_uri_scheme($uri);
+ // Transfer file in 1024 byte chunks to save memory usage.
+ if ($scheme && file_stream_wrapper_valid_scheme($scheme) && $fd = fopen($uri, 'rb')) {
+ while (!feof($fd)) {
+ print fread($fd, 1024);
+ }
+ fclose($fd);
+ }
+ }, 200, $headers);
While I do like this usage of closures to simplify code, I think it's inappropriate in this case. Contrib modules also want to handle file downloads sometimes. It'd be nice to be able to do
return new StreamedResponse(some_function) in those cases.
Comment #69cosmicdreams CreditAttribution: cosmicdreams commented
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1463656_69_cleanup.patch. Unable to apply patch. See the log in the details link for more information. View
This patch cleans up most of #68:
- Fixed todos capitialization
- fixed "unnecssary newline in the listed functions
- Capitialized Drupal
- add the missing period at "The path being looked up by"
- fixed spelling error
And I commented out the return $route from UrlMatcher.php's converDrupalItem since it seems to be the intent of this function to return a proper Route object.
Comment #70Niklas Fiekas CreditAttribution: Niklas Fiekas commented
Yay, I am not the only one. I posted a patch, too, but WSCCI doesn't use the normal patch workflow.
Niklas, please do not post patches here directly as they cannot be merged with the ongoing work in the kernel sandbox. If you want to help out with the code, please grab me in IRC. I'm usually in #drupal-wscci any time I'm available.
Comment #71pounard CreditAttribution: pounard commented
- unnecessary \n after public function onKernelRequestAccessCheck(GetResponseEvent $event) {, public function onKernelRequestPathResolve(GetResponseEvent $event) {, public function onView(GetResponseEvent $event) {, public function execute(FlattenException $exception, Request $request) {, public function match($pathinfo) {
Symfony does put the opening bracket the next line, while Drupal standard is to write it on the same line as the method signature. In that particular context, adding a new line between the method signature and body actually highlight the first line if it's important, and make the code more "airy" and readable.
I'd keep those new lines myself, they make a lot of sense, they semantically give more importance to the line right under them by making it more readable.
Comment #72Wim Leers CreditAttribution: Wim Leers commented
That's fine, if we do that everywhere. Right now, it looks very inconsistent. At least to my eyes — maybe it's just me…
Comment #74pounard CreditAttribution: pounard commented
Basically it does not break a standard to put a single new line to do a proper separation of code blocks, even if the code block is a one liner, right?
Comment #75effulgentsia CreditAttribution: effulgentsia commented
In http://groups.drupal.org/node/223749, Crell wrote:
If debugging tests is not your strong suit, the patch itself still needs reviews! There's a lot going on, and we need architectural feedback sooner rather than later. Don't wait for the patch to pass, because by then it will be too late!
FWIW, I think #64 is solid architecturally. I haven't reviewed the latest work on the main branch yet, and I will review in more detail once all tests are passing, but assuming there aren't major architectural changes between #64 and the patch where tests start passing, I foresee my feedback being more minor polish oriented and not any architectural pushback.
This issue is about laying the foundation of using Symfony's HttpKernel pipeline instead of menu_execute_active_handler(), but from what I can tell, everything essential about what menu_execute_active_handler() does is still invoked, just from the event subscribers. I think this is exactly the right approach to take for this initial patch, and we'll have (I imagine lots of) follow-up issues for discussing both controversial and non-controversial ways to improve upon bootstrap, routing, etc. And that's not to minimize the accomplishments so far: it's not easy figuring out how to mesh Drupal and Symfony together to get the benefits of both, and this patch is an awesome step in that direction.
The issue summary says:
For right now, the only API changes are the need to explicitly declare load functions in menu router items, and page arguments need to be an associative array.
Is that still true? From what I see in #64, there's BC in place for this too.
Comment #75.0effulgentsia CreditAttribution: effulgentsia commented
Fix typo; HttpKernel is not a dependency for HttpKernel - HttpFoundation is ;)
Comment #77RobLoach CreditAttribution: RobLoach commented
|Needs work
|» Needs review
FAILED: [[SimpleTest]]: [MySQL] 34,521 pass(es), 523 fail(s), and 150 exception(s). View
I just wanna see how we're lookin', test-wise.
Comment #80JohnAlbin CreditAttribution: JohnAlbin commented
The in-PHP implementation of Varnish is actually a major, very-significant feature that we DO want to leverage, as that is a large part of what allows us to build an ESI-centric, everything-is-REST, every-block-is-its-own-path system.
The ESI part of this is critical to making Drupal a fantastic mobile solution provider. I just finished watching @fabpot's Drupalcon Denver session and I'm a convert. Being able to conditionally include chunks of the page depending on device capabilities is a corner stone of Luke Wroblewski's RESS idea (Responsive Design + Server Side components).
Comment #82cosmicdreams CreditAttribution: cosmicdreams commented
It might be a good idea to note in the summary of this issue that getting #1183208: Remove variable_get('clean_url') and switch to index.php/path pattern for dirty URL support committed dramatically improves this issue's ability to pass tests. No?
Comment #83cosmicdreams CreditAttribution: cosmicdreams commented
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1463656_83_gutcheck.patch. Unable to apply patch. See the log in the details link for more information. View
Here's a combination of the current kernel branch (from my branch) and the patch from #1183208: Remove variable_get('clean_url') and switch to index.php/path pattern for dirty URL support. Just want to see how it performs.
One thing that stands our for me before testing this is that there seems to be an incomplete effort to convert every use of $_GET['q'] to current_path(). When I was making all of the test pass in #1545672: Fix kernel errors related to "Theme initialization in hook_init()" I had to convert two additional instances of $_GET['q'] to current_path() in order to get everything green.
Those exceptions will likely show up again in testing this patch.
Comment #94sun CreditAttribution: sun commented
@cosmicdreams: Can you use #1486960: Testing testbot for testing this patch, please? I'm going to unpublish your comments on this issue tomorrow, if no one beats me to it. Thanks.
Comment #97cosmicdreams CreditAttribution: cosmicdreams commented
Is both an option?
1. we keep the request_path() function for module developers and for Drupal 8 inform them that the method is depreciated an will go away (or be repurposed) in Drupal 9. request_path could be implemented as an extension to Symfony's request object. http://en.wikipedia.org/wiki/Decorator_pattern
2. Create some new function (or use Symfony's request based syntax directly) that implements Symfony's request handling faithfully and use that throughout core.
or however you want to name things is fine.
Comment #98webchick CreditAttribution: webchick commented
Summary of IRC feedback:
I'm not convinced we'll get D8 far enough along to get Symfony developers to convert to Drupal en masse (or even at all), so my preference is to cater to the 800K users we already have first. That means #2. Especially since our community has a LONG history of not understanding wtf to do with user input.
Another issue is that page callbacks, in theory, are supposed to be re-usable as API functions. Case in point: the taxonomy_autocomplete changes from #1558346: Fix TaxonomyTermTestCase:
@@ -110,6 +110,7 @@ function taxonomy_autocomplete($field_name, $tags_typed = '') {
// Shift off the $field_name argument.
$tags_typed = implode('/', $args);
+ $tags_typed = urldecode($tags_typed);
// Make sure the field exists and is a taxonomy field.
if (!($field = field_info_field($field_name)) || $field['type'] !== 'taxonomy_term_reference') {
I should be able to call
taxonomy_autocomplete('field_tags', 'monkey, banana, dishwasher') and get back JSON. With this change, I'd now need to call this function like
taxonomy_autocomplete('field_tags', urlencode('monkey, banana, dishwasher')) instead which is a) totally non-intuitive, and b) way more tightly couples page callbacks to being to the router system. If we're going to do that, why not just switch to encouraging arg() use everywhere again?
Comment #101webchick CreditAttribution: webchick commented
Yeah, I can see that argument. I'm still not sure it's worth the pain of passing this responsibility to the module developers to take care of, though.
Also on that IRC discussion was mention of some kind of wrapper thingy thing that was already doing things like mapping variables in paths to the correct places and it was suggested that if we have such a "magic" layer, it seems like urldecode() could be something it does, too.
It was also suggested that we could file a feature request upstream with Symfony to handle urldecode() natively, thus removing our need to do special Drupalisms.
Comment #102sun CreditAttribution: sun commented
@Niklas Fiekas digged out an old message from the sf1 mailing list http://markmail.org/message/7urcbz4nl3ypx6e7 that describes well that PHP developers working with any kind of web application expect path components to be
urldecode()d already, which IMHO is still valid and you can feel how much pain it caused Tom Boutell.
Second, the example is incomplete,
$field_name is equally not decoded. AFAICS, arguments would only be decoded if they've been handled by an argument loader (not sure what the new name is), and that is, only because
urldecode() has been performed there already. This makes it very inconsistent for module authors, because some stuff is already decoded, some other is not, so prone to errors.
I think we should decode path arguments by default.
[And third, all of the three affected examples should actually use
%menu_tail. Only
search_view() is using it currently. If there wasn't also
$field_name, then
menu_tail_load() would be the loader to perform the
PS: I'd recommend to use a separate issue for detailed questions in the future (we're already at #100 ;)).
Comment #104catch CreditAttribution: catch commented
Separate issues can also be posted in the core queue.
Comment #104.0catch CreditAttribution: catch commented
Update remaining test link again.
Comment #106catch CreditAttribution: catch commented
It appears that there's also some performance regressions, but I suspect that will be resolved by pre-caching the event dispatcher and the like in later work.
I'd like to see performance data and a specific issue opened to tackle this if there's a problem before this goes in. I'm not opposed to committing known performance regressions, as long as we clearly document what they are and that there's some kind of plan to fix them.
Will try to take another look through the latest patch asap.
Comment #108andypost CreditAttribution: andypost commented
+++ b/core/modules/system/tests/xmlrpc.test
@@ -99,7 +103,8 @@ class XMLRPCValidator1IncTestCase extends WebTestBase {
- $xml_url = url(NULL, array('absolute' => TRUE)) . 'core/xmlrpc.php';
+ global $base_url;
+ $xml_url = $base_url . '/core/xmlrpc.php';
Looks like core now has no ability to detect running from subfolder.
And how url() could work without clean_url variable with now index.php/path
Comment #109cosmicdreams CreditAttribution: cosmicdreams commented
There are lots of follow-up issues to be created, which I will be doing shortly.
Let's put these in the summary. Can this be one? #1577902: [meta] Remove all drupal_static()s + drupal_static() itself
Comment #110sun CreditAttribution: sun commented
The potential performance issues have to be checked in detail upfront. Someone mentioned a 10 minute increase of total time for the full core test suite, which would mean a 33% performance decrease, which obviously wouldn't be acceptable. (I did not verify this.)
Two larger sub-issues remain to be resolved though:
#1536844: Port language/path handling to the kernel model
#1567428: Active trail tests
Additionally, we should remove all unnecessary changes from this branch/patch. (#108 mentions one of them.) If no one beats me to it, I'm going to do that later this week.
Comment #113Niklas Fiekas CreditAttribution: Niklas Fiekas commented
The changes in #108 are absolutely nescessary, because xmlrpc.php is it's own front controller. While I changed that, I worked in a subdirectory, so that should be fine. (Without the changes url() would prefix /index.php/ to core/xmlrpc.php on some setups.)
Comment #114effulgentsia CreditAttribution: effulgentsia commented
Comment #116cosmicdreams CreditAttribution: cosmicdreams commented
|Needs work
|» Needs review
Comment #118effulgentsia CreditAttribution: effulgentsia commented
|Needs work
|» Needs review
PASSED: [[SimpleTest]]: [MySQL] 36,610 pass(es). View
+++ b/core/modules/system/system.test
@@ -821,7 +821,7 @@ class CronRunTestCase extends WebTestBase {
// Run cron anonymously with the valid cron key.
$key = variable_get('cron_key', 'drupal');
$this->drupalGet('cron/' . $key);
- $this->assertResponse(200);
+ $this->assertResponse(204);
This patch just rerolls for cron_key moving from variable_get() to config().
Comment #119tstoeckler CreditAttribution: tstoeckler commented
|Needs review
|» Needs work
+++ b/core/modules/system/tests/modules/error_test/error_test.info
@@ -3,4 +3,4 @@ description = "Support module for error and exception testing."
-hidden = TRUE
+hidden = FALSE
This seems to be an unintended change.
Comment #121effulgentsia CreditAttribution: effulgentsia commented
|Needs work
|» Needs review
+++ b/.htaccess
@@ -108,7 +108,7 @@ DirectoryIndex index.php index.html index.htm
- RewriteRule ^ index.php [L]
+ RewriteRule ^(.*)$ index.php [L]
Is this necessary? If so, why? Tests pass without it.
+++ b/core/includes/common.inc
@@ -705,7 +707,11 @@ function drupal_site_offline() {
function drupal_not_found() {
- drupal_deliver_page(MENU_NOT_FOUND);
+ throw new NotFoundHttpException();
+ // @todo Remove this line.
+ //drupal_deliver_page(MENU_NOT_FOUND);
@@ -718,7 +724,11 @@ function drupal_not_found() {
@@ -718,7 +724,11 @@ function drupal_not_found() {
* drupal_access_denied().
function drupal_access_denied() {
- drupal_deliver_page(MENU_ACCESS_DENIED);
+ throw new AccessDeniedHttpException();
+ // @todo Remove this line.
+ //drupal_deliver_page(MENU_ACCESS_DENIED);
Let's remove these 2 lines then, and have 2 less todos.
+++ b/core/includes/common.inc
@@ -5230,7 +5258,7 @@ function _drupal_bootstrap_full() {
-function drupal_page_set_cache() {
+function drupal_page_set_cache($response_body) {
This new parameter is passed by RequestCloseSubscriber::onTerminate(), but is not used in the function body or documented in the function docblock.
+++ b/core/includes/file.inc
@@ -2046,18 +2049,27 @@ function file_download() {
- file_transfer($uri, $headers);
+ return new StreamedResponse(function() use ($uri) {
+ $scheme = file_uri_scheme($uri);
+ // Transfer file in 1024 byte chunks to save memory usage.
+ if ($scheme && file_stream_wrapper_valid_scheme($scheme) && $fd = fopen($uri, 'rb')) {
+ while (!feof($fd)) {
+ print fread($fd, 1024);
+ }
+ fclose($fd);
+ }
+ }, 200, $headers);
Similar but different code also appears in this patch's changes to update_test.module. Any way to create a helper function to serve both? Also, the legacy file_transfer() function is still called by image_style_deliver(), so this helper function would serve 3 use-cases in core alone.
+++ b/core/includes/file.inc
@@ -2046,18 +2049,27 @@ function file_download() {
- return drupal_not_found();
+ throw new NotFoundHttpException();
Elsewhere in this patch, code is changed from
return MENU_NOT_FOUND to
drupal_not_found(). I think it would be good to be consistent throughout core as to whether menu callbacks should call drupal_not_found() or throw an exception directly.
+++ b/core/includes/install.core.inc
@@ -241,6 +244,14 @@ function install_begin_request(&$install_state) {
+ // Set the global $request object. This is a temporary measure to
+ // keep legacy utility functions working. It should be moved to a dependency
+ // injection container at some point.
Add a "@todo" to the comment so it can be found by people searching for those.
+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -524,15 +524,14 @@ abstract class Connection extends PDO {
- // Add additional debug information.
- if ($query instanceof DatabaseStatementInterface) {
- $e->query_string = $stmt->getQueryString();
- }
- else {
- $e->query_string = $query;
- }
- $e->args = $args;
- throw $e;
+ // Wrap the exception in another exception, because PHP does not allow
+ // overriding Exception::getMessage(). Its message is the extra database
+ // debug information.
+ $query_string = ($query instanceof DatabaseStatementInterface) ? $stmt->getQueryString() : $query;
+ $message = $e->getMessage() . ": " . $query_string . "; " . print_r($args, TRUE);
+ $exception = new DatabaseExceptionWrapper($message, 0, $e);
+ throw $exception;
This looks like good stuff. But how is it related to this issue?
+++ b/core/modules/system/system.module
@@ -1114,11 +1116,13 @@ function system_menu() {
- // Returning nothing causes no output to be generated.
+ // HTTP 204 is "No content", meaning "I did what you asked and we're done."
+ return new Response('a', 204);
What's the 'a' for? If needed, please add a comment explaining it.
+++ b/core/modules/system/system.module
@@ -2008,8 +2012,11 @@ function system_add_module_assets() {
- if (user_access('view the administration theme') && path_is_admin(current_path())) {
- return variable_get('admin_theme');
+ if ($request = request()) {
+ $path = $request->attributes->get('system_path');
+ if (user_access('view the administration theme') && path_is_admin($path)) {
+ return variable_get('admin_theme');
+ }
This seems like the only place in this patch where module code is converted from current_path() to $request->attributes->get('system_path'). Is that intended as a demonstration, or should we remove that and defer to the follow-up issue that removes current_path() everywhere?
+++ b/core/modules/system/tests/xmlrpc.test
@@ -99,7 +103,8 @@ class XMLRPCValidator1IncTestCase extends WebTestBase {
- $xml_url = url(NULL, array('absolute' => TRUE)) . 'core/xmlrpc.php';
+ global $base_url;
+ $xml_url = $base_url . '/core/xmlrpc.php';
Here and the other identical code in this file: I agree that this is a necessary bug fix, but it's unrelated to this patch. I think it should be moved from here to a separate issue.
+++ b/core/update.php
@@ -391,11 +394,24 @@ $default = language_default();
+// There can be conflicting 'op' parameters because both update and batch use
+// this parameter name. We need the 'op' coming from a POST request to trump
+// that coming from a GET request.
+$op = $request->request->get('op');
+if (is_null($op)) {
+ $op = $request->query->get('op');
Does $request->request not handle POST trumping GET? Why do we also need to check $request->query?
Comment #123Niklas Fiekas CreditAttribution: Niklas Fiekas commented
- Use '' rather than 'a' for HTTP 204 response body.
- Remove $response_body parameter.
- Remove commented out code.
to the clean-up branch.
Replacing all instances of drupal_not_found() and drupal_access_denied() with directly throwing the exception (as a first step before doing it even better) would add another 20k to the diff. We should probably defer that to a follow up.
Comment #126fgm CreditAttribution: fgm commented
Some seemingly redundant code:
DrupalKernel::__construct() includes this:
parent::__construct($dispatcher, $resolver);
$this->dispatcher = $dispatcher;
$this->resolver = $resolver;
but these two assignments are precisely what the parent constructor already performs. Same duplication in
Also, not quite sure, but I think
RequestCloseSubscriber::onTerminate() calls
ob_flush() in all situations, although OB is only started in
_drupal_bootstrap_page_header() if
Comment #127pounard CreditAttribution: pounard commented
If I remember well, the dispatcher in the parent class is private, which makes us do this kind of hideous trick. It might have changed last time I checked was 2.0 not 2.1 They changed it, then I guess you're right; we should remove those two unnecessary lines.
Comment #128Niklas Fiekas CreditAttribution: Niklas Fiekas commented
Thanks for reviewing, fgm. That's true for HttpKernel. Pushed that to the clean-up branch. RouterListener, however, uses private members (even on Symfony HEAD), so we can't do that there. (
Not sure if it makes sense, but if it does, maybe we should file an issue in the Symfony tracker. Going to do that and link it here, just to get comments from Symfony people. Awaiting comments on https://github.com/symfony/symfony/pull/4351.)
Guess you're right about the ob_flush(). Filed #1591666: ob_flush() in RequestCloseSubscriber::onTerminate() not correct when drupal_is_cli() to keep track of that.
Comment #129pounard CreditAttribution: pounard commented
I read the code bundled in my vendor/ directory and it's not private, it was in 2.0, it would be weird that they'll switch to protected then back to private again?
Comment #130pounard CreditAttribution: pounard commented
Comment #131Niklas Fiekas CreditAttribution: Niklas Fiekas commented
Yes, totally valid for HttpKernel. Just not RouterListener. See #128, where I also linked to Symfony HEAD and http://api.drupal.org/api/drupal/core%21vendor%21Symfony%21Component%21H....
Comment #134pounard CreditAttribution: pounard commented
The unconditionnal ob_flush() allows any buffered output to be released and sent to the browser, then, the hook_exit() and other userland runtime shutdown tasks can run without keeping the connection opened (I guess) this would make sense, but I don't know if PHP actually releases the frontend connection when doing that (I'm quite sure it doesn't). Something I'm sure of, is that the shutdown code is run in parallel of the output being sent to the browser since it's not PHP which is actually send the data, but the HTTPd upper, so it's a few millisec saved for the full page rendering. I think it's safe to keep this ob_flush(). Notice that we may call ob_flush_end() instead that will release and close the output buffer stream, and except in case of fatal error while Drupal shutdown handlers are running, this would be safe to do.
Comment #135stovak CreditAttribution: stovak commented
You guys are ROCK STARS. SHIP IT!!!!
Comment #136effulgentsia CreditAttribution: effulgentsia commented
From #110:
Additionally, we should remove all unnecessary changes from this branch/patch. (#108 mentions one of them.) If no one beats me to it, I'm going to do that later this week.
I submitted the following patches to the sandbox queue:
#1593702: Remove @todo comment cleanups from unrelated code
#1593674: Remove unrelated change to xmlrpc.test
#1593428: Remove unnecessary change to system_custom_theme()
#1593710: Revert unnecessary file_download() conversion from drupal_access_denied()/drupal_not_found() to throwing exceptions
Comment #137catch CreditAttribution: catch commented
|Needs review
|» Needs work
fwiw I just did some very boilerplate profiling of the default front page with this patch applied, and I'm not seeing a regression at the moment. There's a few Symfony/Kernel methods which are in the same position of menu_execute_active_handler() of having nearly all code executed under them so they show up high for inclusive time, but nothing I noticed that's actually adding much here, also looking through the patch at least in it's current state, there is not really that much new code getting executed, a lot of it is wrapping existing stuff and the changes to avoid complete breakage.
(we do have performance regressions in core compared to D7 from config() (which is a known issue but I'm surprised how noticeable it is compared to the number of systems converted so far), and as David Rothstein noted on another issue, the class loader is noticeable now given the number of classes we're loading, but there's already issues open to look at both of those and they're not introduced here.)
Few things to ask about:
+++ b/core/lib/Drupal/Core/EventSubscriber/RouterListener.php
+ * Drupal-specific Router listener.
+ *
+ * This is the bridge from the kernel to the UrlMatcher.
+ */
+class RouterListener extends SymfonyRouterListener {
+ protected $urlMatcher;
+ protected $logger;
+ public function __construct(UrlMatcherInterface $urlMatcher, LoggerInterface $logger = null) {
+ parent::__construct($urlMatcher, $logger);
+ $this->urlMatcher = $urlMatcher;
+ $this->logger = $logger;
+ if (null !== $this->logger) {
+ $this->logger->info(sprintf('Matched route "%s" (parameters: %s)', $parameters['_route'], $this->parametersToString($parameters)));
yoda conditions--
there's a couple of other things like 4 spaces which are sneaking into the patch from Symfony's coding style, likely from copy/paste method overrides.
What's going on with the logger stuff here? I'd very much not want to see us using both Symfony's logger and watchdog() at the same time, that's a recipe for confusion, and while watchdog() is a tricky one given the module dependency, I'm not aware of any active efforts to refactor that.
Also the raw sprintf() usage feels icky as well, that's what format_string() is for. While we might need to fix things so format_string() can be nicely used by stuff in core/lib, it should be available here so let's use it for now and that's easier to find later compared to both format_string() and raw sprintf().
+++ b/core/lib/Drupal/Core/EventSubscriber/ViewSubscriber.phpundefined
@@ -0,0 +1,128 @@
+ public function onView(GetResponseEvent $event) {
+ $request = $event->getRequest();
+ $method = 'on' . $this->negotiation->getContentType($request);
+ if (method_exists($this, $method)) {
+ $event->setResponse($this->$method($event));
+ }
+ else {
+ $event->setResponse(new Response('Unsupported Media Type', 415));
+ }
This is the most odd bit architecturally, I realise there's the @todo where it's registered but it'd be good to have the follow-up discussion open to discuss what to do about it. Also even with the current limitation of hard-coded subscribers, why a single subscriber handling four arbitrary content types? There's also a lot of copy/pasting between these methods.
+++ b/core/modules/comment/comment.moduleundefined
@@ -2514,3 +2515,23 @@ function comment_file_download_access($field, $entity_type, $entity) {
+ * Access callback: Determines if comment approval is accessible.
+ *
+ * @param $cid
+ * A comment identifier.
+ *
+ * @see comment_approve()
+ * @see comment_menu()
+ */
+function comment_approve_access($cid) {
+ if (!user_access('administer comments')) {
+ return FALSE;
+ }
+ $token = request()->query->get('token');
+ if (!isset($token) || !drupal_valid_token($token, "comment/$cid/approve")) {
+ return FALSE;
+ }
+ return TRUE;
I don't see this approach working if we ever have a token in a menu link or contextual links contextual links - since the acces callback is also run when checking access to the link itself, not only the page callback. I don't think we actually have any of these in core at the moment but it seems a valid use case to support and this looks like either a new limitation or unnecessary refactoring.
Currently this works for the comment approve links, but only because it's shown in the comment links variable, which doesn't run through that menu check. So I think this needs a bit more thought - ideally we want a pattern that'll work if you add a menu link to a token protected path too (note I didn't check manually verify this is a problem but I don't see how it could work).
There's a lot more nitpicky stuff, but apart from the first one here I'm trying to gloss over that for now.
Comment #138catch CreditAttribution: catch commented
|Needs work
|» Needs review
Sorry dreditor status change, not mine. This definitely needs more reviews.
Comment #139quicksketch CreditAttribution: quicksketch commented
Okay, I read the whole blog series on Symfony of posts and everything, and the thing looks pretty cool.
However this patch seems to be sorely lacking. It adds 1,700 lines of code and tons of calls to methods/classes within Symfony that weren't used before. If this were benchmarked today it seems like it would inevitably be slower (if it's not slower, well then kudos and lets put it in). Can we get it to a point where it is actually making improvements (rather than just additions) *before* committing it?
Comment #140pounard CreditAttribution: pounard commented
yoda conditions--
If yoda condition means using:
if (null === $something) {
Instead of:
if ($something === null) {
Then yoda condition++
That kind of typo errors people often do:
if ($something = null) {
To silent errors this leads, while:
if (null = $something) {
A fatal error this will cause: the typo shall not pass.
There is actually a real technical reason why yoda is right: because it makes typo error fail instead of being silent. Last week I saw a patch reroll on some issue because of such typo error.
Comment #141Niklas Fiekas CreditAttribution: Niklas Fiekas commented
Thanks for reviewing!
@catch: The logger call is because were hacking around a parent Symfony method here, that would call the logger anyway. Here's some discussion to solve that problem upstream:
@quicksketch: Probably (but please do surprise me) not. The patch is already pretty big and hard to review. Once it's in we can work in parallel on lots of follow-ups.
Comment #142catch CreditAttribution: catch commented
@pounard: I know why people like yoda conditions, thanks. If you want to change the coding standards please file an issue.
Comment #143pounard CreditAttribution: pounard commented
Even that is in the standard? I mean I read this page about twice a week, I didn't even saw it!
Comment #145catch CreditAttribution: catch commented
I'm still not convinced by the logger/yoda/sprintf arguments at all, but that feels like it needs a side issue to bikeshed it - is the hope that this code gets deleted if that Symfony pull request goes in?
4 spaces was in the earlier code paste, but here's just the relevant line:
+ if (null !== $this->logger) {
+ $this->logger->info(sprintf('Matched route "%s" (parameters: %s)', $parameters['_route'], $this->parametersToString($parameters)));
Valid point about the menu link issue. Is that a problem in this particular case, meaning we need to convert back, or no?
Well it means you if you ever made a menu link/tab/contextual link to any of those paths, then it'll break - and given we had a patch to make those comment links contextual links for D7 that was only rolled back due to a performance regression, that doesn't seem too unlikely at all tbh. So, I think it'd be better to convert back rather than knowingly introduce a regression.
Comment #146David_Rothstein CreditAttribution: David_Rothstein commented
For the access callbacks, we didn't see a reason why they were done inline rather than in a proper access callback other than likely "legacy reasons"; there was no documentation to the contrary.
Besides the menu link issue, I think the (completely undocumented) reason is that access callback functions are intended to be altered by other modules, and the purpose of that is to modify traditional access control, e.g. permissions. But this is an independent security feature so it shouldn't be alterable.
It makes more sense to travel along with the page callback, because whether or not a security token is needed in the first place is directly tied to the exact actions that the page callback winds up performing.
If keeping it in the page callback isn't an option for some reason, maybe another option would be to mark #755584: Built-in support for csrf tokens in links and menu router as a critical issue? That's likely a better way to handle tokens anyway.
Comment #148effulgentsia CreditAttribution: effulgentsia commented
Moving the token check back to the menu callback is an option for now; effulgentsia volunteered to do so in this morning's WSCCI meeting.
That's ready for review: #1595298: Move CSRF token validation back to page callbacks.
Comment #149catch CreditAttribution: catch commented
Thanks for #1595298: Move CSRF token validation back to page callbacks that looks like the right change to me, we can discuss more automated support for tokens in #755584: Built-in support for csrf tokens in links and menu router.
With the ViewSubscriber extensibility issue, the @todo about making that extensible should move to the specific class I think rather than where it's registered. If it was there, I likely wouldn't have need to ask about it in this issue.
Comment #150lsmith77 CreditAttribution: lsmith77 commented
"drupal_page_footer() is now a kernel "terminate" event. This needs to get cleaned up later."
that sounds wrong to me .. terminate events are for stuff you want to do after the response has been send (f.e. sending an email). adding a footer and stuff like this should be done in a response (or a view) listener.
Comment #151Niklas Fiekas CreditAttribution: Niklas Fiekas commented
@lsmith77: Probably the name "drupal_page_footer()" is a bit confusing. Look what it does: http://api.drupal.org/api/drupal/includes%21common.inc/function/drupal_p.... It doesn't really add stuff to the footer, just stuff to close the request, really.
Comment #152pounard CreditAttribution: pounard commented
It is actually a userpace shutdown handler, the name and all its references should be changed.
Comment #154fgm CreditAttribution: fgm commented
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
Patch in #125 had a conflict with the latest commits. Rerolled without changes except the conflict resolution.
Comment #155webchick CreditAttribution: webchick commented
If you, like me, are not following the WSCCI sandbox and only this issue, there is some interesting stuff happening in #1578090: Benchmark/profile kernel.
Comment #156Niklas Fiekas CreditAttribution: Niklas Fiekas commented
@fgm: Since were not using a patch based workflow for the kernel, it's hard to merge your changes back to the kernel branch. Crell will happily give you commit access to the sandbox (if you promise not to change the main kernel branch). You could then branch, merge 8.x and push the branch with the merge commit that solves the problem.
Looks like the patch size is much smaller. Did you git apply the kernel patch, make your changes and then diff without the added files?
Comment #157fgm CreditAttribution: fgm commented
No, I did as usually, apply the patch on previous version in a feature branch, pull 8.x, and rebase the branch. But that visibly didn't work: the result doesn't even install. So I must have erred somewhere.
Comment #159effulgentsia CreditAttribution: effulgentsia commented
|Needs work
|» Needs review
PASSED: [[SimpleTest]]: [MySQL] 36,660 pass(es). View
#154 contains only modified files. It is missing all the new files. Maybe the patch was created with just a
git diff rather than a
git add; git diff --cached (if there's an easier way to roll patches that include new files, I don't know it).
Here's the same with the new files included too.
The only difference this has relative to #125 is something that's already committed to the sandbox's kernel branch: #1593702: Remove @todo comment cleanups from unrelated code.
Comment #162Dave Reid CreditAttribution: Dave Reid commented
+++ b/core/includes/file.incundefined
@@ -2026,18 +2029,27 @@ function file_download() {
- file_transfer($uri, $headers);
+ return new StreamedResponse(function() use ($uri) {
+ $scheme = file_uri_scheme($uri);
+ // Transfer file in 1024 byte chunks to save memory usage.
+ if ($scheme && file_stream_wrapper_valid_scheme($scheme) && $fd = fopen($uri, 'rb')) {
+ while (!feof($fd)) {
+ print fread($fd, 1024);
+ }
+ fclose($fd);
+ }
+ }, 200, $headers);
This seems really strange to me. Why can't we fix file_transfer() to do this logic rather than using an anonymous function that other modules can't re-use? I get that we want to do #1561362: Change file_transfer() to use BinaryFileResponse as a follow-up, but it seems really, really strange that we can't update file_transfer() to pass tests rather than use anonymous functions which so far have confused multiple people.
Comment #163catch CreditAttribution: catch commented
Also whatever happens to file_transfer() won't be matched by file_download() unless someone remembers.
Comment #164Wim Leers CreditAttribution: Wim Leers commented
I agree with Dave Reid and catch here — I also said this back in #68.
Comment #166Dave Reid CreditAttribution: Dave Reid commented
So maybe what I'm saying is that file_transfer() should instead be using the anonymous function itself so that we can re-use file_transfer() for now?
Comment #167Niklas Fiekas CreditAttribution: Niklas Fiekas commented
Makes sense to me. Opened an issue for that: #1602060: Use file_transfer() as a helper function instead of inlining file transfers. Note that we'd now have to really return the StreamedResponse that file_transfer() creates, so it's a bit of an API change, anyway. Note also that hunks like
+ $file = "$path/$project_name.$availability_scenario.xml";
+ if (!is_file($file)) {
+ // Return an empty response.
+ return new Response('', 200, array('Content-Type' => 'text/xml; charset=utf-8'));
+ }
+ return new StreamedResponse(function() use ($file) {
+ // Transfer file in 1024 byte chunks to save memory usage.
+ if ($fd = fopen($file, 'rb')) {
+ while (!feof($fd)) { ...}
+ }, 200, array('Content-Type' => 'text/xml; charset=utf-8'));
- readfile("$path/$project_name.$availability_scenario.xml");
couldn't use that helper function, because file transfer is only for managed files (???). So doing it might not be worthwhile. But I guess that should be discussed in the issue.
Comment #169webchick CreditAttribution: webchick commented
No, that's not right... file_transfer() is a useful general File API function, called by at least 37 contributed modules. Such as:
Thanks, Gotta Download Them All. :)
Comment #170Dave Reid CreditAttribution: Dave Reid commented
I re-added file_transfer() as a wrapper for the StreamedResponse object and anonymous function until it's replaced/improved by #1561362: Change file_transfer() to use BinaryFileResponse as a follow-up.
Comment #171pounard CreditAttribution: pounard commented
@#169 Removing file transfert from D8 makes sense to me, and kernel is gonna break some deeper API's than that, why the modules using it are blockers?
Comment #172webchick CreditAttribution: webchick commented
A better question is why force people to write 10 lines of code when they could call 1 function?
Comment #173effulgentsia CreditAttribution: effulgentsia commented
The goal in #1561362: Change file_transfer() to use BinaryFileResponse is to enable these modules to achieve what they need in 1 line of code, via a class committed to Symfony. The question is what to do in the meantime: provide a wrapper function that might go away once the Symfony class is available, or make modules tracking D8 HEAD write a 10 line anonymous function until the Symfony class is available. I'd prefer the wrapper function: that's not a critical blocker for me if there's too much resistance to the wrapper function in this initial kernel patch, though I don't really understand why there's that resistance.
Comment #174neclimdul CreditAttribution: neclimdul commented
I don't agree with using usage as a valid judge of a solution in this case. Neither 2 uses in core or a million uses in contrib are an argument for doing the right thing. The question should be which makes more sense as an API and what makes it easy to transfer a file to the browser.
Also, what should we just accept as part of the initial patch? Because its been decided its not acceptable for WSCCI to do all its refactoring outside of core, we're not going to just rip into every subsystem and solve the right solution in the first pass. There are things that are going to be obviously not finished in this pass. This is a big refactor, and we're doing a merge with both the goal to provide as much a final product as possible, disrupting core a little as possible, and getting it done as soon as possible. All these things are at odds and we're going to have to accept some middle ground and paint a path to follow up and finish the job. What to do with file transfer seems like one of those things we get in place and work to get the API more in line after the fact.
Comment #175catch CreditAttribution: catch commented
I agree with #173, I don't at all understand the resistance to a wrapper function either.
Comment #176aspilicious CreditAttribution: aspilicious commented
Adding tags to prevent this from rerolling.
Tests affected:
Comment #177pounard CreditAttribution: pounard commented
It wasn't a resistance on my side, just a honest question, I have a better understanding of everyone's concerns know, thanks.
Comment #178catch CreditAttribution: catch commented
aspilicious, thanks for listing the tests! I'm not sure exactly when I'll have time, but I'll try to get some more movement going on the other PSR-0 conversion issues that don't affect this next time i'm committing stuff.
Comment #180cosmicdreams CreditAttribution: cosmicdreams commented
It's likely that recent work on tests will require a reroll for this issue as those tests have moved within the source tree.
Comment #181catch CreditAttribution: catch commented
I've been avoiding everything listed in #176, so hopefully that's worked. However, I'm going to send the latest patch for a re-test just in case.
Comment #182Tor Arne Thune CreditAttribution: Tor Arne Thune commented
curl http://drupal.org/files/1463656-drupal-kernel_40.patch | git apply
error: core/modules/node/node.test: No such file or directory
Comment #183cosmicdreams CreditAttribution: cosmicdreams commented
@182, yep that's exactly what I mean. These tests have indeed moved.
Comment #184aspilicious CreditAttribution: aspilicious commented
Oh great thats my fault! :( srry
- ob_start();
- node_feed(array(), array('copyright' => 'Drupal is a registered trademark of Dries Buytaert.'));
- $output = ob_get_clean();
- $this->assertTrue(strpos($output, '<copyright>Drupal is a registered trademark of Dries Buytaert.</copyright>') !== FALSE);
+ $response = node_feed(array(), array('copyright' => 'Drupal is a registered trademark of Dries Buytaert.'));
+ $this->assertTrue(strpos($response->getContent(), '<copyright>Drupal is a registered trademark of Dries Buytaert.</copyright>') !== FALSE);
Can be applied manually to "NodeFeedTestCase".
Comment #185RobLoach CreditAttribution: RobLoach commented
|Reviewed & tested by the community
|» Needs review
PASSED: [[SimpleTest]]: [MySQL] 36,697 pass(es). View
I understand uploading patches for this is the wrong way, but I was getting fatal errors when rebasing between Drupal and WSCCI's remote git. Updating the version of git running on Drupal.org would fix the problem.
In the mean time, here's the updated patch with the correct paths.
Comment #186Dave Reid CreditAttribution: Dave Reid commented
|Needs review
|» Reviewed & tested by the community
PASSED: [[SimpleTest]]: [MySQL] 36,680 pass(es). View
I believe this should be re-rolled for 8.x.
Comment #187Dave Reid CreditAttribution: Dave Reid commented
|Reviewed & tested by the community
|» Needs review
PASSED: [[SimpleTest]]: [MySQL] 36,681 pass(es). View
Yeah, Rob had it right, I was missing a commit from kernel branch. This should be the same as #185.
Comment #188RobLoach CreditAttribution: RobLoach commented
|Needs review
|» Reviewed & tested by the community
Back to RTBC!
Comment #189moshe weitzman CreditAttribution: moshe weitzman commented
|Reviewed & tested by the community
|» Needs work
I took a few runs through this in the debugger. It looks fine as far as it goes. But IMO we need some more solid discussion around routes before it gets committed. Specifically, I'm looking for a major follow-up issue with a plan in it that discusses:
- How we are going to provide multiple routes per path (see matchDrupalItem method).
- #1599108: Allow modules to register services and subscriber services (events) needs a plan.
- Related to above, we really ought to have a ShinyControllerSubscriber and a route that uses it. We know we are starting from LegacyControllerSubscriber, but I think it is vital to know where we are going.
Changing to needs work but only for follow-ups to be created. Keep calm and carry on (and keep up the Issue Summary for goodness sake).
Comment #191effulgentsia CreditAttribution: effulgentsia commented
Comment #192pwolanin CreditAttribution: pwolanin commented
The summary has:
Remaining tasks For this patch:
which imply that the patch is still rather broken? Either that or the summary's not up to date.
Comment #193effulgentsia CreditAttribution: effulgentsia commented
Working on updating the summary now. If someone else has already started that, ping me in IRC so we avoid conflicts.
Comment #193.0effulgentsia CreditAttribution: effulgentsia commented
Fix link to follow-up issues.
Comment #194pwolanin CreditAttribution: pwolanin commented
to avoid introducing an inconsistency in 8 compared to 7 likely to lead to security issues:
- return MENU_NOT_FOUND;
+ drupal_not_found();
should probably be:
- return MENU_NOT_FOUND;
+ return drupal_not_found();
Comment #195Niklas Fiekas CreditAttribution: Niklas Fiekas commented
Good point. There is a follow-up to remove drupal_not_found() and drupal_access_denied() entirely. Not worth holding/updating the patch.
Comment #196effulgentsia CreditAttribution: effulgentsia commented
|Needs work
|» Reviewed & tested by the community
Fixed the summary. I think #194 is only relevant if we don't do #1587850: Replace drupal_not_found() with throw NotFoundHttpException and #1591604: Replace drupal_access_denied() with throw AccessDeniedHttpException, so let's defer that security/DX question to those issues.
Comment #197quicksketch CreditAttribution: quicksketch commented
And just one last wimper before this steamrolls in anyway. Our current analysis over at #1578090: Benchmark/profile kernel says that this patch makes Drupal bootstrap 10% slower and require 20% more memory. Let's hope we get some real returns on this investment. I still find it a bit appalling that most of the performance improvements are theoretical, but if we're certain that we'll be able to get this back through ESI/block caching/PSR-0 let's move forward (and quickly). If D8 gets any slower I'm going to go bananas.
Comment #198Dries CreditAttribution: Dries commented
I reviewed this patch and I think we should proceed with it. Obviously, this patch requires a ton of follow-up work. From reworking most of the Resolvers, to reworking the bootstrap to replace Drupal's page caching mechanism, to figuring out how to adopt Symfony's routing system. It's a lot of work that needs to be done in the next few months. There is risk in not getting it done, but delaying this patch only increases that risk. There is risk in not being able to address the performance regression, but delaying this patch only increases that risk.
Larry, I think we should plan to commit this patch on Friday between 11am and 1pm Eastern time. Would you be available to help with the merge around that time?
Comment #199catch CreditAttribution: catch commented
@quicksketch, while it hasn't been discussed properly yet, I'd personally consider a measurable performance regression from 7.x to 8.x a critical bug and would be very happy to hold the release up on it, especially considering the legion of regressions that got into Drupal 7, many of which haven't been resolved yet.
On the other hand, I'd much rather a situation where we have a reasonable understanding of what the performance issue is due to #1578090: Benchmark/profile kernel than a surprise regressions weeks or months later because no-one profiled it or did a too-superficial benchmark to find it.
Once this patch is committed we have three known regressions, each of which is well documented - the Symfony autoloader (at least in the configuration we're using), CMI having no static or persistent caching at all, and the stuff added in this patch (mainly the event dispatcher and/or the listeners). From that we can work on both trying to optimize the straight regressions in PHP from the new code added, as well as on the macro performance/scalability work which depends on much heavier refactoring elsewhere.
We should also bear in mind that (like Field API in D7), with the current state of things, some further refactoring to rely more on these systems is likely to regress performance further, for example the url() conversion proposed in #1606794: Implement new routing system rings alarm bells (although I'd love to be pleasantly surprised).
Comment #201Dries CreditAttribution: Dries commented
Crell: let's plan on 11am then. I'll ping you on Skype.
Comment #202Dries CreditAttribution: Dries commented
|Reviewed & tested by the community
|» Fixed
Merged and committed to 8.x.
Thanks to Larry and all the other people who helped.
We have lots of work to do now, but this merge marks the beginning of a new era for Drupal.
Commands used:
$ git remote add wscci http://git.drupal.org/sandbox/Crell/1260830.git
$ git fetch --all
$ git merge --no-ff wscci/kernel
$ git status
# On branch 8.x
# Your branch is ahead of 'origin/8.x' by 215 commits.
nothing to commit (working directory clean)
$ git push
Comment #203effulgentsia CreditAttribution: effulgentsia commented
We have lots of work to do now
Just a reminder, for anyone who wants to help, there's already issues tagged with kernel-followup. If you're inspired by any of them, dig in. Also, if you identify follow-up work that's needed for which there isn't an issue in that queue, please add it. Even if you don't have time to work on it, please make sure the issue is there, so that others who want to help can pick something up. Thanks!
Comment #204Fabianx CreditAttribution: Fabianx commented
Nice :-)
Comment #205sun CreditAttribution: sun commented
|Add a Drupal kernel; leverage HttpFoundation and HttpKernel
|» Change notification: Add a Drupal kernel; leverage HttpFoundation and HttpKernel
|» Unassigned
|» bug
|» Critical
|» Needs work
|Issue tags:
|+Needs change record
- This needs a full stack of change notifications, so bumping to critical.
Note: I had a pretty hard time to figure out what actual changes are required, and since this was merged into core, it wasn't trivial to see all related core changes as with regular patches, so the combined source of changes is the latest patch on this issue. The merge commit also did not state this issue number, so people who did not participate in here at all will have a very hard time to figure out what has been changed and what needs to be changed.
FWIW, this commit shows what changes I had to apply to admin_menu - though bear in mind that I'm not sure whether those are 100% correct — that's why we need change notices here.
- Why are the new DrupalKernel/HttpKernel related files directly in
lib/Drupal/Core/without any component?
That looks very bogus to me and contradicts the entire debate on the PSR-0 structure. When I first looked, I searched for a Drupal\Core\Kernel component, but only found EventSubscriber, which obviously delivers an incomplete picture. Only after grepping the code base I found those loose and lost files directly in Drupal\Core. Very confusing.
+++ b/core/lib/Drupal/Core/ContentNegotiation.php @@ -0,0 +1,54 @@ +class ContentNegotiation { ... + public function getContentType(Request $request) { ... + // AJAX calls need to be run through ajax rendering functions + elseif ($request->isXmlHttpRequest()) { + return 'ajax'; + }
The assumption that any XMLHttpRequest should get an AJAX response is fundamentally wrong.
This broke the most basic AHAH JavaScript functionality; i.e., a simple
$.get()to any Drupal path (say,
/js/admin_menu/cache), which is simply supposed to run a HTTP request against the provided path and return the (HTML) response (with no AJAX framework being involved). Due to the enforced
X-Requested-Withshortcut in ContentNegotiation::getContentType(), that is impossible.
I was able to work around that by (IMO) preemptively returning a Response object from the page callback. But that looks a bit hacky to me.
+++ b/core/lib/Drupal/Core/EventSubscriber/ViewSubscriber.php @@ -0,0 +1,128 @@ +class ViewSubscriber implements EventSubscriberInterface { ... + public function onHtml(GetResponseEvent $event) { + $page_callback_result = $event->getControllerResult(); + return new Response(drupal_render_page($page_callback_result)); + }
Closely related to the above AHAH issue - the onHtml() view event subscriber processes any kind of render array or HTML string response into a full HTML page.
How do you envision HTML fragments to be returned? (i.e., not full HTML pages)
Comment #206aspilicious CreditAttribution: aspilicious commented
Sun well your commit in fact is incorrect now.
I made a change notice for the acces_denied() stuff.
Feel free to adjust it if it's incorrect.
Comment #207David_Rothstein CreditAttribution: David_Rothstein commented
|Change notification: Add a Drupal kernel; leverage HttpFoundation and HttpKernel
|» Change notification and CHANGELOG.txt entry: Add a Drupal kernel; leverage HttpFoundation and HttpKernel
Pretty sure this issue needs a CHANGELOG.txt entry also.
- if (!isset($_GET['token']) || !drupal_valid_token($_GET['token'], 'aggregator/update/' . $feed->fid)) {
+ $token = request()->query->get('token');
+ if (!isset($token) || !drupal_valid_token($token, 'aggregator/update/' . $feed->fid)) {
Does this kind of thing need a change notification too, or will it be handled in another issue? (The patch only converted a couple places; most of core is still using $_GET, $_REQUEST, etc., but maybe the change notification should happen now anyway.)
Our current analysis over at #1578090: Benchmark/profile kernel branch says that this patch makes Drupal bootstrap 10% slower and require 20% more memory. Let's hope we get some real returns on this investment. I still find it a bit appalling that most of the performance improvements are theoretical, but if we're certain that we'll be able to get this back through ESI/block caching/PSR-0 let's move forward (and quickly). If D8 gets any slower I'm going to go bananas.
@quicksketch, while it hasn't been discussed properly yet, I'd personally consider a measurable performance regression from 7.x to 8.x a critical bug and would be very happy to hold the release up on it
So should #1578090: Benchmark/profile kernel be moved to the core issue queue and marked critical, or critical + postponed + "revisit before release" or whatever the magic incantation is these days? :)
Comment #209sun CreditAttribution: sun commented
Comment #211catch CreditAttribution: catch commented
|» task
|Needs work
|» Active
Let's open follow-ups for anything else that's not the change notifications, moving to active/task.
Comment #215Berdir CreditAttribution: Berdir commented
|Needs review
|» Needs work
+++ b/core/CHANGELOG.txtundefined
@@ -1,6 +1,7 @@
+- Replaced the core routing system with one build on the Symfony2 framework.
Comment #216Berdir CreditAttribution: Berdir commented
Also, I think it would be useful to have the current example of how to replace menu_execute_active_handler() in there. That is easier to keep up to date and less easily forgotten. Otherwise, we would basically need to keep a revisit before release task open for adding the final syntax. When keeping it up to date, it's part of whatever issue is going to change it.
Comment #218Berdir CreditAttribution: Berdir commented
I don't see a change record for the request/response objects, do we want to document them in the same one? I think it would make sense, because we are using them in the example code, without explaining them first. Also, I think there isn't one yet for the whole listener thing, right? That could probably go into a separate one?
The "not yet documented" sentence is also still there, I guess that can be removed. Not trying to be picky about the new code example, as it's very likely going to be changed but maybe add an inline comment or two and use some temporary variables to make the lines shorter? There are also some variables in there that aren't explained or defined, like $system_path (I guess that's current_path() or whatever the corresponding method of $request is?)
Comment #220webchick CreditAttribution: webchick commented
|Needs review
|» Fixed
Committed and pushed to 8.x. Thanks!
Comment #221webchick CreditAttribution: webchick commented
|Change notification and CHANGELOG.txt entry: Add a Drupal kernel; leverage HttpFoundation and HttpKernel
|» Add a Drupal kernel; leverage HttpFoundation and HttpKernel
Fixing the title.
Comment #222Berdir CreditAttribution: Berdir commented
I do not want to discuss the code syntax (or even architecture) here, sorry if I was unclear.
As far as I understand change records is that they should be complete in the way that every change is documented, as short as possible, with *understandable* before/after code samples (if applicable). Have a look at http://drupal.org/node/1400186 (entity classes, which evolved over a dozen issues or so) or http://drupal.org/node/1534648 (cache tag api). And the current change record in my opinion is just plain simply not understandable to someone that is new to the API (which is the target audience of change records).
To quote sun from #205: "This needs a full stack of change notifications, so bumping to critical.". Also David Rothstein in #207.
The new Request class will, AFAIK, affect *everyone*, becausing that will be used instead of _GET/_POST/..., right? That's a huge change and it's currently not mentioned at all (yes, it's *used* in the code example there, but that makes it IMHO worse, not better ;)) . Compared to that, the removal of meah() is completely irrelevant. Am I missing something?
That's all I'm trying to say. I'm just confused to see this waved through the (IMHO important) change record/CHANGELOG process, compared to most other issues, even though this is one of the biggest changes so far in 8.x.
Comment #223webchick CreditAttribution: webchick commented
|Add a Drupal kernel; leverage HttpFoundation and HttpKernel
|» Needs change notice: Add a Drupal kernel; leverage HttpFoundation and HttpKernel
|» Needs work
That's a good point. Marking needs work for the change notice.
Comment #225Mark Trapp CreditAttribution: Mark Trapp commented
From the change notice:
Additional implications are noted in separate change notices.
Can links to these other change notices be provided? I'm not sure how people should know which change notices are supposed to be relevant.
Comment #227Mark Trapp CreditAttribution: Mark Trapp commented
Crell: I'm sorry, I guess I wasn't clear. I'm not suggesting every single related change notice be cross-linked to each other, just the ones being referred to in that line.
That is, when I read:
Additional implications are noted in separate change notices.
as part of the block regarding
menu_execute_active_handler()'s removal, it suggests to me that there are specific implications related to replacing
menu_execute_active_handler() that are captured in this change that I should be aware of, but for whatever reason, are mentioned in other specific change notices.
Having not been involved in the development of this change, I have no idea what those specific implications are nor which specific change notices are being alluded to. I can't fix or update the change notice because I don't know what the implications or change notices are to which this notice refers. I could read every single change notice, but again, having not been intimately involved in the development of the kernel, I don't know what notices contain the implications alluded by this change notice.
That is, a red flag has been raised, but I don't have the necessary information to resolve it. If there were specific change notices in mind when that line was written, I don't think it's unreasonable to expect those to be linked, similar to how the request object documentation was linked below the first code block couplet.
However, if I understand you correctly, it sounds as though that perhaps there aren't specific implications that people need to be aware of, but merely general implications of replacing a massive part of Drupal's internals in the same way that upgrading to a new major version of Drupal always creates implications for development or upgrading. If that's the case, the line seems superfluous at best and confusing at worst.
I'm a non-core developer, and up to this point, I thought I was part of the intended audience for these change notices: to be able to get an overview of a change and its implications in one place without having to have intimate knowledge of every single thing that was said or written about the change while it was in development. If i'm wrong, I'll shut up and you can ignore this.
Comment #229Berdir CreditAttribution: Berdir commented
I think one big thing that is still missing are the response objects, not sure if that should be in the same or a new change record. Also wondering how sun's concerns in #205 are going to be addressed, follow-up(s)? (the response related follow-up could also have the goal to describe the response related changes in a change record).
Comment #232podarok CreditAttribution: podarok commented
vote for the #230
Comment #234xjm CreditAttribution: xjm commented
Comment #235xjm CreditAttribution: xjm commented
Comment #235.0xjm CreditAttribution: xjm commented
Updated Remaining Tasks section.


text:
TheExpressionist - Using Just What I Need
Using Just What I Need (7 points)
Used 'Organize Usings' 50 times. Unused usings are evil!
Started - 16 of 50 complete.
If you recently unlocked this achievement, it may not be updated on the server yet.
Please refresh your browser in a minute or two.
Please refresh your browser in a minute or two.
Visual Studio Achievements
Earn achievements while you code! With Visual Studio Achievements, you will unlock various achievements based on your activity.


text:
* Welcome to Random Quotes & Jokes *
You've just found the largest jokes site on the internet!
Over 20,000 jokes all categorized for easy viewing.
Search for jokes by keyword, by rating, or browse by category. Submit, comment, or rate on your favorites, and remember to bookmark us, we update every day!
Copyright © 2009 - 2014


text:
Advanced Search (Items only)
African Americans, Albert B. Hoyt, Albert M. Gardner, Ames Manufacturing Company, Benning Leavitt, Cabot Hall, Chicopee Mills, Fiction, George D. Lund, Giles E. Houghton, Henry M. Whittaker, Historic Newspapers, James G. Smith, James Lyman, John D. Vinton, Joseph D. Ashton, Know-Nothing Party, Legal Notices, Loman A. Moody, Nelson Tyler, Nuptuals, Philos B. Tyler, Poetry, Police Reports, Political Parties, R.B. Nagle, Short Stories, Stephen E. Cleaves, Sumner Vanhorn, Timothy W. Carter, Titus Chapin Sr. Esq., Wilson Eddy
atom, dc-rdf, dcmes-xml, json, omeka-json, omeka-xml, rss2


text:
Mon-Sun : 6:00 am to 11:00 pm
Looking for the best termite pest control company close to Bradenton 34212?
Our trained termite control professionals in Bradenton Florida are here to help you with your residential and commercial termite problems. Call today on 877-782-4054 to find out how quick and easy it is to get great service. We service all Florida areas!
Don't do it yourself, let our professionals take care of your problem.
Let our Bradenton 34212 termite exterminators deal with your infestation before it becomes a bigger problem.
Whether you need help with bed bugs, termites, spiders, mice, scorpions, eagles and other birds, mosquitoes, squirrels, insects, bees, ants, fleas, rats, cockroaches or spiders, or just want to do preventive pest control in Bradenton FL 34212, our advanced home pest control methods will take care of your problem.


text:
Please enter a valid mileage
Curious about the newer model? Check out the 2009 GMC Envoy
Information shown for entry-level SUV.
|Fuel Economy:
City 13/Hwy 19/Comb 15 MPG
|Max Seating: 5
|Engine: 6-Cyl, 4.2 L
|Transmission: Automatic
|Country of Origin: United States
|Country of Assembly: United States
To compare current market prices, check out KBB.com's car classifieds to view the new and used GMC Envoy vehicles for sale near you.