Jump to:

Background

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.

+++ 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.

Verifies

-----------

Dreditor (Chrome) just choked on this big patch so switching to narrative style review ...

UrlMatcher:
- 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.

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.

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.

The kernel branch has been rebased to eliminate the now-unnecessary hook_menu hackery. Now there's a legacy listener that shims in old menu router items that is working for all dynamic paths I've tried so far. Have a look at the sandbox for now. I'll roll a new patch at some point soon.

Still needs work: Overlay, autocomplete and similar callbacks, and Ajax/Ahah callbacks. The latter I think should move to its own custom mimetype at some point, but I don't know if now is the right time.

I also renamed the class to DrupalKernel again.

Moshe: I know you want a "fully expressed" patch, but this patch is a blocker for most of what comes next for both WSCCI and Layouts. We are going to need a new router mechanism (because hook_menu is not going to handle method- and-type-based routing nicely without totally gutting it), but I fear that's going to be its own lengthy discussion. IMO we should use config-based routes for that (Symfony does the same, and it gives us a number of other benefits I won't go into right now but will later), but we haven't fully had that discussion yet. My target is minimum-commitable-patch. Ibid for adding runtime listeners, getting the request object into other parts of bootstrap, etc.

I don't know that we have any need for the Kernel's logger. I've been largely ignoring it. We could probably switch to the HttpKernel timer, since it's there and it's not like ours is anything special. Similarly, we're already carrying around the Symfony session code, which has improved in 2.1 in part due to our feedback, and there is another issue open somewhere to switch to using that.

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. Otherwise, Drupal doesn't work right without Varnish installed, and that is clearly not a good solution. We're just not using it here yet to keep the patch simple.

I think the variable_get() call here is still not converted over to CMI. When that moves over, I'll move this to use CMI instead and even make it injected if I can. :-)

As for refactoring the menu lookup system in the process, eh, maybe. That may require more work to convert from Drupal-style to Symfony-style paths. Not sure. Help experimenting with that is welcome. :-)

bojanz: md5 is faster than sha256, but not drastically AFAIK. We don't use md5 in core anymore because government standards throw a fit when you use the "weak" md5, even if it's not for security purposes. I'm totally open to better approaches there, and I suspect that as we migrate from current routing definition to a new one that line will go away anyway.

The $this->allow() is inherited from the parent class. TBH, I don't fully understand why it's there either. :-) I believe it's used by a method called by that method. I don't have the code in front of me right now to check.

+++ 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.

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.

index.php/ for non-clean URLs is what Symfony does by default. Remember, Drupal *does not use $_GET['q'] internally. It shims it by writing back to it from wherever it got the actual path. When I took a completely non-scientific poll in #drupal-contribute, there was general applause at changing it.

Since I am not sure how much work is involved in retrofitting q= onto HttpFoundation, I'm inclined to just move to index.php and be done with it.

Note: Strictly speaking it doesn't even have to be index.php. HttpFoundation doesn't hard code a file name, which means that you can use index.php, app.php, app_dev.php, or whatever and it still works, assuming you let the components control paths. (I don't in the kernel branch yet because the UrlMatcher, which generates URLs for Symfony, is not available in url().) That's actually what Symfony full stack does (app.php and app_dev.php) for different configurations, and it something we can/should look into as a way to handle install.php, update.php, cron.php, etc: Simply alternative kernels at different paths.

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).

