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

Stop using == '' with regard to PHP 8 #2294

Merged
merged 8 commits into from
Sep 21, 2020
Merged

Conversation

leofeyer
Copy link
Member

@leofeyer leofeyer added the bug label Sep 14, 2020
@leofeyer leofeyer added this to the 4.9 milestone Sep 14, 2020
@leofeyer leofeyer self-assigned this Sep 14, 2020
@leofeyer leofeyer marked this pull request as ready for review September 14, 2020 16:56
@leofeyer leofeyer requested a review from a team September 14, 2020 16:57
@m-vo
Copy link
Member

m-vo commented Sep 14, 2020

For reference, this is the RFC: https://wiki.php.net/rfc/string_to_number_comparison

If I get this correctly many of the comparisons in this PR are actually comparing strings where == imo is still correct (even for '0'). I don't think we should transform them into another representation with type juggling involved but rather make them strict or leave them unchanged.

What about case and in_array (without 3rd parameter) statements that also use non-strict comparisons?

@ausi
Copy link
Member

ausi commented Sep 14, 2020

many of the comparisons in this PR are actually comparing strings

Most of these checks are boolen checks like empty(). I think they are more correct in most cases because an empty array() and the string '0' is mostly considered to be false in our codebase I think.

ausi
ausi previously approved these changes Sep 14, 2020
@m-vo
Copy link
Member

m-vo commented Sep 15, 2020

Well I'm thinking about all the "boolean" values or numbers with an empty state from the db that we get via dbal/models. Things like $objComments->addReply (→ '1', '') or $objParent->start (→ '', '0', '123', ...) that are actually strings and are only interpreted as boolean/integer. They either won't contain 0 (int) or 0 would mean something different.

I'd rather see them written like this:

$objComments->addReply === '1'
$objComments->addReply === ''

$objParent->start !== '' && $objParent->start > $time

@ausi
Copy link
Member

ausi commented Sep 15, 2020

What if someone sets $objComments->addReply = true; before the comparison? The model can come from the registry so this is possible in many cases I think.

@m-vo
Copy link
Member

m-vo commented Sep 15, 2020

Ah, you got a point there.

@m-vo
Copy link
Member

m-vo commented Sep 15, 2020

What about in_array? https://3v4l.org/TqmZG

Do we have any relevant cases of this as well in the codebase?

@leofeyer leofeyer requested a review from ausi September 15, 2020 12:34
@leofeyer
Copy link
Member Author

We have one in_array(0, case, which however does not cause problems. To be sure, we have still adjusted it in 984fc19.

@leofeyer leofeyer merged commit cb8645a into contao:4.9 Sep 21, 2020
@leofeyer leofeyer deleted the fix/php8 branch September 21, 2020 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants