Require the strict comparison operator === for comparing strings [#2795691]

Due to the weakly typed nature of PHP comparing two values might sometimes give unexpected results.

Example:

  if ($this->getOperation() == 'delete') {
    // Delete everything.
  }

If for some reason $this->getOperation() would return TRUE then a boolean comparison will be performed, and everything will be unexpectedly deleted.

A better practice is to use the strict comparison operator for strings:

  if ($this->getOperation() === 'delete') {
    // Delete everything.
  }

I propose to add this to the coding standards, so we can detect and avoid potential bugs like these.

Comments

Comment #1

6 September 2016 at 11:16

pfrenssen created an issue. See original summary.

Comment #2

dawehner

German

CreditAttribution: dawehner as a volunteer and at Chapter Three commented 8 September 2016 at 06:53

I like the idea, but I'm wondering whether we should first go with the smaller step, which would be to strongly encourage people to use the === operator.

Comment #3

sardara CreditAttribution: sardara at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commented 8 September 2016 at 08:59

I really like this proposition. Using strict comparison will also avoid type juggling, so it's a bit faster too.

Comment #4

slootjes CreditAttribution: slootjes commented 23 January 2018 at 20:26

+1 to prevent type juggling which can cause bugs. For sensitive (ie. usernames, tokens) use hash_equals() instead as it prevents timing attacks.

Comment #5

bobbygryzynger

he/him

English

United States

CreditAttribution: bobbygryzynger commented 6 June 2018 at 15:01

+1 I would like to see this added to the standard as well. It produces clearer reading code and cuts out a slew of potentially difficult to debug edge-cases. The relevant sniff to add would be Squiz.Operators.ComparisonOperatorUsage.

Comment #6

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 6 June 2018 at 17:19

This seems like a good idea, but unless either the left-hand or right-hand side of the operator is a literal string, it would be difficult to enforce with a Coder sniffing rule. I guess that is often the case, but not always. I think we normally don't adopt new coding standards that are difficult or impossible to enforce with Coder rules.

Comment #7

dawehner

German

CreditAttribution: dawehner as a volunteer and at Chapter Three commented 17 June 2018 at 22:00

It feels even by documenting we could make a sort of positive impact, simply by rising the awareness of the problem.

Comment #8

effulgentsia CreditAttribution: effulgentsia at Acquia commented 24 July 2018 at 19:01

but unless either the left-hand or right-hand side of the operator is a literal string, it would be difficult to enforce with a Coder sniffing rule

Any reason not to broaden the scope of this proposal to always require the strict operator? I.e., remove "for comparing strings" from the issue title?

Comment #9

pfrenssen

Sofia

CreditAttribution: pfrenssen at European Commission and European Union Institutions, Agencies and Bodies commented 25 July 2018 at 11:33

@effulgentsia this is an interesting idea. After all if you are comparing values you know the type in advance.

There are certainly use cases for the non-strict comparison operator: functions might return different types depending on the result of their action. For example a function might return NULL or FALSE instead of an actual value if an error occurred, or if there are no results. In these cases you can use == as a shortcut. Returning different types (eg FALSE on failure) is a very bad practice but unfortunately PHP code is rife with this.

I think that in these cases it is arguably better to rewrite these using strict comparisons. This will be more verbose but it will also more clearly express what is being done and that the type casting is intentional.

Comment #10

slootjes CreditAttribution: slootjes commented 25 July 2018 at 12:45

@pfrenssen In those cases you could also consider using empty() instead of a weak typed comparison which will pass on the same kind of values (FALSE/NULL/0,etc).