When adding aliases through the admin interface, there is a validation check (function aliasExists) to make sure you are not reusing an existing alias. Strangely the function does not prevent you from making a duplicate alias.

To reproduce, go to /admin/config/search/path/add and add an alias with a source. Then do it again (same alias, same source).

If you use different sources you will be stopped from reusing an alias, but nothing stops you from adding a duplicate. Why allow that?

CommentFileSizeAuthor
#69 interdiff_66-69.txt10.64 KBvsujeetkumar
#69 2350135-69.patch14.19 KBvsujeetkumar
#66 interdiff.txt4.87 KBKapilV
#66 2350135-66.patch3.68 KBKapilV
#62 2350135-interdiff-57-62.txt3.47 KBcburschka
#62 2350135-62.patch6.1 KBcburschka
#61 2350135-interdiff-57-61.txt3.48 KBcburschka
#61 2350135-61.patch6.12 KBcburschka
#57 2350135-interdiff-54-57.txt1.93 KBcburschka
#57 2350135-57.patch2.63 KBcburschka
#54 2350135-interdiff-53-54.txt1.12 KBcburschka
#54 2350135-54.patch2.97 KBcburschka
#53 2350135-interdiff-50-53.txt931 bytescburschka
#53 2350135-53.patch2.7 KBcburschka
#50 2350135-50.patch1.79 KBcburschka
#50 2350135-50-testonly.patch1.03 KBcburschka
#28 fixed-issue-23-2350135-28.patch8.09 KBdeepakaryan1988
#23 path_exists-2350135-23.patch8.09 KBnitvirus
#20 path_exists-2350135-20.patch8.09 KBnitvirus
#16 path_exists-2350135-16.patch8.07 KBnitvirus
#13 interdiff.txt5.07 KBslashrsm
#13 2350135_13.patch8.01 KBslashrsm
#9 interdiff.txt3.55 KBslashrsm
#9 2350135_9.patch4.93 KBslashrsm
#4 2350135_3.patch3 KBslashrsm
#3 2350135_3_TEST_ONLY.patch831 bytesslashrsm
#1 path-exists.patch588 byteshexblot
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hexblot’s picture

Status: Active » Needs review
FileSize
588 bytes

It seems like a gross oversight that I hope I'm wrong about but… it seems that AliasStorage->aliasExists() has the opposite logic of the one intended.

Attaching patch reversing the logic, let's see if the Testbot finds anything that fails due to this…

(PS yes the above patch solves the issue at hand, and properly reports the path as duplicate )

Status: Needs review » Needs work

The last submitted patch, 1: path-exists.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
831 bytes

It seems we need to keep $source check for edit forms and omit it for add forms. Also added test coverage.

slashrsm’s picture

FileSize
3 KB
slashrsm’s picture

Issue tags: +Amsterdam2014

The last submitted patch, 3: 2350135_3_TEST_ONLY.patch, failed testing.

