Improve oEmbed exception logging

Can we also get another error message fixed with this patch?

With this video https://www.youtube.com/watch?v=HmZKgaHa3Fg, I was getting this error:

Could not retrieve the oEmbed resource.

which doesn't have the exception message so isn't very helpful.

This should be changed from:

    } catch (TransferException $e) {
      throw new ResourceException('Could not retrieve the oEmbed resource.', $url, [], $e);
    }

to something like:

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

With the change, the mesage is:

Could not retrieve the oEmbed resource: Client error: `GET https://www.youtube.com/oembed?url=https://www.youtube.com/watch?v=HmZKgaHa3Fg` resulted in a `401 Unauthorized` response: Unauthorized

Addressed the comment on #28
Attached patch against Drupal 10.1.x

Try To Fix the #29 Patch Faild To Apply.

Great point @Kristen Pol in #28.
I just came here to write the same. The current message "Could not retrieve the oEmbed resource" is very unclear and you have no good way to identify which oEmbed resource / entity causes the issue!

Just found this similar issue: 🐛 Don't display OEmbed error to anonymous visitors when resource stops being available Needs work
Guess both should be solved together.

#29 didn't apply, and #30 failed tests, so here's #25 reroll + #28.

The last submitted patch, 33: 2972846-33-media-oembed-error.patch, failed testing. View results →

And an update for the change in error message.

Correcting interdiff.

Last try for today.

The last submitted patch, 37: 2972846-37-media-oembed-error.patch, failed testing. View results →

Can the issue summary be updated please. started with the template.

Also will need additional test coverage.

Updating IS from core template.

I am open to NOT changing user facing messages if that permits us to land the original proposed task more readily. I'll add an alternative patch without the message and related test change so we have that option.

Given 🐛 Don't display OEmbed error to anonymous visitors when resource stops being available Needs work it may be better to leave the exception surfacing to that issue.

@smustgrave if we go that way, do you feel additional tests are required?

Would need a test for the logs.

The current test update is just checking for a ":" being added.

Re #41 I agree the user facing message might stay as-is, simply log the details. I think that's what you'd expect and is most safe.

Patch #39 applies on 9.5.x but results in the following error:

Error: Call to undefined method GuzzleHttp\Exception\ClientException::getUrl() in Drupal\media\Plugin\Validation\Constraint\OEmbedResourceConstraintValidator->handleException() (line 142 of core/modules/media/src/Plugin/Validation/Constraint/OEmbedResourceConstraintValidator.php).

Drupal\media\Plugin\Validation\Constraint\OEmbedResourceConstraintValidator->handleException(Object, 'The provided URL does not represent a valid oEmbed resource.') (Line: 120)
Drupal\media\Plugin\Validation\Constraint\OEmbedResourceConstraintValidator->validate(Object, Object) (Line: 201)
Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateConstraints(Object, '0000000000000a9f0000000000000000', Array) (Line: 153)
Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateNode(Object) (Line: 163)
Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateNode(Object, Array, 1) (Line: 105)
Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validate(Object, NULL, NULL) (Line: 93)
Drupal\Core\TypedData\Validation\RecursiveValidator->validate(Object) (Line: 132)
Drupal\Core\TypedData\TypedData->validate() (Line: 489)
Drupal\Core\Entity\ContentEntityBase->validate() (Line: 471)
Drupal\media\Entity\Media->validate() (Line: 188)
Drupal\Core\Entity\ContentEntityForm->validateForm(Array, Object)
call_user_func_array(Array, Array) (Line: 82)
Drupal\Core\Form\FormValidator->executeValidateHandlers(Array, Object) (Line: 275)
Drupal\Core\Form\FormValidator->doValidateForm(Array, Object, 'media_remote_video_add_form') (Line: 118)
Drupal\Core\Form\FormValidator->validateForm('media_remote_video_add_form', Array, Object) (Line: 593)
Drupal\Core\Form\FormBuilder->processForm('media_remote_video_add_form', Array, Object) (Line: 325)
Drupal\Core\Form\FormBuilder->buildForm(Object, Object) (Line: 73)
Drupal\Core\Controller\FormController->getContentResult(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 580)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 169)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 81)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 58)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 153)
Drupal\cryptolog\CryptologMiddleware->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 718)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

Needs work as of #45. One way might be to implement __toString() differently in the two exception classes and just call that instead of ->getUrl() and others.

Another alternative would be to have two different handleException() function. Which way should we go?

There seem to be further Exceptions like "ClientException", which do not have the url part implemented. I guess data would be enough for those exceptions, and we only add the url for when it is an ResourceException?

Re #47: Yes!

I just wondered why "ClientException" is even handled by our "handleException" method, since we only catch "ResourceException" and "ProviderException".
The problem is line 153 -> 154 in "/var/www/html/web/core/modules/media/src/Plugin/Validation/Constraint/OEmbedResourceConstraintValidator.php":

[...]
   $this->logger->error($message, $context);
   $e = $e->getPrevious();
} while ($e);

This while loop will rerun our handleException method, with the previous Throwable, and since in my case this is a "ClientException" it will fail on $e->getData().

Is this intended behavior? Should we also handle other exceptions then "ResourceException" and "ProviderException", even if we do not explicitly catch them in code?? Seems pretty dirty to me.

Yes, that's indeed intended, as the exceptions are wrapping each other. I personally don't really like this do ... while thing, but that's not our decision, I guess and existed before this patch.

Anyway special methods may only be called, if they are implemented. As written above, another option would be to override __toString in the Exception to provide further information, instead of using if-clauses, but there are many pro's and con's. Someone has to decide here, which way to go or simply take one way...

Interdiff between "2972846-39-media-oembed-error.patch" and "2972846-improve-oembed-exception" can be seen here: https://git.drupalcode.org/project/drupal/-/merge_requests/4059/diffs?co....

A minor problem still remains. As we are logging inside a while loop, there is the possibility of logging multiple errors with practically the same output:

First log message:

Client error: `GET https://www.youtube.com/oembed?url=https://www.youtube.com/watch?v=GtL1h...` resulted in a `400 Bad Request` response: Bad Request

Second log message:

Could not retrieve the oEmbed resource: Client error: `GET https://www.youtube.com/oembed?url=https://www.youtube.com/watch?v=GtL1h...` resulted in a `400 Bad Request` response: Bad Request URL: https://www.youtube.com/oembed?url=https://www.youtube.com/watch?v=GtL1h....

I also tried adjusting / adding tests, but the currently implemented oembed tests are super "stuffed" and overwhelming. Maybe someone else with a bit more inside knowledge could adjust them accordingly. I would've simply enabled the "media" module and tested on the "remote_video" media type, but that seems way too much unnecessary overhead for such a simple test.

Moving to NW for the additional test coverage.

Since this is testing log messages there may be some trait that could do that? Worth posting in #testing for any suggestions.

Alright, I added an error logging test taking inspiration from dblog/tests/src/Functional/DbLogTest.php, testLogEventPage(), on how to receive the logged watchdog event id. The only problem left is, that the test will fail.
For some reason the latest watchdog log, is NOT our logged oembed error event and the event is NOT logged at all, when checking the watchdog overview!
I am unsure, why that is. I already tried enabling the media and media_library modules, and trying some other workarounds (like pressing the "Add" button multiple times and checking on the dblog overview, for any error event to get logged), but with no avail.

I'll ask in the Slack testing channel. Maybe someone there might have an idea.

This is the test output of the "Recent log messages" page with and without media and media_library enabled:

Seems test failure is legit to the issue.

The following 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);

In my opinion, the approach on 🐛 Don't display OEmbed error to anonymous visitors when resource stops being available Needs work is a better solution. It does not display the error to anonymous users (or any user that cannot edit the media) and provides a reference to the Media ID causing the issue.