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:

  1. 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.
  2. 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.

  3. 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
  4. drupal_page_footer() is now a kernel "terminate" event. This needs to get cleaned up later.
  5. 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. :-)
  6. 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.
  7. 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.
  8. 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.

  1. Integrate the kernel and its components with the dependency injection container.
  2. 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.
  3. Develop the new routing system to replace our current one, as discussed.
  4. 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?)
  5. Replace the trivial access listener with something more flexible, powerful, and non-trivial. This should be considered as part of the previous two items.
  6. Revise the Ajax system to be more self-documenting and intuitive, and fit better with this new listener model.
  7. Eliminate our various "screw with the global request data" bootstrap routines, since we will not be screwing with it anymore.
  8. 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.

CommentFileSizeAuthor
#217 1463656-kernel-changelog.patch1.26 KBCrell
#212 1463656-kernel-changelog.patch1.26 KBCrell
#187 1463656-drupal-kernel.patch101.64 KBDave Reid
#186 1463656-drupal-kernel.patch103.03 KBDave Reid
#185 1463656-184-drupalkernel.patch101.64 KBRobLoach
#179 1463656-drupal-kernel.patch101.54 KBCrell
#161 1463656-drupal-kernel.patch104.73 KBCrell
#159 1463656-drupal-kernel-159.patch104.1 KBeffulgentsia
#154 0001-Patch-125.patch51.28 KBfgm
#125 1463656-drupal-kernel.patch105.68 KBCrell
#118 1463656-drupal-kernel-118.patch105.73 KBeffulgentsia
#105 1463656-drupal-kernel.patch105.72 KBCrell
#91 1545672_2_kernel.patch102.06 KBcosmicdreams
#88 1463656-drupal-kernel-88.patch96.21 KBaspilicious
#83 1463656_83_gutcheck.patch92.18 KBcosmicdreams
#77 kernel.patch50.41 KBRobLoach
#69 1463656_69_cleanup.patch34.01 KBcosmicdreams
#64 1463656-drupal-kernel.patch33.93 KBCrell
#60 1463656-drupal-kernel.patch31.96 KBCrell
#56 1463656-drupal-kernel-56.patch31.97 KBNiklas Fiekas
#56 1463656-drupal-kernel-56-interdiff.txt2.33 KBNiklas Fiekas
#54 1463656-drupal-kernel-54.patch29.64 KBNiklas Fiekas
#54 1463656-drupal-kernel-54-interdiff.txt4.96 KBNiklas Fiekas
#52 1463656-drupal-kernel_52.patch27.3 KBdeviantintegral
#48 1463656-drupal-kernel.patch27.63 KBCrell
#46 1463656-drupal-kernel.patch25.68 KBCrell
#44 1463656-drupal-kernel.patch1.33 MBCrell
#40 1463656-drupal-kernel.patch18.64 KBDamien Tournoud
#37 1463656-drupal-kernel.patch25.49 KBCrell
#35 1463656-drupal-kernel.patch18.44 KBDamien Tournoud
#33 1463656-drupal-kernel.patch20.96 KBDamien Tournoud
#31 1463656-drupal-kernel.patch20.6 KBDamien Tournoud
#27 1463656-drupal-kernel.patch28.6 KBCrell
#25 1463656-drupal-kernel.patch28.13 KBCrell
#10 1463656-drupal-kernel-changes.patch24.61 KBsun
#1 1463656-drupal-kernel.patch449.96 KBCrell
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Crell’s picture

Status: Active » Needs review
FileSize
449.96 KB

And patch. I can almost guarantee testbot won't like it yet. :-)

Status: Needs review » Needs work

The last submitted patch, 1463656-drupal-kernel.patch, failed testing.

moshe weitzman’s picture

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

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.

EclipseGc’s picture

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

webchick’s picture

bojanz’s picture

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.

yched’s picture

Issue tags: +symfony

add tag ?

bojanz’s picture

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.

Crell’s picture

I think this issue supersedes it, but lets leave it open since we'll probably want to pick it up later to push the request object out to more places than just the router.

sun’s picture

Same as #1, but without all the Symfony libraries.

