Use arguments resolver in RequestHandler to have consistent parameter ordering

+1 for the principle.

why is it error prone? how can the controller resolver help us? what kind of issue are we fixing here?

+++ b/core/modules/rest/src/RequestHandler.php
@@ -69,24 +98,32 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
+      if (!empty($definition['use_controller_resolver'])) {

why is this configurable? Can't we do it always?

Otherwise great idea, we should get rid of the code in the else{} clause.

IOW: RequestHandler passes arguments in whatever order they came in, not in the order that is expected.

Right. but my interpretation/explanation is correct, right?

Aha, that would make sense! Then this is more of a bug report and we should just replace the brittle part in RequestHandler.

Wrong patch?

\Drupal\Tests\rest\Kernel\RequestHandlerTest needs to be updated, then tests will pass again. That seems like a great novice task.

+++ b/core/modules/rest/src/RequestHandler.php
@@ -70,24 +99,17 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
+      $request = clone $request;

why do we clone the request here? Please add a comment.

And the issue summary update is still missing.

#19: can you point to which variable is being renamed, which is the BC break?

I understand now. Thanks for explaining.

I have no idea how to provide BC for that.

Bluntly speaking: this is a plain bugfix, and any existing code relying on non-matching-variable names is therefore buggy too.

One alternative could be to see whether the controller resolver finds every parameter. In case it does, use it, otherwise fallback to the previous logic?

Sounds great!

Do we still think this is worth doing?

Also: this is more an improvement than fixing an actual bug, so changing to Task. Finally, this seems very much un-novice :)

the BC policy makes some things just impossible hard.

Hear hear.

Although I think that in this case, there may be a simple small oversight in the test coverage that's causing the failures? :)

Let's see…

First: straight rebase.

Turns out there's just a few trivial bugs in #25 :)

But even with these changes, you only the legacy approach works:

 core/modules/rest/src/RequestHandler.php | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/core/modules/rest/src/RequestHandler.php b/core/modules/rest/src/RequestHandler.php
index 54d9d87..4fb6271 100644
--- a/core/modules/rest/src/RequestHandler.php
+++ b/core/modules/rest/src/RequestHandler.php
@@ -121,12 +121,12 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
     // Determine the request parameters that should be passed to the resource
     // plugin.
     $argument_resolver = $this->getArgumentResolver($route_match, $unserialized, $request);
-    try {
-      $arguments = $argument_resolver->getArguments([$resource, $method]);
-    }
-    catch (\RuntimeException $exception) {
+//    try {
+//      $arguments = $argument_resolver->getArguments([$resource, $method]);
+//    }
+//    catch (\RuntimeException $exception) {
       $arguments = $this->getLegacyParameters($route_match, $unserialized, $request);
-    }
+//    }
 
     // Invoke the operation on the resource plugin.
     $response = $response = call_user_func_array([$resource, $method], $arguments);

Well … http://buytaert.net/making-drupal-upgrades-easy-forever basically says we can't do that. At minimum, we need to already implement it now, in parallel.

I'm honestly convinced that this approach works. We just need to figure out what's broken in the current ArgumentResolver code path. I think you know that far better than I do — could you do that?

but the name in the route is called like the entity type.

We can change \Drupal\rest\Plugin\rest\resource\EntityResource::getBaseRoute() to set $parameters['entity'] instead — this is an internal detail, not an API.

fair, but we also have to deal with $original_entity for example.

:( You're right of course.

But I don't see how the current logic can ever work correctly. For GET+DELETE, $entity is the stored entity. For PATCH+POST, $entity is the entity that is being sent in the request body. If I fix that, and fix the wildcard argument isInstance() exception, then this works.

I think this is simpler to understand.

From 127 to 3 fails, yay! Assigning to @dawehner to get him to review my changes and hopefully confirm that it's the right direction. And then perhaps he can drive it home too :)

Fixed a typo, I'm sorry, I'm watching this issue since a long time, and it hurts my eyes everytime :)

@pounard++ :D — I can't believe I didn't see that myself!

@dawehner++ for driving this home! I was so tempted to RTBC this, but I think we need to clean this up just a little bit more.

1. +++ b/core/modules/rest/src/RequestHandler.php
   @@ -119,17 +121,16 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
   +    $argument_resolver = $this->getArgumentResolver($route_match, $unserialized, $request);
   +    try {
   +      $arguments = $argument_resolver->getArguments([$resource, $method]);
   +    }
   +    catch (\RuntimeException $exception) {
   +      $arguments = $this->getLegacyParameters($route_match, $unserialized, $request);
        }
   

   We want this to have a @todo to only keep the "try" case in D9.

