WebAssert::addressEquals($page) should not remove query part

[anton-siardziuk]

I think this is not right that I can not write something like that

I should be on "/login?return_url=/user"

in my feature context

Actually I can write, but under the hood MinkContext will remove query part from url. Interesting that it will not remove fragment part: https://github.com/Behat/Mink/blob/master/src/Behat/Mink/WebAssert.php#L647

This is copy from Behat/MinkExtension#144

[aik099]

I guess this was some kind to normalize url to make sure that 2 urls with same file part and different url parameters are considered as same page.

[stof]

@everzet could you give more info about the reason why it was implemented this way ?

[anton-siardziuk]

I think the only normalization that should be applied there is remove "base_url" part of actual url. And ensure it starts from "/" after that.

[aik099]

Maybe also sorting parameters alphabetically and the rebuilding query string with http_build_query.

[anton-siardziuk]

In my opinion the less work will be done implicitly the best in this particular case.

[jurgenhaas]

I ran into a similar issue here which is even worse: for some reason I have to define the base_url in behat.yml as http://www.example.com/subsite/?language=en which seems to be working fine.

However, when I then do something like I should be on "/user"which then tries to go to http://www.example.com/subsite/?language=en/user which isn't working. The expected URL is that case is http://www.example.com/subsite/user?language=en

[aik099]

@jurgenhaas , what you're describing is a bug, where URL is built by appending all stuff to the end. Please create separate ticket and PR for it. In my QA-Tools library I'm actually storing parsed version of Base URL (via https://github.com/qa-tools/qa-tools/blob/develop/library/QATools/QATools/PageObject/Url/Parser.php class) and merge it with user provided components to construct new url.

[aik099]

@m00t , can you please provide a PR as well?

[anton-siardziuk]

@aik099, I am not sure I can do it in the near future, but I will try.

[jurgenhaas]

@aik099 I have fixed this and provided a PR in MinkExtension, see Behat/MinkExtension#173

[anton-siardziuk]

@jurgenhaas I believe this issue will not be fixed with Behat/MinkExtension#173

[jurgenhaas]

You're right, I was working on the issue mentioned 3 days ago where the base_url has a query but not the path. So, if this approach is going to be accepted, then the next step could be to address a potential query in the path as well.

[anton-siardziuk]

@jurgenhaas I am not sure, but I think our issues does not have anything in common.
BTW I started working on this issue, hopefully PR will be provided soon.

[aik099]

Good work. Don't forget to add tests to any code you write.

[anton-siardziuk]

@aik099, any chances it will be merged? Any feedback?

[aik099]

I've approved your work, but I was hoping @stof will agree with my assertion just to be on the safe side.

[everzet]

@stof I don't recall any meaningful reason. Feel free to change it.

[gquemener]

I guess it was because asserting that one is on /foo/bar while the url is actually /foo/bar?baz=1 is quite true, don't you think?

[gquemener]

Anyway, this is a BC break for people expecting the behaviour I described in my previous message IMO.

[everzet]

@gquemener thanks, Gildas. Yup, now I remember. It's because if you do compare with the query string, you'd get in very interesting situations. Like:

/foo/bar?bar=2&baz=1   !==   /foo/bar?baz=1&bar=2
/foo/bar               !==   /foo/bar?bar=1
/foo/bar?              !==   /foo/bar

With this in mind, I think you should keep addressEqual() and introduce another asserter for queries, that does include query string, but doesn't care about order of these.

[anton-siardziuk]

@gquemener I am not sure why this should be true. Query is part of url, why it should be ignored? If so, then we should revert this commit as well: Behat@38928ce, because mink cannot ignore query, but do not ignore hash

[everzet]

@m00t my thinking was that query is part of URL, but it is not necessary a part of the address - that depends on how you design your routes. Hence addressEqual() does not check query.

[anton-siardziuk]

@everzet so addressEqual() should ignore hashtag as well. I am fine with another asserter for queries, but current addressEquals() seems a little inconsistent to me.

[everzet]

@m00t from this point onwards for addressEquals() it's not about consistency, it's about backwards compatibility.

[anton-siardziuk]

@everzet on the other hand hashtag is part of address for some single page applications.

[anton-siardziuk]

@stof @everzet ok. Feel free to close this PR.

[anton-siardziuk]

@everzet @gquemener I thought about it again and now I disagree with you. BC break introduced in this PR easy to understand and easy to fix. You just update, see that test failed with "failed assert that /login?return=1 === /login?return=2" and fix it. I think there are much more users now actually write

   Then I should be on "/login?return=1"

and do not realize actually that "?return=1" will be ignored, than users who know it and really want to ignore query part.

[aik099]

Unfortunately if the url user will be redirected to will contain variable parameter (e.g. session_id), then you can't match it.

[anton-siardziuk]

@aik099 yeah, this is the problem. As a workaround you can use regexps like "^/login" for this situation. I am not sure if it is acceptable solution, but I think there are not many cases with variable parameters in urls.

[indeyets]

The proper solution is to use terminology and comparison algorithms from IRI spec

[gquemener]

Wow @indeyets, I didn't know about that!
Indeed, this is a proper solution.
Are you aware of an existing php library implementing this rfc?

[indeyets]

@gquemener ml/iri is good. // cc @lanthaler