Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 8 years ago

#31819 closed enhancement (fixed)

Shiny updates: more sophisticated locking

Reported by: davidanderson's profile DavidAnderson Owned by: jorbin's profile jorbin
Milestone: 4.2 Priority: normal
Severity: normal Version: 4.2
Component: Upgrade/Install Keywords: shiny-updates has-patch close
Focuses: ui, javascript, administration Cc:

Description

Now that the "shiny updates" code for 4.2 has settled down, I've been able to have a look.

I develop a plugin (UpdraftPlus - the most installed WP backup plugin; 400,000+ current installs) that hooks into plugin updates (in the paid version), and runs a backup before updating. (So, if the update is broken, you have a convenient backup from just before). Communication on the backup state goes between browser and back-end over AJAX. When the back-end completes, the update operation then proceeds.

Of course, we want to retain this functionality with shiny updates.

Looking at the shiny updates code, it looks like the best way to accomplish this is for my code to set wp.updates.updateLock to prevent the update going ahead, until it's ready ... and then set it back to false and call wp.updates.queueChecker().

This looks like it'll work, though the filesystem credentials dialog makes it slightly more complicated (but I think we can work with that too).

However, I am concerned that a binary boolean lock is inflexible, and makes the shiny updates process difficult and a bit hack-ish to plug into, possibly setting us up for future complications or unanticipated complications out in the wild.

It'd be ideal if WP had JavaScript filters (see: #21170), but they're not there yet. So, I suggest that instead of a boolean lock, instead wp.updates.updateLock becomes an array of functions. When the lock status needs checking, the functions are each called.

Alternatively, and more simply, instead of a boolean, an integer could be used, and different agents wanting to lock could set different bits (the core code would use some, and others would be left for plugins).

Thoughts?

Attachments (2)

31819.diff (640 bytes) - added by jorbin 9 years ago.
31819.patch (724 bytes) - added by iseulde 9 years ago.

Download all attachments as: .zip

Change History (32)

#1 @ericlewis
9 years ago

Hi there @DavidAnderson, thanks for the report.

Are you saying that your plugin's custom updates functionality is broken with the current state of shiny updates?

#2 follow-up: @DavidAnderson
9 years ago

@ericlewis: yes. The change in flow is very significant. Previously, for example (with the "update now" hrefs) I used to block the default event on the links that were clicked, start a backup (over AJAX) and then set window.location to that href once the backup was completed. With shiny updates that technique is now irrelevant (since they also block the default event, and do their own stuff).

I'm working on it now and it's very much more complex than envisaged to get working again with a boolean lock (to deal with all the possible interactions of page elements).

#3 in reply to: ↑ 2 @ericlewis
9 years ago

Replying to DavidAnderson:

Previously, for example (with the "update now" hrefs) I used to block the default event on the links that were clicked

Can you link me to the line in the plugin where this happens?

#4 @jorbin
9 years ago

  • Milestone changed from Awaiting Review to 4.2

I feel like the lock being an array of functions is more complicated than it needs to be while also not really assisting in making this as functional as possible.

One idea that I have in order to help UpdraftPlus or any other plugin that has existing bindings for the updates page to to add a jquery event to the document at the end of $( document ).ready signifying that we have completed adding our events. This way a plugin can optionally deregister a binding and add there own. This would require that we move the bindings into named functions, but reducing anonymous functions is generally a good idea. Essentially could imagine plugins using code that looks like this:

$( document ).ready( function(){  
    $( '.plugin-update-tr .update-link' ).on( 'click', my_cool_plugin_function ); 
});

$( document ).on( 'wp.updates.eventready' , function(){
    $( '.plugin-update-tr .update-link' ).off( 'click', my_cool_plugin_function );
    $( '.plugin-update-tr .update-link' ).off( 'click', wp.updates.updatelinkclick ); 
 
    $( '.plugin-update-tr .update-link' ).on( 'click', my_cool_plugin_function_with_code_that_makes_sense_for_4.2 ); 
});

#5 @jorbin
9 years ago

  • Keywords shiny-updates added

#6 follow-up: @DavidAnderson
9 years ago

@jorbin - that sounds a good idea in principle... but what I really want is for the updates to then proceed after the backup has completed. Unless there's also a way to do that (without copy/pasting half of updates.js), then it could replace one complex problem with a second one.

I've just committed code to our internal SVN that now works with the current setup (including getting FTP credentials). It sets the lock variable to prevent the updates, until it's ready - then it re-sets it and calls the run-queue function. It's ugly, but it works.

