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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amcgowanca created an issue. See original summary.

amcgowanca’s picture

amcgowanca’s picture

Status: Active » Needs review
dawehner’s picture

Issue tags: +Needs tests
bradjones1’s picture

Version: 8.3.2 » 8.6.x-dev

Bump version... got bit by this, today.

dawehner’s picture

For writing some test coverage for this I would suggest to add a new test case into core/tests/Drupal/Tests/Core/Routing/UrlGeneratorTest.php

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

hudri’s picture

Patch from #2 still applies and fixed it for my use case on Drupal v8.5.5

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

sardara’s picture

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

The last submitted patch, 12: 2881077-12-test-only.patch, failed testing. View results

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests +Bug Smash Initiative

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

quietone’s picture

Status: Reviewed & tested by the community » Needs work

This is retesting on Drupal 9.1. Can we get it retesting on a supported version of Drupal?

sardara’s picture

Status: Needs work » Reviewed & tested by the community

Ran on 9.3.x and 9.4.x . Had one random failure once on unrelated tests.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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

diff --git a/core/lib/Drupal/Core/Routing/UrlGenerator.php b/core/lib/Drupal/Core/Routing/UrlGenerator.php
index 7efa6b75b8..0c236cdc14 100644
--- a/core/lib/Drupal/Core/Routing/UrlGenerator.php
+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
@@ -270,16 +270,15 @@ public function generateFromRoute($name, $parameters = [], $options = [], $colle
     $route = $this->getRoute($name);
     $generated_url = $collect_bubbleable_metadata ? new GeneratedUrl() : NULL;
 
-    $fragment = '';
-    if (isset($options['fragment'])) {
-      if (($fragment = trim($options['fragment'])) != '') {
-        $fragment = '#' . $fragment;
-      }
-    }
-
     // Generate a relative URL having no path, just query string and fragment.
     if ($route->getOption('_no_path')) {
       $query = $options['query'] ? '?' . UrlHelper::buildQuery($options['query']) : '';
+      $fragment = '';
+      if (isset($options['fragment'])) {
+        if (($fragment = trim($options['fragment'])) != '') {
+          $fragment = '#' . $fragment;
+        }
+      }
       $url = $query . $fragment;
       return $collect_bubbleable_metadata ? $generated_url->setGeneratedUrl($url) : $url;
     }
@@ -328,6 +327,12 @@ public function generateFromRoute($name, $parameters = [], $options = [], $colle
     }
 
     $query = $options['query'] ? '?' . UrlHelper::buildQuery($options['query']) : '';
+    $fragment = '';
+    if (isset($options['fragment'])) {
+      if (($fragment = trim($options['fragment'])) != '') {
+        $fragment = '#' . $fragment;
+      }
+    }
 
     // The base_url might be rewritten from the language rewrite in domain mode.
     if (isset($options['base_url'])) {

Yes the code is duplicate but we are already this with $query = $options['query'] ? '?' . UrlHelper::buildQuery($options['query']) : '';

smustgrave’s picture

Status: Needs work » Needs review
FileSize
1.66 KB
2.81 KB

So like this?

alexpott’s picture

Status: Needs review » Needs work

@smustgrave and remove the original fragment setting :)

smustgrave’s picture

Status: Needs work » Needs review
FileSize
738 bytes
3.11 KB

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

Still passes 10.1

alexpott’s picture

@sardara do you feel like rtbc'ing this again. The change looks good.

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

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.

sardara’s picture

I arrive late, but indeed I agree with the changes and the RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 66500ab088 to 10.1.x and 0769b54d48 to 10.0.x and 5ad06dcc0e to 9.5.x. Thanks!

  • alexpott committed 66500ab0 on 10.1.x
    Issue #2881077 by smustgrave, sardara, amcgowanca, alexpott, dawehner:...

  • alexpott committed 0769b54d on 10.0.x
    Issue #2881077 by smustgrave, sardara, amcgowanca, alexpott, dawehner:...

  • alexpott committed 5ad06dcc on 9.5.x
    Issue #2881077 by smustgrave, sardara, amcgowanca, alexpott, dawehner:...
alexpott’s picture

Version: 10.1.x-dev » 9.5.x-dev

Committed and pushed to 10.1.x, 10.0.x, and 9.5.x. Thanks!

Status: Fixed » Closed (fixed)

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