- Assigned to larowlan
- Issue was unassigned.
- Status changed to Needs review
about 2 years ago 8:51am 19 January 2023 - Status changed to Needs work
about 2 years ago 9:03am 19 January 2023 - Status changed to Needs review
about 2 years ago 9:50am 19 January 2023 - 🇬🇧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 11:59pm 30 January 2023 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 6:49am 31 January 2023 - 🇮🇳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 9:43am 31 January 2023 - 🇫🇷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. - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
This blocks:
- 📌 [PP-1] NodeRouteProvider should extend DefaultHtmlRouteProvider Needs work
- 📌 [PP-1] Use entity route providers for all content entity types Active
- 📌 [PP-1] Properly deprecate routes editor.image_dialog, editor.link_dialog and editor.media_dialog Postponed
… and probably more. So tagging .
- 🇬🇧United Kingdom joachim
The IS doesn't mention the plan that's in the title.
- 🇫🇷France andypost
-
+++ 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
-
+++ 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 newgetUnAliasedRouteName()
which didn't have a purpose anymore.For aliasing
user.well-known.change_password
touser.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()
fromRouteProvider::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. - 🇫🇷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.
- 🇫🇷France andypost
and it shou8ld prove a test with deprecated route - let it be test module
- 🇷🇴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. - 🇷🇴Romania amateescu
Added the remaining test coverage needed for
\Drupal\Core\Routing\RouteProvider::getRouteAliases()
, the MR is ready for final reviews now! - 🇫🇷France andypost
Looks great and ready to go!
Just not sure about return type which I think which be
iterable
(traversable) instead ofarray
- First commit to issue fork.
- 🇫🇷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
- 🇫🇷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/4004507sqlite 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.
- 🇫🇷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
- 🇷🇴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 :)