(see #1465584: Review external library dependencies in core)

Crell’s picture

Thanks, sun. I'll try to roll a parallel patch for review in later drafts.

effulgentsia’s picture

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.

Crell’s picture

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.

effulgentsia’s picture

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

effulgentsia’s picture

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.

Crell’s picture

It works? Wow, I'm impressed. :-)

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.

catch’s picture

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

Crell’s picture

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.

neclimdul’s picture

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.

Crell’s picture

Issue tags: +WSCCI

Apparently we forgot to tag...

effulgentsia’s picture

Issue tags: -WSCCI

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.

effulgentsia’s picture

Issue tags: +WSCCI

.

Crell’s picture

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

webchick’s picture

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.

Crell’s picture

Status: Needs work » Needs review
FileSize
28.13 KB

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.

Status: Needs review » Needs work

The last submitted patch, 1463656-drupal-kernel.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Make note about file size and remove placeholder stuff.

Crell’s picture

Status: Needs work » Needs review
FileSize
28.6 KB

I think the issue may be Testbot not knowing how to cope without old-style Dirty URLs, so here's a version that keeps q=-style URLs working, at least for now. Let's see if Mr. Roboto likes this one.

Status: Needs review » Needs work

The last submitted patch, 1463656-drupal-kernel.patch, failed testing.

Crell’s picture

Sigh. I cannot replicate this locally; Simpletest enables just fine for me. Is there a tag to request a testbot-guru to have a look?

Damien Tournoud’s picture

Status: Needs work » Needs review
FileSize
20.6 KB

Trying a patch from the kernel-damz branch.

Status: Needs review » Needs work

The last submitted patch, 1463656-drupal-kernel.patch, failed testing.

Damien Tournoud’s picture

Status: Needs work » Needs review
FileSize
20.96 KB

Introducing even more hacks.

Status: Needs review » Needs work

The last submitted patch, 1463656-drupal-kernel.patch, failed testing.

Damien Tournoud’s picture

Status: Needs work » Needs review
FileSize
18.44 KB

Even smaller patch.

Status: Needs review » Needs work

The last submitted patch, 1463656-drupal-kernel.patch, failed testing.

Crell’s picture

FileSize
25.49 KB

Let's try this one...

Crell’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1463656-drupal-kernel.patch, failed testing.

Damien Tournoud’s picture

Status: Needs work » Needs review
FileSize
18.64 KB

Add */* as an HTML-type MIME type for the test bot.

Status: Needs review » Needs work

The last submitted patch, 1463656-drupal-kernel.patch, failed testing.

andypost’s picture

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

neclimdul’s picture

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

Crell’s picture

Status: Needs work » Needs review
FileSize
1.33 MB

Status: Needs review » Needs work

The last submitted patch, 1463656-drupal-kernel.patch, failed testing.

Crell’s picture

Status: Needs work » Needs review
FileSize
25.68 KB

Right, ignore that...

Status: Needs review » Needs work

The last submitted patch, 1463656-drupal-kernel.patch, failed testing.

Crell’s picture

Status: Needs work » Needs review
FileSize
27.63 KB

OK, this one should now be failing correctly. Thanks to sun for teaching me how testbot actually works, which pointed out where the mystery fails were. :-)

Status: Needs review » Needs work
Issue tags: -WSCCI, -symfony

The last submitted patch, 1463656-drupal-kernel.patch, failed testing.

deviantintegral’s picture

Status: Needs work » Needs review

#48: 1463656-drupal-kernel.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +WSCCI, +symfony

The last submitted patch, 1463656-drupal-kernel.patch, failed testing.

deviantintegral’s picture

Status: Needs work » Needs review
FileSize
27.3 KB

Status: Needs review » Needs work

The last submitted patch, 1463656-drupal-kernel_52.patch, failed testing.

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
4.96 KB
29.64 KB

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.
Niklas Fiekas’s picture

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

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
2.33 KB
31.97 KB

Fixing some of the menu callbacks that assume they get the trailing path as another parameter.

Status: Needs review » Needs work

The last submitted patch, 1463656-drupal-kernel-56.patch, failed testing.

Crell’s picture

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.

Niklas Fiekas’s picture

Ah, ok, thank you. I'll ping you.

Crell’s picture

FileSize
31.96 KB

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

RobLoach’s picture

Status: Needs work » Needs review

Testbot?

EclipseGc’s picture

He said it would fail, so he didn't even bother, but w/e it's queued.

Status: Needs review » Needs work

The last submitted patch, 1463656-drupal-kernel.patch, failed testing.

Crell’s picture

FileSize
33.93 KB

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.

chx’s picture

As you are keeping menu_get_item I do not need to comment on this.

Crell’s picture

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.

nielsvm’s picture

Subscribing....

Wim Leers’s picture

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 @TODO
- if(in_array('text/html', $acceptable_content_types) || in_array('*/*', $acceptable_content_types)) { Missing space after if.
- 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);

