Remove suboptimal path_alias__status index from path_alias table

Test only patch

The patch looks good, I've kicked off test runs against pgsql and sqlite.

This would be the first hook_update_N() we've added to a patch release for a while, given it's a straight bugfix and the end result is just dropping the index, that's probably fine though.

One minor comment that could be fixed on commit

The link in the comment is to the wrong issue anyway, so +1 for remove on commit.

larowlan → credited Berdir → .

Looks fine to me too. FWIW, the update could also be a post update since it doesn't alter the schema in any way that would conflict with loading or saving en entity.

I'm wondering about hook_update_N() vs. post_update a bit.

The update just does this:

  $update_manager = \Drupal::service('entity.definition_update_manager');
  $entity_type = $update_manager->getEntityType('path_alias');
  $update_manager->updateEntityType($entity_type);

If we later introduced a change to path alias (extra column or whatever), the code here would add that column before any later update gets to run.

I think we need to do this update the long way, i.e. the "Updating an existing entity type when the update requires schema changes" method from https://www.drupal.org/node/3034742 →

Not because this update in itself will break anything, but because it will prevent another update from doing things the 'safe' way.

Or... maybe we can put the existing logic in a post update (because as @berdir notes it's safe to do it wherever), and then if there was a later schema change, that update would run first before we get to this one (and either drop the index as a side effect, or this one would still work).

In either case needs work for now.

I think we need to do this update the long way, i.e. the "Updating an existing entity type when the update requires schema changes" method from https://www.drupal.org/node/3034742 →

I'm not seeing anything in there that would allow us to remove the index. I can alter the entity type definition but that doesn't help as the index is provided by the entity storage schema and I don't see any public methods to accessing the schema to drop indexes. Looks like the post update hook would be the only way.

re #14 that was in the back of my mind but not fully formed, in that case post update looks good and probably the only option. Moving this back to RTBC.

Committed 1f9798d and pushed to 11.x. Thanks!

I've asked if we should consider backporting this to 10.1 as its a major bug, we ordinarily wouldn't because of the update hook.

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

mstrelan → closed merge request !4457