Add move_dir() function

[afragen]

WordPress lacks an internal function to move a directory from one location to another.

WordPress has a $wp_filesystem->move() but WP_Filesystem_Direct it is designed with only a single file copy fallback. copy_dir() is a recursive file copy from one location to another. copy_dir() is effective but it is more resource intensive and does require the user to delete the source after copying.

move_dir() is a battle tested function that has been part of testing for a feature plugin having over 8000+ active installs.

The idea is to use rename() with copy_dir() as a fallback. Non-direct WP_Filesytem move() commands are a variation of rename() all of which report a boolean result with no fallback. This means that a failure will be caught and the fallback will be used.

WordPress lacks an internal function to move a directory from one location to another.

WordPress has a $wp_filesystem->move() but for WP_Filesystem_Direct it is designed with only a single file copy fallback. copy_dir() is a recursive file copy from one location to another. copy_dir() is effective but it is more resource intensive and does require the user to delete the source after copying.

move_dir() is a battle tested function that has been part of testing for a feature plugin having over 8000+ active installs.

The idea is to use rename() with copy_dir() as a fallback. Non-direct WP_Filessytem move() commands are a variation of rename() all of which report a boolean result with no fallback. This means that a failure will be caught and the fallback will be used.

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

[costdev]

Overall, a +1 from me on implementing a function for the performant moving of directories.

---

Some additional information:

WP_Filesystem_* classes

WP_Filesystem_Direct::move() and the other filesystem ::move() methods are documented to move a file, not to move a directory. For non-direct filesystems, the moving of directories is a happy extra due to the behaviour of FTP/SSH functions.

WP_Filesystem_Direct::move()

While WP_Filesystem_Direct::move() uses rename(), which works for files and directories, if rename() should fail, it falls back to a copy(), a single-file function.

How should a move_dir() function differ?

It makes sense to introduce a move_dir() function that:

• Leverages the directory support of FTP/SSH functions in the ::move() method for non-direct filesystems
• Uses rename() with a fallback to copy_dir() for the direct filesystem.

Performance

In terms of memory usage and speed, a recursive copy involves the contents of the files/directories, including calls to mkdir(), and a subsequent unlink().

rename(), on the other hand, simply updates the metadata. This makes rename() significantly more performant. It is purpose-built for moving files and directories.

Upon failure, falling back to copy_dir() is the appropriate behaviour.

PHP's rename() under the hood

As mentioned above, rename() updates the metadata. However, when PHP's rename( $from, $to ) is called with $from and $to on different drives, it uses a recursive copy. For anyone who has ever dragged and dropped a file/directory from one drive to another, you'll likely have noticed that this makes a copy on the second drive. This isn't a negative, but just a bit of extra information on how rename() handles this scenario for those interested.

Couple things:

1. Perhaps the do_action( 'pre_move_dir' ); and do_action( 'post_move_dir' ); actions would be more useful if the $from and $to are added for more context.
2. Should moving invalidate any caches?

We can certainly pass $to and $from in the pre_move_dir and post_move_dir hooks.

As a replacement for copy_dir in WP_Upgrader::install_package I don't believe there's any cache invalidation needed. There is a specific wp_opcache_invalidate( $to . $filename ) called in copy_dir after a file copy, but since this is a directory copy I don't believe anything is needed.

In testing this function for almost a year I haven't seen any issues.

There is a specific wp_opcache_invalidate( $to . $filename ) called in copy_dir after a file copy, but since this is a directory copy I don't believe anything is needed.

In testing this function for almost a year I haven't seen any issues.

I hope you're right :) This sounds a little bit like the filesystem cache bug in VB. Would probably be quite hard to trigger/detect. Also, if I'm reading it right, seems the OPcache for all files that were moved with copy_dir() is invalidated? May be good to try to test this specifically, just in case.

I think @costdev answered this well in https://core.trac.wordpress.org/ticket/57386#comment:23

I think @costdev answered this well in...

Yea, https://core.trac.wordpress.org/ticket/57386#comment:23 points out that wp_opcache_invalidate() doesn't resolve the VB issue. Also the cache cleaning will have to be called for each file in the moved directory (and all sub-directories). This will slow down move_dir() quite a bit. However not invalidating the OPcache may bring bugs especially on large/busy sites.

From the PHP manual:

OPcache improves PHP performance by storing precompiled script bytecode in shared memory, thereby removing the need for PHP to load and parse scripts on each request.

So if the cache is not invalidated, PHP might end up running the old, precompiled code instead of the code form the newly moved file.