Obvious.

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

cosmicdreams’s picture

FileSize
34.01 KB

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.

Niklas Fiekas’s picture

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.
pounard’s picture

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

Wim Leers’s picture

That's fine, if we do that everywhere. Right now, it looks very inconsistent. At least to my eyes — maybe it's just me…

Crell’s picture

Symfony code should be following Symfony coding standards. Drupal code should be following Drupal standards.

pounard’s picture

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?

effulgentsia’s picture

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.

effulgentsia’s picture

Issue summary: View changes

Fix typo; HttpKernel is not a dependency for HttpKernel - HttpFoundation is ;)

Crell’s picture

Good point! That has changed. I have updated the summary accordingly.

I don't believe anything of substance has changed since #64. Just ongoing tweakage to get tests passing. If anything major changes I'll post a new patch back here.

Crell’s picture

Issue summary: View changes

Update summary based on how the patch has evolved.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
50.41 KB

I just wanna see how we're lookin', test-wise.

Status: Needs review » Needs work

The last submitted patch, kernel.patch, failed testing.

Crell’s picture

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

JohnAlbin’s picture

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

Crell’s picture

Issue tags: +wscci-hitlist

Tagging

cosmicdreams’s picture

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?

cosmicdreams’s picture

FileSize
92.18 KB

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.

cosmicdreams’s picture

Status: Needs work » Needs review

go testbot

Status: Needs review » Needs work
Issue tags: -WSCCI, -symfony, -wscci-hitlist

The last submitted patch, 1463656_83_gutcheck.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review

#83: 1463656_83_gutcheck.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +WSCCI, +symfony, +wscci-hitlist

The last submitted patch, 1463656_83_gutcheck.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
96.21 KB

Lets try this on

Status: Needs review » Needs work

The last submitted patch, 1463656-drupal-kernel-88.patch, failed testing.

cosmicdreams’s picture

Ah thanks, that's the worst of it. I suspect if I were to find and replace every use of $_GET['q'] with current_path() this would look much better. That's my next task

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
102.06 KB

I've probably done something silly. Starting with the current kernel branch, I applied the patch from #1183208: Remove variable_get('clean_url') and switch to index.php/path pattern for dirty URL support
Then, I did a find and replace for all $_GET['q'] and replaced it with current_path().
In testing the tests that this issue targets, I found that all of them passed. But that got me to wonder, what else would pass?

So I'm submitting this patch for the testbot to test. Hopefully there will be less that 750 failed tests and less than 16k exceptions.

P.S. hopefully this highlights the importance of #1183208: Remove variable_get('clean_url') and switch to index.php/path pattern for dirty URL support

Status: Needs review » Needs work

The last submitted patch, 1545672_2_kernel.patch, failed testing.

cosmicdreams’s picture

Looks like I missed something with my patch

sun’s picture

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

Crell’s picture

I just unpublished several testbot ping-pong comments. Let's keep those in #1486960: Testing testbot until we get something green. That's why that issue is there. Thanks.

Crell’s picture

Issue summary: View changes

Add a link to the latest test failures.

Crell’s picture

Issue summary: View changes

Update hitlist link

Crell’s picture

Issue summary: View changes

Update hitlist link.

Crell’s picture

OK, question for the audience:

#1558346: Fix TaxonomyTermTestCase
#1557590: Fix UserUserSearchTestCase
#1558166: Search Tests

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.

Thoughts? There's apparently a number of tests backed up on this one.

cosmicdreams’s picture

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.

webchick’s picture

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.
   array_shift($args);
   $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?

Crell’s picture

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.

Crell’s picture

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

webchick’s picture

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.

sun’s picture

@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 ;)).
Crell’s picture

Resolution documented here: #1565084: urldecode()ing of the path

sun: Discuss in the core queue, discuss elsewhere, make up your mind! :-P

catch’s picture

Separate issues can also be posted in the core queue.

