Duplicate alias entities created with 'Create a new alias. Leave the existing alias functioning' setting

Hey all, Thank you for all your hard work on this. Has anyone made progress on coming up with tests for this so that we can have a working patch to test? It's out of my skillset, but these duplicates are killing my sites as well.

We have experienced this issue and patch in #2 worked for us.

Providing new patch including tests to push this one.

Thank you!

The last submitted patch, 38: 3107144-38-test-only.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

#38 looks good
Thank you

In the end, will a @hook_update_N()@ be included in the fix to remove existing duplicates?

In extreme cases, such an update hook might be problematic. E.g.:

https://www.drupal.org/project/pathauto/issues/3107144#comment-14278822 🐛 Duplicate alias entities created with 'Create a new alias. Leave the existing alias functioning' setting RTBC

Could people who said the original patch did not work please test out patch #38? Thank you.

In the end, will a @hook_update_N()@ be included in the fix to remove existing duplicates?

We had multiple websites with millions of duplicate alias entries.
In the end we cleaned it up ourselfs and adjusted our sync scripts to delete duplicate aliases manually on each sync run.

Patch #38 works for us. Prevents a big mess! We've been all in on this solution for quite some time.

#38 says it's the patch from #2 plus tests, but comparing:

- https://www.drupal.org/files/issues/2020-01-17/pathauto-3107144-2.patch →
- https://www.drupal.org/files/issues/2023-06-07/3107144-38.patch →

#38 retains some now-redundant code which was removed in the original patch. This looks like a mistake (albeit a functionally-harmless one).

I still see that code in the current 8.x-1.x HEAD commit, so it's seems like a going concern:
https://git.drupalcode.org/project/pathauto/-/blob/ed2d4d30479ddbc051281...

I tried to make an interdiff of those two patches → , but interdiff failed. First time I've seen that.

$ interdiff pathauto-3107144-2.patch 3107144-38.patch > interdiff-3107144-2-to-38.txt
1 out of 1 hunk FAILED -- saving rejects to file /tmp/interdiff-1.Y0ZoFC.rej
interdiff: Error applying patch1 to reconstructed file

Here's a rerolled patch without the redundant code.

This is an important fix, I think it should go in, there's not much code to look at, mostly test.

I feel bad for all the tens of thousands of instances just accumulating aliases and compounding a mistake into a larger and larger mess until eventually they just complain but have no clue what is the root cause of the issue.

The last submitted patch, 48: 3107144-48.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Back to RTBC.

The only suggestion from the testbot in #51 which is in any way connected to this patch is:

+  /**
+   *
+   */
   public function assertAliasIsUnique($conditions) {

As that function is in line with the others in the file, I don't think that should keep this patch from being RTBC. (Adding docs to all of those test functions could always be a separate issue.)

Sorry, not RTBC due to test failure.

@joseph.olstad including an interdiff ( docs → , more info) between the patch you're improving on and the one you're proposing helps fixes to land faster by making review easier.

Ok, but test failures which have nothing to do with the patch should not be shoe-horned into the patch, so I believe only that single function header doc is needed. Anything else is a different issue.

OK, if it's unrelated then it shouldn't block merging - apologies if that's the case. I saw that #38 above _did_ pass. Will leave to maintainer discretion (and I won't set this issue back from RTBC for that reason ... although the d.o testbot may).

This failing test behaves differently when tested with 10.1 vs 9.5, via 📌 PHP 8.2 compatibility Fixed .

The failing test is on /admin/config/search/path/patterns at the time of failure, and I think that failure is unrelated to the code and tests changed in this issue.

That does however explain why we see it pass when tested with 10.1, and fail on MR runs against 9.5 😀

Thanks for digging into that, xurizaemon.

I've re-rolled the patch and I went ahead and fixed all the phpcs issues I was getting. This includes ones which the testbot hadn't complained about, so maybe this project has more relaxed testing settings?

