Problem

  1. For outbound HTTP requests performed in a test, a request needs to use the "simpletest" User-Agent.

  2. #1447736: Adopt Guzzle library to replace drupal_http_request() converted the legacy code of drupal_http_request() into an event subscriber for the http_default_client (Guzzle) service.

  3. However, this event subscriber is added always — even though it is only needed when a test is executed.

Proposed solution

  1. Add a service provider for tests, which only adds the request event subscriber service definition and method call when a test is executed.

API changes

  • None.
CommentFileSizeAuthor
#31 2168809-31.patch3.34 KBdamiankloip
#26 test.services.26.patch3.35 KBsun
#20 interdiff.txt666 bytessun
#20 test.services.20.patch3.35 KBsun
#16 interdiff.txt1.4 KBsun
#16 test.services.16.patch3.34 KBsun
#10 interdiff.txt2.08 KBsun
#10 test.services.10.patch3.91 KBsun
#8 interdiff.txt4.16 KBsun
#8 test.services.8.patch3.75 KBsun
#6 2168809-6.patch2.82 KBdamiankloip
#2 2168809-2.patch3.48 KBdamiankloip
d8.remove-http_client_simpletest_subscriber.patch3.54 KBdamiankloip
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, d8.remove-http_client_simpletest_subscriber.patch, failed testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs work » Needs review
FileSize
2168809-2.patch3.48 KB

Let's move the subscriber outside of simpletest module. Haven't looked at the failures yet, they seem to all be coming from when cron is invoked from a test.

Status: Needs review » Needs work

The last submitted patch, 2: 2168809-2.patch, failed testing.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented

#1447736: Adopt Guzzle library to replace drupal_http_request() added a http_client_simpletest_subscriber service to the container. This adds the service as an event subscriber to the http_default_client (Guzzle client) service. This is only for testing purposes, so let's make it so.

I assume this is the passthrough of the web test user-agent. It definitely belongs in the standard container, as it belongs in the site-under-test (which is an unaltered Drupal site), not in the test runner.

We could have a submodule of simpletest that we only enable in the site-under-test. But at this point, it's probably easier not to fix what's not broken.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

It definitely belongs in the standard container, as it belongs in the site-under-test (which is an unaltered Drupal site), not in the test runner.

We are altering things for the test environment anyway. So that is not really true.

it's probably easier not to fix what's not broken.

It works, yes. But it's not right having a test specific subscriber for all http requests made just because it's easier to leave it in the standard container.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
FileSize
2168809-6.patch2.82 KB
damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs work » Needs review
sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
test.services.8.patch3.75 KB
interdiff.txt4.16 KB

As outlined more in-depth in #2165549: Reduce number of password hashing iterations in all tests to improve test performance, I think it makes more sense to do this instead:

Introduce dedicated Test component and conditionally register services/event-subscribers.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented 
  1. +++ b/core/lib/Drupal/Core/CoreServiceProvider.php
