Problem/Motivation
Steps to reproduce:
- Checkout 8.4.x (17ebfff72673728bc8495d67f35061ad8bdcc299)
- Install the standard profile via drush without creating custom blocks
- Checkout 8.5.x (bf47414a25e8d8f1fb2b5101a2c18c6829d99627)
- Run updates via drush (
drush updb -y)
Expected result: only success messages are displayed
Actual result: the "The entity type block_content does not have a "published" entity key." notice is displayed twice
Proposed resolution
TBD
Remaining tasks
- Find a solution
- Write a patch
- Reviews
User interface changes
None
API changes
TBD
Data model changes
None
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | 2942533-8.patch | 696 bytes | plach |
| #2 | 2942533-good.sql_.gz | 765.07 KB | plach |
| #2 | 2942533-bad.sql_.gz | 442.82 KB | plach |
Comments
Comment #2
plachAttached you can find a DB dump that produces notices if updgraded. The other attached dump does not produce notices. The only difference is that in the latter I also created some custom blocks (and some nodes), starting from the former.
I found this while testing #2936511: ContentEntityType::getRevisionMetadataKeys() never considers the BC logic because of the new "revision_default" key.
Comment #3
berdirThe difference is likely that not having any data results in it re-creating the schema from scratch and in that case probably triggers the problem. My guess is that we need to make sure that block_content_8400() (which is 8.5 only actually, and not added in 8.4) runs *before* the revision_default update function.
Comment #4
plachYep, pretty sure that's what happens: you can easily notice that in the two DBs the
statuscolumn is created in a different order.Comment #5
berdirSo what's happening for me when updating from 8.3 is that system_update_8400() runs first, calls $base_fields = \Drupal::service('entity_field.manager')->getBaseFieldDefinitions($entity_type_id); with block_content, that goes to \Drupal\Core\Entity\EntityPublishedTrait::publishedBaseFieldDefinitions() which throws that exception because at the moment, the class already does use that trait, obviously, but the entity type key is not there yet. Not on the cached entity type definition anyway.
And I think that's exactly why we're not seeing this error in our tests, because we run the update tests without cache tables, which has resulted in a bunch of problems in update functions before that we haven't seen in our tests.
I'm not sure how to properly address that to be honest. Should we maybe always invalidate plugin caches before running any updates? Should we skip caches completely during updates?
Comment #6
plachAt least for the entity type and field manager I guess it might make sense. IIRC we have tests where we do clear caches to get fresh key values. Alternatively we could do that manually in that case as well.
Comment #7
alexpottMaybe we should add version information to these caches like we do for other version sensitive caches.
Comment #8
plachThis is the easy fix, let's keep discussing a proper fix.
As you can see manually clearing caches is not a great fix, because it's not immediately obvious who should be responsible for doing that. Probably it depends on the update execution order.
Comment #9
plachAs correctly pointed out by @alexpott and @Berdir, this prevents some update functions from running, so it should be at very least major. Not sure about critical, since simply rerunning the updates is enough to work around the issue.
Comment #10
nick hope commentedThe patch in #8 worked for me when upgrading from 8.4.4 to 8.5.0-beta1. Thank you very much.
Comment #11
amateescu commentedAnother option that I see is to add some functionality to
EntityDefinitionUpdateManagerfor:- getting all the entity type definitions with the possibility to filter by fieldable entity types
- getting all the field storage definitions with the possibility to filter by base fields
This would allow update functions to not have to call the "live definitions" at all.
Comment #12
donaldp commentedMoved to where it should have been in the possible related issue, which pointed to this one...
https://www.drupal.org/project/drupal/issues/2951242
Comment #13
armyguyinfl commentedCore update from 8.3.x - 8.5.x
https://www.drupal.org/project/drupal/issues/2951242#comment-12591871
Comment #14
amateescu commentedOpened #2976103: Make it possible to retrieve all the last installed entity type definitions at once from the update manager for my idea in #11.
Comment #16
mmbkThis patch is open for quite a while, and meanwhile the actual version is D8.6.7
We are applying this patch with every new release, but it if understand it correctly, this is not necessary any more. Is this correct?
The related issue https://www.drupal.org/project/drupal/issues/2951242 is closed, shouldn'd this be closed as well?
At least I can confirm that it is working, and probably forgotten - hopefully there are no D8.3.x sites in the wild anymore.
Comment #17
alexpott@mmbk let's close this a duplicate and if anyone is still experiencing this then they can comment or re-open.