search

WordPress / WordPress-Coding-Standards

PHP_CodeSniffer rules (sniffs) to enforce WordPress coding conventions
MIT License
2.56k stars 487 forks source link

Core: Ban Yoda conditions, loose comparisons and assignments in conditions #1624

Closed jrfnl closed 1 year ago

jrfnl commented 5 years ago

Is your feature request related to a problem?

Yoda conditions are the practice to reverse how comparisons in conditions are written, having the value being compared to on the left hand of the condition and the variable on the right hand, to prevent accidentally assigning something to a variable in a condition.

// This condition:
if ( $variable === 'value' ) {}

// ...would be written like this in Yoda style:
if ( 'value' === $variable ) {}

// ... to prevent this:
if ( $variable = 'value' ) {}

The problem with this is that code readability is decreased significantly to prevent a beginner's mistake which shouldn't be made in the first place and the chances of which are small when using strict comparisons anyhow.

Now, the WordPress coding standards contains a few sniffs related to this:

Note: none of these sniffs currently contain (auto-)fixers.

Describe the solution you'd like

At this moment, I'd like to propose the following:

Impact

Once these changes have been made to WPCS and Core has upgraded, contributors can be encouraged to start looking at the newly detected issues and to fix up the existing (non-strict) comparisons. This should be done carefully as changing the type of comparison is a behavioural change. It is recommended to make sure that all changes are covered by unit tests before making this type of change.

Next phase

As a next step, I'd like to propose the handbook to be adjusted to forbid Yoda conditions.

A new sniff will need to be written to detect the use of Yoda Conditions, throw an error and where possible, auto-fix this.

Alternatively it could be considered to add the Slevomat Coding Standard as a dependency to WPCS and use the existing sniff in that standard. The downsides of that would be:

Additional context (optional)

Some discussion about this has been had in other channels. I'm quoting some relevant parts of that discussion here.

@jrfnl Regarding Yoda conditions - we've talked about this before. IMO this rule should be dropped in favour of the "No assignment in condition" rule and the associated sniff WordPress/Generic.CodeAnalysis.AssignmentInCondition which is currently in Extra should be moved to Core.

At the time you asked me to investigate whether there is a sniff available out there which could revert existing Yoda conditions. The answer to that is "Yes". The external Slevomat Coding Standard contains a sniff which can auto-fix Yoda to non-Yoda. Tests could (should) be run to see how well it works, but this sniff could be used in a one-time action to move away from Yoda. Note: if the sniff would not be good enough/would turn out to be buggy, I'm happy to contribute fixes to the Slevomat standard just to make moving away from Yoda possible.

Having said that, as the tooling is available which would allow us to move away from Yoda and still prevent assignments in conditions, I would strongly suggest now is a good time to take a decision on this.

Source: https://core.trac.wordpress.org/ticket/45934#comment:14

@pento Yah, I agree that WordPress/Generic.CodeAnalysis.AssignmentInCondition should be moved into Core, violations should be fixed, and then Yoda conditions should be removed. There's some discussion on #42885 about Yoda conditions, too.

Notably, the JS team are planning on removing guidance on Yoda conditions, I'm inclined to disallow Yoda conditions, rather than simply allow either style, though.

Source: https://core.trac.wordpress.org/ticket/45934#comment:15

Opinions ?

Luc45 commented 2 years ago

These were never included in the Yoda condition rules.

Ah, you're right, thanks for the clarification. I have verified this in multiple ways and that statement does hold true.

Yoda and Numeric Comparison verification As expected, Yoda rules does not enforce any kind of inverted order for numeric comparisons, in multiple scenarios as tested below. None of these scenarios are flagged by Yoda rules: `/vendor/bin/phpcs foo.php --standard=WordPress --sniffs=WordPress.PHP.YodaConditions` ```php 20 ) { // no-op } // Numeric comparison with variable - Inverted if ( $target <= $count ) { // no-op } // Numeric comparison with variable if ( $count > $target ) { // no-op } // Numeric comparison with callable - Inverted if ( $getTarget() <= $count ) { // no-op } // Numeric comparison with callable if ( $count > $getTarget() ) { // no-op } // Numeric comparison with callable (variation) - Inverted if ( $target <= $getCount() ) { // no-op } // Numeric comparison with callable (variation) if ( $getCount() > $target ) { // no-op } // Numeric comparison with callable (variation 2) - Inverted if ( $getTarget() <= $getCount() ) { // no-op } // Numeric comparison with callable (variation 2) if ( $getCount() > $getTarget() ) { // no-op } ```

