WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 10 months ago

#12375 closed enhancement (fixed)

HTTP class contains redundant defined() check

Reported by: TobiasBg Owned by: nacin
Milestone: 3.0 Priority: normal
Severity: normal Version:
Component: HTTP API Keywords: has-patch commit
Focuses: Cc:

Description

Line 554 of wp-includes/class-http.php contains a somewhat redundant logic:

if ( ! defined('WP_HTTP_BLOCK_EXTERNAL') || ( defined('WP_HTTP_BLOCK_EXTERNAL') && WP_HTTP_BLOCK_EXTERNAL == false ) )
The part after the
will only be executed if the constant is defined, thus it's overhead to check that again. Also checking constants for false (like WP_HTTP_BLOCK_EXTERNAL == false) is not good practice.

I have attached two patches, one that simply removes the redundant check and simplifies the WP_HTTP_BLOCK_EXTERNAL == false to ! WP_HTTP_BLOCK_EXTERNAL.
The second patch has the same logic, but has De Morgan's laws applied. I could decide what looks better :-)

Attachments (2)

12375-a.patch (605 bytes) - added by TobiasBg 6 years ago.
Patch to fix reduncancy and clean up comparison
12375-b.patch (607 bytes) - added by TobiasBg 6 years ago.
Same patch, but with De Morgan's laws applied

Download all attachments as: .zip

Change History (10)

@TobiasBg6 years ago

Patch to fix reduncancy and clean up comparison

@TobiasBg6 years ago

Same patch, but with De Morgan's laws applied

comment:1 @nacin6 years ago

I'm pretty sure we do this redundancy in multiple places. Used to be more, though I know we've eliminated some of them.

comment:2 @TobiasBg6 years ago

I grep'ed through the core source, but couldn't find any more occurances.

comment:3 @TobiasBg6 years ago

There are three or four more files that have two defined() checks in one line, but those check for different constants.

comment:4 @nacin6 years ago

Fair enough, looks like this is the last one then.

comment:5 @nacin6 years ago

  • Owner changed from dd32 to nacin
  • Status changed from new to accepted

comment:6 @nacin6 years ago

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

(In [13422]) Clean up redundant defined() check. Props TobiasBg fixes #12375

comment:7 @najamelan10 months ago

I just wanted to share an idea on the matter. Surely checking if it's defined is necessary to avoid php spewing warnings, however I don't like either of the patches. It seems the proper way to do this is to test somewhere early on in code that is garanteed to run on every request (wp-settings.php) like this:

if( ! defined( WP_HTTP_BLOCK_EXTERNAL ) )

    define( WP_HTTP_BLOCK_EXTERNAL, false );

Now it can be counted on to exist everywhere, and code can be simplified where it matters. I mean it's still extra code to work around the lack of an elegant matter in php to express this, but the test exists only once, far away from code logic.

It might seem strange when there is only one occurrence of this elsewhere, but that's right now, and might change later since there is quite some inconsistencies in the user interface when using WP_HTTP_BLOCK_EXTERNAL. It will throw errors on the plugins page because it can't connect to wordpress, it will still show you update as quick action even though this can no longer work, it will want to put a plugin count in the menu bar, on the admin-bar etc...

On top of that this could be done with all defines used in the core to clean things up all over.

comment:8 @TobiasBg10 months ago

This ticket was closed on a completed milestone. Please open a new ticket for your suggestion.
Note that the handling of constants has already changed a lot since this ticket was closed.

Note: See TracTickets for help on using tickets.