I ran code sniffer on a project recently and noticed that this error was produced "The value of a comparison must not be assigned to a variable" for this bit of code

if ($result = $mysqli->query($query)) {

.

Is code readability the only reason that this would be considered a bad practice?

    In this case, the code analysis tool's warning is bogus. It applies when you do things like this:

    if ($x = 1)

    when you actually wanted to write:

    if ($x == 1)

    (Though the warning is still rather poorly worded, in my opinion.)

    You could try to suppress the warning by writing:

    if (($result = $mysqli->query($query))) {

      = assigned to variable
      == compare

        Actually Dagon, I'm familiar with assigning vs comparing. What's interesting about this bit of code is it comes directly from the php manual and it works for the purpose intended. I was just curious whether or not there was something I was missing regarding this type of variable assignment.

          Hey, I'm curious ... does the suppression method suggested work?

          Also, you may be way over my head, so sorry for sticking my neck out here ... what laserlight is saying is that the code sniffer's parser isn't smart enough to realize that "->" isn't the same operator as ">" ... correct? And, therefore, the result should be ignored.

          "->" is NOT a comparison operator; "==", "===", "!=", "<>", "!==", "<", ">", ">=" and "<=" are comparison operators. This is a bug in the sniffer; I'd contact Greg Sherwood or his organization @squizlabs.com directly; I doubt they want to keep releasing bugs into the wild 🙂

            dalecosp wrote:

            Also, you may be way over my head, so sorry for sticking my neck out here ... what laserlight is saying is that the code sniffer's parser isn't smart enough to realize that "->" isn't the same operator as ">" ... correct? And, therefore, the result should be ignored.

            No, the tool is not smart enough to recognise that the assignment is intentional. The warning is that the assignment could have been a mistake, and a comparison was intended.

            Well, at least that is how I interpret it. As I noted, "The value of a comparison must not be assigned to a variable" is poorly worded, and I guess you could consider it a bug because there is no comparison involved in the code snippet to begin with (other than the implied boolean comparison of the if statement's conditional). Something like "A comparison may have been incorrectly written as an assignment to a variable" may be clearer.

              laserlight;10968333 wrote:

              No, the tool is not smart enough to recognise that the assignment is intentional. The warning is that the assignment could have been a mistake, and a comparison was intended.

              Well, at least that is how I interpret it. As I noted, "The value of a comparison must not be assigned to a variable" is poorly worded, and I guess you could consider it a bug because there is no comparison involved in the code snippet to begin with (other than the implied boolean comparison of the if statement's conditional). Something like "A comparison may have been incorrectly written as an assignment to a variable" may be clearer.

              Ah, my OOP is quite weak; how is this (exactly), a comparison? The list of comparison operators doesn't include this one, but as I see it this is a conditional test of a boolean return value from a database method ... ?

                dalecosp wrote:

                Ah, my OOP is quite weak; how is this (exactly), a comparison? The list of comparison operators doesn't include this one, but as I see it this is a conditional test of a boolean return value from a database method ... ?

                I think that you are harping on the -> operator, causing you to miss the point. Let's wrap the call:

                function foo($mysqli, $query)
{
    return $mysqli->query($query);
}

                Now, assuming that the code sniffer is consistent, this will result in the exact same warning from the code sniffer:

                if ($result = foo($mysqli, $query)) {

                What you were saying in post #5 is that a warning of "The value of a comparison must not be assigned to a variable" for something like this is wrong because -> is not a comparison operator:

                $x = $y->z($bar);

                But that implies that a warning of "The value of a comparison must not be assigned to a variable" for something like this makes sense:

                $x = $y > $z;

                However, such a warning would still be bogus, because assigning the result of a comparison to a variable is a valid technique.

                  Thankies! I get it now 🙂

                  I do still wonder if anyone who uses the Sniffer ought to report it as a bug, though. With this thread for reference 😃 "Must not" is very strong language, and could have people pulling out what hairs they have left for little reason.

                    Write a Reply...