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
Broken equality comparisons #17
Comments
Thank you for reporting this and testing the coding standard. However this is expected behavior. We enforce strict comparison where possible. In your example an integer is not equal to a string, that's why it fails. I'm not sure what your usecase is but this might be a solution: var_dump(1 === (int) '1'); |
On a side note, you can also disable the rules you don't want. |
But should this be rewritten / fixed by the script? Throwing an error in this case is correct but an automatic correction only leads to mistakes. I think this issue report is valid. |
@geerteltink I think it is more of a case where this should remain as warnings that have to be fixed manually as they change code behavior. |
Sure, i think phpcs should detect that issue, but phpcbf must not try to autoresolve |
The current ci phpcs behavior for all our components is to fail on warnings and errors. Unless we change this behavior, changing this rule to a warning does not solve anything. This tool is not being used to check rules and throw a notification or warning message out there which everyone ignores. This tool is there to make sure the code that is merged remains clean and consistent conform our standards.
phpcs does not change code automatically. It checks the rules we think should apply to our code base. You can fix the warnings and errors it generates manually or manually trigger the autofix function.
First of all, how do you want to fix this manually? The only way to fix it and comply with that rule is to change the loose comparison to strict comparison. You can do a dry run to see what will be changed or you can try to autofix the issues with phpcs and look at git diff to see what changed before committing. There are more rules that change code behavior. e.g. I remember we had the same conversation about replacing caught Exceptions with Throwables. That had valid use cases where you need to catch Exceptions only and it was removed. Having said all that, the question is what do we want? Do we want strict comparison in our code base or not? |
To clarify: It should warn and fail on this problem – this should and must be so! I see what you were getting at: the tools "Coding Sniffer" and "Beautifier and Fixer" must run one after the other and the fixer only runs when the sniffer was successfully completed. Therefore this request is invald! It would be good if this was noted in the instruction: https://github.com/laminas/laminas-coding-standard/tree/2.0.0rc1#usage |
To clarify... The fixer is never running automatically. It's only triggered manually as an aid for the developer to save time. If the coding sniffer is passing, phpunit is running after it. If the coding sniffer is failing, phpunit doesn't even run. |
In most cases before replace you need check expression manually, e.g. var_dump(1 == '1');
var_dump(1 === (int) '1');
That rule doesn't fix automatically
Preffered order is: clean legacy code where it's possible automatically witch cs-fix, then clean other issues manually with cs-check help |
As stated before we want to enforce strict comparison in our components. There will always be some rules that don't fit in other projects as these rules are very opinionated. You can disable the rules that you don't like. |
So, maybe Warning that using the tool may break the application? |
The documentation is now online. You can read it here: https://docs.laminas.dev/laminas-coding-standard/v2/coding-style-guide/#6-operators
We enforce this in our components together with more rules which we think should be applied to make sure our coding style is the same across all packages. If you run If you don't like some of the sniffs that are being used you can disable them as you like: https://github.com/squizlabs/PHP_CodeSniffer/wiki/Annotated-Ruleset |
@geerteltink I think you're missing the problem here. Nobody is arguing that the tool shouldn't flag non-strict comparisons. They're arguing that invoking phpcbf should not change them. This way, on running
This approach ensures that running phpcbf doesn't break code that worked by changing behavior, and forces the developer to either figure out how to do a strict comparison, or flag the non-strict usage via a comment, indicating it's intended behavior. Fixers should fix style issues, not change behavior. Changing from non-strict to strict comparison is a behavior change. |
Fixed in #18 |
v2.0.0rc1
phpcbf breaks equality comparasions.
Reproducing.
Source code:
Test:
Code cleaning:
Clean source code:
Test again
Expected: bool(true)
The text was updated successfully, but these errors were encountered: