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

refactor: enable instanceof and strictBooleans rector set #9339

Merged

Conversation

samsonasik
Copy link
Member

Description

  • flip negated instanceof
  • apply strict bool check

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@samsonasik samsonasik marked this pull request as draft December 27, 2024 01:58
@samsonasik samsonasik marked this pull request as ready for review December 27, 2024 02:20
@samsonasik
Copy link
Member Author

Ready to merge 👍

system/Cache/Handlers/FileHandler.php Outdated Show resolved Hide resolved
system/Config/DotEnv.php Outdated Show resolved Hide resolved
system/Database/BaseBuilder.php Outdated Show resolved Hide resolved
system/Database/MigrationRunner.php Outdated Show resolved Hide resolved
system/Database/MySQLi/Forge.php Outdated Show resolved Hide resolved
system/Validation/Rules.php Outdated Show resolved Hide resolved
system/Validation/Rules.php Outdated Show resolved Hide resolved
system/Validation/StrictRules/Rules.php Outdated Show resolved Hide resolved
system/Validation/StrictRules/Rules.php Outdated Show resolved Hide resolved
@samsonasik samsonasik force-pushed the refactor-enable-instance-strict branch from a709cbe to 638a0d3 Compare December 27, 2024 03:06
@samsonasik
Copy link
Member Author

samsonasik commented Dec 27, 2024

@paulbalandan review incorporated 👍 ,

for strpos() vs str_contains(), since it check with negation ! strpos(), which include 0 value, means 0 is found at position 0, it not changed to str_contains() to keep behaviour.

except it exactly compare to false will only changed to str_contains() to ensure it safe.

@michalsn
Copy link
Member

Hmm... wouldn't the str_contains() be more accurate? I know it changes the behavior a bit, but probably when the code was written no alternative for strpos() was available because of the minimal PHP version.

In this case, we're just checking if collate is already present in the $sql and, if not, adding it. Using strpos() with in_array() seems like complicating the code unnecessarily 🤔

@neznaika0
Copy link
Contributor

Aah, take a look first #9302
I've already fixed most of the cases.

@samsonasik
Copy link
Member Author

@neznaika0 your PR can be merged first if approved :)

Copy link
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

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

looking at the strpos scenarios i think @michalsn is right. before the code is just checking for the existence of the substring, so it doesn't matter if it is found at the beginning.

for the non-falsy string checks, i think those were originally meant for non-empty-string checks?

system/Database/Postgre/Connection.php Outdated Show resolved Hide resolved
system/HTTP/ResponseTrait.php Outdated Show resolved Hide resolved
system/Router/RouteCollection.php Outdated Show resolved Hide resolved
system/Session/Handlers/MemcachedHandler.php Outdated Show resolved Hide resolved
system/Test/CIUnitTestCase.php Outdated Show resolved Hide resolved
@samsonasik
Copy link
Member Author

All incorporated 👍

Copy link
Member

@michalsn michalsn left a comment

Choose a reason for hiding this comment

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

Thank you, looks good!

@samsonasik
Copy link
Member Author

samsonasik commented Dec 27, 2024

Feel free to merge PR #9302 from @neznaika0 first, I can rebase later ;)

@paulbalandan paulbalandan added the refactor Pull requests that refactor code label Dec 27, 2024
@paulbalandan
Copy link
Member

@samsonasik this can now be rebased

@samsonasik samsonasik force-pushed the refactor-enable-instance-strict branch from aa6f5ee to 095aea9 Compare December 28, 2024 14:43
@samsonasik
Copy link
Member Author

@paulbalandan rebased 👍

Copy link
Member

@michalsn michalsn left a comment

Choose a reason for hiding this comment

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

👍

@paulbalandan paulbalandan merged commit ee4ee98 into codeigniter4:develop Dec 28, 2024
41 checks passed
@paulbalandan
Copy link
Member

Thank you, @samsonasik

@samsonasik samsonasik deleted the refactor-enable-instance-strict branch December 28, 2024 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Pull requests that refactor code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants