Make WordPress Core

Opened 2 years ago

Closed 17 months ago

#56057 closed task (blessed) (fixed)

Add conditional to WP_Upgrader::install_package() for Rollback testing

Reported by: afragen's profile afragen Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.2 Priority: normal
Severity: normal Version: 6.1
Component: Upgrade/Install Keywords: has-patch
Focuses: Cc:

Description (last modified by afragen)

In order to facilitate testing of the Rollback feature using the Rollback Update Failure feature plugin, a conditional testing for the feature plugin is needed to utilize the new move_dir() function.

@SergeyBiryukov reports precedent for this.

Once committed, PR2225 will be updated to remove it.

Updated: New PR allows for more focused filtering of the function copying the downloaded update into the proper place. Located in WP_Upgrader::install_package()

Change History (43)

This ticket was mentioned in PR #2874 on WordPress/wordpress-develop by afragen.


2 years ago
#1

Adding a conditional to WP_Upgrader::install_package() for testing of Rollback feature and Rollback feature plugin.

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

#2 @davidbaumwald
2 years ago

  • Type changed from task (blessed) to feature request

I'm converting this to a feature to match the rest of the project.

I found the precedent @SergeyBiryukov mentioned...

[25580], [26865], and [32625].

#3 @pbiron
2 years ago

  • Description modified (diff)

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


2 years ago

#5 @audrasjb
2 years ago

  • Keywords early added

The patch looks good to me.

Are we sure the below indentation is compliant to WP coding standards?

                if ( class_exists( 'Rollback_Update_Failure\WP_Upgrader' )
                        && function_exists( '\Rollback_Update_Failure\move_dir' )
                ) {

afragen commented on PR #2874:


2 years ago
#6

Now single line conditional.

#7 @costdev
2 years ago

@SergeyBiryukov is there anything left to do on PR 2874 before commit?

#8 @SergeyBiryukov
2 years ago

Added a comment to explain what this is :) I think it's ready for commit.

#9 @SergeyBiryukov
2 years ago

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

In 53578:

Upgrade/Install: Add a conditional to facilitate testing of the Rollbacks feature project.

The Rollback Update Failure feature project creates a temporary backup of plugins and themes before updating. This aims to make the update process more reliable and ensure that if a plugin or theme update fails, the previous version can be safely restored.

If the Rollback Update Failure plugin is installed, WP_Upgrader::install_package() will use the move_dir() function from there for better performance. Instead of copying a directory from one location to another, it uses the rename() PHP function to speed up the process, which is instrumental in creating a temporary backup without a delay. If the renaming failed, it falls back to copy_dir() WP function.

This conditional aims to facilitate broader testing of the feature. It is temporary, until the plugin is merged into core.

Props afragen, pbiron, costdev, davidbaumwald, audrasjb, jrf, SergeyBiryukov.
Fixes #56057. See #51857, #54166.

#11 follow-up: @peterwilsoncc
2 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I am not fond of the snowflake code for a feature plugin here. While WordPress has a commitment to backward compatibility it is going to have to stick around for a long time.

Is there a reason the use of do_action( "update-custom_{$action}" ); in src/wp-admin/update.php wasn't expanded to allow the use of custom updaters within the built-in actions?

#12 @costdev
2 years ago

Temporary conditionals for features have precedent in Core as mentioned in this comment.

This conditional is temporary and only during Rollback's testing. It doesn't fall under the backward compatibility promise as the conditional will never make it into a major release.

We had approval from 3-4 committers on this approach. I appreciate that it's not the ideal, but the ideal is not necessary here. The Make post to Call for Testing is also already published.

#13 @peterwilsoncc
2 years ago

There is a substantial difference between adding an action and checking for a specific class name within a feature plugin that is under development.

There are also circumstantial difference, the existing hook update-custom_{$action} could have been expanded for use as a robust and future friendly solution.

#14 in reply to: ↑ 11 @SergeyBiryukov
2 years ago

