Support route aliasing (Symfony 5.4) and allow deprecating the route name

Created on 15 July 2020, over 4 years ago
Updated 16 January 2023, about 2 years ago

Problem/Motivation

Over in 📌 [PP-1] NodeRouteProvider should extend DefaultHtmlRouteProvider Needs work we are trying to deprecate 'node.add_page' and 'node.add' routes.

The problem is these route names are getting used in a lot of places:

  • The URL generation. e.g. Url::fromRoute('node.add_page')
  • Menu Link/local task/local action generation.
  • Drupal::routeMatch()->getRouteName is used in various contexts for doing things conditionally.
  • Breadcrumb builders primarily seem to use route names to derive context.
  • There might be a few more.

Over in 📌 [PP-1] NodeRouteProvider should extend DefaultHtmlRouteProvider Needs work , NodeRouteProcessorBc is added which take care of URL generation but what about the rest?

Proposed resolution

Figure out a way to deprecate route names.

Remaining tasks

Figure out all the endge cases by looking at contribs usage. http://grep.xnddx.ru/search?text=node.add and http://grep.xnddx.ru/search?text=node.add_page
Finalize the implementation detail.
Implement the soultion.

User interface changes

None.

API changes

This will be an API addition. An API to allow the deprecation of the routes.

Data model changes

None.

Release notes snippet

Not worth mentioning in release notes.

📌 Task
Status

Needs work

Version

10.1

Component
Routing 

Last updated 5 days ago

Created by

🇨🇦Canada jibran Toronto, Canada

