Add filter to WP_Upgrader::install_package for copy_dir()

[afragen]

copy_dir() is a recursive file copy for directories and is used when updating plugins, themes, and core.

There are several tickets related to timeouts and optimization of the update process. As part of Rollback #51857, a new function move_dir() and a new filter #56057 were introduced. This ticket is an improvement upon #56057.

Optimizing the plugin/theme update process can be solved using the move_dir() function and a filter similar to #56057.

#57375 was opened to add move_dir() to core. This ticket is to add the new filter.

The new filter would easily allow the substitution of move_dir for copy_dir adding more efficiency to the plugin/theme update process by having the user add a simple filter. This could improve the efficiency and performance for 99+% of users who opt-in and will likely fix #53832, #54166, and #34676.

add_filter( 'upgrader_copy_directory', function($callback) {
    return 'move_dir';
}, 10, 1 );

I believe the use cases are sufficient to merit inclusion to core, additionally both of these have been tested rigorously in the Rollback PR/feature plugin.

Once/If the PR is committed I will submit a plugin to dot org allowing anyone to activate this feature.

copy_dir() is a recursive file copy for directories and is used when updating plugins, themes, and core.

There are several tickets related to timeouts and optimization of the update process. As part of Rollback #51857, a new function move_dir() and a new filter #56057 were introduced. This ticket is an improvement upon #56057.

Optimizing the plugin/theme update process can be solved using the move_dir() function and a filter similar to #56057.

#57357 was opened to add move_dir() to core. This ticket is to add the new filter.

The new filter would easily allow the substitution of move_dir for copy_dir adding more efficiency to the plugin/theme update process by having the user add a simple filter. This could improve the efficiency and performance for 99+% of users who opt-in and will likely fix #53832, #54166, and #34676.

{{{php
add_filter( 'upgrader_copy_directory', function($callback) {

return 'move_dir';

}, 10, 1 );
}}}

I believe the use cases are sufficient to merit inclusion to core, additionally both of these have been tested rigorously in the Rollback PR/feature plugin.

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

[peterwilsoncc]

As ​discussed in Slack and posted on #56057 a couple of weeks ago, I don't think this is a suitable filter for Core.

It provides no general purpose benefit to theme and plugin developers.

Please use the custom actions hooks for testing the rollback plugin. I'm going to close this off as wontfix and the snowflake code to revert the callable sniff can be reverted on the original ticket.

[afragen]

This has nothing to do with testing Rollback. Perhaps you should read the description and perhaps try to understand the potential problems that such a filter can solve.

Peter, perhaps you could articulate why this filter on its own is unsuitable for core on its own merits. I have explained exactly how, using a simple plugin, this filter could benefit 99+% of all WordPress users.

Please don’t let your personal bias against Rollback impair your judgment.

[peterwilsoncc]

It's exactly the same filter that has already been discussed at lenght on #56057 in the links above. It was previously proposed in ​PR3791.

Both Aaron Ozz and I provided feedback at the time.

Please don’t let your personal bias against Rollback impair your judgment.

I do not have any bias against rollbacks or improving the updating of plugins, themes or Core. However, I am unwilling to have the same discussion on this ticket that I've linked to above and had on #56057.

[afragen]

Peter you commented in #56057.

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.

I'm not certain of your definition of "snowflake code" but this PR corresponds to a generic hook that has additional use cases aside from testing a feature plugin. Additionally, I have provided use cases and opened this ticket as @azaozz requested.

A simple plugin such as, ​Faster Updates, could be used to improve the performance and efficiency for most users and it has a high likelihood of fixing #53832, #54166, and #34676 for those users.

[peterwilsoncc]

Please refer to my comment from June suggesting the update-custom_{$action} hook as a suitable generic hook.

As discussed in Slack last week, the sibling hook update-core-custom_{$action} is available on the update core pages.

These hooks have been used previously when the updaters have been updated.

I've been suggesting this course of action for over six months. I'm willing and eager to test rollbacks but when my feedback isn't listened to it will slow the process down.

