WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#31964 closed enhancement (fixed)

Use a replaceable function for the beforeupload handler with shiny updates

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

Description

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.

Attachments (5)

unload-function-replaceable.patch (602 bytes) - added by DavidAnderson 4 years ago.
Make beforeunload function in shiny updates replaceable
unload-function-replaceable2.patch (621 bytes) - added by DavidAnderson 4 years ago.
Second version of patch
unload-function-replaceable3.patch (620 bytes) - added by adamsilverstein 4 years ago.
whitespace fix
before-unload-makeitwork.patch (363 bytes) - added by DavidAnderson 4 years ago.
Patch against current trunk to make it work
before-unload-makeitwork2.patch (370 bytes) - added by DavidAnderson 4 years ago.
Really make it work this time... (forgot to pass on the returned value)

Download all attachments as: .zip

Change History (20)

@DavidAnderson
4 years ago

Make beforeunload function in shiny updates replaceable

#1 follow-up: @DavidAnderson
4 years ago

  • Keywords has-patch shiny-updates added

@DavidAnderson
4 years ago

Second version of patch

#2 @SergeyBiryukov
4 years ago

  • Milestone changed from Awaiting Review to 4.2

#3 in reply to: ↑ 1 @adamsilverstein
4 years ago

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!.

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


4 years ago

#5 @ocean90
4 years ago

  • Priority changed from normal to low
  • Type changed from defect (bug) to enhancement

Please create patches from the root directory.

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

#6 follow-up: @jorbin
4 years ago

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.

#7 @DavidAnderson
4 years ago

@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?

#8 @jorbin
4 years ago

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

In 32126:

Use named function instead of anonymous function

The named function is testable and replaceable.

Fixes #31964
Props DavidAnderson, adamsilverstein, jorbin

#9 @DavidAnderson
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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...

Last edited 4 years ago by DrewAPicture (previous) (diff)

@DavidAnderson
4 years ago

Patch against current trunk to make it work

#10 @DavidAnderson
4 years ago

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
4 years ago

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

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


4 years ago

#12 in reply to: ↑ 6 @jorbin
4 years ago

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?

#13 @DavidAnderson
4 years ago

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?

#14 @jorbin
4 years ago

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

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.

#15 @DavidAnderson
4 years ago

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.

Note: See TracTickets for help on using tickets.