Problem/Motivation

I thought it might be interesting to propose this as a new standard.

From the SEI CERT Oracle Coding Standard for Java
(https://www.securecoding.cert.org/confluence/display/java/EXP51-J.+Do+no...):

Using the assignment operator in conditional expressions frequently indicates programmer error and can result in unexpected behavior. The assignment operator should not be used in the following contexts:

  • if (controlling expression)
  • while (controlling expression)
  • do ... while (controlling expression)
  • for (second operand)
  • switch (controlling expression)
  • ?: (first operand)
  • && (either operand)
  • || (either operand)
  • ?: (second or third operands) where the ternary expression is used in any of these contexts

Noncompliant Code Example

In this noncompliant code example, the controlling expression in the if statement is an assignment expression:

public function f($a, $b) {
  if ($a = $b) {
    /* ... */
  }
}

Although the programmer's intent could have been to assign b to a and test the value of the result, this usage frequently occurs when the programmer mistakenly used the assignment operator = rather than the equality operator ===.

Compliant Solution

The conditional block shown in this compliant solution executes only when a is equal to b:

public function f($a, $b) {
  if ($a === $b) {
    /* ... */
  }
}

Unintended assignment of b to a cannot occur.

Compliant Solution

When the assignment is intended, this compliant solution clarifies the programmer's intent:

public function f($a, $b) {
  if (($a = $b) === TRUE) {
    /* ... */
  }
}

Compliant Solution

It may be clearer to express the logic as an explicit assignment followed by the if condition:

public function f($a, $b) {
  $a = $b;
  if ($a) {
    /* ... */
  }
}

Noncompliant Code Example

In this noncompliant code example, an assignment expression appears as an operand of the && operator:

public function f($a, $b, $flag) {
  while(($a = $b) && $flag) {
    /* ... */
  }
}

Because && is not a comparison operator, assignment is an illegal operand. Again, this is frequently a case of the programmer mistakenly using the assignment operator = instead of the equals operator ===.

Compliant Solution

When the assignment of b to a is unintended, this conditional block is now executed only when a is equal to b and flag is true:

public function f($a, $b, $flag) {
  while(($a === $b) && $flag) {
    /* ... */
  }
}

Applicability

The use of the assignment operator in controlling conditional expressions frequently indicates programmer error and can result in unexpected behavior.

As an exception to this guideline, it is permitted to use the assignment operator in conditional expressions when the assignment is not the controlling expression (that is, the assignment is a subexpression), as shown in the following compliant solution:

public function readLines($reader) {
  $line = '';
  while(($line = $reader->readLine()) !== NULL) {
    // ... Work with $line.
  }
}

Comments

JvE created an issue. See original summary.

pfrenssen’s picture

Big +1 from me. I even thought that we already had this rule but apparently not. I haven't done any research but I believe this is already the accepted de-facto standard in the majority of the Drupal core code base. It would be good to formalize this in a coding standard.

Also apart from consistency in the code base, this is the kind of rule that will actually help to prevent bugs from sneaking in.

joelpittet’s picture

I've been seeing this around and yes +1 to this proposal.

greg.1.anderson’s picture

I personally would also prefer to reject:

public function f($a, $b) {
  if (($a = $b) === TRUE) {
    /* ... */
  }
}

This construct is simply easier to read when split across two lines.

I think it would be okay to allow, but perhaps deprecate a similar construct in loops:

public function f($a, $b) {
  while (($x = y($a, $b)) === TRUE) {
    /* ... */
  }
}

I'd prefer not to see this in code either, although in this case it is less convenient to refactor.

fafnirical’s picture

Should we aim for consistency between the PHP and JavaScript coding style?

The JavaScript coding standards currently always forbid assignment operators in conditional statements, even if they're enclosed in parentheses: https://github.com/airbnb/javascript/blob/eslint-config-airbnb-v14.1.0/p... (not overridden by http://cgit.drupalcode.org/drupal/tree/core/.eslintrc.json).

JvE’s picture

pfrenssen: I believe this is already the accepted de-facto standard in the majority of the Drupal core code base

State of drupal core assignments in conditions as per PHPstorm's inspection (PHP>Probable bugs>Assignment in condition):

drupal version violations sloc violations per 1000 loc
6.x 362  49K 7,43
7.x 285 135K 2,10
8.0.x 780 636K 1,23
8.1.x 610 524K 1,16
8.2.x 646 551K 1,17
8.3.x 673 577K 1,17
8.4.x 689 594K 1,16
joachim’s picture

+1 to this, and +1 to #4 too.

Code benefits much more from being clear and readable than from being brief.

drunken monkey’s picture

I think I'm rather -1 on making this a general rule. Sure, in a lot of cases it's a bad idea, but in others it's definitely easy enough to read, and clear that it's not a mistake. E.g.: while ($row = db_fetch_object($result)) {. That's used a lot and, I'd argue, very clear and familiar to almost everyone. I'm sure similar examples could also be found for OOP code. And I don't think it's possible to define a coding standard on this in a way that would allow the "clean" usage of the construct. I'd rather suggest maybe just adding a statement that people should think before using an assignment as a condition, on whether this is clear enough. For pure variable assignments ($a = $b), e.g., I'd argue that this is never the case – it's just too easy to confuse with a typo of $a == $b. (Maybe we could just forbid those? Or are they not being used anyways?)

The fact that we'd have 600+ violations of this standard at the moment (at least in its strictest form, I guess) also makes it clear, I think, that this is far from being a "de facto standard" (like forbidding the or and and operators is).

At the very least, we should still allow this as part of a while condition, as it's much harder to rewrite that in a compliant way. In all the other cases, it's at least simple enough to get rid of.

JvE’s picture

db_fetch_object() was last seen in Drupal 6.

The pattern while ($VARIABLE = occurs only 36 times in Drupal 8.

The most common use is not with database records but with array walking:
while ($part = array_shift($parts)) {
which can sometimes be refactored to a foreach and other times to
while (($part = array_shift($parts)) !== NULL) {

The other violations are mostly things like
if ($permission_access = $account->hasPermission($permission)) {
where you have to look twice to see if assignment or comparison was meant.

Another common pattern is
if ($cache = $this->cacheGet($cid)) {
where the intent is clearer.
Would it hurt to change this to

$cache = $this->cacheGet($cid);
if ($cache !== FALSE) {

or
if (($cache = $this->cacheGet($cid)) !== FALSE) {
?

Andrej Galuf’s picture

As long as the second compliant solution is allowed, I'm +1 with this, as it covers the only use case where I am using direct assignments (file handlers).

tizzo’s picture

The coding standards committee discussed this today and feel that there is sufficient support here to announce this issue for final discussion. It will be included in the next announcement.

borisson_’s picture

I'm +1 on this one as well, but I share @drunken monkey's about while ($item = $things->getResultIterator()) {. I'd love for that to still be legal while disallowing it for if-statements specifically.

JvE’s picture

while ($item = $things->getResultIterator()) {
doesn't look like code we'd want. What if one of the things the iterator is supposed to return is '' or 0 or FALSE?
I'd say disallowing that and allowing

while (($item = $things->getResultIterator()) !== NULL) {

makes the intent of the code clearer while reducing the potential for bugs.

Chi’s picture

-1 from me. I happen to like this feature in PHP. When appropriately used it makes the code less verbose.

drunken monkey’s picture

doesn't look like code we'd want. What if one of the things the iterator is supposed to return is '' or 0 or FALSE?

Then, of course, you shouldn't use it. But that's a completely separate issue, you'd have the same problem with:

$item = $things->getResultIterator();
if ($item) {
…

No matter the code style, you should always be aware of possible return values and check appropriately.

bojanz’s picture

-1.

Feels too prescriptive.

JvE’s picture

Feels too prescriptive.

This I do agree on.

This proposal was mainly created to counter the stance of "If allowed are assignments in conditions then Yoda conditions to protect us we need" ;)

It might be nice though to have some sort of standard to fall back on to avoid discussions about allowing or disallowing code like
if ($permission_access = $account->hasPermission($permission)) {

tim.plunkett’s picture

If code like

if ($children = Element::children($element)) {

is subject to this rule as I understand it to be, then

-1
David Strauss’s picture

A +1 for me. Making code short is less important than a low defect rate and easy readability when returning to it later. This coding approach also makes it slightly more clear that anything set has the scope of the function, not the controlling expression or block within it.

benjy’s picture

-1 from me, I think use cases like #18 are common and quite readable. The example in the issue summary looks worse because it's two variables that are one letter long, e.g. $a and $b. It's much more obvious when you have if ($variable = my_method_call())

tim.plunkett’s picture

If the rule was "no comparisons and assignment in the same line" that'd be fine.
If you need the value for later AND want to introspect it, split it up.

For example,

if (($value = $form_state->getValue('site_frontpage')) && $value[0] !== '/') {

That is not reasonable.
That should absolutely be

$value = $form_state->getValue('site_frontpage');
if ($value && $value[0] !== '/') {

But checking the truthiness of the return value of a function call, as in mentioned in previous comments, should be fair game.

tim.plunkett’s picture

Just a rough search:

~/www/d8$ grep -r --include "*.php" "if (.* = " core | wc -l
     820
~/www/d8$ grep -r --include "*.php" "if (.* = .*==" core | wc -l
      35
borisson_’s picture

If the rule was "no comparisons and assignment in the same line" that'd be fine.
If you need the value for later AND want to introspect it, split it up.

I think this makes more sense, and is closer to the intention of the issue, at least how I understood it, this is much easier to build consensus around.

+1

JvE’s picture

if ($children = Element::children($element)) {
looks a lot like
if ($children == Element::children($element)) {
but with very different meaning and behavior, leading to hard to find bugs.

In order to to reduce the defect rate and provide clarity there appear to be three ways to combat this:

  1. disallow/flag/discourage if ($children = Element::children($element)) { when assignment is meant.
  2. enforce/suggest/encourage if (Element::children($element) == $children) { when comparison is meant.
  3. use tests to reduce bug potential and use standards only to facilitate easier diffing/merging

There are currently two proposals on the coding standards table:
One that disallows solution 2: #2372543: [policy, no patch] Explicitly disallow yoda conditions
And one that allows solution 1: #2907332: Disallow assignments in conditions

andypost’s picture

-1

Most of conditions that used with field values can't use strict type check so there will be always room for error
Also while with iterator is very common and imo much more readable

JvE’s picture

Also while with iterator is very common

Where? Not in Drupal 8. These are all the occurrences:

$ grep -ri --exclude=*.sh 'while ($[a-z0-9_]* = ' core
core/modules/migrate/src/Plugin/Discovery/AnnotatedClassDiscoveryAutomatedProviders.php:    } while ($parser = StaticReflectionParser::getParentParser($parser, $this->finder));
core/modules/system/tests/src/Functional/Entity/Update/LangcodeToAsciiUpdateTest.php:    while ($row = $query->fetchAssoc()) {
core/modules/node/src/Plugin/migrate/source/d6/ViewMode.php:    while ($field_row = $result->fetchAssoc()) {
core/modules/block/block.module:  while ($part = array_shift($parts)) {
core/modules/taxonomy/src/Form/OverviewTerms.php:        while ($pterm = $tree[--$tree_index]) {
core/modules/taxonomy/src/TermStorage.php:        while ($tid = array_shift($terms_to_search)) {
core/modules/taxonomy/src/TermStorage.php:          } while ($child = next($this->treeChildren[$vid][$parent]));
core/modules/field/src/Plugin/migrate/source/d6/FieldInstancePerViewMode.php:    while ($field_row = $result->fetchAssoc()) {
core/modules/field/src/Plugin/migrate/source/d6/FieldInstancePerFormDisplay.php:    while ($field_row = $result->fetchAssoc()) {
core/modules/comment/src/CommentStatistics.php:    while ($entry = $stats->fetchObject()) {
core/modules/book/src/BookManager.php:    while ($item = array_pop($links)) {
core/lib/Drupal/Component/Diff/Engine/DiffEngine.php:      while ($pt2 = next($seps)) {
core/lib/Drupal/Component/Utility/UserAgent.php:      } while ($prefix = substr($prefix, 0, strrpos($prefix, '-')));
core/lib/Drupal/Core/Menu/MenuTreeStorage.php:    while ($tree_link_definition = array_pop($links)) {
core/lib/Drupal/Core/Diff/DiffFormatter.php:    while ($line = array_shift($del)) {
core/lib/Drupal/Core/Database/Connection.php:    while ($savepoint = array_pop($this->transactionLayers)) {
core/lib/Drupal/Core/File/MimeType/ExtensionMimeTypeGuesser.php:    while ($additional_part = array_pop($file_parts)) {
core/lib/Drupal/Core/Theme/ThemeManager.php:      while ($pos = strrpos($hook, '__')) {
core/lib/Drupal/Core/Theme/Registry.php:    while ($pos = strrpos($base_hook, '__')) {
core/lib/Drupal/Core/Config/ConfigManager.php:    while ($dependent = array_pop($dependents)) {
core/lib/Drupal/Core/Config/TypedConfigManager.php:    while ($name = array_shift($parts)) {
core/lib/Drupal/Core/Archiver/ArchiveTar.php:                while ($v_data = @fread($v_file_from, 1024)) {
core/themes/engines/twig/twig.engine:      while ($pos = strrpos($hook, '__')) {
core/tests/Drupal/KernelTests/KernelTestBase.php:    while ($callback = array_shift($callbacks)) {
core/tests/Drupal/KernelTests/Core/Theme/RegistryTest.php:    } while ($suggestion = array_shift($suggestions));
core/tests/Drupal/KernelTests/Core/Command/DbDumpTest.php:    while ($row = $query->fetchAssoc()) {
core/tests/Drupal/KernelTests/Core/Command/DbDumpTest.php:    while ($row = $query->fetchAssoc()) {
core/tests/Drupal/Tests/BrowserTestBase.php:    while ($caller = Error::getLastCaller($backtrace)) {

I would not call this "very common".

They may be hard to rewrite in some cases and we could surely make an exception for these.
Just trust the developers to know their comparison tables.

james.williams’s picture

Subjectively, I would be against this proposal.

On a possibly more objective note, surely the effort needed to change all the existing core (and contrib) code that would currently fail this rule is not worth the benefits? As far as I'm aware, the benefits are:

1) less potential of a specific bug - but really, how many of them have been caused by this? I can't imagine effort fixing them would be *saved* by this. Is it possible to have a linter highlight all the places where this is done, to make identifying bugs easier, without actually requiring the effort to be 'wasted' on refactoring so much working code?

2) better standards help with on-boarding new developers - I really don't think this particular standard is clear cut enough to help new developers on the project. If anything, I expect many to be turned off by yet more stringent coding standards requirements. It's hard to draw the line, but too may standards means too many hoops to jump through which will put new developers off too. Standards that are clearly accepted by developers within & outside the Drupal project are well worthwhile for that, as they are easy to accept for new developers. But the amount of debate on this one shows it's nowhere near as acceptable. (In my opinion.)

Maybe we need standards for what new standards should be allowed ;-)

rfulcher’s picture

+1 on this.

colan’s picture

-1. Agree with #8. It improves readability and reduces the number of lines of code.

heddn’s picture

-1.

NiklasBr’s picture

-1, I agree with #8 and #27.

Also, I believe everyone is comfortable with code like for ($i = 0; $i <= 7; $i++) {}. The proposal as it currently stands will only forbid assignment for the second operand, which I believe also introduces small inconsistencies in the coding standards. Even though the adherence to the rules can be automatically checked, small inconstancies like this feel harder to wrap ones head around. Either disallow across the board or allow it as it is today.

effulgentsia’s picture

For the people saying -1 due to enjoying the readability/terseness of using assignments in conditions, would you be more open to this proposal if it was modified to allowing assignments within their own parentheses within conditions? In other words, the current issue summary makes a provision for allowing:

if (($children = Element::children($element)) !== NULL)

But what if we simply allowed:

if (($children = Element::children($element)))

as well?

This matches the default ("except-parens") option of ESLint's no-cond-assign rule. It is also the way that PHPStorm accepts those as not "Probable bugs" during code inspection.

In other words, for only the cost of 2 extra characters on a line that needs it, we'd have automated protection from places where it's an accidental bug/typo.

heddn’s picture

I like if ($children = Element::children($element)). I dislike if (($children = Element::children($element)) !== NULL). The later is too prone to bugs.

effulgentsia’s picture

@heddn: So what's your take on if (($children = Element::children($element)))? In other words, instituting a rule equivalent to ESLint's no-cond-assign with the "except-parens" option?

I'm asking because clearly this issue does not have sufficient support to approve as-is. But since most of the -1s are in order to maintain terseness, I'm wondering whether instead of closing the issue as "won't fix" entirely, if modifying it to allow just a set of parentheses to communicate intentional assignment would make it have more support and address people's concerns.

less potential of a specific bug - but really, how many of them have been caused by this?

I don't have statistics on how common accidental assignment bugs are, but it's apparently common enough for w3schools to list it as the first common JS mistake. They don't have a page for common PHP mistakes.

heddn’s picture

I don't like the extra parens. But I could get used to them. I do not like assignment and conditional on the same line. It gets too confusing.

Meaning, I prefer if ($children = Element::children($element)) but could get used to if (($children = Element::children($element))). I still dislike if (($children = Element::children($element)) !== NULL)

colan’s picture

I agree with #35 mostly, but still find the extra parentheses unnecessary. I'm not sure why my IDE complaining about in-line assignments would take precedence over my complaining about extra parentheses. ;)

effulgentsia’s picture

Issue tags: +Security improvements

By the way, in C/C++, assignments in conditions has long been recognized as risky practice. Compilers generate a warning for it, even with the double parentheses I suggested in #32. In 2003, there was a backdoor attempted on the Linux kernel that tried to exploit the fact that these bugs are easy to miss during a code review. And the issue summary links this proposal to a recommendation by SEI CERT, so tagging this as a potential security improvement.

Also, per #5, this is a coding standard used by the airbnb coding standards, which is the most commonly used JavaScript coding standards within many web projects, including now in Drupal. This issue, however, is for PHP, not JS or C++, but still pointing out the parallels, in case that matters to us.

However, I'm hearing that people here don't like the proposal, and that's fine. I'm not sure whether to close it as "won't fix" or leave it open for more discussion.

joelpittet’s picture

Title: Disallow assignments in conditions. » Disallow assignments in conditions
Issue summary: View changes

Definitely a contentious issue, I'm leaning to "won't fix" because not sure I can give a solid argument that would convince most who have came out -1 this. I'm still a +1 for the issue title "Disallow assignments in conditions" with the exception on the while ($line = $reader->readLine()) But exceptions don't make for good rules and they keep growing as this issue ages.🤕

Fixing a IS typo and title typo.

David Strauss’s picture

I still think it's better to have a restrictive standard with an exception or two than no restriction at all.

May I suggest the following?

  • Allow simple assignments in conditions, including "if" ones. By "simple," I mean only a variable, an single equals operator, and an expression returning a value.
  • Require, where possible, that constant values be on the left-hand side when using the == or === comparisons.

If we're allowing assignments in conditions, we should at least mitigate "assignment instead of comparison" mistakes.

benjy’s picture

-1 for #34, just feels like we'd be introducing a new Drupalism.

Makes me ask the question, why do we even bother debating our own standards now, couldn't we just adopt PSR2 and follow the rest of the community.

David Strauss’s picture

Makes me ask the question, why do we even bother debating our own standards now, couldn't we just adopt PSR2 and follow the rest of the community.

I would love to adopt PSR2, but it explicitly does not address this issue:

There are many elements of style and practice intentionally omitted by this guide. These include but are not limited to:
[snip]
* Operators and assignment

effulgentsia’s picture

However, for everything that PSR2 does cover, if anyone knows of where Drupal PHP coding standards currently differ from PSR2, please file new issues for those items. It would be great to align with PSR2 as much as possible, or at least make explicit choices to deviate if/where we think we need to.

pfrenssen’s picture

Status: Active » Closed (won't fix)

This was discussed on the Coding Standards Committee call today. Since the opinions of the community are divided on this and there is no clear consensus we have decided to mark this as Won't Fix and to not formally disallow inline assignments.

effulgentsia’s picture

However, for everything that PSR2 does cover, if anyone knows of where Drupal PHP coding standards currently differ from PSR2, please file new issues for those items. It would be great to align with PSR2 as much as possible, or at least make explicit choices to deviate if/where we think we need to.

By the way, I just found #2645010: Switch to PSR-2 as of Drupal 9 which has some prior conversation about this. I still think specific issues to comply more with PSR2, even if not fully, are still valid to open and advocate for.

benjy’s picture

I would love to adopt PSR2, but it explicitly does not address this issue:

I wasn't suggesting PSR2 solved this issue directly, it was more of a general comment as to why we still debate many of these coding standards (or try to come up with new ones) when I presume a standard like PSR2 would solve many of our issues.

Thanks for the link effulgentsia, following that now.