system_update_8804() fails if any path alias is created before it runs

@pameeela, did you update the code for Pathauto to 1.6-beta and Drupal core to 8.8.0 at the same time?

Are you sure that you didn't copy the database without emptying it first?

if you already have a path_alias table from an earlier update, it might fail like this?

drush sql-sync doesn't delete extra tables, when testing updates, always run drush sql-drop first.

And if you run this SQL statement: SELECT * FROM `url_alias` WHERE `pid` = 7142; on the non-updated database, do you get a single result?

Run into a related-issue when trying to upgrade my Drupal 8.7.8 to 8.8.0-rc1

Pathauto version: 8.x-1.6

Run drush updb -y

Get this :

[notice] Update started: system_update_8804
[error]  SQLSTATE[42S02]: Base table or view not found: 1146 Table 'mywebsite.path_alias' doesn't exist:...

That's not the same problem, I suggest you create a new issue. We'll need a full stack trace for that, might be that installing the module triggers some kind of contrib/custom code.

I can confirm that issue appears when going back and forward from Drupal 8.8.x to 8.7.x and back as I was struggling with the uptdate from D8.7 to D8.8 as per default installed 8.8.x-dev and I had to roll back.
Just drop tables restore backup and ru updates again solved the issue.

In response of #11, when executing database update on 8.8.x, tables path_alias and path_alias_revision are created.

Going back to 8.7.x and restore database using mysql source will just update data from table known in 8.7.x version. Table path_alias and path_alias_revision still exist, they are not deleted.

Going back to 8.8.x and executing database update will try to create path_alias and path_alias_revision (no error), but create data in (failed because data already exist).

To resolve this problem, when update to 8.8x and tables path_alias_* exists, just drop these table. Entity update will be a success.

I'm having this same issue upgrading from 8.7.8 to 8.8.1
pathauto 1.6 was already installed in advance of this update.

Update: I saw in another thread with a similar issue a suggestion to disable the pathauto module before running UPDB and CIM.This worked in my case- and I'm updating from 8.7.8 to 8.8.1 where pathauto 1.6 was already previously updated well in advance.

After the initial failure where this error popped up (and I lost all of my path aliases as a result):

1. restored the DB from a backup and rolled the code branch back to the previous commit
2. disabled pathauto via drush
3. deployed the recent code commit containing the upgrade to drupal core 8.8.1 from 8.7.8
4. ran updb and then all the updates were successful- no errors.
5. This was followed by CIM where pathauto was re-activated again and all seemed happy.

In my case this had nothing to do with the DB pre-update having the path_alias and path_alias_revision table, as they did not exist before the update I tried to run for drupal core 8.8.1.

I had a different scenario which I tracked down to a hook_modules_installed() implementation that did some basic sanity checking after any new module is installed and that ended up calling save on an entity. Hence after system_update_8803() I ended up with a single entry in the path_alias table and the update failed.

My specific case is presumably unusual but there could be lots of other cases with the same effect. system_update_8804() is extremely sensitive to anything else on the system saving any entities in the time interval after updating the code files and before the update hook runs.

It seems like it would be much better if this hook could be adapted to be more resilient. For example just skip any rows in url_alias where the row in path_alias already exists. I suspect that many sites have yet to be upgraded to D8.8 so this would help the community. However I don't have time to provide a patch; I've solved my own problem and added some notes here for others. If the maintainers want to close this again then I'm not going to argue.

We hit this issue on a site where core updated from 8.7.10 to 8.8.1, and then there was a delay before the database updates were run. Looking at the update script, it seems to be atomic -- it processes all entries in the url_alias table, and at the end deletes that -- creating path_alias entities for each entry.

So I truncated the path_alias and path_alias_revision tables, and reran the updates, and this appeared to resolve the issue and successfully migrate the urls.

According to the codes from line 2459 to 2467 in system.install

 // If we're not in maintenance mode, the number of path aliases could change
    // at any time so make sure that we always use the latest record count.
    $missing = $database->select('url_alias', 't')
      ->condition('t.pid', $sandbox['current_id'], '>')
      ->orderBy('pid', 'ASC')
      ->countQuery()
      ->execute()
      ->fetchField();
    $sandbox['#finished'] = $missing ? $sandbox['progress'] / ($sandbox['progress'] + (int) $missing) : 1;

If the site is not in the Maintenance Mode while the database updates is being conducted, in an extreme scenario, such as big amounts of urls are inserted into the old 'url_alias' table, there might be a delay and then might be a duplicated entity id.
So make sure your site is in the Maintenance Mode while updating. You also need to make sure that the path_alias table doesn't exist before updating, if you updated the core to 8.8.0 for your site before.

In another extreme scenario where there a huge number of records in the old url_alias table, it might require more time to move those records between url_alias and path_alias table. So in this case, you need to make sure having enough resource (memory, time-out) for the update.

Tagging as a requirement for Drupal 9.0-beta1.

Re-titling this to make it clearer what's going on.

Updates that are saving entities should always happen in hook_post_update_NAME() for this reason, or at minimum declare a dependency on this update in hook_update_dependencies().

Some questions:

• Is it too late to move the guts of system_update_8804() into a new hook_post_update_NAME()?
• system_update_8804() uses a sandbox to run through all records, are people still hitting timeout errors?

• or at minimum declare a dependency on this update in hook_update_dependencies()

  What update is dependent upon this one?

• Shouldn't system_update_8805() have been made a hook_post_update_NAME() too? At the very least it depends upon the changes in 8804.

@DamienMcKenna I think there's a misunderstanding with #20-#21, I was referring to contrib updates.

system_update_8804() doesn't save entities using the entity API, it does direct queries (for speed mainly). It's intended to both replace the path alias schema and convert all the existing records before anyone else would touch a path alias, so needs to be a hook_update_N(). system_update_8805() is also messing with path alias field/entity definitions so is better as a hook_update_N(). If we moved it to a post-update as-is, then any other post update could also cause the fatal error described here.

Contributed/custom updates that result in saving entities should be moved to post updates, or they can add a dependency on system_update_8804() so that it runs before they do. I don't think there are known updates that do this, #15 refers to a (custom?) hook_modules_installed() implementation which is a bit different.

One thing I thought of would be adding a hook_path_alias_presave() which checks if the schema verson is > 8804 and throws an exception if it's not, this would cause anything trying to run before this update to fail, but with a clearer error message, although it'd be runtime code we'd need to leave in throughout Drupal 8.8 and 8.9.

There is a definite issue affecting the path alias update on some sites with #2917600: update_fix_compatibility() puts sites into unrecoverable state which has clearer steps to reproduce.

> Contributed/custom updates that result in saving entities should be moved to post updates, or they can add a dependency on system_update_8804() so that it runs before they do. I don't think there are known updates that do this, #15 refers to a (custom?) hook_modules_installed() implementation which is a bit different.

Yes, the problem with 8804 is that it installs a module, which calls that hook, which understandably doesn't check for being called during updates. But it also has to do that, I don't really see any other way.

As nice as making the path_alias API a module is going to be in the future, with making it optional and so on, we certainly paid the price for it with all those edge cases of people getting the module installed halfway or other stuff breaking.

One thing I thought of would be adding a hook_path_alias_presave() which checks if the schema verson is > 8804 and throws an exception if it's not, this would cause anything trying to run before this update to fail, but with a clearer error message, although it'd be runtime code we'd need to leave in throughout Drupal 8.8 and 8.9.

Thanks for this suggestion, I'll put it together and see what folks think of it.

So what this would mean is that system_update_8803(), which actually enables the path alias module, would fail with an exception.

However if you then were able to resolve the bug (say commenting out the problematic hook_modules_installed() implementation), you should be able to get path alias module installed, then go on to run system_update_8804(). This seems like a clearer failure than the fatal.