i.e. What I really need is an elegant way to delay the shiny updates until I'm ready - and then give them the single to go ahead. I've currently found a way, but it's not pretty.

#7 in reply to: ↑ 6 @adamsilverstein
9 years ago

It sounds like what you ideally need is a hook where you can have your JS code run before the ajax call to perform the update.

If core had such a hook, your hooked function could return a Deferred object or promise that when resolved would trigger the shiny update to continue. Another use case for #21170 :)

Replying to DavidAnderson:

@jorbin - that sounds a good idea in principle... but what I really want is for the updates to then proceed after the backup has completed. Unless there's also a way to do that (without copy/pasting half of updates.js), then it could replace one complex problem with a second one.

I've just committed code to our internal SVN that now works with the current setup (including getting FTP credentials). It sets the lock variable to prevent the updates, until it's ready - then it re-sets it and calls the run-queue function. It's ugly, but it works.

i.e. What I really need is an elegant way to delay the shiny updates until I'm ready - and then give them the single to go ahead. I've currently found a way, but it's not pretty.

#8 @DrewAPicture
9 years ago

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

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


9 years ago

#10 @DavidAnderson
9 years ago

There are obviously a lot of issues involved here. But, for my feedback as someone trying to interact with this for WP 4.2 (with a very widely installed plugin), it might help to highlight the two most hack-ish thing I had to do in order to make my code work again:

1) The only way to detect when a shiny update had finished was for me to use setInterval() to poll and check wp.updates.updateQueue.length. This ugliness could be removed by wp.updates.updateSuccess() calling a jQuery trigger.

2) When an update link is clicked, the previous HTML (the update/more info links) is lost forever. Shiny updates replaces it with "Updating...". But, my code pops up a dialog that has a "cancel" option. If they cancel, I can't bring back the previous HTML without ugliness/pain - e.g. parsing and saving it all on page load (yuk). It'd be better if shiny updates would show/hide the HTML instead of simply replacing it.

#11 @jorbin
9 years ago

  • Keywords needs-patch added

1) I like the idea of firing an event on what in the code we are referring to as the $message.

2) As of [31949] that isn't the case. Take a look at wp.updates.requestForCredentialsModalCancel and wp.updates.updatePlugin

@jorbin
9 years ago

#12 @jorbin
9 years ago

  • Keywords has-patch dev-feedback added; needs-patch removed

The above patch adds events to both Success and Error outcomes of running an update. I decided to attach them to the document rather than the individual elements to provide for the greatest amount of flexibility in updating the HTML in the future. I pass in the response from the server so that we can the appropriate plugin can be identified.

@DavidAnderson - If you want to also share your pre-release code, I'm happy to run some tests and also see if there are more optimizations that we can make here to help.

#13 @ericlewis
9 years ago

Chatting about this with @isuelde @helen and @DrewAPicture, here are some thoughts.

Plugins should (try) not to depend on markup looking specific for critical path functionality, as it is subject to change. Obviously, we should balance this statement with our project's commitment to backwards compatibility.

If your goal is to perform a backup before upgrading a plugin, that would be better handled in the PHP application layer rather than triggering it via front-end user actions. This would remove the plugins' dependency on the markup to take a particular form.

You could tap into the upgrade process via the upgrader_pre_install hook and perform your backup. This hook runs before every individual plugin upgrade.

Last edited 9 years ago by ericlewis (previous) (diff)

#14 @DavidAnderson
9 years ago