I suggest reclosing this ticket and reviewing all the notes Tonya and I (among others) have provided so a robust approach supporting known environments can be found.

[SergeyBiryukov]

Replying to peterwilsoncc:

Please refer to my comment from June suggesting the update-custom_{$action} hook as a suitable generic hook.

As discussed in Slack last week, the sibling hook update-core-custom_{$action} is available on the update core pages.

I might be missing something, but wouldn't using these hooks to replace a single copy_dir() call, while technically possible, require recreating the entire core, plugin, and theme update processes? That seems like quite an overkill and a challenge to maintain.

Could there be an easier, more reliable solution to replace the copy_dir() usage in this context?

[peterwilsoncc]

Yes, using those hooks would require some amount of reproducing Core. The last big change to the updater, Shiny Updates, included a custom list table, replaced Core ajax functionality. It's a well travelled path.

My experience is that this is is substantially more complicated than replacing copy_dir and using the custom hooks will allow for the various suggestions made over the last few months to be tested.

[afragen]

My experience is that this is is substantially more complicated than replacing copy_dir and using the custom hooks will allow for the various suggestions made over the last few months to be tested.

What precisely do you think this is?

This ticket is only about adding a generic hook.

[peterwilsoncc]

"This" is the rollbacks plugin mentioned in the description of this ticket.

As I mentioned above, both @azaozz and I reviewed the code in the ​linked pull request #3805 prior to Christmas in its earlier incarnation in ​pull request #3791 linked to #56057.

Both of us have expressed reservations to exactly the same code this ticket proposes in the ticket linked in the description, #56057. It was out of that discussion that the use of the generic hooks was resuggested,

If asked again, I am happy to repost my suggestion that other generic hooks be used rather than introducing a new hook that serves the single purpose of rollback testing.

[afragen]

Firstly, please re-read the description. This ticket is not about testing Rollback. The reference to Rollback was for background.

My understanding of @azaozz’s comment was the we’re trading one hook for another to test Rollback, what are other use cases, and it should be in another ticket. I believe I have addressed each of those.

This ticket is only about a simple hook addition, it is not about changing the update process. Don’t extrapolate it into anything else.

Please explain why a simple hook addition is bad. This is not part of Rollback. This isn’t even close to shiny updates.

[azaozz]

@peterwilsoncc, @afragen, sorry for not being clear enough. Yes, my suggestion was for a new ticket to be opened that is not related to the feature plugin. Imho the use cases for having a (permanent) filter can be discussed better here.

Looking at the PR, couple thoughts:

1. The proposed filter is "binary". The return value can only be 'move_dir' or the default. Such binary filters may be better written as apply_filters( 'upgrader_use_move_dir', false ); and used with add_filter( 'upgrader_use_move_dir', '__return_true' );. Also perhaps passing some more context to the filter like $source, $remote_destination would make it eventually more useful, perhaps?

2. Looking at #57375 and the new move_dir() function, why would WP need a filter to use it if it is better/faster, and has been tested sufficiently? If there still are any stability concerns, perhaps a filter to revert to the old copy_dir() may be needed? However looking at the patch there, it falls back to copy_dir(), so not sure if a filter is needed at all. Will also comment on #57375.

[afragen]

Replying to azaozz:

@peterwilsoncc, @afragen, sorry for not being clear enough. Yes, my suggestion was for a new ticket to be opened that is not related to the feature plugin. Imho the use cases for having a (permanent) filter can be discussed better here.

Looking at the PR, couple thoughts:

1. The proposed filter is "binary". The return value can only be 'move_dir' or the default. Such binary filters may be better written as apply_filters( 'upgrader_use_move_dir', false ); and used with add_filter( 'upgrader_use_move_dir', '__return_true' );. Also perhaps passing some more context to the filter like $source, $remote_destination would make it eventually more useful, perhaps?

At the moment the filter is "binary". It's entirely possible that someone might create a better solution. But yes mostly "binary".

For an upgrader_use_move_dir filter:
__return_true would be great for the high majority of users, but VirtualBox users would need to disable this - and may only realize this after encountering the VirtualBox bug (which results in broken upgrades).

