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” 这会导致控制器获取不到原始变量中的值
Comment | File | Size | Author |
---|---|---|---|
#10 | 2830631-10.patch | 3.95 KB | alexpott |
#9 | remove_raw-2830631-9.patch | 1.02 KB | Anonymous (not verified) |
#9 | 2830631-9.patch | 1.88 KB | Anonymous (not verified) |
#9 | 2830631-9-test-only.patch | 1.17 KB | Anonymous (not verified) |
#2 | 2830631-2.patch | 734 bytes | Anonymous (not verified) |
Comments
Comment #2
Anonymous (not verified) CreditAttribution: Anonymous commented@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.
Comment #3
cilefen CreditAttribution: cilefen commentedI don't think release manager review is needed.
Comment #4
cilefen CreditAttribution: cilefen commentedCan coverage be added to ControllerResolverTest?
Comment #5
cilefen CreditAttribution: cilefen commentedAre you sure this is a bug? See #2281619: Convert most direct usages within module code of routing related request attributes to use RouteMatchInterface instead.
Comment #6
andypostSure, this is a bug because condition checks key in
$raw_parameters
but$attributes
used later!Comment #7
Anonymous (not verified) CreditAttribution: Anonymous commentedIf we need test, what about this:
Or we need remove this part?
Comment #8
cilefen CreditAttribution: cilefen commented@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.
Comment #9
Anonymous (not verified) CreditAttribution: Anonymous commentedTest-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:
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.
Comment #10
alexpottSo 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.
Comment #11
alexpottUpdated the issue summary.
Comment #12
alexpottComment #13
alexpottComment #14
mondrakeComment #15
alexpottChatted 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
Comment #16
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedLooks great to me!
Comment #17
alexpottNeither the service http://codcontrib.hank.vps-private.net/search?text=argument_resolver.raw... nor the class http://codcontrib.hank.vps-private.net/search?text=RawParameterValueReso... is used by contrib.
Comment #21
catchSo 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!