There is no love lost between me and clean URLs, but they are useful for development when in a subdirectory (so that you don't have to mess around with RewriteBase and always stashing your .htaccess file before messing around with Git). I can live either way, I guess.

That assumes we get rid of them entirely, which I'm not convinced we want to do. And as this patch is a blocker for a lot of things, I don't want to tackle "related" things unless they're fairly easy to do. Removing all traces of $_GET['q'] from core is a NOT simple task. See the request object patch above for an example of how difficult it is. :-)

Gr, I haven't had the time I thought I'd have to work on this. Updated patch against 8.x, for testbot, including the changes from #13. Now does not include the Symfony libraries as they're already there. Hopefully this will give us a better idea of what's left to fix before we can get this in.

@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.

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.

Here's an updated patch that includes the refactoring done at DrupalCon with Fabien. It still doesn't pass, but it's more injected. :-) Also, it includes some stubs for content negotiation, which will be replacing later with a real library.

Also, at Fabien's recommendation I've shifted from using listeners for everything to just listeners for touch-points. The upside is that it means the handler logic for different delivery systems is not tightly coupled to the EventDispatcher system. The downside is that I then don't know how we'll register new delivery systems (or whatever they get called in the new system). We'll figure that out.

I'm not setting this to needs review because I know testbot will still choke on it. For those who care, I am using #1486960: Testing testbot as a place to throw up patches for testbot without spamming everyone in this thread. I am also going to unpublish some of the comments above that are just me fighting with testbot to keep this page easier to read. If you really want to see me fighting with testbot, see the linked issue. :-)

New patch. We're now under 1000 failures! :-) This also now demonstrates how StreamedResponse works for private files. I am also now convinced that the way forward for the new router will be to use the ChainRouter library that lsmith is working on splitting from Symfony CMF and just building 2 entirely independent routing systems at once. That will just make life easier.

I don't know what will become of menu_get_item() in the long run. For the time being we're not removing it, but I expect that at minimum the router parts of that, including the $_GET['q'] fallback logic at the start of it, will go away eventually. Beyond that, I don't know what will happen as far as menu links go.

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.

Yay, I am not the only one. I posted a patch, too, but WSCCI doesn't use the normal patch workflow.

Crell:

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.

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.

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.

RobLoach: I'm using #1486960: Testing testbot for that, so no need to spam everyone in this thread with routine status checks. :-) I am also now keeping a link to the most recent set of failures in the summary for this issue to act as our "hit list".

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.

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.

All three have the same root cause. request_path() auto-urldecode()s the path. HttpFoundation does not. It provides the controller with the unadulterated path. That means a number of tests now fail on encoding-sensitive path strings, such as those above.

So, what do we do about it? We can either:

1) Do what Symfony does and pass in the raw URL, which means controllers (page callbacks) need to urldecode() values themselves if necessary. This is not a difficult task. Pro: Easy to implement, and closer to what other Kernel-using projects would be doing. (That makes it easier to onboard new people.) Con: A little more work for module developers.

2) Re-add the decoding ourselves somewhere. Pro: Not an API change, no more work for module developers. Con: Yet another Drupalism in a rather basic part of the logic that other Symfony-family projects are not doing. Also not certain right now where in the code flow we'd do it. Edit: webchick made a good analogy that to someone coming from other Symfony projects this might be like magic_quotes all over again.

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.

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:

// 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?

Don't think page callbacks; page callbacks are all going away entirely. Think "request controllers", which return responses. They're not random API functions that just coincidentally happen to be called from the kernel.

cosmicdreams: No, request_path() is going away in favor of data on the request object. And even then, we're talking about the data that gets passed into the controller. The request object itself will not always be passed in.

webchick: I would argue the opposite: There should be a REAL API call that does that sort of lookup, and the controller is just a trivially thin wrapper around it. Controllers in this model should be fairly dumb glue. If you're using the controller as an API call, it means someone didn't think through their code very well. (It's like putting actual save logic into a form submit callback. Don't do that!)

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.

@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 urldecode().]

PS: I'd recommend to use a separate issue for detailed questions in the future (we're already at #100 ;)).

After several weeks of work by I think over a dozen people, we finally have this test passing. :-) We tried to keep the overall impact minimal, but it did require futzing with many parts of the system.

