Problem/Motivation

See #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase).

This will also involve converting cURL calls to Guzzle because cURL is not compatible with BTB.

\Drupal\history\tests\HistoryTest has a couple of curlExec calls, which are:

a) hard to read
b) not compatible with BTB

Proposed resolution

Move it to use guzzle directly.

Remaining tasks

User interface changes

API changes

Data model changes

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

dawehner created an issue. See original summary.

dawehner’s picture

legovaer’s picture

Status: Active » Needs work
FileSize
5.07 KB

I have been working on this issue as you can see in the attached patch. Please have a look at my progress so far. It's not yet finished by for some reason I keep getting a 403 when executing getNodeReadTimestamps().

Anything I'm missing here?

// Perform HTTP request.
    $url = Url::fromRoute('history.get_last_node_view')
      ->setAbsolute();
    $this->verbose($this->serializer->encode($post, 'json'));
    return \Drupal::httpClient()->post($url->toString(), [
      'body' => $this->serializer->encode($post, 'json'),
      'headers' => [
        'Content-Type' => 'application/x-www-form-urlencoded',
      ]
    ]);

Also: do we have other examples?

legovaer’s picture

FileSize
4.43 KB

Patch in #3 contained changes that weren't related to this issue.

borisson_’s picture

Status: Needs work » Needs review
FileSize
2.49 KB
4.46 KB

You created a protected $client but weren't using it in the code. So I changed that. No idea why the 403 is happening.

Status: Needs review » Needs work

The last submitted patch, 5: let_s-2795049-5.patch, failed testing.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Jo Fitzgerald’s picture

FileSize
1.15 KB
4.71 KB

The 403 is happening because the Guzzle request is from an unauthenticated (anon) user. After 5+ hours investigating this I'm not much closer to finding the solution, but at least we now know the problem!

I suspect the key is to pass cookies along with the body and headers (see patch), but I can't find a method to create a valid CookieJar. There's no point running this past the testbot - I can guarantee it will fail.

I think there is also an issue with the format of the node_ids being posted, because the call fails even when the user restrictions are removed from getNodeReadTimestamps().

Jo Fitzgerald’s picture

Title: Let's \Drupal\Tests\history\Functional\HistoryTest use guzzle » Convert web tests to browser tests for history module
Assigned: Unassigned » Jo Fitzgerald
Issue summary: View changes
Jo Fitzgerald’s picture

Assigned: Jo Fitzgerald » Unassigned
Status: Needs work » Needs review
FileSize
5.69 KB
5.91 KB
6.52 KB
  1. Converted from Web Test to Browser Test.
  2. Replace cUrl (not in Web Test Base) with Guzzle.
klausi’s picture

