Problem/Motivation

Media entities are identical in Media Entity 1.x and core Media, right?

Well...not quite, no. In core, certain revision-related fields (like the revision log message) were renamed. This is not normally an issue because Media Entity's update path to core specifically renames those fields in the database. But if you have overridden those base fields, things go pear-shaped very quickly.

Proposed resolution

Two things:

  1. The media-entity-check-upgrade command needs to check if the base fields in question have been overridden, and if they have, it needs to warn the user to delete those overrides from their exported config.
  2. The update path needs to revert the overridden base fields (i.e., by deleting the base_field_override entities in question).

Remaining tasks

Do those things in a patch. Ideally write an automated test, but if that's not possible, manually test. Then commit.

User interface changes

None.

API changes

None.

Data model changes

None really.

Steps to reproduce

  1. Install Media Entity 1.x and create at least one bundle.
  2. Override the revision_log field for at least one of those bundles.
  3. Export config.
  4. Run the update path to core Media.
  5. Try to re-import your config.
  6. Enjoy the fireworks.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

suresh kumara created an issue. See original summary.

phenaproxima’s picture

Status: Active » Postponed (maintainer needs more info)

Hi -- we need steps to reproduce this, not just an error message and backtrace (although those are helpful).

justclint’s picture

Hi phenaproxima, we seem to be experiencing this issue as well. We just recently updated our system from lightning 2.2.0 to 2.2.8 and core from 8.4.0 to 8.4.5. In this process we've gone through the media & moderation migration. Not sure if this is meaningful but were using BLT. With blt we have 2 build scenarios, blt setup (also used by travis ci) and blt sync:refresh.

The problem files seem to be:

config/default/core.base_field_override.media.document.revision_log.yml
config/default/core.base_field_override.media.image.revision_log.yml
config/default/core.base_field_override.media.instagram.revision_log.yml
config/default/core.base_field_override.media.tweet.revision_log.yml
config/default/core.base_field_override.media.video.revision_log.yml

I had found this post here which mentions a fix by deleting those files as somehow not needed anymore but as you can see in my comment this may not be accurate.

The following is just after updating lightning 2.2.8 and core 8.4.5 and running completing the lightning migration steps.

So first off the issue at hand is that our build (blt setup and travisci) would fail here:

Synchronized configuration: create language.content_settings.media.document.                                                                                                                                                                                         [ok]
Synchronized configuration: create field.field.media.document.field_media_in_library.                                                                                                                                                                                [ok]
Synchronized configuration: create core.base_field_override.media.document.uid.                                                                                                                                                                                      [ok]
Synchronized configuration: create core.base_field_override.media.document.status.                                                                                                                                                                                   [ok]
Undefined index: revision_log BaseFieldOverride.php:160                                                                                                                                                                                                                 [notice]
Argument 2 passed to Drupal\Core\Entity\ContentEntityStorageBase::onFieldDefinitionUpdate() must implement interface Drupal\Core\Field\FieldDefinitionInterface, null given, called in                                                                               [error]
/var/www/redacted/docroot/core/lib/Drupal/Core/Field/Entity/BaseFieldOverride.php on line 200 and defined ContentEntityStorageBase.php:158
E_RECOVERABLE_ERROR encountered; aborting. To ignore recoverable errors, run again with --no-halt-on-error                                                                                                                                                           [error]
Drush command terminated abnormally due to an unrecoverable error.                                                                                                                                                                                                   [error]
The external command could not be executed due to an application error.                                                                                                                                                                                              [error]
Command dispatch complete                                                                                                                                                                                                                                               [notice]
Error while sending QUERY packet. PID=18507 Statement.php:59                                                                                                                                                                                                         [warning]
[Acquia\Blt\Robo\Tasks\DrushTask]  Exit code 1  Time 09:15
[error]  Failed to install Drupal! 
[error]  Command `internal:drupal:install ` exited with code 1. 
Connection to 127.0.0.1 closed.

Removing those files did initially work and our blt setup and our travis build both pass successfully.

However, when we go to do a blt sync:refresh our build fails here:

Synchronized extensions: install views_ui.                                                                                                                                                                                                                                  [ok]
Synchronized extensions: uninstall shield.                                                                                                                                                                                                                                  [ok]
Synchronized extensions: uninstall acquia_connector.                                                                                                                                                                                                                        [ok]
Synchronized configuration: delete core.entity_view_mode.media.full.                                                                                                                                                                                                        [ok]
Argument 1 passed to Drupal\Core\Entity\ContentEntityStorageBase::onFieldDefinitionUpdate() must implement interface Drupal\Core\Field\FieldDefinitionInterface, null given, called in                                                                                   [error]
/var/www/redacted/docroot/core/lib/Drupal/Core/Field/Entity/BaseFieldOverride.php on line 217 and defined ContentEntityStorageBase.php:158
E_RECOVERABLE_ERROR encountered; aborting. To ignore recoverable errors, run again with --no-halt-on-error                                                                                                                                                               [error]
Drush command terminated abnormally due to an unrecoverable error.                                                                                                                                                                                                       [error]
[Acquia\Blt\Robo\Tasks\DrushTask]  Exit code 255  Time 04:25
Beginning post config import operations...
> custom:post-config

I realize the blt usage here maybe outside of scope but just wanted to provide context as to how we are arriving at this issue.

I dont believe we can/should just delete these revision_log files so Im still doing some testing but if theres any details you think I could provide that may be helpful, please let me know.

Thanks!

phenaproxima’s picture

Status: Postponed (maintainer needs more info) » Active

Thanks for the detailed information, @justclint. I'll see what we can do about reproducing and/or fixing this.

marknatividad’s picture

Subscribing to this thread as I am experiencing the same issues. We have recently upgraded Lightning from 2.2.0 to 2.2.8 and Drupal core from 8.4.0 to 8.4.5.

phenaproxima’s picture

So I've had a chance to look at this, and I have a theory.

Media entities, as defined in the Media Entity module, had a revision log message field called...revision_log. That is a field you overrode, which is why you have a base field override for it.

However, Media entities as defined by core Media (which is used by Lightning 2.2.8) renamed the field to revision_log_message. I believe the problem you are encountering is because the base field overrides have not been updated. This is something, IMHO, that Media Entity should have done as part of its upgrade path.

I would suggest changing the base field overrides like so, before running the update. Given that the base field override looks like this:

uuid: 42c2ec47-fbf1-45c2-87f3-df315a87950d
langcode: en
status: true
dependencies:
  config:
    - media_entity.bundle.image
id: media.image.revision_log
field_name: revision_log
entity_type: media
bundle: image
label: 'Revision Log'
description: 'The log entry explaining the changes in this revision.'
required: false
translatable: true
default_value: {  }
default_value_callback: ''
settings: {  }
field_type: string_long

Change the 'id' key to media.image.revision_log_message, and the 'field_name' key to 'revision_log_message'.

Hopefully, running the update with those changes will fix your problem. Let me know what happens! If this does fix it, we will need to correct it in Media Entity (which shouldn't be a problem, as I'm part of its maintenance team).

justclint’s picture

Thanks for taking a look into this @phenaproxima!

Ill go ahead and give this a shot and then report my results.

Web-Beest’s picture

@phenaproxima: thank you, thank you, THANK YOU!
This was exactly the problem for me. Not related to Lightning though, just a normal 8.5.1 installation

phenaproxima’s picture

Title: media document revision config import error. » Media Entity upgrade path does not revert overridden base fields
Project: Lightning » Media entity
Version: 8.x-2.25 » 8.x-2.x-dev
Component: Media » Code
Priority: Normal » Major
Issue summary: View changes
Issue tags: +Media Initiative

I am moving this to Media Entity's issue queue and updating the issue summary.

justclint’s picture

Sorry for the late response. Finally got back around to this. I just tried your solution @phenaproxima and I can confirm as well that it resolves the issue.

Thanks!

acontia’s picture

I can confirm that #6 fixes the issue for me too.

I'll add a bit more of detail to make this thread more searchable, it took me a while to find this.

I was getting this error after migrating from Media Entity to Media Core (for all media types: Image, Document and Video entity types). Initially everything seemed to work without issues, but then found that I couldn't re-import the configuration from files to the database (using Features) because I was getting this error:

Error: Call to a member function getFieldStorageDefinition() on null in Drupal\Core\Field\Entity\BaseFieldOverride->getFieldStorageDefinition() (line 115 of /var/www/dep/docroot/core/lib/Drupal/Core/Field/Entity/BaseFieldOverride.php)

Edit:
I also had to rename the file I was trying to import, from core.base_field_override.media.document.revision_log.yml to core.base_field_override.media.document.revision_log_message.yml

mpp’s picture

chr.fritsch’s picture

Status: Active » Needs review
FileSize
4.22 KB

Here is a patch that tries to fix it. Would be nice to get some manual testing on this.

chr.fritsch’s picture

I adjusted the config creation and use now the lower db api for that.

chr.fritsch’s picture

Just a re-roll

phenaproxima’s picture

Thanks, @chr.fritsch! Can we get a fail patch on this one?

chr.fritsch’s picture