@ericlewis: that approach was used by our "automatic backups" from May(-ish) 2013 onwards, until fairly recently. It has some significant problems, which caused unhappy users and support requests and we were glad to get rid of it. The biggest problem is that many shared web hosting setups are not generous in how much time they allow PHP to run before killing it off (and do not respect set_time_limit()) - or, if they are, the user may still have a gigantic amount of data in one of their tables. (When you've got 400,000 users of your plugin, the uncommon becomes a regular event). If the user's database and/or the plugin is large, and if they're also sending the backup to cloud storage, then there's a too-large risk that the backup will be killed before it can finish; and thus also before the update can go ahead. There's no way to know when the webserver's about to kill you off. (And sometimes, even if the backup had time to complete, the limit would be reached during the update process instead; the current updates screen in WordPress itself notes that the operation can take considerable time).

Because of that issue, we switched methods, to have the backup started over AJAX and monitored from the browser. This allows the backup to be resumed from where it got up to, every time it gets killed off - and thus to run until it finishes. (UpdraftPlus backups are resumable/crash-safe - they can be killed off at any point and picked up again - even with 400,000 users I don't believe we've had even one support request in the last 12 months due to PHP killing off a backup process at an inconvenient time that could not be resumed). Then, and only then, the updates can go ahead. Since switching approaches, we've seen support requests for automatic backups dry up: they just work.

The other issue is getting feedback on the backup process. If it's done entirely during the back-end update progress whilst the page loads, then there's no reliable way to do this. Different PHP and webserver setups deal with output buffering differently, and often can't be over-ridden. It's normally possible to control it a little, but not always: the result is that many users see zero output reporting on progress until the backup finishes (or dies); or, they get every line of the log spewed over the screen.

All being well, I'll look at the patch and send more thoughts early next week...

Final bit: "Plugins should (try) not to depend on markup looking specific for critical path functionality, as it is subject to change." Of course. We certainly tried. But, it's a choice between tracking the WP "Updates" pages closely between versions, or using a method which requires a backup to complete inside the unknown maximum time that the webserver is going to allow PHP to run. The former option is ultimately much better for reliable backups.

#15 @nacin
9 years ago

  • Keywords close added

Hi @DavidAnderson,

We're going to be unable to support this request without being able to study what your plugin is doing, and what any new version is attempting to do.

Quoting jorbin:

@DavidAnderson - If you want to also share your pre-release code, I'm happy to run some tests and also see if there are more optimizations that we can make here to help.

If you'd like, you can email your code to nacin or helen (both "at" wordpress.org) and we will share it with the contributors in this ticket. We're going to need to close this unless we hear from you.

Thanks,
Nacin

#16 @DavidAnderson
9 years ago

Will do. There's a double public holiday in the UK, so things are going a bit slowly.

#17 @DavidAnderson
9 years ago

OK...

I've just emailed both those addresses with a file autobackup.php. Install UpdraftPlus from https://wordpress.org/plugins/updraftplus/ , and then drop this file into <plugin_dir>/updraftplus/addons/

This code was developed on the core trunk 6 days ago. It looks like there have been a few tweaks to shiny updates since then - so, it may not be 100% working, but hopefully you'll see the method being used.

Note that that file - autobackup.php - contains code for doing auto-backups in 3 distinct situations: 1) WP background updates 2) Traditional non-shiny updates 3) Shiny updates. i.e., plenty of the code in it is not relevant to this ticket. The method admin_footer_inpage_backup() is the one that outputs the JavaScript used to intercept and delay the update request whilst a backup is done - note that this JavaScript also is meant to run/work on older WP versions without shiny updates. (The actual code that does the backup on the back-end and monitors it via AJAX on the front-end is all in the free plugin - includes/updraft-admin-ui.js has the JavaScript in it, and admin.php has the AJAX handler; you can see it in action in another situation if you just press the "Backup Now" button in the settings page, Settings -> UpdraftPlus Backups).

Finally - it'd be good if, on a different but related matter, the patch in #30441 could get into WP 4.2. That would also reduce a bit of hackiness when running an automatic backup in a different situation; specifically, just prior to automatic background updates.

#18 @jorbin
9 years ago

@DavidAnderson

Thanks for sharing the file with Helen and Nacin. I've taken a look and a couple of things:

1) I think adding the events to completion will eliminate your need to do polling. I'm going to do that now.

2) Having the lock in place at the start is going to degrade the experience since we have the AYS check. An alternative would be to use the jQuery ajax events. For example the code below will detect an attempt at a shiny update before it is sent. You can save a copy of the request, abort the original, and add the interaction that you need for your plugin. Then on completion of that interaction, restart the original request (you'll need to add some logic to ensure this doesn't create an infinite loop. )

getQueryParams = function(queryString) {
    var query = queryString; 
    if (!query) {
        return false;
    }
    return _
    .chain(query.split('&'))
    .map(function(params) {
        var p = params.split('=');
        return [p[0], decodeURIComponent(p[1])];
    })
    .object()
    .value();
};

jQuery.ajaxSetup( {beforeSend: function(e, data) {
    var requestData = getQueryParams( data.data );

    if ( data.type == "POST" && requestData.action === "update-plugin" ) {
        // store the request you want to make
        // do what you need to do   
        e.abort();
    }
}} );

3) As I mentioned above, as of [31949] the original html of what turns into "updating..." is saved.

#19 @jorbin
9 years ago

In 32061:

Trigger events upon the completion of a shiny update

Plugins need to be able to do actions when a shiny update completes. While we don't have complete javascript actions and filters, we do have jQuery events that we can fire to assist plugin authors.

