WordPress.org

Make WordPress Core

Opened 21 months ago

Closed 21 months ago

Last modified 18 months ago

#48965 closed enhancement (fixed)

Prefer strict PHP comparisons over loose PHP comparisons

Reported by: pikamander2 Owned by: SergeyBiryukov
Milestone: 5.4 Priority: normal
Severity: minor Version:
Component: General Keywords:
Focuses: coding-standards Cc:

Description

My understanding is that it's usually better to use strict comparisons (===, !==) rather than loose comparisons (==, !=) because they're more explicit and therefore less prone to silly edge case bugs.

Although it wouldn't affect the logic of the code in some cases, it could still be helpful for knowing that no implicit type conversion is happening and therefore making certain things faster to debug. Plus, the process of converting loose comparisons might reveal some dormant bugs in the code that would have been difficult to find otherwise.

So, here are some examples of what I'm proposing:

Change this:

if ( $url && $url != network_home_url( , 'http' ) ) :

to this:

if ( $url && $url !== network_home_url( , 'http' ) ) :

And this:

if ( $signup->domain . $signup->path == ) {

To this:

if ( $signup->domain . $signup->path === ) {

And this:

if ( SITECOOKIEPATH != COOKIEPATH ) {

to this:

if ( SITECOOKIEPATH !== COOKIEPATH ) {

I'd be willing to do a lot of the conversions myself, but first I wanted to see if that's something that we're interested in.

Attachments (7)

wp-activate.php.patch (902 bytes) - added by pikamander2 21 months ago.
wp-comments-post.php.patch (472 bytes) - added by pikamander2 21 months ago.
Use strict comparisons and yoda conditions
wp-cron.php.patch (839 bytes) - added by pikamander2 21 months ago.
Use strict comparisons and yoda conditions
wp-links-opml.php.patch (588 bytes) - added by pikamander2 21 months ago.
Use strict comparisons and yoda conditions
wp-login.php.patch (2.2 KB) - added by pikamander2 21 months ago.
Use strict comparisons and yoda conditions
wp-mail.php.patch (1.1 KB) - added by pikamander2 21 months ago.
Use strict comparisons and yoda conditions
wp-signup.php.patch (706 bytes) - added by pikamander2 21 months ago.
Use strict comparisons and yoda conditions

Download all attachments as: .zip

Change History (22)

#1 @pikamander2
21 months ago

Here are some past cases where converting loose comparisons (and using yoda conditions) seems to have been accepted:

Just wanted to check if that's still desired and what all I should watch out for while doing so.

Last edited 21 months ago by SergeyBiryukov (previous) (diff)

#2 @SergeyBiryukov
21 months ago

  • Focuses coding-standards added

@pikamander2
21 months ago

Use strict comparisons and yoda conditions

@pikamander2
21 months ago

Use strict comparisons and yoda conditions

@pikamander2
21 months ago

Use strict comparisons and yoda conditions

@pikamander2
21 months ago

Use strict comparisons and yoda conditions

@pikamander2
21 months ago

Use strict comparisons and yoda conditions

@pikamander2
21 months ago

Use strict comparisons and yoda conditions

#3 @pikamander2
21 months ago

@SergeyBiryukov - Here are some strict comparison and yoda condition updates to the WordPress PHP files in the root directory.

#4 @pikamander2
21 months ago

@SergeyBiryukov @garrett-eclipse - Any updates on this? I can go through more files if these changes look good.

#5 @SergeyBiryukov
21 months ago

  • Milestone changed from Awaiting Review to 5.4
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#6 @SergeyBiryukov
21 months ago

Just noting that wp-comments-post.php is already handled in [47028] / #49105.

#7 @SergeyBiryukov
21 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 47054:

Coding Standards: Use strict comparison and Yoda conditions in the root directory files.

Props pikamander2.
Fixes #48965.

#8 follow-up: @Otto42
18 months ago

Possible backwards compatibility break on this:

https://wordpress.org/support/topic/5-4-sending-post-via-email-results-in-no-title-for-the-post-title/

Specifically line 213 in wp-mail.php, where it detects a blank title. Seems that it wasn't really an empty string in some cases, but depended on it being empty enough.

#9 @pikamander2
18 months ago

@Otto42 - Yeah, that's not too surprising. My biggest fear with pushing these changes is that they might break production code.

On the flip side though, that's exactly why these kinds of changes are needed. Loose comparisons allow for a lot of poorly-defined behavior and weird interactions. The more comparisons we convert to strict, the easier it becomes to see what the function is expecting and can handle.

Pinging @SergeyBiryukov for thoughts.

Last edited 18 months ago by pikamander2 (previous) (diff)

#10 @pikamander2
18 months ago

I guess the simple fix here would be something like this:

<?php
if ( true === empty($post_title) )

@Otto42 - When applied to wp-mail.php, does that code fix your problem?

#11 @Otto42
18 months ago

I don't use wp-mail myself, I was just relaying from that support thread. You might consider asking that guy in the forum to change the code in his file to see if that solves his problem. If so, then it could be a reasonable fix for .1.

Edit: Also, I would just go with if ( empty($post_title) ) instead.

Last edited 18 months ago by Otto42 (previous) (diff)

#12 @MattyRob
18 months ago

This is because $post_title = xmlrpc_getposttitle( $content ); is returning NULL instead of an empty string which is therefore failing the strict comparison as newly implemented.

Perhaps in this instance it may be better to use:
if ( empty( $post_title ) ) {

Instead of:
if ( '' === $post_title ) {

THat's seem to work in my basic testing.

Last edited 18 months ago by MattyRob (previous) (diff)

#13 @MattyRob
18 months ago

Actually, thinking again using empty() would not allow for a post title of '0' (as in zero).

Perhaps we need a better check or revert the strict check

if ( '' === (string) $post_title ) {

#14 in reply to: ↑ 8 @SergeyBiryukov
18 months ago

Thanks everyone for highlighting the issue!

Since this ticket was closed on a completed milestone, I've created a new one for 5.4.1: #49853. Let's continue the discussion there.

This ticket was mentioned in Slack in #core by joyously. View the logs.


18 months ago

Note: See TracTickets for help on using tickets.