Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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:
- 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.
- 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
- Install Media Entity 1.x and create at least one bundle.
- Override the revision_log field for at least one of those bundles.
- Export config.
- Run the update path to core Media.
- Try to re-import your config.
- Enjoy the fireworks.
Comment | File | Size | Author |
---|---|---|---|
#26 | 2933338-26.patch | 5.37 KB | chr.fritsch |
| |||
#26 | interdiff-2933338-24-26.txt | 3.46 KB | chr.fritsch |
#24 | 2933338-24.patch | 5.38 KB | chr.fritsch |
| |||
#24 | interdiff-2933338-22-24.txt | 961 bytes | chr.fritsch |
#22 | 2933338-22.patch | 5.38 KB | chr.fritsch |
|
Comments
Comment #2
phenaproximaHi -- we need steps to reproduce this, not just an error message and backtrace (although those are helpful).
Comment #3
justclint CreditAttribution: justclint as a volunteer commentedHi 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:
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:
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!
Comment #4
phenaproximaThanks for the detailed information, @justclint. I'll see what we can do about reproducing and/or fixing this.
Comment #5
marknatividad CreditAttribution: marknatividad as a volunteer commentedSubscribing 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.
Comment #6
phenaproximaSo 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:
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).
Comment #7
justclint CreditAttribution: justclint as a volunteer commentedThanks for taking a look into this @phenaproxima!
Ill go ahead and give this a shot and then report my results.
Comment #8
Web-Beest@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
Comment #9
phenaproximaI am moving this to Media Entity's issue queue and updating the issue summary.
Comment #10
justclint CreditAttribution: justclint as a volunteer commentedSorry 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!
Comment #11
acontia CreditAttribution: acontia commentedI 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:
Edit:
I also had to rename the file I was trying to import, from
core.base_field_override.media.document.revision_log.yml
tocore.base_field_override.media.document.revision_log_message.yml
Comment #12
mpp CreditAttribution: mpp as a volunteer and at AmeXio for District09 commentedSeems to be related to https://www.drupal.org/project/drupal/issues/3043741.
Comment #13
chr.fritschHere is a patch that tries to fix it. Would be nice to get some manual testing on this.
Comment #14
chr.fritschI adjusted the config creation and use now the lower db api for that.
Comment #15
chr.fritschJust a re-roll
Comment #16
phenaproximaThanks, @chr.fritsch! Can we get a fail patch on this one?
Comment #17
chr.fritschSure, so re-uploading #15
Comment #19
phenaproximaWondering if we shouldn't use mb_substr() and mb_strlen() here, to avoid super-edge cases with non-ASCII characters.
I think we should add a comment above this line, explaining what we're doing here.
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.
Nit: These lines can all be chained. We don't need $field_overwrite as its own variable.
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"?
Why is $field_overwrites an array?
Comment #20
chr.fritschThanks for reviewing @phenaproxima
I addressed all the comments from #19.
Comment #21
alexpottWe should test that this is fixed correctly. To be media.type.image (I think)
<3 - using config files here makes update path test so much easier to review.
Comment #22
chr.fritschExtending the test coverage
Comment #23
phenaproximaShould be using assertSame() here.
Otherwise, this is ready as far as I can tell.
Comment #24
chr.fritschComment #25
phenaproximaGood go to, in my opinion, once green.
Comment #26
chr.fritschThis is about “base field overrides” but we called them “base field overwrites”...
So I fixed that.
Comment #28
chr.fritschFixed