I've split it into three commits (and hence I've used a fork/MR): one for the testbot complaints, one for the actual issue patch, and one for the additional phpcs fixes. If the maintainer doesn't want the last commit, it'll be easy to omit that.

I've queued up the same tests as before, and I presume we'll get the same failure on 9.5.

FYI to those keen to land this - thanks for your persistence especially, and I hope my pushback here didn't feel like I was trying to slow progress.

Last night I opened 🐛 Failing test in module's existing coverage prevents unrelated patch submissions passing tests Fixed to take a look at it, and based on feedback to that issue opened 📌 Switch to Gitlab CI Needs review which it is hoped will clear the PathautoUiTest test failure blocker on this and any other Pathauto issues.

Now that 🐛 Failing test in module's existing coverage prevents unrelated patch submissions passing tests Fixed is fixed, I think this can go to go back to RTBC; I've tested that failing test locally on Drupal 9.5.x (on MySQL), and it passed. I believe that test failure was DrupalCI "being weird".

Excellent, thank you xurizaemon; duly ignoring results for versions < 10.1 and I've queued up another couple of 10.1 test runs.

I see there was a failure for "PHP 8.2 & MySQL 8, D10.1" previously, so I'm also re-testing that in case that was temporary. It's probably not temporary, but I think not connected to this patch either, so I've queued up a test with the same parameters → on your no-op patch in 🐛 Failing test in module's existing coverage prevents unrelated patch submissions passing tests Fixed to verify whether that's the case.

...and in fact that's finished already with the same failure; so I believe we can also ignore that one for the purposes of this issue.

My extra tests here failed as well. The common factor seems to be MySQL 8, so it looks like there's an issue with pathauto and MySQL 8? (There's no test option for PHP 8.2 and MySQL 5.7 though, so I can't completely isolate it to that.)

Just to summarise the last set of tests wrt recent discussion:

Passed for Drupal 10.1 (the only currently-testable version for this project):

PHP 8.1 & pgsql-14.1, D10.1 48 pass
PHP 8.1 & pgsql-13.5, D10.1 48 pass
PHP 8.1 & MySQL 5.7, D10.1 48 pass
PHP 8.2 & pgsql-14.1, D10.1 48 pass
PHP 8.2 & pgsql-13.5, D10.1 48 pass

Failing on account of unrelated issues testing Drupal versions < 10.1:

PHP 7.3 & MySQL 5.7, D9.5 46 pass, 2 fail
PHP 8.1 & pgsql-13.5, D9.5 46 pass, 2 fail
PHP 8.1 & pgsql-13.5, D10.0.7 46 pass, 2 fail

Failing on account of unrelated issues testing with MySQL 8:

PHP 8.2 & MySQL 8, D10.1 43 pass, 1 fail
PHP 8.1 & MySQL 8, D10.1 43 pass, 1 fail
PHP 8.2 & MySQL 8, D10.1 43 pass, 1 fail

I'd appreciate if someone could open a separate issue about the MySQL 8 test fails.

Also, I don't think I've ever gotten any feedback on #34, the fact that there are very, very few use cases for using this setting that I can think of. Why would you want multiple historical aliases (there are some use cases for multiple aliases for the same thing, but less so for old aliases due to content changes).

The strongly recommended setting is to update the alias and use redirect.module to set up redirects for the old path. As asked in #34, a follow-up issue with the goal of better explaining that setting would be appreciated. With this configuration, you won't have duplicate identical aliases anymore but you'll still end up with possibly many *different* aliases for the same source path.

And last, all the coding standard changes make this a *lot* more complicated to review, as the patch/MR is 3-4x as large as it needs to be, so a reviewer needs to go through and find the actually relevant changes. The general rule is to only fix coding standard on lines you need to change.

All that out of the way, merged.

@berdir, thanks for the commit, hope to see a tag also soon.

Thank you Berdir!