catch’s picture

Issue summary: View changes

Update remaining test link again.

Crell’s picture

Issue summary: View changes

Update link to remaining test failures.

Crell’s picture

Status: Needs work » Needs review
FileSize
105.72 KB

And we have green!

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.

Review time!

[Edit: Dumb typo.]

Crell’s picture

Issue summary: View changes

Update summary to reflect changes in the actual patch.

catch’s picture

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.

Crell’s picture

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.

andypost’s picture

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

cosmicdreams’s picture

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 usages of drupal_static() & drupal_static_reset()

sun’s picture

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.

Crell’s picture

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

Crell’s picture

andypost: index.php instead of ?q= already happened in #1183208: Remove variable_get('clean_url') and switch to index.php/path pattern for dirty URL support. It now auto-detects whether clean URLs were used to get to the page, and uses whichever the browser used. url() is also going to be replaced with (or turned into a wrapper for) a Symfony Generator class. That includes subfolder handling. (I know subfolder handling does work, because my test copy was running in a subfolder during (development).

Niklas Fiekas’s picture

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

effulgentsia’s picture

Issue tags: -WSCCI, -symfony, -wscci-hitlist

#105: 1463656-drupal-kernel.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1463656-drupal-kernel.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review

#105: 1463656-drupal-kernel.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +WSCCI, +symfony, +wscci-hitlist

The last submitted patch, 1463656-drupal-kernel.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
105.73 KB
+++ 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().

tstoeckler’s picture

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

Crell’s picture

Oops. Yes it is. I thought I fixed that. I'll restore that in the next patch snapshot.

effulgentsia’s picture

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

Crell’s picture

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.

For the url() change in xmlrpc.php, see #113.

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

Thanks, effulgentsia!

Niklas Fiekas’s picture

Pushed

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

Crell’s picture

Crell’s picture

FileSize
105.68 KB

Updated patch based on the discussion above, plus some additional documentation improvements.

fgm’s picture

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 RouterListener::__construct(),

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

pounard’s picture

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.

Niklas Fiekas’s picture

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.

pounard’s picture

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?

pounard’s picture

Niklas Fiekas’s picture

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

Crell’s picture

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.

ob_flush() is called unconditionally in drupal_page_footer() now. Whether that's necessary or not I don't know, but I'd rather address that in a follow-up so as to reduce the amount of things we change in this patch. I have filed #1591682: ob_flush() called unconditionally in terminate listener as a follow-up.

Crell’s picture

Oopsies, lots of cross posting. :-) Sorry, Niklas. You can go ahead and clean up your branch since the code's already in kernel.

Crell’s picture

Issue summary: View changes

Markup fixes.

pounard’s picture

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.

stovak’s picture

You guys are ROCK STARS. SHIP IT!!!!

effulgentsia’s picture

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

catch’s picture

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

catch’s picture

Status: Needs work » Needs review

Sorry dreditor status change, not mine. This definitely needs more reviews.

quicksketch’s picture

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?

pounard’s picture

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.

Niklas Fiekas’s picture

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:
https://github.com/symfony/symfony/pull/4351
https://github.com/symfony/symfony/pull/4363

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

catch’s picture

@pounard: I know why people like yoda conditions, thanks. If you want to change the coding standards please file an issue.

pounard’s picture

Even that is in the standard? I mean I read this page about twice a week, I didn't even saw it!

Crell’s picture

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?

Added #1594870: Decide what KernelEvents::VIEW subscribers should be used for, document it somewhere, and make ViewSubscriber comply to follow-up on the View subscriber. See there for additional details about why it's currently setup the way it is.

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.

catch’s picture

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.

David_Rothstein’s picture

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.

Crell’s picture

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

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. However, I do think that #755584: Built-in support for csrf tokens in links and menu router is an important issue for D8 anyway on its own merits.

effulgentsia’s picture

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.

catch’s picture

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.

lsmith77’s picture

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

Niklas Fiekas’s picture

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

pounard’s picture

It is actually a userpace shutdown handler, the name and all its references should be changed.

Crell’s picture

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’s picture

FileSize
51.28 KB

Patch in #125 had a conflict with the latest commits. Rerolled without changes except the conflict resolution.

webchick’s picture

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.

Niklas Fiekas’s picture

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

