Use a replaceable function for the beforeupload handler with shiny updates

[DavidAnderson]

In #31819, I proposed changing the anonymous function used for the beforeunload handler in shiny updates (which is used for the "Are you sure?" alert).

The attached does two things:

1) Uses a replaceable function instead of an anonymous one, allowing plugins to hook in and adapt the process. (I have a use case for this).

and

2) Triggers an wp-plugin-shinyupdates-loaded event, so that plugins can do this at an appropriate time.

[DavidAnderson]

Make beforeunload function in shiny updates replaceable

[DavidAnderson]

Second version of patch

[adamsilverstein]

whitespace fix

[adamsilverstein]

Replying to DavidAnderson:

Added a little whitespace in last patch. Curious why you wrapped the function call in an an anonymous function wrapper (between patches 1 and 2) - was that required?

Looks good, thanks!.

[ocean90]

Please create patches from the root directory.

Would namespacing the event help too?
$().on( 'beforeunload.wp-update', function() {} ) and then $().off( 'beforeunload.wp-update' ).

[jorbin]

beforeunload is a browser event, it doesn't need to be namespaced.

I don't think we need the 'wp-plugin-shinyupdates-loaded' event, at least not where it is since if you set a updates.js as a dependency in enqueue_script updates.js will be loaded first. The majority of events are bound to document.ready which won't have taken place yet either.

I'm cleaning up the rest of the patch (the deanonymyzing of the beforeunload function) and will commit that.

[DavidAnderson]

@jorbin - Thanks. This all makes sense. I was being cautious, based on my lack of familiarity with some of these concepts.

@ocean90 - I'd opened it as a 'defect', because I'm working on a plugin with 400,000 users of its various editions, that I can't yet get to do in WP 4.2 beta 4 what I could in WP 4.1. i.e. A regression. (See #31819). This current ticket, I believe, clears the way to removing the final issue. (I'll confirm that asap after @jorbin's patch). I'd feared it being overlooked if I chose 'enhancement', followed by a support tsunami on release day. Is there a way to indicate a 'regression' status when opening a new ticket?

[jorbin]

In 32126:

Use named function instead of anonymous function

The named function is testable and replaceable.

Fixes #31964
Props DavidAnderson, adamsilverstein, jorbin

[DavidAnderson]

The patch in [32126] does not work.

The answer to @adamsilverstein's question - "Curious why you wrapped the function call in an an anonymous function wrapper (between patches 1 and 2) - was that required?" - is "yes".

This time I have verified that by actual, repeated testing... ;-)

On my previous patch where I added the wrapper, it was very late in the day, I was very keen to get something into Trac before ending the day, and I added the wrapper based upon my understanding that variable referencing in JavaScript works as it does in PHP. Testing shows this to have been correct.

New patch about to be attached...

[DavidAnderson]

Patch against current trunk to make it work

[DavidAnderson]

Snippet used for testing:

add_action('admin_footer', 'my_admin_footer');
function my_admin_footer() {
        ?>
        <script>
                jQuery(document).ready(function() {
                        wp.updates.beforeunload = function() { console.log("HI!"); return true; }
                });
        </script>
        <?php
}

Note that testing with alert() may not work at all - Firefox, at least, doesn't seem to process an alert() firing during this event.

[DavidAnderson]

Really make it work this time... (forgot to pass on the returned value)

[jorbin]

You need to enqueues a script that has updates.js as a dependency and not just place your js directly in the footer.

I created a sample plugin that demonstrates this ​https://gist.github.com/aaronjorbin/5644934946c05ad08b9c

Wrapping the call to wp.updates.beforeunload in anonymous function defeats the purpose of using a named function that can be removed. What browser are you seeing an issue with it in?

[DavidAnderson]

Thanks...

I'm in Firefox 37. So, looking at the gist - this differs from my test by using .on() and .off(); I was instead replacing the value of wp.updates.beforeunload and leaving the event handlers as they were.

Isn't that what accounts for the difference, rather than the enqueueing?

My live code, which works with my patch, was basically this:

var old_beforeunload_function = wp.updates.beforeunload;
// Over-write the default function
wp.updates.beforeunload = function() {
    if (our_condition_fulfilled) { return old_beforeunload_function(); }
    // Otherwise: let the unload proceed
};

So, I suppose I could instead do this, to make it work with the current code:

jQuery( window ).off( 'beforeunload', wp.updates.beforeunload );
 
jQuery( window ).on( 'beforeunload', function(){
    if (our_condition_fulfilled) { return wp.updates.beforeunload(); }
    // Otherwise: let the unload proceed
});

Does that seem right?

[jorbin]

I would absolutely use on and off instead of duck punching wp.updates.beforeunload.

I haven't tested adding js directly to the footer since that isn't the correct way to add javascript.

[DavidAnderson]

Thanks. All now seems good, on RC1. Phew!

Just one bit of information you may need after WP 4.2 releases and if you get relevant reports... if:

1) the user has a plugin installed that uses Yahnis Elsts' plugin updater class (most 3rd party plugins do - ​https://github.com/YahnisElsts/plugin-update-checker)
2) and if the version of Yahnis' class is an older one

Then:

a) Only the first shiny update of a wordpress.org plugin will work. Subsequent ones fail. I traced this as far as discovering that only updates known via Yahnis' class would appear in the 'update_plugins' transient, after the first shiny update. Reproducible: every time

b) A PHP warning will be output for every plugin using Yahnis' class on the plugins page. Reproducible: every time.

Both these problems are resolved by updating to the current version of Yahnis' class. I have not bothered to bisect exactly which versions are affected, or to identify what the root cause is behind it.

Summary: if after WP 4.2 you see reports of people for whom shiny updates after the first fail until they reload the plugin page, ask if they've got 3rd party plugins. Probably the problem will disappear if they de-activate the 3rd party plugin.