RouteProcessorSubscriber is quickly getting crufty. Especially after overlay and SCOTCH get moved over to the new model, it's going to be really gross.
So instead, let's move all of that logic to route enhancers, which is what RouteProcessorSubscriber really is. It just predates us having route enhancers. :-)
This patch (as soon as I have a nid) should change no functionality, but removes a hard coded list in favor of an easily-extensible mechanism that's easier to read. I think that's a win.
Follow Ups
Comment | File | Size | Author |
---|---|---|---|
#44 | drupal-1938980-44.patch | 21.79 KB | dawehner |
#44 | interdiff.txt | 2.19 KB | dawehner |
#42 | 1938980-dynamic-controllers-b.patch | 21.25 KB | Crell |
#42 | interdiff.txt | 782 bytes | Crell |
#38 | interdiff.txt | 4.46 KB | dawehner |
Comments
Comment #1
Crell CreditAttribution: Crell commentedAnd patch.
Comment #2
pounardThis sounds weird.
Comment #3
Crell CreditAttribution: Crell commentedThat's the code that's already there, from the Ajax Form issue. This patch is just moving it around. I agree that it looks weird. :-) I asked mkadin to weigh in there, although I don't know if we want to refactor that in this issue or later.
Comment #4
tim.plunkettWe must be missing tests :(
You register
route_enhancer.page
twice, first for the Ajax one and then overwrite it.Comment #5
Crell CreditAttribution: Crell commentedNot only were we missing tests, we were *really* missing them. The code in the AjaxEnhancer only worked for legacy ajax requests, not new ones. Oopsies.
I've adjusted it to handle both cases, and added tests to confirm same.
Comment #7
Crell CreditAttribution: Crell commentedOK, let's be a little less aggressive with simpletest for now...
Comment #8
larowlanCorrect me if I'm wrong but this lets us effectively nominate two controllers, as the 'real' controller can be passed as the 'content'. As I found out over in #1944472: Add generic content handler for returning dialogs, this currently isn't the case with Ajax routes, where it expects the _controller key, if that isn't present - it returns a not found. So +1 to this approach.
Needs to be reverted
Marking needs work because of the inadvertent changes to the info.yml file.
Comment #9
Crell CreditAttribution: Crell commentedYou know git, I'm really starting to get pissed off at you and the way you keep including things in my patches that I tell you not to...
Comment #10
larowlanThe code looks great to me, and cleans up what was fast becoming a mess.
Manual install works fine.
Manually installed and tested ajax example module to ensure that we don't loose ajax functionality - works as expected.
This blocks #1944472: Add generic content handler for returning dialogs so getting this in frees that up, which in turn will improve DX around the new dialog/modal API.
Just some minor coding standards nitpicks then this is rtbc.
This doesn't match the doc standards at http://drupal.org/coding-standards/docs#file
same
same
Comment #11
ParisLiakos CreditAttribution: ParisLiakos commentedFixed all @file docblocks, no commit credit plz:)
Comment #12
ParisLiakos CreditAttribution: ParisLiakos commented* Contains Drupal\router_test\TestContent.
stupid backslash
Comment #13
larowlanassuming bot comes back green
Comment #14
sdboyer CreditAttribution: sdboyer commentedoverall, definitely +1 to this patch. establishing standards that anything which enhances/mutates the route should happen during the true "routing" process will allow us to do things like, say, mock just the routing portion of request handling but still be reasonably sure that we've gotten exactly the same fully-realized route as we would have with a full kernel handleRaw() pass.
the only drawback - by moving these out of a single class and into their own discrete classes, we are making it just a little harder for people to see the big picture of everything that's happening during the routing process. at least, without diagrams. still, that's preferable to having just a single service that ties together multiple not-really-related paradigms, as that would create an awkward overriding situation for other systems that wanted to target just one of the content detection + controller-setting services.
so, yeah, RTBC+1.
Comment #15
tim.plunkettI started using this in #1913618: Convert EntityFormControllerInterface to extend FormInterface and noticed a couple coding standards issues. Leaving RTBC.
Comment #16
Crell CreditAttribution: Crell commentedConfirmed, still RTBC. Can haz commitz?
Comment #17
xjm#15: enhancers-1938980-15.patch queued for re-testing.
Comment #18
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks.
Comment #19
moshe weitzman CreditAttribution: moshe weitzman commentedDuring my review, I expressed a little grumpiness that we are testing this with
$this->drupalGet('/router_test/test10');
. If we want to just test the routeEnhancers, we could mock up a route and then render the page without doing any http. Then we assert that the output matches our spec. This would help show the value of all this abstraction that we have added. We'd have to find a more targeted way to the Ajax route enhancer$this->assertEqual($this->drupalGetHeader('Content-Type'), 'application/json', 'Correct mime content type was returned');
Comment #20
xjmAnother followup would be to document the meaning of
$defaults
somehow, and other oddities on this class--Alex said we are inheriting an oddly named variable from the Symfony RouteEnhancer interface, which says:(Uh. K?) Alex suggested maybe defining a DruaplRouteEnhancerInterface to help the DX (until we possibly add improvements upstream). Alex also mentioned that there's more a general issue around improving the DX here--can someone clarify what issue that is?
Comment #21
larowlanWrt Ajax tests, #1944472: Add generic content handler for returning dialogs adds some
Comment #22
swentel CreditAttribution: swentel commentedThis broke the formatter settings on 'Manage display' screens, spotted by larowlan already, but just wanted to conform the fact that it's broken.
Comment #23
xjm#22 sounds critical; should we revert this?
Comment #24
dawehnerThe issue is basically, that there is no routing definition of system/ajax yet, but there simply has to exist an issue for that already?
Comment #25
xjmDiscussed in IRC. This affects a number of forms (Views, Field UI) and looks to be non-trivial to fix, plus it's missing test coverage, whatever it is.
git revert 221099b5
Comment #26
larowlanThis looks like test coverage - what's going on here?
http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...
Comment #27
larowlanSo that test coverage passes so we definitely have an issue with WebTestBase::drupalPostAJAX().
Comment #28
larowlanSo here's a patch to verify the fail.
WebTestBase::drupalPostAJAX() doesn't use correct Accept header.
Then for the fix, incorporates #1954882: Convert system/ajax to a route patch by @dawehner (make sure he gets some credit if this gets committed)
As noted there, this leaves hook_menu's theme_callback orphaned (See #1954892: Replace 'theme callback' and hook_custom_theme() with a clean theme negotiation system)
note the RTBC was for the rollback, leaving it at that so it get's on committer's radar Note this patch is not RTBC only the revert.
Comment #29
webchickI reverted this temporarily, so we could work on a proper fix + test coverage. Clean-up phase is not the time to be introducing critical bugs. :)
Back to a normal, needs review, task.
Comment #30
dawehnerThe following calls have to be adapted: FileInclusionTest.php RebuildTest.php
Comment #31
dawehnercrosspost, sorry.
Comment #32
dawehnerIt seems to be for me that we could simplify this elseif and else into one single statement.
Comment #34
sunComment #35
dawehnerThis patch combines #15 with #28
Comment #37
dawehnerWow there are currently three different ways how an ajax request is built:
This still breaks though it should be way less.
Comment #38
dawehnerWith the interdiff as well.
Comment #40
moshe weitzman CreditAttribution: moshe weitzman commentedStill hoping for a test which does not use http. I'll paraphrase my earlier comment "Sad that we are testing this with $this->drupalGet('/router_test/test10');. If we want to just test the routeEnhancers, we could mock up a route and then render the page without doing any http. Then we check that the Response output and http headers match our spec. This would help show the value of all the kernel abstraction that we have added. "
Comment #41
andypostAlso unittesting should help to profile this change, I mostly care about sub-requests. Is this change executes more code?
Comment #42
Crell CreditAttribution: Crell commentedThe field form tests were simple. We don't need to prepare() the response in the controller. It will get prepare()d on its own later, and calling prepare() was causing it to get rendered twice which was throwing off our tests (which are supposedly functional tests but are checking specific array keys. Shoot me now...) Patch attached to fix that.
Digging through the other issues, it looks like the problem is that when an #ajax array is returned (at least in the ajax form test module) it gets rendered to an empty string. I suspect that's because FormAjaxController should be doing that rendering itself rather than relying on ViewSubscriber calling drupal_render(), which doesn't know what to do with #ajax. I think the fix is to make FormAjaxController smarter, possibly merging with AjaxController. I don't have time to work on that right now, but posting the current patch so that someone else can dig into it. If not, I'll try and have a crack at it tonight.
Comment #44
dawehnerDecided to remove the workaround in AjaxController: AjaxController should either get the ajax response or a usual drupal legacy render array.
If we put this logic onto the FormAjaxController we check exactly what's returned by #ajax callbacks, which seems to be the better place for that. Urgs, that's all not really nice.
One big advantage would be certainly to start converting all remaining procedural ajax commands
and get rid of '#type' => 'ajax'.
Comment #45
dawehnerSee #1957590: Remove remaining procedural ajax command usages. to remove all old calls.
Comment #47
dawehnerAfter debugging for a while here is the reason:
drupal_get_js() is used to determine the js/css changes and put it into the ajaxPageState.
In D8 (without this patch) this code is called during rendering.
in D8 with this patch, this code is called in the AjaxController after $http_kernel->forward(), which means,
that the scope of the container is not 'request' anymore, which means that we don't have the right request object anymore.
One workaround which didn't really worked out was to manually set the request context back in the AjaxController, because prepare() is called on the main request not the subrequest.
Comment #48
larowlan@dawehner - note also #1941288: Regression: ajaxPageState not being updated with AjaxResponse, assets (js/css) being added twice - seems like some overlap
Comment #49
dawehnerOh I see, so we first have to fix the other issue, urgs.
Comment #50
larowlanThis is blocking #1913618: Convert EntityFormControllerInterface to extend FormInterface and #1944472: Add generic content handler for returning dialogs
Comment #51
Crell CreditAttribution: Crell commentedI've split the original intent of this issue off to a new thread, so that we can move forward: #1963470: Move controller derivation to route enhancers (and just that).
Once the simple bits are in, this can be just about fixing the ajax system.
Comment #52
Crell CreditAttribution: Crell commentedThe other issue is in, so retitling.
Comment #53
mkadin CreditAttribution: mkadin commentedRemoving the old ajax system has been ongoing here #1959574: Remove the deprecated Drupal 7 Ajax API and is now green. Should we close this then?
Comment #54
Crell CreditAttribution: Crell commentedYeah, we can finis up the cleanup there.
Comment #54.0
Crell CreditAttribution: Crell commentedAdded follow up