In that terms thinking it would be best to specifically test this case, and perhaps run wp_opcache_invalidate() for each moved file when OPcache is in use.

See https://core.trac.wordpress.org/ticket/36455 and https://core.trac.wordpress.org/ticket/50354 re: above comment.

So how does OPcache function when the whole directory is replaced and not file by file?

I have to say I've run this code for well over a year on local and server environments and I can't think of a single time the new code wasn't run.

There doesn't seem to be any opcache invalidation for uses of $wp_filesystem->move() where the primary method is rename with fallback to copy.

how does OPcache function when the whole directory is replaced and not file by file?

Hmmm, not sure. Looking at https://core.trac.wordpress.org/ticket/36455 seems it still needs invalidating of OPcache file by file?

Perhaps asking @getsource (@mikeschroder) for some help here would be best :)

@azaozz In that terms thinking it would be best to specifically test this case, and perhaps run wp_opcache_invalidate() for each moved file when OPcache is in use.

No reason to test whether the bug can occur IMO: You're absolutely right. PHP will _definitely_ execute outdated code without invalidating the cache when OPcache is in use, which could result in any number of issues.

Even though the speed of move_dir() will be negatively affected by a loop through $wp_filesystem->dirlist() results, the speed will still likely be higher and memory usage still significantly lower than a recursive copy + invalidation + delete. That's for profiling to verify though :slightly_smiling_face:

@azaozz the PR has been updated with a new function wp_opcache_invalidate_directory(). It actually is pretty performant. I kept it under this PR as move_dir() requires it.

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

PHP will definitely execute outdated code without invalidating the cache when OPcache is in use

Thanks for confirming.

the PR has been updated with a new function wp_opcache_invalidate_directory().

Great. Looking at it, perhaps it can be simpler? As far as I see $wp_filesystem->dirlist() returns a nested (multidimensional) array of all files and folders. Can probably use array_walk_recursive() to get to all files. Perhaps something like:

function wp_opcache_invalidate_directory( $path ) {
	global $wp_filesystem;

	$path = trailingslashit( $path );
	$files = $wp_filesystem->dirlist( $path, false, true );

	if ( ! is_array( $files ) ) {
		return;
	}

	array_walk_recursive( $files, function ( $filename ) {
		wp_opcache_invalidate( $path . $filename );
	}
}

(The above is just an example. Untested!)

PHP will definitely execute outdated code without invalidating the cache when OPcache is in use

Thanks for confirming.

the PR has been updated with a new function wp_opcache_invalidate_directory().

Great. Looking at it, perhaps it can be simpler? As far as I see $wp_filesystem->dirlist() returns a nested (multidimensional) array of all files and folders. Can probably use array_walk_recursive() to get to all files. Perhaps something like:

function wp_opcache_invalidate_directory( $path ) {
	global $wp_filesystem;

	$path = trailingslashit( $path );
	$files = $wp_filesystem->dirlist( $path, false, true );

	if ( ! is_array( $files ) ) {
		return;
	}

	array_walk_recursive( $files, function ( $filename ) {
		wp_opcache_invalidate( $path . $filename );
	} );
}

(The above is just an example. Untested! array_walk_recursive() runs the callback only on "leaf" nodes, not on nested array nodes.)

Also wondering if using scandir() to get all files may be faster than $wp_filesystem->dirlist(), as the latter seems to be doing more things. Then array_walk_recursive() can be replaced by a recursive scandir(). Not sure if it's worth the effort for now.

@azaozz I did timing tests on wp_opcache_invalidate_directory() it only takes 0.25 seconds to run on Gutenberg.

I'll get the above and report back.

I already hit a snag witharray_walk_recursive, in a plugin with no subfolders it loops over every item in the dirtiest() array, not just the filename. In testing with Gutenberg update it just doesn't do the nested folder recursion.

​Here's array_walk_recursive(), map_deep(), and a modified map_deep_with_index() with a real WP_Filesystem_Direct::dirlist() result. In the callbacks, $key is name, type, and so on, rather than the parent directory. It's there for completeness, but isn't useful.

Unfortunately, the path is incomplete in all three cases.

*Note:* list_files() would have been great here, as it returns absolute paths. Unfortunately, it's not FTP-safe.

wp_opcache_invalidate_directory simplified for only one level of recursion. Good eye Colin.

I already hit a snag with array_walk_recursive...

In the callbacks, $key is name, type, and so on, rather than the parent directory.

I see. Yes, the format of the array returned by dirlist() is not particularly suitable for use with array_walk_recursive. That array can probably be "reduced" to only useful values, but that's another recursive loop. There seem to be better ways.

wp_opcache_invalidate_directory simplified for only one...

Yes, it looks better now. However the params still look pretty confusing:

• $dir is (must be?) a string when calling the function. It's the path to the directory containing the PHP files for which the OPcache has to be invalidated. However it can also be an associative array whose format matches a sub-array returned by WP_Filesystem_Direct::dirlist(). If that's the case the second param is required/must be set too.
• $path is only used in recursion and must be the (partial) path to the directory listing array passed for the first param. It must be set/is not optional when the first param is an array. Is ignored when the first param is a string.

Thinking that wp_opcache_invalidate_directory() would be much better if it only accepts one param. The second, $path can probably be passed in the sub-array, no need of all the confusion about it. That would still mean that $dir can be either a string or an array, but we can document the use case and explain that it requires a string in order to work properly.

Perhaps the code can be something like:

function wp_opcache_invalidate_directory( $dir ) {
	global $wp_filesystem;

	if ( is_string( $dir ) ) {
		$path = $dir;
		$dir  = $wp_filesystem->dirlist( $dir, false, true );
	} elseif ( is_array( $dir ) && array_key_exists( $dir[ 'directory_path' ] ) ) {
		$path = $dir[ 'directory_path' ];
	} else {
		return;
	}

	foreach ( $dir as $name => $details ) {
		if ( ! empty( $details[ 'files' ] ) && is_array( $details[ 'files' ] ) ) {
			$details[ 'files' ][ 'directory_path' ] = trailingslashit( $path ) . $name;
			wp_opcache_invalidate_directory( $details['files'] );
			continue;
		}
		wp_opcache_invalidate( trailingslashit( $path ) . $name );
	}
}

As $path was only added for the purposes of recursion, if it's to be removed, there is always the option of a closure. This would simplify the calling of wp_opcache_invalidate_directory() by only accepting a single parameter: string $dir, and would remove any chance of user error.

A fuller implementation would look something like this:

{{{php
function wp_opcache_invalidate_directory( $dir ) {

global $wp_filesystem;

if ( ! is_string( $dir )

=== trim( $dir ) ) {

_doing_it_wrong(

FUNCTION,
sprintf(

/* translators: %s: The '$dir' argument. */
( 'The %s argument must be a non-empty string.' ),
'$dir'

),
'6.2.0'

);
return;

}

$dirlist = $wp_filesystem->dirlist( $dir, false, true );

if ( empty( $dirlist ) ) {

return;

}

$invalidate_directory = function( $dirlist, $path = ) use ( &$invalidate_directory ) {

foreach ( $dirlist as $name => $details ) {

if ( ! empty( $detailsfiles? ) ) {

$invalidate_directory(

$detailsfiles?,
trailingslashit( $path ) . trailingslashit( $name )

);
continue;

}

$invalidate_directory( trailingslashit( $path ) . $name );

}

}

$invalidate_directory( $dirlist );

}
}}}
@azaozz What do you think of this approach?

As $path was only added for the purposes of recursion, if it's to be removed, there is always the option of a closure. This would simplify the calling of wp_opcache_invalidate_directory() by only accepting a single parameter: string $dir, and would remove any chance of user error.

I like this suggestion, seems more straightforward 👍

I recalled a discussion in #54028 that closures should be used sparingly in core as they are non-trivial to unhook, but in cases like this, where it's not meant to be unhookable and is used to simplify the approach, I think it's fine.

@azaozz @costdev an updated version of the closure version is here, ​https://github.com/afragen/faster-updates/blob/develop/functions/move.php#L100-L139

closures should be used sparingly in core as they are non-trivial to unhook

Right, that applies only to using lambda (anonymous) functions as callbacks for hooks as they cannot be "unhooked". There are no technical problems with using closures/lambda/anonymous functions in all other cases, however there are some drawbacks:

• In most cases they are impossible to test.
• Usually they lack sufficient documentation as proper docblocks are not used for them.

Generally lambda functions and closures are preferable when they are really straightforward/simple/self-documenting. That eliminates the above drawbacks.

What do you think of this approach?

an updated version of the closure version is here

Yea, the first thing I tried was a closure too. It makes the code a bit harder to read/understand so I opted for a "standard" recursive function, but a closure can also be used there (would need docs, see the drawbacks above).

However _doing_it_wrong() should not be used here imho. It is a function that is intended to "tell off" a developer when they are doing something wrong. It is not intended as a way to catch bugs. The meaning of it is more like:

"Stop! What you're doing is wrong! Fix your code!"

I know, _doing_it_wrong() has been used improperly more and more recently. Perhaps WP needs a similar function but with "softer" meaning/message.

For wp_opcache_invalidate_directory() if you insist on throwing a PHP notice, perhaps a standard trigger_error() would be enough?

[azaozz]

Just to make it clearer as this was mostly discussed on #57386: currently committing of this is blocked because the new functions are not used in core. See https://core.trac.wordpress.org/ticket/57386#comment:28 and the following comments there.

@azaozz I made several updates trying to resolve most (hopefully all) of your recent concerns.

[costdev]

In Chassis, I ran a microtime() test between copy_dir() and move_dir() on plugins/woocommerce. Both copy_dir() and move_dir() included OPcache invalidation.

Test 1:

copy_dir(): 18.564819812775
move_dir(): 2.5664520263672 (86% faster)

Test 2:

copy_dir(): 17.737833023071
move_dir(): 2.5115830898285 (85% faster)

Test 3:

copy_dir(): 17.455080986023
move_dir(): 2.4744238853455 (85% faster)

I also tried swapping the order so that move_dir() came first, in case the first one would always run a bit slower. The results were the same.

[afragen]

Replying to azaozz:

Just to make it clearer as this was mostly discussed on #57386: currently committing of this is blocked because the new functions are not used in core. See https://core.trac.wordpress.org/ticket/57386#comment:28 and the following comments there.

Per @SergeyBiryukov this should no longer be blocked. https://core.trac.wordpress.org/ticket/57386#comment:39

@azaozz I've updated the docblock as well.

I've updated the docblock

Docs look great now :)