2. +++ b/core/modules/rest/src/RequestHandler.php
   @@ -139,4 +140,91 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
   +   * Creates an argument resolver, which can pass along the REST parameters.
   

   Probably needs rephrasing.

3. +++ b/core/modules/rest/src/RequestHandler.php
   @@ -139,4 +140,91 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
   +  /**
   +   * @param \Drupal\Core\Routing\RouteMatchInterface $route_match
   +   *   The route match
   +   * @param mixed $unserialized
   +   *   The unserialized data.
   +   * @param \Symfony\Component\HttpFoundation\Request $request
   +   *   The request.
   +   *
   +   * @return array
   +   */
   

   We want to finish the docblock here: add the missing first line, add the missing trialing period, add @return line, and … mark it @deprecated! <3

I finally did the IS update that #5 requested. I also updated the title because this is no longer about using controller resolver, but about using the arguments resolver.

In doing that, I realized that we need to remove the "fall back to legacy approach" at some point, which means we need a CR and we probably want to nudge users in the right direction by …

+++ b/core/modules/rest/src/RequestHandler.php
@@ -119,17 +121,16 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
+    try {
+      $arguments = $argument_resolver->getArguments([$resource, $method]);
+    }
+    catch (\RuntimeException $exception) {
+      $arguments = $this->getLegacyParameters($route_match, $unserialized, $request);
     }

… logging a warning in the case of the "catch" case here.

+++ b/core/modules/rest/src/RequestHandler.php
@@ -119,17 +121,16 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
-    $response = call_user_func_array([$resource, $method], array_merge($parameters, [$unserialized, $request]));
+    $response = $response = call_user_func_array([$resource, $method], $arguments);

I might miss something but is it a reason to double the "$response ="?

#56: I just wanted to post that same remark :)

See #2870649-12: REST should respond with a 409 for a POST request to an existing entity — this is a blocker for that issue.

Using legacy menu arguments when the resolver throws exceptions is a very good move, I was looking for a solution in a custom non Drupal project and you just gave it to me ! Thank you very much for this.

@pounard :)

@dawehner 👍

1. +++ b/core/modules/rest/src/RequestHandler.php
   @@ -139,4 +141,99 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
   +
   +      // Try to find a parameter which is an entity.
   +      foreach ($route_arguments as $value) {
   +        if ($value instanceof EntityInterface) {
   +          $upcasted_route_arguments['original_entity'] = $value;
   +        }
   +      }
   +    }
   +    else {
   +      // Try to find a parameter which is an entity.
   +      foreach ($route_arguments as $value) {
   +        if ($value instanceof EntityInterface) {
   +          $upcasted_route_arguments['entity'] = $value;
   +        }
   +      }
   +    }
   

   1. Should this break when it finds an entity? If not why not?

   2. Should we find the entity before the if/else, then just assign it in the two code branches?

2. +++ b/core/modules/rest/src/RequestHandler.php
   @@ -139,4 +141,99 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
   +   * @deprecated in Drupal 8.0.0, will be removed before Drupal 9.0.0. Use the ¶
   

   8.4.0. Also trailing whitespace. Both fixable on commit but while I'm here.

#66 looks great. It introduced one whitespace bug though, fixed that.

Looks good and is some nice clean up - couple of observations

1. 
   ...
   +      @trigger_error('Passing in arguments the legacy way is deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0. Provide the right parameter names in the method, similar to controllers. See https://www.drupal.org/node/2894819', E_USER_DEPRECATED);
   ...
   +   * @deprecated in Drupal 8.4.0, will be removed before Drupal 9.0.0. Use the
   +   *   argument resolver method instead, see ::createArgumentResolver().
   

   We need to reference the change notice here.

2. +++ b/core/modules/rest/src/RequestHandler.php
   @@ -139,4 +141,98 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
   +    $parameters = [];
   +    // Filter out all internal parameters starting with "_".
   +    foreach ($route_parameters as $key => $parameter) {
   +      if ($key{0} !== '_') {
   +        $parameters[] = $parameter;
   +      }
   +    }
   

   any reason we didn't use array_filter here?