See #31819

#20 @jorbin
9 years ago

  • Keywords dev-feedback removed

@iseulde
9 years ago

#21 @iseulde
9 years ago

. is used for namespaces, so better to use dashes.
https://github.com/jquery/jquery/blob/2.1.1/src/event.js#L19

#22 @jorbin
9 years ago

In 32063:

Use dashes instead of dots as separator for jQuery events in shiny updates

. is used for namespaces, so better to use dashes.

see #31819
props iseulde

#23 @jorbin
9 years ago

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

I'm going to go ahead and mark this as closed since we have added the new events. Discussion can continue if needed.

#24 @DavidAnderson
9 years ago

@jorbin: OK... thanks for this. I've made some time to catch up.

There were 3 things:

1) The "update available" text being brought back when an update is cancelled via the backup dialog. This is now implemented - thank you.

2) Using the new jQuery event instead of polling. This is also now implemented - thanks again.

3) Not locking the queue, but instead intercepting the jQuery AJAX event, and then re-booting it. You said "Having the lock in place at the start is going to degrade the experience since we have the AYS check" - am I right in thinking that this is referring to the "do you want to close this page?" check (that's new - it wasn't there when I opened this ticket - am I right in thinking that?).
Intercepting + replaying the AJAX event feels like a dirty low-level hack, whereas locking the queue feels like the right concept, if this "do you want to close?" check could be avoided. Also, the AJAX technique won't work well with bulk updates, as there's multiple AJAX calls then. (Though, I suppose I could try to detect when they're related/sequential...?). Though, currently, it looks like shiny updates have been disabled when using the "bulk updates" form - is that so? (I think I saw a Trac ticket somewhere mentioning it).

Also - when I do multiple shiny updates, only the first succeeds. Subsequent ones fail. This is so with my code removed also, so it seems not to be related to anything I've done. My current WP version is 4.2-beta4-32091.

#25 @DavidAnderson
9 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Re-opening - in case the comments aren't getting seen whilst the ticket is in the 'closed' state. (Sorry if this is the wrong thing to do - I wanted to make sure they're not missed).

Regarding the queue locking and the unwanted "are you sure you want to leave?" message... a straightforward solution for this would be to not use an anonymous function.

i.e. Instead of...

$( window ).on( 'beforeunload', function() {
	if ( wp.updates.updateLock ) {
		return wp.updates.l10n.beforeunload;
	}
});

... use:

wp.updates.beforeunload = function() {
	if ( wp.updates.updateLock ) {
		return wp.updates.l10n.beforeunload;
	}
}

$( window ).on( 'beforeunload', wp.updates.beforeunload);

Then, I can replace the function with my own that can tweak the logic appropriately for my use case.

#26 @DrewAPicture
9 years ago

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

Discussion can continue while the ticket is closed. Notifications are sent either way.

#27 @DavidAnderson
9 years ago

I've attached an appropriate patch in #31964, which makes the beforeunload function replacable by plugnis, and adds a jQuery trigger so that plugins can do this (or any other action that depends on shiny updates JS having finished loading) at the right time.

#28 @jorbin
9 years ago

In 32133:

Move wp-plugin-update-success event to after lock is released

Fixes #31978
See #31819
Props DavidAnderson

#29 @jorbin
8 years ago

In 38218:

Updates: Standardize JS Custom Event Names

Custom JS events are triggered on the document in order for plugins to have something to hook into. The standard began in #31819 is dash separated and begins with wp to signify the namespace, followed by the subject of our action ( "theme", "plugin", etc.) followed by the action and an optional indicator of status ( "install-success", "deleting" ).

This brings some of the theme hooks in line with the standard. As of now, all plugin actions in src/wp-admin/js/updates.js have an equal corresponding theme action.

Fixes #37598.
See #37512, #37216, #31819.
Props olarmarius, ocean90.

#30 @jorbin
8 years ago

In 38219:

Updates: Standardize JS Custom Event Names

Merges [38218] in to 4.6 branch.

Custom JS events are triggered on the document in order for plugins to have something to hook into. The standard began in #31819 is dash separated and begins with wp to signify the namespace, followed by the subject of our action ( "theme", "plugin", etc.) followed by the action and an optional indicator of status ( "install-success", "deleting" ).

This brings some of the theme hooks in line with the standard. As of now, all plugin actions in src/wp-admin/js/updates.js have an equal corresponding theme action.

Fixes #37598.
See #37512, #37216, #31819.
Props olarmarius, ocean90.

Note: See TracTickets for help on using tickets.