Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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?
Comment | File | Size | Author |
---|---|---|---|
#69 | interdiff_66-69.txt | 10.64 KB | vsujeetkumar |
#69 | 2350135-69.patch | 14.19 KB | vsujeetkumar |
#66 | interdiff.txt | 4.87 KB | KapilV |
#66 | 2350135-66.patch | 3.68 KB | KapilV |
#62 | 2350135-interdiff-57-62.txt | 3.47 KB | cburschka |
Comments
Comment #1
hexblot CreditAttribution: hexblot commentedIt 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 )
Comment #3
slashrsm CreditAttribution: slashrsm commentedIt seems we need to keep $source check for edit forms and omit it for add forms. Also added test coverage.
Comment #4
slashrsm CreditAttribution: slashrsm commentedComment #5
slashrsm CreditAttribution: slashrsm commentedComment #7
Primsi CreditAttribution: Primsi commentedLooks good.
Comment #8
alexpottIf 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?
Comment #9
slashrsm CreditAttribution: slashrsm commentedRe-used message that was already there. Agreed with @alexpott to create two versions of the error message.
Comment #11
Primsi CreditAttribution: Primsi commentedInterdiff looks ok. Approving again.
Comment #12
alexpottDo we have test coverage for these code paths?
This change seems to be missing test coverage
Comment #13
slashrsm CreditAttribution: slashrsm commentedUpdated 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.
Comment #16
nitvirus CreditAttribution: nitvirus at Srijan | A Material+ Company commentedThe Patch wasn't working so re-rolling the patch made by @slashrsm.
Comment #18
Manjit.SinghComment #19
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedThe patch overall looks good. Some minor feedback though:
This should be wrapped on 80 chars, so "all" will need to go to the next line.
Can't we use $this->t() there?
Can't we use $this->t() there?
Comment #20
nitvirus CreditAttribution: nitvirus at Srijan | A Material+ Company commentedOk,
Uploading the patch again, with the changes.
Comment #21
Manjit.Singh@pjonckiere Is there any difference between 2nd and 3rd point in #19
Comment #22
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedYes, those are two separate lines: 49 & 52.
The first line has to be wrapped as close to 80 chars as possible. Only "all" should have be moved to the second line.
Comment #23
nitvirus CreditAttribution: nitvirus at Srijan | A Material+ Company commentedThanks for replying so soon.
Uploading the patch again.
Comment #24
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedThanks! Let's trigger the test bot.
Comment #27
deepakaryan1988Comment #28
deepakaryan1988Re-rolling the patch which in comment #23.
Comment #29
deepakaryan1988Comment #30
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedAnd 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.
The indentation of the first line is not right. There should be two less spaces.
Comment #32
deepakaryan1988ok @pjonckiere .
Sorry for this. I will make sure to attach interdiff in the future.
Comment #33
deepakaryan1988Removing sprint weekend tag!!
As suggested by @YesCT
Comment #37
osopolarDuplicated 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:
Setting the path that way works fine on importing content with drush (i.e.
drush migrate-import term
). But when I update the terms withdrush 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:Comment #40
mvbaalen CreditAttribution: mvbaalen as a volunteer commented@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)
Comment #42
TomiMikola CreditAttribution: TomiMikola at Wunder commentedIn case someone desperately wants to do cleanup this SQL query seems to work fine by removing the duplicates:
https://stackoverflow.com/a/4075640
Comment #43
idebr CreditAttribution: idebr at iO commentedComment #44
kellyimagined CreditAttribution: kellyimagined commented#42 was a great solution!
Comment #45
tamasd CreditAttribution: tamasd at Pronovix commentedI 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 byid 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.Comment #48
lhguerra CreditAttribution: lhguerra at Taller commentedI'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.
Comment #49
cburschkaAt 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?
Comment #50
cburschkaI 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.
Comment #52
cburschkaIt 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.
Comment #53
cburschkaThe 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.
Comment #54
cburschkaOf course, if the entity is loaded, then its properties need to be set to the form values.
Comment #57
cburschkaI 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()".
Comment #59
cburschkaI 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.
Comment #60
cburschkaTo 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.
Comment #61
cburschkaChanging EntityResourceTestBase to call ::getNormalizedPostEntity() only once. This allows ::getNormalizedPostEntity() to generate different values in each call, ensuring that entity values are unique when needed.
Comment #62
cburschkaBad patch.
Comment #65
Tim Corkerton CreditAttribution: Tim Corkerton as a volunteer commentedI 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.
Comment #66
KapilV CreditAttribution: KapilV as a volunteer and at Innoraft for Drupal Care, Drupal Association commentedThe patch does not apply. Hear a reroll.
Comment #67
KapilV CreditAttribution: KapilV as a volunteer and at Innoraft for Drupal Care, Drupal Association commentedComment #69
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commented"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.
Comment #71
cburschkaThe 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.
Comment #74
mxr576This 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()
)Comment #75
mxr576This problem exists since Drupal 7... (at least)