Keep the query string when comparing the current address

[Amoifr]

Fixes #656

cleanUrl() now keeps the query string, the same way #357 taught it to keep the fragment back in 2013.

Two things came out of the homework you asked for in the issue, @aik099.

Why it was implemented this way: it never was a decision. In #566 you wrote "I guess this was some kind to normalize url", @stof asked @everzet for the reason, and the answer never came. So there is no intent to preserve here.

The behaviour was locked in by the test suite. Before writing a single new test, exactly one test failed: testAddressEquals already used http://example.com/script.php/sub/url?param=true#webapp/nav and asserted that addressEquals('/sub/url#webapp/nav') passes. It now expects the query string. I added two tests built on the /login?return_url=/user example from the original report, one per direction.

I deliberately did not normalize the query string. You floated sorting the parameters and rebuilding with http_build_query() in 2014, and @anton-siardziuk answered that the less is done implicitly the better; your 2022 reply agreed with "about the same fix for query string", so a literal treatment like the fragment is what this does. Say the word if you want the normalization too.

The part that is your call. This changes assertion results: a suite that wrote addressEquals('/login') while sitting on /login?x=1 used to pass and will now fail. cleanUrl() is protected, so subclasses are affected as well. CONTRIBUTING.md says to branch from master, so that is where this is, but retarget it to 2-architecture-changes if you would rather not ship it in a minor.

522 tests green, PHPStan clean. CHANGES.md untouched.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.59%. Comparing base (9b08f62) to head (ea22735).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #883      +/-   ##
============================================
+ Coverage     98.56%   98.59%   +0.02%     
- Complexity      389      396       +7     
============================================
  Files            24       24              
  Lines           909      924      +15     
============================================
+ Hits            896      911      +15     
  Misses           13       13              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

[stof]

Suites doing assertions might have legitimate use cases for both behaviors. So maybe we need to make this change opt-in instead of always preserving the query string (potentially breaking existing suites that work since years).

And the name of the protected method getCurrentUrlPath is not correct anymore if this returns the also the query string.

[Amoifr]

Good point on the BC risk, done. WebAssert::__construct() now takes a second argument:

public function __construct(Session $session, bool $compareQueryString = false)

cleanUrl() only appends the query when it is on, so the default behaviour is byte for byte what it was. The pre-existing testAddressEquals went back to its original expectation, and the two new tests build a WebAssert with the flag enabled.

Two shape decisions I made on my own, happy to change either:

• I used a plain bool rather than a constants class like the NamedSelectorMode we are adding in Allow the named selector to skip the partial fallback #882, since the option only has two states. If you would rather keep the two consistent, that is a small change.
• $compareQueryString is private, not protected like $session, so a subclass overriding cleanUrl() cannot read it. I went with the smaller API surface, but tell me if you would rather expose it.

On getCurrentUrlPath(): you are right that the name would become wrong, but I did not rename it, and I would like your call before I do. It is protected, so renaming it breaks any extension that subclasses WebAssert, and there are a fair few. With the flag off by default the name also stays accurate for everyone who does not opt in. The options I see are to rename it to getCurrentAddress() and keep getCurrentUrlPath() as a deprecated proxy, or to leave it alone and treat the name as describing the default. Which do you prefer?

522 tests green, PHPStan clean.

[stof]

I think this should be a parameter of the assertion method, not of the constructor. The same project might have different needs for assertions in different places

[Amoifr]

Done, the flag is on the assertion methods now and the constructor is back to __construct(Session $session).

$assert->addressEquals('/login?return_url=/user', true);
$assert->addressNotEquals('/login?return_url=/admin', true);
$assert->addressMatches('/return_url/', true);

I gave it to addressMatches() as well so the three address assertions behave the same way; happy to drop it there if you would rather keep that one path-only.

How it is plumbed, since that decides what happens to the two protected extension points:

• cleanUrl() and getCurrentUrlPath() keep their exact signatures. Adding an optional argument to either would have been a fatal error for any subclass overriding them, which is not a price worth paying for an opt-in.
• With the flag off, an assertion still goes through getCurrentUrlPath(), byte for byte the old behaviour, subclass overrides included.
• With the flag on, a private helper calls cleanUrl() and splices the query back in front of the fragment. An override of cleanUrl() therefore still applies; an override of getCurrentUrlPath() does not, since by contract it cannot return a query string. Nothing existing is affected, because opting in is new.

That also settles your naming point: getCurrentUrlPath() never returns a query string, so the name stays accurate and nothing needs deprecating.

The one signature change left is the three public assertion methods, so a subclass overriding addressEquals() would have to add the parameter. Tell me if that bothers you and I will look at another shape.

525 tests green, PHPStan clean.