__return_false is a safer option to allow most users to opt-in (which could also be done by installing a "Faster Updates" canonical plugin, for example), but should be documented that VirtualBox users should not use the filter. It's worth noting that some users may not know that their environment runs on VirtualBox.

This filter PR can be easily modified for your suggestion.

2. Looking at #57375 and the new move_dir() function, why would WP need a filter to use it if it is better/faster, and has been tested sufficiently? If there still are any stability concerns, perhaps a filter to revert to the old copy_dir() may be needed? However looking at the patch there, it falls back to copy_dir(), so not sure if a filter is needed at all. Will also comment on #57375.

move_dir() is more appropriate, faster, and has been tested. However, during testing, VirtualBox environments encountered an longstanding issue in VirtualBox's filesystem cache when rename() was followed with unlink(). This pair aren't in move_dir(), but just noting that should both run on the same path, this bug may be encountered and I think it has been encountered by @peterwilsoncc.

In terms of who this affects, ​Oracle does not support VirtualBox for production. While it's possible to use it for hosting, I don't think that we should support what Oracle doesn't. This means it will only affect development environments using VirtualBox, who have chosen to use the filter, so a small portion of a small portion of a small portion of users.

Some more background on the VirtualBox issue.
​https://www.virtualbox.org/ticket/8761#comment:24
​https://www.virtualbox.org/ticket/17971

[afragen]

Examples of how the filter could look. Not sure which is better.

if ( ! apply_filters( 'upgrader_use_move_dir', false ) ) {
	$result = copy_dir( $source, $remote_destination );
} else {
	$result = move_dir( $source, $remote_destination );
}

if ( apply_filters( 'upgrader_use_move_dir', false ) ) {
	$result = move_dir( $source, $remote_destination );
} else {
	$result = copy_dir( $source, $remote_destination );
}

[peterwilsoncc]

Replying to afragen:

In terms of who this affects, ​Oracle does not support VirtualBox for production. While it's possible to use it for hosting, I don't think that we should support what Oracle doesn't. This means it will only affect development environments using VirtualBox, who have chosen to use the filter, so a small portion of a small portion of a small portion of users.

Some more background on the VirtualBox issue.
​https://www.virtualbox.org/ticket/8761#comment:24
​https://www.virtualbox.org/ticket/17971

I think this conclusion is incorrect. VitualBox users should not have to make any changes to work around core.

MAMP, WAMP, VirtualBox and other developer environments are not recommended in hosting environments but are all suitable for local development environments.

WordPress.com VIP provide guides for two VB based development environments, ​Chassis and ​VVV. Altis DXP also provides a ​VB based local environment.

The intended workflow for these is to upgrade themes and plugins locally, test and then push to production. If updating locally is unavailable to users of these platforms without a plugin it's a bug, not a feature.

[afragen]

Replying to peterwilsoncc:

Replying to afragen:

In terms of who this affects, ​Oracle does not support VirtualBox for production. While it's possible to use it for hosting, I don't think that we should support what Oracle doesn't. This means it will only affect development environments using VirtualBox, who have chosen to use the filter, so a small portion of a small portion of a small portion of users.

Some more background on the VirtualBox issue.
​https://www.virtualbox.org/ticket/8761#comment:24
​https://www.virtualbox.org/ticket/17971

I think this conclusion is incorrect. VitualBox users should not have to make any changes to work around core.

MAMP, WAMP, VirtualBox and other developer environments are not recommended in hosting environments but are all suitable for local development environments.

WordPress.com VIP provide guides for two VB based development environments, ​Chassis and ​VVV. Altis DXP also provides a ​VB based local environment.

The intended workflow for these is to upgrade themes and plugins locally, test and then push to production. If updating locally is unavailable to users of these platforms without a plugin it's a bug, not a feature.

The above section by me a response to @azaozz about his following statement.

2. Looking at #57375 and the new move_dir() function, why would WP need a filter to use it if it is better/faster, and has been tested sufficiently?

