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

I think the list can be grouped into two cases. URL generation and checking for the current route. Generation is reasonably easy, as done on the node patch. Checking the current route is the problem, and given how the API works, I honestly have no idea how to deal with that. I can't think of anything to make that work. It returns exactly one string that we compare, so that's a hard break.

Either we get an OK from a release manager to make that breaking change, possibly only for a major release or the issue is won't fix and we'll have to live with node not conforming to the naming standard and if necessary find alternative ways to deal with that, like support non-standard route names for link templates somehow.

Route Aliasing does look promising, would likely unblock #2723579: NodeRouteProvider should extend DefaultHtmlRouteProvider and make #3153559: Switch Node revision UI to generic UI easier to implement.

Here's initial stub to allow parse aliases - using code from https://github.com/symfony/symfony/pull/38464/files#diff-2b6a8c06ec813f6...

looks like it will throw deprecation on routeCollection::get() so needs some test to make sure it works

Our router table supports to save aliases so reusing current storage

The only issue I see is broken local tasks (no tab displayed at dblog clean-up admin/reports/dblog/confirm)

So it should be covered by tests

@jibran surely no, but this route is handy to test local tasks (is where I stuck in patch)

Using the built-in symfony mechanism sounds like a the right starting point.

Looking at the patch quickly, I'm concerned that the route saved to the table doesn't have any flag as a separate table column that it's an alias, but i guess the fact that it's not saved with a path or pattern outline means it will never be found doing a lookup by path?

I'd also, ideally, want some way in the route or route match to list the known aliases of a matched route. This could be helpful for e.g. writing a hook_help() or something else looking at the current route name that would work before and after this change.

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

Looks it's blocker for #2723615: Use entity route providers for all content entity types

@dpi thanks for #18 good idea to have it as alias

@pwolanin do we need a flag in routing table for aliases? I think as the "route" column deserialized we can determine is it alias or route.

Updated remaining tasks based on conversation, going to poke at the local tasks issue for a bit

This addresses @pwolanin's request at #16 and fixes local tasks per #12

To-do - decide if we want to trigger a deprecation when a local task references a deprecated route name.
Tests for the new methods on router provider.

Done for the time being

Patch in #22 is the same as #12, though the interdiff looks correct.

Thanks I git stashed my autoload.php changes before I committed the diff to my local branch, so stashed my changes too

Here's the correct patch for the interdiff above

Thanks 👍 I bet it needs upgrade hook and test as well because schema change

The hook is there, but the test is nitt

What do people think about triggering a deprecation if a local task points to an alias?

I think every use of deprecated route should cause a warning but it could be overkill for logs.

But local actions like tasks need it but usage in URL generation may need throttling

I think every use of deprecated route should cause a warning

Sounds like a good idea.

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.

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

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

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)

Better to use route from #18

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

This blocks:

1. #2723579: NodeRouteProvider should extend DefaultHtmlRouteProvider
2. #2723615: Use entity route providers for all content entity types
3. #3337813: Properly deprecate routes editor.image_dialog, editor.link_dialog and editor.media_dialog

… and probably more. So tagging blocker.

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

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

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

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

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 #3024296: Add option to log deprecation errors, it should provide enough visibility for deprecated routes.

Thank you! Still needs work to fix CS and

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

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

hide patches as MR is used

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.

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

Looks great and ready to go!

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

the only question is array vs iterable

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

sqlite pass all

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

    Search Block Page Id Update Path (Drupal\Tests\search\Functional\Update\SearchBlockPageIdUpdatePath)
     ✘ 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/search/tests/src/Functional/Update/SearchBlockPageIdUpdatePathTest.php:33
       ┴

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

     ✔ Outline path match zero
     ✔ Outline path no match
     ✔ Route caching
     ✔ Route by name
     ✔ Get routes by pattern with long patterns
     ✘ Route aliases
       ┐
       ├ Error: Call to a member function getId() on false
       │
       │ /builds/issue/drupal-3159210/core/tests/Drupal/KernelTests/Core/Routing/RouteProviderTest.php:767
       ┴
    
    1 test triggered 1 PHP warning:
    
    1) /builds/issue/drupal-3159210/core/lib/Drupal/Core/Routing/RouteProvider.php:267
    unserialize(): Error at offset 44 of 48 bytes
    
    Triggered by:
    
    * Drupal\KernelTests\Core\Routing\RouteProviderTest::testRouteAliases (2 times)
      /builds/issue/drupal-3159210/core/tests/Drupal/KernelTests/Core/Routing/RouteProviderTest.php:755

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

@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

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

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)

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

@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 :)

Back to needs work for my remark on the PR.

Replied to the MR comment.

I think it's ready for commiters

One question.

There is no change to RouteProvider::getRoutesByPath(), shouldn't be filtering aliases in any case where we're not explicitly loading the route by name? If not in that method then somewhere else?

@catch, there's no need to do any additional filtering in RouteProvider::getRoutesByPath() because we only store data in the name, route and alias columns, so the current conditions on pattern_outline or number_parts already exclude aliases.

Another place would be ::preLoadRoutes(), but that's called by ::getRoutesByNames() (which in turn is used by ::getRouteByName() and the new ::getRouteAliases()), so no need to filter there.

And then there's ::getAllRoutes() which is documented to return an array of Route objects and has this doxygen:

   * Usage of this method is discouraged for performance reasons. If possible,
   * use RouteProviderInterface::getRoutesByNames() or
   * RouteProviderInterface::getRoutesByPattern() instead.

Added a filter in ::getAllRoutes() and moving back to RTBC because I don't think this change requires test coverage.

Thanks for #69 that all looks sensible.

No other complaints, so committed/pushed to 11.x, thanks!

I updated the CR to have two examples and to include the message property that was missing. Also, added this to 'How to deprecate' and added alias to Structure of routes.

Thanks @quietone!

Opened a followup for this: #3506653: Add an API for comparing the (current) route name that takes into account deprecated routes