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

Replace is_null function calls with === null #35

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

tmewes
Copy link
Contributor

@tmewes tmewes commented Sep 30, 2024

Description

Currently, is_null and === null are used very inconsistently, even though they are equivalent and === null is a bit faster (see e.g. https://stackoverflow.com/a/8229005).
This PR replaces all is_null function calls in app with === null (and !is_null analogously with !== null).

Perhaps an additional check in a workflow would be good to ensure consistency so that only === null is used in the future.

The changes in this PR are just suggestions as usual. :)

Type of change

  • Refactor

@fballiano
Copy link
Contributor

Hi @tmewes! What's the rector rule for this? cause we have a rector workflow:

But I also see that this one breaks PHPStan for some reason

;-)

@tmewes
Copy link
Contributor Author

tmewes commented Sep 30, 2024

Unfortunately, I couldn't find a ready-made Rector rule on https://getrector.com/find-rule for that (only for Exakat - another PHP analyzer tool) and I don't know enough about Rector to be able to build it myself quickly. :/ Should I take a look at it and try to create an own rule for that (including testing and learning about Rector in general) or do you know more about it? Or should we do that later?

Yes, PHPStan has something to note about in three files, but as far as I can see it has rules for both is_null() and === null here. It's only checking the modified lines, right? Then these matches affect is_null() just as before. I only adjusted and standardized all occurrences in app in this PR without changing any behavior (at least that was the plan).
How do we proceed? I would actually rather not make any potential behavior changes to the content of this PR that could potentially cause side effects. If necessary, we could do this in other PRs and then, e.g. remove unnecessary places where null is checked in order to fix the PHPStan messages. What do you think?

Please excuse me for not being very familiar with these tools yet. I will change that in the medium term.

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.

2 participants