The only other thing we could look at would be somehow preventing hook_modules_installed() from running at all during system_update_8803(), and then running it when all updates have finished or something. But that's quite a big refactoring of the update/module installation system - maybe for a follow-up to discuss though?

Updated the issue summary. Discussed with @alexpott and we both think the exception is the way to go here, at least until we get more concrete reports of exactly how this is going wrong with contributed or custom code.

I fixed this by changing the limit value from 200 to 20000(more items then i have) there is a problem with the batching loop i think if your ids are not in sequence. sorry I don't have the code to reference.
I can look later if anyone needs

Working on this

This implements the proposed solution + a settings switch to disable the schema check, that is currently relied upon in Kernel tests.

Is there a way to write a test to prove the failure described in the issue summary?
In the meantime, here's a review

1. +++ b/core/modules/system/system.module
   @@ -1489,3 +1492,26 @@ function system_entity_form_display_presave(EntityFormDisplayInterface $display)
   +  if (Settings::get('system.path_alias_schema_check', TRUE) && drupal_get_installed_schema_version('system') < 8804) {
   

   Should this setting be added to core/assets/scaffold/files/default.settings.php? I spot-checked a couple other widely-used settings and they are all in there.

2. +++ b/core/modules/system/system.module
   @@ -1489,3 +1492,26 @@ function system_entity_form_display_presave(EntityFormDisplayInterface $display)
   +    if ($db_update_access->access(\Drupal::currentUser())->isAllowed()) {
   +      $args = [':url' => Url::fromUri('base://update.php')->toString()];
   +      \Drupal::messenger()->addError(t('Path aliases cannot be saved until <a href=":url">database updates</a> are performed.', $args));
   +    }
   +    else {
   +      \Drupal::messenger()->addError(t('Path aliases cannot be saved until <em>database updates</em> are performed.'));
   +    }
   

   This is so kind :)