Replying to peterwilsoncc:

Is there a reason the use of do_action( "update-custom_{$action}" ); in src/wp-admin/update.php wasn't expanded to allow the use of custom updaters within the built-in actions?

Thanks for bringing this up! I had not considered using that hook and would be happy to explore that path.

#15 @afragen
2 years ago

It seems that install_package() fires much before this hook. We need to prevent the copy_dir() in install_package() from firing except for when we want.

Not sure we can utilize this hook.

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


23 months ago

#17 @audrasjb
23 months ago

As per today's bugscrub, the idea is to keep it reopened to see if a filter could be used instead of checking for the plugin class name.

Also, it will stay reopened to make sure the temporary conditional is removed when 6.1 is branched, if the feature can't make it to 6.1.

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


22 months ago

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


22 months ago

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


22 months ago

#21 @audrasjb
22 months ago

  • Keywords early removed

Removing the early keyword as per today's bugscrub (and the above comment).

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


21 months ago

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


21 months ago

#24 @davidbaumwald
20 months ago

  • Type changed from feature request to task (blessed)

After confirming with @SergeyBiryukov, moving this to a task so work can continue beyond Beta.

#25 @SergeyBiryukov
20 months ago

In 54484:

Upgrade/Install: Revert a temporary conditional for testing the Rollbacks feature project.

This will be readded in 6.2-alpha after a 6.1 branch is created.

Follow-up to [53578].

See #56057.

#26 @SergeyBiryukov
20 months ago

  • Milestone changed from 6.1 to 6.2

#27 @SergeyBiryukov
20 months ago

In 54643:

Upgrade/Install: Add a conditional to facilitate testing of the Rollbacks feature project.

The Rollback Update Failure feature project creates a temporary backup of plugins and themes before updating. This aims to make the update process more reliable and ensure that if a plugin or theme update fails, the previous version can be safely restored.

If the Rollback Update Failure plugin is installed, WP_Upgrader::install_package() will use the move_dir() function from there for better performance. Instead of copying a directory from one location to another, it uses the rename() PHP function to speed up the process, which is instrumental in creating a temporary backup without a delay. If the renaming failed, it falls back to copy_dir() WP function.

This conditional aims to facilitate broader testing of the feature. It is temporary, until the plugin is merged into core.

Follow-up to [53578], [54484].

Props afragen, pbiron, costdev, davidbaumwald, audrasjb, jrf, SergeyBiryukov.
See #56057.

This ticket was mentioned in Slack in #core-upgrade-install by costdev. View the logs.


18 months ago

This ticket was mentioned in Slack in #core-upgrade-install by afragen. View the logs.


17 months ago

This ticket was mentioned in PR #3791 on WordPress/wordpress-develop by @afragen.


17 months ago
#30

Instead of the custom conditional for testing Rollback, add a filter hook that enables the ability to use custom code in place of copy_dir() in WP_Upgrader::install_package(). If the filter returns the default, false, then the default copy_dir() will be used.

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

#31 @peterwilsoncc
17 months ago

I don't think the filter in the linked PR #3791 is well designed.

Instead of introducing a generic hook (be it filter or action) that could be used in multiple use cases, it's introducing a filter that is pretty tightly limited to the use case of testing the rollback plugin.

However, once the rollback plugin is tested the filter needs to be maintained for the forseable future by WordPress maintainers.

#32 follow-up: @afragen
17 months ago

This filter can also be used by VirtualBox users to switch back to copy_dir() if rollback is committed. Thereby maintaining its use case.

#33 in reply to: ↑ 32 @peterwilsoncc
17 months ago

Replying to afragen:

This filter can also be used by VirtualBox users to switch back to copy_dir() if rollback is committed. Thereby maintaining its use case.

To repeat myself, again, it is not the job of VirtualBox users to work around limitations in core. It is quite the reverse.

If the intent of this filter is to be a get-out-of-jail-free card for VB users then I am more reluctant to have it in core.

