copyRawVariables should support default route parameters

Can we add a test to demonstrate the bug and ensure the fixed behaviour is retained?

Thanks!

Should we postpone this on #2743303: EntityForm::getEntityFromRouteMatch() does not support route-level parameters

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Patch failed to apply.

Rerolled patch for 10.1.x branch, please review it.

Thanks!

Issue summary lines up with the solution in the patch
#5 shows that the test case covers the issue.

The tests from #5 were lost somehow

#16 that was my mistake. See the tests have been added back.

I'm not convinced this is the right fix.

If we look at \Drupal\Core\Routing\RouteMatch::getParameterNames raw parameters should include defaults.

However, the _raw_variables set in \Drupal\Core\Routing\Enhancer\ParamConversionEnhancer::copyRawVariables doesn't give the same treatment to defaults as \Drupal\Core\Routing\RouteMatch::getParameterNames

I think the fix should live in \Drupal\Core\Routing\Enhancer\ParamConversionEnhancer::copyRawVariables instead, to make the two of them consistent.

This should then render the change in this patch unnecessary and make the two internally consistent.

Thoughts?

Thanks for updating issue summary too @Tim Bozeman

See the change suggested by @larowlan has been implemented and test-only patch shows the test coverage is still good.

🔥 Love it when we get to a simpler solution

This is on my list to commit once we get out of the freeze - which ends at 10pm Thursday in my timezone. There is a four day weekend here, so it likely won't be until Tuesday next week.

Committed to 10.1.x, so this is just awaiting backport post freeze

Glad @larowlan's comment in #21 happened

Backported to 10.0.x

Requeued tests on 9.5.x as HEAD had a random fail

Backported to 9.5.x after green test run - thanks all

Sorry for posting in a closed issue, just want to be sure this didn't introduce a regression in some form ...

The CiviCRM module has been doing this in a route_callbacks callback:

$route = new Route(
  '/' . $path . '/{extra}',
  [
    '_title' => isset($item['title']) ? $item['title'] : 'CiviCRM',
    '_controller' => 'Drupal\civicrm\Controller\CivicrmController::main',
    'args' => explode('/', $path),
    'extra' => '',
  ],
  [
    '_access' => 'TRUE',
    'extra' => '.+',
  ]
);

Notice the args key within the second argument ($defaults) passed to the Route constructor - all keys not starting with a _ inside that parameter are now (due to this change) eventually being treated as translation parameters (in Drupal/Core/Controller/TitleResolver.php:67), causing an array being considered a string and eventually resulting in a TypeError: htmlspecialchars(): Argument #1 ($string) must be of type string, array given in htmlspecialchars() (Line 432 in /path/to/drupal/web/core/lib/Drupal/Component/Utility/Html.php).

Is this args key being an array just wrong here or did this issue introduce a regression? Is there documentation on what is allowed to be passed into a Route's defaults?

Edit: I think CiviCRM was wrong by making this args parameter an array while route parameters should be strings or integers. There's now a PR for fixing that in the CiviCRM module. However, this is still different behavior than before 9.5.9.

@jensschuppe can you open a followup for that - I think default arguments can totally be arrays and as such that automagic '@arg' '%arg' should be checking if $value is a scalar - i.e. that sounds like a bug in TitleResolver in my book

@larowlan There's a new issue for the problem: #3352384

I also think that the bug is in TitleResolver, at least in conjunction with the new behavior of Drupal 9.5.9 that all route defaults whose keys do not start with an underscore _ are now automatically considered route parameters and thus get copied as arguments for title translation.

Thanks

#3352384 seems to cover another issue, see #3358402 instead.