1. Unless I'm mistaken, after triple-checking that, that's already the case in the trigger_error() call? Oh you mean in the docblock!
2. Because we're moving existing code. See the second hunk: this exact code is being moved away from there.

   (If we're moving legacy code into a deprecated helper method that we're going to remove in the future, you have every reason possible to not change it: out of scope change, avoid risk of introducing new edge cases, and so on.)

Thanks, agree

Credited catch for performance improvements.

Committed as 5679964 and pushed to 8.5.x

Published change record.

Would welcome a follow-up to use array_filter as per #69.2

Fixed some version details in the change notice. I think it can be improved. @Wim Leers can you please have a look at it? Thanks!

Somehow this causes test fails in monitoring:

https://www.drupal.org/node/2907681#comment-12253309

What I'm doing is kinda strange (a hack on a rest plugin to support returning a list), but still seems strange that this broke that.

Thanks @dawehner for the help.

I fixed this now in monitoring in #2908002: Fix HEAD test fails in REST tests.

The question is whether this is a regression/BC break and whether we care enough about that to revert this? I'm OK if we don't, but I'd suggest we should at least document this case then in the change record.

Being able to specify the method of the resource plugin for a route would be quite nice and a nicer workaround than what I did until we have #2100637: REST views: add special handling for collections (I think that's the issue for official list/collection output in rest resources) but that's a separate issue.

Being able to specify the method of the resource plugin for a route

Can you clarify this a bit? Can you not already do this by overriding \Drupal\rest\Plugin\ResourceBase::getBaseRoute()?

What he means is this line:

$method = strtolower($request->getMethod());

The problem is you can define whatever you want in your routes.. the concept that http method == php methed on the RestResource plugin is hardcoded and basically impossible to change without duplicating the whole method there.

Instead, it would like this:

$method ? $route_match->getRouteObject()->getOption('_rest_method') ?: strtolower($request->getMethod());

Then I could just say _rest_method = 'list' in my routes() method and wouldn't need my current ugly hacks.

Ah, I see! That's indeed one of the more questionable design decisions about Resource(Interface|Base) + RequestHandler.

I do find _rest_method = 'list' confusing too though. The REST module basically has spent no time/thought on how to deal with collections. AFAICT it deferred that entirely to Views. Which is often undesirable or overkill of course. I think the way you implemented it (http://cgit.drupalcode.org/monitoring/tree/src/Plugin/rest/resource/Moni... + http://cgit.drupalcode.org/monitoring/tree/src/Plugin/rest/resource/Moni...), with a single get() responsible for both individual and collection GET requests with an optional parameter makes total sense. Shall we discuss that further in a follow-up, to officially support that pattern? So that you don't need those routes() overrides anymore?

Then we could at least start having basic collection GET support. If you're +1, either create a follow-up or comment here and I'll create that follow-up.

Opening an issue certainly makes sense, but I'm not sure that it should be like I did with monitoring. For one thing, configuration is currently tied to methods, what if a resource doesn't support lists or only supports lists, or supports both but you only want to enable one of the two things? But we can discuss that in a dedicated issue. I already mentioned #2100637: REST views: add special handling for collections above, not sure if that *is* that issue but it's certainly related?

No, that issue is specifically about using REST views for collections, what we're talking about is about specifically not using the views module for retrieving collections via REST.

I created #2908800: RestResource plugins: use get() method with optional parameter (or no parameter) to signal list support? to discuss this, and posted my concrete proposal in #2908800-2: RestResource plugins: use get() method with optional parameter (or no parameter) to signal list support?. Let's continue the discussion there :)

I also updated #2893804, #2893804-5: Remove rest.module BC layers includes the interdiff to remove the BC layers introduced in this issue/patch.

Looks like this caused a regression: #2954171: Custom REST resource plugin's POST not working after upgrade 8.4.5->8.5.

It's been confirmed at #2954171: Custom REST resource plugin's POST not working after upgrade 8.4.5->8.5 that this caused a regression for everybody who has

public function post(array $data = []) 

They need to change it to:

public function post(array $data) 

Besides needing to look into why exactly this is broken, I am very surprised so many people have the exact same code. Plus, it doesn't even make sense: a default value means you're able to do a POST request without sending any data!

Apparently #49 introduced that and none of us questioned that change. The reason probably being:

Plus, it doesn't even make sense: a default value means you're able to do a POST request without sending any data!

I'll update the CR.

CR updated: https://www.drupal.org/node/2894819/revisions/view/10632228/10888480.