Add back rollback PRs

[afragen]

Adds back PRs for rollback that were reverted from 5.9 release.

Updates to fix issue with VirtualBox and updates move_dir() to return better values.

Invaluable help from @peterwilsoncc, @costdev, @BronsonQuick, @aristath and many others

Trac tickets:
https://core.trac.wordpress.org/ticket/51857
https://core.trac.wordpress.org/ticket/54166
https://core.trac.wordpress.org/ticket/54543

---

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

[peterwilsoncc]

Are you able to create a pull request that is the revert of the revert and then propose any changes as a PR on to that branch. That will allow the folks working on this ticket to keep track of what is happening.

I still don't have enough of an idea about the updater to approve new code, tracking down a bug is much easier than approval, so I'm willing to help out with testing and so on of patches but I don't want to be the committer because if it came to the crunch, I wouldn't be able to explain what I committed.

But once the new code is split out of this PR, I will happily assist with manual testing and reviewing architecture decisions.

[afragen]

Lots of testing with @costdev and @BronsonQuick. Below is a summary.

In testing, VirtualBox was the only common denominator. We found the following issues with VB.
https://www.virtualbox.org/ticket/1393
https://www.virtualbox.org/ticket/17971

When we tested it seems the issue is entirely about VirtualBox it not working well with the rename(). In testing, skipping the rename() in move_dir() for VB eliminated the issue entirely as the fallback is copy_dir().

We thought the most reproducible method of determining whether or not VirtualBox was running, was for the user to set either a constant or an environmental variable.

A new method in WP_Filesystem_Direct to test for either the new constant ENV_VB or the environmental variable WP_ENV_VB and return a boolean would be the simplest. Obviously the variable names can be changed.

All testing was via the Rollback Update Failure feature plugin which contained the changes.

Pinging @SergeyBiryukov and @hellofromtonya

[peterwilsoncc]

A new method in WP_Filesystem_Direct to test for either the new constant ENV_VB or the environmental variable WP_ENV_VB and return a boolean would be the simplest.

If at all possible, my preference would be to try and determine if the install is running on VirtualBox without requiring a constant be defined. VVV, Chassis and other WP dedicated systems could add these, other VB based set ups may not know to add them.

Obviously the variable names can be changed.

I'd go with something more generic but naming things is hard so will ponder.

[afragen]

Part of the issue is there doesn't seem to be a simple test to determine if a system is running on VirtualBox. If we ever do figure that out we can always add it to the is_virtual_box() function.

[afragen]

I'm removing the auto-update functionality to focus on the core Rollback feature. I will create a new ticket for the auto-update feature.

[azaozz]

Just a nitpick but thinking this code could be a bit more readable. Perhaps something like (untested):

$dest_dir = $wp_filesystem->wp_content_dir() . 'temp-backup/';
$sub_dir  = $dest_dir . $args['dir'] . '/';

if ( ! $wp_filesystem->is_dir( $sub_dir ) ) {
    if ( ! $wp_filesystem->is_dir( $dest_dir ) ) {
        $wp_filesystem->mkdir( $dest_dir );
    }

    if ( ! $wp_filesystem->mkdir( $sub_dir ) ) {
        // Couldn't make the backup directory.
        return new WP_Error( 'fs_temp_backup_mkdir', $this->strings['temp_backup_mkdir_failed'] );
    }
}

Something like this is also a better "code formatting" imho.

Also wondering why FS_CHMOD_DIR is not used here (may be missing something?). Seems to be used in every other call to $wp_filesystem->mkdir() in core.

[pbiron]

at 1st glance, I agree that your suggestion is easier to read. Will have to actually test to make sure the logic is the same. Will report back when that's been done.

agree that we should be chmod'ing to created dirs

[afragen]

Agree with above. Code tested and committed.

[SergeyBiryukov]

Thanks for the PR! Merged in r55720. Great job everyone! 🙌