consolidate plugin/theme/core upgrade/install functions

[DD32]

Currently a LOT of code is being duplicated for the install/upgrade functions, Currently we have seperate functions for:

• Core Upgrade
• Plugin Upgrade
• Plugin Install (from API URL)
• Plugin Install (from Uploaded package)
• Theme Upgrade
• Theme Install (Eventually)

Ideally, It'd be good to consolidate the Plugin/Theme functions together as they're basically copy-paste jobs of each other.

I'm leaving this for 2.8 unless someone else decides to take it upon themselves to attempt it, I'd prefer to get something extensible rather than purely massive as well, which is why i'm leaving it for when theres more time.

[ShaneF]

If I had a day to work on it, I could make it all one thing.

[DD32]

As its going, It doesn't seem likely i'll get this for 2.8

[Denis-de-Bernardy]

I'm putting a bounty on this one. If it gets committed into WP 2.8, whoever submits the patch is invited to email me (sales at semiologic dot com). I'll send him $200 by paypal.

D.

[DD32]

This is *very* much more difficult than it sounds.

Each step of the upgrade/install process for each is very similar to eachother, but given theres about a dozen of these steps..

In addition to that, sometimes they need to be called in a different order, Its really not clean to combine them at all.. Well, Not with the way they're currently written.

Re-writing it from scratch is an option, but thats something that would have to wait until 2.9 (as 2.8 is getting too close AFAIK)

[DD32]

(That being said, I've got the plugin upgrader/installer 80% in classes to reduce the code ammount, But, In the end, Its actually 3x the complexity..)

[Denis-de-Bernardy]

Replying to DD32:

This is *very* much more difficult than it sounds.

I'm aware, but I'm also monitoring the ticket, and can readily offer suggestions. (I'd do it myself if my own todo list weren't so long.)

Each step of the upgrade/install process for each is very similar to eachother, but given theres about a dozen of these steps..

K, allow me to steer you towards what I'd need as a developer then. :-)

My interest in this one has to do with my theme, whose zip is over 5M. This is due to the presence of two dozen different skins in it (each with their own set of image files and a screenshot). My intent is to add a skin installer/upgrader.

I could do this in one of two ways. The first is to copy/paste a huge chunk of WP code and mimic its upgrades as they occur. Obviously not a panacea.

The other would be to have some kind of generic object that I can simply inherit. Ideally, it would go something like this, in the themes' functions.php:

class skin_upgrader extends WP_Upgrader {
  // the remote requests to check for new versions
  var $service;

  // the option where we store what $service replies, also used for plugin hooks
  var $option;

  // how frequently do with hit the update service
  var $cron_interval;

  // the physical path where we copy the installed/upgraded skins to
  var $path;

  // whether to keep the downloaded files' folder (core upgrader would use false here)
  var $keep_folder = true;
}

When I initially looked into this, the stuff for core, themes and plugins seemed close enough that it could have been done already. The WP_Upgrader class could take care of:

• setting up a cron automatically
• periodically calling home to check for updates in a standardized manner
• installing or upgrading when asked to (permission issues to consider here)
• handling the calls to get more information on the install or upgrade by returning the url that should get opened using thickbox.

The class doesn't need to care of the actual UI screens. Those are probably too unique to be shared. But it should expose an API that can be used in these screens, so as to spare oneself the hassle of needing to rewrite the workflow-related functions for each of plugin, theme, core... and in my case, skins.

Ideally, we also toss a few plugin hooks in the class at strategic locations. If $options contains update_core for instance, it makes sense to fire things such as:

// anyone want to add to the response, allowing for shared screens?
$response = apply_filters($this->options . '_response', $response);

// it's quite difficult at the moment to change the more info link for plugins
// to something that is not from WP
$info = apply_filters($this->options . '_info', $info);

// anything we should do before upgrading?
do_action($this->options);

// anything we should do after upgrading?
do_action($this->options . '_done');

Makes more sense now?

[DD32]

Makes more sense now?

*Very* much more :)

Which has given me a really good idea on how to abstract it.. I'll take another think about it. No promises if i'll even attempt it though.

[Denis-de-Bernardy]

Cool. :-)

Don't hesitate to comment in here if you're uncertain of how this or that should get abstracted or implemented. Surely between the two of us we'll identify how to get each point resolved.

