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

Broken equality comparisons #17

Closed
autowp opened this issue Mar 11, 2020 · 14 comments
Closed

Broken equality comparisons #17

autowp opened this issue Mar 11, 2020 · 14 comments
Milestone

Comments

@autowp
Copy link
Contributor

autowp commented Mar 11, 2020

v2.0.0rc1

phpcbf breaks equality comparasions.

Reproducing.

Source code:

<?php

var_dump(1 == '1');

Test:

$php test.php 
bool(true)

Code cleaning:

./vendor/bin/phpcbf test.php
PHPCBF RESULT SUMMARY
----------------------------------------------------------------------
FILE                                                  FIXED  REMAINING
----------------------------------------------------------------------
/home/dvp/eclipse-workspace/autowp/test.php           1      1
----------------------------------------------------------------------
A TOTAL OF 1 ERROR WERE FIXED IN 1 FILE
----------------------------------------------------------------------

Clean source code:

<?php

var_dump(1 === '1');

Test again

$php test.php 
bool(false)

Expected: bool(true)

@geerteltink
Copy link
Member

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');

@geerteltink geerteltink added the Invalid This doesn't seem right label Mar 12, 2020
@geerteltink
Copy link
Member

On a side note, you can also disable the rules you don't want.

@froschdesign
Copy link
Member

@geerteltink

We enforce strict comparison where possible.

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.
Or did I misunderstand something?

@Xerkus
Copy link
Member

Xerkus commented Mar 12, 2020

@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.

@Xerkus Xerkus reopened this Mar 12, 2020
@autowp
Copy link
Contributor Author

autowp commented Mar 12, 2020

Sure, i think phpcs should detect that issue, but phpcbf must not try to autoresolve

@geerteltink
Copy link
Member

geerteltink commented Mar 13, 2020

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.

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.

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.

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.

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. declare(strict_types=1);. If you add this it changes behavior of the code. I'm pretty sure you can find more rules like that, which change behavior.

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?

@froschdesign
Copy link
Member

froschdesign commented Mar 13, 2020

@geerteltink

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.

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.
Unfortunately I missed this connection at first.

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
The hint may be banal, but if you don't deal with it every day it is crucial.

@geerteltink
Copy link
Member

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.

@autowp
Copy link
Contributor Author

autowp commented Mar 13, 2020

The only way to fix it and comply with that rule is to change the loose comparison to strict comparison

In most cases before replace you need check expression manually, e.g.

var_dump(1 == '1');
var_dump(1 === (int) '1');

There are more rules that change code behavior. e.g. declare(strict_types=1);

That rule doesn't fix automatically

fixer only runs when the sniffer was successfully completed

Preffered order is: clean legacy code where it's possible automatically witch cs-fix, then clean other issues manually with cs-check help

@geerteltink
Copy link
Member

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.

@autowp
Copy link
Contributor Author

autowp commented May 7, 2020

So, maybe Warning that using the tool may break the application?

@geerteltink
Copy link
Member

The documentation is now online. You can read it here: https://docs.laminas.dev/laminas-coding-standard/v2/coding-style-guide/#6-operators

Loose comparison operators SHOULD NOT be used, use strict comparison operators instead. e.g. use === instead of ==.

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 phpcs it gives you warnings about which files and which lines that are not valid according to the ruleset. If you run phpcs -s you can even see which sniffs are violated and it shows you which can be autofixed with running phpcbf.

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

@weierophinney
Copy link
Member

@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 phpcs, we'll get a warning or error, and the user will then need to manually:

  • Fix the issue by introducing a strict comparison, OR
  • Place a phpcs:ignore directive prior to the comparison.

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.

@weierophinney weierophinney reopened this Jun 4, 2020
@froschdesign froschdesign removed the Invalid This doesn't seem right label Jun 4, 2020
@geerteltink geerteltink added this to the 2.1.0 milestone Sep 7, 2020
@geerteltink
Copy link
Member

Fixed in #18

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

No branches or pull requests

5 participants