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

[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()

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

[davidbaumwald]

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

I found the precedent @SergeyBiryukov mentioned...

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

[audrasjb]

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' )
                ) {

Now single line conditional.

[costdev]

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

[SergeyBiryukov]

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

[SergeyBiryukov]

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.

Thanks for the PR! Merged in https://core.trac.wordpress.org/changeset/53578.

[peterwilsoncc]

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?

[costdev]

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.

[peterwilsoncc]

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.

[SergeyBiryukov]

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.

[afragen]

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.

[audrasjb]

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.

[audrasjb]

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

[davidbaumwald]

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

[SergeyBiryukov]

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.

[SergeyBiryukov]

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.

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

[peterwilsoncc]

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.

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

[peterwilsoncc]

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.

[costdev]

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.

[azaozz]

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.

[peterwilsoncc]

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.

[costdev]

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.

[peterwilsoncc]

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

[afragen]

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

[afragen]

It should be OK to revert r54643

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

[SergeyBiryukov]

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.