Improve oEmbed exception logging

Does this include not completely shut down the site if network connection is bad or the oEmbed service (url) is not available? I see no reason to break stuff instead of returning an error informing about that situation.

First draft.

Feedback welcome.

Found another place for improvement. See interdiff.

To be clear about addition: without it in the locked environment where Drupal talks to the world via proxy and resource destination domain is not allowed you will see this in log for media "cURL error 56: Received HTTP code 403 from proxy after CONNECT (see https://curl.haxx.se/libcurl/c/libcurl-errors.html)". Not helpful at all, but with the request URI you will be clear about what was the request for.

Re-roll of #12 for 9.1.x.

As correctly pointed out by the colleague ( thanks Phil ) TransferException doesn't have the getRequest() method.

One problem is that Guzzle only returns the first 120 characters of the response if there's an exception in the request; RequestException::create() calls get_message_body_summary() (without the second argument to set the message length so it defaults to 120 characters), which calls Message::bodySummary(), which truncates the string to the default of 120 characters. Argh.

I think in order to get the full string from guzzle you'd need to extend GuzzleHttp\Exception\RequestException, override the getResponseBodySummary() method and pass in a larger number to get_message_body_summary(), and change core to use the new exception handler instead of the Guzzle one.

Re-rolled for 9.4.

n.b. #20 no longer applies as of 9.4.8.

Re-rolled for 9.4.x (accounting for #3310510: Harden error logging of OEmbed thumbnail fetching logic (YouTube errors contain special characters) released in 9.4.8).

I think we need a patch for 10.1.x-dev. That's the new patch version. see #23

The same patch has passed testing for 10.1.x, so I guess we don't need a re-roll for that after all.

I'm still trying to get the testbot to show green for some test cases, but I think those issues are unrelated to the patch.

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 error for this video 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 wrote a blog post about that: https://julian.pustkuchen.com/en/understanding-drupal-media-error-messag...

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

#29 didn't apply, and #30 failed tests, so here's #25 reroll + #28 for #drupalsouth code sprint

And an update for the change in error message.

Correcting interdiff.

Last try for today.

Ok, this works, but on reflection: by changing the error message the user sees, I'm now concerned that we might be straying into messing with localisation.

The text change here is from "Could not retrieve the oEmbed resource." to "Could not retrieve the oEmbed resource: " followed by the supplied exception message.

I don't see instructions on considerations for changing UI text in https://www.drupal.org/docs/develop/user-interface-standards/interface-text - I recall we have responsibilities here though.

On mobile now and can't load localize. Drupal.org but appreciate input on how to best make that improvement.

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 change (logging but not messages) more easily. I'll add an alternative patch without the message and related test change so we have that option.

Given #3202896: Don't display OEmbed error to anonymous visitors when resource stops being available it may be better to leave user facing changes 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.

Really annoying to not see details in the log and only this not very helpful message. Perhaps we should gain the priority here.

Patch #39 applies on 9.5.x but results in the following error when creating a new external Video Media Entity:

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)

Edit: The reason is that both kind of Exceptions are handled through the same function:

// Ensure that the URL matches a provider.
    try {
      $provider = $this->urlResolver->getProviderByUrl($url);
    }
    catch (ResourceException $e) {
      $this->handleException($e, $constraint->unknownProviderMessage);
      return;
    }
    catch (ProviderException $e) {
      $this->handleException($e, $constraint->providerErrorMessage);
      return;
    }

but only Drupal\media\OEmbed\ResourceException has a ->getUrl() so that needs to be split.
The second exception here is a GuzzleHttp\Exception\ClientException so the call fails!

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?

EDIT: Damn "ClientException" doesn't even have the "getData()" function implemented...

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, AFTER the "ResourceException" gets thrown and displayed on the page:

Seems test failure is legit to the issue.