Problem/Motivation
All entity types having the content_translation_uid
field end up having an index name that is too long to be kept intact so it's hashed. The code computing entity schema data relies on the index name to detect field-level indexes/keys (which is very fragile), so it does not recognise hashed-named-indexes as field-level ones and thus a mismatch is reported, as the field is interpreted as a new entity-level index.
Proposed resolution
Fix comparison.
Remaining tasks
- Commit
User interface changes
The "Entity/field definition" status report item is always displayed now.
API changes
none
Data model changes
none
Original summary
I was able to enable translation for "custom menu link" under Admin => Configuration => Region and Language => Content language and translation.
However, after that, page "admin/reports/status" give an error:
Entity/field definitions: Mismatch detected. Mismatched entity and/or field definitions.
I think this is related to translation, so put it under translation_entity.module component.
Please help.
Thanks.
Comment | File | Size | Author |
---|---|---|---|
#87 | entity-schema_data_fix-2597628-87.patch | 11.73 KB | plach |
Comments
Comment #2
dpacassiI have a similar bug when working with translatable taxonomy vocabularies.
I did the following:
1) Install a fresh copy of Drupal 8.0.0-rc1
2) Install all core modules under "Multilingual" and it's required modules
3) Install the "Taxonomy" module
4) Add a new taxonomy vocabulary with the "Show language selector on create and edit pages"-option checked
-> No problems so far!
5) Add another taxonomy vocabulary with the options "Show language selector on create and edit pages" AND "Enable translation" checked
-> Now I get following error message in the status report:
Entity/field definitions, Mismatch detected
Mismatched entity and/or field definitions.
If I remove all taxonomy vocabularies, the error message stays in the status report.
The only way to get rid of it, is by uninstalling the taxonomy module.
Currently running Drupal 8 on PHP 5.5.26 and MySQL 5.5.42.
Let me know if you need any further information!
I would be happy to help you out!
Comment #3
hguo CreditAttribution: hguo commented@dpacassi
Thanks for providing the details of your case. What you described matches exactly what I have been experiencing.
I'm most concerned whether this would any performance implication. Can you shed some light on that?
Thanks!
Comment #4
gdev2018 CreditAttribution: gdev2018 commentedComment #5
gdev2018 CreditAttribution: gdev2018 commentedI have the same error occurred when updating the version beta10 to RC2. Thus I have a pure core without some additional modules and content.
Sorry to change the priority to critical, I also every day to track their number and really looking forward to when they will not be at all.
Comment #6
catchMoving to entity system.
Comment #7
jhedstromRunning entity schema updates resolves the issue, but there is no longer any way to do that through the UI IIRC.
Drush provides
drush entity-updates
ordrush updb --entity-updates
.Comment #8
jhedstromI started looking at writing a test, but found that some of our tests already manually apply entity schema updates after enabling translations. From
ContentTranslationOperationsTest::setUp
:So perhaps the fix is to run entity schema updates after enabling translations (then remove those manual runnings from the tests)?
Comment #9
jhedstromThis fixes the issue in manual testing, and will most likely remove the need for content translation tests to manually run entity schema updates.
Comment #10
jhedstromLet's see if this fails/passes as expected.
Comment #13
jhedstromWell that was the opposite of expected. So we'll probably need an additional test here.
Comment #14
effulgentsia CreditAttribution: effulgentsia at Acquia commentedLet's create a separate issue to get #10's test-only patch committed, unless there's some other need for those lines. But I suspect those lines are from before we had ContentTranslationManager::setEnabled() calling ContentTranslationUpdatesManager::updateDefinitions() calling EntityDefinitionUpdateManager::installFieldStorageDefinition().
Come to think of it, perhaps that issue could explore whether similar lines can be removed from other tests.
Comment #15
jhedstromOpened #2600138: Remove manual calls to EntityDefinitionUpdateManager::applyUpdates() from test setup.
Comment #16
jhedstromThe failures here are because we can't simply call
updateEntityType
, as this may be an install or delete operation too. I don't immediately see any public methods that would provide insight into what operation is going on (the protectedgetChangeList()
has the needed information).Comment #17
Gábor HojtsyHow fast are those updates? We used to have schema updates ran after translation config changes but that required a batch to execute. We eliminated that due to underlying changes making that possible (not sure of the technicalities though :/).
Comment #18
hguo CreditAttribution: hguo commentedJust tried rc2, this issue still exists as expected.
Comment #19
jhedstromBefore we removed the automatic running of these during db updates, they weren't in a batch operation. I think since they explicitly avoid *modifying* existing column data, they are either just adding new fields or removing old ones.
One thought is to just have the update manager call
applyUpdates()
since that contains the necessary logic for determining if an entity type is being installed, updated, or removed. Attached is a patch with that approach (which also removes the test only patch from #10 which is in a separate issue now).Comment #21
dawehnerIn order to make proper progress on this issue I would suggest to put
https://www.drupal.org/node/2597628#comment-10474214
into a test.Comment #22
catchI
Applying all updates at once means it'll be easy to run into a race condition with definition/schema updates elsewhere. Although thinking about it, that's also possible just with getting the previous state too - we don't know what some other process is doing / is about to do with that when we get it.
Feels like we can rely on this to handle state A to state B, but only if we're able to add locking around the operation.
Something like:
- acquire lock
- update definitions
- apply updates
- release lock
Also adding some related issues.
Comment #23
jhedstromThis adds locking. The 2 failing tests were due to the 'text' module not being enabled, even though it is a declared dependency of entity_test.
Comment #24
jhedstromThis adds a test that mimics the underlying steps in #2. Note, the first call to applyUpdates is simpler than manually installing all entity types created by a basic kernel test.
Comment #25
webchickThe core committers met and discussed this and other critical issues today in relation to picking a D8 release date.
This definitely feels like the "nuclear critical" one. It's both actively introducing data integrity issues to new D8 sites, and extremely tricky to write an upgrade path for. Unlike the rest of the critical issues in the queue, we also don't really have a viable "plan B" if this isn't resolved by D8.0.0's release date.
Some "plan B" suggestions we discussed but weren't thrilled with were:
- Move Content Translation to a hidden module so it can't be enabled at all from the UI. Obviously, totally un-ideal from the standpoint of selling D8's multilingual capabilities.
- Disable the checkboxes in the UI for any entity type that has content created (e.g. menu links). This would mirror what Field UI does, but catch wasn't 100% convinced that this would solve the problem.
So for this issue, what we should prioritize is: 1) test coverage to show the problem (are we not testing the UI at all or something?!) 2) a "plan b" for what we do if this issue isn't resolved in the next couple of weeks. We also discussed splitting off the upgrade path to a separate issue so we can at least stop new sites from getting this problem before release, even if we can't fix existing sites until 8.0.2 or whatever.
Added a heading for "plan B" and put @todo for now.
Comment #27
catchI've opened #2602996: Upgrade path for #2597628 for figuring out the upgrade path for affected sites, we should try to keep any discussion of that over there so this issue can focus on preventing sites getting into that state in the first place.
Comment #28
jhedstromI found another use of
::applyUpdates()
in the content translation tests. I think this is why the test-only patch in #10 passed unexpectedly. It's inContentTranslationTestBase::enableTranslation()
, called from::setUp()
. I'm testing now to see if it can be added to #2600138: Remove manual calls to EntityDefinitionUpdateManager::applyUpdates() from test setup, or if it requires the fix in this issue in order to be removed.Comment #29
plachWorking on a test implementing #2.
Comment #30
plachThis should fix the issue without requiring any update. The patch is quite different from the earlier ones so no interdiff (the test patch is only slightly changed, though). Even if this were red/green as expected, some additional test coverage at API level would be good to have before closing this. I'll work on that tomorrow.
Comment #31
plachComment #32
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI took a closer look at this, and the problem only seems to occur with menu links and taxonomy terms, per the issue title. The problem does not occur with nodes. I didn't check the other entity types.
The reason it doesn't occur with nodes is that nodes have their own author field, so ContentTranslationHandler::getFieldDefinitions() doesn't add the
'content_translation_uid'
field. This is (one of? the only?) field that creates the problem, because it comes with an index.So what happens is that ContentTranslationUpdatesManager::updateDefinitions() successfully installs the various new fields provided by ContentTranslationHandler::getFieldDefinitions(), including adding their indexes to the db table, but because it doesn't invoke updateEntityType() per #9, that new index never gets registered with the
$entity_type.entity_schema_data
KV entry in theentity.storage_schema.sql
collection. That is what creates the mismatch.So if we need to resort to a "plan B", then given the above, it could potentially be to only disable translations for the vulnerable entity types, until a future patch can provide the complete fix.
However, note that the entity database tables are actually in the correct state (whether the translation is enabled with or without existing entities already in the db), just that KV entry is out of date. So a complete fix might not require very much more than #9. Related to that:
I think those failures in #10 can be fixed. We should take a closer look at what those install/delete conditions are. Perhaps $entity_manager->getLastInstalledDefinition() in addition to the $installed_storage_definitions we're already working with there is sufficient for the code to figure out what to do?
I agree that applyUpdates() is too broad and that we should go back to something more like #9, assuming we can fix the above referenced failures. We could even be more conservative by doing
updateEntityType($entity_manager->getLastInstalledDefinition($entity_type->id()))
, since we're not reacting to an entity type definition change: the only "update" we need is to get the schema manager to sync up its schema information with the schema that's already installed for the currently installed entity type definition.A lock might still be a good idea. Though if it is, we should lock the entire ContentTranslationUpdatesManager::updateDefinitions() function, not only whatever new code needs to be added for this issue.
Comment #33
effulgentsia CreditAttribution: effulgentsia at Acquia commented#32 was an xpost with #30. If #30 fixes the problem of the KV mismatch without requiring a separate updateEntityType() call, that's even better.
Comment #34
plachActually what I found is that all entity types having the
content_translation_uid
end up having an index name that is too long to be kept intact so it's hashed. The code computing entity schema data relies on the index name to detect field-level indexes/keys (which is very fragile), so it does not recognize hashed-named-indexes as field-level ones and thus a mismatch is reported, as the field is interpreted as a new entity-level index.Field schema data is correctly registered in the KV store,
it's just not used correctlyit's just unrelated to the issue.Comment #35
plachThis can be simplified
Comment #37
alexpottAm I right in thinking this is only needed because we're doing this in a KernelTestBase? If so I think this is a bad idea. We're fixing the test as we go.
Comment #38
alexpottI'm converting the test to use the UI.
Comment #39
alexpottThis seems to work nicely. I had to change the requirements to always report on the the entity/field update status - which makes sense now they are no longer coupled to database updates.
The test-only patch is basically the interdiff.
Comment #42
alexpottFixing the test fail caused by the changes to the system status report.
Comment #43
Gábor HojtsyUpdates make sense to me. It is indeed better to webtest this. It is also fine to display a confirmation about entity status in the reports, not just include it when its broken. Just some small remarks:
While we do not run tests with translations, we have a convention of testing for output text that is t()-ed by t()-ing what we assert as well.
Same for button labels.
Also I think we are really close and this therefore does not need a plan B. An issue summary update is required anyway, the underlying problem / proposed solution is not explained.
Finally is this definition cached and do we need to clear that for this fix on existing sites? That historically required a no-up update function to sure that the caches were cleared (via update.php).
Comment #44
alexpottFixed up #43
This is not cached so there is no need even for an empty update function afaics.
Comment #45
jhedstromDoes this add any advantage over directly checking if updates are needed via the
entity.definition_update_manager
service? It ties this test fairly closely to the UI/requirement hook.Comment #46
plachThe root issue has nothing to do with the CT module, so we still need some API-level integration tests. Working on those.
One of the reasons to not display the entity definition status except that in problematic conditions is that the entity/field definition terminology was deemed not user-friendly and thus confusing. I don't think we should be making UI changes here, they are completely unnecessary to fix this issue and we already have #2554911: Inform users/developers about entity/field mismatch details if it happens to discuss/fix the UI properly.
Comment #47
plachAnd here is the additional test coverage. Test-only patch is the interdiff.
Comment #48
plachI'd still like to see the UI changes reverted but this is not my decision :)
If we don't do that we need to update the UI changes IS section.
Comment #50
plachComment #52
effulgentsia CreditAttribution: effulgentsia at Acquia commented#2602996: Upgrade path for #2597628 was closed as a duplicate, so I think yes, we either need to reopen that one, or else add a hook_update_N() to this patch to clean up all the affected
*.entity_schema_data
KV entries. Those are not caches, but it should be possible to detect ones that are recording an index that they should no longer be according to this improved code's logic, and correspondingly remove it from that entry.Comment #53
plachI think there's no cache to be cleared: while coding/manually testing this I just kept refreshing the status report page and I saw the error appearing/disappearing depending on the code correctness.
It would be great if @hguo and @dpacassi could confirm that applying the patch is enough to get rid of the status report error.
Comment #55
plachComment #56
plachRe-uploading the last patch, not sure what's happening.
Comment #58
alexpott@plach I'm not sure that we should call the change to a line on the status report a UI change - because the user interface has not changed.
Comment #59
plachI don't have a better definition for that change, sorry, but it looks unnecessary to me. I was suggesting to revert it because it has UX implications (albeit minimal) that would be better discussed elsewhere, to avoid possibly delaying this issue.
Comment #60
alexpottSo the problem with removing the changes to the UI means we have no reliable way of testing this through the UI which makes the ContentTranslationEnableTest extremely fragile. Trying to improve that shows we have a significant issue in keeping the parent and site under test in sync since I can't get the test to pass even though I'm whacking it with a hammer..
This raises massive questions about much of the testing we do for multilingual where we do stuff in the test-runner and through the UI - i've said this before and I'll say it again - this is an extremely bad practice only done for quicker running tests but in reality no one is testing what they think they are.
Comment #61
Gábor HojtsyI think the UI change is very minimal and the status report is full of developery things anyway. Also the UI can be further improved later. Let's keep the UI change and move forward. The bugfix is a lot more important, it is not worth delaying the D8 release on one line in the status report ;) Reuploading that prior patch from #56, basically undoing #60.
Not RTBC due to the unclear resolution of #52. That is the kind of question that are actually serious IMHO :)
Comment #63
alexpottRe #52/#61 I've discussed this with @plach in IRC and we are both sure that the values in the KV store are correct and do not need fixing and there is no cache to clear.
Comment #64
alexpottAlso I'm more than fine with leaving the status report changes in :)
Comment #65
plachSpoke with @alexpott, we reached similar conclusions to #61. Funny that I started delaying this issue to avoid delaying it ;)
About #52 I can only re-state that I think KV data is not related to this issue and no cache-clear is needed but maybe I'm missing @eff's point.
Comment #66
plachAdded screenshot
Comment #67
Gábor HojtsyAll righto, looks good to me now :) Thanks for the updated issue summary.
Comment #69
effulgentsia CreditAttribution: effulgentsia at Acquia commentedAh, I figured out the discrepancy in what I was seeing vs. what you all were. So, if you do the following:
- Install HEAD, Standard profile.
- Enable Content Translation module.
- "Enable translation" for Tags vocabulary.
- Go to /admin/reports/status and notice the mismatch warning.
- Apply #61.
- Go to /admin/reports/status and notice that entity and field definitions are "up to date". Yay!
But, if you do the following:
The reason is that #5 above results in the
entity.storage_schema.sql
:taxonomy_term.entity_schema_data
KV value getting saved with thetaxonomy_term__1a0c187e9a
index prior to applying the patch. Then the patch fixes the code to not expect that index there.Note that the situation can be re-fixed by redoing step #5 after applying the patch, so potentially, we can tell people, hey, if you manually messed with things like that during an earlier RC, then you need to redo that same manual step on the next RC.
But, I'm not sure if there's any code path by which that onEntityTypeUpdate() might have been called without the site owner's manual intervention (e.g., some other update function, whether in core or contrib), in which case, releasing this patch and it resulting in #8 above would be confusing.
If we want to fix this, some options might be:
$edum->updateEntityType($edum->getEntityType())
for every entity type. However, we'd need to check if that works correctly for all database drivers, in case it tries to delete and recreate indexes that it shouldn't.*.entity_schema_data
KV entries that should no longer be there after this patch.Setting to "Needs review" for more input on whether to add such an update function.
Comment #70
effulgentsia CreditAttribution: effulgentsia at Acquia commentedHere's that attachment.
Comment #71
effulgentsia CreditAttribution: effulgentsia at Acquia commentedOr, instead of #70, you can use Drush per #7.
Additionally:
Yeah, here's one path:
The reason is that prior to beta15 (#2542748: Automatic entity updates can fail when there is existing content, leaving the site's schema in an unpredictable state), every run of update.php would do a full EDUM::applyUpdates(), which would get the
taxonomy_term__1a0c187e9a
index into theentity.storage_schema.sql
:taxonomy_term.entity_schema_data
KV entry.Comment #76
plachAh, excellent point! Let's see whether I can come up with a sensible update function...
Comment #79
alexpottAre we sure that we should do this? There is no functionality in core or Drush (apart from tools that allow you to run custom PHP) to get HEAD in this state. For me this opens up a can of worms - where does this responsibility start and end?
Comment #80
plach@alexpott:
The steps in #71 can be performed by any site builder, I reproduced them manually with no difficulty. I'm almost done with the update function, I'll post it and then we can decide whether it's worth including it.
Comment #81
plachAnd here is the update function, if we end-up including it we'll need additional test coverage.
Comment #82
alexpottWell we only supported the upgrade path from beta 15. Harsh but true. I think I'd rather post a script for people to run rather than support an upgrade path we don't have to - especially one that mucks about with entity defintiions
Comment #83
alexpottAlso I tried adding the following assertions to the UpdatePathTestBaseFilledTest because it was added as part of beta 12 and has content_translation installed...
But it failed - so I think testing the upgrade path is going to be quite interesting.
Comment #84
catchLet's tackle the upgrade path in the separate issue open for it (will need to be unduplicated) as a major. I think it's very important to remove this error state for new sites to avoid confusion before the next rc.
Comment #85
Gábor HojtsyI agree we should not need to support an upgrade path for versions we said we are not going to support an upgrade path for ;)
Comment #86
plachOk, then patch to commit is #61 :)
Comment #87
plachOk, removed the update function per #82 and following. The interdiff contains a minor clean-up I did not post yet. I'm posting the removed update in function in #2602996: Upgrade path for #2597628, in case anyone needs it.
Comment #88
plachComment #89
Gábor HojtsyThe changeis minor, still looks RTBC to me :)
Comment #90
effulgentsia CreditAttribution: effulgentsia at Acquia commentedPushed to 8.0.x. Thanks all!
Comment #92
Gábor HojtsyThanks all, this was tough! :)
Comment #95
bryanmanalo CreditAttribution: bryanmanalo commentedcomment #7 works, thanks!
Comment #96
GiorgosK#7 works
drush updb --entity-updates