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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Crell’s picture

Status: Active » Needs review
FileSize
9.73 KB

And patch.

pounard’s picture

+      $defaults['_content'] = $defaults['_controller'];
+      $defaults['_controller'] = '\Drupal\Core\AjaxController::content';

This sounds weird.

Crell’s picture

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

tim.plunkett’s picture

Issue tags: +Needs tests

We must be missing tests :(

You register route_enhancer.page twice, first for the Ajax one and then overwrite it.

Crell’s picture

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

Status: Needs review » Needs work

The last submitted patch, 1938980-dynamic-controller-cleanup.patch, failed testing.

Crell’s picture

Status: Needs work » Needs review
FileSize
13.51 KB

OK, let's be a little less aggressive with simpletest for now...

larowlan’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Routing/Enhancer/AjaxEnhancer.phpundefined
@@ -0,0 +1,48 @@
+        $defaults['_controller'] = '\Drupal\Core\AjaxController::content';

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

+++ b/core/modules/rest/tests/modules/rest_test_views/rest_test_views.info.ymlundefined
@@ -6,4 +6,4 @@ core: 8.x
+#hidden: true

Needs to be reverted

Marking needs work because of the inadvertent changes to the info.yml file.

Crell’s picture

Status: Needs work » Needs review
FileSize
13.08 KB

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

larowlan’s picture

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

+++ b/core/lib/Drupal/Core/Routing/Enhancer/AjaxEnhancer.phpundefined
@@ -0,0 +1,48 @@
+ * @File Contains \Drupal\Core\Routing\Enhancer\AjaxEnhancer.

This doesn't match the doc standards at http://drupal.org/coding-standards/docs#file

+++ b/core/lib/Drupal/Core/Routing/Enhancer/FormEnhancer.phpundefined
@@ -0,0 +1,41 @@
+ * @File Contains \Drupal\Core\Routing\Enhancer\FormEnhancer.

same

+++ b/core/lib/Drupal/Core/Routing/Enhancer/PageEnhancer.phpundefined
@@ -0,0 +1,39 @@
+ * @File Contains \Drupal\Core\Routing\Enhancer\PageEnhancer.

same

ParisLiakos’s picture

Fixed all @file docblocks, no commit credit plz:)

ParisLiakos’s picture

* Contains Drupal\router_test\TestContent.
stupid backslash

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

assuming bot comes back green

sdboyer’s picture

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

tim.plunkett’s picture

FileSize
4.92 KB
13.34 KB

I started using this in #1913618: Convert EntityFormControllerInterface to extend FormInterface and noticed a couple coding standards issues. Leaving RTBC.

Crell’s picture

Confirmed, still RTBC. Can haz commitz?

xjm’s picture

#15: enhancers-1938980-15.patch queued for re-testing.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

moshe weitzman’s picture

During 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');

xjm’s picture

