WordPress.org

Make WordPress Core

Opened 7 months ago

Last modified 5 weeks ago

#42885 new defect (bug)

Fix PHPCS "Yoda Condition checks" for root files of WordPress Core.

Reported by: chetan200891 Owned by:
Milestone: 5.0 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch 2nd-opinion
Focuses: coding-standards Cc:

Description

In this ticket I am adding patch to fix PHPCS "Yoda Condition checks" errors for WordPress root files.

Attachments (1)

42885.diff (7.3 KB) - added by chetan200891 7 months ago.
Created patch.

Download all attachments as: .zip

Change History (11)

@chetan200891
7 months ago

Created patch.

#1 @netweb
7 months ago

  • Keywords has-patch commit added
  • Milestone changed from Awaiting Review to 5.0

Perfect @chetan200891, thanks for the patch 👍🏼

In 42885.diff

Coding Standards: Fix Yoda conditions in /src/*.php files.

  • Fixes all instances of the PHPCS/WPCS WordPress.PHP.YodaConditions sniff for the/src/*.php files.

#2 @jdgrimes
7 months ago

Before committing this, maybe a good time to reconsider this rule? Now that we can run PHPCS over the code, we could just disallow assignments in conditions, and then Yoda conditions wouldn't really be necessary. As I understand it, the main reason for the Yoda conditions rule is to avoid accidentally using an assignment in a condition when a comparison was intended. Disallowing assignments in conditions would make that impossible to begin with.

I know that some developers find Yoda conditions more difficult to grok than others. @jrf has brought this up before, for example. I don't have a strong opinion either way, but I thought it was worth mentioning. The rule, a bit antiquated, seems.

Perhaps this should really be discussed elsewhere though. I'll bring it up in Slack.

This ticket was mentioned in Slack in #core-coding-standards by jdgrimes. View the logs.


7 months ago

#4 @GaryJ
7 months ago

We don't know how many instances of assignments in conditions there are that would need to be fixed.

Why not:

  • merge the current patch, if it reduces violations of the current rules - lets at least get consistency with something, even if that is later dropped.
  • fix up assignments in conditions (it should generally make code more readable anyway).
  • turn assignment in conditions check on either at the core phpcs.xml.dist, or WordPress-Core + Handbook or both).
  • discuss the relevance/redundancy of Yoda conditions now the other work is done.

Personally, I don't have trouble reading A == B or B == A, but if we're forcing harder readability for some, for the sake of avoiding possible bugs, maybe that's not a good thing. IDEs can usually do assignment in condition inspections, even if not part of the PHPCS ruleset.

Maybe some devs prefer Yoda conditions + having assignments in conditions?

Maybe a handy separate repo would have a sniff that could detect conditions written as Yoda, so devs could run it to find all the places where they may want to change back to not-Yoda (if the Yoda rule is dropped from the Handbook)?

Until we get to that stage though, I see no reason this shouldn't be committed.

#5 @pento
7 months ago

  • Keywords 2nd-opinion added; commit removed

I would be curious to hear about arguments in favour of allowing assignments in conditions.

Ultimately, the aim is to have phpcs run on every commit. If we don't allow assignments in conditions, then the reason for Yoda conditions (avoiding accidental assignment in conditions) should no longer be necessary.

#6 @SergeyBiryukov
5 months ago

#43404 was marked as a duplicate.

#7 @netweb
5 months ago

Please ignore this comment

Last edited 8 weeks ago by netweb (previous) (diff)

#8 @jipmoors
3 months ago

Ultimately, the aim is to have phpcs run on every commit. If we don't allow assignments in conditions, then the reason for Yoda conditions (avoiding accidental assignment in conditions) should no longer be necessary.

I disagree that it's the only reason of having that sniff.

Having consistency on how the condition is formatted is a gain that this sniff provides. Making sure the developer does not have to think about the way it should be read is a big gain.

#9 @afercia
2 months ago

#44227 was marked as a duplicate.

#10 @netweb
5 weeks ago

#44028 was marked as a duplicate.

Note: See TracTickets for help on using tickets.