WordPress.org

Make WordPress Core

Opened 18 months ago

Closed 7 months ago

Last modified 7 months ago

#22378 closed enhancement (fixed)

Add ability to hijack plugin install/upgrade downloads

Reported by: rmccue Owned by: dd32
Milestone: 3.7 Priority: normal
Severity: normal Version: 3.4
Component: Upgrade/Install Keywords: has-patch commit
Focuses: Cc:

Description

Problem

Many plugins have their own updating system. For paid plugins, this can involve
sending authentication/authorization headers. At the moment, to implement this,
certain parts of the plugin upgrader system have to be duplicated to replace
parts of the system, such as the downloading method. In addition, the package
download URL has to be a valid URL. Some systems may wish to use alternate URLs
apart from HTTP/FTP-based URLs.

The code for this is shared between the single plugin upgrader and the bulk
plugin upgrader. While the upgrader class can be replaced fairly easily with
existing filters for single plugins, it's not possible to do this for the bulk
upgrader (especially when multiple systems want to do this), requiring falling
back to a low-level filter like http_request_args, ensuring that one only
hooks in after the admin_action_update-selected action has failed.

Solution

Add a filter to hijack the download system before it can act at all. This allows
replacing the entire downloading system, with the only requirement being to
return a valid package filename. This filter will work across both the single
and bulk upgraders, ensuring consistency in the approach.

Patch attached. Related: #22377, #22129

Attachments (3)

22378.diff (578 bytes) - added by rmccue 18 months ago.
Add filter to WP_Upgrader::download_package()
22378.2.diff (575 bytes) - added by rmccue 18 months ago.
More consistent filter name
22378.3.diff (888 bytes) - added by DrewAPicture 7 months ago.
+ docs

Download all attachments as: .zip

Change History (21)

comment:1 scribu18 months ago

  • Cc scribu added

comment:2 scribu18 months ago

  • Type changed from defect (bug) to enhancement

rmccue18 months ago

Add filter to WP_Upgrader::download_package()

comment:3 rmccue18 months ago

Trivial example of how to implement it:

<?php

function test_new_hook($return, $package, $upgrader) {
	if ($return !== false) {
		return $return;
	}

	$package = add_query_arg('key', 'token', $package);

	$upgrader->skin->feedback('downloading_package', $package);

	$download_file = download_url($package);

	if ( is_wp_error($download_file) )
		return new WP_Error('download_failed', $upgrader->strings['download_failed'], $download_file->get_error_message());

	return $download_file;
}

add_filter('wp_upgrader_pre_download', 'test_new_hook', 10, 3);

comment:4 follow-up: scribu18 months ago

We should try to keep some consistency between hook names.

First, there's general upgrader hooks:

'upgrader_pre_install'
'upgrader_post_install'
'upgrader_source_selection'
'upgrader_clear_destination'

And then there's plugin hooks:

'install_plugin_complete_actions'
'update_plugin_complete_actions'
'update_bulk_plugins_complete_actions'

And theme hooks:

'install_theme_complete_actions'
'update_theme_complete_actions'
'update_bulk_theme_complete_actions'

So, a better hook name would be 'upgrader_pre_download'.

rmccue18 months ago

More consistent filter name

comment:5 in reply to: ↑ 4 rmccue18 months ago

Replying to scribu:

We should try to keep some consistency between hook names.

[...]

So, a better hook name would be 'upgrader_pre_download'.

Whoops, missed that. Updated now.

comment:6 johnbillion18 months ago

  • Cc johnbillion added

comment:7 nacin18 months ago

  • Milestone changed from 3.5 to Awaiting Review

This is not 3.5 material.

comment:8 toscho18 months ago

  • Cc info@… added

comment:9 SergeyBiryukov17 months ago

  • Version changed from trunk to 3.4

comment:10 dd327 months ago

  • Keywords needs-docs added
  • Milestone changed from Awaiting Review to 3.7

Needs filter documentation, moving for review

DrewAPicture7 months ago

+ docs

comment:11 DrewAPicture7 months ago

  • Keywords needs-docs removed

22378.3.diff adds a docblock for the upgrader_pre_download filter hook.

Side note: We'll need to do a functional docs pass through this whole class in a separate ticket.

comment:12 rmccue7 months ago

  • Keywords commit added

If I may be so bold...

comment:13 dd327 months ago

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

In 25427:

Upgrader: Allow plugins to short-circuit the package download. Props rmccue and DrewAPicture. Fixes #22378

comment:14 farinspace7 months ago

my apologies if I am not understanding this correctly ... doesn't the plugin still need to be activated in order to use the hook, if the plugin is deactivated, wordpress will always check the plugins repo and if a plugin of the same name exists will return that data?

I think wordpress would be more open if a plugin author could specify their own update URI, this would avoid namespace grabs just for the sole purpose of not having the repo interfere with and external updater .. see ticket #25314

Last edited 7 months ago by farinspace (previous) (diff)

comment:15 dd327 months ago

doesn't the plugin still need to be activated in order to use the hook,

Yes, This is for cases where a plugin is offering a non-wordpress.org update location. This is not currently supported by WordPress in any way, except that plugins can, and do, bring such things to the table. So yes, that plugin WOULD have to be active for this to be of any use to the plugin.

comment:16 follow-up: farinspace7 months ago

Would the ability for wordpress to forgo an update check for a specific "installed" but deactivated plugin sound better .. something as simple as a plugin comment header parameter .. "UpdateURI" plugin authors provide a URI but it would be used as a boolean mostly, asking wordpress to skip an update check, which then would give the plugin a chance to hook in after activation .. I just want to make a reasonable request for such a feature and understand all implications.

comment:17 in reply to: ↑ 16 ; follow-up: dd327 months ago

Replying to farinspace:

Would the ability for wordpress to forgo an update check for a specific "installed" but deactivated plugin sound better ..

This ticket is not really related to your request in the least, I can't find the ticket reference for it, but short story is that there are no current plans for what you're requesting.

comment:18 in reply to: ↑ 17 SergeyBiryukov7 months ago

Replying to dd32:

This ticket is not really related to your request in the least, I can't find the ticket reference for it, but short story is that there are no current plans for what you're requesting.

#22129 appears to be related (and its duplicate #25314).

Last edited 7 months ago by SergeyBiryukov (previous) (diff)
Note: See TracTickets for help on using tickets.