+++ b/core/lib/Drupal/Core/Routing/Enhancer/PageEnhancer.phpundefined
@@ -0,0 +1,43 @@
+  public function enhance(array $defaults, Request $request) {
+    if (empty($defaults['_controller']) && !empty($defaults['_content']) && $this->negotiation->getContentType($request) === 'html') {
+      $defaults['_controller'] = '\Drupal\Core\HtmlPageController::content';
+    }
+    return $defaults;

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

     * @param array $defaults the getRouteDefaults array                        
     *                                                                          
     * @return array the modified defaults. Each enhancer MUST return the $defaults but may add or remove values                                               
     */

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

larowlan’s picture

swentel’s picture

This broke the formatter settings on 'Manage display' screens, spotted by larowlan already, but just wanted to conform the fact that it's broken.

HTTP Result Code: 415
Debugging information follows.
Path: /system/ajax
StatusText: Unsupported Media Type
ResponseText: Unsupported Media Type
xjm’s picture

#22 sounds critical; should we revert this?

dawehner’s picture

The issue is basically, that there is no routing definition of system/ajax yet, but there simply has to exist an issue for that already?

xjm’s picture

Title: Move controller derivation to route enhancers » [Regression, needs rollback] Move controller derivation to route enhancers
Assigned: Crell » Unassigned
Category: task » bug
Priority: Normal » Critical
Status: Fixed » Reviewed & tested by the community

Discussed 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

larowlan’s picture

This looks like test coverage - what's going on here?
http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...

larowlan’s picture

So that test coverage passes so we definitely have an issue with WebTestBase::drupalPostAJAX().

larowlan’s picture

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

webchick’s picture

Title: [Regression, needs rollback] Move controller derivation to route enhancers » Move controller derivation to route enhancers
Category: bug » task
Priority: Critical » Normal
Status: Reviewed & tested by the community » Needs review

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

dawehner’s picture

Title: Move controller derivation to route enhancers » [Regression, needs rollback] Move controller derivation to route enhancers
Category: task » bug
Priority: Normal » Critical
Status: Needs review » Reviewed & tested by the community

The following calls have to be adapted: FileInclusionTest.php RebuildTest.php

dawehner’s picture

Title: [Regression, needs rollback] Move controller derivation to route enhancers » Move controller derivation to route enhancers
Category: bug » task
Priority: Critical » Normal
Status: Reviewed & tested by the community » Needs review

crosspost, sorry.

dawehner’s picture

+++ b/core/lib/Drupal/Core/Routing/Enhancer/AjaxEnhancer.phpundefined
@@ -41,6 +41,10 @@ public function enhance(array $defaults, Request $request) {
+    elseif (!empty($defaults['_controller']) && empty($defaults['_content']) && $this->negotiation->getContentType($request) === 'drupal_ajax') {
+      $defaults['_content'] = $defaults['_controller'];
+      $defaults['_controller'] = '\Drupal\Core\AjaxController::content';
+    }
     else {
       if (empty($defaults['_controller']) && !empty($defaults['_content']) && $this->negotiation->getContentType($request) === 'drupal_ajax') {

It seems to be for me that we could simplify this elseif and else into one single statement.

Status: Needs review » Needs work

The last submitted patch, route-enhancer-1938980.28.pass_.patch, failed testing.

sun’s picture

Title: Move controller derivation to route enhancers » Move controller derivation to route enhancers
dawehner’s picture

Status: Needs work » Needs review
FileSize
18.34 KB

This patch combines #15 with #28

Status: Needs review » Needs work

The last submitted patch, drupal-1938980-35.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
20.74 KB

Wow there are currently three different ways how an ajax request is built:

  • You return a AjaxResponse object
  • You return an array of array('#type' => 'ajax', ...)
  • You return just the changed html

This still breaks though it should be way less.

dawehner’s picture

FileSize
4.46 KB

With the interdiff as well.

Status: Needs review » Needs work

The last submitted patch, drupal-1938980-37.patch, failed testing.

moshe weitzman’s picture

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

andypost’s picture

Also unittesting should help to profile this change, I mostly care about sub-requests. Is this change executes more code?

Crell’s picture

Status: Needs work » Needs review
FileSize
782 bytes
21.25 KB

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

Status: Needs review » Needs work

The last submitted patch, 1938980-dynamic-controllers-b.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.19 KB
21.79 KB

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

dawehner’s picture

Status: Needs review » Needs work

The last submitted patch, drupal-1938980-44.patch, failed testing.

dawehner’s picture

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

    if ($container->has('content_negotiation') && $container->isScopeActive('request')) {
      $type = $container->get('content_negotiation')->getContentType($container->get('request'));
    }

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.

larowlan’s picture

dawehner’s picture

Oh I see, so we first have to fix the other issue, urgs.

larowlan’s picture

Crell’s picture

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

Crell’s picture

Title: Move controller derivation to route enhancers » Fix Ajax system; the last remnants of the old API must be swept away

The other issue is in, so retitling.

mkadin’s picture

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

Crell’s picture

Status: Needs work » Closed (duplicate)

Yeah, we can finis up the cleanup there.

Crell’s picture

Issue summary: View changes

Added follow up