Just wondering if the VirtualBox bug should be perhaps mentioned there? Maybe just few words and a link to trac? Perhaps https://core.trac.wordpress.org/ticket/57386#comment:22 ?

Current updates are passing all testing, working as expected, and resulting in no errors. 💥

Thinking this looks better and better :)

A bit concerned about usleep( 200000 );. Afaik adding delays (even only 200ms) can be pretty bad for big/busy sites. On the other hand the delay here is only after moving a directory which happens very rarely, so probably wouldn't affect anything much (filesystem operations can be "slow" in PHP terms).

As far as I understand that delay is a pretty common "fix" for some fs shortcomings in VirtualBox, and is used in other popular software like ​Composer. Perhaps adding that to the inline comment/docs would be good.

Looking at do_action( 'pre_move_dir', $from, $to ); and do_action( 'post_move_dir', $from, $to, $result ); wondering if both are needed and what are the use cases for them. I agree, it's good to give plugins access here, just need to try to imagine the use cases.

Perhaps pre_move_dir can be changed to an "override filter" that would let plugins use the server's native commands with exec() or system(). Or maybe make a custom plugin for specific servers that is different from core. Or even replace move_dir() with copy_dir() in some cases.

Such "override filter" would usually look something like:

$override = apply_filters( 'pre_move_dir', false, $from, $to );
if ( false !== $override ) {
    return $override;
}