And last, all the coding standard changes make this a *lot* more complicated to review, as the patch/MR is 3-4x as large as it needs to be, so a reviewer needs to go through and find the actually relevant changes. The general rule is to only fix coding standard on lines you need to change.

Ah, sadly you didn't notice that there were three separate commits, specifically for that reason :/

https://git.drupalcode.org/project/pathauto/-/merge_requests/55/commits was intended to be extremely easy to review.

I'd noted in #58 that "I've split it into three commits (and hence I've used a fork/MR): one for the testbot complaints, one for the actual issue patch, and one for the additional phpcs fixes."

In general I wouldn't ever try to review a MR as a single diff unless there was only one commit, so I suggest always checking the commits list of any MR.

Sorry for not reporting back to your #34 question.

Yes, you can skip it (not sure why you even would have a pattern then in that case)

In our case we have a product sync with an external database (which should be rather self explenatory so the title of the product results in the path alias)
But there is also the possibility to manually add products to the drupal website (which is of course stupid but still our customer requires it) therefore the path auth pattern was required so that the alias is also generated automatically for those manually added products.

In the end it was our fault to expect the path field from core and/or the path auto module to automatically handle path changes when doing

$entity = $this->node_storage->load($id);
$entity->set('path', array('alias' => '/produkte/somename', 'pathauto' => \Drupal\pathauto\PathautoState::SKIP));
if($entity->save()) {
  // some more logic
} else {
  
}

instead (for everyone else checking this) you have to do this

$path_alias_manager = \Drupal::entityTypeManager()->getStorage('path_alias');
$aliasObjects = $path_alias_manager->loadByProperties([
  'path'     => '/node/' . $entity->id(),
  'langcode' => 'de' // <== set your language here if you have translatable nodes/aliases
]);

if(!empty($aliasObjects)) {
  $lastElement = end($aliasObjects);
  foreach($aliasObjects as $alias_object) {

    if($alias_object === $lastElement) {
      $alias_object->alias = $value['alias'];
      $alias_object->save();
    } else {
      // Delete duplicate alias
      $alias_object->delete();
    }

  }
} else {
  // If no path alias are present create it "normally"
  $entity->set('path', ['alias' => '/produkte/something', 'pathauto' => \Drupal\pathauto\PathautoState::SKIP]);
}

In general I wouldn't ever try to review a MR as a single diff unless there was only one commit, so I suggest always checking the commits list of any MR.

I should add that the same applies to squashing commits at merge time. I'm mostly sure I configured that MR to not have its commits squashed, but I see that squashing has occurred, which means that anyone subsequently reviewing the code history doesn't get the benefit of the individual commits. Whether or not to squash commits for a MR ought to be considered on a case-by-case basis -- sometimes it makes total sense, but in other cases squashing only destroys useful information for no benefit.

Also: Thank you for getting a new release out so promptly.

@jweowu: That's not how drupal.org works. Merge requests are merged through the drupal.org UI and they are always squashed and the commit message defined in the issue is used.

I did in fact not see the split commits, that does help, but in the end, I still need to review all the changes to merge a merge request, and it is preferred to keep things strictly separate. In contrib, it depends on the maintainer and the change, but merge requests against Drupal core will definitely get rejected over unrelated changes.

Merge requests are merged through the drupal.org UI and they are always squashed and the commit message defined in the issue is used.

Oh, wow... that's awful. I know that's how things have always worked for patch files as almost invariably there is just one patch file, but I just assumed that with the advent of gitlab and merge requests there would also have had been option of merge commits!

With the testbot regularly rejecting contributions over unrelated changes, such unrelated changes are inevitable in the process of getting patches a green light; and even without unrelated test failures, large change sets are typically better as a series of atomic commits.

Thank you for clarifying this. I can't fathom why anyone made that decision, but it's good to know.

@jwoewu, @berdir

Are there any other followup tickets needed for this other than the HEAD tests failing for MySQL 8?
🐛 HEAD tests failing against MySQL 8 Active

No, I think everything else that came up already has its own issue at this point.

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