I was pointing out why the default state of the filter should use copy_dir(). If the logic in the filter does not respect this logic please let me know. I've been on night call for the past week, my apologies if I got the logic reversed. The intent is for those wishing to use move_dir() to opt-in and need to use the filter.

I plan on adding a canonical plugin to the plugin repository to make this simpler. We are currently working on something to this plugin to let VB users also use move_dir() if they too opt-in.

[azaozz]

Thanks, I think I understand the problem here better now.

VirtualBox users should not have to make any changes to work around core.

I agree. It is unfortunate that this bug hasn't been fixed in VB. If I understand how this works (looking at ​https://www.virtualbox.org/ticket/8761#comment:24) seems it would be very hard to trigger the bug. For this two directories with identical names will have to be moved, and the first has to be moved to the exact spot where the second was. Then if a file that exists in both directories is deleted in the first directory (that now occupies the spot where the second directory was), it gets deleted in the second directory.

Wondering if VirtualBox can be detected from PHP running in the guest OS? Tried searching the web but no clear answer. Seems it may be possible, for example looking at the video card (or other hardware) manufacturer string. If yes, I wouldn't be opposed to having a separate function to detect it.

Then it will be possible to use the new move_dir() by default, and this filter won't probably be needed.

Another, "last resort" solution may be to "hide" the new move_dir() functionality inside one of the rollback functions and use it only for rollbacks. Not ideal but will avoid triggering the VB bug. The advantage is that it will work for "everybody" out of the box.

[afragen]

Replying to azaozz:

Thanks, I think I understand the problem here better now.

VirtualBox users should not have to make any changes to work around core.

I agree. It is unfortunate that this bug hasn't been fixed in VB. If I understand how this works (looking at ​https://www.virtualbox.org/ticket/8761#comment:24) seems it would be very hard to trigger the bug. For this two directories with identical names will have to be moved, and the first has to be moved to the exact spot where the second was. Then if a file that exists in both directories is deleted in the first directory (that now occupies the spot where the second directory was), it gets deleted in the second directory.

It doesn't seem to be hard to trigger the bug from VirtualBox. It seems to happen more consistently when a VB user is updating plugins that are large with a complex folder structure.

Much of the issues with VB have been documented in the following locations. I'm sorry, but it's a lot. If you follow the Rollback testing call to action and you are using a VB environment you will likely encounter it.

https://core.trac.wordpress.org/ticket/51857
​https://github.com/WordPress/wordpress-develop/pull/2225
​https://make.wordpress.org/core/2022/06/26/rollback-feature-testing-call-to-action/
​https://github.com/costdev/wp-virtualbox-testing

Wondering if VirtualBox can be detected from PHP running in the guest OS? Tried searching the web but no clear answer. Seems it may be possible, for example looking at the video card (or other hardware) manufacturer string. If yes, I wouldn't be opposed to having a separate function to detect it.

Then it will be possible to use the new move_dir() by default, and this filter won't probably be needed.

You will find in the above discussions as well as a long back scroll through #core-upgrade-install, that detecting when someone is using VB is likely impossible.

Another, "last resort" solution may be to "hide" the new move_dir() functionality inside one of the rollback functions and use it only for rollbacks. Not ideal but will avoid triggering the VB bug. The advantage is that it will work for "everybody" out of the box.

The filter is where move_dir() is hidden from users who don't opt-in.

The point of these separate tickets for the WP_Upgrader::install_package filter and adding move_dir() was to move away from the imposed testing requirements of Rollback as defined/discussed in #51857.

I'm hoping the Faster Updates plugin and the inclusion of a filter might be able to allow VB users to use move_dir() without issue. More testing of that can more easily be done post-commit.

[costdev]

Just adding some clarification on how the VirtualBox bug can be reproduced based on my testing during the past year:

Short version

• Move oldname to another location.
• Move new version newname into place, renamed to oldname.
• Delete oldname from the other location. 🐞

Long version

Directories

/parent1/DirectoryA
/parent2/DirectoryB
/parent3/

Moves

• Move DirectoryA to /parent3/DirectoryA
• Move DirectoryB to /parent1/DirectoryA