Even though the enforcement is not there at a technical level, this popped up on a code review for me on a codebase that has Yoda enforced, I have asked the developer if he had struggled to write that line as much as I did to read it and he vented with me that he had struggled a lot. I will inform him that he does not need to invert numeric comparisons.

Now, the most important: Does anyone oppose @jrfnl proposal? I would like to pledge that we refrain all further replies to this lengthy thread to the resolution of this issue, so that we can get to a conclusion.

jrfnl commented 2 years ago

As this has been a long-standing issue with widely opposing views and lots of people with firm opinions involved, I believe a post on Make to announce our intention to change this is needed to move this forward.

Volunteers to write the post very welcome. I'm happy to review it before publication.

In my opinion, these are the action steps needed to get this sorted (in order):

2ndkauboy commented 2 years ago

@Luc45 I don't oppose with the initial idea of @jrfnl to not enforce the Yoda condition any more (although I personally prefer it). I would just ask us not to "ban", disallow it, as some other people recommended in the comments. Basically what @jrfnl mentioned in this comment: https://github.com/WordPress/WordPress-Coding-Standards/issues/1624#issuecomment-470821000

Luc45 commented 2 years ago

@Luc45 I don't oppose with the initial idea of @jrfnl to not enforce the Yoda condition any more (although I personally prefer it). I would just ask us not to "ban", disallow it, as some other people recommended in the comments. Basically what @jrfnl mentioned in this comment: https://github.com/WordPress/WordPress-Coding-Standards/issues/1624#issuecomment-470821000

I understand, and this is indeed a very delicate topic. As banning Yoda might seem as unfair to some as enforcing is to others.

Considering both sides, and trying to be fair, I am considering an equation like this: 

$ecosystem_score = $developer_happiness + $codebase_consistency;

By enforcing Yoda, we can, with the data we have, infer that it decreases $developer_happiness. (data: count of "yoda vs regular" and reactions in this proposal)

// Considering the proportion of Yoda vs Regular conditionals count in the WP.org plugin ecosystem as $developer_happiness:
// Yoda: 20% of conditionals found in large-scale sample
// Regular: 80% of conditionals found in large-scale sample

$ecosystem_score = 0.2 + 1; // 1.2 Enforcing Yoda
$ecosystem_score = 0.8 + 1; // 1.8 Banning Yoda
$ecosystem_score = 1 + ?; // ? Allowing Both

The critical point is that if we don't ban Yoda, we will never have enough incentive to get rid of it, thus decreasing $codebase_consistency over time, leading to a lower $ecosystem_score, which also brings down $developer_happiness.

Considering what is best for the ecosystem with the best of my knowledge, I believe that we should pick a side. If someone is passionate about Yoda, they can always disable the PHPCS sniff or even enforce Yoda in their projects, or simply lock the standard to ^2.

Volunteers to write the post very welcome.

Your proposal in this thread is already a very nice starting point to a post. I am sorry for not being able to help on that, as I'm going through one of those "crunch times" that happens every time you have a deadline and lots of work to do, which is a feeling I think most software developers know very well.

Someone...? :disguised_face:

Luc45 commented 2 years ago

@Luc45 I don't oppose with the initial idea of @jrfnl to not enforce the Yoda condition any more (although I personally prefer it). I would just ask us not to "ban", disallow it, as some other people recommended in the comments. Basically what @jrfnl mentioned in this comment: https://github.com/WordPress/WordPress-Coding-Standards/issues/1624#issuecomment-470821000

I think it's fine for us to have different opinions. As a collective we are always making choices, and divergence is a natural part of this process. Our sum of choices over time defines the success or failure of the collective enterprise, so it's a big responsibility to have opinions and defend them, which is only worthy as long as we still believe in what we are saying.

With that in mind, and aware that this divergence exist, I find myself in a crossroad: We can either write the Make post and bring further discussions there, as a definitive, formal place. Or only write the Make post once we have reached a consensus on whether to ban yoda or to be indifferent about it.

I think the concept of a "Consensus" in a collective can actually be harmful to strive for. As to achieve consensus on every decision, we might often need to make suboptimal concessions for immediate happiness that are not healthy to the collective as a whole, like politics.

