WebAssert::addressEquals() and AssertLegacyTrait::assertUrl() fail to check the querystring

Fixing failed test

Fixing a one character nit in a comment, but otherwise the patch looks good so marking RTBC.

Does this need a change record? Contrib/custom tests might be unexpectedly relying on the existing broken behaviour.

Let's add a change record, but otherwise looks good.

This feels like a bug in Mink too - so can we open an issue in their tracker for them as a courtesy?

This is failing cspell because of the word `scriptalertxsssubjectscript` so we need to add that to the exception list I think

Already discussed at length upstream in https://github.com/minkphp/Mink/pull/566 which, if I read it correctly, was closed without fix due to backward compatibility concerns.

https://github.com/minkphp/Mink/issues/656 is also open with no response.

Added cspell ignore comment.

Fixed on commit - cspell++, larowlan-- for not reading the output properly the first time

diff --git a/core/modules/system/tests/src/Functional/Form/ConfirmFormTest.php b/core/modules/system/tests/src/Functional/Form/ConfirmFormTest.php
index ce114f83eb..2a0e059098 100644
--- a/core/modules/system/tests/src/Functional/Form/ConfirmFormTest.php
+++ b/core/modules/system/tests/src/Functional/Form/ConfirmFormTest.php
@@ -49,7 +49,7 @@ public function testConfirmForm() {
     // Test cancelling the form with a complex destination.
     $this->drupalGet('form-test/confirm-form-array-path');
     $this->clickLink(t('ConfirmFormArrayPathTestForm::getCancelText().'));
-    // Verfiy that the form's complex cancel link was followed.
+    // Verify that the form's complex cancel link was followed.
     $this->assertUrl('form-test/confirm-form?destination=admin/config');
   }

Committed 1e15ed9 and pushed to 9.1.x. Thanks!

Because there's a risk that this will disrupt contrib testing, I think this is not eligible for backporting. Happy to have the discussion if folks think there is a case for it - if so please re-open with reasoning.

Published change record

Is there a way to ignore the query string? I have some tests that were broken by this - these tests were ignoring the query string because it is just a csrf token and I don't need or want to check that in every test. I have one test to ensure it's present, but in my functional tests I really want to be just testing the functionality, not the validity of the token every time.

Is there a way to ignore the query string?

I've used this in the past

$this->assertStringContainsString('the/expected/bit', $this->getSession()->getCurrentUrl());

This change is not quite correct and rather than fixing things making test writing more difficult.

• It's breaking contrib in new ways. See https://www.drupal.org/pift-ci-job/1873184 for example.
• Asserting against a real destination on DrupalCI is hard because DrupalCI runs in a sub-directory. See #2957953-99: Editing menus user-experience has regressed for example

I think this change when in the wrong direction - \Behat\Mink\WebAssert::cleanUrl() removes query strings for good reason. Query strings are highly dynamic and tricky to assert on.

Patch attached partially reverts #16 so contrib testing is not broken.

Revert this means we're still not checking query strings correctly - so that does need to be fixed. Then I think we need a better plan with how to test query strings if present and how to cope with destination. The only reason that

    // Verify that the form's complex cancel link was followed.
    $this->assertSession()->addressEquals('form-test/confirm-form?destination=admin/config');

in the original patch is because \Drupal\form_test\ConfirmFormArrayPathTestForm::getCancelUrl() does not construct the destination correctly... it just hard codes 'destination' => 'admin/config', - which is wrong.

Whatever solution we come up with we need to massively improve the CR for the deprecation triggered when assertUrl() is called with more than one argument...

    if (func_num_args() > 1) {
      @trigger_error('Calling AssertLegacyTrait::assertUrl() with more than one argument is deprecated in drupal:8.2.0 and the method is removed from drupal:10.0.0. Use $this->assertSession()->addressEquals() instead. See https://www.drupal.org/node/3129738', E_USER_DEPRECATED);
    }

https://www.drupal.org/node/3129738 is not at all helpful.

Let's get the revert in and then figure out what to do to move this forward.

Could we only apply the strict rule if the expected URL contains a question mark?

@longwave that's a start - but the destination param with DrupalCI's sub-directory is PITA. See #2957953-99: Editing menus user-experience has regressed - it's too easy to have a test that passes locally and fails on CI - or vice versa. Other people have trod on this ground before... https://github.com/minkphp/Mink/pull/566

So as discussed in Slack we likely need to special case the destination querystring to handle the base URL change, and perhaps also fix legacy assertUrl() to accept a second argument containing the querystring, which Simpletest allowed but was never converted fully to WebTestBase.

I've confirmed that PathAutoUiTest is fixed by the revert.

Agreed with reverting. Will commit once this comes back green.

Committed/pushed to 9.2.x and cherry-picked to 9.1.x, thanks!

Moving back to needs work.

I've unpublished the CR. We've lived with the false positives here for years. Make something critical to get attention feels wrong. But I'm not going to play issue status games. I feel this should be left at major like the original bug because it is just the original bug.

Re-rolled.

I found 3 more files that need changes:
- modules/system/tests/src/Functional/Routing/RouterTest.php
- modules/search/tests/src/Functional/SearchLanguageTest.php
- modules/block/tests/src/Functional/BlockTest.php

@mondrake: The changes you made look good.

One nitpick.

The patch is for me RTBC.
Only the IS and the CR need an update.

Added a few minor comments. Do we need to do anything special for the destination query string, as @alexpott alluded to earlier in this issue?

The testbot is not happy.