+++ b/core/modules/history/tests/src/Functional/HistoryTest.php
@@ -41,6 +53,14 @@ protected function setUp() {
+    $domain = parse_url($this->getUrl(), PHP_URL_HOST);
+    $session_id = $this->getSession()->getCookie($this->getSessionName());
+    $opts = [
+      'cookies' => CookieJar::fromArray([$this->getSessionName() => $session_id], $domain),
+      'http_errors' => FALSE,
+    ];
+    $this->client = \Drupal::service('http_client_factory')->fromOptions($opts);

I think this setup is not necessary. We already have a Guzzle client in the Mink driver we are using for browser tests. $this->getSession()->getDriver()->getClient()->getClient(); should give you the Guzzle client, right?

We really need an ApiTestBase for such cases, see #2812967: API test base: Provide a test helper to test REST and similar APIs.

Not sure what is better for this issue, instantiating a new Guzzle client or reusing the existing one from Mink.

Jo Fitzgerald’s picture

How about this then? It now uses the existing Guzzle client from Mink rather than instantiating a new one.

The cookies are the vital thing that means the Guzzle calls are successful. The could (probably) be obtained for each Guzzle request individually, but it is more efficient to do so in the set-up.

FYI some of the functionality is based on that in modules/system/tests/src/Functional/CsrfRequestHeaderTest.php.

dawehner’s picture

+++ b/core/modules/history/tests/src/Functional/HistoryTest.php
@@ -41,6 +60,11 @@ protected function setUp() {
+    $domain = parse_url($this->getUrl(), PHP_URL_HOST);
+    $session_id = $this->getSession()->getCookie($this->getSessionName());
+    $this->cookies = CookieJar::fromArray([$this->getSessionName() => $session_id], $domain);
+    $this->client = $this->getSession()->getDriver()->getClient()->getClient();

We could quickly add a todo pointing to this other issue.

boaloysius’s picture

The patch in #14 don't apply to https://www.drupal.org/project/drupal/releases/8.4.x-dev I downloaded today (7th march). I am uploading the updated patch.

boaloysius’s picture

I am doing my first reroll. In #14 i did it manually. In this I use git.

The last submitted patch, 14: 2795049-14.patch, failed testing.

dawehner’s picture

  1. +++ b/core/modules/history/tests/src/Functional/HistoryTest.php
    @@ -41,6 +60,12 @@ protected function setUp() {
    +
    +    $domain = parse_url($this->getUrl(), PHP_URL_HOST);
    +    $session_id = $this->getSession()->getCookie($this->getSessionName());
    +    $this->cookies = CookieJar::fromArray([$this->getSessionName() => $session_id], $domain);
    +    $this->client = $this->getSession()->getDriver()->getClient()->getClient();
       }
    

    Let's move this code onto a getHttpClient method.

  2. +++ b/core/modules/history/tests/src/Functional/HistoryTest.php
    @@ -49,34 +74,23 @@ protected function setUp() {
    +      'cookies' => $this->cookies,
    
    @@ -86,15 +100,17 @@ protected function getNodeReadTimestamps(array $node_ids) {
    +      'cookies' => $this->cookies,
    

    Are you sure you have to specify the cookies? The client should have the information already, at least according to your setup

  3. +++ b/core/modules/history/tests/src/Functional/HistoryTest.php
    @@ -49,34 +74,23 @@ protected function setUp() {
    +      'headers' => [
    +        'Accept' => 'application/json',
    +        'Content-Type' => 'application/x-www-form-urlencoded',
           ],
    

    Its nice how the code is a bit easier to read than the weird curl stuff.

Jo Fitzgerald’s picture

  1. Created getHttpClient() method.
  2. Yes, the cookies are definitely required - that is the crux of the issue. The http request has no innate knowledge of the session created in setUp() so we have to provide it. If you remove the cookies from the post request it will not work.

Also added the @see mentioned in #13.

dawehner’s picture

+++ b/core/modules/history/tests/src/Functional/HistoryTest.php
@@ -121,28 +132,42 @@ public function testHistory() {
+    // Similar code is also employed to test CSRF tokens.
+    // @see \Drupal\Tests\system\Functional\CsrfRequestHeaderTest::testRouteAccess()
+    $domain = parse_url($this->getUrl(), PHP_URL_HOST);
+    $session_id = $this->getSession()->getCookie($this->getSessionName());
+    $this->cookies = CookieJar::fromArray([$this->getSessionName() => $session_id], $domain);
+    return $this->getSession()->getDriver()->getClient()->getClient();

Did you tried using \Drupal::service('http_client_factory')->fromOptions(['cookies' => $this->cookies']); I could imagine that this would help.

Jo Fitzgerald’s picture

Yes, I used $this->client = \Drupal::service('http_client_factory')->fromOptions($opts); in #10, but @klausi advised using $this->getSession()->getDriver()->getClient()->getClient(); instead in #11.

klausi’s picture

Status: Needs review » Needs work

The cookies are ugly, I tried some variations myself with setting it on the Guzzle client but it also didn't work for me. We might find something better in the future, but for now this is already a great improvement and even makes the test code much more readable.

  1. +++ b/core/modules/history/tests/src/Functional/HistoryTest.php
    @@ -33,6 +38,20 @@ class HistoryTest extends WebTestBase {
    +  /**
    +   * The Guzzle HTTP client.
    +   *
    +   * @var \GuzzleHttp\Cookie\CookieJar;
    +   */
    +  protected $cookies;
    

    doc block is wrong, this is not he client. Should be "The cookie jar holding the testing session cookies for Guzzle requests."

  2. +++ b/core/modules/history/tests/src/Functional/HistoryTest.php
    @@ -121,28 +132,42 @@ public function testHistory() {
    +   * @return \GuzzleHttp\Client
    +   */
    

    Description missing, should be "The client with BrowserTestBase configuration."

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
10.5 KB
838 bytes
  1. Corrected the doc block.
  2. Added the description.
klausi’s picture

Looks good!

Uploading the same patch created with "git diff -M5%" to prove that the file has moved.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed fce944d to 8.4.x and af000d8 to 8.3.x. Thanks!

  • alexpott committed fce944d on 8.4.x
    Issue #2795049 by Jo Fitzgerald, boaloysius, legovaer, borisson_, klausi...

  • alexpott committed af000d8 on 8.3.x
    Issue #2795049 by Jo Fitzgerald, boaloysius, legovaer, borisson_, klausi...
boaloysius’s picture

Status: Fixed » Closed (fixed)

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

andypost’s picture

Follow-up to convert remaining test #2898437: Convert HistoryTimestampTest to kernel test