WordPress.org

Make WordPress Core

Opened 9 months ago

Closed 8 months ago

#37707 closed enhancement (fixed)

Change the inclusion of 'plugins.php' to require_once

Reported by: jorbin Owned by: jorbin
Milestone: 4.7 Priority: normal
Severity: normal Version:
Component: Bootstrap/Load Keywords:
Focuses: Cc:

Description

This grew out of a twitter conversation between @andy and I.

Right now, there are a small number of cases where access to the plugins API before the bootstrap process makes sense. WP-CLI is one of these, and when auto-prepended files are needed. Right now, that means directly interacting with the plugin global variables. If we switch to require_once,

Common wisdom says that require is faster than require_once. But common wisdom can be wrong, so I tested it.

The time difference between require and require_once seems to be about 0.01 milliseconds. I think this slight slowdown is worth the flexibility that is gained. Local testing with an opcache enabled showed that there is essentially no difference with an opcache.

Attachments (1)

37707.diff (510 bytes) - added by jorbin 9 months ago.

Download all attachments as: .zip

Change History (11)

@jorbin
9 months ago

#1 @dd32
9 months ago

This gets a +1 from me, although it feels a little odd at first to expect that part of WordPress may be loaded before WordPress is loaded :)

In the past I've been told that some opcaches have had bugs where stat (or validate_timestamps) configuration options are disabled and the usage of require_once() would bypass it, I don't see that as a good reason against this change though. It could be done with a conditional include on if ( ! function_exists( 'add_filter' ) ) if that was an issue.

#2 follow-up: @jorbin
9 months ago

I tested with function_exists as well, and the speed was comparable. I do worry that might make the plugin system essentially pluggable and I hate that idea. Though someone that does that is really voiding the warranty.

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


8 months ago

#4 in reply to: ↑ 2 @pento
8 months ago

Replying to jorbin:

I do worry that might make the plugin system essentially pluggable and I hate that idea. Though someone that does that is really voiding the warranty.

I've heard this sentiment previously. :-)

+1 on commit, because I like to keep future me on his toes.

#5 @jorbin
8 months ago

  • Owner set to jorbin
  • Resolution set to fixed
  • Status changed from new to closed

In 38282:

Bootstrap/Load: Include Plugin API via require_once

Currently, auto-prepended files need to directly modify the globals in order to add filters and actions. This is a bad idea. Globals are bad. You should never directly interact with the Plugin globals.

Fixes #37707.

#6 @danielbachhuber
8 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Now that /wp-includes/plugin.php requires the WPINC constant to be set, it's not possible to require /wp-includes/plugin.php before wp-settings.php is loaded. WPINC is set at the beginning of wp-settings.php

#7 @dd32
8 months ago

Cross-referencing [38571] so the above comment makes sense.

We could simply use require dirname(__FILE__) . '/class-wp-hook.php'; there instead.

#8 @pento
8 months ago

  • Keywords has-patch removed

Yeah, that seems the most sane solution to me.

@danielbachhuber: Could you confirm that this works for you?

#10 @pento
8 months ago

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

In 38589:

Bootstrap: Use dirname() when loading class-wp-hook.php from plugin.php.

To allow plugin.php to be loaded before the rest of WordPress is loaded, it cannot rely on WordPress constants, such as ABSPATH and WPINC.

Instead, we can assume that class-wp-hook.php will be in the same directory as plugin.php, so dirname( __FILE__ ) will give us the correct path to load from.

Props pento, dd32.
Fixes #37707.

Note: See TracTickets for help on using tickets.