Also totally legal, it is generally stupid to do an assignment in an if() statement (for instance, it is not possible in Ada, for good reasons).

Writing:

  $a = $b;
  if ($a) { ...

won't kill anybody, but writing:

  if ($a = $b != 0) { ...

is not going to work going to work as expected by 99% of the programmers (okay, maybe only 60%, still!)

As a C/C++ programmer, I can tell you that we run in the exact same problem and I still see tons of people doing it wrong... and getting bugs like shown here.

I don't really know how the parser works, if we can easily detect an if() statement, then checking if an equal operator appears between the () should be easy.

Thank you.
Alexis Wilke

Comments

dawehner’s picture

dawehner
Primary language German
commented

But we have to exclude stuff like:

if ($result = functioncall()) {
  
}

Its used everywhere.

AlexisWilke’s picture

AlexisWilke commented

Dereine,

Yes. And writing that on two lines is safer and easier to read:

$result = functioncall();
if ($result) {
  ...
}

The one where I would agree we'd have a "problem" is the while() with a database call as in:

while ($row = db_fetch_array($result)) {
  ...
}

And in this case it is annoying to remove the ability...

snufkin’s picture

snufkin commented

Well a useful trick is to do comparisons like this:

if ('test' == $testval) {
 ...
}

and if you entered just one '=' php will simply throw an error, because you cant assign value to strings. I quite like the compact form of ($result = blabla), i do it quite often in cache checks, or db result checks.

AlexisWilke’s picture

AlexisWilke commented

snufkin,

In this example,

  if ($a = $b != 0) { ...

It would become this:

  if (0 != $a = $b) { ...

which then would work properly.

Now, how many programmers, of all those who program modules on Drupal will write such a statement? (i.e. the 2nd one.)

The idea for coder is to help silly billies like me to find bugs, I think... 8-)

hefox’s picture

hefox commented

Assignments in control structures is perfectly legal in and, tmk (considering it's used in core a lot in the $bleh = blah() way), follows coding standards fine. I'd won't fix this if I was maintainer of coder.

To my understanding of coder, it's not suppose to be adding *new* coding standards, but instead enforcing existing drupal coding standards, ie to have coder add this check, first need to have drupal coding standards have this. Coder tough love?

douggreen’s picture

douggreen commented
Version: 6.x-2.x-dev » 7.x-2.x-dev

I added a similar rule to the 7.x-2.x druplart review (because I have made this suggestion in my latest yet unpublished article. I agree that there shOuld be some valid exceptions and would appreciate your help in getting this right.

AlexisWilke’s picture

AlexisWilke commented

It is cool to hear that you want to apply this after all. 8-)

I'm not using Drupal 7.x at all so I won't be able to help in the implementation and I'm not too sure where D7.x is at in terms of using the assignment operator "in the wrong place".

I would test the if(), while() and for() second term at the start.

The only one that was a problem in D6 was the while($row = db_query()) { ... }. But D7 changed the way they handle database queries so that may not even be a concern anymore and such loops can be fixed using something like this:

  for (;;) {
    $row = db_query(...);
    if (!$row) {
      break;
    }
    ... // do work on $row
  }

A way to determine what should be accepted would be to first do a run on Drupal Core and see what appear in all those if(), while(), and for(;;) so we can determine what is considered "okay" and what is not.

Thank you.
Alexis Wilke

P.S. Also it is a little more writing, the compilers know better than you how to optimize and writing everything inside an if() or while() does not help make the code run faster. So the for(;;) {...} example I present here is as fast as a while($row = db_query()).

salvis’s picture

salvis
he/his
commented

@AlexisWilke:

You're going way overboard with this abomination.

Just because you want to flag 

  if ($a = $b != 0) { ...

is no reason to bark at perfectly fine constructs such as 

if ($result = functioncall()) {
  ...
}

and

while ($result = expression) {
  ...
}

I completely disagree with your #2:

Yes. And writing that on two lines is safer and easier to read:

$result = functioncall();
if ($result) {
  ...
}

It is harder to write, harder to read, it obscures the logic, it wastes precious space on the screen, in the eye, and in the mind.

We're all free to have our pet peeves, but forcing others to comply with yours is a different matter.

A big -1!

douggreen’s picture

douggreen commented

I am adding some form of check to an optional review, not the style review. And I've changed the output to clearly flag which review each warning comes from. I think that people who want more suggestions will run this review, just like people who want more suggestions now run coder tough love. I was hoping to bring coder tough love under the coder distribution (I haven't talked to @Morbus yet). If there's an uproar over this, we can certainly leave the optional reviews as separate projects, but IMO, distributing them all as once is helpful.

douggreen’s picture

douggreen commented
Assigned: Unassigned » douggreen
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Issue summary: View changes
Status: Active » Closed (won't fix)

Coder 7.x is frozen now and will not receive updates. Coder 8.x-2.x can be used to check code for any Drupal version, Coder 8.x-2.x also supports the phpcbf command to automatically fix conding standard errors. Please check if this issue is still relevant and reopen against that version if necessary.