Problem/Motivation

If an outbound path processor is configured to perform some path alterations based on the route, then the route name and parameters might need to be known. However we pass only the route, but not the corresponding route name and parameters.

Proposed resolution

Apart from the route, also pass the route name and parameters.

CommentFileSizeAuthor
#35 3202329-nr-bot_4iyo5nix.txt942 bytesneeds-review-queue-bot
#2 3202329-2.patch719 byteshchonov

Issue fork drupal-3202329

Command icon Show commands

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

About issue forks

Comments

hchonov created an issue. See original summary.

hchonov’s picture

hchonov
Primary language Bulgarian
Location Burgas
commented
StatusFileSize
new 3202329-2.patch719 bytes
hchonov’s picture

hchonov
Primary language Bulgarian
Location Burgas
commented
Status: Active » Needs review

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.

hchonov’s picture

hchonov
Primary language Bulgarian
Location Burgas
commented

An example of one of the possible use cases is the following module that requires this feature: https://www.drupal.org/project/decoupled_routes

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.

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

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative, +Needs tests

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 could use a test case to show the issue.

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

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

dieterholvoet made their first commit to this issue’s fork.

dieterholvoet’s picture

dieterholvoet commented
Title: Outbound path processors miss the route name » Outbound path processors miss the route name and parameters
Category: Bug report » Feature request
Issue summary: View changes

This change could mean a big performance improvement in the Domain module: #3232343: DomainSourcePathProcessor is resource intensive.

I started a MR with the changes from the patch. I also added route_parameters to the $options array, we need that to extract entity information from the URL.

dieterholvoet opened merge request !11189

riyas_nr made their first commit to this issue’s fork.

riyas_nr’s picture

riyas_nr commented
Status: Needs work » Needs review

Added test coverage for adding route_name and route_parameters to the $options array in UrlGenerator::generateFromRoute() . Moving to NR.

mably’s picture

mably commented

This is a must have.

As said above by @dieterholvoet, without access to the those route_name and route_parameters options, we have to use Url::fromUserInput in the DomainSourcePathProcessor of the Domain module and it causes a significant performance issue (besides some annoying loops).

And using the Route Provider or Router in a path processor service creates some circular dependency problems.

nebel54’s picture

nebel54
Primary language German
Location Vienna
commented
Status: Needs review » Reviewed & tested by the community

The patch is looking good and working as expected.

astonvictor’s picture

astonvictor commented

the patch works for me
+1 RTBC

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs tests

Committed/pushed to 11.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

catch closed merge request !11189

  • catch committed 282300f7 on 11.x 
    Issue #3202329 by hchonov, dieterholvoet, riyas_nr: Outbound path...

  • catch committed 446af516 on 11.3.x 
    Issue #3202329 by hchonov, dieterholvoet, riyas_nr: Outbound path...

  • catch committed 0eccf7d1 on 11.3.x 
    Revert "Issue #3202329 by hchonov, dieterholvoet, riyas_nr: Outbound...

  • catch committed 2020b680 on 11.x 
    Revert "Issue #3202329 by hchonov, dieterholvoet, riyas_nr: Outbound...
catch’s picture

catch
he/him
Primary language English
commented
Status: Fixed » Needs work

Had to revert - test coverage here needs updating to use PHPUnit attributes instead of annotations.

mably opened merge request !13624

mably’s picture

mably commented

Hi @catch, do you have any example of how it should be done?

EDIT: Just had to look to the other tests to find the solution.

mably’s picture

mably commented
Status: Needs work » Needs review
idebr’s picture

idebr commented
Status: Needs review » Needs work

MR shows a failure in the pipeline (probably unrelated, but I don't have the permission to re-run it), see https://git.drupalcode.org/issue/drupal-3202329/-/pipelines/644390/test_...

riyas_nr’s picture

riyas_nr commented
Status: Needs work » Needs review

dcam made their first commit to this issue’s fork.

dcam’s picture

dcam commented
Status: Needs review » Reviewed & tested by the community

The latest commit implements @legacy-covers, the lack of which is what caused the pipeline failures after the MR was committed back in October. I rebased the MR and all of the up-to-date linting is passing. This is RTBC again.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3202329-nr-bot_4iyo5nix.txt942 bytes

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

dcam’s picture

dcam commented
Status: Needs work » Reviewed & tested by the community

Post-bot-rebellion rebase

longwave made their first commit to this issue’s fork.

  • longwave committed 3439f06f on 11.x 
    feat: #3202329 Outbound path processors miss the route name and...

  • longwave committed d590556a on main 
    feat: #3202329 Outbound path processors miss the route name and...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Second time lucky, sorry this took so long to get back in.

Committed and pushed d590556a447 to main and 3439f06f917 to 11.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

longwave closed merge request !13624

mably’s picture

mably commented

Thanks @longwave! Happy to see this useful enhancement get merged.

Do you know if we will get this in 11.3 or 11.4?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Currently 11.4, as features only arrive in minor versions. I guess it can't hurt to cherry-pick to 11.3 if it's useful there though?

mably’s picture

mably commented

Yes, it would definitely be useful in 11.3. But we can also wait for 11.4.

Status: Fixed » Closed (fixed)

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