fix: install lookup_hash column for existing sites via hook_update_10001#40
Merged
Conversation
…okup_hash Existing sites installed before the hash-indexed lookup refactor (commit e215a95) hit a 500 on every page render because VariantSelectorBase::loadExperimentByLookupHashes() now filters an entity query on `lookup_hash`, a column that exists in the new storage schema but was never added to previously-installed tables. The original refactor deliberately skipped a hook_update with the justification that the submodules were unreleased, which is not a valid reason to skip schema updates in a contrib module. Each update_10001(): 1. Adds the lookup_hash varchar(64) NOT NULL column with empty default. 2. Backfills hashes from each row's (path|langcode) or (menu_link_plugin_id|langcode) using the entity class's own computeLookupHash() helper so values match what preSave() writes. 3. Fails with a listed hash set via UpdateException if pre-existing duplicate (target, langcode) rows would violate the UNIQUE key. 4. Adds the rl_*_lookup_hash UNIQUE key and the secondary composite index declared by {PageTitle,MenuLink}ExperimentStorageSchema. 5. Registers the base field via the last-installed schema repository instead of EntityDefinitionUpdateManager::installFieldStorageDefinition(), because the latter would re-apply the full storage schema (including the UNIQUE key just added) and fail on "key already exists". Verified on a preview-checkout install: drush updb ran both updates, backfilled existing rows, applied all four indexes (SHOW INDEX confirms the unique and composite indexes on both tables), /home returns 200, and no new QueryException rows land in watchdog.
jjroelofs
force-pushed
the
fix/rl-lookup-hash-update-hook
branch
from
c4ee4ee to
010cfea
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
rl_page_title_update_10001()andrl_menu_link_update_10001()so existing sites installed before the hash-indexed lookup refactor (commit e215a95) get the newlookup_hashcolumn, backfilled hashes, UNIQUE key, and secondary composite index.VariantSelectorBase::loadExperimentByLookupHashes()throwsQueryException: 'lookup_hash' not foundon every page render, 500ing anything that hitshook_page_attachments.How each update works
$schema->addField()addslookup_hash varchar(64) NOT NULL DEFAULT ''so the NOT NULL add does not fail on tables with rows.{PageTitle,MenuLink}Experiment::computeLookupHash()so values match exactly whatpreSave()writes at runtime.UpdateExceptionwith the offending hash set if any rows collide (prevents the UNIQUE key add from exploding mid-migration).addUniqueKey()+addIndex()apply the schema declared by the storage schema classes.entity.last_installed_schema.repository::setLastInstalledFieldStorageDefinition(), deliberately bypassingEntityDefinitionUpdateManager::installFieldStorageDefinition()because that re-applies the full storage schema and fails on "key already exists".Test plan
drush updb -yon a site with the old schema runs both updates, backfills existing rows, applies indexes.SHOW INDEX FROM rl_page_title_experimentconfirmsrl_page_title_lookup_hash(UNIQUE) +rl_page_title_path_langcode(composite, 191-char prefix).SHOW INDEX FROM rl_menu_link_experimentconfirmsrl_menu_link_lookup_hash(UNIQUE) +rl_menu_link_plugin_langcode(composite, 191-char prefix)./homereturns 200 after the update.lookup_hashQueryException rows in watchdog after the fix.drush updbre-run shows "No pending updates" (idempotent).