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

Objects with __toString() can be asserted. #760

Merged
merged 3 commits into from
May 7, 2019
Merged

Conversation

tamasd
Copy link
Contributor

@tamasd tamasd commented Mar 27, 2018

In a recent change, some regex matches changed to stripos() calls.
The problem is that stripos() converts the needle parameter to int
if it is not a string. While objects having __toString() can be
converted into string by most of the internal PHP functions, they
cannot be converted to int, resulting in an error.

I found this issue when running Drupal tests. Drupal passes objects to these assert functions (most notable TranslatableMarkup.

@stof
Copy link
Member

stof commented Mar 27, 2018

It converts it to int ? WTF. The function expects a string, not an int.

tests/WebAssertTest.php Outdated Show resolved Hide resolved
tests/helpers/Stringer.php Outdated Show resolved Hide resolved
tests/helpers/Stringer.php Outdated Show resolved Hide resolved
tests/helpers/Stringer.php Outdated Show resolved Hide resolved
tests/helpers/Stringer.php Outdated Show resolved Hide resolved
tests/helpers/Stringer.php Outdated Show resolved Hide resolved
@aik099
Copy link
Member

aik099 commented Mar 27, 2018

The responseHeaderContains method and others expect string as 2nd argument. Fact, that Drupal tests are passing object in there with expectation for it to be converted to a string by some magic surely violates proper function usage and therefore isn't supported use-case.

This was already discussed in #743 (comment), where preg_match was replaced by stripos.

@tamasd
Copy link
Contributor Author

tamasd commented Mar 27, 2018

@aik099 A little backstory on how I found this issue. I was developing tests for a Drupal module that we are developing internally. The test called traits from the Drupal core, and those traits asserted translated strings (which are objects). I looked around and it seems that the Drupal core does this in a lot of places.

Drupal core also depends on the master branch of mink for some reason. The easier fix would be to make Drupal depend on the last stable version, however that is a temporary fix, because a new mink version going to be released eventually.

I think adding a cast is much simplier than requiring bigger projects to go through bigger refactorings. And since many PHP functions convert object with __toString() to strings silently, it is not unreasonable to expect this behavior.

Also, PHP 7 does this conversion automatically in case when a function parameter is type hinted. See here: https://3v4l.org/vthrq

@tamasd tamasd force-pushed the master branch 2 times, most recently from 97f0d08 to dd19ac3 Compare May 3, 2019 09:45
tamasd added 2 commits May 3, 2019 11:47
In a recent change, some regex matches changed to stripos() calls.
The problem is that stripos() converts the needle parameter to int
if it is not a string. While objects having __toString() can be
converted into string by most of the internal PHP functions, they
cannot be converted to int, resulting in an error.
@mxr576
Copy link

mxr576 commented May 6, 2019

@aik099 Could you please take a look at this PR. I am not sure whether the failed test on Travis is actually a blocker of the merge or not.

@aik099
Copy link
Member

aik099 commented May 7, 2019

@mxr576 , checked. All issues have been resolved now.

Failing build is related to some Packagist/GitHub issue and can be ignored.

@mxr576
Copy link

mxr576 commented May 7, 2019

🆒, so when this will be merged?

@aik099 aik099 merged commit 153e4bb into minkphp:master May 7, 2019
@aik099
Copy link
Member

aik099 commented May 7, 2019

Sure, @mxr576. Merged now. Thanks @tamasd .

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.

4 participants