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.57%. Comparing base (9b08f62) to head (9b07001).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##             master     #883   +/-   ##
=========================================
  Coverage     98.56%   98.57%           
- Complexity      389      391    +2     
=========================================
  Files            24       24           
  Lines           909      911    +2     
=========================================
+ Hits            896      898    +2     
  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.