Naturally this work is by no means complete, but this gives us a foundation to move forward. There are lots of follow-up issues to be created, which I will be doing shortly. There are also lots of @todos in the code, mostly things where a better solution would have taken longer to implement and have been more invasive. The goal here was minimally invasive to get the plumbing in place. 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 don't want to optimize prematurely.

I've updated the summary with the notable changes.

In order to keep things moving, I've talked to Dries and we're going to timebox this patch. Please do review, but major changes to the approch taken here are not on the table. We do want to refine this patch before it goes in fully.

Assuming there are not major problems, and if no one else beats me to it, I will mark this issue RTBC on Monday 28 May. Dries or catch (I presume Dries for something of this size) will then merge (not commit, merge; please contact me to coordinate if needed) ths kernel branch before 31 May. We can then proceed with the next steps, which consists of lots and lots of cleanup, the new router system, and otherwise trying to unblock the poor folks over in the SCOTCH initiative who are waiting not-so-patiently for us to let them get to work.

Of course, if someone feels that this issue is ready before then I am more than happy for someone to beat me to it. :-) I am presenting on WSCCI this coming weekend (the 18th/19th of May) and there's also a code sprint on the 20th. I would be very very happy if this patch was in before then so I could try and ramp up people at the sprint, but I understand that may not be feasible. Still, a guy can wish.

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.

catch: I haven't benchmarked it; Someone mentioned earlier tonight that the testbot was running slower with this patch, but I have no idea compared to what or what else may be going on in testbot. (It's been acting up this weekend.) Someone (not me) will need to profile it to see if there are any points of interest there.

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.)

Which changes are unnecessary? I tried to stick to just necessary changes, although some of them include refactoring that COULD be done differently, but at this point would be more work to break off and then have to adjust this patch for (especially given the timeline).

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.)

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.

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.

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?

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?

Re the rewrite rule: I changed that to match the recommended rewrite rule from the Symfony documentation. I have no real preference either way as long as it works. (mod_rewrite rules are black magic to me.) I'm fine if we want to revert that as long as nothing breaks.

Actually doing the @todos in drupal_not_found() etc: Yes, let's do.

drupal_page_set_cache(): Core is using output buffering(!) to handle the page cache at this point. I'm not entirely certain why. If we can safely move over to using the value that terminate passes in, I'm fine with that.

file transfer changes: What I'd actually like to do here is #1561362-2: Change file_transfer() to use BinaryFileResponse. That is, push such a utility upstream into Symfony where it belongs, and then all three of those locations become essentially one-liners. I was only making the minimal necessary changes here to get tests to pass. Is it worth it to refactor those now, when we already have an issue open to replace all three?

database exception changes: See the revised issue summary above. That was necessitated by the way Symfony translates exceptions.

Response('a', 204): The 'a' is totally vestigial. We have to pass something in the constructor there. Empty string would work better. (The Response::prepare() method strips the body from a 204 anyway, because 204s have no body by definition.)

Converting to $request->attributes->get('system_path'): I believe that was done before the index.php patch, at which point current_path() was still not actually useful. I'm fine with leaving it as an example. That code is going to change eventually anyway, so I don't know how much work we want to put into it either direction.

$request->request is, I believe, just POST. POST and GET are not merged, nor should they be, because they are different things. The PHP $_REQUEST superglobal is a bug IMO and not something that should be supported directly. So, no, it doesn't "handle POST trumping GET". The fact that we even need to think about that here is a design flaw in the existing code, but we're not fixing that problem here.

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.

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.

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.)

Looks like at some point recently Symfony switched the kernel properties from private to protected. Sweet. Removed that workaround. RouterListener still has the same private property problem, though, so I documented why we're working around it that way. That will be in the next snapshot.

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.

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.)

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().

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.

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.

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?