Moves Result

/parent1/DirectoryA
/parent2/
/parent3/DirectoryA

Delete

• Delete files in /parent3/DirectoryA

Expected Delete Result

• /parent1/DirectoryA will be intact. ✅
• /parent2/ will be empty. ✅
• /parent3/ will be empty. ✅

Actual Delete Result in VirtualBox

• /parent1/DirectoryA will be partially deleted. ❌
• /parent2/ will be empty. ✅
• /parent3/DirectoryA will be partially deleted. ❌

Notes

• The bug is likely to occur for a simple directory structure, but will always occur with multiple nested directories and files.
• clearstatcache()/wp_opcache_invalidate() don't resolve the issue (note: move_dir() is not recursive, so calling clearstatcache()/wp_opcache_invalidate() on each moved file would mean looping $wp_filesystem->dirlist(), negating the performance benefits of rename()). Either way, the issue is with VirtualBox's cache, not PHP's cache, so PHP will just keep getting the same incorrect information from VirtualBox until its cache is cleared.
• Detection of VirtualBox would at least require system level command execution. If shell_exec/exec() in Core is a non-starter, then exploration into the reliability of detecting VirtualBox would also be a non-starter.

[SergeyBiryukov]

Replying to azaozz:

Another, "last resort" solution may be to "hide" the new move_dir() functionality inside one of the rollback functions and use it only for rollbacks. Not ideal but will avoid triggering the VB bug. The advantage is that it will work for "everybody" out of the box.

Part of the issue is that copy_dir() can be quite slow for large plugins (since it copies each file individually and is recursively called for subdirectories), so using it both to create a backup of the existing plugin version and to install the new plugin version makes the process twice as slow and more prone to timeouts, the very issue that move_dir() was intended to solve.

This got me thinking though, if we're not introducing rollbacks at this time, then the conditions to trigger the bug as outlined in comment:23 should never happen:

• Move oldname to another location.
• Move new version newname into place, renamed to oldname.
• Delete oldname from the other location. 🐞

What happens instead:

• Delete oldname from the original location.
• Move new version newname into place, renamed to oldname.

So perhaps it is safe enough to just use move_dir() here? The filter is then just an additional precaution to make this opt-in, which may or may not be necessary.

[afragen]

Replying to SergeyBiryukov:

Replying to azaozz:

Another, "last resort" solution may be to "hide" the new move_dir() functionality inside one of the rollback functions and use it only for rollbacks. Not ideal but will avoid triggering the VB bug. The advantage is that it will work for "everybody" out of the box.

Part of the issue is that copy_dir() can be quite slow for large plugins (since it copies each file individually and is recursively called for subdirectories), so using it both to create a backup of the existing plugin version and to install the new plugin version makes the process twice as slow and more prone to timeouts, the very issue that move_dir() was intended to solve.

This got me thinking though, if we're not introducing rollbacks at this time, then the conditions to trigger the bug as outlined in comment:23 should never happen:

• Move oldname to another location.
• Move new version newname into place, renamed to oldname.
• Delete oldname from the other location. 🐞

What happens instead:

• Delete oldname from the original location.
• Move new version newname into place, renamed to oldname.

So perhaps it is safe enough to just use move_dir() here? The filter is then just an additional precaution to make this opt-in, which may or may not be necessary.

@SergeyBiryukov I think we might need to test that hypothesis first using a canonical plugin. The only difference is that Rollback will create at least 2 moves and possibly 3. We can then test the filter that might fix the issues with VB if VB has exec_shell(). If that does test we’ll then we could change the filter so the default is move_dir().

[costdev]

You're right @SergeyBiryukov, move_dir() itself does not create the conditions for the VirtualBox bug to occur. Those conditions depend on the usage of move_dir() (read: rename()), and unlink().

In that sense, move_dir() can be considered as technically safe as any other function: It works great, but use it in a bad combination and you'll have problems.

I'd like to avoid introducing a filter to test whether move_dir() should be the default, and instead create a test plugin that utilizes the update-core-custom_ hooks to simply replace $result = copy_dir() with $result = move_dir().