[DD32]

I've actually got a free weekend for once again...

I'm thinking a base class that does most of the basic steps:

Class Upgrader:
+filesystem init()

 Generic funcs:
+download archive()
+unpack archive()
+install files to location($srchive_source, $location)
+delete files/folders()

 Specific funcs (probably in child classes)
+plugin::install()
+plugin::upgrade()
+theme::install()
+theme::upgrade()
 (Which all call the parents functions in order, with needed params)

The major problem is strings, Plugin and Theme strings are different, The error messages need to reflect the component.. So that'll be something that needs to be worked around.

My original plan was to have the child class set $this->stringsno_dir? ='No Plugin directory found', but that has its downsides, rather unreadable code. Perhaps moving to returning WP_Error('unique_error'), and then handling that if( error_type($error, 'unique_error) return WP_error(',,;', 'The plugin couldnt be unpacked');

The Plugin/Theme install/upgrade functionalities could be easily handled by a single piece of UI code, due to the simple nature of the output. It might be possible to link some plugin hooks in so that a custom class (like you want) could use the majority of the same code.

[Denis-de-Bernardy]

+1 to the class you describe. We're on the same wavelength. One of the child-specific functions could be the default, too. It seems to me that playing around with a few variables would spare ourselves the need to rewrite the install()/upgrade() methods.

It might be desirable to ignore the core updates in all of this, too, and merely stick to sharing the code used by extensions such as plugins and themes.

The major problem is strings, Plugin and Theme strings are different, The error messages need to reflect the component.. So that'll be something that needs to be worked around.

The parent class itself should probably contain a $messages variable, along the lines of an array ($code => $details) in its constructor. Whether it's shared between error and success messages is no big deal in practice. But it should definitely be around imo: The messages need to be translatable, and one cannot use arrays or functions in a class's variable declarations -- only constants. They could be grouped in some kind of init_messages() method for tidiness.

Personally, I'd see it getting called in the UI screens as such:

if ( !is_wp_error($skin_upgrader->install($skin_id)) )
  //...

if ( !is_wp_error($skin_upgrader->upgrade($skin_id)) )
  //...

$skin = $skin_upgrader->get($skin_id);
if ( !is_wp_error($skin) )
  //...

// $skin could be an object or an array, it's no big deal
$skin->id; // unique id, i.e. its folder
$skin->name; // the skin's name
$skin->url; // the url for more details
$skin->package; // the zip file's url
//... etc

$skins = $skin_upgrader->query($args); // an array of skin objects, $args based on the $_GET request
if ( !is_wp_error($skins) )
  //...

$tags = $skin_upgrader->get_tags(); // popular tags, probably as objects too
if ( !is_wp_error($tags) )
  //...

If you feel like doing the UI too, it should probably look like this instead:

$skin_query = $skin_upgrader->query($args); // $args based on the $_GET request

$skin_query->display(); // this would highlight the HTTP errors as needed

A separate class is needed imo.

[Denis-de-Bernardy]

A separate note for the cron-related stuff, in case you did not give it thoughts yet. Some of that part definitely does NOT belong in the class. It should be managed as a separate function, that only loads the classes to the extent they're needed.

function update_plugin_cron() {
  // load classes
  include_once path/to/class-upgrader.php
  include_once path/to/class-upgrader-plugins.php
  // initialize and actually run the cron job
}

add_action('update_plugin_cron', 'update_plugin_cron');

[DD32]

The original intention of this ticket was to combine the actual backend processing of it, So i've finished up doing that.

I still want to clean up some variables and attack the UI functions a bit more, There are some things which are duplicated still (The credential checks namly). Having A Single class which does the UI for install/upgrade would be nice, and is what i'll be working towards later this week.

The patch i'm about to attach, Consolidates the Plugin/Theme upgrade/install, and some of the Core Upgrade functionality into a WP_Upgrader class, with Plugin/Theme/Core being child classes which set the options and specific strings..

As for commiting this patch, If a core-dev feels like it, and thinks its worthy, Go ahead.. But it should be noted, I've only tested on Windows, I havnt tried working via FTP yet, or a linux server. The plugin deletion code has been copied over from the old code directly, So there should be no mass-deletions, But it needs testing before i'd consider saying "Everyone, use this in production!!"

As for your request for 2.8, That could be up to the core devs on the actual release schedule 2.8 has..

[DD32]

Tested:

• Plugin Install from Web
• Plugin Upgrade
• Theme Upgrade
• Theme Install

Untested:

• FTP / SSH
• Plugin Install from Uploaded file (Should work..)
• Core Upgrade functionality (Theoretically should work, But variable naming needs checking)

[Denis-de-Bernardy]

Haven't tested yet, but, a few questions/remarks while scanning the current patch. I suppose a few are due to my not being 100% up to date on this area of WP:

• Changes to wp-admin/includes/class-wp-filesystem-base.php, as far as I can tell, move bits of code around *and* adds trailingslashit() calls. Might this not be rejected for backwards compatibility reasons due to the fact that it translates to API changes?

• Maybe create a separate ticket, wp-admin/includes/class-wp-filesystem-direct.php cleanup + bug fix, for things you did to that file? Clearly it's a commit candidate in its own right, and a valid one at that. Else it'll get lost in this ticket and might never get fixed.

• The same goes for wp-admin/includes/class-wp-filesystem-ftpext.php, wp-admin/includes/class-wp-filesystem-ftpsockets.php and wp-includes/update.php

• WP_Upgrader::download_package(): "$download_file = download_url($package); TODO, move to class?" -- probably not, as it would break backward compat

• WP_Upgrader::unpack_package(): Maybe toss $package in the feedback?

• WP_Upgrader::unpack_package(): You're not using the $delete_package variable

• WP_Upgrader::install_package(): "Please always pass these" -- maybe switch the comment to: A WP error will be returned if these aren't set

• WP_Upgrader::install_package(): Sometimes you check for if ( $clear_working ) before deleting on error, and sometimes not. Shouldn't it always delete on error?

• Theme_Upgrader::upgrade_strings(): "Maintainence mode?" -- yeyeye, +1 to that.

• Theme_Upgrader::install(): Probably just a copy/past, but... shouldn't the strings be $theme_files? :-)

• @set_time_limit( 300 ); should be in the WP_Upgrade class. WP is like... ~2M nowadays? I've seen plugins and themes that were nearly as large or larger than that. My own theme currently is 5M when zipped. While we're at it, I think we could bump this to 600s. The current upgrade updates about 15M worth of files in under five minutes on most servers, but I did need to add a filter in a plugin so as to make things work on some slower servers in order to bump the limit to twice its value.

In your opinion, how shareable are the check_for_update functions?

D.

[DD32]

Changes to wp-admin/includes/class-wp-filesystem-base.php, as far as I can tell, move bits of code around *and* adds trailingslashit() calls. Might this not be rejected for backwards compatibility reasons due to the fact that it translates to API changes?

I've been debugging a few related issues over the last while, And have been making small fixes to the code, but not submitting patches. This wasnt a final patch so I've just submitted them along with it. I'll make a patch to fix up the FS stuff once i finish testing.

WP_Upgrader::download_package(): "$download_file = download_url($package); TODO, move to class?" -- probably not, as it would break backward compat

I probably will move it to the class, Its not used anywhere else by core.. I'll leave a stub function which redirects it to this class though..

WP_Upgrader::unpack_package(): You're not using the $delete_package variable

I noticed that myself :)

WP_Upgrader::install_package(): "Please always pass these" -- maybe switch the comment to: A WP error will be returned if these aren't set

Already done

Theme_Upgrader::upgrade_strings(): "Maintainence mode?" -- yeyeye, +1 to that.

(Theres a trac ticket -somewhere- for this)

Theme_Upgrader::install(): Probably just a copy/past, but... shouldn't the strings be $theme_files? :-)

Probably :P

@set_time_limit( 300 );

I guess its a possibility, we already set a memory limit of 256MB for zip unziping..

In your opinion, how shareable are the check_for_update functions?

Not sure, Not very i dont think, I've done some UI work since this patch, and am still trying to work out if it looks cleaner afterwards or not :P

It might be possible to streamline the update checks a bit, but i dont think so (But i'll come back to that)

[Denis-de-Bernardy]

I probably will move it to the class, Its not used anywhere else by core.. I'll leave a stub function which redirects it to this class though..

As long as backward compat is ensured... I'm sure I'm not the only one who uses that function in plugins. ;-)

D.

[Denis-de-Bernardy]

Mmm... given:

​http://core.trac.wordpress.org/changeset/10912

We probably have time to get this reorganization of existing code into WP 2.8. I can't picture WP 2.8 getting released before mid-may -- or then someone needs to go buy a good book on QA.

[DD32]

Just a quick note:

I'm currently attempting to deal with FTP issues. Its simple enough with the Direct FS class, Absolutely no problems what so ever, ....Then you have to start to jump through hoops with FTP since its so freaking unreliable (What do you mean that folder doesn't exist, I JUST CREATED IT!)