Primsi’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/path/src/Form/AddForm.php
@@ -33,4 +34,18 @@ protected function buildPath($pid) {
+      $form_state->setErrorByName('alias', t('The alias %alias is already in use in this language.', array('%alias' => $alias)));

If the language is not specified this is a very strange error message.

Are we sure we have test coverage for all of the changes made here?

slashrsm’s picture

Status: Needs work » Needs review
FileSize
4.93 KB
3.55 KB

Re-used message that was already there. Agreed with @alexpott to create two versions of the error message.

slashrsm queued 9: 2350135_9.patch for re-testing.

Primsi’s picture

Status: Needs review » Reviewed & tested by the community

Interdiff looks ok. Approving again.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/path/src/Form/AddForm.php
    @@ -33,4 +34,23 @@ protected function buildPath($pid) {
    +      else {
    +        $form_state->setErrorByName('alias', t('The alias %alias is already in use in this language.', array('%alias' => $alias)));
    +      }
    
    +++ b/core/modules/path/src/Form/PathFormBase.php
    @@ -156,7 +156,12 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
    +      else {
    +        $form_state->setErrorByName('alias', t('The alias %alias is already in use in this language.', array('%alias' => $alias)));
    +      }
    

    Do we have test coverage for these code paths?

  2. +++ b/core/modules/path/src/Plugin/Field/FieldWidget/PathWidget.php
    @@ -90,8 +90,16 @@ public static function validateFormElement(array &$element, FormStateInterface $
    -      $is_exists = \Drupal::service('path.alias_storage')->aliasExists($alias, $element['langcode']['#value'], $element['source']['#value']);
    -      if ($is_exists) {
    +      if (empty($element['pid']['#value'])) {
    +        $alias_exists = \Drupal::service('path.alias_storage')
    +          ->aliasExists($alias, $element['langcode']['#value']);
    +      }
    +      else {
    +        $alias_exists = \Drupal::service('path.alias_storage')
    +          ->aliasExists($alias, $element['langcode']['#value'], $element['source']['#value']);
    +      }
    +
    +      if ($alias_exists) {
    

    This change seems to be missing test coverage

slashrsm’s picture

Status: Needs work » Needs review
FileSize
8.01 KB
5.07 KB
  1. We have for add form, but we didn't for edit form. Added that.
  2. Added.

Updated tests revealed some other conditions where duplicates could be created. Fixed that. Also added unique key to url_alias schema. I think it makes sense to enforce it on DB level too. This way we really make sure there are no duplicates created.

mgifford queued 13: 2350135_13.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 13: 2350135_13.patch, failed testing.

nitvirus’s picture

Status: Needs work » Needs review
FileSize
8.07 KB

The Patch wasn't working so re-rolling the patch made by @slashrsm.

Status: Needs review » Needs work

The last submitted patch, 16: path_exists-2350135-16.patch, failed testing.

Manjit.Singh’s picture

Issue tags: +india, +SprintWeekend2015
Anonymous’s picture

The patch overall looks good. Some minor feedback though:

  1. +++ b/core/modules/path/src/Form/AddForm.php
    @@ -32,5 +33,24 @@ protected function buildPath($pid) {
    +    // Language is only set if language.module is enabled, otherwise save for all
    +    // languages.
    

    This should be wrapped on 80 chars, so "all" will need to go to the next line.

  2. +++ b/core/modules/path/src/Form/AddForm.php
    @@ -32,5 +33,24 @@ protected function buildPath($pid) {
    +        $form_state->setErrorByName('alias', t('The alias %alias is already in use.', array('%alias' => $alias)));
    

    Can't we use $this->t() there?

  3. +++ b/core/modules/path/src/Form/AddForm.php
    @@ -32,5 +33,24 @@ protected function buildPath($pid) {
    +        $form_state->setErrorByName('alias', t('The alias %alias is already in use in this language.', array('%alias' => $alias)));
    

    Can't we use $this->t() there?

nitvirus’s picture

Ok,

Uploading the patch again, with the changes.

Manjit.Singh’s picture

@pjonckiere Is there any difference between 2nd and 3rd point in #19

Anonymous’s picture

Yes, those are two separate lines: 49 & 52.

+++ b/core/modules/path/src/Form/AddForm.php
@@ -32,5 +33,24 @@ protected function buildPath($pid) {
+    // Language is only set if language.module is enabled, ¶
+    // otherwise save for all languages.

The first line has to be wrapped as close to 80 chars as possible. Only "all" should have be moved to the second line.

nitvirus’s picture

Thanks for replying so soon.

Uploading the patch again.

Anonymous’s picture

Status: Needs work » Needs review

Thanks! Let's trigger the test bot.

The last submitted patch, 20: path_exists-2350135-20.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 23: path_exists-2350135-23.patch, failed testing.

deepakaryan1988’s picture

Assigned: Unassigned » deepakaryan1988
deepakaryan1988’s picture

Re-rolling the patch which in comment #23.

deepakaryan1988’s picture

Assigned: deepakaryan1988 » Unassigned
Anonymous’s picture

Status: Needs work » Needs review

And triggering testbot again.

Please provide interdiffs, as it makes it easier to review the changes in between patches. See the guideline on creating an interdiff for more info.

I see #22 was not addressed yet. And I noticed another nitpick (see below).

I'll do a more thorough review once this gets green again.

+++ b/core/modules/system/system.install
@@ -1036,6 +1036,9 @@ function system_schema() {
+      'unique keys' => array(
+      'source_alias' => array('alias', 'source'),
+    ),

The indentation of the first line is not right. There should be two less spaces.

Status: Needs review » Needs work

The last submitted patch, 28: fixed-issue-23-2350135-28.patch, failed testing.

deepakaryan1988’s picture

ok @pjonckiere .
Sorry for this. I will make sure to attach interdiff in the future.

deepakaryan1988’s picture

Removing sprint weekend tag!!
As suggested by @YesCT

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

osopolar’s picture

Duplicated path entries could also be result of update-migrations, as described in Migrate: Duplicate entries in url_alias after update migrations (on Drupal Answers). Not sure if it could be part of this issue or should another issue for migration be opened. I think the problem is the same, there is nothing that prevents inserting the same path twice. In My opinion there should be a check if data already exists outside of form submission logic.

I set the url alias in the migration templates like:

process:
  path/alias: source_path
  path/pathauto:
    plugin: default_value
    default_value: 0

Setting the path that way works fine on importing content with drush (i.e. drush migrate-import term). But when I update the terms with drush migrate-import term --update a new but the same alias gets inserted into the url_alias table. For example the term with tid 1 gets imported once and updated 2 times there will be the following aliases in url_alias table:

select * from url_alias where source = '/taxonomy/term/1';
+------+------------------+---------------+----------+
| pid  | source           | alias         | langcode |
+------+------------------+---------------+----------+
| 1514 | /taxonomy/term/1 | /term-1-alias | de       |
| 2731 | /taxonomy/term/1 | /term-1-alias | de       |
| 3632 | /taxonomy/term/1 | /term-1-alias | de       |
+------+------------------+---------------+----------+

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mvbaalen’s picture

@osopolar: I use the following workaround for the url_alias duplicates created by migrations until this issue is fixed:

Migrate: Duplicate entries in url_alias after update migrations - 85986 (on Drupal Answers)

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

TomiMikola’s picture

In case someone desperately wants to do cleanup this SQL query seems to work fine by removing the duplicates:
https://stackoverflow.com/a/4075640

DELETE t0
FROM url_alias AS t0
JOIN url_alias AS t1 ON t1.source = t0.source AND t1.pid < t0.pid;
idebr’s picture

kellyimagined’s picture

#42 was a great solution!

tamasd’s picture

I can reproduce this issue with JSONAPI. I save multiple nodes with the same path alias from an external service. My initial understanding was that the path alias gets overridden. However, those nodes can't be saved again because I get the "The alias ... is already in use in this language." error message.

Digging deeper, I found that path alias entities get duplicated in the database. The AliasRepository::lookupByAlias() function orders the records by id DESC, so the latest one will be loaded. However, UniquePathAliasConstraintValidator::validate() uses a different logic, and I can't save the node anymore on the UI.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

lhguerra’s picture

I'm writting just to point ou that it also happens when I set an alias manually in a node. I'll try to force using pathauto in every case for my project, since it already does what I want, just not when I want.

cburschka’s picture

At a glance, it looks like the code has diverged too much for the last patch to still be viable. See also #3092090: Remove legacy Path Alias subsystem.

However, the problem still exists in 9.1.x: Duplicate aliases are allowed if the source is also identical.

The validation code is now in Drupal\Core\Path\Plugin\Validation\Constraint\UniquePathAliasConstraintValidator::validate().

It's not clear to me that changing the "in this language" part of the validation message (per #8) is in the scope of this issue. That problem seems to already exist in the current code, and can probably be fixed independently?

cburschka’s picture

Version: 8.9.x-dev » 9.1.x-dev
Status: Needs work » Needs review
FileSize
1.03 KB
1.79 KB

I may grossly misunderstand the original approach, because it looks as if this one-liner should be enough.

The "path <> $path" condition simply shouldn't exist at all - any new alias is invalid if its alias value matches that of any existing alias, and any existing alias is invalid if its alias value matches that of an alias with a different ID. The path values of the aliases simply don't matter.

Status: Needs review » Needs work

The last submitted patch, 50: 2350135-50.patch, failed testing. View results

cburschka’s picture

It looks like this exposes a separate problem with the path alias field - when editing an entity with a path field, the path entity is always considered "new" even if one already exists.

cburschka’s picture

The problem is that the form validator always creates a new entity, which the constraint validator then rejects for conflicting with the existing one.

It should instead try to load the entity by its PID if possible, and only create a new entity if that fails.

Test-only patch is unchanged.

cburschka’s picture

Of course, if the entity is loaded, then its properties need to be set to the form values.

The last submitted patch, 53: 2350135-53.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 54: 2350135-54.patch, failed testing. View results

cburschka’s picture

Status: Needs work » Needs review
FileSize
2.63 KB
1.93 KB

I made an error there. If we load and set values on the "real" entity from EntityStorageBase during validation, those values leak through (thanks to the static cache in EntityStorageBase). Now I understand why a new entity has to be created.

However, we can simply make sure to set the "id" property on the newly created entity, and then alter the constraint validator to check for a non-null ID instead of "isNew()".

Status: Needs review » Needs work

The last submitted patch, 57: 2350135-57.patch, failed testing. View results

cburschka’s picture

I suspect that these failures are validations that should already have run into the "duplicate path alias" error, but did not do so until now. If that is true, then all we need to do is update the expected error messages in the test. But I'm not certain.

cburschka’s picture

To summarize the problem: The REST tests make an assumption about entities that doesn't work for PathAlias. Namely, they use a single method called ::getNormalizedPostEntity() to generate sample values for an entity, and assume that these values can remain constant.

But:

1.) If the alias value is constant, then the test will try to generate multiple entities with the same alias and fail.
2.) If the alias value is not constant, then the test will try to generate an entity, compare it to a fresh ::getNormalizedPostEntity() and fail because the values are different.

This is an existing bug which is only uncovered by fixing this one and disallowing duplicate PathAlias entities.

cburschka’s picture

Changing EntityResourceTestBase to call ::getNormalizedPostEntity() only once. This allows ::getNormalizedPostEntity() to generate different values in each call, ensuring that entity values are unique when needed.

Status: Needs review » Needs work

The last submitted patch, 62: 2350135-62.patch, failed testing. View results

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Tim Corkerton’s picture

I am running a site in parallel with a D6 site that continually feeds data up to the D8 site periodically via cron. Every time a node is updated a new alias is created.

Is there any chance of getting this issue fixed? The patch seems OK in principal.

KapilV’s picture

The patch does not apply. Hear a reroll.

KapilV’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 66: 2350135-66.patch, failed testing. View results

vsujeetkumar’s picture

Status: Needs work » Needs review
FileSize
14.19 KB
10.64 KB

"UiHelperTrait::drupalPostForm() with $submit as an object is deprecated " So updated the "drupalPostForm" according the given link "https://www.drupal.org/node/3168858", Please have a look.

Status: Needs review » Needs work

The last submitted patch, 69: 2350135-69.patch, failed testing. View results

cburschka’s picture

The last two patches should be skipped for future work. #69 changes unrelated parts of the code, and #66 (like #69) only contains the changesets for the tests, not the actual code changes.

Most recent actual patch was #62.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mxr576’s picture

This was also a known issue to me, although today I was thinking what if this is a feature and not a bug... =] just other parts of the system need to be changed to handle it.

What if we could have dedicated user journeys with dedicated path aliases for different personas for the same page? Like end-users would use the /dashboard/group/foo alias for accessing Foo Group entity's canonical page, but admin-like personas have an admin/group/foo path alias. Path aliases are content entities with entity access support...

Yes, if multiple aliases are available for a user that still needs to be handled and one should be returned (just like it happens now, thanks for the orderBy+fetchassoc in \Drupal\path_alias\AliasRepository::lookupBySystemPath())

mxr576’s picture

This problem exists since Drupal 7... (at least)

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.