Problem/Motivation

This is a follow-up to #3106659: Media types with missing source fields break the status report page.

The media system assumes that all media types' source fields are present and accounted for. It does not generally try to gracefully handle missing source fields. Normally that's okay, as it is an explicit assumption of the media system. However, it would be nice if media types with missing source fields were reported as errors on the status report page, so that site builders would be made aware that there is a problem, rather than being surprised with nasty fatal errors when people try to interact with the media system.

Steps to reproduce

Create a media type, and then delete its source field (using drush php or some other under-the-hood mechanism). If you then visit the status report page, you'll get a fatal error (before #3106659: Media types with missing source fields break the status report page is fixed), or no indication that the media type is broken (after).

Proposed resolution

media_requirements() should explicitly report errors for media types whose source field doesn't exist.

Remaining tasks

Write a patch with tests, then review and commit it. Awww yeah.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

TBD

CommentFileSizeAuthor
#9 interdiff_4-9.txt2.42 KBdanflanagan8
#9 3205866-9.patch3.6 KBdanflanagan8
#4 3205866-4.patch2.48 KBdanflanagan8

Issue fork drupal-3205866

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

phenaproxima created an issue. See original summary.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

I've obviously got zero control over this, but I recommend that this change be backported. It should be a big benefit to site builders, with near-zero risk of disruption.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Status: Active » Needs review
StatusFileSize
new 3205866-4.patch2.48 KB

Here's a patch to get the ball rolling here. We haven't defined exactly what the message should be. Not sure if we need UX to approve something. Anyway, this adds a reasonable error and updates the relevant test to check for it.

joachim’s picture

joachim commented
Status: Needs review » Reviewed & tested by the community

LGTM!

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

No complaints from me either. Un-crediting myself, since I didn't do anything here.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/media/media.install
@@ -135,6 +136,18 @@ function media_requirements($phase) {
       if (empty($source_field_definition) || !is_a($source_field_definition->getItemDefinition()->getClass(), ImageItem::class, TRUE)) {

Actually, a couple of small requests.

First, this empty() check is now redundant. Could you remove it?

Secondly, it would be clearer if we moved the pre-existing comment, currently living just above your new code -- the one explaining the image stuff -- just above the is_a() check. Meanwhile, we should keep your new comment, about checking for empty fields, where it is. That would make it clearer what each check is actually doing.

Apart from these points, no complaints!

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

Crediting myself again for patch-shaping feedback. :)

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Status: Needs work » Needs review
StatusFileSize
new 3205866-9.patch3.6 KB
new interdiff_4-9.txt2.42 KB

Good ideas, I think. Does this look better? I moved the comments to the appropriate places within the foreach and I also got rid of the redundant empty check.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community

It looks great. One small nitpick, if you have time to address it, is that in one line of the new requirement, you call t(), and in the next line, you use new TranslatableMarkup(). I think that t() returns a TranslatableMarkup object, so IMHO we should use t() in both places for consistency.

However, that's not something I care to block commit over, especially since the use of TranslatableMarkup doesn't appear to have resulted in an additional use statement. So fix it if you feel up to it, otherwise enjoy this RTBC!

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented

That inconsistency is a little funny. I started by copying the array for the image style warning message in the same foreach. I think since the new message matches the older code around it, I'll just leave it as it is and revel in the RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 9.3.x-dev » 9.2.x-dev
Category: Feature request » Bug report
Status: Reviewed & tested by the community » Fixed

Committed and pushed d8b4e40b05 to 9.3.x and 3ea4389651 to 9.2.x. Thanks!

I think this is a bug fix - reporting errors that people need to fix is important.

diff --git a/core/modules/media/media.install b/core/modules/media/media.install
index 0ab57d1d87..1be5ae6db6 100644
--- a/core/modules/media/media.install
+++ b/core/modules/media/media.install
@@ -131,7 +131,7 @@ function media_requirements($phase) {
       if (empty($source_field_definition)) {
         $requirements['media_missing_source_field_' . $type->id()] = [
           'title' => t('Media'),
-          'description' => new TranslatableMarkup('The source field definition for the %type media type is missing.',
+          'description' => t('The source field definition for the %type media type is missing.',
             [
               '%type' => $type->label(),
             ]

I fixed the inconsistency - seeing these used next to each other is odd. FWIW now that bootstrap.inc is included by composer functional code can use t() without issue.

  • alexpott committed d8b4e40 on 9.3.x 
    Issue #3205866 by danflanagan8, phenaproxima: media_requirements()...

  • alexpott committed 3ea4389 on 9.2.x 
    Issue #3205866 by danflanagan8, phenaproxima: media_requirements()...

Status: Fixed » Closed (fixed)

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