[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 dev-feedback  |     Focuses:  performance
------------------------------------+--------------------------
Changes (by costdev):

 * keywords:  has-patch => has-patch dev-feedback


Comment:

 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
 [https://github.com/afragen/faster-updates/tree/generic-hook 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?

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