The last submitted patch, 17: 2933338-17-FAIL.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

phenaproxima’s picture

  1. +++ b/media_entity.install
    @@ -177,6 +177,23 @@ function _media_entity_get_bundles_using_exif() {
    +    list($bundle, $field) = explode('.', substr($override, strlen($prefix)));
    

    Wondering if we shouldn't use mb_substr() and mb_strlen() here, to avoid super-edge cases with non-ASCII characters.

  2. +++ b/media_entity.install
    @@ -388,10 +405,20 @@ function media_entity_update_8201() {
    +  $field_renames = ['revision_uid' => 'revision_user', 'revision_timestamp' => 'revision_created', 'revision_log' => 'revision_log_message'];
    

    I think we should add a comment above this line, explaining what we're doing here.

  3. +++ b/media_entity.install
    @@ -388,10 +405,20 @@ function media_entity_update_8201() {
    +    $db->changeField('media_revision', $old_field, $new_field, $field_definitions[$new_field]->getColumns()[$field_definitions[$new_field]->getMainPropertyName()]);
    

    Will this handle fields with multiple columns? It seems like we might want to do that, for robustness. (However, if this is too crazy, we can defer it to a separate issue if and when it arises in the wild.)

    However, I think we should refactor this line for readability. Let's just set up several variables to store things like the column name, the column definitions, etc. It's too hard to parse as-is.

  4. +++ b/media_entity.install
    @@ -388,10 +405,20 @@ function media_entity_update_8201() {
    +        $field_overwrite = $config_factory->getEditable('core.base_field_override.media.' . "$bundle.$old_field");
    +        $field_overwrite->set('id', "media.$bundle.$new_field");
    +        $field_overwrite->set('field_name', $new_field);
    +        $field_overwrite->save(TRUE);
    

    Nit: These lines can all be chained. We don't need $field_overwrite as its own variable.

  5. +++ b/media_entity.install
    @@ -388,10 +405,20 @@ function media_entity_update_8201() {
    +        $config_factory->rename('core.base_field_override.media.' . "$bundle.$old_field", 'core.base_field_override.media.' . "$bundle.$new_field");
    

    These string concatenations are a bit weird and hard to read. Can we simply do "core.base_field_override.media.$bundle.$old_field" and "core.base_field_override.media.$bundle.new_field"?

  6. +++ b/tests/src/Functional/CoreMediaUpdatePathTest.php
    @@ -172,6 +172,31 @@ class CoreMediaUpdatePathTest extends UpdatePathTestBase {
    +    $field_overwrites[] = Yaml::decode(file_get_contents(__DIR__ . '/../../fixtures/core.base_field_override.testfor2933338.yml'));
    

    Why is $field_overwrites an array?

chr.fritsch’s picture

Thanks for reviewing @phenaproxima

I addressed all the comments from #19.

alexpott’s picture

  1. Just FYI - Re #19.1 - these are machine names so they are validated to be ASCII. But there's no harm in using mb_substr().
  2. +++ b/tests/fixtures/core.base_field_override.testfor2933338.yml
    @@ -0,0 +1,18 @@
    +dependencies:
    +  config:
    +    - media_entity.bundle.image
    

    We should test that this is fixed correctly. To be media.type.image (I think)

  3. +++ b/tests/src/Functional/CoreMediaUpdatePathTest.php
    @@ -172,6 +172,31 @@ class CoreMediaUpdatePathTest extends UpdatePathTestBase {
    +    $field_overwrites = Yaml::decode(file_get_contents(__DIR__ . '/../../fixtures/core.base_field_override.testfor2933338.yml'));
    

    <3 - using config files here makes update path test so much easier to review.

chr.fritsch’s picture

Extending the test coverage

phenaproxima’s picture

Status: Needs review » Needs work
+++ b/tests/src/Functional/CoreMediaUpdatePathTest.php
@@ -193,7 +193,11 @@ class CoreMediaUpdatePathTest extends UpdatePathTestBase {
+    $this->assertEqual($new_field_config->get('id'), 'media.image.revision_log_message');
+    $this->assertEqual($new_field_config->get('field_name'), 'revision_log_message');

Should be using assertSame() here.

Otherwise, this is ready as far as I can tell.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
961 bytes
5.38 KB
phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Good go to, in my opinion, once green.

chr.fritsch’s picture

This is about “base field overrides” but we called them “base field overwrites”...
So I fixed that.

  • chr.fritsch committed cc4374d on 8.x-2.x
    Issue #2933338 by chr.fritsch, phenaproxima, alexpott: Media Entity...
chr.fritsch’s picture

Status: Reviewed & tested by the community » Fixed

Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.