The "yoda conditions" there are all in code that was borrowed directly from Symfony in subclasses, and I saw no value in changing them when there is no effective difference. The same is true for the logger code and the sprintf(), and pretty much 99% of RouterListener. That's all Symfony code that may change in the future that I saw no reason to refactor at this point when it works well enough (or in some cases never actually executes) and is self-contained this way. Bringing in format_string() right now is unnecessary and introduces a hard dependency; we're trying to remove those.

Also, I like yoda conditions. :-)

I'll re-review for any lingering 4-space indentation, since those cause a mental parse error. I didn't see any of those, though. catch, can you identify where you saw them?

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. The old code didn't work (tests failed), so rather than converting it to hard-code a request object call and throw an exception in the page callback we moved that logic to the access callback where it would normally belong. Valid point about the menu link issue. Is that a problem in this particular case, meaning we need to convert back, or no? In general, an access callback is the correct place for it. Also, later conversions will rip the routing system away from menu links entirely to be separate questions. That makes it a temporary issue either way.

Performance-wise, Mark Sonnabaum has been testing it and found that there's about a 1.7k increase in the number of function calls, ~40% of which is from the increased use of the class loader so is not an issue to address here. He's still investigating further.

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:

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.

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.

Alterability isn't really what we were thinking about here. Proper use of the API is. If you're going to die for access reasons, better to do so earlier and bail out after doing less work. That said, I don't buy a "this sort of access should be alterable but not that kind" argument; If you are writing code of any sort you already have access to break anything you want. :-)

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.

"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.

Yeah, drupal_page_footer() is doing the stuff that the kernel terminate event does. It's just stupidly named, probably for legacy reasons because once upon a time it did deal with footer-level output. It was just never renamed when that changed.

@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?

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.

#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).

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.

We cannot pass-forward a parameter ($url) to a named function. We would still have to at bare minimum wrap a call to file_transfer() in an anonymous function so that we can use ($url). We would then still need to rip the headers and output buffering out of file_transfer(), and the exit calls, and then modify anywhere else it's called to match. I was going for "minimal changes" in this patch, and refactoring file_transfer into a delayed callback just to then remove it again in a later patch didn't seem very minimal.

Using anonymous functions is also a normal part of PHP 5.3 development. I see no reason to avoid them when they make sense in context, such as here. (In this case there's no sane way to not use them. This is exactly where they are useful as a tool.)

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.

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.

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.

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.

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).

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).

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.

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?

@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).

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!

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.

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-With shortcut 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.

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? :)

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.

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?)

I really don't want to overload the change notice with documentation. The changes we're making in this issue and follow-ups are much bigger than a change notice should contain. We absolutely will want/need more robust revised documentation, which when it exists the change notice can link to. But a change notice should not be the carrier of major documentation. That's findable only by a tiny sliver of the potential population.

The meah() change is the only one that really "fits" into this change notice, IMO. The listeners should be separate, once there is an API for that. (That's in progress in another issue.) drupal_not_found() et al are already in another change notice. An in-depth discussion of Request/Response architecture does not belong here.

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.

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.

There are a few that have already been created. However, I expect that many more will be created in the future as more changes roll in as a result of this one. We cannot link to those of course because they haven't happened yet.

As noted above, I think the proper answer here is real, complete documentation and then the change notice links to that. When we introduced the new database layer in D7, for instance, the change notice listed a few brief examples but mostly just linked to the much larger and more extensive new documentation. I figure the same approach is appropriate here, or will be as soon as such extensive documentation exists. (I haven't written it yet because it would go out of date as soon as I write it, since we're going to be changing things several times before they settle down.)

I really don't mean to sound like a stubborn stick in the mud trying to avoid writing docs. :-) I just am trying to be frugal with my limited time and not spend a great deal of it documenting transitional states that we have every intention of changing ASAP. If someone else wishes to flesh out the change notice further, however, I will certainly not stop them.

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).

As of yet, few things are returning Response objects. A change notice for that should be tied up in whatever future change makes most controllers/page callbacks actually want to return them. I think the rest of #205 has a separate follow-up issue or issues already.