Don't display OEmbed error to anonymous visitors when resource stops being available

Adding patch to log instead of sending a message on oEmbed fetch error.

Drupal 8 is end-of-life as of November 17, 2021 → . There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Minimal change to #2.
(Sorry, a little problem in the previous patch.)

I agree this issue makes sense and it's a bug, since the "Could not retrieve the oEmbed resource" being displayed to anonymous visitors when a remote video stops being available isn't ideal.

However, I do think that's a good indicator for editors. So my suggestion is that instead of always logging, we should check $media->access('update')
- If the user can edit the media item, show the error on screen, so they can take action to fix the problem
- If the user can't edit, it's likely a visitor, in which case we should just just log the error.

And yes, we should definitely return NULL in any case (as suggested by #6), otherwise the site will break hard when it tries to call methods on the non-existing $resource variable.

Also, we need tests! :)

Drupal 9.4.0-alpha1 → was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Patch from #11 🐛 Don't display OEmbed error to anonymous visitors when resource stops being available Needs work works fine.

Tested with Drupal 9.3.13

Drupal 9.5.0-beta2 → and Drupal 10.0.0-beta2 → were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

#11 🐛 Don't display OEmbed error to anonymous visitors when resource stops being available Needs work is good for me too.

Tested with Drupal 9.4.7

Added 10.1.x: PHP 8.1 & MySQL 5.7 to #11 to see how it works out.

Rewrote a bit issue-3202896-6.patch, added link for editing media entity.

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened → , as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Sounds like a good addition!

Only thing that is left is a proper test to move this issue forward.

Should we perhaps close this issue and merge efforts in ✨ Improve oEmbed exception logging Needs work to make fixing these issues and testing more easy? Both are touching the message details and are very close... What do you think?

To me the approach in this issue is better than ✨ Improve oEmbed exception logging Needs work .

✨ Improve oEmbed exception logging Needs work : The change in core/modules/media/src/OEmbed/ResourceFetcher.php will now expose a lot of information to an anonymous user when the fetcher error message is displayed by \Drupal\media\Plugin\media\Source\OEmbed::getMetadata(). That is not desired on any website.

  throw new ResourceException('Could not retrieve the oEmbed resource: ' . $e->getMessage(), $url, [], $e);

When indexing media with unavailable resource using search api, I see the following error in the log:
Warning: Undefined variable $resource_url in Drupal\media\Plugin\media\Source\OEmbed->getMetadata() (line 253 of core/modules/media/src/Plugin/media/Source/OEmbed.php)

Steps to reproduce:
- Create media entity using live stream video using e.g. youtube.
- From youtube, finish the steaming and make it unavailable.
- Use search api to index media.
- run search index.
- Check the log.

I think we should use media source field value to display the resource url as follows:

$this->logger->error(
  $e->getMessage() . ' Resource URL: %resource_url | Media ID: %media_id',
  ['%resource_url' => $media->getSource()->getSourceFieldValue($media), '%media_id' => $media->id()]
);

recrit → changed the visibility of the branch 3202896-dont-display-oembed to hidden.

recrit → changed the visibility of the branch 11.x to hidden.

recrit → changed the visibility of the branch 10.3.x to hidden.

recrit → changed the visibility of the branch 10.4.x to hidden.

@msnassar The Undefined variable $resource_url should be fixed in the latest MR.
I created an MR against 11.x with error message updated to the following. I removed the "|" from the message since system logs often are delimited with "|" which could cause issue with parsing the log.

      $this->logger->error(
       'An error occurred while fetching an oEmbed resource for Media ID: %media_id, Embed URL: %media_url, oEmbed Resource URL: %resource_url. Error: @error',
        [
          '%resource_url' => !empty($resource_url) ? $resource_url : 'unknown',
          '%media_url' => $media_url,
          '%media_id' => $media->id(),
          '@error' => $e->getMessage(),
        ]
      );

Attached is a static patch of the MR for composer builds. Please contrib to the MR for any further development.

The patch issue-3202896-7.patch and the diff drupal-3202896-MR9357-11.x--20240828-1.diff look good for me.

Have not reviewed but was previously tagged for tests which still appear to be needed.

See #20 I still think it would make more sense to get that one finished instead...? (And yes, it's annoying for users ;))

erindarri → changed the visibility of the branch 10.3.x to active.

Applied a patch from the Merge Request diff successfully to Drupal Core 10.3.13.

I reviewed the Merge Request in #2972846 first, but that issue does not add an access check before logging the error and does not appear to resolve the issue of this error showing to anonymous users. As another user commented in that issue, it appears the changes there are exposing even more detailed information to anonymous users which exacerbates the issue presented here.

Maybe it could dispatch an event when the source isn't found?

I have a site with over 100,000 of entities with media references, many of these have become old and I would like to be able to flag the posts so I can easily find them and remove them or take some appropriate action.

If i was able to listen for an event when there is an error I could do this.

I ran into a unique fatal error caused by the patch. In short, the media entity being used to generate the edit link for the error message will not always be saved in the database, and will not have an ID or valid edit link. I recommend adding a condition to check if for the ID of the media entity before continuing.

In length, a content type with an oEmbed media entity will reference the media entity using an entity reference field. When using layout builder for the default display for the content type, the content preview for layout builder uses the generateSampleValue method on the entity reference item to generate a preview. The method attempts to find a random, valid entity to use as a sample value. If it cannot find one, it generates an entity on the fly. The entity generated on the fly is not saved, and does not have a valid ID. This throws a fatal error then when using this code because it attempts to generate an edit link on a media entity without an ID. I recommend adding a condition to check if for the ID of the media entity before continuing.

Perhaps there's a better way to go about this then adding a check for the $media->id().

I added the conditional to the merge request and cut a new patch on 10.4.x

loze → changed the visibility of the branch 3202896-oembed-error-message to hidden.

Maybe it could dispatch an event when the source isn't found?

This sounds like a good follow up task as it expands the scope of this, and I think it would be better to fix the user-facing issue as originally reported first. This is definitely something I have noticed and agree it is a problem!