[DD32]

attachment 7875.4.diff added

• See the comment block:

  /**
   * This file is an attempt at an abstracted version of the plugin/theme/core installer/upgrader which can be used interchangably for all uses needed within WordPress.
   * It is designed to be as flexible as possible, but some logic may seem rather, crazy to say the least.
   * Yes, this header is designed to be replaced before commiting, Hopefully i'll get some proper documentation in here.
   *
   * This File obviously needs some new PHPDoc, However:
   * Tested:
   *   Theme/Plugin Upgrades/Installs
   *   Core Upgrade
   *   FTP Extension, FTP Sockets, Direct.
   * Untested:
   *   SSH2 Layer - Needs a good cleanup.
   *
   * TODO: Remove this commentblock and replace with some better docs.
   */
  

As i'm sure you've noticed, I've not replaced the doc header yet, thats coming once everything seems tested ok.
(patch .5 is about to be submitted now)

[ShaneF]

I'll test this on SSH2 layer once 2.8 becomes stable enough for production use.

[Denis-de-Bernardy]

ssh tests might need the #8210 patches on top

@Shane: Don't wait too long to test. This ticket is quite large and won't make it as a last minute patch.

[janeforshort]

If you want to get it committed, try to get a bunch of people to test it on various platforms. Maybe you could mail the hackers list to get a few people to pitch in and test it.