#34 @costdev
17 months ago

If the intent of this filter is to be a get-out-of-jail-free card for VB users then I am more reluctant to have it in core.

To be fair, the PR explains that this wasn't the intent of this filter:

Instead of the custom conditional for testing Rollback, add a filter hook that enables the ability to use custom code in place of copy_dir() in WP_Upgrader::install_package(). If the filter returns the default, false, then the default copy_dir() will be used.

i.e. To avoid having code in Core that specifies a feature plugin, and instead using established Core extensibility features to enable extenders (including feature plugins) to replace functionality where required.

#35 @azaozz
17 months ago

As far as I understand it there are no differences between the use cases for the filter in PR #3791 and function_exists() in [54643]. Both are intended to facilitate development of the Rollback feature plugin. In that terms both are temporary and should not be present in a WordPress release, only in development (alpha, beta, etc.) versions.

If there are other use cases for such filter that may warrant having it permanently in core, please open another (enhancement) ticket about them.

Last edited 17 months ago by azaozz (previous) (diff)

#36 @afragen
17 months ago

  • Description modified (diff)

#37 @peterwilsoncc
17 months ago

As discussed in the slack thread yesterday (requires login via make.wordpress.org/chat) I think the preferable approach it to use the update-custom_{$action} and update-core-custom_{$action} hooks to customize the upgrade process for the rollback plugin.

Changing the action on the form submission on either the plugin or core upgrade screen will bypass Core's existing update code and allow you to replace it wholesale.

This will offer greater flexibility than the filter proposed above/checking for a particular callable.

#38 @costdev
17 months ago

While utilizing the update-core-custom_{$action} and update-custom_{$action} hooks can work for some upgrade paths, it will not work for all - WP-CLI for example.

This means that testing the rollback plugin via WP-CLI, for example, will require testers to manually change Core to use move_dir() instead of copy_dir().

As we have to ensure that BC is protected in all upgrade paths, the two hooks above are certainly useful, but not a complete replacement for testing the plugin - especially for non-dev contributors who may have WP-CLI, but are unfamilar/uncomfortable with editing code

Any ideas of how paths such as WP-CLI could be covered are much appreciated.

Last edited 17 months ago by costdev (previous) (diff)

#39 @peterwilsoncc
17 months ago

You could make a custom command and extend WPCLI\Plugin_Command or \WP_CLI\CommandWithUpgrade if needs be.

#40 @afragen
17 months ago

Per @azaozz comment above, I am removing the new filter from discussion here and will create a new ticket at the appropriate time.

#41 @afragen
17 months ago

It should be OK to revert r54643

Testing for Rollback is in process via other methods as @peterwilsoncc has described above.

#42 @SergeyBiryukov
17 months ago

In 55055:

Upgrade/Install: Revert a temporary conditional for testing the Rollbacks feature project.

The Rollback Update Failure feature project has been split into two plugins for testing:

  • Faster Updates speeds up plugin or theme updates by moving files rather than copying them, thus decreasing the memory usage and reducing the chance of timeouts or running out of disk space during updates.
  • Rollback Update Failure creates a temporary backup of plugins and themes before updating. This aims to make the update process more reliable and ensure that if a plugin or theme update fails, the previous version can be safely restored.

The current priority of the feature project is to test the new move_dir() function, which offers better performance than copy_dir(). Instead of copying a directory in a recursive manner file by file from one location to another, move_dir() uses the rename() PHP function to speed up the process, which is instrumental in updating large plugins without a delay. If the renaming failed, it falls back to the copy_dir() WP function.

The move_dir() function is self-contained in the Faster Updates plugin and does not require any special hooks in core, so the conditional previously added to WP_Upgrader::install_package() to facilitate testing is no longer needed and can be removed.

Follow-up to [53578], [54484], [54643].

Props afragen, costdev, peterwilsoncc.
See #56057, #57375, #57386.

#43 @afragen
17 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.