Problem
Through the implementation of an outbound path processor, it is currently impossible to override the $options['fragment']
value and for it to be used in the processed and generated URL.
Steps, expected & actual
- Implement an outbound path processor that alters the
$options['fragment']
value for a URL being generated from a route (note: I have not tested other URL generators with respect to this same problem yet). - The expected URL value should include the the specified fragment.
- The actual URL which is generated does not include the fragment specified in the outbound path processor.
In code
The implementation of Drupal\Core\Routing\UrlGenerator::generateFromRoute(...) determines the $fragment
variable value from $options['fragment']
early for its usage on routes which have the _no_path
option specified. However, in cases where $options['path_processing']
is true, the processors may alter the $options['fragment']
value but it has no affect on the $fragment
within the generateFromRoute(...)
method as the altered value is never re-assigned.
Comment | File | Size | Author |
---|---|---|---|
#24 | 2881077-24.patch | 3.11 KB | smustgrave |
| |||
#24 | interdiff-22-24.txt | 738 bytes | smustgrave |
#12 | interdiff-2-12.txt | 1.3 KB | sardara |
#12 | 2881077-12.patch | 2.23 KB | sardara |
#12 | 2881077-12-test-only.patch | 1.43 KB | sardara |
Comments
Comment #2
amcgowanca CreditAttribution: amcgowanca commentedComment #3
amcgowanca CreditAttribution: amcgowanca commentedComment #4
dawehnerComment #5
bradjones1Bump version... got bit by this, today.
Comment #6
dawehnerFor writing some test coverage for this I would suggest to add a new test case into
core/tests/Drupal/Tests/Core/Routing/UrlGeneratorTest.php
Comment #8
hudriPatch from #2 still applies and fixed it for my use case on Drupal v8.5.5
Comment #12
sardara CreditAttribution: sardara at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commentedAdapting an existing test to also include the fragment in the altered options.
I was wondering if we needed to test more in depth the other parameters of options, but let's go for this now.
Comment #18
smustgrave CreditAttribution: smustgrave at Mobomo commentedRreran on 9.5 and it still applies cleanly.
Removed Needs tests tag as a test was reused
Tests-only patch is failing as expected.
Reviewed patch and looks clean to me.
Comment #19
quietone CreditAttribution: quietone at PreviousNext commentedThis is retesting on Drupal 9.1. Can we get it retesting on a supported version of Drupal?
Comment #20
sardara CreditAttribution: sardara at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commentedRan on 9.3.x and 9.4.x . Had one random failure once on unrelated tests.
Comment #21
alexpottNice find and fix!
Url generation is something that can happen a lot - perhaps not with $option['fragment'] set but still. I think we should only do fragment processing when necessary... atm we can do it twice even if there no changes. Therefore I think we should do something like:
Yes the code is duplicate but we are already this with
$query = $options['query'] ? '?' . UrlHelper::buildQuery($options['query']) : '';
Comment #22
smustgrave CreditAttribution: smustgrave at Mobomo commentedSo like this?
Comment #23
alexpott@smustgrave and remove the original fragment setting :)
Comment #24
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #26
smustgrave CreditAttribution: smustgrave at Mobomo commentedStill passes 10.1
Comment #27
alexpott@sardara do you feel like rtbc'ing this again. The change looks good.
Comment #28
borisson_This change looks great, the test coverage is very small but seems to cover everything in this patch. I don't mind the code duplication here because it looks more clear with it being inline in this case.
Comment #29
sardara CreditAttribution: sardara for European Commission and European Union Institutions, Agencies and Bodies commentedI arrive late, but indeed I agree with the changes and the RTBC.
Comment #30
alexpottCommitted and pushed 66500ab088 to 10.1.x and 0769b54d48 to 10.0.x and 5ad06dcc0e to 9.5.x. Thanks!
Comment #34
alexpottCommitted and pushed to 10.1.x, 10.0.x, and 9.5.x. Thanks!