Background
This is step 1 in the WSCCI/Symfony shift in our core routing logic. See this post for background. I will not repeat it here for brevity. (Rare for me, I know.)
Proposed resolution
This issue puts a Symfony2 HttpKernel-based routing model on top of our current bootstrap, effectively replacing menu_execute_active_handler(). It is not fully developed yet. There are still a lot of @todos in the code. The goal of this patch is to get the foundation in place so that we can build on top of it, hopefully in parallel.
Please read this blog series on the Symfony2 components before reviewing this patch. I know it's long (although it's actually a quick read), but reading that will vastly help understand the moving parts in this patch and save everyone time explaining things that are already documented.
Given that, what this patch does is as follows:
- Creates a Drupal kernel, called DrupalKernel, that extends the Symfony HttpKernel implementation. That lets us do our own setup, but leverage all of the existing workflow from Symfony itself.
-
Creates a Drupal-specific UrlMatcher class, extending Symfony's. This class is responsible for determining the correct Route (what Drupal 7 calls a router item) for a given request. At this point it is just a fairly trivial wrapper around the existing menu lookup system. This is a temporary measure; follow-up work will add a new, more robust routing component that can live along side this one for now, and then we can remove the old one entirely once everything is ported over.
- Registers a number of subscribers to the kernel. Subscribers are essentially a convenient way to bundle Listeners, which are more or less the Symfony equivalent of hooks. HttpKernel, which we are using here, has numerous event points to control various parts of the request process and allow others to hook into it. See the code for all of the nitty gritty
- drupal_page_footer() is now a kernel "terminate" event. This needs to get cleaned up later.
- We do not actually route on the path of the request. Drupal 7 manipulated the request path in-place, stored it into $_GET['q'], and then pretended that was what the browser asked for. Instead, we now have a request attribute, system_path, that we populate in the kernel "request" event. Currently we do four things to it, all ported from the legacy handling, from different listeners: urldcode() the path, handle language prefixing on the path, replace an empty request with the front page value, and url alias dereferencing. Although it wasn't the original intent, it means that we have essentially replaced hook_url_inbound_alter() entirely with "you can do that in the kernel request event if you want", and then ported all of our existing logic to that. As a side effect, it means that it will likely be possible to move url alias functionality out of core to a module if we want. I don't know if we want to do that, but the fact that we will be able to (after we refactor url generation, too) is really cool. :-)
- Any controller that is a function we assume to be a legacy controller and wrap in an anonymous function. That means we're not actually using any of Symfony's magic controller argument handling yet, but it means we needed to change almost nothing in the current routing system. That's a good trade off. Once the new routing system is in place we will leverage all of that fun new stuff.
- Returning MENU_NOT_FOUND or MENU_ACCESS_DENIED no longer works. The new correct way to handle those is to throw an exception, which the new ExceptionController will turn into the appropriate page response.
- The database system no longer throws PDOExceptions. Because of Symfony's FlattenException class, we cannot pass extra arbitrary public properties forward on the exception object to carry the query information. Instead, we ported the "the query that broke was" logic into the database layer directly, and took advantage of PHP 5.3's nested exception capability.
Note: Please *do not post new patches here*. The code is being worked on in the kernel branch of the WSCCI sandbox. New patches will be rolled from that. If you want to help writing code, contact Crell.
Remaining tasks
Follow up issues are being tagged kernel-followup. The following is a list that was started prior to that issue tag. Make sure these items are posted as issues and tagged appropriately.
- Integrate the kernel and its components with the dependency injection container.
- Replace current_path(), request_path(), and so forth with direct access to the request object, somehow. Or better, refactor enough code so that we don't need utility functions or the direct request object at all.
- Develop the new routing system to replace our current one, as discussed.
- Either extend or replace hook_menu() so that we can declare multiple router items at the same path for different mime types and HTTP methods. Since the router system and menu links system have no business being as intertwined as they are now, replacing hook_menu() entirely is a viable approach we will consider. (Possibly put it in CMI?)
- Replace the trivial access listener with something more flexible, powerful, and non-trivial. This should be considered as part of the previous two items.
- Revise the Ajax system to be more self-documenting and intuitive, and fit better with this new listener model.
- Eliminate our various "screw with the global request data" bootstrap routines, since we will not be screwing with it anymore.
- Lots of other things, but we'll get there. :-)
User interface changes
There shouldn't be any of note. This is all at the API level.
API changes
1) Delivery callbacks are gone in favor of kernel View events.
2) *Most* things are emulated fairly well at the moment, so most modules do not need to change. The big API changes will come in follow-ups. However, some more esoteric edge cases (returning NULL from a page handler, anywhere that we print and then call drupal_exit(), etc.) cannot be emulated so are being altered to simply return a response object.
Comment | File | Size | Author |
---|---|---|---|
#217 | 1463656-kernel-changelog.patch | 1.26 KB | Crell |
#212 | 1463656-kernel-changelog.patch | 1.26 KB | Crell |
#187 | 1463656-drupal-kernel.patch | 101.64 KB | Dave Reid |
#186 | 1463656-drupal-kernel.patch | 103.03 KB | Dave Reid |
#185 | 1463656-184-drupalkernel.patch | 101.64 KB | RobLoach |
Comments
Comment #1
Crell CreditAttribution: Crell commentedAnd patch. I can almost guarantee testbot won't like it yet. :-)
Comment #3
moshe weitzman CreditAttribution: moshe weitzman commentedWould it make sense to call this DrupalKernel?
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.
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.
Comment #4
EclipseGc CreditAttribution: EclipseGc commented$this->context instead of variable_get('site_frontpage', 'user');
This is a CMI thing, so we can't really update that till CMI's landed in core. Should be trivial after that though I would think.
Comment #5
webchickCMI is in core. ;) http://drupalcode.org/project/drupal.git/commit/44963050b20866f13c516848...
Comment #6
bojanz CreditAttribution: bojanz commentedDrupalKernel 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?
CMI perhaps? I guess that's the plugin discovery discussion again, irrelevant at this moment.
What also caught my eye is that Drupal's UrlMatcher::match($pathinfo) needs a comment explaining $this->allow().
It is initialized at the top of the method, and evaluated at the end, but the actual modification happens in another method call (matchCollection(), which is defined in the parent class), which took me a few minutes to realize. And I'm still unclear about what it actually does.
The sha256 hash call also looked curious, later saw that it is done to make the path pass validation when passed as the name parameter. Maybe we can get away with using a faster function such as md5 (if it's actually faster at all, just a stray thought.)
As for the size of the added Symfony code, I'm not really worried about that. It's their code. If there are extra pieces we are not using, it's not the end of the world.
And since in the future we could use Composer to just pull in the Symfony dependencies and not keep it ourselves (unless we patched it and have it in our own repo, but still separate from the main repo), that would be even more of a non-issue.
This is a general theme we'll always run into with libraries, we had it with JQuery UI, we have it in Symfony.
Comment #7
yched CreditAttribution: yched commentedadd tag ?
Comment #8
bojanz CreditAttribution: bojanz commentedWhat happens now with #1417320: Introduce the Request object?
It relates to #3 and has no matching changes in the current patch, so I didn't want to mark it as duplicate.
Comment #9
Crell CreditAttribution: Crell commentedI 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.
Comment #10
sunSame as #1, but without all the Symfony libraries.
(see #1465584: Review external library dependencies in core)
Comment #11
Crell CreditAttribution: Crell commentedThanks, sun. I'll try to roll a parallel patch for review in later drafts.
Comment #12
effulgentsia CreditAttribution: effulgentsia commentedLooks like some really good progress has been made, and I'm happy to see HttpKernel itself being used, not just the interface. Crell's in the middle of some work on removing the need for any hook_menu() changes from this initial patch, and once he merges that back into the "kernel" branch, I'll look into the AJAX bits to see if we can fully remove delivery callbacks as part of this patch.
Comment #13
Crell CreditAttribution: Crell commentedThe 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.
Comment #14
effulgentsia CreditAttribution: effulgentsia commented- Why this (and surrounding) change to the url() function as part of this issue?
- Hard-coding 'index.php' instead of using $options['script'] is wrong.
- $GLOBALS['conf']['clean_url'] seems to be NULL after a fresh install (is that a new bug/feature in D8?) In any case, combined with the hard-coded index.php, that's what messes up Overlay. Removing this line (on a setup where clean URLs work) makes Overlay work.
Comment #15
effulgentsia CreditAttribution: effulgentsia commentedOk, ignore #14. I see now that this issue is changing non-clean URLs from http://example.com/?q=foo to http://example.com/index.php/foo. If we really want to do that, that will necessitate fixing several things both Overlay related and not. Specifically within overlay, the Drupal.overlay.getPath(), Drupal.overlay.eventhandlerOverrideLink(), Drupal.overlay.fragmentizeLink() functions, and maybe others. So I guess a question is: do we really need to make this change now, or can we leave non-clean URLs as http://example.com/?q=foo until a future issue?
"kernel" branch as-is has Overlay working just fine once you enable clean URLs, so this really does just boil down to what we want to do with non-clean URLs at this time.
Comment #16
Crell CreditAttribution: Crell commentedIt 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.
Comment #17
catchWith changing the paths is this will break existing links on all sites that don't have clean URLs - that's a small enough proportion of sites that I can't find myself caring about it, but if we're going to do that I think we could go further and just drop explicit support for it per #1183208: Remove variable_get('clean_url') and switch to index.php/path pattern for dirty URL support. Then all those sites that now have the wonders of mod_rewrite or the equivalent we could also redirect the old URLs for in a commented section of .htaccess.
I'm short of time at the moment but I'll try to take a look at the Kernel branch (or an updated patch here if it arrives).
Comment #18
Crell CreditAttribution: Crell commentedThere 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.
Comment #19
neclimdulnot to mention those "oh gah clean urls decided not to work! Let me turn them off real quick and keep the site running while I fix it" moments.
Comment #20
Crell CreditAttribution: Crell commentedApparently we forgot to tag...
Comment #21
effulgentsia CreditAttribution: effulgentsia commentedSee also #1474018: Remove all traces of old-style dirty URLs, $_GET['q'], and 'clean_url' variable. I recommend including that into the scope of this first patch, because if this new kernel is changing our approach to dirty URLs, I think we need to incorporate that all the way, including into our tests.
Comment #22
effulgentsia CreditAttribution: effulgentsia commented.
Comment #23
Crell CreditAttribution: Crell commentedThat 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. :-)
Comment #24
webchickJust a note that #1465584: Review external library dependencies in core got committed, so the next patch doesn't need to include all of the Symfonic bits.
Comment #25
Crell CreditAttribution: Crell commentedGr, 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.
Comment #26.0
(not verified) CreditAttribution: commentedMake note about file size and remove placeholder stuff.
Comment #27
Crell CreditAttribution: Crell commentedI 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.
Comment #29
Crell CreditAttribution: Crell commentedSigh. I cannot replicate this locally; Simpletest enables just fine for me. Is there a tag to request a testbot-guru to have a look?
Comment #31
Damien Tournoud CreditAttribution: Damien Tournoud commentedTrying a patch from the kernel-damz branch.
Comment #33
Damien Tournoud CreditAttribution: Damien Tournoud commentedIntroducing even more hacks.
Comment #35
Damien Tournoud CreditAttribution: Damien Tournoud commentedEven smaller patch.
Comment #37
Crell CreditAttribution: Crell commentedLet's try this one...
Comment #38
Crell CreditAttribution: Crell commentedComment #40
Damien Tournoud CreditAttribution: Damien Tournoud commentedAdd */* as an HTML-type MIME type for the test bot.
Comment #42
andypostThere's still some usage in core: password and filter, also some grep'ed within symfony code
Comment #43
neclimdul@andypost the password code is legacy conversion and the filter is just some filler to obfuscate html comments so there's arguably no valid collision. The Symfony stuff is external code that we'll have to review but has no relevance to the fact that that is the reason we don't use md5 which is why I chose it for getting an automated unique value for the key.
I'd like to ask that we table this bike-shedding while there is some deep work being done on this patch and try to focus on what can get us to passing tests. That alone might remove that code and we've got the entire rest of the development cycle to argue the optimization of a hash function.
Comment #44
Crell CreditAttribution: Crell commentedComment #46
Crell CreditAttribution: Crell commentedRight, ignore that...
Comment #48
Crell CreditAttribution: Crell commentedOK, 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. :-)
Comment #50
deviantintegral CreditAttribution: deviantintegral commented#48: 1463656-drupal-kernel.patch queued for re-testing.
Comment #52
deviantintegral CreditAttribution: deviantintegral commentedPulling over the patch from https://drupal.org/node/1486960#comment-5772004
Comment #54
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedHere's a try to fix custom 403 and 404 pages.
/
prefix to paths.Comment #55
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedOh ... I realize that with the prefix, this comparision seams to be flawed.
Comment #56
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedFixing some of the menu callbacks that assume they get the trailing path as another parameter.
Comment #58
Crell CreditAttribution: Crell commentedNiklas, please do not post patches here directly as they cannot be merged with the ongoing work in the kernel sandbox. If you want to help out with the code, please grab me in IRC. I'm usually in #drupal-wscci any time I'm available.
Comment #59
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedAh, ok, thank you. I'll ping you.
Comment #60
Crell CreditAttribution: Crell commentedHere'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. :-)
Comment #61
RobLoachTestbot?
Comment #62
EclipseGc CreditAttribution: EclipseGc commentedHe said it would fail, so he didn't even bother, but w/e it's queued.
Comment #64
Crell CreditAttribution: Crell commentedNew 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.
Comment #65
chx CreditAttribution: chx commentedAs you are keeping menu_get_item I do not need to comment on this.
Comment #66
Crell CreditAttribution: Crell commentedI 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.
Comment #67
nielsvm CreditAttribution: nielsvm commentedSubscribing....
Comment #68
Wim LeersCoding 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 afterif
.- unnecessary
\n
afterpublic 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:
- 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
Obvious.
While I do like this usage of closures to simplify code, I think it's inappropriate in this case. Contrib modules also want to handle file downloads sometimes. It'd be nice to be able to do
return new StreamedResponse(some_function)
in those cases.Comment #69
cosmicdreams CreditAttribution: cosmicdreams commentedThis patch cleans up most of #68:
And I commented out the return $route from UrlMatcher.php's converDrupalItem since it seems to be the intent of this function to return a proper Route object.
Comment #70
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedYay, I am not the only one. I posted a patch, too, but WSCCI doesn't use the normal patch workflow.
Crell:
Comment #71
pounardSymfony does put the opening bracket the next line, while Drupal standard is to write it on the same line as the method signature. In that particular context, adding a new line between the method signature and body actually highlight the first line if it's important, and make the code more "airy" and readable.
I'd keep those new lines myself, they make a lot of sense, they semantically give more importance to the line right under them by making it more readable.
Comment #72
Wim LeersThat's fine, if we do that everywhere. Right now, it looks very inconsistent. At least to my eyes — maybe it's just me…
Comment #73
Crell CreditAttribution: Crell commentedSymfony code should be following Symfony coding standards. Drupal code should be following Drupal standards.
Comment #74
pounardBasically it does not break a standard to put a single new line to do a proper separation of code blocks, even if the code block is a one liner, right?
Comment #75
effulgentsia CreditAttribution: effulgentsia commentedIn http://groups.drupal.org/node/223749, Crell wrote:
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:
Is that still true? From what I see in #64, there's BC in place for this too.
Comment #75.0
effulgentsia CreditAttribution: effulgentsia commentedFix typo; HttpKernel is not a dependency for HttpKernel - HttpFoundation is ;)
Comment #76
Crell CreditAttribution: Crell commentedGood 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.
Comment #76.0
Crell CreditAttribution: Crell commentedUpdate summary based on how the patch has evolved.
Comment #77
RobLoachI just wanna see how we're lookin', test-wise.
Comment #79
Crell CreditAttribution: Crell commentedRobLoach: 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".
Comment #80
JohnAlbinThe ESI part of this is critical to making Drupal a fantastic mobile solution provider. I just finished watching @fabpot's Drupalcon Denver session and I'm a convert. Being able to conditionally include chunks of the page depending on device capabilities is a corner stone of Luke Wroblewski's RESS idea (Responsive Design + Server Side components).
Comment #81
Crell CreditAttribution: Crell commentedTagging
Comment #82
cosmicdreams CreditAttribution: cosmicdreams commentedIt might be a good idea to note in the summary of this issue that getting #1183208: Remove variable_get('clean_url') and switch to index.php/path pattern for dirty URL support committed dramatically improves this issue's ability to pass tests. No?
Comment #83
cosmicdreams CreditAttribution: cosmicdreams commentedHere's a combination of the current kernel branch (from my branch) and the patch from #1183208: Remove variable_get('clean_url') and switch to index.php/path pattern for dirty URL support. Just want to see how it performs.
One thing that stands our for me before testing this is that there seems to be an incomplete effort to convert every use of $_GET['q'] to current_path(). When I was making all of the test pass in #1545672: Fix kernel errors related to "Theme initialization in hook_init()" I had to convert two additional instances of $_GET['q'] to current_path() in order to get everything green.
Those exceptions will likely show up again in testing this patch.
Comment #84
cosmicdreams CreditAttribution: cosmicdreams commentedgo testbot
Comment #86
cosmicdreams CreditAttribution: cosmicdreams commented#83: 1463656_83_gutcheck.patch queued for re-testing.
Comment #88
aspilicious CreditAttribution: aspilicious commentedLets try this on
Comment #90
cosmicdreams CreditAttribution: cosmicdreams commentedAh 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
Comment #91
cosmicdreams CreditAttribution: cosmicdreams commentedI'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
Comment #93
cosmicdreams CreditAttribution: cosmicdreams commentedLooks like I missed something with my patch
Comment #94
sun@cosmicdreams: Can you use #1486960: Testing testbot for testing this patch, please? I'm going to unpublish your comments on this issue tomorrow, if no one beats me to it. Thanks.
Comment #95
Crell CreditAttribution: Crell commentedI 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.
Comment #95.0
Crell CreditAttribution: Crell commentedAdd a link to the latest test failures.
Comment #95.1
Crell CreditAttribution: Crell commentedUpdate hitlist link
Comment #95.2
Crell CreditAttribution: Crell commentedUpdate hitlist link.
Comment #96
Crell CreditAttribution: Crell commentedOK, 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.
Comment #97
cosmicdreams CreditAttribution: cosmicdreams commentedIs both an option?
1. we keep the request_path() function for module developers and for Drupal 8 inform them that the method is depreciated an will go away (or be repurposed) in Drupal 9. request_path could be implemented as an extension to Symfony's request object. http://en.wikipedia.org/wiki/Decorator_pattern
2. Create some new function (or use Symfony's request based syntax directly) that implements Symfony's request handling faithfully and use that throughout core.
or however you want to name things is fine.
Comment #98
webchickSummary 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:
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 liketaxonomy_autocomplete('field_tags', urlencode('monkey, banana, dishwasher'))
instead which is a) totally non-intuitive, and b) way more tightly couples page callbacks to being to the router system. If we're going to do that, why not just switch to encouraging arg() use everywhere again?Comment #99
Crell CreditAttribution: Crell commentedDon'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.
Comment #100
Crell CreditAttribution: Crell commentedwebchick: 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!)
Comment #101
webchickYeah, I can see that argument. I'm still not sure it's worth the pain of passing this responsibility to the module developers to take care of, though.
Also on that IRC discussion was mention of some kind of wrapper thingy thing that was already doing things like mapping variables in paths to the correct places and it was suggested that if we have such a "magic" layer, it seems like urldecode() could be something it does, too.
It was also suggested that we could file a feature request upstream with Symfony to handle urldecode() natively, thus removing our need to do special Drupalisms.
Comment #102
sun@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 becauseurldecode()
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
. Onlysearch_view()
is using it currently. If there wasn't also$field_name
, thenmenu_tail_load()
would be the loader to perform theurldecode()
.]PS: I'd recommend to use a separate issue for detailed questions in the future (we're already at #100 ;)).
Comment #103
Crell CreditAttribution: Crell commentedResolution documented here: #1565084: urldecode()ing of the path
sun: Discuss in the core queue, discuss elsewhere, make up your mind! :-P
Comment #104
catchSeparate issues can also be posted in the core queue.
Comment #104.0
catchUpdate remaining test link again.
Comment #104.1
Crell CreditAttribution: Crell commentedUpdate link to remaining test failures.
Comment #105
Crell CreditAttribution: Crell commentedAnd 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.]
Comment #105.0
Crell CreditAttribution: Crell commentedUpdate summary to reflect changes in the actual patch.
Comment #106
catchI'd like to see performance data and a specific issue opened to tackle this if there's a problem before this goes in. I'm not opposed to committing known performance regressions, as long as we clearly document what they are and that there's some kind of plan to fix them.
Will try to take another look through the latest patch asap.
Comment #107
Crell CreditAttribution: Crell commentedcatch: 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.
Comment #108
andypostLooks like core now has no ability to detect running from subfolder.
And how url() could work without clean_url variable with now index.php/path
Comment #109
cosmicdreams CreditAttribution: cosmicdreams commentedLet's put these in the summary. Can this be one? #1577902: [META] Remove all usages of drupal_static() & drupal_static_reset()
Comment #110
sunThe potential performance issues have to be checked in detail upfront. Someone mentioned a 10 minute increase of total time for the full core test suite, which would mean a 33% performance decrease, which obviously wouldn't be acceptable. (I did not verify this.)
Two larger sub-issues remain to be resolved though:
#1536844: Port language/path handling to the kernel model
#1567428: Active trail tests
Additionally, we should remove all unnecessary changes from this branch/patch. (#108 mentions one of them.) If no one beats me to it, I'm going to do that later this week.
Comment #111
Crell CreditAttribution: Crell commentedWhich 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).
Comment #112
Crell CreditAttribution: Crell commentedandypost: 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).
Comment #113
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThe changes in #108 are absolutely nescessary, because xmlrpc.php is it's own front controller. While I changed that, I worked in a subdirectory, so that should be fine. (Without the changes url() would prefix /index.php/ to core/xmlrpc.php on some setups.)
Comment #114
effulgentsia CreditAttribution: effulgentsia commented#105: 1463656-drupal-kernel.patch queued for re-testing.
Comment #116
cosmicdreams CreditAttribution: cosmicdreams commented#105: 1463656-drupal-kernel.patch queued for re-testing.
Comment #118
effulgentsia CreditAttribution: effulgentsia commentedThis patch just rerolls for cron_key moving from variable_get() to config().
Comment #119
tstoecklerThis seems to be an unintended change.
Comment #120
Crell CreditAttribution: Crell commentedOops. Yes it is. I thought I fixed that. I'll restore that in the next patch snapshot.
Comment #121
effulgentsia CreditAttribution: effulgentsia commentedIs this necessary? If so, why? Tests pass without it.
Let's remove these 2 lines then, and have 2 less todos.
This new parameter is passed by RequestCloseSubscriber::onTerminate(), but is not used in the function body or documented in the function docblock.
Similar but different code also appears in this patch's changes to update_test.module. Any way to create a helper function to serve both? Also, the legacy file_transfer() function is still called by image_style_deliver(), so this helper function would serve 3 use-cases in core alone.
Elsewhere in this patch, code is changed from
return MENU_NOT_FOUND
todrupal_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.Add a "@todo" to the comment so it can be found by people searching for those.
This looks like good stuff. But how is it related to this issue?
What's the 'a' for? If needed, please add a comment explaining it.
This seems like the only place in this patch where module code is converted from current_path() to $request->attributes->get('system_path'). Is that intended as a demonstration, or should we remove that and defer to the follow-up issue that removes current_path() everywhere?
Here and the other identical code in this file: I agree that this is a necessary bug fix, but it's unrelated to this patch. I think it should be moved from here to a separate issue.
Does $request->request not handle POST trumping GET? Why do we also need to check $request->query?
Comment #122
Crell CreditAttribution: Crell commentedRe 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!
Comment #123
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedPushed
to the clean-up branch.
Replacing all instances of drupal_not_found() and drupal_access_denied() with directly throwing the exception (as a first step before doing it even better) would add another 20k to the diff. We should probably defer that to a follow up.
Comment #124
Crell CreditAttribution: Crell commentedCreated #1587850: Replace drupal_not_found() with throw NotFoundHttpException
Comment #125
Crell CreditAttribution: Crell commentedUpdated patch based on the discussion above, plus some additional documentation improvements.
Comment #126
fgmSome seemingly redundant code:
DrupalKernel::__construct()
includes this: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()
callsob_flush()
in all situations, although OB is only started in_drupal_bootstrap_page_header()
if!drupal_is_cli()
.Comment #127
pounardIf 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.1They changed it, then I guess you're right; we should remove those two unnecessary lines.Comment #128
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThanks for reviewing, fgm. That's true for HttpKernel. Pushed that to the clean-up branch. RouterListener, however, uses private members (even on Symfony HEAD), so we can't do that there. (
Not sure if it makes sense, but if it does, maybe we should file an issue in the Symfony tracker. Going to do that and link it here, just to get comments from Symfony people.Awaiting comments on https://github.com/symfony/symfony/pull/4351.)Guess you're right about the ob_flush(). Filed #1591666: ob_flush() in RequestCloseSubscriber::onTerminate() not correct when drupal_is_cli() to keep track of that.
Comment #129
pounardI read the code bundled in my vendor/ directory and it's not private, it was in 2.0, it would be weird that they'll switch to protected then back to private again?
Comment #130
pounardhttps://github.com/symfony/symfony/blob/master/src/Symfony/Component/Htt... it's protected there.
Comment #131
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedYes, totally valid for HttpKernel. Just not RouterListener. See #128, where I also linked to Symfony HEAD and http://api.drupal.org/api/drupal/core%21vendor%21Symfony%21Component%21H....
Comment #132
Crell CreditAttribution: Crell commentedLooks 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.
Comment #133
Crell CreditAttribution: Crell commentedOopsies, lots of cross posting. :-) Sorry, Niklas. You can go ahead and clean up your branch since the code's already in kernel.
Comment #133.0
Crell CreditAttribution: Crell commentedMarkup fixes.
Comment #134
pounardThe unconditionnal ob_flush() allows any buffered output to be released and sent to the browser, then, the hook_exit() and other userland runtime shutdown tasks can run without keeping the connection opened (I guess) this would make sense, but I don't know if PHP actually releases the frontend connection when doing that (I'm quite sure it doesn't). Something I'm sure of, is that the shutdown code is run in parallel of the output being sent to the browser since it's not PHP which is actually send the data, but the HTTPd upper, so it's a few millisec saved for the full page rendering. I think it's safe to keep this ob_flush(). Notice that we may call ob_flush_end() instead that will release and close the output buffer stream, and except in case of fatal error while Drupal shutdown handlers are running, this would be safe to do.
Comment #135
stovak CreditAttribution: stovak commentedYou guys are ROCK STARS. SHIP IT!!!!
Comment #136
effulgentsia CreditAttribution: effulgentsia commentedFrom #110:
I submitted the following patches to the sandbox queue:
#1593702: Remove @todo comment cleanups from unrelated code
#1593674: Remove unrelated change to xmlrpc.test
#1593428: Remove unnecessary change to system_custom_theme()
#1593710: Revert unnecessary file_download() conversion from drupal_access_denied()/drupal_not_found() to throwing exceptions
Comment #137
catchfwiw 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:
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().
This is the most odd bit architecturally, I realise there's the @todo where it's registered but it'd be good to have the follow-up discussion open to discuss what to do about it. Also even with the current limitation of hard-coded subscribers, why a single subscriber handling four arbitrary content types? There's also a lot of copy/pasting between these methods.
I don't see this approach working if we ever have a token in a menu link or contextual links contextual links - since the acces callback is also run when checking access to the link itself, not only the page callback. I don't think we actually have any of these in core at the moment but it seems a valid use case to support and this looks like either a new limitation or unnecessary refactoring.
Currently this works for the comment approve links, but only because it's shown in the comment links variable, which doesn't run through that menu check. So I think this needs a bit more thought - ideally we want a pattern that'll work if you add a menu link to a token protected path too (note I didn't check manually verify this is a problem but I don't see how it could work).
There's a lot more nitpicky stuff, but apart from the first one here I'm trying to gloss over that for now.
Comment #138
catchSorry dreditor status change, not mine. This definitely needs more reviews.
Comment #139
quicksketchOkay, I read the whole blog series on Symfony of posts and everything, and the thing looks pretty cool.
However this patch seems to be sorely lacking. It adds 1,700 lines of code and tons of calls to methods/classes within Symfony that weren't used before. If this were benchmarked today it seems like it would inevitably be slower (if it's not slower, well then kudos and lets put it in). Can we get it to a point where it is actually making improvements (rather than just additions) *before* committing it?
Comment #140
pounardIf yoda condition means using:
Instead of:
Then yoda condition++
That kind of typo errors people often do:
To silent errors this leads, while:
A fatal error this will cause: the typo shall not pass.
There is actually a real technical reason why yoda is right: because it makes typo error fail instead of being silent. Last week I saw a patch reroll on some issue because of such typo error.
Comment #141
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThanks 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.
Comment #142
catch@pounard: I know why people like yoda conditions, thanks. If you want to change the coding standards please file an issue.
Comment #143
pounardEven that is in the standard? I mean I read this page about twice a week, I didn't even saw it!
Comment #144
Crell CreditAttribution: Crell commentedThe "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.
Comment #145
catchI'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:
Well it means you if you ever made a menu link/tab/contextual link to any of those paths, then it'll break - and given we had a patch to make those comment links contextual links for D7 that was only rolled back due to a performance regression, that doesn't seem too unlikely at all tbh. So, I think it'd be better to convert back rather than knowingly introduce a regression.
Comment #146
David_Rothstein CreditAttribution: David_Rothstein commentedBesides the menu link issue, I think the (completely undocumented) reason is that access callback functions are intended to be altered by other modules, and the purpose of that is to modify traditional access control, e.g. permissions. But this is an independent security feature so it shouldn't be alterable.
It makes more sense to travel along with the page callback, because whether or not a security token is needed in the first place is directly tied to the exact actions that the page callback winds up performing.
If keeping it in the page callback isn't an option for some reason, maybe another option would be to mark #755584: Built-in support for csrf tokens in links and menu router as a critical issue? That's likely a better way to handle tokens anyway.
Comment #147
Crell CreditAttribution: Crell commentedAlterability 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.
Comment #148
effulgentsia CreditAttribution: effulgentsia commentedThat's ready for review: #1595298: Move CSRF token validation back to page callbacks.
Comment #149
catchThanks for #1595298: Move CSRF token validation back to page callbacks that looks like the right change to me, we can discuss more automated support for tokens in #755584: Built-in support for csrf tokens in links and menu router.
With the ViewSubscriber extensibility issue, the @todo about making that extensible should move to the specific class I think rather than where it's registered. If it was there, I likely wouldn't have need to ask about it in this issue.
Comment #150
lsmith77 CreditAttribution: lsmith77 commented"drupal_page_footer() is now a kernel "terminate" event. This needs to get cleaned up later."
that sounds wrong to me .. terminate events are for stuff you want to do after the response has been send (f.e. sending an email). adding a footer and stuff like this should be done in a response (or a view) listener.
Comment #151
Niklas Fiekas CreditAttribution: Niklas Fiekas commented@lsmith77: Probably the name "drupal_page_footer()" is a bit confusing. Look what it does: http://api.drupal.org/api/drupal/includes%21common.inc/function/drupal_p.... It doesn't really add stuff to the footer, just stuff to close the request, really.
Comment #152
pounardIt is actually a userpace shutdown handler, the name and all its references should be changed.
Comment #153
Crell CreditAttribution: Crell commentedYeah, 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.
Comment #154
fgmPatch in #125 had a conflict with the latest commits. Rerolled without changes except the conflict resolution.
Comment #155
webchickIf you, like me, are not following the WSCCI sandbox and only this issue, there is some interesting stuff happening in #1578090: Benchmark/profile kernel.
Comment #156
Niklas Fiekas CreditAttribution: Niklas Fiekas commented@fgm: Since were not using a patch based workflow for the kernel, it's hard to merge your changes back to the kernel branch. Crell will happily give you commit access to the sandbox (if you promise not to change the main kernel branch). You could then branch, merge 8.x and push the branch with the merge commit that solves the problem.
Looks like the patch size is much smaller. Did you git apply the kernel patch, make your changes and then diff without the added files?
Comment #157
fgmNo, I did as usually, apply the patch on previous version in a feature branch, pull 8.x, and rebase the branch. But that visibly didn't work: the result doesn't even install. So I must have erred somewhere.
Comment #159
effulgentsia CreditAttribution: effulgentsia commented#154 contains only modified files. It is missing all the new files. Maybe the patch was created with just a
git diff
rather than agit add; git diff --cached
(if there's an easier way to roll patches that include new files, I don't know it).Here's the same with the new files included too.
The only difference this has relative to #125 is something that's already committed to the sandbox's kernel branch: #1593702: Remove @todo comment cleanups from unrelated code.
Comment #160
Crell CreditAttribution: Crell commentedeffulgentsia: 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.
Comment #160.0
Crell CreditAttribution: Crell commentedAdded link to follow-up issue tag.
Comment #161
Crell CreditAttribution: Crell commentedNew patch, includes the latest cleanups and adds another @todo for clarity.
Comment #162
Dave ReidThis seems really strange to me. Why can't we fix file_transfer() to do this logic rather than using an anonymous function that other modules can't re-use? I get that we want to do #1561362: Change file_transfer() to use BinaryFileResponse as a follow-up, but it seems really, really strange that we can't update file_transfer() to pass tests rather than use anonymous functions which so far have confused multiple people.
Comment #163
catchAlso whatever happens to file_transfer() won't be matched by file_download() unless someone remembers.
Comment #164
Wim LeersI agree with Dave Reid and catch here — I also said this back in #68.
Comment #165
Crell CreditAttribution: Crell commentedWe 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.)
Comment #166
Dave ReidSo maybe what I'm saying is that file_transfer() should instead be using the anonymous function itself so that we can re-use file_transfer() for now?
Comment #167
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedMakes 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
couldn't use that helper function, because file transfer is only for managed files (???). So doing it might not be worthwhile. But I guess that should be discussed in the issue.
Comment #168
Crell CreditAttribution: Crell commentedGah! 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.
Comment #169
webchickNo, 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. :)
Comment #170
Dave ReidI re-added file_transfer() as a wrapper for the StreamedResponse object and anonymous function until it's replaced/improved by #1561362: Change file_transfer() to use BinaryFileResponse as a follow-up.
Comment #171
pounard@#169 Removing file transfert from D8 makes sense to me, and kernel is gonna break some deeper API's than that, why the modules using it are blockers?
Comment #172
webchickA better question is why force people to write 10 lines of code when they could call 1 function?
Comment #173
effulgentsia CreditAttribution: effulgentsia commentedThe goal in #1561362: Change file_transfer() to use BinaryFileResponse is to enable these modules to achieve what they need in 1 line of code, via a class committed to Symfony. The question is what to do in the meantime: provide a wrapper function that might go away once the Symfony class is available, or make modules tracking D8 HEAD write a 10 line anonymous function until the Symfony class is available. I'd prefer the wrapper function: that's not a critical blocker for me if there's too much resistance to the wrapper function in this initial kernel patch, though I don't really understand why there's that resistance.
Comment #174
neclimdulI don't agree with using usage as a valid judge of a solution in this case. Neither 2 uses in core or a million uses in contrib are an argument for doing the right thing. The question should be which makes more sense as an API and what makes it easy to transfer a file to the browser.
Also, what should we just accept as part of the initial patch? Because its been decided its not acceptable for WSCCI to do all its refactoring outside of core, we're not going to just rip into every subsystem and solve the right solution in the first pass. There are things that are going to be obviously not finished in this pass. This is a big refactor, and we're doing a merge with both the goal to provide as much a final product as possible, disrupting core a little as possible, and getting it done as soon as possible. All these things are at odds and we're going to have to accept some middle ground and paint a path to follow up and finish the job. What to do with file transfer seems like one of those things we get in place and work to get the API more in line after the fact.
Comment #175
catchI agree with #173, I don't at all understand the resistance to a wrapper function either.
Comment #176
aspilicious CreditAttribution: aspilicious commentedAdding tags to prevent this from rerolling.
Tests affected:
system.test
image.test
system/bootsrap.test
system/error.test
system/xmlrpc.test
taxonomy.test
Comment #177
pounardIt wasn't a resistance on my side, just a honest question, I have a better understanding of everyone's concerns know, thanks.
Comment #178
catchaspilicious, thanks for listing the tests! I'm not sure exactly when I'll have time, but I'll try to get some more movement going on the other PSR-0 conversion issues that don't affect this next time i'm committing stuff.
Comment #179
Crell CreditAttribution: Crell commentedOK, 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...
Comment #180
cosmicdreams CreditAttribution: cosmicdreams commentedIt's likely that recent work on tests will require a reroll for this issue as those tests have moved within the source tree.
Comment #181
catchI've been avoiding everything listed in #176, so hopefully that's worked. However, I'm going to send the latest patch for a re-test just in case.
Comment #182
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedComment #183
cosmicdreams CreditAttribution: cosmicdreams commented@182, yep that's exactly what I mean. These tests have indeed moved.
Comment #184
aspilicious CreditAttribution: aspilicious commentedOh great thats my fault! :( srry
Can be applied manually to "NodeFeedTestCase".
DAMNIT :(
Comment #185
RobLoachI understand uploading patches for this is the wrong way, but I was getting fatal errors when rebasing between Drupal and WSCCI's remote git. Updating the version of git running on Drupal.org would fix the problem.
In the mean time, here's the updated patch with the correct paths.
Comment #186
Dave ReidI believe this should be re-rolled for 8.x.
Comment #187
Dave ReidYeah, Rob had it right, I was missing a commit from kernel branch. This should be the same as #185.
Comment #188
RobLoachBack to RTBC!
Comment #189
moshe weitzman CreditAttribution: moshe weitzman commentedI 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:
Changing to needs work but only for follow-ups to be created. Keep calm and carry on (and keep up the Issue Summary for goodness sake).
Comment #190
Crell CreditAttribution: Crell commented#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.
Comment #191
effulgentsia CreditAttribution: effulgentsia commentedPer #179, assigning to Dries.
Comment #192
pwolanin CreditAttribution: pwolanin commentedThe summary has:
which imply that the patch is still rather broken? Either that or the summary's not up to date.
Comment #193
effulgentsia CreditAttribution: effulgentsia commentedWorking on updating the summary now. If someone else has already started that, ping me in IRC so we avoid conflicts.
Comment #193.0
effulgentsia CreditAttribution: effulgentsia commentedFix link to follow-up issues.
Comment #194
pwolanin CreditAttribution: pwolanin commentedto avoid introducing an inconsistency in 8 compared to 7 likely to lead to security issues:
should probably be:
Comment #195
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedGood point. There is a follow-up to remove drupal_not_found() and drupal_access_denied() entirely. Not worth holding/updating the patch.
Comment #196
effulgentsia CreditAttribution: effulgentsia commentedFixed the summary. I think #194 is only relevant if we don't do #1587850: Replace drupal_not_found() with throw NotFoundHttpException and #1591604: Replace drupal_access_denied() with throw AccessDeniedHttpException, so let's defer that security/DX question to those issues.
Comment #197
quicksketchAnd just one last wimper before this steamrolls in anyway. Our current analysis over at #1578090: Benchmark/profile kernel says that this patch makes Drupal bootstrap 10% slower and require 20% more memory. Let's hope we get some real returns on this investment. I still find it a bit appalling that most of the performance improvements are theoretical, but if we're certain that we'll be able to get this back through ESI/block caching/PSR-0 let's move forward (and quickly). If D8 gets any slower I'm going to go bananas.
Comment #198
Dries CreditAttribution: Dries commentedI reviewed this patch and I think we should proceed with it. Obviously, this patch requires a ton of follow-up work. From reworking most of the Resolvers, to reworking the bootstrap to replace Drupal's page caching mechanism, to figuring out how to adopt Symfony's routing system. It's a lot of work that needs to be done in the next few months. There is risk in not getting it done, but delaying this patch only increases that risk. There is risk in not being able to address the performance regression, but delaying this patch only increases that risk.
Larry, I think we should plan to commit this patch on Friday between 11am and 1pm Eastern time. Would you be available to help with the merge around that time?
Comment #199
catch@quicksketch, while it hasn't been discussed properly yet, I'd personally consider a measurable performance regression from 7.x to 8.x a critical bug and would be very happy to hold the release up on it, especially considering the legion of regressions that got into Drupal 7, many of which haven't been resolved yet.
On the other hand, I'd much rather a situation where we have a reasonable understanding of what the performance issue is due to #1578090: Benchmark/profile kernel than a surprise regressions weeks or months later because no-one profiled it or did a too-superficial benchmark to find it.
Once this patch is committed we have three known regressions, each of which is well documented - the Symfony autoloader (at least in the configuration we're using), CMI having no static or persistent caching at all, and the stuff added in this patch (mainly the event dispatcher and/or the listeners). From that we can work on both trying to optimize the straight regressions in PHP from the new code added, as well as on the macro performance/scalability work which depends on much heavier refactoring elsewhere.
We should also bear in mind that (like Field API in D7), with the current state of things, some further refactoring to rely more on these systems is likely to regress performance further, for example the url() conversion proposed in #1606794: Implement new routing system rings alarm bells (although I'd love to be pleasantly surprised).
Comment #200
Crell CreditAttribution: Crell commentedIf 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.
Comment #201
Dries CreditAttribution: Dries commentedCrell: let's plan on 11am then. I'll ping you on Skype.
Comment #202
Dries CreditAttribution: Dries commentedMerged 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:
Comment #203
effulgentsia CreditAttribution: effulgentsia commentedAWESOME!!!!!
Just a reminder, for anyone who wants to help, there's already issues tagged with kernel-followup. If you're inspired by any of them, dig in. Also, if you identify follow-up work that's needed for which there isn't an issue in that queue, please add it. Even if you don't have time to work on it, please make sure the issue is there, so that others who want to help can pick something up. Thanks!
Comment #204
Fabianx CreditAttribution: Fabianx commentedNice :-)
Comment #205
sunNote: 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.
lib/Drupal/Core/
without any component?That looks very bogus to me and contradicts the entire debate on the PSR-0 structure. When I first looked, I searched for a Drupal\Core\Kernel component, but only found EventSubscriber, which obviously delivers an incomplete picture. Only after grepping the code base I found those loose and lost files directly in Drupal\Core. Very confusing.
The assumption that any XMLHttpRequest should get an AJAX response is fundamentally wrong.
This broke the most basic AHAH JavaScript functionality; i.e., a simple
$.get()
to any Drupal path (say,/js/admin_menu/cache
), which is simply supposed to run a HTTP request against the provided path and return the (HTML) response (with no AJAX framework being involved). Due to the enforcedX-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.
Closely related to the above AHAH issue - the onHtml() view event subscriber processes any kind of render array or HTML string response into a full HTML page.
How do you envision HTML fragments to be returned? (i.e., not full HTML pages)
Comment #206
aspilicious CreditAttribution: aspilicious commentedSun 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.
Comment #207
David_Rothstein CreditAttribution: David_Rothstein commentedPretty sure this issue needs a CHANGELOG.txt entry also.
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.)
So should #1578090: Benchmark/profile kernel be moved to the core issue queue and marked critical, or critical + postponed + "revisit before release" or whatever the magic incantation is these days? :)
Comment #208
sunAlso: #1617776: Fatal error: require(): Cannot redeclare class drupal\core\config\drupalconfig in UniversalClassLoader
Comment #209
sunNew follow-up: #1619446: All autocomplete and drupal_json_output() responses are broken (returning Ajax instead of JSON)
Comment #210
Crell CreditAttribution: Crell commentedI'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.
Comment #211
catchLet's open follow-ups for anything else that's not the change notifications, moving to active/task.
Comment #212
Crell CreditAttribution: Crell commentedChangelog 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.).
Comment #214
Crell CreditAttribution: Crell commented#212: 1463656-kernel-changelog.patch queued for re-testing.
Comment #215
Berdir*built?
Comment #216
BerdirAlso, I think it would be useful to have the current example of how to replace menu_execute_active_handler() in there. That is easier to keep up to date and less easily forgotten. Otherwise, we would basically need to keep a revisit before release task open for adding the final syntax. When keeping it up to date, it's part of whatever issue is going to change it.
Comment #217
Crell CreditAttribution: Crell commentedFixed the spelling mistake.
Edit: Also tweaked the change notice node.
Comment #218
BerdirI don't see a change record for the request/response objects, do we want to document them in the same one? I think it would make sense, because we are using them in the example code, without explaining them first. Also, I think there isn't one yet for the whole listener thing, right? That could probably go into a separate one?
The "not yet documented" sentence is also still there, I guess that can be removed. Not trying to be picky about the new code example, as it's very likely going to be changed but maybe add an inline comment or two and use some temporary variables to make the lines shorter? There are also some variables in there that aren't explained or defined, like $system_path (I guess that's current_path() or whatever the corresponding method of $request is?)
Comment #219
Crell CreditAttribution: Crell commentedI 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? :-)
Comment #220
webchickCommitted and pushed to 8.x. Thanks!
Comment #221
webchickFixing the title.
Comment #222
BerdirI do not want to discuss the code syntax (or even architecture) here, sorry if I was unclear.
As far as I understand change records is that they should be complete in the way that every change is documented, as short as possible, with *understandable* before/after code samples (if applicable). Have a look at http://drupal.org/node/1400186 (entity classes, which evolved over a dozen issues or so) or http://drupal.org/node/1534648 (cache tag api). And the current change record in my opinion is just plain simply not understandable to someone that is new to the API (which is the target audience of change records).
To quote sun from #205: "This needs a full stack of change notifications, so bumping to critical.". Also David Rothstein in #207.
The new Request class will, AFAIK, affect *everyone*, becausing that will be used instead of _GET/_POST/..., right? That's a huge change and it's currently not mentioned at all (yes, it's *used* in the code example there, but that makes it IMHO worse, not better ;)) . Compared to that, the removal of meah() is completely irrelevant. Am I missing something?
That's all I'm trying to say. I'm just confused to see this waved through the (IMHO important) change record/CHANGELOG process, compared to most other issues, even though this is one of the biggest changes so far in 8.x.
Comment #223
webchickThat's a good point. Marking needs work for the change notice.
Comment #224
Crell CreditAttribution: Crell commentedBerdir 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. :-)
Comment #225
Mark TrappFrom the change notice:
Can links to these other change notices be provided? I'm not sure how people should know which change notices are supposed to be relevant.
Comment #226
Crell CreditAttribution: Crell commentedA 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.
Comment #227
Mark TrappCrell: 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:
as part of the block regarding
menu_execute_active_handler()
's removal, it suggests to me that there are specific implications related to replacingmenu_execute_active_handler()
that are captured in this change that I should be aware of, but for whatever reason, are mentioned in other specific change notices.Having not been involved in the development of this change, I have no idea what those specific implications are nor which specific change notices are being alluded to. I can't fix or update the change notice because I don't know what the implications or change notices are to which this notice refers. I could read every single change notice, but again, having not been intimately involved in the development of the kernel, I don't know what notices contain the implications alluded by this change notice.
That is, a red flag has been raised, but I don't have the necessary information to resolve it. If there were specific change notices in mind when that line was written, I don't think it's unreasonable to expect those to be linked, similar to how the request object documentation was linked below the first code block couplet.
However, if I understand you correctly, it sounds as though that perhaps there aren't specific implications that people need to be aware of, but merely general implications of replacing a massive part of Drupal's internals in the same way that upgrading to a new major version of Drupal always creates implications for development or upgrading. If that's the case, the line seems superfluous at best and confusing at worst.
I'm a non-core developer, and up to this point, I thought I was part of the intended audience for these change notices: to be able to get an overview of a change and its implications in one place without having to have intimate knowledge of every single thing that was said or written about the change while it was in development. If i'm wrong, I'll shut up and you can ignore this.
Comment #228
Crell CreditAttribution: Crell commentedThere 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.
Comment #229
BerdirI think one big thing that is still missing are the response objects, not sure if that should be in the same or a new change record. Also wondering how sun's concerns in #205 are going to be addressed, follow-up(s)? (the response related follow-up could also have the goal to describe the response related changes in a change record).
Comment #230
Crell CreditAttribution: Crell commentedAs 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.
Comment #232
podarokvote for the #230
Comment #234
xjmComment #235
xjmComment #235.0
xjmUpdated Remaining Tasks section.