Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixing issue when url is / #627

Closed
wants to merge 1 commit into from
Closed

fixing issue when url is / #627

wants to merge 1 commit into from

Conversation

benjamin-hubert
Copy link

No description provided.

@aik099
Copy link
Member

aik099 commented Dec 16, 2014

This PR corresponds to be #624 issue.

@aik099
Copy link
Member

aik099 commented Dec 16, 2014

Related PR with query parameter not handled by same cleanUrl method: #566

@stof
Copy link
Member

stof commented Dec 17, 2014

This needs to be covered by a test

@aik099
Copy link
Member

aik099 commented Dec 17, 2014

I guess test, that asserts that waning/notice is no longer happens when url is /. Since PHPUnit converts all warnings/notices to exceptions (not sure if it's a default behavior and that we haven't explicitly disabled it for Mink test suite) we can easily put @expectedException annotation to catch them.

@stof
Copy link
Member

stof commented Dec 17, 2014

@aik099 actually not using @expectedException, because we don't expect an exception.
What the test should do is doing an assertion with such an URL. And the fact that there is a notice in master would make the test error out.

@aik099
Copy link
Member

aik099 commented Dec 17, 2014

OK. @stof have taken a look at associated issue that also changes cleanUrl method, which already have tests: #566 ? Can it be merged?

@stof
Copy link
Member

stof commented Dec 19, 2014

Well, this bugfix is still not covered by tests to avoid regressions

@stof stof modified the milestone: 1.7 Jan 8, 2015
@stof stof closed this in 3a29eda Jan 8, 2015
@stof
Copy link
Member

stof commented Jan 8, 2015

I merged #616 instead as it was first and I preferred the way the code was formatted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants