Problem/motivation
If a media type's source field is deleted, fatal errors can occur on the status report page.
This is mostly by design; the media system assumes the source field exists and does not try to gracefully handle missing source fields. That said, a missing source field probably shouldn't cause a fatal error on the status report. It's one thing for that to happen on pages that are specific to the media system, but our code could be a little more graceful in hook_requirements().
Proposed resolution
In media_requirements(), silently skip over media types whose source field is missing. In #3205866: media_requirements() should report missing source fields, we should actually make the hook report the missing source fields as actionable errors.
Remaining tasks
Open the aforementioned follow-up, then review and commit the patch.
API changes
None.
Data model changes
None.
UI changes
None.
Release notes snippet
None needed.
Comment | File | Size | Author |
---|---|---|---|
#26 | 3106659-26-PASS.patch | 1.95 KB | phenaproxima |
#26 | 3106659-26-FAIL.patch | 1.3 KB | phenaproxima |
#12 | media-install-requirements-3106659.patch | 507 bytes | mulukallaarun |
#8 | 3106659-8.patch | 3.74 KB | audacus |
Comments
Comment #2
Drupaldev2013 CreditAttribution: Drupaldev2013 commentedI have applied this patch and It's work
Comment #3
joekersI had this issue when some of the config for a new media type hadn't been deployed properly so should try looking at that first.
I don't think the patch is needed but if it were you should take a look at the coding standards:
https://www.drupal.org/docs/develop/standards/coding-standards#controlst...
Comment #4
cilefen CreditAttribution: cilefen commentedComment #5
rajanvalecha12 CreditAttribution: rajanvalecha12 as a volunteer commentedThe patch in #2 didn't apply correctly to either core release 8.8.1 or 8.8.x.
Thus adding a patch as per 8.8.x.
Comment #7
aleevasI tried to fix last patch
Comment #8
audacus CreditAttribution: audacus at Previon Plus AG commentedI had to add another check because the source field definitions were
null
for the typemedia_entity_slideshare
of the module Media entity slideshare.Comment #10
aleevasSeems to it's works now for both version: 8.8 and 8.9
Comment #11
ddhuri CreditAttribution: ddhuri as a volunteer commented#8 works for me.
Thanks.
Comment #12
mulukallaarun CreditAttribution: mulukallaarun commentedAfter upgrade from Drupal 8.7.11 to 8.8.4, I also faced this error:
Error: call to a member function getitemdefinition() on null in media_requirements() (line 140 of /core/modules/media/media.install).
I had used the attached patch if it can help others.
Comment #14
cobenash#12 works for me.
Comment #15
Nicolas S. CreditAttribution: Nicolas S. commentedPatch #12 works too for me
Comment #18
phenaproximaTransferring credit from #3145098: Media requirements exception when field for a media type does not exist., which has been closed as a duplicate of this one. I am also copying that issue's summary into here, and tagging this issue for test coverage.
Comment #19
phenaproximaIMHO this is a major bug that breaks things for site builders and developers, so bumping priority.
Comment #21
RobbMLewis CreditAttribution: RobbMLewis as a volunteer and commentedPatch 12 works for me. This does break the site unless applied. Specifically, it kills Commerce. for my site. Can this get bumped up and put into core?
Comment #22
kndr@phenaproxima said in the comment https://www.drupal.org/project/drupal/issues/3145098#comment-13773168
If it is a foundational assumption of the media API (I totally agree with that) so how can we test this issue? What manual steps need to be done to break the functionality? I assume that brutal force like deleting a source field from a database is out of the question.
Comment #23
Michèle CreditAttribution: Michèle commentedPatch #12 works for me, too. Thank you very much!
I guess this has something to do with the minimal install profile I've used...
Comment #24
RobbMLewis CreditAttribution: RobbMLewis as a volunteer and commentedAnother core update, and once again patch 12 had to be applied. What do we need to do to get this into the next core update?
Comment #25
phenaproximaFor a start, I think an automated test which proves that the bug fix works as intended would go a long way to getting this change in. If anyone is interested in giving that a shot, there is extensive documentation about how to write tests at https://www.drupal.org/docs/automated-testing/phpunit-in-drupal.
Comment #26
phenaproximaHere you go -- a test-only patch to prove that the bug exists, and the same test combined with #12 to prove that it fixes the problem.
Comment #28
dgaspara CreditAttribution: dgaspara at NTT DATA commentedWorks for me. Thanks
Comment #29
timfletcher CreditAttribution: timfletcher as a volunteer commented#12 works for me. I was unable to load the Status Report page.
Comment #30
iamweird CreditAttribution: iamweird at Attico International commentedI confirm that #26 fixed the error for me.
Comment #31
marcoscanoLooks good to me. Thanks everyone!
Comment #32
danflanagan8I wrote a horrible update hook like the one below:
Then I went to /admin/reports/status as got this white screen:
Error: Call to a member function getItemDefinition() on null in media_requirements() (line 138 of core/modules/media/media.install).
Then I applied the PASS patch from #26. The status report page was fixed. However, there were not any warnings about the video media type being messed up. I would expect something like that to be listed in the ENTITY/FIELD DEFINITIONS error perhaps. What is the expected behavior here?
Also when I went to edit the Video media type (the one I mucked up) I got this message in a whitescreen:
Error: Call to a member function getLabel() on null in Drupal\media\MediaSourceBase->buildConfigurationForm() (line 199 of core/modules/media/src/MediaSourceBase.php).
This was at /admin/structure/media/manage/video. This situation might be outside the scope of this issue but it's possible to read this issue as "Missing source field shouldn't cause any whitescreens."
Comment #33
danflanagan8Tagging as "Needs Issue Summary Update" to clarify exactly what is getting fixed here. (Probably just preventing the fatal error.) Tagging as "Needs followup" so that we can work on adding an error to the status page about the missing source field. But that work shouldn't slow down fixing the fatal error.
Comment #34
phenaproximaComment #35
phenaproximaDowngrading the issue status, since this is definitely in the "shouldn't occur under normal circumstances" class of bugs.
Comment #36
phenaproximaFiled #3205866: media_requirements() should report missing source fields to add explicit errors to the status report. Therefore, with an improved issue summary and a follow-up for a nicer fix, I'm restoring RTBC. :)
Comment #37
phenaproximaComment #42
larowlanCommitted b2686e3 and pushed to 9.2.x. Thanks!
As there is little risk of disruption, backported this to 9.1.x
Thanks all
🐛🔨
Comment #44
loopy1492 CreditAttribution: loopy1492 commentedIt is still possible to get this error from time to time. In our case, it was because one of the many strange updates to Lightning Media did not get captured in config at some point.
Sadly, the real solution as far as I can tell is to hunt down the config file for the media type and find the source field definition for it. So, for our specific case, I had to hunt down media.type.remote_video.yml and look for the source_configration.source_field in there. Fortunately, it wasn't blank and saw that it was field_media_oembed_video.
I then did a files search for field_media_oembed_video. Of course, it wasn't in my default config folder so I had to go find it in the contrib folders. In the
folder, I found both the field and the field storage configs:
Finding the field storage was easy, but I had to kind of hope that remote_video.field_media_oembed_video would work since just plain field_media_oembed_video did not exist anywhere in the code base, contrib or custom.
I imported the config and the errors disappeared. Previously, the field edit page for that media type was also broken and it is no longer broken.