This makes testing much simpler:

• Testers don't need to apply a PR to test in various environments.
• We don't need to introduce a permanent filter (or one that is reverted at Beta 1) just to test if move_dir() breaks on VirtualBox in this context.
• We don't need to write a Canonical plugin that covers update-core.php, plugins.php, plugin-install.php, themes.php, theme-install.php, automatic updates and WP-CLI. We only need to make sure that updates via one of these paths works as expected in a test plugin. So, for a test plugin using update-core-custom_ hooks, every tester would just update via update-core.php.
• The hook callbacks inject Custom_Upgrader::install_package(), a duplicate of WP_Upgrader::install_package() with the only change being copy_dir() to move_dir().
• We test the plugin in VirtualBox environments such as Chassis and VVV.
• If VirtualBox testing proves safe, then we also do standard testing on Linux, Windows and Mac (this has already been done within the Rollback feature project, but we can still do a little extra to have a more complete result for the test plugin).
• If all proves safe, we have an in-Core enhancement rather than a Canonical plugin-based enhancement.
• If VirtualBox testing does not prove safe, then we have solid evidence that an opt-in filter to use move_dir() is the only reasonable way to ensure that Core continues to work for all users, and those who don't use VirtualBox can opt-in to the upgrader_use_move_dir filter to reap the performance benefits, reducing the time, memory, and diskspace used for updates. A Canonical plugin can be released to provide an as-close-to-Core-as-possible path for users to take advantage of this.

Finally, while introducing a temporary filter for testing is simple, I'm trying to minimize work for committers, and noise in the ticket/repo's commit history/scrubs. As a bonus, I've already written ​an implementation that utilizes the update-core-custom_ hooks, injecting Custom_Upgrader::install_package() using move_dir() and it's ready to be packaged for testing. Previously, this implementation didn't cover all of the required paths for a Canonical plugin. Now that we only need to cover one path to test whether move_dir() is safe by itself in VirtualBox, the implementation is sufficient for testing.

If this way forward is supported, I believe that between all of us in this ticket and the move_dir() ticket, we can easily have it sufficiently tested in the next couple of weeks.

Thoughts?

[afragen]

I like @costdev's above plan for testing VirtualBox without #56057. This should mean that #56057 can be reverted with the understanding that after testing it would simply be a switch to move_dir() or we would need the PR presented here.

I still think it's appropriate to commit #57375 as move_dir() will be a great addition to Core and is needed regardless of which path we end up taking.

[SergeyBiryukov]

Replying to afragen:

I like @costdev's above plan for testing VirtualBox without #56057. This should mean that #56057 can be reverted with the understanding that after testing it would simply be a switch to move_dir() or we would need the PR presented here.

Same here, the plan outlined in comment:26 sounds great to me.

I still think it's appropriate to commit #57375 as move_dir() will be a great addition to Core and is needed regardless of which path we end up taking.

I agree, the PR on #57375 looks good to me. That said, I believe we generally only introduce new core functions once they are actually used in core, so perhaps it can be committed as soon as the approach here is finalized.

[azaozz]

Replying to SergeyBiryukov:

Same here, the plan outlined in comment:26 sounds great to me.

Sounds good here too.

I believe we generally only introduce new core functions once they are actually used in core

Was just thinking that too. If move_dir() is opt-in, i.e. it is unused/disabled by default, and a plugin is needed to enable/use it, why not add the function to the plugin?

Then, if there is a workaround, or VB can be detected, or move_dir() is opt-out instead of opt-in, the function can be moved to core and the plugin discontinued.

[afragen]

@azaozz move_dir() is currently in the Faster Updates testing plugin.

​https://github.com/afragen/faster-updates

[azaozz]

Replying to afragen:

Yes, it is there for testing. However if there is no workaround for the VB bug, and move_dir() will have to be opt-in, thinking it can be released in a canonical plugin. Adding it to core when core is not using it won't be good as Sergey points out above.

[costdev]

Only including move_dir() in a Canonical plugin would cause a problem if someone saw the upgrader_use_move_dir hook in source/DevHub and decided to use it.