@@ -53,6 +53,7 @@ public function register(ContainerBuilder $container) {
+    $this->registerTest($container);

@@ -170,4 +171,20 @@ public static function registerUuid(ContainerBuilder $container) {
+  protected function registerTest(ContainerBuilder $container) {

    I am good with this approach. My main quibble is that we don't want crap like this registered in the container all the time. 

  2. +++ b/core/lib/Drupal/Core/CoreServiceProvider.php
index a8de4a8..3acaf0c 100644
--- a/core/lib/Drupal/Core/Http/Plugin/SimpletestHttpRequestSubscriber.php

--- a/core/lib/Drupal/Core/Http/Plugin/SimpletestHttpRequestSubscriber.php
+++ b/core/lib/Drupal/Core/Test/EventSubscriber/TestHttpRequestSubscriber.php

    Yes! Although if we are already in the Test namespace can we just call the subscriber HttpRequestSubscriber?

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Title: Remove the http_client_simpletest_subscriber service from core.services.yml » Do not add the Guzzle HTTP client request subscriber outside of test environment
Component: base system » simpletest.module
Assigned: Unassigned » sun
Issue summary: View changes
Issue tags: +API clean-up, +Testing system
Related issues: +#2210637: Upgrade to Guzzle 4
FileSize
test.services.10.patch3.91 KB
interdiff.txt2.08 KB

Renamed to HttpRequestSubscriber.

Adjusted issue title + summary accordingly.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Issue summary: View changes
Related issues: +#2165549: Reduce number of password hashing iterations in all tests to improve test performance
damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

This might as well wait for Guzzle 4 now? This patch will not work with Guzzle 4.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

After reviewing and studying that patch, it doesn't really look like these two patches will conflict in serious ways with each other.

That said, the Guzzle4 upgrade seems to hard-code this class as a before-send event emitter — other service providers are no longer able to adjust/extend the http client service through the service definition...? :-/

Perhaps I shouldn't have RTBC'd just yet... OTOH, it's going to be much easier to revise the new code once the main upgrade changes + Guzzle4 is in core.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

Agreed, we need to just get Guzzle 4 in before we can really decide what we want to do IMO.

Guzzle 3 had an addSubscriber proxy method on the client to register things, Guzzle 4 does not have this unfortunately, hence the hardcoded call in the constructor.

After reviewing and studying that patch, it doesn't really look like these two patches will conflict in serious ways with each other.

So I think that is not true, as we will no longer be able to just add a method call like that anymore :/

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Yeah, let's discuss that in #2237681: Revise Guzzle client configuration and event registration :-)

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
test.services.16.patch3.34 KB
interdiff.txt1.4 KB

Updated for new http_client_subscriber compiler pass.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Great. This is the same patch as before, just adapted for Guzzle 4.

Once this in, we can move forward with #2165549: Reduce number of password hashing iterations in all tests to improve test performance + look into further clean-ups and optimizations.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented 
  1. +++ b/core/lib/Drupal/Core/CoreServiceProvider.php
@@ -57,6 +57,7 @@ public function register(ContainerBuilder $container) {
+    $this->registerTest($container);

    It really would be nicer if we only added this pass when in a test env (like earlier patches were doing) but understand this is a good compromise. Still calling this subscriber code but not registering this guzzle subscriber all the time. 

  2. +++ b/core/lib/Drupal/Core/CoreServiceProvider.php
@@ -157,4 +158,18 @@ public static function registerUuid(ContainerBuilder $container) {
+      ->register('http_client.test.subscriber', 'Drupal\Core\Test\EventSubscriber\HttpRequestSubscriber')

    Minor, but I think the service namespacing should be the other way round. i.e. 'http_client.subscriber.test' ? 

  3. +++ b/core/lib/Drupal/Core/Test/EventSubscriber/HttpRequestSubscriber.php
@@ -2,26 +2,25 @@
+class HttpRequestSubscriber implements SubscriberInterface {

    Better use of the namespace. Removing Simpletest from the class name is a good idea IMO.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
  1. registerTest() contains the check itself and early-returns. I see where you're coming from, though the counter-argument would be that the code is encapsulated in a single spot instead of multiple places. I could go either way, don't really have a preference.
  2. Hm. If we're concerned about namespacing, then I'd actually say that it's not correctly namespaced at all right now. ;) The originating component is Test, not Http\Client, so more correct would be: test.http_client.request_subscriber
  3. Yup.
sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
test.services.20.patch3.35 KB
interdiff.txt666 bytes

Renamed service ID to test.http_client.request_subscriber.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

20: test.services.20.patch queued for re-testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs review » Reviewed & tested by the community

This seems fair to me. Let's do it!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 20: test.services.20.patch, failed testing.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented

20: test.services.20.patch queued for re-testing.

The last submitted patch, 20: test.services.20.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Reviewed & tested by the community
FileSize
test.services.26.patch3.35 KB

Merged 8.x.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 26: test.services.26.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review

26: test.services.26.patch queued for re-testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs review » Reviewed & tested by the community

and again.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 26: test.services.26.patch, failed testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs work » Needs review
FileSize
2168809-31.patch3.34 KB

Rerolled.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 31: 2168809-31.patch, failed testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs work » Needs review

31: 2168809-31.patch queued for re-testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Not doing this outside of tests seems sensible.

However I can't remember, at all, why we need to do this in tests either? This is only for requests from the tested site during a test run which we should not be doing much if at all.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

We are doing it, the most obvious example is aggregator test which create feeds based on nodes, and if we run cron as a request or test the UI, that needs to go back to the test environment.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

The two prime examples are Aggregator tests (in which the child site performs outbound HTTP requests to itself) and SimpleTest's very own functional tests (in which the test runs a test in a child-child-site).

It's fine to remove it from the production container, but it should be enabled by default whenever a kernel is built in a test environment.

  • Commit 2b2b97d on 8.x by catch: 
    Issue #2168809 by sun, damiankloip: Do not add the Guzzle HTTP client...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

OK I think I get this just about.

I think it'd be better if we added a custom header and checked that, rather than messing with the user agent. I can see a test relying on the standard Drupal user agent, which would then not be here. However this issue isn't introducing that problem.

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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