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
- Search core for uses of the loose comparison operators
==
and!=
, and identify any patterns in usage. - Come up with a plan to group changes to strict comparison, including adding test coverage and/or error handling where appropriate.
- 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.
Comment | File | Size | Author |
---|---|---|---|
#46 | 1530652-======.46.patch | 1.37 MB | Haza |
#35 | 1530652-======.35.patch | 1.37 MB | Haza |
#33 | 1530652-======.33.patch | 1.37 MB | Haza |
#31 | 1530652-======.31.patch | 1.37 MB | Haza |
#28 | 1530652-======.patch | 1.38 MB | tim.plunkett |
Comments
Comment #1
tim.plunkettLeft out of this patch:
The last two are our files, I think, but they are both minified, so I wasn't 100% sure about touching them.
Comment #3
Lars Toomre CreditAttribution: Lars Toomre commentedIf we are changing '==' to '===' throughout core, shouldn't we also be doing the same with '!=' to '!=='?
Comment #4
tim.plunkett@Lars Toomre, yes, but Drupal won't even install yet, so one thing at a time.
Comment #5
tim.plunkettOkay, tracked it down, there were two lines in core/includes/module.inc that needed to be casted.
Comment #6
tim.plunkettHere's s/!=/!== as well. I was bored waiting for the test to pass.
Comment #8
tim.plunkettOkay, 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!
Comment #10
tim.plunkettOkay, I think I found the one exception in the lib patch.
Comment #11
nod_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.
Comment #12
tim.plunkettWell the patch that passes in #10 has no JS, but when I reroll the others, I will be sure to leave them out.
Comment #13
gregglessubscribe 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?.
Comment #14
tim.plunkettWhen 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.
Comment #15
tim.plunkettI found one instance of a needed cast, so I'm trying this again.
It was in drupalCreateRole, which would cause a LOT of fails.
Comment #17
tim.plunkettLast one cleaned up 890 fails with one change, and here's another, in
DrupalWebTestCase::handleForm()
this time.Comment #19
tim.plunkett1818 more fails down!
Now casting $user->uid in _drupal_session_read().
Comment #21
klausiBefore 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.
Comment #22
tim.plunkettI 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 :)
Comment #23
tim.plunkettAlso, just out of curiosity (I used D7 since D8 includes parts of Symfony):
Comment #24
nod_Just'd like to remind that there are still js files caught in the fight in the -modules.patch :)
Comment #25
tim.plunkettOh whoops, I was just excluding /misc. I'm holding off for now anyway.
Comment #26
tim.plunkettNot worth it.
Comment #27
tim.plunkettThis shouldn't have been closed, it should still be done.
Comment #27.0
tim.plunkettAdapted into an issue summary
Comment #28
tim.plunkettJust for funsies.
Comment #29
tim.plunkettComment #31
HazaTrying to reroll patch against HEAD, let's see if it is applicable or not.
Comment #33
HazaComment #35
HazaInstall should, at least, pass now.
Comment #37
tim.plunkettThanks @Haza!
Comment #39
tim.plunkettComment #41
tim.plunkett/me sighs
Missed a conflict resolution marker <<<<<<<<
Comment #43
tim.plunkett@Haza I didn't try to undo your work, but without any interdiffs in your comments, I don't know what to change back :(
Comment #44
HazaYep, 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.
Comment #45
chx CreditAttribution: chx commentedSorry 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.
Comment #46
HazaOk, 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 :)
Comment #48
HazaShouldn'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...
Comment #54
GoZ CreditAttribution: GoZ at Iosan, Barbe-Rousse commentedLet's reborn this as meta issue.
Comment #55
GoZ CreditAttribution: GoZ at Iosan, Barbe-Rousse commentedComment #56
GoZ CreditAttribution: GoZ at Iosan, Barbe-Rousse commentedComment #57
GoZ CreditAttribution: GoZ at Iosan, Barbe-Rousse commentedComment #60
GoZ CreditAttribution: GoZ at Iosan, Barbe-Rousse commentedComment #61
GoZ CreditAttribution: GoZ at Iosan, Barbe-Rousse commentedComment #62
GoZ CreditAttribution: GoZ at Iosan, Barbe-Rousse commentedComment #63
GoZ CreditAttribution: GoZ at Iosan, Barbe-Rousse commentedComment #64
cilefen CreditAttribution: cilefen at Institute for Advanced Study commentedThere is a coding standards issue about this.
Comment #65
larowlanPlease 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.
Comment #66
GoZ CreditAttribution: GoZ at Iosan, Barbe-Rousse commented@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.
Comment #67
larowlanI 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:
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.
Comment #68
cilefen CreditAttribution: cilefen at Institute for Advanced Study commentedI 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.
Comment #72
deulenko CreditAttribution: deulenko commentedComment #73
deulenko CreditAttribution: deulenko commentedComment #74
deulenko CreditAttribution: deulenko commentedComment #75
deulenko CreditAttribution: deulenko commentedComment #76
novchuk.v CreditAttribution: novchuk.v commentedComment #77
xjmThank 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!
Comment #78
xjmReplacing the "remaining tasks" list with a rough outline for what actually needs to happen here (which is not per-module issues).
Comment #79
novchuk.v CreditAttribution: novchuk.v commented@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.
Comment #81
quietone CreditAttribution: quietone as a volunteer commentedPoint 3 of the remaining tasks is to add a coding standards rule, so adding tag.
Comment #82
andypost