[wp-trac] [WordPress Trac] #57375: Add move_dir() function

[WordPress Trac]

#57375: Add move_dir() function
----------------------------+--------------------------
 Reporter:  afragen         |       Owner:  (none)
     Type:  enhancement     |      Status:  new
 Priority:  normal          |   Milestone:  6.2
Component:  Filesystem API  |     Version:  trunk
 Severity:  normal          |  Resolution:
 Keywords:  has-patch       |     Focuses:  performance
----------------------------+--------------------------

Comment (by 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`.
 [https://developer.wordpress.org/reference/functions/copy_dir/ 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.

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/57375#comment:50>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform