Problem/Motivation
It is not documented in the coding standards that Drupal does not use Yoda conditions.
This can lead to some discussions when patches with that style are rejected.
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.
Comment | File | Size | Author |
---|
Comments
Comment #1
Matt V. CreditAttribution: Matt V. commentedI'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.
Comment #2
tizzo CreditAttribution: tizzo commentedMoving this issue to the Coding Standards queue per the new workflow defined in #2428153: Create and document a process for updating coding standards.
Comment #3
RobLoachHere'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.
Comment #4
catchComment #5
tizzo CreditAttribution: tizzo commentedThe 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.
Comment #6
pfrenssenI 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.
Comment #7
tizzo CreditAttribution: tizzo commentedI personally give this a +1 and am including this in today's announcement. The feedback will be reviewed in our meeting on 9/19.
Comment #8
ZenDoodles CreditAttribution: ZenDoodles as a volunteer and at Palantir.net commentedPlease 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.
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.
Comment #9
valthebaldI'm against this as well. The reasoning is questionable, especially given Symfony recommendation. Talking about _personal_ preferences, I like yoda conditions
Comment #10
Fidelix CreditAttribution: Fidelix as a volunteer commentedSymfony 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.
Comment #11
pfrenssenJust 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.
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:
Comment #12
dpiApprove of Yoda, I do.
Comment #13
JvE CreditAttribution: JvE as a volunteer commented+1 for formalizing the disallowing of yoda conditions.
Comment #14
valthebaldYou know, there are some real reasons people use yoda conditions :)
Comment #15
pivica CreditAttribution: pivica at MD Systems GmbH commented+1 to disallow yoda conditions.
Comment #16
ZenDoodles CreditAttribution: ZenDoodles as a volunteer and at Palantir.net commentedTo be clear, I'm not suggesting we enforce Yoda conditionals. I'm saying we should not add this misguided coding standard.
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.
Comment #17
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedYoda 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:
Require this instead:
Disallow unclear word order, we should.
Comment #18
mikeker CreditAttribution: mikeker as a volunteer commentedAfter 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.
Comment #19
JvE CreditAttribution: JvE as a volunteer commentedStill +1 for documenting that our standards are
and not
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.
Comment #20
fafnirical+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.
Comment #21
joachim CreditAttribution: joachim as a volunteer commented> 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?
Comment #22
jgloverattronedotcom CreditAttribution: jgloverattronedotcom commentedI'm fine with either of the following:
1) disallowing assignments within conditionals
2) requiring yoda conditions (not eliminating them)
Comment #23
be55roberts CreditAttribution: be55roberts commentedAnother 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.
Comment #24
JvE CreditAttribution: JvE as a volunteer commentedFor discussion: #2907332: Disallow assignments in conditions
Comment #25
gbirch CreditAttribution: gbirch commented-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?
Comment #26
TR CreditAttribution: TR commentedThis 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.
Comment #27
joachim CreditAttribution: joachim as a volunteer commented> 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.
Comment #28
TJ CreditAttribution: TJ commented+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...):
Comment #29
gbirch CreditAttribution: gbirch commentedTJ, 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.
Comment #30
JvE CreditAttribution: JvE as a volunteer commented@gbirch, I use PHPstorm's inspection (PHP>Probable bugs>Assignment in condition)
Yoda I need not.
Comment #31
pfrenssen@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
Comment #32
effulgentsia CreditAttribution: effulgentsia at Acquia commented@ZenDoodles: Re:
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:
Do you disagree with this idea in general, or only in regards to yoda conditions, and if the latter, why?
Comment #33
tizzo CreditAttribution: tizzo commentedThe 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.
Comment #34
pfrenssen#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.
Comment #35
peterhebert CreditAttribution: peterhebert commentedI 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.
Comment #36
PolI'm in favour of using those "yoda" conditions. I also do agree with @pfrenssen even if he seems to not like them :-)
@pfrenssen, you're right when you say that:
But don't forget that having 100% of tests coverage on a project is not always possible.
It is sometimes very hard to detect them, of course, code mutation testing would help a lot, but I don't think it has been adopted by the community so far.
Comment #37
bkosborneLooks like quite a split on this issue! I'm in favor of forbidding them. I find them to be confusing to read and adds to cognitive load significantly, but maybe I would eventually get used to it as others have said. I think that modern IDEs and good code review are reason enough to not allow them anymore. But what's important is that the community has some official stance on this.
Comment #38
gbirch CreditAttribution: gbirch commentedOn the one side we have people who find reading Yoda conditions confusing, partly because they violate normal English syntax, and partly from pure unfamiliarity. (For what it's worth, I used to be on your side, but it took me under 30 minutes to get used to Yoda coding. It's really not a big hurdle.)
On the other side, we have people who find that using Yoda conditions prevents difficult-to-detect errors in their code (admittedly, this is only relevant for the few of us who have not achieved 100% test coverage ... ok, that is a snark).
On the third hand, we have a standard-setting group that has already decided to table the issue of whether to forbid assignments in conditional statements -- the exact problem that Yoda conditions address. The obvious solution is to table this issue too.
But if we truly need to have an official stance, as @bkosborne suggests, and we don't have an internal consensus, then for consistency we should go with the standard adopted by Symfony, which represents the largest and probably the most important source of third-party code that Drupal has adopted. In short, Yoda conditions should be required. https://symfony.com/doc/current/contributing/code/standards.html
Comment #39
cilefen CreditAttribution: cilefen as a volunteer commentedDeliberations on this are now in their fifth year. I would have preferred this (no Yoda) *and* #2907332: Disallow assignments in conditions, because together they solve the problem of avoiding accidental assignment while promoting readability. Both are laudable goals of a coding standard.
I’m not a Yoda-hater but let’s be practical about solving a problem and facilitating the greatest number of contributors.
Comment #40
valthebaldThere is always an option to keep the status quo
Comment #41
pfrenssenThe standard-setting group is mostly concerned with defining a coding standard that matches the established code style patterns as closely as possible so we can have automated tools pointing out any deviation from the standard.
Any change to the coding standards is always a community decision so we have this issue to discuss it and weigh the impact it will have on the community.
For this particular issue it is quite clear that the majority of the code that has been written for Drupal doesn't use yoda conditions, but this is not yet captured in the standard. For as long as the community is divided on the topic, we can not make a decision yet.
We can possibly revise our approach to more readily adopt existing patterns in the standards, but this again should be a community decision.
Of course as always it is entirely optional for any project or contributed module to choose whether or not they adopt the Drupal coding standards fully or partially. It is recommended to follow the standard as closely as possible, but this is at the discretion of the module authors.
Comment #42
markhalliwellPerhaps not. However it certainly favors it as such, e.g. patches would likely be rejected and asked to be changed to follow core's existing codebase "standard". In fact, I suspect you'd be hard pressed to find any top-level contrib project even that would allow yoda conditions used in abundance.
This doesn't sound like a very feasible approach.
The community will always be divided on this topic. That is why creating a coding standard is important in the first place; to establish a singular pattern that everyone should follow.
This issue only has 39 participants, this hardly reflects the "community" as a whole. Maybe a public poll should be taken instead. One that doesn't require social media to participate (e.g. online anonymous form to simply tally votes). A Drupal Planet post or d.o banner pointing to this issue/poll.
---
Also, what keeps us from tentatively adopting this as a policy, i.e. soft rollout.
Automation tools could easily create warnings about this (rather than errors) to indicate this could potentially become a problem and point to this issue.
This would allow us to effectively see how much of an impact this would actually have on the community. As it stands now, this "impact" that is discussed is very arbitrary and hypothetical.
We could give it a month or two and then revisit where we stand on the topic. If nothing major creeps up we can adopt. If we suddenly realize it'll be more trouble than it's worth (albeit highly unlikely), we can reject.
In fact, I don't know why we don't do this already for any new proposed policy.
Comment #43
JvE CreditAttribution: JvE as a volunteer commentedI don't think the community is divided on whether or not Drupal currently uses Yoda conditions.
The division is in whether or not to document that Drupal does not use Yoda conditions.
Some people think it should be written down to avoid endless discussions during patch reviews.
Other people feel such documentation impinges on their freedom to use Yoda conditions in their own code and do not want it written down.
And of course a lot of people confuse the issue by giving their opinion on whether or not Yoda conditions are a good thing. Or why Drupal should or should not use Yoda conditions.
Comment #44
bkosborneThis is a great idea. Would be great if the D.A. would create the infrastructure to support something like this.
Comment #45
pfrenssenThe DA already has the infrastructure to do public announcements: all important announcements that impact Drupal core are made on https://groups.drupal.org/core - this channel is also used to announce proposed changes to do coding standards. These announcements are also included in Drupal Planet. If you are interested in coding standards it would be good to keep an eye on these channels.
A banner on drupal.org seems overkill. There are not so many people in our community that are that deeply interested in the coding standards. Having a turnout of 30+ people is considered a big success! Most of our issues get responses from a handful of people.
It seems most people have a neutral attitude towards the coding standards and see it as something that is useful but not critical to their daily lives as a programmer. Probably the only way to get massive responses is to change something fundamental. I can only imagine the outrage and complaints if we would change indenting to 4 spaces :)
Comment #46
donquixote CreditAttribution: donquixote commentedTLDR:
If we want to introduce this convention for the sake of having a convention, ok with me to bend to the will of the majority and to the way things have been commonly done in the past.
However: I think the arguments so far have been quite one-sided.
------
Personally I love yoda conditions.
The "prevent accidental assignment" was never the main argument for me.
If you compare an if() condition with a sentence in English (or German), then yes, the yoda style would be against the usual grammar rules.
I personally don't think of code as equivalent to human-language grammar. But I do think that other principles from human language information processing apply.
We see this in newpaper or clickbait headlines, and also in news articles written in the "inverted pyramid style".
Such headlines use tricks to put the most important, most catchy or most distinguishing piece of information in the front. Leading adjectives allow doing this while staying within the grammatical framework of the language.
In conditions, the most relevant piece of information is what distinguishes it from the "else" case.
E.g.
"If on fire is the house on the end of the street, send the firefighters. Otherwise, ...".
"If the house on the end of the street is on fire, send the firefighters. Otherwise, ...".
In a headline, there would be tricks used to allow putting the "fire" in the front, and comply with the expected grammar for a headline.
In code, we want to comply with the grammar of PHP.
I also like patterns like this:
The part within the strpos() or fgetcsv() might get quite long, so it is nice to have the FALSE !== and the assignment at the front.
Combining assignment and comparison into one line can be confusing, but in the examples above it is a very simple and repeatable pattern.
Seeing the literal values in the front of each if/elseif case also helps to check if all cases are covered. E.g. did we check for NULL, if this is a possible value?
Not covering all possible value (types) is one of the most common sources of bugs in Drupal.
----
Again, as said before: I assume that I am in a minority here, and we probably should go ahead with the proposed convention. I just wanted to voice my own motivation.
Comment #47
donquixote CreditAttribution: donquixote commentedAdding to my previous comment.
At first sight this would translate to
, which indeed feels wrong and violates English grammar.
To me, I would rather translate this as
This is fine with English grammar, and the 'clear' is first(-ish).
Comment #48
donquixote CreditAttribution: donquixote commentedI encountered this code the other day, which is quite interesting for this discussion.
(I am simplifying and anonymizing it a bit, to not put anyone to shame, and because I am paranoid.)
I am also adding a line break, so that the "yoda or not" comparison is on a separate line.
If you inspect this code carefully and spot the mistake, good for you!
(EDIT: I notice that drupal.org's text wrapping is working against my point.. too bad)
If not, try again with this modified version:
After the "yoda swap", it becomes more obvious that something with this code is fishy:
The return value of drupal_valid_token() is expected to be boolean, but is compared to a string.
Coincidentally it all works out well in this case. We already verified that $_GET['token'] is true-ish (= not empty), and the == operator's type coercion does the rest.
But what I want to argue here is that the yoda condition makes it easier to spot what is being compared.
On the other hand, having the comparison appended at the end of a long statement obfuscates what is going on, and may even obfuscate bugs. (which in this case may be critical for security - we just got off lucky).
Of course even better would be to further break it down, to reveal the anatomy:
After a reverse yoda swap it looks like this:
Again, the comparison operator is in a place where you are less likely to notice it, even though it is the (unintended) centerpiece of the expression.
(EDIT: I notice that I am arguing from my own personal experience and perception. It would be interesting to back this up by user testing, focusing on this specific question. E.g. how long do people need to spot the bug, in the yoda version vs the non yoda version..)
Comment #49
JvE CreditAttribution: JvE as a volunteer commentedThank you for your input donquixote.
As per issue summary and comments #11, #41 and #43:
Current situation:
- Drupal does not use Yoda
- This is not documented in the coding standards
Situation proposed in this ticket:
- Drupal does not use Yoda
- This is documented in the coding standards
Not options in this ticket (but feel free to open new tickets):
- Convert the entire code base to use Yoda
- Document in the coding standards that mixing up Yoda and non-Yoda is fine
- Document that Yoda is good
- Document that Yoda is bad
Comment #50
donquixote CreditAttribution: donquixote commentedAnother option:
Don't talk about "Yoda" or "not Yoda", but instead bring up a list of examples, see how they would look like in either version, and then determine more specific guidelines about what should go to the left or the right of a comparison operator.
Comment #51
JvE CreditAttribution: JvE as a volunteer commentedYes, that would be number 2 of the "not options in this ticket" list.
Again, this ticket is not about what should go to the left or the right of a comparison operator.
It is about documenting the fact that Drupal places the variable on the left side and has constants, literals, and function calls on the right side.
That way reviewers have a standard to point to when rejecting core patches with Yoda conditions.
If you want Drupal to adopt Yoda conditions (like Symfony or Wordpress) or to adopt a mix of the two styles based on some set of rules, then please open a new ticket.