Closed (fixed)
Project:
Drupal core
Version:
8.0.x-dev
Component:
routing system
Priority:
Major
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
17 Nov 2014 at 18:32 UTC
Updated:
5 Dec 2014 at 09:14 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
dawehnerThere we go.
Comment #2
tim.plunkettI guess this is a dupe of #2092647: Consolidate _form, _controller, _entity_form, etc. into just _controller??
Didn't you post both of those patches!?
Comment #3
dawehnersee #2092647-21: Consolidate _form, _controller, _entity_form, etc. into just _controller?
Comment #4
fabianx commentedTagging major, setting parent
Comment #5
fabianx commentedAnd RTBC, +1 to this change :)
Comment #6
catchWhat about the bc subscriber mentioned in #2092647-25: Consolidate _form, _controller, _entity_form, etc. into just _controller? - that'd make it very, very easy to rip out the bc layer later. If there's a good reason not to do that, fine with that too as long as there's clear comments on what's for bc and what isnt'.
Comment #7
dawehnerThis BC layer is already in core:
\Drupal\Core\EventSubscriber\ContentControllerSubscriber::onRequestDeriveController(). The nameof it currently is quite fine, if you ask me. The other method in that subscriber thought, might in the wrong place.
Let's rip of the BC layer and see what happens.
Comment #9
wim leersGlad we did #7, that's how we found those failures… and AFAICT they're because
EntityRouteEnhanceret al. set_contentinstead of_controller, we need to update those too :) Also updated docs in a few places.Comment #10
Crell commentedIf I read this correctly, it removes support for _content but doesn't remove the code that would handle it? So we're leaving the BC code in place but not using it...?
catch: Do you want to leave the BC layer in for _content (ie, carry around something we're not using or recommending for the entire D8 cycle), rip it out entirely (ie, a hard BC break in a beta, even if a trivial one to adjust), or leave it in for one beta only to give people time to adapt existing code and then kill it?
I would favor option 3 (keep for one beta) but defer to you on the final decision there.
Comment #11
fabianx commented#9: Did you grep the code base for any more occurrences of _content?
Comment #12
dawehnerHa, I remember when doing the intitial patch, I have seen those instances and just thought ... screw it, i'll do that later :)
Found some more docs instances.
Comment #13
wim leers#10: AFAICT we only removed
_contentsupport temporarily (in #7) to ensure that it's indeed only there for BC. That's how we found some things were still using_content. Since that's now fixed, I think the plan is now to restore that (revert the interdiff of #7). Please confirm, dawehner?#9: Yes, but clearly not good enough, since #12 found some more.
#12: good catch :)
Comment #14
catchI've opened #2377967: Remove bc layer for _content _controller change for the actual bc layer removal, let's leave it in for this issue - let's the patch get in without having to resolve that. Also I think it's friendlier to contrib ports to have one commit where you can use the new thing, then a later commit where you can't do the old thing even if that's just a day or so.
Comment #15
wim leersThis is a straight reupload of #12 minus the changes in
ContentControllerSubscriber(which removed BC), to keep BC. Manually removed those hunks.Comment #16
Crell commentedOK, let's do.
Comment #17
dawehner+1 to wims last patch.
Comment #19
catchOK so this is minorly disruptive to contrib modules (they'll have to make the same change to routing YAML as were made in this patch), but it's very unlikely to affect other changes in core since we don't touch routing YAML very often and the other changes are internal to the routing system.
Also from an impact point of view it reduces the number of things to learn in the new routing YAML, and a lot of modules haven't ported yet, so it'll be easier in the long run.
I've updated https://www.drupal.org/node/2119699 and https://www.drupal.org/node/2224207 for change notices. That's all I could find with _content although was a quick look.
Before we do #2377967: Remove bc layer for _content _controller change we should probably have a draft 8-8 change notice and also update the handbook docs.
Comment #20
yukare commentedJust a question on this, What happens with all elements in the page ?
Because _controller render only the content you put inside(i use it to return a css file) but _content return everything(blocks/menus/theme).
Comment #21
Crell commentedThat distinction is no longer there. What happens now depends on the mime type of the request. text/html means "use this as the body of a themed page" (ie, the 90% use case). But you can still return a Response object yourself and skip all additional rendering, regardless of the output. (That was true for both _content and _controller before.)
Comment #22
wim leersYep, if you want to control the entire response, just return a
Responseobject, that still works in exactly the same way. What's changed is that we detect if a render array was returned instead of aResponse, and if so, turn that render array into aResponse.