I'm migrating from Drupal 6 a set of nodes looking like this, with a pattern including one token, the title:

nid language title
605 en John Doe
4631 en John Doe
4671 en John Doe
6171 en John Doe
9341 zh-hans John Doe

The first four general aliases as expected - john-doe, john-doe-0, etc. The last, however, throws the exception "The path "/prefix/john-doe-0" collides with the route with identifier entity.node.canonical, whose path is /node/{node}"

I've stepped through AliasUniquifier, and could use another more detailed step-through but don't have time today, so capturing what I've seen so far - in isReserved(), $this->aliasManager->getPathByAlias($alias, $langcode) does not find a match on /prefix/john-doe for 'zh-hans', so returns /prefix/john-doe (it returns the passed alias if it does not resolve the alias to a path). The call to isRoute() in here saves lastRouteName as entity.node.canonical. We would want isReserved() to return FALSE, leading to the creation of alias /prefix/john-doe with language zh-hans, but instead isRoute() returns TRUE so isReserved() returns TRUE. It then tries again with /prefix/john-doe-0 - this time, though, isRoute() is seeing a _route of entity.node.canonical, which matches lastRouteName, so it throws the exception. Boom!

CommentFileSizeAuthor
#19 interdiff.txt728 bytesmikeryan
#19 route_collision-2718375-19.patch7.38 KBmikeryan
#16 interdiff.txt2.92 KBmikeryan
#16 route_collision-2718375-16.patch6.58 KBmikeryan
#16 route_collision-2718375-16-FAIL.patch1.19 KBmikeryan
#9 interdiff.txt3.8 KBmikeryan
#9 route_collision-2718375-9.patch7.17 KBmikeryan
#9 route_collision-2718375-9-FAIL.patch1.96 KBmikeryan
#2 use-route-provider-2718375-2.patch4.53 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mikeryan created an issue. See original summary.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Priority: Normal » Critical
Issue tags: +beta blocker, +Needs tests
FileSize
use-route-provider-2718375-2.patch4.53 KB

Nice catch.

I think there are some existing issues which describe the same problem but so far, we didn't identify the reason. This is at least one of them.

I've been looking into this a bit and I might have found a way to make it work. Instead of the router/url matcher, this now directly uses the lower-level route provider, then we can avoid that it looks up aliases and only matches on actual routes.

This currently also avoids matching on routes with slugs/placeholders. I'm not 100% sure about that. While it fixes a whole bunch of other issues as well, it has also certain risks: It will no longer match on user/bla, which is what we want, but it will also not match on user/1. But I think it can be argued that that's a misconfiguration. could be user/9999 and that user is then later on created, so we have no guarantee anyway.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: use-route-provider-2718375-2.patch, failed testing.

The last submitted patch, 2: use-route-provider-2718375-2.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

@Berdir I think this is right way to go... on c3.com there is a view with the path blog/%user and blogs have an automatic alias of blog/%title. Yes you can argue this is a misconfiguration. But if you manually add the alias to the blog it works and if you using pathauto it does not. For me, pathauto should only do the validation core does when adding an alias. Maybe though there should be a warning somewhere which the site owner can choose to ignore.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Virtuoso Performance for North Studio commented

I can confirm this patch works for my use case - looks like the test failures just mean the tests need to be updated.

  1. +++ b/src/AliasUniquifier.php
