oEmbed resource fetcher should allow adjusting timeout

Hello,

I just wanted to add the error message that led me to realised that this was the solution.

In case other encounter the same issue during a migration.

Could not retrieve the oEmbed resource.

Thanks for the patch Hopefully, we can see it in core soon.

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

This will need a change record for the new setting variable.

Change record has been drafted here https://www.drupal.org/node/3362722 →

Change record seems good.

Triaging the RTBC queue.

I read the issue summary and the comments. I see that the change record has been reviewed. I also expect to see comments that the patch has been read and tested.

1. +++ b/core/assets/scaffold/files/default.settings.php
   @@ -305,6 +305,18 @@
   +# $settings['http_client_config']['timeout'] = 10;
   

   It is my understanding that the values here are the defaults. Therefor, this should be 5.

2. +++ b/core/assets/scaffold/files/default.settings.php
   @@ -305,6 +305,18 @@
   + * If you have outgoing HTTP requests for oEmbed resources, some custom
   + * providers may require a longer timeout than the default 5 seconds.
   + * - $settings['http_client_config']['timeout'] = 10; Would increase this to
   + *   10 seconds.
   + * You could conditionally set this to higher for CLI commands by checking
   + * if PHP_SAPI === 'cli'.
   + */
   +# $settings['http_client_config']['timeout'] = 10;
   

   Since this needs a change I looked at this paragraph more. I think the paragraph should change to use @code/@endcode block. I wasn't sure how to include the sentence about PHP_SAPI. I leave that to the whoever works on this. Something like this:

   * If you have outgoing HTTP requests for oEmbed resources, some providers may
   * require a longer timeout than the default. You may uncomment this setting and
   * change the value.
   *
   * For example:
   * @code
   * $settings['http_client_config']['timeout'] = 10;
   * * @encode
   * This changes the timeout to 10 seconds.
   */
   $settings['http_client_config']['timeout'] = 5;

The name of the new settings is rather generic but is only used for oEmbed. Should the name be changed to reflect that?

I then read the CR and decided to update it to flow better and put the example in a header.

I am going to set this back to NW for these changes.

anup.singh → made their first commit to this issue’s fork.

The name of the new settings is rather generic but is only used for oEmbed. Should the name be changed to reflect that?

It is not used only for oEmbed. The primary use of `http_client_config` is in ClientFactory where it allows overriding the default http client settings, including the timeout (which by default is set to 30s).

The description in default.settings.php should probably also mention that changing this setting will impact all code that uses the http_client service without providing custom timeout explicitly.

Maybe a better way to approach this would be to provide a separate setting for the oEmbed ResourceFetcher timeout, since from what I understand the 5s limit was added in https://www.drupal.org/project/drupal/issues/3202145 → specifically to stop using the default global timeout value.

So actually the https://curl.se/libcurl/c/CURLOPT_TIMEOUT_MS.html default is 0, so we should set the default comment out value in settings.php to 0 (this $settings['http_client_config']['timeout'] is eventually set to override Guzzle's $config[\CURLOPT_TIMEOUT_MS]:

$timeoutRequiresNoSignal = false;
if (isset($options['timeout'])) {
  $timeoutRequiresNoSignal |= $options['timeout'] < 1;
  $conf[\CURLOPT_TIMEOUT_MS] = $options['timeout'] * 1000;
}

The description in default.settings.php should probably also mention that changing this setting will impact all code that uses the http_client service without providing custom timeout explicitly.

Good point!

Believe you also have to update in sites/default.

Ah of course, thanks!

Tweaked CR slightly to include versions.

But tests are green and setting seems good.

This feels like it might be better as a container parameter than in settings. I never really know where is the best place to put these sorts of things.

This is going to affect way more that just oEmbed. See \Drupal\Core\Http\ClientFactory::fromOptions() - at the very least the docs need updating. But it's tricky because the default is not 5... the default in ombed is 5 but the default for other outbound requests is 30.

I think also this answers #21 - settings seems to be the place this is already configurable from.

Not sure what to do here.

Hmmm yes, too bad that ResourceFetcher does not follow the $settings['http_client_config'].

Perhaps we make this setting specific to oembed resource fetcher as its anyways only setting the timeout and is not allowing a wider array of setings like ClientFactory is.

$settings['oembed.resource_fetcher']['http_client_config']['timeout'] = 5;

Or just

$settings['oembed.resource_fetcher']['timeout'] = 5;

I think $settings['oembed.resource_fetcher']['timeout'] = 5; is okay. But here's where I think @longwave I'm with longwave. I think this should be a container parameter and injected into the media.oembed.resource_fetcher.

So something like:

parameters:
  media.resource_fetcher_timeout: 5
services:
// Other stuff
  media.oembed.resource_fetcher:
    class: Drupal\media\OEmbed\ResourceFetcher
    arguments: ['@http_client', '@media.oembed.provider_repository', '@cache.default', '%media.resource_fetcher_timeout%']

And then adjust the constructor to have protected int $timeout = 5 and use that in the code.

scott_euser → changed the visibility of the branch 3228350-timeout-setting to hidden.

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

Thanks, makes sense to me.

Probably just this small tweak to the unit test is fine given that parameters are a Symfony functionality that are already well used, so this will just ensure that that parameter is maintained in the future.

I did update the existing constructor to use promoted constructor arguments to match while in there, but happy to roll that back if you prefer.

In terms of test failure, not sure how core/modules/block/tests/src/Functional/BlockCacheTest.php is related? Perhaps its an issue from somewhere else?

Change record updated as well [#3362722]

The issue title and summary no longer match the changes made in the MR following the discoveries in this issue, they need updating to help reviewers understand the changes that are being made and why.

Thanks for flagging, good point! I have updated both, attempting to match the format of #3202145: oEmbed resource fetcher needs to set a reasonable connection timeout → for the title.

My link to the change record was wrong (I had linked to this issue!), here is the change record https://www.drupal.org/node/3362722 → .

Sorry missed saving my title change clearly - whoops!

Thanks! It's even easier to override than that, you can just add a services.yml alongside your settings.php (and include it from settings.php), and override any container parameter there - updated the IS a bit for this.

Seems new approach still has appropriate coverage

1) Drupal\Tests\media\Unit\ResourceFetcherTest::testFetchTimeout
TypeError: Double\GuzzleHttp\Client\P1::request(): Return value must be of type Psr\Http\Message\ResponseInterface, null returned
/builds/issue/drupal-3228350/core/modules/media/src/OEmbed/ResourceFetcher.php:67
/builds/issue/drupal-3228350/core/modules/media/tests/src/Unit/ResourceFetcherTest.php:52
ERRORS!
Tests: 2, Assertions: 5, Errors: 1.

believe the change is good and title makes sense.

Tweaked versions slightly but assume that will have to be updated if it makes it in 10.3 or 10.4

Committed/pushed to 11.x and cherry-picked to 10.4.x, thanks!

catch → closed merge request !8089

This change record need an update to at least mention the name of the parameter

Thanks for getting this in!

So I think this is the correct way to implement @longwave's comment in #33. Add the following to your services.yml or local services.yml (depending on your setup in settings.php/settings.local.php):

parameters:
  media.resource_fetcher_timeout: 10

If you already have a parameters section in your yml, you would just add this as a new line within that section.

  media.resource_fetcher_timeout: 10

The above example sets the research fetcher timeout to be 10 seconds instead of the default 5 seconds.

If anyone can confirm that I believe I have edit access to the change record. It would probably also be good to update the doc page here → to give an example of that as well.

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