Also, changing priority to the regular high, since omgbbq should really be saved for blockers.

[ryan]

If those who want it in are willing to test and fix anything that comes up in the beta cycle, I don't mind if it goes in. I don't want to be left holding the bag after the initial commit. :-)

[Denis-de-Bernardy]

Replying to ryan:

If those who want it in are willing to test and fix anything that comes up in the beta cycle, I don't mind if it goes in. I don't want to be left holding the bag after the initial commit. :-)

Oh, I need this. Definitely count on me for fixes that might end up needed during the beta cycle.

[DD32]

attachment 7875.6.diff added

• A few small bugfixes since last patch
• Tested with SSH2 (See #8210)
• Tested with non-chrooted FTP Installs (Need some various Chroot setups to be tested, ie.to ~/ and to ~/public_html/domain.com/ as well as multiple WP installs in those folder)
  • The code is virtually the same, Not much was changed there (If any), Its mearly the way the install/upgrade code runs with relation to those folders.

• Testers are wanted.
  • If you have a "different" server config or have in the past had issues with plugin upgrades (esp. if it was a problem when upgrades first came out!)
  • Please only run with a test-account/install at first, I've found it stable and safe, doesnt mean there isnt a nasty bug in some race condition.

@Ryan: Indeed i'll be working to fix anything which comes up after an initial commit, although I hope there'll be no major bug reports myself..

[demetris]

Results from testing 7875.6.diff for FTP installs and updates on a shared-hosting account (with A Small Orange).

Updating 3 plugins (1 active, 2 inactive)

• 1x: Could not copy file, on inactive plugin
• 2x: Success
• 1x: Success but got dialog to download update.php, on the other inactive plugin

Updating 3 themes (1 active, 2 inactive)

• 3x: Success

Installing 3 themes

• 3x: Success

Installing 3 plugins

• 3x: Success

DD32, let me know if you need any information.

[DD32]

@demetris: Was there anything special about the plugins? What was the end result for the failing plugins (Ie. Deleted? Still there and not touched, Etc)

Seems strange that updating plugins failed while themes were fine..

I'll post up a plugin for logging the connection in a moment.

[DD32]

attachment fs-debug-logger.php added

If anyone has an error occur consistently (Or maybe not so consistent..) install that plugin (Warning: It requires PHP 5.1+ i think, at least PHP5.0 i would expect), and email me(WordPress @ dd32.id.au will do) the log it creates. the log can be found in your uploads folder called 'debug.txt'

It simply sits between the WordPress Upgrader and the Filesystem abstraction and records whats being said, Since i know what the upgrader should be doing, I can tell whats calling what in the log.

[demetris]

Replying to DD32:

@demetris: Was there anything special about the plugins? What was the end result for the failing plugins (Ie. Deleted? Still there and not touched, Etc)

Seems strange that updating plugins failed while themes were fine..

I'll post up a plugin for logging the connection in a moment.

Sorry, I was not detailed enough:

In the “could not copy file” instance, the plugin updated fine the 2nd time I tried.

In the instance where I got the dialog to download update.php, the update had been successful, as it turned out.

So, in the end, all 3 plugins were updated successfully.

I’ll install the logging plugin, and attach the logs here if there are any more problems.

[DD32]

In the instance where I got the dialog to download update.php, the update had been successful, as it turned out.

Sounds like it could be a output buffer problem.. I'll try a few different setups, The fact it only happened 1/10 seems to sound like a random glitch to me however or a timeout somewhere

I’ll install the logging plugin, and attach the logs here if there are any more problems.

Cheers :)
Be warned however, It logs everything in the one file, if you leave it active and do a few upgrades, it could end up being rather large.