Looks like one of the TestBot's incarantions HDs is full, already encountered this in another issue and asked around in Slack (https://drupal.slack.com/archives/C51GNJG91/p1621922820016900), added this one also.

FWIW: On my incarnation simply ordering a retest made my issue go away (probably because I've got a different TestBot than before).
Ordered a re-test here as well.

No it's something more evil, like a contributor not testing before posting a patch. Horrible!

Oh noes! That never ever happens! 😈

All the changes look good to me.
As far as I can see, we got all instances that need the new method.
There is a CR for the new method.
For me it is RTBC.

The only reason I am not setting the status to RTBC, is because the patch from the 9.2 MR does not apply to 9.3.

Oh my, I can't get MR working for me. Reverting to g'old patch.

And who was the person who said that working with MR's was so much better then working with those patches? :grinning:

@mondrake I do not know what you did with the MR. I have tried compare the MR, that was for me RTBC, to the new patch. And I could not do it.

I had saved the MR as a patchfile on my local machine and all the changes are the same. Therefore this is RTBC for me.

I wonder if we should push on the upstream project again. I think if we include a query string in addressEquals then it should check it. It's far too easy to write an assertion and think you've got it right - as shown by our code base.

I think if we want to implement a workaround in core we should do something like only asserting on query strings if the assertion contains a query string. So if you do $this->assertSession()->addressEquals('admin/structure/block/list/classy?block-placement=' . Html::getClass($block['id'])); it will test the query string but if you do $this->assertSession()->addressEquals('admin/structure/block/list/classy'); it won't. That way you preserve BC but you break when the unexpected thing is happening.

+++ b/core/tests/Drupal/Tests/WebAssert.php
@@ -767,6 +787,28 @@ public function addressNotEquals($page) {
+    if (func_num_args() > 1) {
+      throw new \ArgumentCountError('Called ' . __METHOD__ . ' with more than one argument');
+    }

Do we need this given it is a new method?

1. +++ b/core/modules/forum/tests/src/Functional/ForumIndexTest.php
   @@ -54,7 +54,8 @@ public function testForumIndexStatus() {
   +    $this->assertSession()->urlQuerystringEquals("?forum_id=$tid");
   

   Nit: Querystring -> QueryString

2. +++ b/core/tests/Drupal/FunctionalTests/WebAssertTest.php
   @@ -54,6 +56,37 @@ public function testResponseHeaderDoesNotExist() {
   +    $this->assertSession()->urlQueryStringEquals('');
   

   Why does this pass? What is it testing?

I don't think we need more methods - I think once we've implemented it for us we should try again upstream - because if you include a query then I think it should either error or check it... and checking it seems better.

addressEquals() only takes strings - not sure how this worked before, as you can't cast a Url to string?

Also, should we be strict with the query strings here, or not? ie. if we expect ?a=1 should ?a=1&b=2 be considered a pass, because that would be OK from most web applications' point of view?

Whoops, didn't see our overridden cleanUrl() - I think we should just use that.

+++ b/core/tests/Drupal/Tests/WebAssert.php
@@ -754,6 +771,18 @@ public function addressEquals($page) {
+    $page = $this->cleanUrl($page);
+    if (strpos($page, '?') !== FALSE) {

$this->cleanUrl($page) is gonna remove the query from $page so the if will never evaluate to true... we need to write a test that is expected to fail :)

Argh yes. Let's try again, splitting cleanUrl() in two and adding some tests.

+++ b/core/tests/Drupal/Tests/WebAssert.php
@@ -59,7 +74,23 @@ protected function cleanUrl($url) {
+    parse_str($query, $parameters);

This is tricky. This futzes with stuff.

I'd approach this a bit differently. I think we should maintain the same Behat\Mink exception and do something very similar as the parent but in the case that $page includes a query then it should be checked too. Something like the patch attached. Both could do with quite a bit of documentation. Another concern is that you can't assert that a page has no query parameters (like HEAD) but I'm not too sure that in practise that matters. Also this fix is pretty much what I think the upstream fix should look like.

Fixing the test fails...

@mondrake I think we should reduce the futzing with the comparison not add. However, if we do want to do that we could consider using \GuzzleHttp\Psr7\UriNormalizer as this has all the tools to do url comparison... however reading the docs there:

    /**
     * Sort query parameters with their values in alphabetical order.
     *
     * However, the order of parameters in a URI may be significant (this is not defined by the standard).
     * So this normalization is not safe and may change the semantics of the URI.
     *
     * Example: ?lang=en&article=fred → ?article=fred&lang=en
     *
     * Note: The sorting is neither locale nor Unicode aware (the URI query does not get decoded at all) as the
     * purpose is to be able to compare URIs in a reproducible way, not to have the params sorted perfectly.
     */
    const SORT_QUERY_PARAMETERS = 128;

I think "IMHO these two URLs are equivalent (=equal); the order of the keys is not relevant." is an assumption that is 99% true but I'd bet there is a contrib / custom module out there where this is not correct.

I also think that evaluating 'test-page?a=b&c=d' to be equal to 'test-page?c=d&a=b' is not in the spirit of the principle of least surprise (also the current behaviour of addressEquals and addressNotEquals also goes against this)

I had the same concern as #94 but #95 makes sense and we can always revisit this later to make it less strict if it turns out we need that for some reason.

Another concern is that you can't assert that a page has no query parameters

This is what this assertion was meant to do in the test?

    $this->assertSession()->addressEquals('test-page?');

Anyway, RTBC +1

Let's try this again :)

Committed/pushed to 9.3.x, thanks!