@@ -74,14 +63,14 @@ class AliasUniquifier implements AliasUniquifierInterface {
    *   The url matcher service.

    The route provider service. 

  2. +++ b/src/AliasUniquifier.php
@@ -118,6 +107,7 @@ class AliasUniquifier implements AliasUniquifierInterface {
+        $this->lastRouteName = NULL;

@@ -167,20 +157,17 @@ class AliasUniquifier implements AliasUniquifierInterface {
+    $routes = $this->routeProvider->getRoutesByPattern($path);
...
+        return TRUE;

    lastRouteName no longer exists.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

Correct, we need to update those existing tests and also add some new tests to cover at least your use case.

Should be fairly easy, just create a node with the same title in two different languages. Any chance you can look into that? Not sure when I will find time.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Virtuoso Performance for North Studio commented
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
route_collision-2718375-9-FAIL.patch1.96 KB
route_collision-2718375-9.patch7.17 KB
interdiff.txt3.8 KB

Here we go... But in hindsight maybe I should've put the new test in PathautoLocaleTest?

The last submitted patch, 9: route_collision-2718375-9-FAIL.patch, failed testing.

The last submitted patch, 9: route_collision-2718375-9-FAIL.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 9: route_collision-2718375-9.patch, failed testing.

The last submitted patch, 9: route_collision-2718375-9.patch, failed testing.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Virtuoso Performance for North Studio commented

The failure occurs here in PathautoLocaleTest::testLanguageAliases (which looks like the place I should have added my test, but anyway...):

    // Check that the new node had a unique alias generated with the '-1'
    // suffix.
    $this->assertEntityAlias($node, '/content/english-node-1', LanguageInterface::LANGCODE_NOT_SPECIFIED);

Isn't this test wrong? Shouldn't it be creating alias /content/english-node-0 with langcode 'und'? The test is creating aliases in this order:

alias langcode
/english-node en drupalCreateNode (pathauto disabled)
/french-node fr saveEntityAlias
/content/english-node und saveAlias
/content/english-node-0 en node save() with pathauto enabled

At this point we do

    $node = $this->drupalCreateNode(array('title' => 'English node', 'langcode' => LanguageInterface::LANGCODE_NOT_SPECIFIED));

Which should create /content/english-node-0 with langcode und, not /content/english-node-1 as is being asserted, right?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

I think so too. I think this was some side effect of the old code that actually ended up doing two loops and did -1.

+++ b/src/Tests/PathautoNodeWebTest.php
@@ -146,6 +147,38 @@ class PathautoNodeWebTest extends WebTestBase {
   /**
+   * Tests that nodes with the same title but different languages are properly
+   * handled.
+   */
+  public function testNodeDifferentLanguages() {

this isn't really node specific, I agree that it would be better in PathautoLocaleTest. Or PathautoKernelTest because we do't need the UI.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Virtuoso Performance for North Studio commented
Status: Needs work » Needs review
FileSize
route_collision-2718375-16-FAIL.patch1.19 KB
route_collision-2718375-16.patch6.58 KB
interdiff.txt2.92 KB

Capturing where I am with this - the weird thing here is, the "fail" test actually succeeds. Almost exactly the same code failed as expected as a simpletest, but moved to the kernel tests it appears to work.

Status: Needs review » Needs work

The last submitted patch, 16: route_collision-2718375-16.patch, failed testing.

The last submitted patch, 16: route_collision-2718375-16.patch, failed testing.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Virtuoso Performance for North Studio commented
Status: Needs work » Needs review
FileSize
route_collision-2718375-19.patch7.38 KB
interdiff.txt728 bytes

Lost my fix to the broken test on the last go-round...

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous at Netuxo Ltd (RIP) commented

Applying that patch and enabling a pattern I created before gives me:

The website encountered an unexpected error. Please try again later.
Recoverable fatal error: Argument 4 passed to Drupal\pathauto\AliasUniquifier::__construct() must implement interface Drupal\Core\Routing\RouteProviderInterface, instance of Symfony\Cmf\Component\Routing\ChainRouter given, called in /var/www/clients/client0/web1/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 284 and defined in Drupal\pathauto\AliasUniquifier->__construct() (line 69 of modules/pathauto/src/AliasUniquifier.php).

Drupal\pathauto\AliasUniquifier->__construct(Object, Object, Object, Object, Object) (Line: 284)
Drupal\Component\DependencyInjection\Container->createService(Array, 'pathauto.alias_uniquifier') (Line: 177)
Drupal\Component\DependencyInjection\Container->get('pathauto.alias_uniquifier', 1) (Line: 494)
Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters(Object) (Line: 236)
Drupal\Component\DependencyInjection\Container->createService(Array, 'pathauto.generator') (Line: 177)
Drupal\Component\DependencyInjection\Container->get('pathauto.generator') (Line: 158)
Drupal::service('pathauto.generator') (Line: 156)
Drupal\pathauto\Entity\PathautoPattern->preSave(Object) (Line: 434)
Drupal\Core\Entity\EntityStorageBase->doPreSave(Object) (Line: 389)
Drupal\Core\Entity\EntityStorageBase->save(Object) (Line: 259)
Drupal\Core\Config\Entity\ConfigEntityStorage->save(Object) (Line: 358)
Drupal\Core\Entity\Entity->save() (Line: 637)
Drupal\Core\Config\Entity\ConfigEntityBase->save() (Line: 51)
Drupal\pathauto\Form\PatternEnableForm->submitForm(Array, Object)
call_user_func_array(Array, Array) (Line: 111)
Drupal\Core\Form\FormSubmitter->executeSubmitHandlers(Array, Object) (Line: 51)
Drupal\Core\Form\FormSubmitter->doSubmitForm(Array, Object) (Line: 583)
Drupal\Core\Form\FormBuilder->processForm('pathauto_pattern_enable_form', Array, Object) (Line: 314)
Drupal\Core\Form\FormBuilder->buildForm(Object, Object) (Line: 74)
Drupal\Core\Controller\FormController->getContentResult(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 574)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
call_user_func_array(Object, Array) (Line: 139)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 62)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 98)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 77)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 50)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 628)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

You need to clear caches

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous at Netuxo Ltd (RIP) commented

Thanks.
1. patch
2. drush cr all

It works!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

I guess the patch should include an empty update function to force a container rebuild then?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

If someone wants to add that, fine with me, but pathauto is only alpha at the moment, so I don't care too much.

@alexpott: Re #6, that argument doesn't completely work. See discussion in #2655874: Improve error handling for "The given alias pattern always matches the route", core doesn't do any validation for routes at all for aliases. I can, for example, create an alias from /user/login to /haha-now-you-can-not-login-anymore :) And if you use /user/[user:name] for example, then it just takes someone to create a user called login and you have a fun problem.

So, we can't do it exactly like core. But probably core should have the same validation?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH for Acquia commented
Status: Needs review » Fixed

The related issues and also this got a bunch of confirmations that this works. Committed.

  • Berdir committed 5a0bd95 on 8.x-1.x authored by mikeryan 
    Issue #2718375 by mikeryan, Berdir: Fixed route collision reported when...

Status: Fixed » Closed (fixed)

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