If Core didn't use move_dir() by default, it would still have a toggle for whether to use it, suggesting that the function should be available. If it isn't available, an opt-in would then be an opt-in to a fatal error.

The only other alternative I see at the moment is:

<?php
if ( function_exists( 'move_dir' ) && apply_filters( 'upgrader_use_move_dir' ) ) {}

Which, while simple, doesn't feel like the right approach. We're not trying to allow a custom implementation of directory moves at this location, but rather an opt-in to a more performant Core implementation.

Curious to hear others' thoughts on this.

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

PR now using switch filter.

{{{php
if ( ! apply_filters( 'upgrader_use_move_dir', false, ... ) {

copy_dir();

} else {

move_dir();

}
}}}

[azaozz]

Replying to costdev:

if someone saw the upgrader_use_move_dir hook in...

But this filter doesn't exist in WP.

If Core didn't use move_dir() by default, it would still have a toggle for whether to use it

Not exactly. move_dir() seems unsafe to use. The problem is not in the function itself, and is not in the PHP's rename() (although it seems the VB bug can easily be corrected there). The problem is that rename() may trigger a known bug in VirtualBox. So any code that uses PHP's rename() is potentially unsafe when run in a VirtualBox.

So instead of adding an unused function to core (which is generally unacceptable), the idea is to have move_dir() in a plugin. There also won't be any need for filters, toggles, etc.

Then, if a workaround for the VirtualBox bug is found, the move_dir() function can safely be used in core. At that time a filter may also be added to bypass or disable it (if deemed necessary).

In these terms thinking this ticket can probably be closed now. A new ticket for a workaround for the VirtualBox bug seems like a better idea. Or perhaps this ticket can be repurposed?

[costdev]

To check if replacing copy_dir() with move_dir() in WP_Upgrader::install_package() produced any issues on VirtualBox, I installed the Faster Updates plugin and tested on Chassis (VirtualBox 6.1). The full results can be found ​here.

---

Highlights:

• No errors upon activation.
• No errors when navigating settings pages.
• The plugin's filter callback containing shell_exec() in case the VirtualBox bug was encountered was not needed. All worked well while it was commented out.

Notes:

• The slower speeds are always less than a second difference.
• The times are based on how fast my finger is at syncing the start/end of the updates with my phone's stopwatch.
  • For small differences, this may provide false positives/negatives, so consider a margin of error.
  • For large differences, the average plugin improvement is 32%. It's highly unlikely my finger is that slow.
  • Consider the average click/tap time of ~200ms as a margin of error.

---

This appears to confirm my findings that rename() itself is not an issue with VirtualBox, but that a sequence of rename( A, B ) -> rename( C, A ) -> unlink( B ) doesn't give VirtualBox's cache a chance to clear, and produces the bug. This sequence is not used by WP_Upgrader::install_package(), and was only part of the Rollback feature plugin. IMHO, it's not very likely that extenders would use this sequence.

[SergeyBiryukov]

Replying to costdev:

This appears to confirm my findings that rename() itself is not an issue with VirtualBox

Right, I believe that means move_dir() can be safely used in WP_Upgrader::install_package(), and both #57375 and this ticket are now unblocked. No workaround is needed for VirtualBox at this time, as the problematic sequence is never used in normal updates, only in the current implementation of rollbacks, which is out of scope for this ticket and can be further investigated in #51857.

[afragen]

@SergeyBiryukov awesome news.

I can update the PR for #57375 to add the change from copy_dir() to move_dir() if you want.

https://core.trac.wordpress.org/ticket/57386

Yep, agree this looks a bit cleaner than having "external" move_dir() function. Maybe even the move() functions in WP_Filesystem_* should be updated to handle directories? Maybe a bit tricky to handle the $overwrite param there though. It is false by default which perhaps should default to "merging" the content of directories?

Looking at the changes, wondering if wp_opcache_invalidate_directory() should also be part of WP_Filesystem_Base? (Perhaps can skip the wp_ prefix then).

Another thing: seems wp_opcache_invalidate_directory() should run after move_dir() no matter which filesystem method was used (FTP, SSH, etc.).

Maybe even the move() functions in WP_Filesystem_* should be updated to handle directories?

Coincidentally, I've just been discussing this very thing with @costdev.

Apparently move handles both files and directories in the other file systems, just not the direct file system.

@azaozz Maybe even the move() functions in WP_Filesystem_* should be updated to handle directories?

The ::move() methods for WP_Filesystem_FTPExt, WP_Filesystem_ftpsockets and WP_Filesystem_SSH2 already support moving directories. The original intent of the ::move() methods seems to be to support moving files. However, the non-direct filesystem classes happen to use commands that support both files and directories.

The additional move_dir() methods aren't necessary for the other filesystem classes and would be more code for us to maintain in the long run. IMHO, if the preference is to bring this functionality at the class level, then updating WP_Filesystem_Direct::move() to add this functionality is the most appropriate way doing this.

Maybe a bit tricky to handle the $overwrite param there though. It is false by default which perhaps should default to "merging" the content of directories?

For PHP's rename():

• Files: Can overwrite existing files.
• Directories: Will emit a warning for an existing directory.

Therefore, we can still use $overwrite with a default to false, but we need to have a condition like this after the if ( ! $overwrite... ) condition:
{{{php
if ( $this->is_dir( $source ) && $this->is_dir( $destination ) ) {

$this->delete( $destination, true );

}
}}}

Looking at the changes, wondering if wp_opcache_invalidate_directory() should also be part of WP_Filesystem_Base? (Perhaps can skip the wp_ prefix then).

With wp_opcache_invalidate() being a general function, I'm not sure it's worth making the directory equivalent a class method. That said, there's no reason why we couldn't have both, with the general functions being wrappers for the class-level, but that seems like another ticket down the road.

Another thing: seems wp_opcache_invalidate_directory() should run after move_dir() no matter which filesystem method was used (FTP, SSH, etc.)?

Indeed, and the same goes for wp_opcache_invalidate() when used on single files. #3798 runs this ​here no matter the filesystem method.

That said, are we sure there's no use for a general move_dir() function? We have a copy_dir() function available to extenders, and a move_dir() function may be useful too. If there's a desire to have move_dir(), should we be making a decision that affects the filesystem classes at this stage, or is it an opportunity a future enhancement where we could make both copy_dir() and move_dir() wrappers for filesystem class methods?

Maybe even the move() functions in WP_Filesystem_* should be updated to handle directories?

Coincidentally, I've just been discussing this very thing with @costdev.

Apparently move handles both files and directories in the other file systems, just not the direct file system.

Some information on this that I posted in the Upgrade/Install channel yesterday:

• WP_Filesystem_FTPExt::move() - ​ftp_rename - "Renames a file or a directory on the FTP server"
• WP_Filesystem_ftpsockets::move() - ​$this->ftp->rename - Uses RNFR/RNTO which work on files and directories.
• WP_Filesystem_SSH2::move() - ​ssh2_sftp_rename - Ultimately calls ​sftp_rename (C).

While the last one states "Rename a file on the remote server", search for "Removing and Renaming Files" on ​this link, which states:

Files (and directories) can be renamed using the SSH_FXP_RENAME message.

Which, as you can ​see here, is what libssh2's sftp_rename() function uses.

So non-direct filesystems already have directories covered. It was the direct filesystem that lacked this specific behaviour, via rename(). WP_Filesystem_Direct::move() isn't suitable as it falls back to ::copy(), which only works for files, not directories. Hence, copy_dir() and now move_dir().

I'm closing this PR as I don't think the file system classes need the new method.

As the non-direct classes already support moving directories in their ::move() methods, I think the best approach is to make it consistent.

Was just thinking about that too. Seems best to fix ::move() where needed and make sure it's consistent. Then have a move_dir() wrapper similarly to copy_dir().

Related ​https://github.com/WordPress/wordpress-develop/pull/3909.

Closing in favor of ​PR3911

[afragen]

Closing in favor of #57557