[Denis-de-Bernardy]

merging #9303 into this one: we need to make sure the update_* transients get deleted after bulk operations such as deletes.

[DD32]

merging #9303 into this one: we need to make sure the update_* transients get deleted after bulk operations such as deletes.

I've re-opened #9303 to keep it separate.

At present the Deletion's are not handled by this ticket, They can be merged into this after its been commited however.

(I'd like to keep small specific issues which will probably remain even with this commited to their own tickets if one exists, It just helps track the process of this ticket)

[ryan]

I don't have an FTP setup handy, but I'll run through all of the upgrades and installs using direct and then commit if you think it is ready.

[DD32]

I don't have an FTP setup handy, but I'll run through all of the upgrades and installs using direct and then commit if you think it is ready.

I can lend you my FTP test account if you wish :)

I've not been able to fault it yet since my final patch here.

[demetris]

More test results for 7875.6.diff and the debug logger from me...

I have not been able to use the debug logger: When I activate it I get a dialog to download update.php every time I click to “upgrade automatically” a plugin.

Other than that, things work fine, on 3 different setups.

1. On my test share-hosting account with A Small Orange everything works OK, including theme deletion (which I had not tried before).

2. On another test shared-hosting account (free, with 000webhost.com) everything works fine.

3. I installed the consolidated patch on a live site as well (op111.net) and everything works fine there too.

(On another free shared-hosting account (with freehostia.com) that I use for testing I cannot test the patch at all, because I get the “An Unexpected HTTP Error occurred during the API request.” message.)

Cheers

[DD32]

I have not been able to use the debug logger: When I activate it I get a dialog to download update.php every time I click to “upgrade automatically” a plugin.

It requires PHP 5.1+, Probably dont have it, or something else that it requires.. To be honest, I've hardly even tested it, it worked on my local setup :)

because I get the “An Unexpected HTTP Error occurred during the API request.” message.

sounds like a host issue with the HTTP Transports, Unrelated to these changes..

[ryan]

Force ftp so I could the connection screen. I see two h2s: "Upgrade Plugin" and "Connection Information".

Installs and upgrades using direct went off without a hitch.

[ryan]

(In [11005]) consolidate plugin/theme/core upgrade/install functions. Props DD32. see #7875

[DD32]

Force ftp so I could the connection screen. I see two h2s: "Upgrade

Plugin" and "Connection Information".

Minor UI glitch, Its been fixed before, I was going to wait until that patch was commited before attempting to patch it again (The patch was large enough as it was, and didnt have any bad bugs).

I'll get a patch up for that (and anything else which comes up during the day) tonight.

[DD32]

Props Sivel for the $url non-needed args.

[DD32]

A whitespace cleanup is needed on both update.php and class-wp-upgrader.php, Mainly empty lines with tabs & a few trailing spaces.

[DD32]

