Make WordPress Core

Opened 5 months ago

Last modified 4 months ago

#62043 new enhancement

Allow plugins and themes perform extra checks upon install/upgrade via filter

Reported by: leonidasmilossis's profile LeonidasMilossis Owned by:
Milestone: 6.8 Priority: normal
Severity: normal Version: 6.7
Component: Plugins Keywords: has-patch 2nd-opinion
Focuses: Cc:

Description

Currently, if a plugin (or a theme) wants to perform custom checks upon plugin install/upgrade, they need to either hook into the existing upgrader_source_selection filter or extend the Plugin_Upgrader class, which is suboptimal as it leads to code duplication and/or processes being repeated on every install/upgrade.

Ideally, we can have a more specific filter for more granularity in the process.

For example, say a plugin wants to check against a custom plugin header (potentially added via the extra_plugin_headers filter). By having a filter at the end of the Plugin_Upgrader::check_package() (or the respective one from Theme_Upgrader), after all the core checks have been passed, we can allow plugin authors to just add additional checks depending on their needs.

That filter will allow plugins to hook into it and return a WP_Error like the checks above it, to prevent installs/upgrade when a custom check fails.

All that would be especially useful for plugin add-ons that require a minimum installed version of their main plugin to be active before installed/upgraded.

Change History (15)

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


5 months ago
#1

  • Keywords has-patch added

#2 @LeonidasMilossis
5 months ago

With the above PR, the following filters are introduced:

  • plugin_upgrader_checked_package
  • theme_upgrader_checked_package

That way, a plugin can extend the checks performed by core upon plugin install, by hooking into the new filter, in order to check the minimum required version of the main plugin whenever an add-on is installed/upgraded, like:

<?php
add_filter( 'plugin_upgrader_checked_package', [ $this, 'check_requirement' ], 10, 2 );

public function check_requirement( $source, $info ) {
        $requires_xyz = ! empty( $info['Requires XYZ'] ) ? $info['Requires XYZ'] : false;
        if ( $requires_xyz === false ) {
                return $source;
        }

        if ( version_compare( XYZ_VERSION, $requires_xyz, '>=' ) ) {
                return $source;
        }

        $error = sprintf(
                __( 'The XYZ version on your site is %1$s, however the uploaded plugin requires %2$s.', 'xyz-addon' ),
                XYZ_VERSION,
                esc_html( $requires_xyz )
        );

        return new WP_Error(
                'incompatible_xyz_required_version',
                __( 'The package could not be installed because it\'s not supported by the currently installed XYZ version.', 'xyz-addon' ),
                $error
        );
}

#3 @SergeyBiryukov
5 months ago

  • Milestone changed from Awaiting Review to 6.7

#4 @afercia
5 months ago

Seems a good use case for two new filters to me.

Question: should the two new filters follow the naming convention of the hooks already present in the Plugin_Upgrader and Theme_Upgrader classes? They have the same names, not prefixed by plugin or theme. Then, a third param is passed to the hooks to determine whether the package type is 'plugin' or 'theme'. See:

  • upgrader_overwrote_package
  • upgrader_process_complete

@davidbaumwald commented on PR #7338:


5 months ago
#5

Looking at the current patch, I'm wondering if there should be any sort of guardrails on the $source that's returned from the new filter to ensure it's either a WP_Error or a (string) $path and that path is in the WP_CONTENT_DIR, WP_PLUGINS_DIR, or whatever path we'd want this constrained to. @afragen and I discussed this a bit ad WCUS CD.

Extra props @afragen.

@LeonidasMilossis commented on PR #7338:


5 months ago
#6

Personally, I'd rather not guard against $source being a part of any specific path, which would limit the third-party author's flexibility. The filter is added to grant them that flexibility, is there a reason I miss to restrain that?

#7 @davidbaumwald
4 months ago

  • Keywords 2nd-opinion added
  • Milestone changed from 6.7 to 6.8

I'd still like to have this reviewed by other committers and maintainers to see if there's any issue with doing this and what, if any, considerations should be made in the patch.

With 6.7 Beta 1 releasing in a few hours, this is being moved to 6.8 given recent momentum towards a resolution.

If any committer feels the remaining work can be resolved in time for Beta 1 and wishes to assume ownership, feel free to update the milestone accordingly.

@LeonidasMilossis commented on PR #7338:


4 months ago
#8

In any case, I've now added the guardrails of validating against being either a string or an error, which is already an improvement.

As for guarding against the path being in WP_CONTENT_DIR, we didn't do that before and the path was already filterable, so I'm not sure if we need to add this now. I'm interested in what you think @afragen.

I also pushed a change unifying the two new filters into one, as per feedback from @afercia .

@afragen commented on PR #7338:


4 months ago
#9

I need to review further but initially I don't think the final return should be WP_Error. It should be $source.

Is it even possible for $source to be a WP_Error going into this filter? I'm not sure that check is needed.

@LeonidasMilossis commented on PR #7338:


4 months ago
#10

Is it even possible for $source to be a WP_Error going into this filter?

Indeed, I dont think it's possible to be an WP_Error going into this filter. It _can_ be a WP_Error going out of it (which is basically the centerpiece of how we try to give plugin/theme authors a way for their extra custom checks).

I'm not sure that check is needed.

I'm totally fine with removing the check, I was mostly trying to follow the advice above and guardrail the $source.

Which is basically why I'm returning a WP_Error if coming out of the filter we get a non-string, non-WP_Error value, since that would indicate wrong usage of the filter. But maybe I missed the point of the advice?

In any case, thanks for responding :)

@afragen commented on PR #7338:


4 months ago
#12

I think the only way a WP_Error returns from the filter is is someone uses the filter to return it.

@afragen commented on PR #7338:


4 months ago
#13

If we move up the filter to just before the checks for is_wp_error() in the functions I think this can be greatly simplified.

@afragen commented on PR #7338:


4 months ago
#14

Honestly, I'm still trying to see why upgrader_pre_install won't work for you. I say this as someone who has extensively worked in code related to the upgrade/install component.

The use cases outlined in https://core.trac.wordpress.org/ticket/62043 could work very easily with that existing hook.

@LeonidasMilossis commented on PR #7338:


4 months ago
#15

I'm still trying to see why upgrader_pre_install won't work for you

Because in order to achieve what this new filter allows us to do, we need access to the get_plugin_data() values and, from what I can tell, those are not readily available in upgrader_pre_install.

To give an example of how that new filter could be used:

\add_filter( 'upgrader_checked_package', [ $this, 'check_required_version' ], 10, 3 );

public function check_required_version( $source, $info, $package_type ) {
	$requires_main_plugin_version = ! empty( $info['Requires Main Plugin'] ) ? $info['Requires Main Plugin'] : false;

	if ( ! $this->check_requirement( $requires_main_plugin_version  ) ) {
		$error = \sprintf(
			\__( 'The Main Plugin version on your site is %1$s, however the uploaded plugin requires %2$s.', 'text-domain' ),
			\MAIN_PLUGIN_VERSION,
			\esc_html( $requires_main_plugin_version )
		);

		return new WP_Error(
			'incompatible_main_plugin_version',
			\__( 'The package could not be installed because it\'s not supported by the currently installed Main Plugin version.', 'text-domain' ),
			$error
		);
	}

	return $source;
}

private function check_requirement( $required_version ) {
	if ( $required_version === false ) {
		return true;
	}

	return \version_compare( \MAIN_PLUGIN_VERSION, $required_version . '-RC0', '>=' );
}

(consider that the Requires Main Plugin info has been added via the extra_plugin_headers filter)

So, the above code in the main plugin would prevent installations/upgrades of its add-on, if that add-on had a required version for the main plugin.

I think the only way a WP_Error returns from the filter is is someone uses the filter to return it.

Yes, that exactly would be the way for that plugin to prevent an installation of an incompatible add-on version.

If we move up the filter to just before the checks for is_wp_error() in the functions I think this can be greatly simplified.

I'm failing to see how that could be the case 🤔

@afragen commented on PR #7338:


4 months ago
#16

How does the following not achieve your goal?

Using get_file_data() just grabs the header data from the plugin. Requesting specific data in $header_arr only returns that data.

add_filter(
	'upgrader_pre_install',
	function ( $bool, $hook_args ) {
		$header_arr = array( 'RequiresMainPlugin' => 'Requires Main Plugin' );
		$plugin_headers               = get_file_data( trailingslashit( WP_PLUGIN_DIR ) . $hook_args['plugin'], $header_arr );
		$requires_main_plugin_version = ! empty( $plugin_headers['RequiresMainPlugin'] ) ? $plugin_headers['RequiresMainPlugin'] : false;
		// Add check_requirement logic. Return WP_Error if fails.

		return $bool;
	},
	10,
	2
);
Note: See TracTickets for help on using tickets.