The current implementation of the title resolution mechanism does not behave as expected when both _raw_variables and _title_arguments are set in a route. Specifically, when _raw_variables is defined in the route, the getTitle() method continues to return the static value specified in the route rather than utilizing the provided _title_arguments. This creates confusion and prevents dynamic title generation based on URL parameters.
ex :
custom_module.test_route:
path: '/test-route/{test}/{test2}'
defaults:
_controller: '\Drupal\custom_module\Controller\CustomPageController::content'
_title: 'Static title @test @test2'
_title_arguments:
'@test': 'value'
'@test2': 'value2'
requirements:
_permission: 'access content'API page: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Controlle...
The code for getting the arguments for the route in TitleResolver.php file in getTitle() is as follows:
$args = [];
if (($raw_parameters = $request->attributes->get('_raw_variables'))) {
foreach ($raw_parameters->all() as $key => $value) {
$args['@' . $key] = $value;
$args['%' . $key] = $value;
}
}
if ($title_arguments = $route->getDefault('_title_arguments')) {
$args = array_merge($args, (array) $title_arguments);
}
Here '_title_arguments' are the arguments specified in the routing file, which IMO, should be the default value in case "_raw_variables" are not there.
So IMO, the array_merge() should be:
array_merge((array) $title_arguments, $args);
- Needs Review
- None
- None
- None
- None
| Comment | File | Size | Author |
|---|
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
compare changes, plain diff MR !8584
Comments
Comment #2
vivek panicker commentedComment #3
vivek panicker commentedComment #11
smustgrave commentedThis issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.
This will need a test case to show the issue.
Did not test for the issue.
Comment #13
nlisgo commentedThis seems to be clearly described. Tagged as Needs tests and it should be easy to add a test.
Comment #14
nlisgo commentedWill work on a fix with test.
Comment #16
nlisgo commentedComment #17
nlisgo commentedComment #18
smustgrave commentedCan issue summary be updated using the issue template please
Comment #19
nlisgo commentedComment #20
shalini_jha commentedComment #21
shalini_jha commentedComment #22
shalini_jha commentedI have tested MR 8584, and after these changes, when the parameter is available in the URL, the getTitle method returns the value from the URL parameter instead of the static value from the routing. Additionally, I have updated the issue summary.
Moving this to NR, Kindly review.
Comment #23
vivek panicker commentedThanks @shalini.
@smustgrave the issue summary is fixed and MR is tested now.
Comment #24
smustgrave commentedThis issue needs a rebase 488 commits behind, the pipeline can't be reliable at that point.
Comment #25
shalini_jha commentedI have rebased this MR, and the pipeline has passed successfully. Moving it to 'Needs Review'.
Kindly review.
Comment #26
smustgrave commentedSo see that 2 tests have been added but when the test-only feature is ran only testStaticTitleWithDefaultArgumentsOverridden is failing.
Comment #27
shalini_jha commentedI have verified that the
testStaticTitleWithDefaultArgumentsmethod checks the default behaviour of using static titles with preset arguments without testing dynamic overrides, which is why it does not fail. The issue is specifically addressed intestStaticTitleWithDefaultArgumentsOverridden, where dynamic URL parameters should replace default values but are not being applied correctly.Please let me know if we should remove the method that tests the default functionality. Moving this to "Needs Review."
Comment #28
smustgrave commentedLets consolidate them. The code seems very similar so probably overlaps.
Comment #29
shalini_jha commentedComment #30
shalini_jha commentedI have consolidated both test methods into a single one and tested it. It is working as expected. Moving this to NR for review. Kindly review.
Failing test :
Comment #31
smustgrave commentedFeedback appears to be addressed.
Comment #32
quietone commentedI read the IS, comments and the MR. This looks in order. I don't know much about routing so did some investigation.
The code being changed was added in 2014 in [ #2120235] but it doesn't address the purpose of the
_title_arguments. The Structure of routes statesIf that is true then I wonder if this is working as designed. If it is not this needs a title update.
Comment #33
alexpottThis is a change in behaviour. Potentially someone might be relying on the ability of _title_arguments to override the arguments based on the routes raw parameters. That means I think we need a change record. Unfortunately there is no documentation in core about _title_arguments as ideally the behaviour should be documented there. The change being made here however is inline with the documentation linked to from #32 as this now means that _title_arguments can only define additional arguments and values and not override stuff coming from the route.
Comment #34
shalini_jha commentedThank you for the review and feedback. Based on the suggestions provided, I tested the functionality after implementing the recommended changes, and it is now working as expected. There is no need for an array_merge after retrieving the arguments from _raw_variables. I also re-ran the tests, and they are passing successfully.
I have also drafted a CR based on the work done and provided an explanation of the changes made. However, I’m unsure if anything else needs to be added. I kindly request you to review it and share your feedback. Thank you!
Comment #35
smustgrave commentedThink the title of the MR probably makes more sense.
CR seems well detailed
Believe all feedback has been addressed.
Comment #36
alexpottAs this is a subtle behaviour change I'm going to only put this in 11.2.x to give people a chance to notice this change.
Committed 0c9b4fc and pushed to 11.x. Thanks!