Problem/Motivation

PHP is weakly typed and this can lead to bugs like https://bugs.php.net/bug.php?id=54547.
In addition, it causes our code to be less explicit than it could be; core is littered with 0/1 where it should be FALSE/TRUE, and strings where they should be integers.

Proposed resolution

Adopt a policy of always using the strongly typed comparison operators, and use them.
It might even expose a bizarre edge-case bug or two.

Create a child issue for each module. Take care of each conditions, only real strict comparison should be converted.
Ex:

// No needs changes because we potentially compare two differents types.
$nid = 1;
if ( $node->id() == $nid) {

}

// Needs changes, we are sure to have string in both.
if ( $node->id() === $node->original->id()) {

}

Remaining tasks

  1. Search core for uses of the loose comparison operators == and !=, and identify any patterns in usage.
  2. Come up with a plan to group changes to strict comparison, including adding test coverage and/or error handling where appropriate.
  3. Add a coding standards rule to check for loose comparison and enable when core is compliant.

User interface changes

N/A

API changes

Policy change: never use == or !=, always === or !==

Original report by chx

I wanted to get rid of them at least since Paris (remember my presentation?) but now that https://bugs.php.net/bug.php?id=54547 is filed and turns out there's more than just filling a presentation and a blog with the quirkiness but there are dire security weaknesses as well (search for ximaz in the bug report), it's time to use === and cast if necessary. It won't be necessary mostly.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
741.09 KB

Left out of this patch:

  • core/vendor/Symfony/*
  • core/misc/ui/*
  • core/misc/jquery.js
  • core/misc/jquery.form.js
  • core/misc/jquery.cookie.js
  • core/misc/farbtastic/farbtastic.js
  • core/lib/Drupal/Component/Archiver/ArchiveTar.php

The last two are our files, I think, but they are both minified, so I wasn't 100% sure about touching them.

Status: Needs review » Needs work

The last submitted patch, drupal-1530652-1.patch, failed testing.

Lars Toomre’s picture

If we are changing '==' to '===' throughout core, shouldn't we also be doing the same with '!=' to '!=='?

tim.plunkett’s picture

@Lars Toomre, yes, but Drupal won't even install yet, so one thing at a time.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
741.1 KB

Okay, tracked it down, there were two lines in core/includes/module.inc that needed to be casted.

tim.plunkett’s picture

FileSize
939.81 KB

Here's s/!=/!== as well. I was bored waiting for the test to pass.

Status: Needs review » Needs work

The last submitted patch, drupal-1530652-6.patch, failed testing.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett
Status: Needs work » Needs review
FileSize
561.12 KB
17.94 KB
129.34 KB

Okay, trying to split them up a bit here, to see which one gets closest.

This is me clinging to hope that I don't have to split this up into 100+ patches.

Sorry testbot!

Status: Needs review » Needs work

The last submitted patch, drupal-1530652-8-modules.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
17.93 KB

Okay, I think I found the one exception in the lib patch.

nod_’s picture

Status: Needs review » Needs work

That's a great thing :D but can you let out all the JS files please?

we don't have tests and it requires much more than a simple replace, some code rely on javascript's goofy coercion. We're working on it: #1428534: Use === and !==

(edit) sorry didn't mean to change the status, but still applies.

tim.plunkett’s picture

Status: Needs work » Needs review

Well the patch that passes in #10 has no JS, but when I reroll the others, I will be sure to leave them out.

greggles’s picture

Title: Drupal uses == » Switch from == to === to prevent mistakes and make brute force password attacks harder

subscribe and updated title about why might care.

The word "dire" in the OP is slightly dramatic. This is an important thing to fix, but D7+ still have brute force prevention and D6 contrib handles that as well. How do I protect my site against Brute Force Attacks?.

tim.plunkett’s picture

When we're under thresholds, I'll reroll this for core/lib/Drupal since its easy and rather small, but after that this can become a meta issue and new ones will be opened for each module/include.

tim.plunkett’s picture

I found one instance of a needed cast, so I'm trying this again.
It was in drupalCreateRole, which would cause a LOT of fails.

Status: Needs review » Needs work

The last submitted patch, drupal-1530652-15-modules.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
572.63 KB

Last one cleaned up 890 fails with one change, and here's another, in DrupalWebTestCase::handleForm() this time.

Status: Needs review » Needs work

The last submitted patch, drupal-1530652-17-modules.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
573.84 KB

1818 more fails down!
Now casting $user->uid in _drupal_session_read().

Status: Needs review » Needs work

The last submitted patch, drupal-1530652-19-modules.patch, failed testing.

klausi’s picture

Before writing patches we should have an actual discussion. What do we want to fix? Do we want to get rid of every single "==" in our code base? Do we also want to get rid of "!=="? The PHP bug linked by chx only indicates that we should be careful when we compare numbers with "==", no?

We desperately need an issue summary here, I'm just confused.

tim.plunkett’s picture

I added one. It could be expanded more. This may turn out to be an intellectual exercise, but the only one harmed thus far is the testbot :)

tim.plunkett’s picture

Also, just out of curiosity (I used D7 since D8 includes parts of Symfony):

$:~/checkout/symfony$ grep -nr " == " * | wc -l
     283
$:~/checkout/symfony$ grep -nr " === " * | wc -l
    1241
$:~/checkout/symfony$ grep -nr " != " * | wc -l
      97
$:~/checkout/symfony$ grep -nr " !== " * | wc -l
     665

$:~/checkout/drupal7$ grep -nr " == " * | wc -l
    1929
$:~/checkout/drupal7$ grep -nr " === " * | wc -l
     313
$:~/checkout/drupal7$ grep -nr " != " * | wc -l
     546
$:~/checkout/drupal7$ grep -nr " !== " * | wc -l
     222
nod_’s picture

Just'd like to remind that there are still js files caught in the fight in the -modules.patch :)

tim.plunkett’s picture

Oh whoops, I was just excluding /misc. I'm holding off for now anyway.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Status: Needs work » Closed (won't fix)

Not worth it.

tim.plunkett’s picture

Status: Closed (won't fix) » Needs work

This shouldn't have been closed, it should still be done.

tim.plunkett’s picture

Issue summary: View changes

Adapted into an issue summary

tim.plunkett’s picture

Issue summary: View changes
FileSize
1.38 MB

Just for funsies.

tim.plunkett’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 28: 1530652-======.patch, failed testing.

Haza’s picture

Status: Needs work » Needs review
FileSize
1.37 MB

Trying to reroll patch against HEAD, let's see if it is applicable or not.

Status: Needs review » Needs work

The last submitted patch, 31: 1530652-======.31.patch, failed testing.

Haza’s picture

Status: Needs work » Needs review
FileSize
1.37 MB

Status: Needs review » Needs work

The last submitted patch, 33: 1530652-======.33.patch, failed testing.

Haza’s picture

Status: Needs work » Needs review
FileSize
1.37 MB

Install should, at least, pass now.

Status: Needs review » Needs work

The last submitted patch, 35: 1530652-======.35.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.37 MB
1.6 KB

Thanks @Haza!

Status: Needs review » Needs work

The last submitted patch, 37: 1530652-strict-equals.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.39 MB

Status: Needs review » Needs work

The last submitted patch, 39: 1530652-strict-equals-39.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.39 MB

/me sighs
Missed a conflict resolution marker <<<<<<<<

Status: Needs review » Needs work

The last submitted patch, 41: 1530652-strict-equals-41.patch, failed testing.

tim.plunkett’s picture

@Haza I didn't try to undo your work, but without any interdiffs in your comments, I don't know what to change back :(

Haza’s picture

Yep, I forgot to add that. My fault.

I'll try to have a look in that tomorrow (europe here).
I'll start from your last patch and this time, I won't forgot to make an interdiff.

chx’s picture

Sorry to disappoint but doing this in a single big patch is not going to be feasible I am afraid this probably needs a sandbox and then broken up in a series of patches.

Haza’s picture

Status: Needs work » Needs review
FileSize
1.37 MB

Ok, not an interdiff here, but it is "just" a rerolled patch against HEAD and it integrate the changes from #37. Drupal is able to install we admin is able to log in.

@chx : You're right. Patch is so big, and will break everytime, it is almost not feasible.

Btw, i'm curious about how many test fails we'll get :)

Status: Needs review » Needs work

The last submitted patch, 46: 1530652-======.46.patch, failed testing.

Haza’s picture

Shouldn't we change this issue to a meta issue, and create (as a starting point) a child issue for each core module, to do the changes. And them, at the end, do the change for core itself ?

Just a guess...

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

GoZ’s picture

Title: Switch from == to === to prevent mistakes and make brute force password attacks harder » [META] Switch from == to === to prevent mistakes and make brute force password attacks harder
Version: 8.5.x-dev » 8.6.x-dev
Issue summary: View changes

Let's reborn this as meta issue.

GoZ’s picture

Issue summary: View changes
GoZ’s picture

Issue summary: View changes
GoZ’s picture

Issue summary: View changes

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

GoZ’s picture

Issue summary: View changes
GoZ’s picture

Issue summary: View changes
GoZ’s picture

Issue summary: View changes
GoZ’s picture

Issue summary: View changes
cilefen’s picture

There is a coding standards issue about this.

larowlan’s picture

Please be aware of the issue scope guidelines, specifically the section on files

I appreciate the effort to rebirth this as a meta issue, but the drain it puts on committers/reviewer resources could be counter-productive.

GoZ’s picture

@larowlan i have a lot of examples of fixes which should be made on core an submodules. Each time we use a META issue to patch each module. I agree making one big patch is a pain. Also making as many patch as module in a same issue will be hard to review. That's why we use the META issue approach.

I don't understand why do you talk about counter-production. Drupal code and everyone work has always been a reference for quality code and coding respect standards. One of the purpose of this issue is to keep Drupal in this way.

larowlan’s picture

I have no issues with the concept of the patch, but rather with the approach of doing it one-per-module, this takes committer and review time away from other issues.

E.g. #3073996: Replace Loose comparison by Strict comparison in ban module is a two line patch, but the effort to commit it is non-trivial.

Our scope guidelines state:

Very large patches are extremely difficult to create and review. For this reason, it can seem like a good idea to create a series of patches like "Fix coding standards errors in module A", "Fix coding standards errors in module B", etc. However, this can turn out to be the worst of both worlds: the patch mixes multiple different concepts and contexts, while also leaving the codebase in an incomplete state.

So I think this issue needs to be returned to its original title from comment #53 and above, and the issues for each module need to be combined and patched here. Those issues can then be marked closed (works as designed) and we can provide issue credit for all those who've worked on them to this issue.

cilefen’s picture

I agree with @larowlan about scope. We will be facing dozens of small patches based on this meta issue. Moreover, how can we enforce this standard in future commits? Can this be automated, perhaps with slevomat/coding-standard? The coding standards issue remains open: #2795691: Require the strict comparison operator === for comparing strings. I realize that issue is about strings, but even that aspect is under discussion.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

deulenko’s picture

Issue summary: View changes
deulenko’s picture

Issue summary: View changes
deulenko’s picture

Issue summary: View changes
deulenko’s picture

Issue summary: View changes
novchuk.v’s picture

Issue summary: View changes
xjm’s picture

Thank you for your work on cleaning up Drupal core's code style! @deulenko, please read the previous comments about issue scope guidelines.

In order to fix core coding standards in a maintainable way, all our coding standards issues should be done on a per-rule basis across all of core, rather than fixing standards in individual modules or files. If the change is too large to be made in a single patch, it should be made on a per-concept basis, not a per-module basis.

For background information on why we usually will not commit coding standards fixes that aren't scoped in that way, see the core issue scope guidelines, especially the note about coding standards cleanups. That document also includes numerous suggestions for scoping issues including documentation coding standards cleanups.

Contributing to the overall plan above will help ensure that your fixes for core's coding standards remain in core the long term.

In the case of this issue, changing from loose to strict comparison could also change the behavior of of some code, so I think it needs to be done more carefully, with test coverage additions in some cases.

I am going to close the child issues of this meta because several committers have explained that we cannot accept patches on a per-module basis. Please come up with a new plan for these improvements. Thanks!

xjm’s picture

Issue summary: View changes

Replacing the "remaining tasks" list with a rough outline for what actually needs to happen here (which is not per-module issues).

novchuk.v’s picture

@xjm
If I'm not mistaken, you mean that we only need to change according to a certain pattern? For example, comparing the some entity id in the first patch and some string to another patch etc.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

Issue tags: +Coding standards

Point 3 of the remaining tasks is to add a coding standards rule, so adding tag.

andypost’s picture

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.