Live updates comments and jobs are added and updated live.
  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Assigned to larowlan
  • Issue was unassigned.
  • Status changed to Needs review about 2 years ago
  • Status changed to Needs work about 2 years ago
  • Status changed to Needs review about 2 years ago
  • 🇬🇧United Kingdom AaronMcHale Edinburgh, Scotland

    I think every use of deprecated route should cause a warning

    Sounds like a good idea.

  • Status changed to Needs work almost 2 years ago
  • The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

    Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

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

  • Status changed to Needs review almost 2 years ago
  • 🇮🇳India pooja saraah Chennai

    Fixed failed commands on #25
    Attached patch against Drupal 10.1.x

  • Status changed to Needs work almost 2 years ago
  • 🇫🇷France andypost

    Closed as duplicate 📌 Rename the 'alias' option for URLs Closed: duplicate and proper re-roll, leaving NW for tests

  • 🇫🇷France andypost

    All failed tests showing that deprecated route is used somehow, so it needs to clean-up usage

    Moreover the error message is not clear enough - no route alias name is displayed (only in argument dblog.delete

    Exception: Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'alias' in 'field list': INSERT INTO "test90401139router" ("name", "route", "alias") VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2); Array
    (
        [:db_insert_placeholder_0] => dblog.confirm
        [:db_insert_placeholder_1] => O:31:"Symfony\Component\Routing\Alias":2:{s:35:"Symfony\Component\Routing\Aliasid";s:12:"dblog.delete";s:44:"Symfony\Component\Routing\Aliasdeprecation";a:3:{s:7:"package";s:12:"drupal/dblog";s:7:"version";s:4:"10.1";s:7:"message";s:106:"The "%alias_id%" route alias is deprecated. You should stop using it, as it will be removed in the future.";}}
        [:db_insert_placeholder_2] => dblog.delete
    )
    
    Drupal\Core\Routing\MatcherDumper->dump()() (Line: 164)
  • 🇫🇷France andypost

    Better to use route from #18

    We could alias user.well-known.change_password -> user.edit as a working example of aliasing without deprecation.

  • 🇬🇧United Kingdom joachim

    The IS doesn't mention the plan that's in the title.

  • 🇫🇷France andypost
    1. +++ b/core/lib/Drupal/Core/Routing/MatcherDumper.php
      @@ -248,9 +264,17 @@ protected function schemaDefinition() {
      +        'alias' => [
      ...
             'indexes' => [
               'pattern_outline_parts' => ['pattern_outline', 'number_parts'],
      +        'alias' => ['alias'],
      
      +++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
      @@ -476,4 +487,30 @@ protected function getCurrentLanguageCacheIdPart() {
      +  public function getRouteAliases(string $route_name): RouteCollection {
      ...
      +    $routes = $this->connection->select($this->tableName, 'router')
      +      ->fields('router', ['name'])
      +      ->condition('alias', $route_name)
      ...
      +    $select = $this->connection->select($this->tableName, 'router')
      +      ->fields('router', ['alias'])
      +      ->condition('name', $aliased_route_name);
      
      +++ b/core/modules/system/system.post_update.php
      @@ -51,3 +51,11 @@ function system_post_update_linkset_settings() {
      +function system_post_update_rebuild_matcher_dumper_with_alias_support(): void {
      +  \Drupal::database()->schema()->dropTable('router');
      

      new column and index - schema change should go to update hook I bet and rebuild probably will be automatic

      also it needs upgrade test

    2. +++ b/core/lib/Drupal/Core/Routing/RouteProviderInterface.php
      @@ -104,4 +105,29 @@ public function getAllRoutes();
      +  public function getRouteAliases(string $route_name): RouteCollection;
      ...
      +  public function getUnAliasedRouteName(string $aliased_route_name): ?string;
      

      the only API addition

  • 🇮🇳India _utsavsharma

    Patch for 11.x, as the previous was not failing to apply.

  • last update over 1 year ago
    30,335 pass, 66 fail
  •    // Not using chunks because not many aliases expected.
    +      $insert = $this->connection->insert($this->tableName)->fields([
    +        'name',
    +        'route',
    +        'alias',
    +      ]);
    

    I'm coming here from https://www.drupal.org/project/drupal/issues/3311365 📌 Use PHP attributes for route discovery Needs review . It seems like this assumption might not hold necessarily?

    +/**
    + * Rebuilds the router with alias support.
    + */
    +function system_post_update_rebuild_matcher_dumper_with_alias_support(): void {
    +  \Drupal::database()->schema()->dropTable('router');
    +  \Drupal::service('router.builder')->rebuild();
    +}
    

    Is this a case where it would be better to use ->setRebuildNeeded(). Looking at what post update is doing, it does flush all caches afterwards, which would rebuild the router if needed.

  • 🇷🇴Romania amateescu

    Fixed the feedback from #41 and #43.

    Local tasks are not broken for an aliased route, so I removed the change from LocalTaskManager and the new getUnAliasedRouteName() which didn't have a purpose anymore.

    For aliasing user.well-known.change_password to user.edit, after reading https://symfony.com/blog/new-in-symfony-5-4-route-aliasing I think that's not a valid example, because the two routes are not pointing to the same (URL) path.

    Wrote an upgrade path test, so we only need some test coverage for the new RouteProvider::getRouteAliases() method now.

    As for triggering deprecation errors for local tasks, I don't think that's necessary, the current trigger_error() from RouteProvider::getRouteByName() is enough IMO. And combined with 📌 Add option to log deprecation errors to prepare for Drupal 11 Needs work , it should provide enough visibility for deprecated routes.

  • Merge request !6738Add support for route aliases. → (Open) created by amateescu
  • Pipeline finished with Failed
    11 months ago
    Total: 168s
    #101428
  • 🇫🇷France andypost

    Thank you! Still needs work to fix CS and

    only need some test coverage for the new RouteProvider::getRouteAliases() method now.

  • First commit to issue fork.
  • Pipeline finished with Success
    11 months ago
    Total: 803s
    #101959
  • 🇫🇷France andypost

    and it shou8ld prove a test with deprecated route - let it be test module

  • 🇫🇷France andypost

    hide patches as MR is used

  • Pipeline finished with Canceled
    about 1 month ago
    Total: 90s
    #371633
  • Pipeline finished with Failed
    about 1 month ago
    Total: 139s
    #371634
  • Pipeline finished with Failed
    about 1 month ago
    Total: 181s
    #371640
  • Pipeline finished with Success
    about 1 month ago
    Total: 517s
    #371643
  • 🇷🇴Romania amateescu

    Added some test coverage in \Drupal\Tests\system\Functional\Routing\RouterTest but I think we need to add more in \Drupal\KernelTests\Core\Routing\RouteProviderTest, so leaving at NW.

  • Pipeline finished with Failed
    about 1 month ago
    Total: 837s
    #371711
  • Pipeline finished with Failed
    about 1 month ago
    Total: 564s
    #371735
  • Pipeline finished with Failed
    about 1 month ago
    Total: 533s
    #372258
  • 🇷🇴Romania amateescu

    Added the remaining test coverage needed for \Drupal\Core\Routing\RouteProvider::getRouteAliases(), the MR is ready for final reviews now!

  • Pipeline finished with Failed
    about 1 month ago
    Total: 122s
    #375180
  • Pipeline finished with Success
    about 1 month ago
    Total: 481s
    #375184
  • 🇫🇷France andypost

    Looks great and ready to go!

    Just not sure about return type which I think which be iterable (traversable) instead of array

  • First commit to issue fork.
  • Pipeline finished with Success
    7 days ago
    Total: 5750s
    #393860
  • Pipeline finished with Canceled
    7 days ago
    Total: 129s
    #393950
  • Pipeline finished with Success
    7 days ago
    Total: 536s
    #393952
  • Pipeline finished with Success
    7 days ago
    Total: 547s
    #393986
  • 🇫🇷France andypost

    the only question is array vs iterable

  • Pipeline finished with Failed
    7 days ago
    Total: 1893s
    #394000
  • 🇫🇷France andypost

    any somehow update hooks fails on mysql

    Run updates
           ┐
           ├ The update failed with the following message: "Failed: Drupal\Core\Database\SchemaException: MySQL needs the 'alias' field specification in order to normalize the 'alias' index in Drupal\mysql\Driver\Database\mysql\Schema->getNormalizedIndexes() (line 332 of /builds/issue/drupal-3159210/core/modules/mysql/src/Driver/Database/mysql/Schema.php)."
           │
           │ /builds/issue/drupal-3159210/core/tests/Drupal/Tests/UpdatePathTestTrait.php:68
           │ /builds/issue/drupal-3159210/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php:196
           │ /builds/issue/drupal-3159210/core/modules/workspaces/tests/src/Functional/Update/WorkspaceAssociationStringIdsUpdatePathTest.php:42
  • Pipeline finished with Failed
    7 days ago
    Total: 2796s
    #394010
  • Pipeline finished with Failed
    7 days ago
    Total: 529s
    #394069
  • Pipeline finished with Failed
    6 days ago
    Total: 364s
    #394617
  • Pipeline finished with Failed
    6 days ago
    Total: 345s
    #394623
  • Pipeline finished with Failed
    6 days ago
    Total: 2082s
    #394629
  • 🇫🇷France andypost

    upgrade fails on mysql (every upgrade test) https://git.drupalcode.org/issue/drupal-3159210/-/jobs/4004297

    pgsql fails only 1 test RouteProviderTest https://git.drupalcode.org/issue/drupal-3159210/-/jobs/4004507

    sqlite pass all

  • 🇬🇧United Kingdom catch
      ├ The update failed with the following message: "Failed: Drupal\Core\Database\SchemaException: MySQL needs the 'alias' field specification in order to normalize the 'alias' index in Drupal\mysql\Driver\Database\mysql\Schema->getNormalizedIndexes() (line 332 of /builds/issue/drupal-3159210/core/modules/mysql/src/Driver/Database/mysql/Schema.php)."
    

    To me this looks broken in the MySQL driver.

    ::addField() calls ::createKeysSql(), ::createKeysSql() calls ::getNormalizedIndexes(), ::getNormalizedIndexes() expects a $spec['fields'] to be passed in with the field definition(s) and that is not done.

  • Pipeline finished with Success
    6 days ago
    Total: 813s
    #394788
  • Pipeline finished with Success
    6 days ago
    Total: 3462s
    #394813
  • 🇫🇷France andypost

    @amateescu Thank you fixing update hook! Probably it needs follow-up and todo added to allow add field with index to mysql table in one transaction

  • 🇫🇷France andypost

    The only failure is pgsql with strange warning

        1) /builds/issue/drupal-3159210/core/lib/Drupal/Core/Routing/RouteProvider.php:267
        unserialize(): Error at offset 44 of 48 bytes
  • 🇫🇷France andypost

    on pgsql able to reproduce, the failing value is

    php -r 'var_dump(unserialize(hex2bin("4f3a33313a2253796d666f6e795c436f6d706f6e656e745c526f7574696e675c416c696173223a323a7b733a34343a22")));'
    PHP Warning:  unserialize(): Error at offset 44 of 48 bytes in Command line code on line 1
    PHP Stack trace:
    PHP   1. {main}() Command line code:0
    PHP   2. unserialize($data = 'O:31:"Symfony\\Component\\Routing\\Alias":2:{s:44:"') Command line code:1
    Command line code:1:
    bool(false)
    
  • 🇫🇷France andypost

    Fixed pgsql and filed issue for mysql schema 🐛 Mysql Schema::addField() incorectly processing indexes specification Active

  • Pipeline finished with Success
    6 days ago
    Total: 2563s
    #394904
  • Pipeline finished with Success
    6 days ago
    Total: 1159s
    #394953
  • 🇷🇴Romania amateescu

    @andypost, thanks for fixing the pgsql issue, I was really scratching my head around it. Reverted your latest commit, we can't add them at the same time until the issue you just opened is fixed, and it's also not worth waiting for it :)

  • Pipeline finished with Success
    6 days ago
    Total: 6503s
    #394978
  • 🇳🇱Netherlands daffie

    Back to needs work for my remark on the PR.

  • 🇷🇴Romania amateescu

    Replied to the MR comment.

  • 🇫🇷France andypost

    I think it's ready for commiters

Production build 0.71.5 2024