Reconcile wp-settings-cli.php with wp-settings.php

[danielbachhuber]

WP-CLI uses a custom wp-settings-cli.php to load WordPress, instead of wp-settings.php. While I appreciate the ​historical justifications for the bootstrap process, it would be much better if WP-CLI could use wp-settings.php because, occasionally, there are ​substantial problems arising from needing to maintain a fork.

Before we can deprecate wp-settings-cli.php though, we'll need to reconcile the modifications made to the bootstrap process. I've ​put together a branch to compare the current differences.

Here's a summary:

• [x] Core: Provide a way of forcefully bypassing wp_maintenance() so that WP-CLI can continue to operate when .maintenance exists.
• [x] WP-CLI: Load WP_CLI\Utils just before wp-settings.php is called, instead of in wp-settings-cli.php. ​#wp-cli-2277
• [x] Core: Provide a way of overloading wp_debug_mode(), which isn't called at all in wp-settings-cli.php.
• [x] Core: Provide a way of forcefully skipping loading wp-content/advance-cache.php, which historically has caused problems with WP-CLI.
• [ ] Core: Provide one or more hooks to make runtime modifications before and after require_wp_db() is called, and after wp_start_object_cache() is called. Will need further discovery and testing.
• [ ] Core: Provide a way of overloading wp_not_installed(), which outputs headers and can call die()
• [x] Core: Provide a way to skip the loading of specific plugins and/or themes.
• [x] Address how we should handle WP-CLI's current exclusion of wp-includes/vars.php

We can split these into sub-tickets as the discussion evolves. I look forward to the feedback.

[jorbin]

• [ ] Core: Provide a way of forcefully bypassing wp_maintenance() so that WP-CLI can continue to operate when .maintenance exists.

Could you override the $upgrading global and always force it to be more than 10 minutes old? This will cause wp_maintenance() to always return even if . maintenance exists?

• [ ] Core: Provide a way of overloading wp_debug_mode(), which isn't called at all in wp-settings-cli.php.

What is the rationional for skipping wp_debug_mode() in wp-cli? If it is to turn display errors off, could a constant be added to https://core.trac.wordpress.org/browser/trunk/src/wp-includes/load.php#L304 ?

• [ ] Core: Provide a way of forcefully skipping loading wp-content/advance-cache.php, which historically has caused problems with WP-CLI.

Can you point out the wp-cli tickets that explain the historical issues to better understand this?

• [ ] Core: Provide a way to skip the loading of specific plugins and/or themes.

the active_plugins option could be filtered for plugins.

For themes, I assume you mean skipping the active theme? What do you replace it with? Are the stylesheet_directory and template_directory not sufficiant?

[danielbachhuber]

Replying to jorbin:

Could you override the $upgrading global and always force it to be more than 10 minutes old? This will cause wp_maintenance() to always return even if . maintenance exists?

Where would WP-CLI do this, though? Here's the relevant code in wp_maintenance():

global $upgrading;

include( ABSPATH . '.maintenance' );
// If the $upgrading timestamp is older than 10 minutes, don't die.
if ( ( time() - $upgrading ) >= 600 )
	return;

There's no way to stomp the $upgrading global after .maintenance has been included.

What is the rationional for skipping wp_debug_mode() in wp-cli? If it is to turn display errors off, could a constant be added to https://core.trac.wordpress.org/browser/trunk/src/wp-includes/load.php#L304 ?

We originally removed wp_debug_mode() so we could conditionally enable WP_DEBUG at runtime with a --debug flag:

• ​https://github.com/wp-cli/wp-cli/commit/372c8f15f40664f41e5e6870c5cb6f4b71c78a24
• ​https://github.com/wp-cli/wp-cli/issues/177

Can you point out the wp-cli tickets that explain the historical issues to better understand this?

Sure:

• ​https://github.com/wp-cli/wp-cli/pull/164
• #22683
• ​https://github.com/wp-cli/wp-cli/commit/8f3b6601377f4e7230a1275833ada39bfa182c50

