Problem/Motivation

Steps to reproduce:

  1. Checkout 8.4.x (17ebfff72673728bc8495d67f35061ad8bdcc299)
  2. Install the standard profile via drush without creating custom blocks
  3. Checkout 8.5.x (bf47414a25e8d8f1fb2b5101a2c18c6829d99627)
  4. 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

CommentFileSizeAuthor
#8 2942533-8.patch696 bytesplach
#2 2942533-good.sql_.gz765.07 KBplach
#2 2942533-bad.sql_.gz442.82 KBplach

Comments

plach created an issue. See original summary.

plach’s picture

Attached 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.

berdir’s picture

The 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.

plach’s picture

Yep, pretty sure that's what happens: you can easily notice that in the two DBs the status column is created in a different order.

berdir’s picture

So 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?

plach’s picture

Should we skip caches completely during updates?

At 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.

alexpott’s picture

Maybe we should add version information to these caches like we do for other version sensitive caches.

plach’s picture

Status: Active » Needs review
StatusFileSize
new696 bytes

This 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.

plach’s picture

Priority: Normal » Major

As 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.

nick hope’s picture

The patch in #8 worked for me when upgrading from 8.4.4 to 8.5.0-beta1. Thank you very much.

amateescu’s picture

Another option that I see is to add some functionality to EntityDefinitionUpdateManager for:

- 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.

donaldp’s picture

Moved to where it should have been in the possible related issue, which pointed to this one...
https://www.drupal.org/project/drupal/issues/2951242

armyguyinfl’s picture

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mmbk’s picture

Status: Needs review » Reviewed & tested by the community

This 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.

alexpott’s picture

Status: Reviewed & tested by the community » Closed (duplicate)

@mmbk let's close this a duplicate and if anyone is still experiencing this then they can comment or re-open.