WordPress.org

Make WordPress Core

Opened 4 months ago

Last modified 9 days ago

#50849 new defect (bug)

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

Reported by: desrosj Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 5.5
Component: Upgrade/Install Keywords: has-patch needs-testing
Focuses: Cc:

Description

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.

Attachments (1)

50849.diff (1.6 KB) - added by desrosj 4 months ago.

Download all attachments as: .zip

Change History (20)

@desrosj
4 months ago

#1 follow-up: @desrosj
4 months ago

  • Milestone changed from Awaiting Review to 5.5.1

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.

#2 @desrosj
4 months ago

  • Version changed from trunk to 5.5

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


3 months ago

#4 @johnbillion
3 months ago

  • Keywords needs-testing added

This ticket was mentioned in Slack in #core-auto-updates by audrasjb. View the logs.


3 months ago

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


3 months ago

#7 @desrosj
3 months ago

  • Milestone changed from 5.5.1 to 5.5.2

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

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


6 weeks ago

#9 in reply to: ↑ 1 @hellofromTonya
5 weeks ago

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?

#10 @desrosj
5 weeks ago

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.

#11 @whyisjake
4 weeks ago

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

#12 @whyisjake
4 weeks ago

  • Milestone changed from 5.5.2 to 5.5.3

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

#13 @JeffPaul
4 weeks ago

  • Milestone changed from 5.5.3 to 5.6

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.

This ticket was mentioned in Slack in #core-auto-updates by audrasjb. View the logs.


3 weeks ago

#16 @hellofromTonya
3 weeks ago

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?

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


3 weeks ago

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


9 days ago

#19 @hellofromTonya
9 days ago

  • Milestone changed from 5.6 to Future Release

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.

Note: See TracTickets for help on using tickets.