copyRawVariables should support default route 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.

Rerolled for D10! 🚀

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

Sorry about that! 😅
Here's the test and patch against 10.0.7.

The last submitted patch, 18: test-only.patch, failed testing. View results →

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

Ah yes, that's the right place to fix this! When we change it in ParamConversionEnhancer->copyRawVariables() I no longer need the patch from  #2743303-22: EntityForm::getEntityFromRouteMatch() does not support route-level parameters → . Thanks for figuring that out @larowlan!

The last submitted patch, 22: test-only.patch, failed testing. View results →

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

Me too! Here's a back port to 9.5

Thanks again! 🚀

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

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

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?

hmm, I don't know. 😬 The only documentation → I could find didn't really specify if a default can be an array.

@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 ✨ Add Exception for TypeError Argument must be String in \Drupal\Component\Utility\Html escape{} Needs work

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 🐛 [regression] route defaults are now automatically route parameters Fixed instead.