Make WordPress Core

Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#22378 closed enhancement (fixed)

Add ability to hijack plugin install/upgrade downloads

Reported by: rmccue's profile rmccue Owned by: dd32's profile 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 11 years ago.
Add filter to WP_Upgrader::download_package()
22378.2.diff (575 bytes) - added by rmccue 11 years ago.
More consistent filter name
22378.3.diff (888 bytes) - added by DrewAPicture 11 years ago.
+ docs

Download all attachments as: .zip

Change History (21)

#1 @scribu
11 years ago

  • Cc scribu added

#2 @scribu
11 years ago

  • Type changed from defect (bug) to enhancement

@rmccue
11 years ago

Add filter to WP_Upgrader::download_package()

#3 @rmccue
11 years 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);

#4 follow-up: @scribu
11 years 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'.

@rmccue
11 years ago

More consistent filter name

#5 in reply to: ↑ 4 @rmccue
11 years 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.

#6 @johnbillion
11 years ago

  • Cc johnbillion added

#7 @nacin
11 years ago

  • Milestone changed from 3.5 to Awaiting Review

This is not 3.5 material.

#8 @toscho
11 years ago

  • Cc info@… added

#9 @SergeyBiryukov
11 years ago

  • Version changed from trunk to 3.4

#10 @dd32
11 years ago

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

Needs filter documentation, moving for review

@DrewAPicture
11 years ago

+ docs

#11 @DrewAPicture
11 years 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.

#12 @rmccue
11 years ago

  • Keywords commit added

If I may be so bold...

#13 @dd32
11 years 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

#14 @farinspace
11 years 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 11 years ago by farinspace (previous) (diff)

#15 @dd32
11 years 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.

#16 follow-up: @farinspace
11 years 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.

#17 in reply to: ↑ 16 ; follow-up: @dd32
11 years 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.

#18 in reply to: ↑ 17 @SergeyBiryukov
11 years 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 11 years ago by SergeyBiryukov (previous) (diff)
Note: See TracTickets for help on using tickets.