Improve table structure for {url_alias} #2

subscribe

If this is actually a bug, and not a feature request, it should be against 7.x, since 8.x is not open for development yet.

Well. After reading the original thread I vaguely understand that the unique key has been changed because there are means in the UI to prevent duplicate aliases. Well there are other methods to create content in Drupal that don't use the UI. Since march we have accumulated a bunch of duplicates and one case caused a lot of trouble for us today. I am evaluating if something has been coded improperly on our end...

But the unique key is a constraint which has a functional meaning besides improving performance. Also... introducing those kinds of changes in updates that come along with security fixes should be reviewed in general.

The mechanism guarding against duplicate aliases as of now will not work if the aliases are created at the same time. Removing the unique key constraint has opened the opportunity for duplicates to occur. This is definitely undesired behaviour (a bug) and should be fixed by either reverting to the old schema or using a lock.

If anything, we should add a new unique index on alias, language. For existing sites that have this problem it'll be necessary to have an update that finds and deletes duplicate url aliases as well.

The "bug" is hypothetical at this point, so this is a task, which might be backported to D7.

Well, it's definitely not hypothetical. I can assure you it happened on my site.

I am working on a site that has thousands of same src-dst pairs, some of those more than 70 times -- and no language in sight.

The dst_language_pid index in the OP doesn't exist anymore, is this still valid?

This is valid as long as the UNIQUE KEY constraint is missing, unless someone prefers a different solution for the problem.

I spent most of the day trying to figure out why I had duplicate entries in the url_alias table. Turns out it was because of #1737048: 2 URL aliases created when saving new node as published. If there has been a unique constraint in places, this would have been caught long ago in development, before it became a problem.

The attached patch adds the unique key. It would be best to have an update hook clear away duplicates and add the key to existing databases. This fixes it for new installations.

In AliasTest.php, the test that is causing the failure is 'Newer alias record is returned when comparing two LANGUAGE_NOT_SPECIFIED paths with the same alias.' So, this test deliberately violates the unique key that my patch adds.

I think I should change this test so that it tests that the unique constraint is working. Can I use a try()catch() block for this? Am I on the right track?

I tried putting a try()catch() block around the database insertion which ought to fail if the constraint is working, but the exception gets caught elsewhere, so the test still doesn't work.

This revised patch just removes the obsolete test.

Sorry, this is the correct patch.

#17: core_url_alias_unique_925474.patch queued for re-testing.

Reroll.

Issue summary has been updated.

This removes an unneeded test. The UNIQUE constraint cannot be tested with the Drupal testing system.

Reroll.

Tags

Create short explanation of current state of issue.

Reroll.

Maybe better to use db_merge() because there's no support for handling unique key exceptions

@andypost, I don't understand. Can you elaborate?

28: core_url_alias_unique_925474.patch queued for re-testing.

I don't understand @andypost's comment either, but must be instead of:

+ 'unique keys' => array('alias_langcode' => array('alias', 'langcode')),

This is a small patch after all.

Re-reading it, I understand: My patch would cause an attempt to insert a duplicate alias to fail, which preserves data integrity.

If in addition to my patch, the alias creation code used db_merge() instead of db_insert(), then such an attempt would update the existing alias instead of failing, which might be a preferred fail condition.

Either way, my patch can go in.

Reroll.

Reroll. This version adds the UNIQUE constraint and changes the code for adding aliases so that if you try to add an alias on the same language and path as an existing one, it instead updates the existing alias. The resulting behavior is identical to before, so no changes are required to any of the tests.

Reroll.

Patch in #49 still applies.

+++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateUrlAliasTest.php
@@ -69,7 +69,7 @@ public function testUrlAlias() {
-    $path = \Drupal::service('path.alias_storage')->load(array('pid' => $path['pid']));
+    $path = \Drupal::service('path.alias_storage')->load(array('source' => $conditions['source']));

Why is this change necessary?

I don't remember. Does it work without it? My D8 install is not working right now.

I think it is a better test: Do the test starting with the source and ensure that the right alias comes out.

This makes sense to me. It still applies nicely. Drupal 8 is going to be the most multilingual version of Drupal ever, seems kinda silly to not include this.

Tagging for i18n.

Would there be any performance issues with the merge?

suppose proper tags

I don't think the merge should cause a performance issue. It may be a little bit slower than insert, but it only runs once, not in a loop.

Does anyone understand why there are two test failures of the same item? Why does this test run twice?

Reroll without change to test.

The code already has a check for whether it should insert or update, so all that is needed is the unique constraint. This passes when I run it on my local.

OK, so the merge is needed. The only problem is that with the merge, the D6 migration stuff doesn't work. I don't understand what is going wrong there.

Thanks for plugging away at this. Would be good if we could bring in more i18n folks to review it.

This is a reroll of the last version of this patch which passes the testbot.

Patch rerolled.

Reroll. I'm getting a new error when I try this locally. I want to see if the testbot gets the same thing.

this would need an update function

@#84: Yes, I wanted to get it working first, then worry about the update hook.

I am getting the MySQL error "1071 Specified key was too long". We didn't used to get this error for that line of code. I don't know what is different. I can get around it by making the key only include the first part of the alias, but that could prevent the insertion of valid aliases.

aliases were converted to entities in #2336597: Convert path aliases to full featured entities and included adding a constraint for uniqueness, UniquePathAlias. So, I reckon this is now outdated.

Therefore, closing as outdated. If this is incorrect reopen the issue, by setting the status to 'Active', and add a comment explaining what still needs to be done.

Thanks!