Incorrect action passed to hooks when updating plugin/theme by uploading ZIP file

[desrosj]

When a plugin or theme is being upgraded or downgraded by uploading a new zip file as added in #9757, the action passed in $hook_extra to the upgrader_process_complete action hook is always install, regardless of the action being taken.

[desrosj]

It seems the upgrader_process_complete hook is run twice: once when the zip file is uploaded, and again when the actual overwrite takes place after clicking the proceed button.

50849.diff​ is fixing the second but not the first. I think, ideally, the hook should not fire when uploading a file to update a plugin or theme as the process is no actually complete.

If the decision is to continue firing it in both spots, then $hook_extra should pass along more information as to where the process is at.

[desrosj]

With ​5.5.1 being a shorter cycle, punting this so it can receive proper attention.

[hellofromTonya]

In the patch, the same code is repeated twice. That makes it less maintainable as any future changes must be made in both places.

Both of these classes are extending off of WP_Upgrader. The code can be abstracted to the base class as a protected method. Then it's available for both the theme and plugin classes to directly invoke that new method.

Maybe something like this in the WP_Upgrader class:

<?php
protected function get_hook_extra_action_type() {
        if (
                ! empty( $this->skin->overwrite )
                &&
                in_array( $this->skin->overwrite, array( 'update-theme', 'downgrade-theme' ), true )
        ) {
                return $this->skin->overwrite;
        }

        return 'install';
}

What do you think @desrosj?

[desrosj]

Revisiting this, the hook should not be fired twice. Let's move to fire it only after the replacement is made and the update is complete.

The code can be abstracted to the base class as a protected method. Then it's available for both the theme and plugin classes to directly invoke that new method.

This should be fine! But we'll just need to test all of the different scenarios carefully to confirm.

[whyisjake]

@desrosj, with the 5.5.2 release slated for October 29th, how do you feel about wrapping this up?

[whyisjake]

@desrosj let me know that this could be pushed to 5.5.3.

[JeffPaul]

We're working on a short cycle 5.5.3 release in the very near future and have no further minor releases planned so I'm punting this to 5.6 in hopes it'll have the patch tested and confirmed for that major release.

Trac ticket: https://core.trac.wordpress.org/ticket/50849

[hellofromTonya]

​PR 684 does the following:

• Applies 50849.diff patch
• Refactors to encapsulate the theme and plugin code into the base class, ie method get_hook_extra_action
• Prevents the hook from running during install action. It now only runs when it's actually completed, ie update and downgrade.

Testing Results:

• Upload plugin with same version: hook ran 1x and 'action' = 'update-plugin'
• Upload plugin with newer version: hook ran 1x and 'action' = 'update-plugin'
• Upload plugin with older version: hook ran 1x and 'action' = 'downgrade-plugin'
• Upload theme with same version: hook ran 1x and 'action' = 'update-theme'
• Upload theme with newer version: hook ran 1x and 'action' = 'update-theme'
• Upload theme with older version: hook ran 1x and 'action' = 'downgrade-theme'

@desrosj can you do a code review and test it out please?

[hellofromTonya]

RC1 is tomorrow. Per ​conversation in Slack, moving this one to Future Release and @desrosj will review.

Please punt to Future Release, but I’ll add a task for myself to revisit that after 5.6.

[francina]

@desrosj how do you feel about this ticket for 5.8? @hellofromTonya do you think it needs a refresh?
Thanks!

[justinahinon]

It would be nice to know how someone who have no experience with the upgrader can reproduce the issue being mentioned and also test the patch/PR.

[desrosj]

Hi @justinahinon! Good point. Here are some steps to test:

Attach a function to the upgrader_process_complete action hook that will log information. This should print the contents of the second parameter to the debug log:

add_action( 'upgrader_process_complete', function( $upgrader, $hook_info ) {
error_log( print_r( $hook_info, true ) );
}, 10, 2 );

• Attempt to upgrade and downgrade a plugin and theme by uploading a zip file.
• The information should print to the log twice: once after uploading the zip, and once after clicking the confirmation to replace the installed version with the uploaded version.
• Also, observe that the action index of the array is always install.
• Apply patch and repeat.
• Information should only be printed once, and only after clicking the confirmation (the upload completing does not signify a "complete" process.
• The action should now be set to upgrade-plugin|theme or downgrade-plugin|theme.