Problem/Motivation

We are supposed to be able to use _raw_variables for argument resolution but this has never worked because the code is treating a ParameterBag like an array (which it is not).

\Drupal\Core\Routing\Enhancer\ParamConversionEnhancer::copyRawVariables() has always returned a ParameterBag object and that object has never had array access so all of this looks like an oversight.

Proposed resolution

Remove pretence of been able to use _raw_variables for argument resolution rather than fixing something we can not possibly be using.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

Private service argument_resolver.raw_parameter has been removed. It didn't actually work.

Original report

in Drupal\Core\Controller\ControllerResolver::doGetArguments (file: \core\lib\Drupal\Core\Controller\ControllerResolver.php) the bug code:

  protected function doGetArguments(Request $request, $controller, array $parameters) {
    $attributes = $request->attributes->all();
    $raw_parameters = $request->attributes->has('_raw_variables') ? $request->attributes->get('_raw_variables') : [];
    $arguments = array();
    foreach ($parameters as $param) {
      if (array_key_exists($param->name, $attributes)) {
        $arguments[] = $attributes[$param->name];
      }
      elseif (array_key_exists($param->name, $raw_parameters)) {
        $arguments[] = $attributes[$param->name]; // here!
      }

"$attributes" here should be "$raw_parameters"
sorry, I'm not good at English , The following is a Chinese description:

在Drupal\Core\Controller\ControllerResolver::doGetArguments中
此处的变量误写成了“$attributes”,它应该是“$raw_parameters” 这会导致控制器获取不到原始变量中的值

CommentFileSizeAuthor
#10 2830631-10.patch3.95 KBalexpott
#9 remove_raw-2830631-9.patch1.02 KBAnonymous (not verified)
#9 2830631-9.patch1.88 KBAnonymous (not verified)
#9 2830631-9-test-only.patch1.17 KBAnonymous (not verified)
#2 2830631-2.patch734 bytesAnonymous (not verified)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yunke created an issue. See original summary.

Anonymous’s picture

Issue summary: View changes
Priority: Critical » Major
Status: Active » Needs review
FileSize
734 bytes

@yunke, thank you for found it! Your comment is clear enough (but it is better to use the code format). Also, let's make it as Major, but not Critical.

cilefen’s picture

I don't think release manager review is needed.

cilefen’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Can coverage be added to ControllerResolverTest?

cilefen’s picture

Title: the controller arguments parsing bug » ControllerResolver::doGetArguments does not set raw parameters into arguments
Priority: Major » Normal
andypost’s picture

Sure, this is a bug because condition checks key in $raw_parameters but $attributes used later!

Anonymous’s picture

If we need test, what about this:

  /**
   * Tests getArguments with a route match and a request with raw variables.
   *
   * @covers ::getArguments
   * @covers ::doGetArguments
   */
  public function testGetRawArgumentsWithRouteMatchAndRequest() {
    $request = Request::create('/test');
    $request->attributes->set('_raw_variables', ['request' => 'RAW']);
    $mock_controller = new MockController();
    $arguments = $this->controllerResolver->getArguments($request, [$mock_controller, 'getControllerWithRequestAndRouteMatch']);
    $this->assertTrue(in_array('RAW', $arguments),'Set raw variables');
  }

Or we need remove this part?

      elseif (array_key_exists($param->name, $raw_parameters)) {
        $arguments[] = $attributes[$param->name];
      }
cilefen’s picture

@andypost Yes, it seems that way when looking at the function in isolation. But looking at #2281619-52: Convert most direct usages within module code of routing related request attributes to use RouteMatchInterface instead I am not totally sure.

Anonymous’s picture

Test-only is interdiff with #2.

#2281619: Convert most direct usages within module code of routing related request attributes to use RouteMatchInterface instead contains next change:

+++ b/core/lib/Drupal/Core/Controller/ControllerResolver.php
@@ -138,11 +138,15 @@ protected function createController($controller) {
   protected function doGetArguments(Request $request, $controller, array $parameters) {
     $attributes = $request->attributes->all();
+    $raw_parameters = $request->attributes->has('_raw_variables') ? $request->attributes->get('_raw_variables') : [];
     $arguments = array();
     foreach ($parameters as $param) {
       if (array_key_exists($param->name, $attributes)) {
         $arguments[] = $attributes[$param->name];
       }
+      elseif (array_key_exists($param->name, $raw_parameters)) {
+        $arguments[] = $attributes[$param->name];
+      }
       elseif ($param->getClass() && $param->getClass()->isInstance($request)) {
         $arguments[] = $request;
       }
@@ -166,21 +170,6 @@ protected function doGetArguments(Request $request, $controller, array $paramete

@@ -166,21 +170,6 @@ protected function doGetArguments(Request $request, $controller, array $paramete
         throw new \RuntimeException(sprintf('Controller "%s" requires that you provide a value for the "$%s" argument (because there is no default value or because there is a non optional argument after this one).', $repr, $param->name));
       }
     }
-
-    // The parameter converter overrides the raw request attributes with the
-    // upcasted objects. However, it keeps a backup copy of the original, raw
-    // values in a special request attribute ('_raw_variables'). If a controller
-    // argument has a type hint, we pass it the upcasted object, otherwise we
-    // pass it the original, raw value.
-    if ($request->attributes->has('_raw_variables') && $raw = $request->attributes->get('_raw_variables')->all()) {
-      foreach ($parameters as $parameter) {
-        // Use the raw value if a parameter has no typehint.
-        if (!$parameter->getClass() && isset($raw[$parameter->name])) {
-          $position = $parameter->getPosition();
-          $arguments[$position] = $raw[$parameter->name];
-        }
-      }
-    }

But I still do not know how it prevent our issue with fix $attributes -> $raw_parameters typo.
Or do we need to remove this case altogether (see remove_raw-2830631-9.patch)? Because the current logic is meaningless.

alexpott’s picture

Version: 8.2.3 » 8.8.x-dev
Priority: Normal » Critical
Related issues: +#2912169: Inject the argument resolver into HttpKernel::__construct
FileSize
3.95 KB

So we ported this code to core/lib/Drupal/Core/Controller/ArgumentResolver/RawParameterValueResolver.php as part of #2912169: Inject the argument resolver into HttpKernel::__construct and ostensibly fixed this - but actually we didn't!!!

Because we copied over another bug from \Drupal\Core\Controller\ControllerResolver::doGetArguments().

Everything is assuming _raw_variables is an array whereas it is a ParameterBag() so on PHP 7.3 and below array_key_exists($param->name, $raw_parameters) is always FALSE. I'm bumping this to critical as it is currently breaking our PHP7.4 compatibility #3086374: Make Drupal 8 & 9 compatible with PHP 7.4.

I think the correct thing here is to completely remove _raw_argument handling - it's never worked and no one on this issue is arguing for it here.

alexpott’s picture

Issue summary: View changes

Updated the issue summary.

alexpott’s picture

Issue summary: View changes
alexpott’s picture

Title: ControllerResolver::doGetArguments does not set raw parameters into arguments » Remove code that tries to use _raw_variables for route argument resolution as it does not work
mondrake’s picture

Issue tags: +PHP 7.4
alexpott’s picture

Chatted about the service removal with @catch - we agreed a change record was a good idea so I created one https://www.drupal.org/node/3089181

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Looks great to me!

alexpott’s picture

  • catch committed e669b4e on 9.0.x
    Issue #2830631 by alexpott, cilefen, yunke, andypost: Remove code that...

  • catch committed 00d5589 on 8.9.x
    Issue #2830631 by alexpott, cilefen, yunke, andypost: Remove code that...

  • catch committed f4979fa on 8.8.x
    Issue #2830631 by alexpott, cilefen, yunke, andypost: Remove code that...
catch’s picture

Status: Reviewed & tested by the community » Fixed

So if this was only dead code it would probably be 9.x-only at this point, but given it's blocking PHP 7.4 agreed it's critical and we should just remove it. I can't see a way to do best-effort bc here, it should be impossible to end up in the situation you're using this because it would never work, so not sure what bc layer could look like.

Committed/pushed to 9.0.x, 8.9.x and 8.8.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.