fgm’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 0001-Patch-125.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
104.1 KB

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

Crell’s picture

effulgentsia: Commit to a feature branch, diff against the branch you want. Works great. :-)

fgm: As above, please let me roll patches from the sandbox. Anything posted here otherwise gets lost, and we don't want stuff getting lost.

Crell’s picture

Issue summary: View changes

Added link to follow-up issue tag.

Crell’s picture

FileSize
104.73 KB

New patch, includes the latest cleanups and adds another @todo for clarity.

Dave Reid’s picture

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

catch’s picture

Also whatever happens to file_transfer() won't be matched by file_download() unless someone remembers.

Wim Leers’s picture

I agree with Dave Reid and catch here — I also said this back in #68.

Crell’s picture

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

Dave Reid’s picture

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?

Niklas Fiekas’s picture

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.

Crell’s picture

Gah! I JUST finished removing file_transfer() entirely (it was only used in 2 places in core) and replacing it with the inline stream object. Vis, going the other way.

webchick’s picture

No, that's not right... file_transfer() is a useful general File API function, called by at least 37 contributed modules. Such as:

http://drupal.org/project/d2c
http://drupal.org/project/demo
http://drupal.org/project/download_file
http://drupal.org/project/dumper
http://drupal.org/project/media_crop
http://drupal.org/project/monster_menus
etc.

Thanks, Gotta Download Them All. :)

Dave Reid’s picture

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.

pounard’s picture

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

webchick’s picture

A better question is why force people to write 10 lines of code when they could call 1 function?

effulgentsia’s picture

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.

neclimdul’s picture

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.

catch’s picture

I agree with #173, I don't at all understand the resistance to a wrapper function either.

aspilicious’s picture

Issue tags: +Avoid commit conflicts

Adding tags to prevent this from rerolling.

Tests affected:
system.test
image.test
system/bootsrap.test
system/error.test
system/xmlrpc.test
taxonomy.test

pounard’s picture

It wasn't a resistance on my side, just a honest question, I have a better understanding of everyone's concerns know, thanks.

catch’s picture

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.

Crell’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
101.54 KB

OK, this should be the last one.

This chases HEAD a bit more, removes some unneeded use statements, and re-adds a rewritten file_transfer() utility function as a stop gap. (Hat tip to msonnabaum and Dave Reid, respectively.)

As noted in #105, this should be clean enough to merge so I am going to go ahead and mark RTBC. Dries and I will coordinate on getting it merged in this week.

Thank you everyone for your help in coding and reviewing us this far! There's still a long way to go, though...

cosmicdreams’s picture

It's likely that recent work on tests will require a reroll for this issue as those tests have moved within the source tree.

catch’s picture

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.

Tor Arne Thune’s picture

curl http://drupal.org/files/1463656-drupal-kernel_40.patch | git apply
error: core/modules/node/node.test: No such file or directory
cosmicdreams’s picture

@182, yep that's exactly what I mean. These tests have indeed moved.

aspilicious’s picture

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

DAMNIT :(

RobLoach’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
101.64 KB

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.

Dave Reid’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
103.03 KB

I believe this should be re-rolled for 8.x.

Dave Reid’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
101.64 KB

Yeah, Rob had it right, I was missing a commit from kernel branch. This should be the same as #185.

RobLoach’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC!

moshe weitzman’s picture

Status: 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:

  1. How we are going to provide multiple routes per path (see matchDrupalItem method).
  2. #1599108: Allow modules to register services and subscriber services (events) needs a plan.
  3. 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).

Crell’s picture

Status: Needs work » Reviewed & tested by the community

#1606794: Implement new routing system

And the summary is up to date. Have a look at the linked list of follow up issues. There's about a dozen of them.

effulgentsia’s picture

Assigned: Crell » Dries

Per #179, assigning to Dries.

pwolanin’s picture

Assigned: Dries » Crell
Status: Reviewed & tested by the community » Needs work

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.

effulgentsia’s picture

Assigned: Crell » effulgentsia

Working on updating the summary now. If someone else has already started that, ping me in IRC so we avoid conflicts.

effulgentsia’s picture

Issue summary: View changes

Fix link to follow-up issues.

pwolanin’s picture

Assigned: effulgentsia » Dries

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();
Niklas Fiekas’s picture