(This would let plugins use their own logic, return false to fall back to core's logic or return WP_Error on failure.)

Not sure if there's a good use case for post_move_dir. Perhaps can be removed?

[peterwilsoncc]

@afragen @azaozz I've created a pull request with an alternative approach but messed up linking it to this ticket.

Instead of detecting the filesystem in the global move_dir() function, it calls the new method $filesystem->move_dir() and deals with the required steps in each class. I think this is a little neater.

It doesn't take in to account any of the feedback in the review above, pushed through by the GH bot, as it was written in parallel to that comment. Excellent timing.

The 200ms delay does seem a little ugly to me but in my testing yesterday, even with that delay running updates was significantly faster.

The PR is in draft as it needs unit tests, treat it as a POC for now.

[peterwilsoncc]

Ugh, messed up linking to the PR in the comment above sorry: see ​https://github.com/WordPress/wordpress-develop/pull/3897

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

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

Not sure which is best.

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

@azaozz The hooks were originally for a system-level VirtualBox solution that was a no-go, so I think we're now in that situation where we're trying to find use cases for the hooks, rather than find hooks for known use cases. We might be better removing these for now, and considering them for addition when we have some use cases.

I've discussed this with @costdev and he shares this opinion identically.

[peterwilsoncc]

As discussed on ​PR#3897, the direct file system is the only class in which the ::move() method does not handle both files and directories.

I'm of the view that this is a bug and it's probably best to modify the direct filesystems method to support both files and directories if possible.

Whether or not a global move_dir() function is created as an alias is probably worth discussing, as it would be consistent with the copy_fir() function.

I see this was discussed in passing during the meeting a few hours ago, so it would be lovely if a few people could share their thoughts on this ticket.

@azaozz yeah somehow our nice little thread got mucked up. Here's what I currently have based on your suggestions, I think.

{{{php if ( ! is_string( $dir )

=== trim( $dir ) ) {

if ( ! is_string( $dir )

=== trim( $dir ) ) {

if ( defined( 'WP_DEBUG' ) && WP_DEBUG ) {

$error_message = sprintf(
/* translators: %s: The function. */

( 'The argument for %s must be a non-empty string.' ),
'<code>wp_opcache_invalidate_directory()</code>'

);
phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_trigger_error
trigger_error( $error_message );

}
return;

}

}}}

Sorry for all the disjointed comments. PR updated to use the following. I'm going to resolve other comment threads.

{{{php if ( ! is_string( $dir )

=== trim( $dir ) ) {

if ( WP_DEBUG ) {

$error_message = sprintf(

/* translators: %s: The function name. */
( '%s expects a non-empty string.' ),
'<code>wp_opcache_invalidate_directory()</code>'

);
phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_trigger_error
trigger_error( $error_message );

}
return;

}

}}}

[costdev]

@peterwilsoncc I see this was discussed in passing during the meeting a few hours ago, so it would be lovely if a few people could share their thoughts on this ticket.

Absolutely. I planned to post here this evening after the meeting.

I'm of the view that this is a bug

The methods were originally written and documented to support moving a file. It just so happens that the functions to move a file in the FTPExt, ftpsockets and SSH2 classes, also support moving a directory. For that reason, all of the methods currently meet their documented behaviour, and WP_Filesystem_Direct::move() isn't bugged.

it's probably best to modify the direct filesystems method to support both files and directories if possible.

I agree - the enhancement is worth making to WP_Filesystem_Direct::move().

Whether or not a global move_dir() function is created as an alias is probably worth discussing, as it would be consistent with the copy_dir() function.

I think a global move_dir() function is important here, for the following reasons:

copy_dir() exists
Extenders can copy a directory by calling copy_dir( $from, $to, $skip_list ). If we didn't have a global move_dir() function, it would be $wp_filesystem->move( $from, $to, $overwrite ). A global move_dir() function maintains consistency and ease of use for extenders.

Error reporting
The ::move() methods of the filesystem classes return bool. ​copy_dir returns true|WP_Error. This provides the extender with the reasons why there was a failure. We can't change the return type of the ::move() methods, but we can at least continue the pattern of providing a global function that returns WP_Error upon failure.

Time
Architecturally, I fully support enhancing WP_Filesystem_Direct::move() to support moving a directory. However, this needs careful testing to ensure there are no BC breaks. This almost guarantees that the benefits of this ticket won't be seen until 6.3 is released. The functionality in move_dir() has been tested (with VirtualBox 7 testing coming soon).

If we look at the general move_dir() function with the view that this will later become a wrapper function for the ::move() methods, then we can ensure we implement it in a way that makes it easy to convert it to a wrapper function, once the changes to WP_Filesystem_Direct::move() have been implemented and tested during the 6.3 cycle.

If we add move_dir() now, we can make the change to WP_Upgrader::install_package() and deliver faster, more stable updates to users in 6.2. This means we can reduce timeouts and diskspace/memory issues for users now, then follow up in 6.3 to improve the architecture while ensuring the filesystem API isn't broken in the process.

$overwrite parameter added to move_dir() to simplify refactor of WP_Filesystem_Direct::move()

Thanks to @costdev we now have unit tests for both functions. 🎉

[afragen]

We have updated PR3798 with passing unit tests and refactored to enable a more fluid transition to update WP_Filesystem_Direct::move(), as discussed in PR3897.

It makes more sense to move forward with PR3798 in 6.2 and have a tested update for the Filesystem transition in 6.3. I say this as PR3798 has had much more testing and PR3897 has had none.

Both @costdev and @bronsonquick will be testing on VirtualBox 7 in the coming days. If we encounter some issues, it would be nice to get this blessed ahead of the Beta 1 deadline so that after that testing it can be committed, unless someone thinks it's able to be committed prior to this last testing.

PR3798 would benefit all users and by itself has the potential to mitigate many/all timeout related plugin/theme update issues.

[costdev]

Virtual Box 7.0.6 - Chassis Test Results

I tested with the Faster Updates plugin (i.e. ​PR 3798) by itself, as well as with Rollback Update Failure, as this has more complex use of move_dir() and is a helpful tool for more intensive testing.

• Oracle did not fix the VirtualBox bug in VirtualBox 7. 🐞
• VirtualBox 7 + Faster Updates works ✅
• VirtualBox 7 + Faster Updates + Rollback works ✅
• VirtualBox 7 + Faster Updates (wp_opcache_invalidate_directory() disabled, therefore no filemtime() or filesize() called) reproduces the VirtualBox bug 🐞. Expected behaviour ✅
• VirtualBox 7 + Faster Updates + Rollback + Mixed simulated failures works ✅
• VirtualBox 7 + Faster Updates + Rollback + All simulated failures works ✅
• Tested with 7 plugins (Single and Bulk updates - three each):
  • Akismet
  • Hello Dolly
  • Jetpack
  • MailPoet
  • WooCommerce
  • Yoast SEO
  • WPForms Lite
• Tested with 7 themes (Single and Bulk updates - three each):
  • Twenty Ten
  • Twenty Twelve
  • Twenty-Twenty One
  • Twenty-Twenty Two
  • Twenty-Twenty Three
  • Block Aarambha
  • OceanWP

Timing Test
I ran bulk upgrades of the 7 plugins listed above. Here are the times:

• WordPress 6.1.1: 2m 15s
• Faster Updates: 1m 27s (35% faster)

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.

This PR adds directory support to WP_Filesystem_Direct::move() for consistency with the other WP_Filesystem_* classes.

This includes:

• Updates to WP_Filesystem_Direct::move().
• A new global function: wp_opcache_invalidate_directory(), to recursively call wp_opcache_invalidate().
• A new wrapper function: move_dir(), for extender convenience and more informative failure reasons.
• Tests for move_dir().
• Tests for wp_opcache_invalidate_directory().

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

[ironprogrammer]

Test Report

This testing validates recent PR/test plugin updates that resolve PHP warnings logged in VirtualBox 6.

Patch tested: ​PR 3798 via the ​Faster Updates plugin zip

Environment

• OS: macOS 12.6.2 (Intel)
• Virtual Server: Chassis 5.1.0 (Vagrant 2.3.4 + VirtualBox 6.1.42)
• Web Server: nginx/1.18.0
• PHP: 7.4.33
• WordPress: 6.1.1
• Browser: Safari 16.2
• Active Plugins:
  • Faster Updates (since ​commit 5a0a12f)
  • Rollback Update Failure v4.1.2 (tested both active/inactive)

Actual Results

• ✅ Plugins updated successfully:
  • Akismet Anti-Spam
  • Hello Dolly
  • Jetpack
  • MailPoet
  • WooCommerce
  • Yoast SEO
• ✅ No PHP errors/warnings/notices were logged during updates.
• ✅ Plugin file/directory counts matched those of direct installation via WP-CLI.
• ✅ The content/temp-backup/plugins/ directory was clean after updates completed (Chassis uses content/ instead of wp-content/).
• ✅ After update, plugins could be activated successfully with no errors logged.

Supplemental Artifacts

• Please refer to ​this gist for the scripts used to force old versions and count plugin files/directories, as well as file count results.

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.

While working extensively on #57375 in various discussions on that ticket and on ​Slack it was decided to move forward with ​PR3909.

PR3909 does not include the needed change to WP_Upgrader::install_package() to take advantage of its new code. This ticket and PR will accomplish that. The plan is to replace copy_dir() in WP_Upgrader::install_package() with move_dir().

This change was tested extensively during the development of #57375

Related: #57375, #57386

Trac ticket:

This PR will fail until ​PR3903 has been committed.

[costdev]

@peterwilsoncc raised a good follow-up for this on the PR to ensure docs are consistent for the other filesystem ::move() methods.

Posting it here so it isn't lost on the PR:

Once this is merge ready, let's add the same docs changes to the other classes.
Parameters will need to be updated to change file to "file or directory."

[bronsonquick]

I've just completed some testing using the Faster Updates plugin @ version 0.5.5 and doing these all via Dashboard -> Updates.

My current setup is as follows:

• Chassis Version 5.1.0
• VirtualBox Version 7.0.6 r155176
• MacOS Ventura 13.1 (22C65)

I did three tests both with and without Faster Updates and all of them were for plugins as I don't know many large themes off the top of my head!

My results are as follows:

• WP Forms Lite - From 1.6.8.1 to 1.7.9.1
  • With Faster Updates
    1. 14:52 seconds
    2. 14:05 seconds
    3. 14:04 seconds
  • Without Faster Updates
    1. 17:61 seconds
    2. 17:43 seconds
    3. 17:18 seconds
• Jetpack - From 10.2 to 11.7.1
  • With Faster Updates
    1. 23:19 seconds
    2. 25:77 seconds
    3. 23:56 seconds
  • Without Faster Updates
    1. 32:25 seconds
    2. 34:41 seconds
    3. 34:88 seconds
• Woocommerce - From 5.2.1 to 7.3.0
  • With Faster Updates
    1. 24:32 seconds
    2. 25:51 seconds
    3. 28:21 seconds
  • Without Faster Updates
    1. 39:28 seconds
    2. 39:23 seconds
    3. 38:02 seconds

I then did a final test three times which involved bulk updating all three of those plugins. i.e:

• WP Forms Lite - From 1.6.8.1 to 1.7.9.1
• Jetpack - From 10.2 to 11.7.1
• Woocommerce - From 5.2.1 to 7.3.0
  • With Faster Updates
    1. 1:01:72 seconds
    2. 1:01:56 seconds
    3. 1:07:19 seconds
  • Without Faster Updates
    1. 1:44:81 seconds
    2. 1:30:77 seconds
    3. 1:28:19 seconds

I can also confirm the following:

• No PHP errors/warnings/notices were logged during the updates.
• All temp directories created during the updates were cleaned up afterwards.
• After updating, all the plugins could be activated successfully with no errors logged.

Thanks for all your hard work on this folks. It's impressive seeing the results!

For clarity: The latest push was just to rebase on trunk and make some minor updates to test documentation 🙂

[peterwilsoncc]

I've just added what I hope will be a final review on the ​linked PR #3909 updating the filesystem classes for consistency.

I think this is in great shape for inclusion in core.

Discussing this with @azaozz, for 6.2 we'd like to limit the use of move_dir() to plugins and themes only. The Core updater ought to retain its current behavior.

This logic for this is simple: if the theme or plugin updater gets broken then it can be fixed by pushing out a minor update of WordPress. If the core updater gets broken then that's not so much of an option.

PR#3909 doesn't include implementation but I think @afragen has a pr for that around somewhere.

Manually testing PR#3909 with the following diff applied worked as expected on Chassis per my notes from the other week.

src/wp-admin/includes/class-wp-upgrader.php

@@ -594 +594 @@
                 }
 
                 // Copy new version of item into place.
-                $result = copy_dir( $source, $remote_destination );
+                $result = move_dir( $source, $remote_destination, true );
 
                 // Clear the working folder?
                 if ( $args['clear_working'] ) {

[azaozz]

Replying to peterwilsoncc:

for 6.2 we'd like to limit the use of move_dir() to plugins and themes only. The Core updater ought to retain its current behavior.

This logic for this is simple: if the theme or plugin updater gets broken then it can be fixed by pushing out a minor update of WordPress. If the core updater gets broken then that's not so much of an option.

Yes, thinking this is the best way to confirm everything works as expected on all servers and installations.

[afragen]

FWIW I never considered using move_dir() for core at this time. It hasn't been tested for this.

I would have tried to test it but the use of copy_dir() for core updates makes extensive use of $skip_list and I don't think it's feasible to do this yet. I do have a thought about it and it would entail using the $skip_list inside move_dir() to unset() after they had been moved.

At some point I'll open a ticket for this with a PR. 😉

Also, the other ​PR3911 is in #57557

Tests are failing and will fail as move_dir() is not defined in core yet and it's not defined in this PR.

[azaozz]

In 55204:

Filesystem API: Add directory support to WP_Filesystem_Direct::move().

Introduces:

• New function: wp_opcache_invalidate_directory(), to recursively call wp_opcache_invalidate() after overwriting .php files.
• New function: move_dir(), similar to copy_dir() that uses WP_Filesystem::move() followed by wp_opcache_invalidate_directory(), and has a fallback to copy_dir().

Props: costdev, afragen, peterwilsoncc, sergeybiryukov, ironprogrammer, flixos90, bronsonquick, mukesh27, azaozz.
Fixes #57375.

Committed: https://core.trac.wordpress.org/changeset/55204

Update move_dir() to better accommodate non-direct ::move().

This will attempt to delete the destination, required for rename(), before calling ::move(). If unable it will fail.

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

[azaozz]

In 55219:

Filesystem API: Update move_dir() to better handle the differences in the WP_Filesystem::move() methods.

Changes move_dir() to attempt to delete the destination when overwriting, before calling WP_Filesystem::move().

Props: afragen, costdev, azaozz.
Fixes: #57375.

[SergeyBiryukov]

Thanks for the commits!

Looking at [55219], should $wp_filesystem->exists( $to ) be saved to a variable to avoid the second call?

See 57375.diff​ (also uses consistent formatting for WP_Error while we're at it).

[afragen]

All seems reasonable @SergeyBiryukov

[azaozz]

Replying to SergeyBiryukov:

Looking at [55219], should $wp_filesystem->exists( $to ) be saved to a variable...

Yep, makes sense. The two conditionals can also be wrapped in the $wp_filesystem->exists( $to ). Something like:

if ( $wp_filesystem->exists( $to ) ) {
    if ( ! $overwrite ) {
        return new WP_Error( ....
    } elseif ( ! $wp_filesystem->delete( $to, true ) ) {
        return new WP_Error( ....
    }
}

[SergeyBiryukov]

In 55223:

Filesystem API: Simplify two conditionals in move_dir().

This updates the check whether the destination directory already exists to only call $wp_filesystem->exists() once.

Follow-up to [55204], [55219], [55220].

Props azaozz, afragen, SergeyBiryukov.
Fixes #57375.

[pbiron]

Test Report

This testing validates the PR commits for #57375 & #57557 are working as expected.

Environment

OS: Windows 11 Pro for Workstations
Web Server: apache 2.4.53
PHP: 7.4.29
WordPress: 6.1.1 & 6.2-alpha-55222 (includes the latest PR commits for #57375 & #57557)
Browser: Chrome 109.0.5414.120
Active Plugins: Gravity Forms

Actual Results

✅ Plugins updated successfully:

Akismet Anti-Spam
Gravity Forms
Hello Dolly
Jetpack
MailPoet
WooCommerce
WP Forms Lite
Yoast SEO

✅ No PHP errors/warnings/notices were logged during updates.
✅ Plugin file/directory counts matched those of direct installation via WP-CLI.
✅ After update, plugins could be activated successfully with no errors logged.

Timing Test

I ran bulk upgrades of the 8 plugins listed above. Here are the times (average of 5 trials each):

6.1.1: 1m 27s
6.2-alpha-55222: 1m 5s (25% faster)

Note: these times do NOT include the time to download the update packages (I'm on a slow network connection, so that download time would be out-of-whack compared to "normal" users)

Additional Info

I also did a number of tests from wp-admin/plugins.php (tho, didn't gather timing info on those tests). ✅ Plugins updated successfully.

I also did a number of tests using the FTPext and ftpsockets filesytems (with FileZilla Server Windows 1.6.6). However, because of problems with the FTP server setup I was getting intermittent errors while unzipping the larger packages (mostly Jetpack and Woo). I'm confident that the changes in 6.2-alpha-55222 are working correctly with those filesystems, but can't provide reliable timing info because of the intermittent errors.

Merged.

Merged.

[peterwilsoncc]

In 55226:

Filesystem API: Prevent fatal error in move_dir().

Correctly instantiate WP_Error() within move_dir() to prevent a fatal error when unable to delete an existing directory that is intended to be replaced.

Follow-up to [55204], [55219], [55220], [55223].

Props swissspidy, costdev, afragen.
Fixes #57375.

Adds a test for the WP_Error object, destination_not_deleted_move_dir, in move_dir().

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

[peterwilsoncc]

Reopening this while a test is wrapped up in the ​linked pull request.

It looks like all paths are covered in ​the existing tests except for the one that caused the fatal error fixed in [55226]. A logic check on this would be most helpful.

[peterwilsoncc]

In 55244:

Filesystem API: Add test for uncovered WP_Error in move_dir().

Introduces a test for the WP_Error object destination_not_deleted_move_dir in the move_dir() function.

Follow up to [55226].

Props costdev, mukesh27.
Fixes #57375.

Merged in https://core.trac.wordpress.org/changeset/55244 / ​https://github.com/WordPress/wordpress-develop/commit/9d8e1ae0689ac5bb1b78d67a90364814b04abc81

Thank you!

[milana_cap]

@afragen @SergeyBiryukov do we have a volunteer to write a Dev note?

[afragen]

@costdev is writing a dev note.