Problem/Motivation

Yoda conditions are very hard to read, and require getting into an (IMO bad) habit to write.

We already recommend strict comparison where it makes sense, which prevents the problem of accidental assignment anyway, and tests should catch accidental assignment.

Proposed resolution

Explicitly document in the coding standards that yoda conditions will not be accepted, probably in the same section that recommends strict comparison.

Remaining tasks

User interface changes

API changes

Data model changes

Original report:
I knock back core patches for these (have only seen very rarely), and never see them in contrib modules, so I think it'd be good to document that we don't use them.

If someone wants to start using them, they could open a new issue to discuss that, but making it clear what status quo is would be useful I think.

CommentFileSizeAuthor
#31 accidental-assignment.png27.75 KBpfrenssen
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Matt V.’s picture

I'm ambivalent about them personally, but I think it's worth mentioning that they're encouraged in some other projects. Specifically, yoda conditions are encouraged within the WordPress coding standards.

tizzo’s picture

Project: Drupal Technical Working Group » Coding Standards
Component: Code » Coding Standards
RobLoach’s picture

Status: Active » Needs review

Here's an article about why Yoda Conditions can be problematic:
https://dev.to/greg0ire/why-using-yoda-conditions-you-should-probably-not

Just write your code correctly, and use "===" for a strict equals. Some IDEs warn if there's a constant appearing before an assignment operator now too.

catch’s picture

Issue summary: View changes
tizzo’s picture

The TWG discussed this today and would like to announce this for final discussion. It has strong support and we believe yoda conditions are the exception not the rule. Further, if core patches are at least sometimes not accepted on this basis we feel we should formalize the requirement.

It is worth noting that Symfony uses yoda conditions extensively. While we already deviate from Symfony standards in many respects others have advocated for consistency with their standards in other issues and this would further deviate Drupal's standards from Symfony's.

It would be great to get additional information on how big an impact this would be.

I found a PHPCS sniff from a CakePHP project that might be a starting point for a coder rule.

Speaking personally, I hate yoda conditions and would like to +1 this proposal.

pfrenssen’s picture

I am in favor for this personally. This is in line with how the majority of the core code base is written, and the benefit of using it is low. The article is correct, if you can expend the mental effort to consciously use yoda conditions then you can also double check the operator.

I also agree with the other points. We already discourage inline variable assignments and encourage strict comparison operators. IDE's are great at indicating accidental assignments.

So +1 from me, the benefits don't weigh up against the effort for changing our entire code base.

tizzo’s picture

I personally give this a +1 and am including this in today's announcement. The feedback will be reviewed in our meeting on 9/19.

ZenDoodles’s picture

Please don't.

Yoda conditions are only hard to read until you get used to them. The same could be said for ternary operators, short form arrays, and anonymous functions/closures. Yet all of these are appropriate in many cases.

Also, Yoda conditions are the fifth bullet point in the Symfony coding standards.

Use Yoda conditions when checking a variable against an expression to avoid an accidental assignment inside the condition statement (this applies to ==, !=, ===, and !==);

So the Symfony project requires yoda conditions, and this proposal is suggesting Drupal should disallow it.

There are already too many barriers to entry for contributors. Reading all of the different coding standards for the Drupal project alone would probably take an afternoon. Please don't add even more unnecessary standards which make it more difficult to contribute.

valthebald’s picture

I'm against this as well. The reasoning is questionable, especially given Symfony recommendation. Talking about _personal_ preferences, I like yoda conditions

Fidelix’s picture

Symfony is wrong.
Yoda conditions in PHP are useless.

I don't want Drupal to inherit unreasonable coding standards from other projects for the sake of commonality - we already have too much of that.

pfrenssen’s picture

Just to be on one line, this discussion is not about _adopting_ yoda conditions. This is about formally _disallowing_ yoda conditions so we can have a more coherent code base. Our coding standards are about having a uniform writing style.

This has been proposed because having a mix of standard conditions and yoda conditions within the same code base is not ideal, and to formalize what is currently already an unspoken standard.

There are already too many barriers to entry for contributors. Reading all of the different coding standards for the Drupal project alone would probably take an afternoon. Please don't add even more unnecessary standards which make it more difficult to contribute.

It shouldn't make it more difficult in practice because all new adopted rules will be put into the Coder automated coding standards checker. It is best for new contributors to use this tool rather than spending time to learn the entire coding standard.

On the other hand, having undefined standards is hindering contribution. There have been instances where people have been confused during the writing and reviewing of patches because we have no stance on yoda conditions. This often leads to a discussion about the benefits and drawbacks of yoda conditions, which is arguably a waste of time and out of scope for the issue. If we have a standard people can just mention it and move on with the task at hand.

Some examples of people mentioning the lack of a standard in issues:

It also seems like this is already a de-facto standard in core. Some of our top contributors have already been rejecting patches because of using yoda conditions. Here are some examples:

dpi’s picture

Approve of Yoda, I do.

JvE’s picture

+1 for formalizing the disallowing of yoda conditions.

valthebald’s picture

You know, there are some real reasons people use yoda conditions :)
Only local images are allowed.

pivica’s picture

+1 to disallow yoda conditions.

ZenDoodles’s picture

I don't want Drupal to inherit unreasonable coding standards from other projects for the sake of commonality.

To be clear, I'm not suggesting we enforce Yoda conditionals. I'm saying we should not add this misguided coding standard.

Just to be on one line, this discussion is not about _adopting_ yoda conditions. This is about formally _disallowing_ yoda conditions so we can have a more coherent code base. Our coding standards are about having a uniform writing style.

Disallowing a practice which is generally considered a *good* practice in the PHP community in favor of being consistent just means we are promoting being consistently wrong.

I repeat. Please don't add more unnecessary standards which make it more difficult for people to contribute.

greg.1.anderson’s picture

Yoda conditionals are the wrong solution to #14. The correct solution is to disallow all assignments inside conditionals in the code sniffer.

Define this to be always wrong:

if (($a = someFunction()) == SOME_CONSTANT) {
  // do something
}

Require this instead:

$a = someFunction();
if ($a == SOME_CONSTANT) {
  // do something
}

Disallow unclear word order, we should.

mikeker’s picture

After writing Yoda conditionals for ages (I started in C and C++) I still have to pause to write non-Yoda conditionals. Though after 10 years of Drupal work, it's getting easier. My point is that, yes, it takes some time to get used to it just as it does the any of our coding standards (anyone remember the old string concatenation rules?)

I think it's a bad idea to disallow a style of coding that's actively encouraged by Symfony. We have enough conflicts as it is. I would prefer to see what @greg.1.anderson proposes in #17. That, IMO, makes for more readable code.

JvE’s picture

Still +1 for documenting that our standards are

$result = someFunction();
if ($result === 'desired') {
  // Do something.
}

and not

$result = someFunction();
if ('desired' === $result) {
  // Do something.
}

My personal preference is not to make code harder to read just to spot typos, I use a code checker for that.

Also, this issue is not about which style to use for Drupal (it does not use Yoda), but about making this standard official to avoid confusion and preserve consistency.

+1 if you want to prevent mixing the two styles.
-1 if you want to allow mixing the two styles.

If you want to convert the entire Drupal codebase to use Yoda conditions then please open a new issue.

fafnirical’s picture

+1

Drupal's JavaScript coding standards already explicitly forbid Yoda conditions: https://github.com/airbnb/javascript/blob/eslint-config-airbnb-v14.1.0/p... (not overridden in http://cgit.drupalcode.org/drupal/tree/core/.eslintrc.json). I think it makes sense to be consistent between the PHP and JavaScript coding standards.

The JS coding standards also disallow assignment in conditional statements, as #17 proposes applying to the PHP coding standards. I think a combination of both makes for the most readable code, and would avoid the issues Yoda conditions are designed to avoid.

joachim’s picture

> The correct solution is to disallow all assignments inside conditionals in the code sniffer.

I think that's a good plan. I find assignments inside conditionals hard to read.

Though that should be a separate issue shouldn't it?

jgloverattronedotcom’s picture

I'm fine with either of the following:
1) disallowing assignments within conditionals
2) requiring yoda conditions (not eliminating them)

be55roberts’s picture

Another option would be to allow Yoda Conditionals, but not require them. Then, you are being consistent with existing code, since Symfony already uses them. Just because you allow them does not mean you require them. You tolerate them as a coding style element for those that like to use them. Yoda Conditionals were created to catch assignment errors in conditional expressions. And they do that well IMHO. However, they are really a matter of personal style. If you want to encourage more people writing Drupal code, then you should allow them some measure of personal style.

JvE’s picture

gbirch’s picture

-1 on this proposal.

I admit that when I first ran into yoda conditions, they seemed stupid and hard to read. Once I understood the purpose, however, I switched over to using yoda coding all the time. I cannot tell you how many times it has saved my butt.

This proposal would only be a good idea if we also forbid assignment within conditionals (greg anderson's proposal in #17). As long as we permit assignment within conditional statements, it is counterproductive to actively prevent people from using a coding style that avoids many preventable errors.

And in an appeal to arbitrary authority, please remember that Fabien Potencier is probably one of the best PHP coders out there. If he requires yoda conditions in his code, that seems like a good idea, no?

TR’s picture

Status: Needs review » Needs work

This whole discussion is moot until someone defines exactly what would/would not be forbidden by this ethereal proposed change to the standard.

Seriously, three years of discussion without specifying exactly what's being discussed? And now we're at "final discussion" phase?

Please - propose specific language to be added to the coding standards, then we will have something to talk about. Standards 101 teaches you that you need prescriptive language in a standard. "Yoda conditions" is not prescriptive language.

joachim’s picture

Status: Needs work » Needs review

> Standards 101 teaches you that you need prescriptive language in a standard. "Yoda conditions" is not prescriptive language.

The term 'Yoda conditions' is quite well-understood: https://en.wikipedia.org/wiki/Yoda_conditions

> And in an appeal to arbitrary authority, please remember that Fabien Potencier is probably one of the best PHP coders out there. If he requires yoda conditions in his code, that seems like a good idea, no?

Nobody's perfect, even the best PHP coder. Symfony makes the mistake of having single-author credit in code files, which over here in Drupal we worked out years ago was a bad thing as it inhibit contributions from others. Also, Symfony's documentation is pretty atrocious.

TJ’s picture

+1, and best explanation why Yoda condition is still used these days (comment from: https://dev.to/greg0ire/why-using-yoda-conditions-you-should-probably-no...):

I think this is a lot like the story where a daughter asks her mom why she cuts the ends off the roast before she cooks it. Her mom says that grandma used to do it that way, so she did as well. When grandma is asked by the daughter, she says that she did it because the roast was too big for her small pan...

Honestly, this style of programming came out of necessity. We used it back in the old C/C++ days because the tooling and testing support just wasn't mature compared to today. I remember the first time we upgraded the compiler to a version that would detect this condition. We had all sorts of hidden bugs instantly visible.

I think the reason this style has persisted so long, is that older developers have passed it down due to the development scars we carry from years ago. I do find myself writing this type of code, for the very reason that it's just the way I have always done it. It is good to see these beliefs being questioned, and it will probably change the way I code from here on, just due to it being pointed out. It you have the tooling and testing support, there is just no valid reason to drop in a Yoda condition anymore.

gbirch’s picture

TJ, honestly curious, what testing tools do you (or anyone else) use to identify when if ($a = 'foo') is a typo rather than intended? Both $a = 'foo' and $a == 'foo' are legal in PHP, permitted by Drupal coding standards, and often used in core code.

Thanks.

JvE’s picture

@gbirch, I use PHPstorm's inspection (PHP>Probable bugs>Assignment in condition)
Yoda I need not.

pfrenssen’s picture

FileSize
27.75 KB

@gbirch, the best tool by far is to write unit tests. If your code is covered by unit tests there's no way that an accidental assignment instead of a comparison is ever going to pass the tests.

If you don't write tests then you can best configure your IDE to report this, for example if you use PHP CodeSniffer you can turn on the "Assignment in condition" inspection, and it will highlight it like this, which is pretty hard to miss :)

edit: oops, crossposted with @JvE

effulgentsia’s picture

@ZenDoodles: Re:

There are already too many barriers to entry for contributors. Reading all of the different coding standards for the Drupal project alone would probably take an afternoon. Please don't add even more unnecessary standards which make it more difficult to contribute.

Can you share more on why you think adding this coding standard would be a barrier to contribution? How is it more of a barrier to contribution than standardizing on two spaces (what Drupal does) for indentation instead of four (what Symfony does) or standardizing on having the opening brace after a function declaration on the same line as the function name (what Drupal does) instead of on a new line (what Symfony does)? Per #11, no one needs to read all of the standards: the checking of it is automated for Core and other projects where maintainers choose to run Coder pre commit. And for projects that don't run Coder pre commit, there's no harm in getting it wrong and then fixing it when someone notices the inconsistency and files an issue to fix it.

The intro of https://www.drupal.org/docs/develop/standards/coding-standards links to https://drupalize.me/videos/understanding-drupal-coding-standards?p=2012, and that page says:

These coding standards ensure consistency in code throughout the project, make it easy for developers to move around from one sub-section to another without having to re-learn how to read the code, and most of all help us to spend our time debating the functionality and logic contained in the code and not the semantics of whitespace. You can simply point to the rules and say, "This is what we agreed on."

Do you disagree with this idea in general, or only in regards to yoda conditions, and if the latter, why?

tizzo’s picture

Status: Needs review » Postponed

The TWG discussed in today's meeting and agree that, while we support this proposal, it should not be ratified without first ratifying #2907332: Disallow assignments in conditions. This other issue addresses the primary concerns raised here around catching typos resulting in accidental assignment.

pfrenssen’s picture

Status: Postponed » Active

#2907332: Disallow assignments in conditions is closed as Won't Fix because there was no consensus from the community. This means that this can be unblocked, but it will require further discussion.

peterhebert’s picture

I agree with this - I always thought the requirement in the WordPress coding standards to use Yoda conditions was confusing and annoying. Please make it explicit to not use this in the Drupal coding standards.