Problem/Motivation

In #3159210: Support route aliasing (Symfony 5.4) and allow deprecating the route name we added the ability to deprecate routes, but we later found that usages of RouteMatchInterface::getRouteName() for checking against specific route name(s) are problematic because they can't take into account deprecated route names.

Proposed resolution

Add a method on \Drupal\Core\Routing\RouteMatchInterface that can be used to compare the route name against a given string (or a list of them), and document that RouteMatchInterface::getRouteName() is discouraged for this purpose. That method can't be deprecated because there are valid use-cases for retrieving the route name, e.g. the route and route.name cache contexts, getting local tasks or actions for the current route, etc.

Remaining tasks

Discuss, implement the new method, etc.

User interface changes

Nope.

Introduced terminology

N/A

API changes

API addition: a new method on \Drupal\Core\Routing\RouteMatchInterface.

Data model changes

Nope.

Release notes snippet

TBD.

CommentFileSizeAuthor
#7 3506653-nr-bot.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3506653

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:

Comments

amateescu created an issue. See original summary.

amateescu’s picture

Status: Active » Needs review

Started a MR with a proposal for the new method. Feedback welcome :)

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs change record, +Needs tests

Appears small feedback has been provided by @acbramley.

Wonderif there is a current instance in core that could use this and be converted to show it's worth?

amateescu’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests

Addressed all the feedback so far, but there's one thing left.. the main reason for this API: checking whether the passed-in route name is deprecated.

The problem is that the only way to do that currently is \Drupal\Core\Routing\RouteProvider::getRouteAliases(), which would mean an uncached db query for every call to the new RouteMatch::isRouteName() method. I think we need a way to retrieve and cache (persistent and static) all aliased/deprecated routes somewhere in the route provider. Setting back to NW to get feedback on that.

catch’s picture

I'm a bit confused by #5 but I might have it backwards.

RouteProvider::getRouteByName() has this:

    $result = reset($routes);
    if ($result instanceof Alias) {
      $alias = $result->getId();
      if ($result->isDeprecated()) {
        $deprecation = $result->getDeprecation($name);
        @trigger_error($deprecation['message'], E_USER_DEPRECATED);
      }
      return $this->getRouteByName($alias);
    }

So... in isRouteName(), if it matches the current rout name, we can assume it's not a deprecated alias because no-one should ever actually land on one and just return TRUE.

If it doesn't match, we can call RouteProvider::getRouteByName() on each of the passed in routes, this will trigger the deprecation we need.

Route preloading should mean this isn't an additional database query. There is some rationalisation to do in #3503843: Remove automatic preloading of all "public" routes, cache routes in fast chained bin but it's just another thing to take into account on that issue.

needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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.

amateescu’s picture

Status: Needs work » Needs review

If it doesn't match, we can call RouteProvider::getRouteByName() on each of the passed in routes, this will trigger the deprecation we need.

@catch, the issue is that RouteMatch::isRouteName() will _not_ match the current route name in most (almost all) cases, so we'll be calling ::getRouteByName() all the time. Maybe it's easier to explain with an example:

Let's say we have a few different modules, and each one checks for a route, e.g. ::isRouteName('module1_controller'), ::isRouteName('module2_form'), etc.

Now, imagine that those checks happen on every request, wouldn't this mean that we'll be "attaching" the knowledge of those routes to every page in the context of the route preloader?

catch’s picture

Now, imagine that those checks happen on every request, wouldn't this mean that we'll be "attaching" the knowledge of those routes to every page in the context of the route preloader?

The route preloader currently loads every non-admin route unconditionally on every request, so it wouldn't make things worse or better, unless we change the behaviour in the issue above, which we might do.

smustgrave’s picture

Status: Needs review » Needs work

Appears to have merge conflicts

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

acbramley’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record

Rebased and fixed tests, marking the MR as ready as it's looking great to me.

Added CR https://www.drupal.org/node/3526518

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Applied the MR and searched for \Drupal::routeMatch()->getRouteName() to see if other instances. There were 4 but they weren't doing directly route checking.

All feedback on the MR already appears to be addressed.

Believe good to go

amateescu’s picture

Status: Reviewed & tested by the community » Needs work

We still need to handle deprecated routes somehow, per #5 and following discussion :)

acbramley’s picture

I would be happy to add the handling for deprecated routes but it sounds like there's still some questions on if/where that needs to happen.

#6 would mean we'd need the route provider service in the route match service, right? I'm not sure if this is the right approach but based on #10 it sounds like it might be?

acbramley’s picture

I may be completely off here, but this is what I understood from #6 was right it'd look something like this:

  public function isRouteName(string|array $value, RouteName $operator = RouteName::Equals): bool {
    if (is_array($value) && $operator !== RouteName::In) {
      throw new \InvalidArgumentException('$operator must be RouteName::In when $value is an array.');
    }

    $match = match ($operator) {
      RouteName::Equals => $this->routeName === $value,
      RouteName::StartsWith => str_starts_with($this->routeName, $value),
      RouteName::Contains => str_contains($this->routeName, $value),
      RouteName::EndsWith => str_ends_with($this->routeName, $value),
      RouteName::In => in_array($this->routeName, $value, TRUE),
    };
    // If we have a match, we can assume we aren't on a deprecated route.
    if ($match) {
      return TRUE;
    }
    // Otherwise, get each route by name to trigger deprecations.
    $routeProvider = \Drupal::service('router.route_provider');
    $value = is_array($value) ? $value : [$value];
    foreach ($value as $routeName) {
      $routeProvider->getRouteByName($routeName);
    }

    return FALSE;
  }

When testing this with a debugger, we do call RouteProvider::preLoadRoutes() on every call to getRouteByName which does add a cache get for every unique route that is checked with isRouteName

catch’s picture

@acbramley were you testing on a regular page request? RoutePreloader::onRequest() ought to preload all non-admin routes early in the request to avoid separate look-ups.

acbramley’s picture

@catch I was testing on a node edit page, I can double check a non admin page.

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.