Media sources should handle empty source field values more gracefully

Experiencing this on a site with migrated data where the main source property is not populated. Looks like core should at least check if there's an actual value and:

- a) Return nothing if that's a possible scenario?
or
- b) If it's not a situation that should ever happen, be explicit about it and throw an Exception, so that callers can act on it explicitly.

So here's the thing: no media item is allowed to have an empty source field value.The source field is the one field that, on any given media item, should never be empty. This is a fundamental assumption of the media system.

I think we need to change the approach here. Rather than trying to paper over empty source fields, we need to identify what code path is trying to access an empty source field. So I think the most helpful thing here, for the moment, would be to provide either steps to reproduce the problem, or a backtrace...

When the media_entity contrib module was moved to the core media module, the hard requirement was added that media should always have a source field. So it is technically correct that this leads to issues and I agree with phenaproxima that working around the issue is not a good idea. Basically we have a "broken" installation.

Because media_entity in contrib did not have that hard requirement, there are quite a lot of sites out there that have media types without source field. I have seen issues with this in the upgrade path from contrib to core as well. My suggestion would be that we help users fix the "broken" config, instead of helping them work around it.

So we need to:

1. Provide documentation on how to solve this. Maybe a message in the status report with a link to the documentation?
2. Provide a drush command in contrib the help users fix it? Not entirely sure how this should work since the source field might not exist yet.
3. Provide some kind of wizard to help users fix it by letting the choose an existing field or create a new field for media types without a source field.

Most important thing is probably making users aware of this and providing documentation on how to fix it.

I am having this issue with a fresh 8.8.x installation.

Exception is raised when I create a new media with emtpy field_media_oembed_video.

How to reproduce:

1. Create a new media type
2. Add the field_media_oembed_video
3. Mark the field as not required
4. Create a new media entity with empty value for oembed URL

Result:

The website encountered an unexpected error. Please try again later.
Error: Call to a member function mainPropertyName() on null in Drupal\media\MediaSourceBase->getSourceFieldValue() (line 335 of core/modules/media/src/MediaSourceBase.php). 

I think this is more critical than "a broken installation" (@seanB #7).

We should be able to mark any field as "not required".

Marked this issue as "major".

We should be able to mark any field as "not required".

Except that, the way the media system is designed, you cannot do this with the source field of a media type. It is not currently enforced in the UI due to complicated technical reasons (there is another issue for that, I think), but it's true. The source field of a media type is always required, and it is not allowed to be made optional, for the reason @seanB outlined in #7. And that's that.

I fully agree that we need to either document this, find a way to automatically fix the broken installations, enforce the "requiredness" of the source field in the UI, or some combination of all of these things. And because it is important to take some course of action on this, I agree with having this issue be marked "major".

But, having said that, making the source field optional is not going to fly. Doing that would contradict the explicit architectural and technical design of the media system and would cause an enormous number of problems, on top of being a major, backwards-incompatible API change.

After talking to phenaproxima we agreed on the following (please correct me if I'm wrong).

Existing sites might have a bunch of media items without a source field value. There is not good way to automagically fix that. So for those cases, failing with an exception is bad and for that reason I'm inclined to +1 the improvement in the patch of #2.

Regarding migrations of old sites, it would help very much to have more background on why it would make sense to have a media item without a source value? Same as required node titles, media has a required source field. The source field can contain any value though, depending on the media source. So if needed, sites can always install https://www.drupal.org/project/media_entity_generic or create a custom media source.

Regarding #9. We should not be able to mark any field as not required. The node title or taxonomy term names for example are also always required. The source field for media is just like that. To improve this we should have a followup to probably make it impossible to make a source field non-required, and provide some documentation on why that is.

Looking at #2:

+++ b/core/modules/media/src/MediaSourceBase.php
@@ -332,7 +332,10 @@ public function getSourceFieldValue(MediaInterface $media) {
     $field_item = $media->get($source_field)->first();
...
 

Do we really need both $field_item && !empty($field_item)?

Maybe use $media->get($source_field)->isEmpty() instead?

Here is a patch which implements the proposed change in #11, with a test.

Since MediaSourceInterface::getSourceFieldValue() already defined a "mixed" return type, I don't think this constitutes an API change. We're just now explicitly documenting, and handling, situations which would previously cause a fatal error.

This looks great to me! Thanks.

We have a site where users can delete file entities from admin/content/file (for cleanup purposes). Apparently this is also a big issue when a file entity is deleted without deleting the media entity.

Discussed with @phenaproxima and @seanB. So the advantage here is that is a migration does result in broken media entities - or a user deletes a file entity directly - then the user can use the UI to fix the media entity. That's a good thing.

One problem with the approach is that errors and the expectation that this is not null is more hidden - so we're not failing as fast which might not be what you want when you're building a migration. We also considered adding logging but this is likely to add up filing the logs very quickly.

@phenaproxima came up with the idea

We could potentially also take advantage of a PHP 7 thing, where these types of errors are (I think) \Throwable. They could be caught, say, by MediaForm and turned into error messages.

After some more discussion, we decided we have to accept the reality that media items can have an empty source field value. We don't really want it, but technically this is just something we have to deal with. Some obvious examples:

• The source field is not required (we strongly encourage site builders to not do this, but it is not enforced).
• When the source field is an entity reference, the references entity can be removed (a file entity can be deleted).

Since every existing site could technically allow media items with empty source fields, we have decided on the following:

• Move forward with the current solution on this issue
• Add some NULL handling to core usages (like \Drupal\media\Plugin\media\Source\OEmbed::getMetadata and \Drupal\media\Plugin\Validation\Constraint\OEmbedResourceConstraintValidator::validate)
• Probably improve documentation
• Open follow-up issue to discuss adding som pre or post save logging (opened #3106088: Add logging for media items with empty source field values)

Okay. Here is a patch which adds explicit NULL handling for the two things in core which use getSourceFieldValue(), plus kernel test coverage.

Thanks @phenaproxima. Changes look good to me and the test coverage is pretty thorough. RTBC!

+++ b/core/modules/media/src/Plugin/Validation/Constraint/OEmbedResourceConstraintValidator.php
@@ -83,6 +83,11 @@ public function validate($value, Constraint $constraint) {
+    // The URL may be NULL if the source field is empty, in which case just
+    // there's nothing more to do.
+    if (empty($url)) {
+      return;
+    }

I'm not sure this is correct. I think we should fail validation if this is the case.

How's this? I made an empty URL an explicit validation error, and refactored the test so as to do a little less mocking.

I made an empty URL an explicit validation error

What happens if users decide to make the source field not required? Would this then fail validation?

@seanB if there is no url then the remote media is invalid. This is only for oembed and in this case I think the URL is truly required.

The reason I ask is because users can make the field non-required. Which could be confusing. But I guess since this never worked it shouldn't really be a problem.

Unrelated test failures, so making it RTBC as per comment #39.

Committed and pushed 926087384e to 9.0.x and 58d16a9b5a to 8.9.x. Thanks!

FILE: ...media/tests/src/Kernel/OEmbedResourceConstraintValidatorTest.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 56 | ERROR | [x] Expected 1 blank line before function; 0 found
 62 | ERROR | [x] Expected 1 blank line after function; 0 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

I fixed these coding standards on commit. Note that anonymous classes don't look fantastic with our coding standards but ho hum...