Route "_title_arguments" computation may be incorrect

This 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.

This seems to be clearly described. Tagged as Needs tests and it should be easy to add a test.

Will work on a fix with test.

Can issue summary be updated using the issue template please

I 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.

Thanks @shalini.

@smustgrave the issue summary is fixed and MR is tested now.

This issue needs a rebase 488 commits behind, the pipeline can't be reliable at that point.

I have rebased this MR, and the pipeline has passed successfully. Moving it to 'Needs Review'.
Kindly review.

So see that 2 tests have been added but when the test-only feature is ran only testStaticTitleWithDefaultArgumentsOverridden is failing.

I have verified that the testStaticTitleWithDefaultArguments method 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 in testStaticTitleWithDefaultArgumentsOverridden, 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."

Lets consolidate them. The code seems very similar so probably overlaps.

I 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 :

PHPUnit 10.5.38 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.12
Configuration: /var/www/html/core/phpunit.xml.dist

F                                                                   1 / 1 (100%)

Time: 00:04.851, Memory: 186.00 MB

There was 1 failure:

1) Drupal\Tests\Core\Controller\TitleResolverTest::testStaticTitleWithArguments
Failed asserting that two objects are equal.
--- Expected
+++ Actual
@@ @@
 Drupal\Core\StringTranslation\TranslatableMarkup Object (
     'string' => 'static title @test @test2'
     'arguments' => Array (
-        '@test' => 'override value'
+        '@test' => 'value'
         '%test' => 'override value'
         '@test2' => 'value2'
     )

/var/www/html/core/tests/Drupal/Tests/Core/Controller/TitleResolverTest.php:153

Feedback appears to be addressed.

I 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 → states

_title_arguments: Additional arguments for the title text passed along to t().

If that is true then I wonder if this is working as designed. If it is not this needs a title update.

This 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.

Thank 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!

Think the title of the MR probably makes more sense.

CR seems well detailed

Believe all feedback has been addressed.

As 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!

smustgrave → closed merge request !8584

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