Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#34936 closed enhancement (fixed)

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

Reported by: danielbachhuber's profile danielbachhuber Owned by: jorbin's profile jorbin
Milestone: 4.6 Priority: normal
Severity: normal Version:
Component: Bootstrap/Load Keywords: wp-cli dev-feedback
Focuses: Cc:

Description (last modified by 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.

Attachments (6)

34936.diff (2.0 KB) - added by jorbin 8 years ago.
shortinit.34936.1.diff (544 bytes) - added by danielbachhuber 8 years ago.
34936.2.diff (1.7 KB) - added by kraftbj 8 years ago.
Minor whitespace trimming
34936.3.diff (3.5 KB) - added by jorbin 8 years ago.
34936.4.diff (3.7 KB) - added by DrewAPicture 8 years ago.
34936.5.diff (3.7 KB) - added by DrewAPicture 8 years ago.

Download all attachments as: .zip

Change History (47)

This ticket was mentioned in Slack in #cli by danielbachhuber. View the logs.


8 years ago

#2 in reply to: ↑ description ; follow-up: @jorbin
8 years ago

  • [ ] 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?

#3 in reply to: ↑ 2 ; follow-up: @danielbachhuber
8 years ago

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:

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

Sure:

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.

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


8 years ago

#5 @jorbin
8 years ago

  • Milestone changed from Future Release to 4.6

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

#6 @danielbachhuber
8 years ago

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() ?

#7 in reply to: ↑ 3 @rmccue
8 years ago

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.

This ticket was mentioned in Slack in #cli by danielbachhuber. View the logs.


8 years ago

#9 @jorbin
8 years ago

Related: #36819

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


8 years ago

#11 @amandato
8 years ago

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(); }

#12 @jorbin
8 years ago

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.

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


8 years ago

@jorbin
8 years ago

#14 @jorbin
8 years ago

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.

#15 @DrewAPicture
8 years ago

@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.

#16 @DrewAPicture
8 years ago

  • Keywords has-patch added

#17 @danielbachhuber
8 years ago

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()

#18 @jorbin
8 years ago

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.

#19 follow-up: @ocean90
8 years ago

[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.).

#20 in reply to: ↑ 19 ; follow-ups: @jorbin
8 years ago

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?

#21 in reply to: ↑ 20 @DrewAPicture
8 years ago

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.

Last edited 8 years ago by DrewAPicture (previous) (diff)

#22 in reply to: ↑ 20 ; follow-up: @ocean90
8 years ago

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.

#23 in reply to: ↑ 22 @jorbin
8 years ago

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.

#24 @kraftbj
8 years ago

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
8 years ago

Minor whitespace trimming

#25 @danielbachhuber
8 years ago

  • Description modified (diff)

#26 @chriscct7
8 years ago

  • Owner set to jorbin
  • Status changed from new to assigned

@jorbin
8 years ago

#27 @jorbin
8 years ago

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

@DrewAPicture
8 years ago

#28 @DrewAPicture
8 years ago

@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.

#29 follow-up: @ocean90
8 years ago

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
8 years ago

#30 in reply to: ↑ 29 @DrewAPicture
8 years ago

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.

#31 @ocean90
8 years ago

  • Keywords commit added; dev-feedback removed

#32 @jorbin
8 years ago

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

#33 follow-up: @jorbin
8 years ago

  • Keywords dev-feedback added; has-patch commit removed

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.

#34 in reply to: ↑ 33 @danielbachhuber
8 years ago

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.

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


8 years ago

#36 @danielbachhuber
8 years ago

  • Description modified (diff)

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


8 years ago

#38 @jorbin
8 years ago

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

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.

This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.


8 years ago

#40 @DrewAPicture
8 years ago

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.

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


8 years ago

Note: See TracTickets for help on using tickets.