the active_plugins option could be filtered for plugins.

For themes, I assume you mean skipping the active theme? What do you replace it with? Are the stylesheet_directory and template_directory not sufficiant?

It could be worth trying the filtering approach. It looks like WordPress is smart enough not to stomp $wp_filter if it's already defined before wp-includes/plugin.php, so WP-CLI could inject its callbacks on the global.

[jorbin]

@danielbachhuber and I are going to work on this in 4.6

[danielbachhuber]

It could be worth trying the filtering approach. It looks like WordPress is smart enough not to stomp $wp_filter if it's already defined before wp-includes/plugin.php, so WP-CLI could inject its callbacks on the global.

One problem with this approach is that it's a breaking change to WP-CLI's current behavior. WP-CLI currently supports checking to see whether a plugin is active, while skipping the loading of one or more plugins.

Before

$ wp plugin status wp-api --skip-plugins=wp-api
Plugin wp-api details:
    Name: WP REST API
    Status: Active
    Version: 2.0-beta13
    Author: WP REST API Team
    Description: JSON-based REST API for WordPress, originally developed as part of GSoC 2013.

After

$ wp plugin status wp-api --skip-plugins=wp-api
Plugin wp-api details:
    Name: WP REST API
    Status: Inactive
    Version: 2.0-beta13
    Author: WP REST API Team
    Description: JSON-based REST API for WordPress, originally developed as part of GSoC 2013.

Could we add late filters to wp_get_active_network_plugins() and wp_get_active_and_valid_plugins() ?

[rmccue]

Replying to danielbachhuber:

It could be worth trying the filtering approach. It looks like WordPress is smart enough not to stomp $wp_filter if it's already defined before wp-includes/plugin.php, so WP-CLI could inject its callbacks on the global.

This is how test_add_filter works in the unit testing harness, so it should be pretty reliable.

---

Generally speaking, I think we should have more hooks more early. Given that plugin.php is almost entirely self-contained (we could split the file if needed), I'd be in favour of loading it earlier than it currently, moving it up to the first block of includes (load.php and default-constants.php). We could then add an action to wp_debug_mode for that, filter wp_maintenance, etc. It would result in a small amount more code loaded before advanced-cache kicks in (I imagine negligible in any case), but be super helpful for edge cases and advanced tooling.

[jorbin]

Related: #36819

[amandato]

Re: Core: Provide a way of forcefully bypassing wp_maintenance() so that WP-CLI can continue to operate when .maintenance exists.

The proposed filter I requested in this ticket #36960 with the following if in the .maintenance file would solve this problem.

<?php
if( !( defined('WP_CLI') && WP_CLI ) ) { $upgrading = time(); }

[jorbin]

In 37588:

Bootstrap/Load: Load plugin.php earlier in wp-settings.php

In order to allow non-web initializations of WordPress (such as through wp-cli) to modify things like the check for maintenance mode, plugins.php and the associated functions must be available much earlier. The use of these functions earlier than the loading of plugins is not recommended in most use cases.

Fixes #36819. See #34936.
Props jorbin, danielbachhuber for documentation.

[jorbin]

Above patch adds three filters to solve:

As these filters all run before plugins are loaded, I've marked them @ignore so they don't go on the docs site.

[ ] Core: Provide a way of forcefully bypassing wp_maintenance() so that WP-CLI can continue to operate when .maintenance exists.
[ ] Core: Provide a way of overloading wp_debug_mode(), which isn't called at all in wp-settings-cli.php.
[ ] Core: Provide a way of forcefully skipping loading wp-content/advance-cache.php, which historically has caused problems with WP-CLI.

Would love a docs review from @DrewAPicture and others such @danielbachhuber.

[DrewAPicture]

@jorbin: On the DocBlock summaries, we should be describing what's filterable. In this case, something like:

• "Filters whether to bypass the maintenance mode check."
• "Filters whether to bypass the debug mode check."
• "Filters whether to bypass loading the advanced-cache.php drop-in."

Additionally, as these are effectively what we've in the past referred to as "boolean filters", it would be useful to describe what happens when a truthy value is returned to the hooks.

Finally, I don't think these warrant @ignore tags – they're still very valid hooks and should be discoverable, just maybe not for plugin developers, which we can certainly make clear. Any plugins trying to leverage them are explicitly voiding the warranty by virtue of saying so in the DocBlock descriptions.

[danielbachhuber]

shortinit.34936.1.diff adds a shortinit action with which I can address:

[ ] Core: Provide a way of forcefully skipping loading wp-content/advance-cache.php, which historically has caused problems with WP-CLI.
[ ] Core: Provide one or more hooks to make runtime modifications before and after require_wp_db() is called, and after wp_start_object_cache() is called. Will need further discovery and testing.
[ ] Core: Provide a way of overloading wp_not_installed(), which outputs headers and can call die()

[jorbin]

In 37626:

Introduce filters for skipping parts of the bootstrap process

Non web interfaces with WordPress (such as wp-cli) need to be able to bypass certain checks in the bootstrap process. This introduces three new filters to allow for those checks to be skipped.

1. Provides a way of forcefully bypassing wp_maintenance().
2. Provides a way of forcefully bypassing wp_debug_mode(). See ​https://github.com/wp-cli/wp-cli/issues/177
3. Provide a way of forcefully skipping loading wp-content/advance-cache.php. See ​https://github.com/wp-cli/wp-cli/pull/164

These filters should not be used by plugins (in fact, they run before plugins are loaded, so they can't be used by plugins). In general, they should only be used in non-web interactions with WordPress.

See #34936.
Props jorbin, DrewAPicture.

[ocean90]

[37626]: I'm not really a fan of the naming and the location of these filters.

• The This filter should *NOT* be used by plugins. warning should be removed. I really don't see the need for it and it gets ignored anyway. We shouldn't add the filter in the first place if we're worried about that plugins can use it
• bypass_advanced_cache: I think this should be replaced with enable_advanced_cache:

/**
 * Filters whether to enable advanced caching.
 *
 * @since 4.6.0
 *
 * @param bool $enable Whether to enable advanced caching. Defaults to the value of `WP_CACHE`.
 */
if ( apply_filters( 'enable_advanced_ caching', WP_CACHE )  ) {
	// For an advanced caching plugin to use. Uses a static drop-in because you would only want one.
	_backup_plugin_globals();
	WP_DEBUG ? include( WP_CONTENT_DIR . '/advanced-cache.php' ) : @include( WP_CONTENT_DIR . '/advanced-cache.php' );
	_restore_plugin_globals();
}

Similiar filters are enable_live_network_counts, enable_press_this_media_discovery, enable_post_by_email_configuration or enable_edit_any_user_configuration.

• bypass_debug_mode / bypass_maintenance_mode: Why are the filters inside of the function? If you want to bypass something shouldn't it be placed outside?
• bypass_debug_mode is a bit misleading because you can't really disable the debug mode. There are a few places in core which check the value of WP_DEBUG and these checks can't be disabled with the filter. Is the filter needed at all? What's the issue if it can't be disabled? Shouldn't WP_DEBUG be set to false if you don't want to run something in debug mode?
• bypass_maintenance_mode: I think we should go with the enable_ naming here too:

  /**
   * Filters whether to enable the maintenance_check.
   *
   * @since 4.6.0
   *
   * @param bool $enable Whether to the maintenance_check. Default true.
   */
  if ( apply_filters( 'enable_maintenance_check', true )  ) {
  	wp_maintenance();
  }
  

Docs: Please note that each line should end with a period and the filter parameters should include a variable (@param type $variable Description.).

[jorbin]

Replying to ocean90:

[37626]: I'm not really a fan of the naming and the location of these filters.

• The This filter should *NOT* be used by plugins. warning should be removed. I really don't see the need for it and it gets ignored anyway. We shouldn't add the filter in the first place if we're worried about that plugins can use it

They are loaded before plugins are loaded, so we could change it to they can not be used by plugins. Do you have an alternative suggestion for enabling the ability to skip these parts of the bootstrap process? The alternative suggestion was a WP_CLI constant but we decided a filter looked better than sprinkling a constant that seems like it would be more likely to be abused.

• bypass_advanced_cache: I think this should be replaced with enable_advanced_cache:

I chose bypass instead of enable as I felt that that action that a user of the filter is to bypass something rather than to unenable. I don't feel super strong about this though so if you do, I'm open to changing it.

• bypass_debug_mode / bypass_maintenance_mode: Why are the filters inside of the function? If you want to bypass something shouldn't it be placed outside?

I wanted to avoid littering wp-settings with wrapping if statements around each of the functions.

• bypass_debug_mode is a bit misleading because you can't really disable the debug mode. There are a few places in core which check the value of WP_DEBUG and these checks can't be disabled with the filter. Is the filter needed at all? What's the issue if it can't be disabled? Shouldn't WP_DEBUG be set to false if you don't want to run something in debug mode?

As noted in the link, it's to allow --debug to be used to set debug display settings. Perhaps a better name and documentation would demonstrate that better. Ideas?

Docs: Please note that each line should end with a period and the filter parameters should include a variable (@param type $variable Description.).

Is the $variable really needed when there is no variable used?

[DrewAPicture]

Replying to jorbin:

Replying to ocean90:

Docs: Please note that each line should end with a period and the filter parameters should include a variable (@param type $variable Description.).

Is the $variable really needed when there is no variable used?

Yes, the parser expects it.

See also changes needed for the filter summaries noted at the top of comment:15:

@jorbin: On the DocBlock summaries, we should be describing what's filterable. In this case, something like:

• "Filters whether to bypass the maintenance mode check."
• "Filters whether to bypass the debug mode check."
• "Filters whether to bypass loading the advanced-cache.php drop-in."

Edit: If for whatever reason you're avoiding describing the filters as filters, we still need to use third-person singular verbs in the hook doc summaries. See the ​#language section in the inline docs standards for more on that.

[ocean90]

Replying to jorbin:

They are loaded before plugins are loaded, so we could change it to they can not be used by plugins.

That makes probably more sense. Although, if WP-CLI can do it then a plugin in form of auto_prepend_file (see [37207]) can do it too.

I chose bypass instead of enable as I felt that that action that a user of the filter is to bypass something rather than to unenable. I don't feel super strong about this though so if you do, I'm open to changing it.

I don't feel that strong too, but bypass is currently only used in inline docs and once in a function: media_upload_html_bypass().

As noted in the link, it's to allow --debug to be used to set debug display settings. Perhaps a better name and documentation would demonstrate that better. Ideas?

Okay. Maybe the name should include "error reporting" since that's what the function handles.

---

For WP_CACHE I've found https://core.trac.wordpress.org/browser/trunk/src/wp-settings.php?rev=37626&marks=283-284#L281. That should probably be covered by the filter too, although it's unlikely that the function exists if advanced-cache.php isn't loaded.

[jorbin]

Replying to ocean90:

Replying to jorbin:

They are loaded before plugins are loaded, so we could change it to they can not be used by plugins.

That makes probably more sense. Although, if WP-CLI can do it then a plugin in form of auto_prepend_file (see [37207]) can do it too.

I don't think a plugin could use auto_prepend_file since that needs to be defined in php.ini and if you are changing that setting there, I would hope you know what you are doing. I'm guessing a plugin that says "Go change your php.ini file" would not be looked highly upon and a server setup to allow the web user to chang it also feels like a bad idea.

I chose bypass instead of enable as I felt that that action that a user of the filter is to bypass something rather than to unenable. I don't feel super strong about this though so if you do, I'm open to changing it.

I don't feel that strong too, but bypass is currently only used in inline docs and once in a function: media_upload_html_bypass().

Fair. I'll change it.

As noted in the link, it's to allow --debug to be used to set debug display settings. Perhaps a better name and documentation would demonstrate that better. Ideas?

Okay. Maybe the name should include "error reporting" since that's what the function handles.

How about "enable_wp_debug_setting_error_reporting" with a default value of true?

---

For WP_CACHE I've found https://core.trac.wordpress.org/browser/trunk/src/wp-settings.php?rev=37626&marks=283-284#L281. That should probably be covered by the filter too, although it's unlikely that the function exists if advanced-cache.php isn't loaded.

The function exists check should be sufficient. The current wp-settings-cli.php has that code and it isn't a problem. @danielbachhuber ​created a comparison between the cli version of wp-settings and the core version that others may find valuable as well.

[kraftbj]

Extremely minor, there's some whitespacing that snuck in this and the [37588]. Didn't touch the docs tweaks since y'all are actively discussing it :)

[kraftbj]

Minor whitespace trimming

[jorbin]

@DrewAPicture @ocean90 What do you think of the changes in the above patch? Do they alleviate your concerns?

[DrewAPicture]

@jorbin 34936.3.diff​ is a great improvement.

In 34936.4.diff​ I've made a few key adjustments:

• Typically we don't describe what filters can "allow" but in the case of boolean filters like this, it's kind of the point. We're explicitly filtering whether to allow something to happen. Thus, I've adjusted the summaries to reflect those allowances
• I've also fixed some hook doc syntax and added more complete descriptions for parameters.
• I've adjusted the hook names to more closely reflect their intent (see my first point about "allowances"): enable_maintenance_mode_checks, enable_wp_debug_mode_checks, enable_loading_advanced_cache.

[ocean90]

34936.4.diff​ looks good so far. I'd only change enable_maintenance_mode_checks to enable_maintenance_mode since there are no checks after this filter.

[DrewAPicture]

Replying to ocean90:

34936.4.diff​ looks good so far. I'd only change enable_maintenance_mode_checks to enable_maintenance_mode since there are no checks after this filter.

Good call. I completely glossed over the fact that maintenance.php is not in fact a core file (heh), but a drop-in.

See 34936.5.diff​, where I've adjusted the summary for the maintenance hook to match the style of the hook summary for advanced-cache.php. I also standardized the hook names for the drop-ins:

• enable_loading_maintenance_dropin
• enable_loading_advanced_cache_dropin

I think that makes things pretty clear.

[jorbin]

In 37690:

Bootstrap/Load. Adjust filters added in [37626].

These adjustments improve the documentation for the filters and adjust the names make them more consistent with other filters already in core.

See #34936.
Props DrewAPicture, ocean90, jorbin

[jorbin]

I reverted to enable_maintenance_mode since the dropin is only a small part and this actually has no effect specificly on the dropin. Otherwise, thanks @DrewAPicture and @ocean90.

The final piece that we need to do is to add in the new action that @danielbachhuber's patch suggests. I'm not sure I like the name shortinit for the action. The purpose of the action as descibed in one of the ​linked chats is to provide a place to address $wpdb->error, APC cache check, and wp_not_installed().

I'm not sure the best name for it. I'm thinking pre_shortinit_check but would love to know others ideas.

[danielbachhuber]

Replying to jorbin:

The final piece that we need to do is to add in the new action that @danielbachhuber's patch suggests.

After landing a few pull requests against wp-settings-cli.php, I think we might actually need a couple of actions (or to retool WP-CLI's existing behavior). It would be worth chatting again in Slack before committing any actions.

[jorbin]

The remaining pieces were all able to be accomplished using hooks already in place.

​wp-cli updated today to include wp-settings.php instead of it's fork. Closing this as fixed.

[DrewAPicture]

In 38021:

Docs: Fix a typo in the hook doc description for the enable_loading_advanced_cache_dropin run-time filter.

See #34936. See #37318.