Good point. There is a follow-up to remove drupal_not_found() and drupal_access_denied() entirely. Not worth holding/updating the patch.

effulgentsia’s picture

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

quicksketch’s picture

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.

Dries’s picture

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?

catch’s picture

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

Crell’s picture

If we can do 11 am Eastern, probably. Much later than that and I need to get on a plane to Paris to talk to the Symfony masses about this patch. :-) Nothing like going down to the wire...

We need to be very careful about any further merge conflicts in the next 2 days.

Dries’s picture

Crell: let's plan on 11am then. I'll ping you on Skype.

Dries’s picture

Status: 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
effulgentsia’s picture

AWESOME!!!!!

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!

Fabianx’s picture

Nice :-)

sun’s picture

Title: Add a Drupal kernel; leverage HttpFoundation and HttpKernel » Change notification: Add a Drupal kernel; leverage HttpFoundation and HttpKernel
Assigned: Dries » Unassigned
Category: feature » bug
Priority: Major » Critical
Status: Fixed » Needs work
Issue tags: +Needs change record
  1. 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.

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

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

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

aspilicious’s picture

Sun well your commit in fact is incorrect now.
I made a change notice for the acces_denied() stuff.

http://drupal.org/node/1616360

Feel free to adjust it if it's incorrect.

David_Rothstein’s picture

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

sun’s picture

sun’s picture

Crell’s picture

Assigned: Unassigned » Crell
Issue tags: -wscci-hitlist, -Avoid commit conflicts

I'll take on the changelog.txt and update notification after this weekend, as I'm currently in full Symfony Live mode.

Also removing some no-longer-needed tags.

catch’s picture

Category: bug » task
Status: Needs work » Active

Let's open follow-ups for anything else that's not the change notifications, moving to active/task.

Crell’s picture

Status: Active » Needs review
FileSize
1.26 KB

Changelog patch attached. I also updated our list of used Symfony components, even though not all are a result of this issue.

Change notice here: http://drupal.org/node/1635626

It needs some tweaking, as I'm not sure if we want to list all implications in one place or split them up (eg, drupal_access_denied(), drupal_not_found(), etc.).

Status: Needs review » Needs work
Issue tags: -WSCCI, -Needs change record, -symfony

The last submitted patch, 1463656-kernel-changelog.patch, failed testing.

Crell’s picture

Status: Needs work » Needs review
Issue tags: +WSCCI, +Needs change record, +symfony

#212: 1463656-kernel-changelog.patch queued for re-testing.

Berdir’s picture

Status: Needs review » Needs work
+++ b/core/CHANGELOG.txtundefined
@@ -1,6 +1,7 @@
+- Replaced the core routing system with one build on the Symfony2 framework.

*built?

Berdir’s picture

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.

Crell’s picture

Status: Needs work » Needs review
FileSize
1.26 KB

Fixed the spelling mistake.

Edit: Also tweaked the change notice node.

Berdir’s picture

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

Crell’s picture

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.

Someone want to RTBC the changelog, please? :-)

webchick’s picture

Status: Needs review » Fixed

Committed and pushed to 8.x. Thanks!

webchick’s picture

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

Berdir’s picture

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.

webchick’s picture

Title: Add a Drupal kernel; leverage HttpFoundation and HttpKernel » Needs change notice: Add a Drupal kernel; leverage HttpFoundation and HttpKernel
Status: Fixed » Needs work

That's a good point. Marking needs work for the change notice.

Crell’s picture

Title: Needs change notice: Add a Drupal kernel; leverage HttpFoundation and HttpKernel » Add a Drupal kernel; leverage HttpFoundation and HttpKernel
Status: Needs work » Fixed

Berdir and I talked through this in IRC, and revised the change notice again. We're both comfortable with where it is now. We'll change things further as we break more APIs. :-)

Mark Trapp’s picture

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.

Crell’s picture

A master list of every related change notice would, in time, take over most of the page. It's still growing. I don't know how to create or maintain such a list. Unless you're volunteering? :-)

All change notices are relevant if you're porting a module, though.

Mark Trapp’s picture

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.

Crell’s picture

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.

Berdir’s picture

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

Crell’s picture

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.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

podarok’s picture

vote for the #230

xjm’s picture

Issue tags: -Needs change record
xjm’s picture

xjm’s picture

Issue summary: View changes

Updated Remaining Tasks section.