Redundant index sid on locales_translation

Patch applies cleanly to 9.1.x.

Thanks for the issue and patch.

1) Code looks straightforward.

+++ b/core/modules/locale/locale.install
@@ -304,3 +303,14 @@ function locale_update_8300() {
+/**
+ * Delete useless index.

Nitpick: IMO it would be better to reword like "Delete redundant index." or "Delete unnecessary index."

2) Marking for manual testing.

3) Kicking off tests for 9.1.x.

This needs an update path test.

Found out from @acbramley via #bugsmash there should be plenty of examples by searching for UpdatePathTest. I'm not at computer yet, otherwise I'd find some.

Self-assigning for an update path test. I have prior experience with this :)

Created an update path test with a fail patch to prove that it actually works. :) This includes a long-needed reroll of #4, plus a bugfix.

I'm removing the "Needs manual testing" tag because here be an automated test.

I suppose this is a lot faster, but I'm pretty sure the full dump has translation modules enabled and we could just that dump?

It seems you are correct. Boy, I sure wish I'd known about that before I went through the slogfest of creating a new fixture. Phooey.

> before I went through the slogfest of creating a new fixture. Phooey.

Been there, done that: #3034062-25: Remove hal.module BC layers for rest, hal and serialization before figuring out that the filled dump just works. And with the extended dump referenced there, we'd have the same starting point for newer modules like media, layout builder, ... :)

Looks good now I think. Probably not worth the trouble to backport the update function to older versions.

Nice! That's very clear. Unrelated, but I've noticed a number of patches referring to files that still have 8.* in them like below but I didn't find an issue in the issue queue related to this. I assume that we should create one to change these?

+++ b/core/modules/locale/tests/src/Functional/LocaleUpdatePathTest.php
@@ -0,0 +1,38 @@
+      __DIR__ . '/../../../../system/tests/fixtures/update/drupal-8.8.0.filled.standard.php.gz',

Other examples:

./core/tests/fixtures/files/translations/drupal-8.0.0-beta2.hu.po
./core/tests/fixtures/files/translations/drupal-8.0.0.de.po

8.8.0 is the correct name. It's a database dump of Drupal 8.8. We'll bump that to 9.3 or whatever the minimum Drupal 9 minor version will be that you can update to Drupal 10.

The translation files are used in \Drupal\KernelTests\Core\Installer\InstallerLanguageTest::testInstallerTranslationFiles() and I don't think it really cares about what the version is. We could update it, but it would quickly be out of date again anyway.

Ah! Makes sense. Thanks for the clarification.

+++ b/core/modules/locale/tests/src/Functional/LocaleUpdatePathTest.php
@@ -0,0 +1,38 @@
+      __DIR__ . '/../../../../system/tests/fixtures/update/drupal-8.8.0.filled.standard.php.gz',

Could be dirname(__DIR__, 4) . '/system/tests/fixtures/update/drupal-8.8.0.filled.standard.php.gz',. addressing and stay RTBC.

Committed c758df9 and pushed to 9.1.x. Thanks!

diff --git a/core/modules/locale/locale.install b/core/modules/locale/locale.install
index e7821ab928..6f33b78f00 100644
--- a/core/modules/locale/locale.install
+++ b/core/modules/locale/locale.install
@@ -308,7 +308,7 @@ function locale_requirements($phase) {
 }
 
 /**
- * Deletes an unnecessary index in the locales_location table.
+ * Delete an unnecessary index on the locales_location table.
  */
 function locale_update_9101() {
   $schema = \Drupal::database()->schema();

hook_update_N functions are an exception to the rule the verb should be active - i.e. this should be Delete and not Deletes - because this is shown to a user on the review updates screen. Plus indexes are on, not in, a table. Fixed on commit.