3. +++ b/core/modules/system/tests/src/Functional/Update/PathAliasToEntityUpdateTest.php
   @@ -56,8 +61,49 @@ public function testConversionToEntities() {
   +    $module_installer = $this->container->get('module_installer');
   +    $module_installer->install(['system_test']);
   +    $this->container = \Drupal::getContainer();
   

   This is the correct way of doing this, instead of calling ::rebuildAll() (which is unsafe to do before updates are run), but perhaps it's worth a comment here to prevent someone from trying to refactor this later?

4. +++ b/core/modules/system/tests/src/Functional/Update/PathAliasToEntityUpdateTest.php
   @@ -56,8 +61,49 @@ public function testConversionToEntities() {
   +    $integrity_exception_expected = !$perform_check;
   +    $this->checkFailedUpdates = !$integrity_exception_expected;
   ...
   +    if ($integrity_exception_expected) {
   

   I think this double assignment is trying to be helpful, but it seems confusing to me. Why not
   $this->checkFailedUpdates = $perform_check; and then use !$perform_check below?

This should address #30, except for bullet 1. I briefly discussed it with Tim and we agreed that, since in most cases there will be no need to tweak the new setting, as it is mainly a way to restore BC in case a site is relying on the broken behavior, a CR would be enough. Working on it right now.

Ouch, posted the patch twice instead of the interdiff...

Updated IS and added CR: https://www.drupal.org/node/3114366.

The failing patch had *exactly* the same exception, wonderful!

Per #31, I think it's fine to leave this out of default.settings.php
The CR (which I just reviewed) is sufficient.

Thanks!

1. +++ b/core/modules/system/tests/src/Functional/Update/PathAliasToEntityUpdateTest.php
   @@ -56,8 +61,50 @@ public function testConversionToEntities() {
   +      $this->assertContains("Failed: Drupal\Core\Database\IntegrityConstraintViolationException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '1' for key 'PRIMARY': INSERT INTO {path_alias}",  reset($failure)->getText());
   

   How about asserting on our more useful message we're adding in system_path_alias_presave()?

2. I ponder if we should try and handle the case of modules creating path aliases on hook_alias here a bit more elegantly? Since the structure of a path alias is well known we could detect that we're in-between 8803 and 8804 and in this case do a deferred creation - perhaps worth discussing in a follow-up - depends on the prevalence I guess. We should get the better messages in once we're asserting on the better messages.

@plach pointed out that the improved message is tested so #37.1 is wrong.

Also #37.2 relies on being able to actually defer or stop an entity save without erroring - that's not possible so not worth pursuing.

Committed and pushed 7181d51a36 to 8.9.x and 908abcf4c7 to 8.8.x. Thanks!

Backporting to 8.8.x as the improved messages here help people upgrade to 8.8.x.

diff --git a/core/modules/system/tests/src/Functional/Update/PathAliasToEntityUpdateTest.php b/core/modules/system/tests/src/Functional/Update/PathAliasToEntityUpdateTest.php
index e9ab973545..741ca2478a 100644
--- a/core/modules/system/tests/src/Functional/Update/PathAliasToEntityUpdateTest.php
+++ b/core/modules/system/tests/src/Functional/Update/PathAliasToEntityUpdateTest.php
@@ -95,7 +95,7 @@ public function testConversionToEntities($perform_check) {
 
     if (!$perform_check) {
       $failure = $this->cssSelect('.failure');
-      $this->assertContains("Failed: Drupal\Core\Database\IntegrityConstraintViolationException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '1' for key 'PRIMARY': INSERT INTO {path_alias}",  reset($failure)->getText());
+      $this->assertContains("Failed: Drupal\Core\Database\IntegrityConstraintViolationException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '1' for key 'PRIMARY': INSERT INTO {path_alias}", reset($failure)->getText());
       // Nothing else to assert.
       return;
     }
@@ -103,7 +103,7 @@ public function testConversionToEntities($perform_check) {
     // Check that an exception was thrown on "path_alias" save.
     $exception_info = $state->get('system_test.path_alias_save_exception_thrown');
     $this->assertIdentical($exception_info['class'], \LogicException::class);
-    $this->assertIdentical($exception_info['message'],'Path alias "/test" ("/user") could not be saved because the "system_update_8804" database update was not applied yet.');
+    $this->assertIdentical($exception_info['message'], 'Path alias "/test" ("/user") could not be saved because the "system_update_8804" database update was not applied yet.');
 
     /** @var \Drupal\Core\Extension\ModuleHandlerInterface $module_handler */
     $module_handler = $this->container->get('module_handler');

Fixed coding standards on commit.

Thanks! Published the CR.

+++ b/core/modules/system/tests/src/Functional/Update/PathAliasToEntityUpdateTest.php
@@ -56,8 +61,50 @@ public function testConversionToEntities() {
+    if (!$perform_check) {
+      $failure = $this->cssSelect('.failure');
+      $this->assertContains("Failed: Drupal\Core\Database\IntegrityConstraintViolationException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '1' for key 'PRIMARY': INSERT INTO {path_alias}",  reset($failure)->getText());
+      // Nothing else to assert.
+      return;
+    }

This broke SQLite and Postgres tests. I hotfixed 8.9.x and 8.8.x to resolve the issue. The fix is to change the above code to be:

    if (!$perform_check) {
      $error_message = $this->cssSelect('.failure')[0]->getText();
      // In order to test against multiple database drivers assert that the
      // expected exception occurs and the error contains the expected table
      // name. The specifics of the error depend on the database driver.
      $this->assertContains("Failed: Drupal\Core\Database\IntegrityConstraintViolationException", $error_message);
      $this->assertContains("path_alias", $error_message);
      // Nothing else to assert.
      return;
    }

Committed and pushed 04b7e421c6 to 8.9.x and 6b1edba77f to 8.8.x.

Ran tests against mysql, postgres and sqlite prior to hotfixing.

Of course, thanks @alexpott!

I opened #3115543: Defer running of hook_modules_installed() during updates based on my comment in #3098718-25: system_update_8804() fails if any path alias is created before it runs. I think that's the 'nice' way to handle hook_modules_installed() implementations that might trigger this exception, but it could introduce its own risks too.

Has this issue been ported in 8.8.5?

Trying to upgrade a site from 8.7 to 8.8 still give this same issue with path alias.

Having the same issue upgrading from `8.6.2` to 8.8.5` as well.

What is "same issue"? The message that was added by this issue was added on purpose, it's meant to fail, to give a clear error and steps to avoid it. See the linked change record: https://www.drupal.org/node/3114366.

You'll need to figure out what causes that alias save and prevent it from happening. Ask in a support channel if you can't figure it out. Maybe avoid making too big of a chump at once, update to latest 8.7 first and then to 8.8, that might not help. And don't mix it with contrib module updates.

In my case "the same issue" was about "Duplicate entry" fails.

The only way to upgrade from 8.7 to 8.8 was to truncate both path_alias and path_alias_revision database tables. Otherwise, drush updb is stuck at failing system_update_8804() because of duplicates.

> The only way to upgrade from 8.7 to 8.8 was to truncate both path_alias and path_alias_revision database tables.

That's the thing. On 8.7, those tables don't exist, and they must not exist to update successfully. If already have those tables then there's only one possible explanation for that. You previously attempted a 8.8 update before on the database. When using drush sql-sync or another way import a database then it only overwrites existing tables. Always restart an update with a completely empty database that you import your dump into.

This is a common issue with testing updates on a local/test environment, and I think what we should do in the future is add a requirements check and if you update from earlier than 8804 and have that table already, we fail with an error, telling you to start on a clean database.

Note that this specific issue was a about a slightly different use case, it's when code attempts to create an alias *while* updating.

I am having this issue right now when trying to update project 8.7.12 -> 8.8.5. I tried patch for 8.8.x and it was applied only partially - only first junk. So, I cannot use it for some reason.

But I've got a question: why we are trying to patch module file instead of patching certain update - system_update_8804?

Isn't it it better to fix the certain problem and not to try to fix any other problems. If we patch system.install system_update_8804() so that if will insert only those values that do not exist yet, then the issue is fixed.

Or I am missing something?

I try to update site from 8.7.13 to 8.8.5

- all modules are updated, including pathauto
- I try to do drush updb and it fails as described here (those placeholder errors)
- If I try to remove ALL URL aliases with drush pad, I get "The "path_alias" entity type does not exist."

Update (related to other suggestions above):
- I used clean 8.7.13 database without path_alias and path_alias_revision tables

@Berdir is right: truncating path_alias and path_alias_revision fixed the issue - updb worked without errors.

This is patch I used locally. So, I just truncate tables and print exception if truncate transaction fails with a throable error.

diff --git a/core/modules/system/system.install b/core/modules/system/system.install
index 38a95e1..eb4b0e3 100644
--- a/core/modules/system/system.install
+++ b/core/modules/system/system.install
@@ -2601,6 +2601,15 @@ function system_update_8804(&$sandbox = NULL) {
   }

   $database = \Drupal::database();
+
+  try {
+    $database->truncate('path_alias')->execute();
+    $database->truncate('path_alias_revision')->execute();
+  }
+  catch (\Throwable $e) {
+    printf("\nError on truncating path_alias and path_alias_revision tables: %s\n", $e->__toString());
+  }
+
   $step_size = 200;
   $url_aliases = $database->select('url_alias', 't')
     ->condition('t.pid', $sandbox['current_id'], '>')

What Berdir actually said is that the fact that you already have those tables in your database is wrong, the only reason for that is that you tried to update before and it failed. If this is a test environment, then do drush sql-drop and then drush sql-sync again to have a clean database that should then not contain those tables. The table is called url_alias in Drupal 8.7.

> AND "Path alias" module is tried to be uninstalled the cim command

Never run drush cim after drush updb. After a drush updb, you need to export all configuration changes that were done by the update functions.

It seems I got my update working with following process:

# Update Drupal core codebase (we use drupal/core-recommended package)

composer req --with-dependencies drupal/core-recommended:^8.8

# Add patch:

"drupal/core": {
"system_update_8804": "https://www.drupal.org/files/issues/2020-04-23/core.system_update_8804.t..."
}

# Re-run composer install to apply the patch

composer install

# Database updates

drush updb

# Configuration export

drush cex

# Generate path aliases

drush pathauto:aliases-generate all all
drush pathauto:aliases-generate create canonical_entities:node

Now I seem to have site working and having aliases (at least what I can quickly see).

Thanks Berdir, running drush sql-drop before drush sql-sync worked for me.

I have tried upgrading from 8.7.13 to 8.8.5 but getting this error -
> [notice] Update started: system_update_8801
> [notice] Update completed: system_update_8801
> [notice] Update started: system_update_8802
> [notice] Update completed: system_update_8802
> [notice] Update started: system_update_8803
> [error] Argument 5 passed to Drupal\pathauto\AliasUniquifier::__construct() must implement interface Drupal\path_alias\AliasManagerInterface, instance of Drupal\Core\Path\AliasManager given

Please help me. I have tried the patch mentioned at #56 but it does not work for me.

Update to pathauto 8.x-1.7 only, then after updating to 8.8 update to pathauto 1.8.

Tried the above solution, but still getting same issue. Please let me know if I am missing something here.

> [error] Argument 5 passed to Drupal\pathauto\AliasUniquifier::__construct() must implement interface Drupal\path_alias\AliasManagerInterface, instance of Drupal\Core\Path\AliasManager given

The problem with #56/#58 is that it wipes out all manually entered aliases too, which you obviously cant regenerate.

Yes, that is the downside of that.

No, it does not. The aliases of a Drupal 8.7 site are stored in "url_alias". path_alias is new in D8.8 and must not exist before starting the update. So far, the only reason I've seen why it already exists before starting the update is that people have either been restarting failed updates or sync a production database over a staging/test environment without deleting all tables first. A drush sql-drop before sql-sync or a manual DB import has almost always fixed this.

I did some more research. In a site where path_alias already exists from a failed update, it was populated with about 1000 aliases. Upon deleting the path_alias table and retrying the update, those same aliases don't get repopulated again. It only populated any newer aliases that were created after the failed update. Is it keeping track of the aliases it already converted somewhere?

@kruser from my experience, we've had to use a script to manually port the old aliases over where it didn't work.

In terms of process, with certain version of core updates, we were able successfully run the database updates using drush (i.e., 'drush updb') once we'd manually migrated in the missing path_alias and path_alias_revions empty db table from a clean install. However, that didn't work with all instances, and I'll update with another comment once we've successfully worked through some other sites where we haven't succeeded yet.

Further to Berdir's comments in #61 and #65, this is a good example where it's appears that it's helpful/best to incrementally update core and the related contrib module in lockstep.

@alexpott @plach , since the problem is still affecting a lot of people and #56 and #58 seems to be the current solution, we may want to consider to change the status from fixed, right?

The patch in from comment #56 wasn't working reliably because it was truncating path_alias tables on every batch run, instead of just the first batch. Attaching an updated patch that moves that truncation into the first-batch-only code block.

This patch should normally not be needed, but in case you find yourself in a situation where this particular update hook is firing on a site instance that already has a url_aliases table with data, here's a patch that will hopefully help.

Noob question... How would I apply this patch to a non-dev version of core?

I am still facing this issue on 8.9.15, and the last patch worked for me. Can we change the status to