Clean up guzzle declaration in core tests

As asked on the other issue: Why do we actually have a custom timeout in the first place? Maybe some git blame will give us some idea about it.

Hi,

I have updated listed files:-
core/modules/statistics/src/Tests/StatisticsAdminTest.php
core/modules/statistics/src/Tests/StatisticsLoggingTest.php

Not find these line in two rest files.

adding new patch

added new patch

HI,
@mallezie removed unrelated changes from files. Added new patch

thanks

thanks! @mallezie removed and added new patch.

Agreed on the removal of the timeout value. It was added in #1209532-109: Count node views via AJAX in the statistics module, but that issue did not contain any reasoning why that value was chosen, so falling back to the default timeout value seems fine.

So the only change left is to remove the Guzzle-use-statements. Tagging as novice.

Removing use Guzzle statements from patch #18

So wait, why are we not fixing all 4 instances which exists according to the issue summary, but rather just 2 of them?

@dawehner I think is because comment #3

@faline
Well I checked earlier and they seemed to be there. Do you mind checking again?

@dawehner Sure! I take a look and you are right
in both there are this code (probably this occurs because they are looking for $this->client and in these files there is only $client).

I will update the patch with this two files.

@dawehner I founded one more in /core/modules/statistics/src/Tests/Views/IntegrationTest.php.
Should I change that too?

This patch fixing all 4 instances according to the issue summary

In addition to the 4 mentioned, this patch also correcting the file /core/modules/statistics/src/Tests/Views/IntegrationTest.php.

Thank you @faciline
Great findings!

Updating the scope based on the inclusion of the Views test. Nice work finding the additional usage!

Really great work on this issue, everyone. It's definitely helpful to see that we checked why this was introduced in the first place and also looked for other similar issues in core. Those are two of the things I check almost every time I review an RTBC patch!

I agree with removing the unneeded overrride for the timeout and letting it fall back to the default; 10 vs. 30 is not worth hardcoding a magic number.

There are two other places in core that the http_client_factory service is used. One is in locale.batch.inc:

    $response = \Drupal::service('http_client_factory')->fromOptions(['allow_redirects' => [
      'on_redirect' => function(RequestInterface $request, ResponseInterface $response, UriInterface $request_uri) use (&$actual_uri) {
        $actual_uri = (string) $request_uri;
      }
    ]])->head($uri);

And then in Aggregator, the service is actually injected in core/modules/aggregator/src/Plugin/aggregator/fetcher/DefaultFetcher.php:

      $response = $this->httpClientFactory->fromOptions(['allow_redirects' => [
        'on_redirect' => function(RequestInterface $request, ResponseInterface $response, UriInterface $uri) use (&$actual_uri) {
          $actual_uri = (string) $uri;
        }
      ]])->send($request);

Those are not the same usecase, so I agree that the scope for this issue makes sense as is.

Committed and pushed to both 8.3.x and 8.2.x. (Since it is only changing tests and there is no disruption, this makes sense to backport to keep the branches more in sync). Thanks!