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: fix phpstan only boolean allowed #9302

Merged

Conversation

neznaika0
Copy link
Contributor

Description
Fixed all errors like

Only booleans are allowed in ...

I tried to apply the typing adequately during the checks. For a more accurate fix, you need to update the code instead of phpdoc.
It is possible to additionally check for null, false, <empty-string> somewhere
Fixed 92 errors out of 3925

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

system/CLI/CLI.php Outdated Show resolved Hide resolved
system/CLI/CLI.php Outdated Show resolved Hide resolved
@github-actions github-actions bot added the stale Pull requests with conflicts label Dec 11, 2024
Copy link

👋 Hi, @neznaika0!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

@neznaika0 neznaika0 force-pushed the fix-phpstan-only-boolean-allowed branch from f6bb767 to 37a924d Compare December 11, 2024 17:54
@neznaika0 neznaika0 force-pushed the fix-phpstan-only-boolean-allowed branch 2 times, most recently from 0bf76f5 to f3a85de Compare December 11, 2024 19:32
@neznaika0 neznaika0 force-pushed the fix-phpstan-only-boolean-allowed branch from f3a85de to c31d553 Compare December 26, 2024 19:40
@paulbalandan paulbalandan removed the stale Pull requests with conflicts label Dec 27, 2024
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.

Why is null typed into a string so many times? In many cases it is not necessary. I was just looking at the code, I don't know if phpstan forces such typing? In many places it is very strange.

system/CLI/CLI.php Outdated Show resolved Hide resolved
system/CLI/CLI.php Show resolved Hide resolved
system/CLI/CLI.php Show resolved Hide resolved
system/CLI/CLI.php Show resolved Hide resolved
system/CLI/CLI.php Show resolved Hide resolved
system/Common.php Outdated Show resolved Hide resolved
system/Common.php Outdated Show resolved Hide resolved
system/Database/MigrationRunner.php Outdated Show resolved Hide resolved
system/Database/MigrationRunner.php Outdated Show resolved Hide resolved
tests/system/Email/EmailTest.php Outdated Show resolved Hide resolved
@neznaika0
Copy link
Contributor Author

See comment @paulbalandan

I think this should also include check against ''.

Short code for check null or empty string. Empty string is incorrect to pass in the parameter.

@michalsn
Copy link
Member

@neznaika0 It would be easier if you could respond under the code you're referring to.

Okay, for CLI::print() and CLI::write() it indeed makes sense, but I kindly disagree in other cases.

@neznaika0
Copy link
Contributor Author

If the value can be null, then why can't it be checked for emptiness as ""? I've made changes where an empty line is also invalid.
So there are cases ?string $value
What other explanation is needed?

@michalsn
Copy link
Member

This is not a general conversation about null and empty strings. If you want to justify the disputed change, do it under the code I commented on. It will make our lives easier.

@neznaika0 neznaika0 force-pushed the fix-phpstan-only-boolean-allowed branch from c31d553 to 0b85e5c Compare December 27, 2024 19:05
@neznaika0 neznaika0 force-pushed the fix-phpstan-only-boolean-allowed branch from 0b85e5c to 26f3fb6 Compare December 28, 2024 08:57
@neznaika0
Copy link
Contributor Author

@michalsn revert comparison with null

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!

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

Thank you, @neznaika0

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