Make WordPress Core

Opened 10 months ago

Closed 9 months ago

Last modified 8 months ago

#60504 closed defect (bug) (fixed)

Plugin dependencies: Account for mu-plugin as dependency

Reported by: johnbillion's profile johnbillion Owned by: costdev's profile costdev
Milestone: 6.5 Priority: normal
Severity: normal Version: 6.5
Component: Upgrade/Install Keywords: needs-testing dev-feedback has-patch has-unit-tests
Focuses: Cc:

Description

I believe there could be a situation where a plugin on a site gets automatically deactivated despite its dependencies being met by a mu-plugin, and subsequently that plugin cannot be reactivated.

I'm using "WooCommerce" and "WooCommerce Extension" as examples for clarity.

  • A site is using WordPress 6.4 or earlier which does not contain the plugin dependencies feature
  • The site is using WooCommerce as a mu-plugin
  • The site is using WooCommerce Extension as a regular plugin
  • WooCommerce Extension gets updated to the latest version which adds the "Requires Plugins" header, and its update goes ahead as expected
  • At a later date the site gets updated to WordPress 6.5 which does include the plugin dependencies feature
  • WooCommerce Extension now has an unmet dependency and gets automatically deactivated, despite WooCommerce being in place on the site as a mu-plugin
  • The site is now missing a plugin that cannot be reactivated because its dependency exists as a mu-plugin

I've not had time to test this properly. There could be safeguards in place to prevent a site from getting to this point (eg by preventing the core update to 6.5), however the general problem of a mu-plugin fulfilling a dependency remains. If a user circumvents a safeguard by manually updating, they'll find themselves in the same situation where WooCommerce Extension gets deactivated and cannot be reactivated.

  • What safeguards are in place for this?
  • How would a user recover from it regardless? Can they?
  • Are mu-plugins accounted for at all within plugin dependencies? Should there be a means of forcibly activating a plugin with unmet dependencies?
  • Should plugins be automatically deactivated when their dependencies aren't met? It seems potentially destructive

Related: #60491

Partially related: #60457

Change History (25)

#1 @knutsp
10 months ago

Tricky, since dependency is annotated by the plugin slug (dir), but MU plugins don't necessary have such, only the name of the main file as unique id (among MU plugins).

Some thinkable solutions, as a start:

  1. Use the name of the MU plugin file ( WooCommerce.php as woocommerce ). Contra: MU plugins may be loaded by a file in the parent mu-plugins dir, not directly by WP.
  2. Require a textdomain equal to the slug, as for language packs. This is met by most widely used ones.
  3. Require plugins that can be recommended as MU plugins to filter the plugin list used for dependency checking, injecting their slug/textdomain into the array.
  4. Issue a code snippet added as MU plugin that does this filtering to rescue those who have got locked in the described situation. Contra: Needs ftp or file system access.
  5. Make WP 5.0 detect and warn if a major plugin is loaded as MU. This detection may be based on a limited list of constants, functions or classes. Contra: Will never be complete

#2 @costdev
10 months ago

  • Keywords dev-feedback added

@johnbillion Thanks for opening this ticket!

Indeed, Must-Use plugins were not considered valid dependencies in the Plugin Dependencies feature (going back aways in the discussions). However, you raise a valid issue if a site already has a plugin in its Must-Use plugins that becomes a dependency of a plugin in the Plugins repository.

So, it's possible that said plugins could just be moved to the mu-plugins folder along with the Must-Use dependency. However, Must-Use plugins may be installed by hosts/developers. Any other plugin can be installed by a site owner/developer/administrator, so that doesn't seem like a feasible approach.

So, currently, it looks like the following things would need to be done, at least:

Core patch

  1. Move the initialization of Plugin Dependencies until after do_action( 'muplugins_loaded' ); so that the Must-Use plugin's "loader" file can tell Core what its filename is via the wp_plugin_dependencies_slug hook. - Plugin Dependencies is no longer initialized in bootstrap.
  2. Ensure all calls to get_mu_plugins() and get_plugins() update the plugin_data option, rather than replace it (as is currently done in get_plugins(). - The plugin_data option no longer exists.
  3. Add a new $mustuse_plugins property to WP_Plugin_Dependencies.
  4. In WP_Plugin_Dependencies::get_plugins(), store get_mu_plugins() in the new $mustuse_plugins property, and ensure these are also merged into self::$plugins to avoid several array_merge( self::get_plugins(), self::$mustuse_plugins ) calls elsewhere.
  5. In WP_Plugin_Dependencies::sanitize_dependency_slugs(), add (\.php)? to the regex.
  6. In WP_Plugin_Dependencies::has_unmet_dependencies() make the foreach()'s internal condition:
    <?php
    
    if (
            ! isset( self::$mustuse_plugins[ $dependency ] ) &&
            ( false === $dependency_filepath || is_plugin_inactive( $dependency_filepath ) )
    ) {
            return true;
    }
    
  7. In WP_Plugin_Dependencies::convert_to_slug(), don't replace '.php' with '' anymore. Instead, just trim( $plugin_file, './' ) to remove potential ./file.php or ../../../file.php entries.
  8. Test updates.
  9. Additional tests to cover Must-Use plugins.

Site owners/Hosts/Developers

  1. As Core has no way to know the name of the "loader" file used for the Must-Use plugin, add a filter to your loader file to let Core know the filename.
<?php

add_filter(
        'wp_plugin_dependencies_slug',
        function ( $slug ) {
                static $muplugins = array(
                        'woocommerce'   => true,
                        'wordpress-seo' => true,
                        'akismet'       => true,
                );

                return isset( $muplugins[ $slug ] ) ? basename( __FILE__ ) : $slug;
        }
);

I've done the above and done some manual testing locally, and this seems to work fine, though I can't say I've fully tested the approach in such a short period of time.


This is likely going to need some discussion, so pinging @afragen, @pbiron, @azaozz and @desrosj to get in on this too if they have the time.

@knutsp Can you add some detail about how you think the Text Domain will need to play a part here? Thanks!

Last edited 9 months ago by costdev (previous) (diff)

#3 @johnbillion
10 months ago

WordPress VIP loads several plugins as mu-plugins (notably Jetpack and Query Monitor), it could be a good testing platform.

As Core has no way to know the name of the "loader" file used for the Must-Use plugin, add a filter to your loader file to let Core know the filename

Yeah this is going to be a sticking point, I've worked on many sites where there's one wp-content/mu-plugins/loader.php type file that loads all mu-plugins.

#4 follow-up: @afragen
10 months ago

@johnbillion a question. Does a plugin like WooCommerce, if loaded as an mu-plugin, show as an active plugin?

Perhaps anyone using this pattern should always use a loader plugin and the plugin would then normally be where it is, in wp-content/plugins. If the plugin isn't in wp-content/plugins how is it to be updated? Mu-plugins don't receive updates.

#5 in reply to: ↑ 4 @johnbillion
10 months ago

Replying to afragen:

Does a plugin like WooCommerce, if loaded as an mu-plugin, show as an active plugin?

The "loader" that you need to place in the root of the wp-content/mu-plugins directory does. It's listed under the "Must-Use" tab on the Plugins screen. It's not required to have a plugin header, but if it does then the info will appear there.

Mu-plugins are manually managed. They don't get checked for nor receive auto updates, and because the mu-plugin system in WordPress only looks at .php files in the root of the wp-content/mu-plugins directory it means that any plugin in a subdirectory needs a corresponding "loader" file which is arbitrary. Examples:

#6 @costdev
10 months ago

@johnbillion Yeah this is going to be a sticking point, I've worked on many sites where there's one wp-content/mu-plugins/loader.php type file that loads all mu-plugins.

A filter callback could have several conditions/in_array()/isset() in the if so that if any of the slugs for the MU plugins it loads are found, it passes basename( __FILE__ ) back.

I've updated the example above to include an isset().

Last edited 9 months ago by costdev (previous) (diff)

#7 @costdev
9 months ago

Note: With the changes needed for #60457, 1 and 2 of the Core patch in this comment will no longer be necessary.

I'll get a PR up for the Core patch suggested in that comment as soon as possible.

This ticket was mentioned in PR #6141 on WordPress/wordpress-develop by @costdev.


9 months ago
#8

  • Keywords has-patch has-unit-tests added

Previously, Must-Use plugins were not taken into account as dependencies. This meant that dependents could only be installed if their dependencies were installed as "normal" plugins.

This adds support for detecting Must-Use plugins as dependencies.

Note:
Must-Use plugins are loaded by a "loader" file in wp-content/mu-plugins/, such as woocommerce-loader.php. WordPress Core does not know that this file is connected to the dependency plugin.

To tell WordPress Core that the loader file should be considered the dependency plugin's "main" file, a filter must be used in the loader file.

Example:

// Load the dependency plugin's file.
require_once __DIR__ . '/woocommerce/woocommerce.php';

add_filter(
        'wp_plugin_dependencies_slug',
        static function ( $slug ) {
                if ( 'woocommerce' === $slug ) {
                        // Tell WordPress Core to consider this file to be the dependency's "main" plugin file.
                        $slug = basename( __FILE__ );
                }
                return $slug;
        }
);

#9 @costdev
9 months ago

PR 6141 adds support for Must-Use plugins and is ready for testing.

#10 follow-up: @peterwilsoncc
9 months ago

For a new feature, I don't think requiring developers add code to declare the dependency is overly helpful. The onus should be on WordPress to handle these things rather than plugin developers.

Given I raised this issue on the plugin repo two years ago, it's a little frustrating to be dealing with it in a rush during the beta period.

#11 @costdev
9 months ago

  • Keywords 2nd-opinion added; needs-testing has-patch has-unit-tests removed

For others' reference, here's the issue that Peter raised on the Feature Plugin's repository in 2022: https://github.com/WordPress/wp-plugin-dependencies/issues/2

Noting Dion's comment in particular as indicating reasons why Must-Use plugins weren't considered valid dependencies during the feature's development.

Now that this has been raised in Trac, I think it's fine that we discuss the merits/needs of supporting Must-Use plugins again. Even if Must-Use plugins continue not to be supported, at least we'd have considered and reached that decision twice. Adding 2nd-opinion.

Should there be consensus to support Must-Use plugins as dependencies, PR 6141 is one implementation that demonstrates how Must-Use plugin support could be added and may aid discussion by indicating possible limitations such as the loader file name having no connection to the actual plugin it loads, without the use of the wp_plugin_dependencies_slug filter demonstrated in the PR's description.

Removing has-patch, has-unit-tests and needs-testing so as not to indicate that Must-Use plugin support has been agreed.

#12 in reply to: ↑ 10 @knutsp
9 months ago

  • Keywords needs-testing has-patch has-unit-tests added; 2nd-opinion removed

Replying to peterwilsoncc:

For a new feature, I don't think requiring developers add code to declare the dependency is overly helpful. The onus should be on WordPress to handle these things rather than plugin developers.

How do you suggest finding or defining the plugin slug of a directly loaded MU-plugin? Naming the PHP file is only a convention, not a rule (I use index.php as main). Only for those loaded by a loader file, one can assume the directory basename is the slug, as for normal plugins.

Installing and updating MU plugins is not supported by WP Admin UI. You need skills beyond a plain site admin. There will be reasons for chosing to load a publicly available, by UI installable, plugin as a must-use, but then not to for a dependent plugin.

Directly loading complex plugins with subdirectories is not really supported (dirname conflicts) so you need a loader file, and that is written by a programmer, or pretender.

Plugin developers may add a snippet in readme.

To require a loader file with an embedded filter to make it fill it's role as dependency for normal plugins, and when well documented, seems fine to me.

Future: WP comes with a default MU loader file, ready to edit down to a line (like wp-config.php), with filters built in for an array of plugins by [ slug => dirname/filename.php, ].

#13 @costdev
9 months ago

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

This ticket was mentioned in Slack in #core-upgrade-install by costdev. View the logs.


9 months ago

#15 @afragen
9 months ago

It seems the issue isn't that some devs use mu-plugins, but that some devs add normal dot org plugins into the mu-plugins folder. This means they must add a loader to require the main plugin file. This will load the plugin but the plugin will return a false value for is_plugin_active(), will not properly show as installed, and makes updating the plugins a completely manual process.

This is non-standard behavior even though WordPress has the ability to "make it work". The problem as @johnbillion points out is that mu-plugins are managed manually. The primary issue is that once installed and active in the mu-plugins folder these plugins are no longer capable of receiving updates. This will cause an undue burden on the site admin who must now manually update the plugin and will likely just miss the update as they would receive no indication that an update is available. It makes integration as a plugin dependency difficult, and it may leave a security update missing.

As @knutsp suggested this could be handled by a default MU loader file. @costdev and I have created a MU Loader that only requires the plugin file name (eg. woocommerce/woocommerce.php) to be entered as an array variable in the loader file. This MU Loader will forcibly load the designated plugin within the "normal plugin directory" and filters the active_plugins option so it is detected as active.

This isn't a huge barrier to entry for devs as they must already create a loader file. Further, I believe this solves all issues that have been outlined such as returning a truthy value for is_plugin_active() and showing as installed.

All that is required is the normal installation of the plugin and the addition of the plugin file as an array variable in the MU Loader file. In the case of WooCommerce, simply adding woocommerce/woocommerce.php is sufficient to load WooCommerce as an mu-plugin. If WooCommerce is declared somewhere as a dependency this is also accounted for and WooCommerce also shows as an active plugin.

The mu-loader.php ensures that the plugin shows as both active and installed making use as a plugin dependency seamlessly and doesn't require any addition modifications to core to use plugins loaded as mu-plugins as dependencies. This can also handle multiple plugins installed as mu-plugins.

Using this type of mu-loader, mu-plugins can work as dependencies without needed to modify core.

This ticket was mentioned in Slack in #core-upgrade-install by afragen. View the logs.


9 months ago

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


9 months ago

#18 @johnbillion
9 months ago

At Human Made we have a package for doing the same thing, except it supports defining the list of mu-plugins either via a PHP constant or via a config entry in composer.json for Composer based projects. Essentially the same thing, minus filtering the active_plugins value, which I don't yet have an opinion about.

A default mu-plugin loader could be a good idea. Would it need to consider handling conditional logic that is present in, for example, the WordPress VIP mu-plugins? Conditionally defining the array of mu-plugins to load would mean centralising a lot of the logic that is currently separated into multiple mu-plugin loaders.

#19 follow-up: @johnbillion
9 months ago

[57658] removed the auto-deactivation of plugins with unmet dependencies.

This now means upon updating to WordPress 6.5 an active plugin that has a dependency that is fulfilled by a mu-plugin will remain active (in the same way an active plugin with a dependency that is not fulfilled will remain active).

It remains that an inactive plugin that has a dependency that is fulfilled by a mu-plugin will be prevented from being activated, as its dependency is considered unfulfilled. Bearing in mind that the introduction of a mu-plugin is a manual process I think this is less of a pressing concern, but it's my opinion that we do need a way of allowing a mu-plugin to be recognised as a dependency.

A default mu-loader could be a good idea but we'd need to decide where its configured list of mu-plugins comes from, it might have to come from another mu-plugin. I'm not a big fan of filtering the list of active_plugins to lie about what's active as that could come back to bite, especially if other plugins are reading that option and acting upon it.

#20 in reply to: ↑ 19 @desrosj
9 months ago

Replying to johnbillion:

It remains that an inactive plugin that has a dependency that is fulfilled by a mu-plugin will be prevented from being activated, as its dependency is considered unfulfilled. Bearing in mind that the introduction of a mu-plugin is a manual process I think this is less of a pressing concern, but it's my opinion that we do need a way of allowing a mu-plugin to be recognised as a dependency.

Given this, I'm inclined to say that we can consider this an edge case acceptable for 6.5. It still needs to be addressed, but because of the already manual nature of mu-plugins, I think it's a reasonable nuance to leave unaddressed for now.

#21 @costdev
9 months ago

  • Milestone changed from 6.5 to 6.6
  • Severity changed from critical to normal

Reducing the severity to normal and tentatively punting to 6.6.

As this is a defect (bug) ticket related to the 6.5 cycle, it can be pulled back into the 6.5 milestone during 6.5 RC if we want to action it during this cycle.

#22 @johnbillion
9 months ago

  • Milestone changed from 6.6 to 6.5

Perhaps we should close this ticket as fixed for 6.5 and open a new one for 6.6. The main concern (auto deactivation of a plugin where its dependencies are met by a mu-plugin) has been fixed.

#23 @johnbillion
9 months ago

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

Closing as fixed in [57658]. I've opened #60692 for the follow up.

This ticket was mentioned in Slack in #core-test by oglekler. View the logs.


9 months ago

This ticket was mentioned in Slack in #core-upgrade-install by afragen. View the logs.


8 months ago

Note: See TracTickets for help on using tickets.