[wp-trac] [WordPress Trac] #57386: Add filter to WP_Upgrader::install_package for copy_dir()

[WordPress Trac]

#57386: Add filter to WP_Upgrader::install_package for copy_dir()
-----------------------------+--------------------------
 Reporter:  afragen          |       Owner:  (none)
     Type:  feature request  |      Status:  reopened
 Priority:  normal           |   Milestone:  6.2
Component:  Upgrade/Install  |     Version:  trunk
 Severity:  normal           |  Resolution:
 Keywords:  has-patch        |     Focuses:  performance
-----------------------------+--------------------------

Comment (by afragen):

 Replying to [comment:16 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, [https://community.oracle.com/tech/apps-
 infra/discussion/comment/14522583/#Comment_14522583  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

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