With that said, and taking upon the responsibility of keeping this thread on track so that we can keep a level of efficiency towards the best outcome for the issue at hand, I ask:

@2ndkauboy (and others), why should we allow Yoda in WordPress Coding Standards ^3? Don't you think it would decrease the codebase consistency, which is one of the main goals of a code style convention?

2ndkauboy commented 2 years ago

Why we should allow it? That's easy to answer: because we have enforced it in the past. Now banning it would make the vote more inconsistent, because we would need to change it on all occurrence, this would be a huge change. And we also have to get it out of the muscle memory of every WordPress developer, which might be just as hard 😁

dingo-d commented 2 years ago

Why we should allow it? That's easy to answer: because we have enforced it in the past. Now banning it would make the vote more inconsistent, because we would need to change it on all occurrence, this would be a huge change. And we also have to get it out of the muscle memory of every WordPress developer, which might be just as hard 😁

I don't think this is a good argument. Just because something was used in the past, doesn't mean we cannot change it ever again 😉 With that thinking we should never update the minimum PHP version for core from PHP 5.6 (and we'd all love to see that happen 😂).

I'm not for banning, but I'm not for enforcing it. After the make post is created and the consensus is achieved to remove it, we can just remove it from WPCS.

What shouldn't happen if the Yoda is removed as a requirement, is tons of core commits just changing the Yoda code. That wouldn't serve any purpose IMO (and I think there is a rule somewhere for contributing that useless changes shouldn't happen for the sake of happening).

2ndkauboy commented 2 years ago

@dingo-d this was the question from @Luc45:

@2ndkauboy (and others), why should we allow Yoda in WordPress Coding Standards ^3? Don't you think it would decrease the codebase consistency, which is one of the main goals of a code style convention?

He is arguing that allowing the Coda condition would decrease the codebase consistency. But the opposite is true. WordPress Core is using the Yoda condition, and now banning it would create an inconsistency. It would also break many pending patches to core with a Yoda condition in place.

As for the broader WordPress ecosystem, I did some searches using https://wpdirectory.net/ and found the following:

I was also quite surprised with these numbers, by they mean that (more than) double the number of plugins can be found that use the Yoda condition than plugins using a non-Yoda condition (sure a plugin might be found in both searches).

It seems that the broader WordPress ecosystem is using the Yoda condition ;)

Luc45 commented 2 years ago

For the sake of efficiency, let's focus on making this the final discussion before the Make post. A) To ban Yoda. B) To be indifferent about Yoda.

I prefer A, but I understand your argument in regards to "time to adapt".

With that said, I still think A is the best path moving forward, given a few criteria are met:

1: Codebases that need more time can always stick to WPCS ^2 for a while.

2: Simple instructions on how to automatically swap Yoda for regular conditions on existing codebases are documented.

Swapping Yoda for Regular on a codebase should ideally be a one-liner, something like:

./vendor/bin/phpcbf src --standard=WordPress --sniffs=WordPress.PHP.YodaConditions

(Considering Slevomat Coding Standard is added as a dependency of WPCS ^3 as proposed in the original thread)

In a large codebase with many open PRs branched off before the coding standard has been updated, this is one approach to avoid merge conflicts with open PRs:

  1. Create a branch with only the version bump:

    • git checkout trunk
    • git branch -b wpcs-3
    • composer require wp-coding-standards/wpcs ^3 --dev
    • git add . && git commit -m "WPCS ^3" && git push

  2. Create another branch for trunk fixed by PHPCBF:

    • git checkout trunk
    • git branch -b wpcs-phpcbf-3
    • git merge wpcs-3
    • ./vendor/bin/phpcbf src --standard=WordPress --sniffs=WordPress.PHP.YodaConditions
    • git add . && git commit -m "PHPCBF WPCS ^3 && git push

  3. Foreach PR:

    • git checkout my-pr
    • git merge wpcs-3
    • ./vendor/bin/phpcbf src --standard=WordPress --sniffs=WordPress.PHP.YodaConditions
    • git add . && git commit -m "PHPCBF WPCS ^3" && git push

Now trunk and all open PRs have updated their codebases to regular conditions without any merge conflicts or manual labor.

With that said, this is what I personally defend to keep the codebase consistent between new and existing code. These are my arguments, and if we agree to disagree, that's fine. I am proud that we are able to have a rational discussion about Coding Standads at all, instead of going against each other with pitchforks and torches. This is the sign of a strong community, where everyone is comfortable coming forward and defend their opinion without being torn apart for it.

I'm looking forward for the conclusion and a hero(ine) that will write that Make post, be it as it may.

dingo-d commented 2 years ago

@2ndkauboy

It would also break many pending patches to core with a Yoda condition in place.

Not it wouldn't break anything if it was removed. No sniff is checking the absence of Yoda, only its enforcement. So code written with Yoda style or without it, if you don't include Yoda won't report: Missing Yoda condition this statement is 😂

Also, the argument that a lot of plugins/themes use Yoda or not, is not a concern for WordPress Core. A lot of plugins follow PSR-12 (or 2), but that doesn't mean we'd need to enforce curly braces on the separate line in the WordPress Core.

jrfnl commented 2 years ago

It seems that the broader WordPress ecosystem is using the Yoda condition ;)

Please see the (much more extensive) analysis of the plugins directory done by @Luc45 which proves the opposite: https://github.com/WordPress/WordPress-Coding-Standards/issues/1624#issuecomment-1042435954

2ndkauboy commented 2 years ago

@dingo-d I'm talking about any patch on Trac/GitHub waiting to be merged with changes including a Yoda condition.

As far as we care about the WordPress Core: it uses only Yoda conditions. Plugins can and will use whatever syntax they want to use.

2ndkauboy commented 2 years ago

@jrfnl thanks for pointing me to that comment. I haven't recognized it really.

I did a quick check on a fresh copy of WordPress 5.9.3 if I exclude all external dependencies/libraries, I get the following numbers:

PHP CODE SNIFFER VIOLATION SOURCE SUMMARY
--------------------------------------------------------------------------------------------
    STANDARD  CATEGORY            SNIFF                                                COUNT
--------------------------------------------------------------------------------------------
[x] Slevomat  Control structures  Disallow yoda comparison disallowed yoda comparison  4621
[x] Slevomat  Control structures  Require yoda comparison required yoda comparison     302
[ ] WordPres  PHP                 Yoda conditions not yoda                             9
--------------------------------------------------------------------------------------------

Those 9 WordPress.PHP.YodaConditions.NotYoda violations come from the three "(ms-)deprecated.php" files. But what I also realized is that the SlevomatCodingStandard.ControlStructures.RequireYodaComparison would also flag a comparsion of a function to a string (like strpos( $permalink_structure, '%year%' ) !== false) but WordPress.PHP.YodaConditions.NotYoda does not flag that as a violation.

The summary also shows the number of violations, not the number of plugins, so we can't really tell from that summary how many plugins do use Yoda conditions and how many don't.

If we care about WordPress Core only, the Core is exclusively using Yoda conditions (except for those 9 violations in deprecated functions). If the number of violations of SlevomatCodingStandard.ControlStructures.DisallowYodaComparison is correct though, this would mean that Core would need a huge commit of up to 4621 changed lines.

So I would strongly suggest to go with your initial approach of just removing the sniff and going with B) To be indifferent about Yoda as @Luc45 mentioned, or even keeping the sniff and move the other ones to the Core ruleset.

The bigger issues are WordPress.CodeAnalysis.AssignmentInCondition, WordPress.PHP.StrictComparisons and WordPress.PHP.StrictInArray, but not whether we want to ban the Yoda condition from WordPress Core.

dingo-d commented 2 years ago

I've written a announcement on the core blog, it's still in draft mode.

@jrfnl @GaryJones I'd love to get your feedback on it when you'll find the time.

dingo-d commented 2 years ago

The article is live: https://make.wordpress.org/core/2022/06/14/upcoming-disallow-assignments-in-conditions-and-remove-the-yoda-condition-requirement-for-php/

jrfnl commented 2 years ago

Please participate in this unscientific poll on Yoda conditions: https://twitter.com/jrf_nl/status/1537015003546542080

dingo-d commented 1 year ago

The loose comparisons and assignments in conditions checks are in the core ruleset, but despite the overwhelming argument for removing Yoda conditions by the community, it was decided to leave those in the core ruleset.

If in the future we choose to revisit the Yoda conditions, we'll open a new issue.