Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
The current schema for locales_translation includes 2 indexes:
- sid: sid
- string_type: [sid, type]
Since the former is entirely covered by the second, it provides no optimization and just add a little cost on writes, and should be removed.
Comment | File | Size | Author |
---|---|---|---|
#28 | interdiff-23-28.txt | 700 bytes | jungle |
#28 | 2925318-28.patch | 2.06 KB | jungle |
#4 | 0001-Issue-2925318-by-fgm-remove-redundant-index-on-local.patch | 1.1 KB | fgm |
Comments
Comment #2
fgmComment #4
fgmStupid table name error fixed.
Comment #6
fgm"SQLSTATE[HY000] [1049] Unknown database 'jenkins_drupal_patches_38435'"
Looks like a testbot issue. The only test with a different error also passes locally. Resubmitting.
Comment #7
fgmConfirmed: patch now passes.
Comment #8
fgmOops, hidden wrong patch.
Comment #13
Kristen PolPatch applies cleanly to 9.1.x.
Comment #14
Kristen PolThanks for the issue and patch.
1) Code looks straightforward.
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.
Comment #15
phenaproximaThis needs an update path test.
Comment #16
Kristen PolComment #17
fgmNo idea how to write update path tests: do you have an example ? It can't be a unit test since it uses the DB, maybe a Kernel test ?
Also, I suspect the update will need to be renumbered to 910x since it is now for core 9.1.x ?
Comment #18
Kristen PolFound 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.
Comment #19
phenaproximaSelf-assigning for an update path test. I have prior experience with this :)
Comment #20
phenaproximaCreated 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.
Comment #21
BerdirI suppose this is a lot faster, but I'm pretty sure the full dump has translation modules enabled and we could just that dump?
Comment #22
phenaproximaIt 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.
Comment #23
Berdir> 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.
Comment #25
Kristen PolNice! 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?Other examples:
Comment #26
Berdir8.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.
Comment #27
Kristen PolAh! Makes sense. Thanks for the clarification.
Comment #28
jungleCould be
dirname(__DIR__, 4) . '/system/tests/fixtures/update/drupal-8.8.0.filled.standard.php.gz',
. addressing and stay RTBC.Comment #29
alexpottCommitted c758df9 and pushed to 9.1.x. Thanks!
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.