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

[6.x] Fix Str@contains for PHP 8.0.1 #35915

Closed
wants to merge 4 commits into from
Closed

[6.x] Fix Str@contains for PHP 8.0.1 #35915

wants to merge 4 commits into from

Conversation

rodrigopedra
Copy link
Contributor

Fix issue described in #35891.

The issue only happens when running PHP >= 8.0.1. if the $haystack parameter when calling mb_strpos is not a string, PHP will throw and error.

After fixing it one test started failing. I then fixed Illuminate\Database\Query\Builder@where to explicitly cast the $haystack parameter to a string when calling Str@contains.

It was failing on the last assertion of the Illuminate\Tests\Database\DatabaseQueryBuilderTest@testMySqlWrappingJsonWithBoolean test.

Issue was, besides the Builder@where docblock accept \Closure|string|array the assertion was passing a Illuminate\Database\Query\Expression object as the column name on ->where() call.

I didn't fix the docblock for the Builder@where on this PR, as I think it should be fixed in a separate PR.

One alternative approach is to always cast the $haystack to a string inside the Str@contains implementation.

I prefer the implementation I sent in this PR (return false if not a string) instead, as it conforms with the method current dockblock and with the "Principle of least astonishment".

But I can update the PR if you prefer the alternative implementation.

src/Illuminate/Support/Str.php Outdated Show resolved Hide resolved
tests/Support/SupportStrTest.php Outdated Show resolved Hide resolved
tests/Support/SupportStrTest.php Outdated Show resolved Hide resolved
Copy link
Member

@GrahamCampbell GrahamCampbell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for Taylor: This PR fixes crashes on PHP 8.0.1 because the mb_strpos function now actually wants a genuine string as input, however Illuminate\Database\Query\Builder@where invokes this with an object with a __toString. I think this is likely a regression that should be fixed upstream, but until then (or in the case they don't want to fix it, because they actually want this new behaviour), we need to work around it.

@GrahamCampbell
Copy link
Member

GrahamCampbell commented Jan 17, 2021

@rodrigopedra Can you raise this change at bugs.php.net as a bug. It seems odd this is the only function with that change, and laravel is not calling the function with strict types enabled, so casting should take place at the call site already.

@rodrigopedra
Copy link
Contributor Author

Thanks for the review @GrahamCampbell . Will open the bug for PHP tomorrow.

@rodrigopedra
Copy link
Contributor Author

Hey @GrahamCampbell ,

Before sending the bug to PHP I installed and tested it on PHP 8.0.1, and it actually works:

https://3v4l.org/bh9aV

Even when $haystack is null.

Maybe who opened #35891 have strict types enabled, or the issue is somewhere else.

I am closing this, as by reverting the unneeded cast, the only change would be one test assertion of a Stringable object, which most of the other tests don't use (it is only used to test the Str@is method).

Thanks for your and @crynobone 's time, and please apologize for sending this based only on that issue description.

@rodrigopedra
Copy link
Contributor Author

If both of you think it is worth, I can send a new PR just adding assertions to test Stringable instances for all the tests in that test class.

@rodrigopedra rodrigopedra deleted the str-contains branch January 17, 2021 09:05
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