attachment 7875.7.diff added

• Props Sivel for the $url non-needed args.
• Fixes the mutliple h2 issue.

[DD32]

Replying to Denis-de-Bernardy:

When I initially looked into this, the stuff for core, themes and plugins seemed close enough that it could have been done already. The WP_Upgrader class could take care of:

• setting up a cron automatically
• periodically calling home to check for updates in a standardized manner
• installing or upgrading when asked to (permission issues to consider here)
• handling the calls to get more information on the install or upgrade by returning the url that should get opened using thickbox.

The class doesn't need to care of the actual UI screens. Those are probably too unique to be shared. But it should expose an API that can be used in these screens, so as to spare oneself the hassle of needing to rewrite the workflow-related functions for each of plugin, theme, core... and in my case, skins.

I dont think abstracting this is going to be possible (or rather, A good idea)

Different API's return different results; Different input sets require checking different fields; Different Expire methods; etc. In the end, An abstracted method is going to take up more code, and not gain any benefits over something similar to whats in wp-includes/update.php

Much like the UI cant be too generic, Nor can the update checking..

[DD32]

See Also: #9596 (Text difference between two equivalent links) - For "Back to Theme Installer" vs "Return to Plugin Installer"

[Denis-de-Bernardy]

mmm... might be wrong, but... I thought that update_core, update_plugins, and update_themes were built around the same idea. but yeah, seeing the latest stuff wp_version_check() I see what you're meaning. :D

[DD32]

but... I thought that update_core, update_plugins, and update_themes were built around the same idea

Yeah, They are, But when you actually look at what causes each to expire, and what they actually check from the input, and what parts of the input need to be passed to the API, etc. It quickly becomes very little common code.

The HTTP Api and *_transient() takes care of major parts, other than that its just input selection, error checking, and setting when the update check is called(cron)

[Denis-de-Bernardy]

I'll let you know how well it plays with my theme's skin installer/upgrader, I should have that all done by the end of the month. :-)

[DD32]

I'll let you know how well it plays with my theme's skin installer/upgrader, I should have that all done by the end of the month. :-)

If you get stuck, You've got my details for a bit of help if needed :)

[Denis-de-Bernardy]

found another old ticket related to updates: #5117

[Denis-de-Bernardy]

more of the same: #7395

[ryan]

(In [11012]) Install/upgrade cleanups. Props DD32, sivel. see #7875

[Denis-de-Bernardy]

see also #9402

[azaozz]

(In [11028]) Remove passing by reference in WP_Upgrader, see #7875

[DD32]

attachment 7875.8.diff added

• Adds a bit more detail to error messages. Eg, instead of "Incompatible Archive", "Incompatible Archive PCL_ERR_ETC: Some description.."

[ryan]

(In [11080]) Better error messages. Props DD32. see #7875

[ryan]

[11084] fixes up the php4 constructors. There's a problem with message display in php4.

downloading_package

unpack_package

installing_package

[DD32]

There's a problem with message display in php4.

Cant see why on earth thats happening.. But i'm looking into it.

[DD32]

Well.. actually it is.

$this->skin->set_upgrader($this) will always pass by value, not by reference.. Hm..

[DD32]

attachment 7875.9.diff added

• A couple of typo's
• Fixes Install messages under PHP4
  • Passing $this by reference inside the constructor doesnt work, Calling it after everything has been created properly however, Does work
• few other small tweaks.

Some PHPDoc for the classes (at minimum) is coming up.

[DD32]

attachment 7875.10.diff added

• Includes attachment 7875.9.diff
• Adds some basic Class documentation
  • Methods are not touched, And long descriptions are currently missing. (There is possibly a better PHPDoc marker for that.. Which i havn't used - just a @TODO mark)

[ryan]

(In [11089]) WP_Upgrader updates from DD32. see #7875

[Denis-de-Bernardy]

See also: #5422

[Denis-de-Bernardy]

See also: #6863

[DD32]

I think I might close this ticket as fixed, and open a few more tickets for future expansion on this

• Documentation ticket TODO
• Move Plugin/Theme deletions over to this too
• Any other bugs which testers come across during the Beta/RC time. - It's probably going to be easier to deal with those on a case-by-case basis rather than sifting through a ticket with 70+ comments.