Problem with deleting old files at the end of a core update when the filesystem is case-insensitive

[pbiron]

WP 5.9 updates the external Requests library (used by ​wp_remote_get(), et. al). As part of that update, a number of filenames have changed in case only, e.g.

• wp-includes/Requests/IRI.php => wp-includes/Requests/Iri.php
• wp-includes/Requests/Transport/cURL.php => wp-includes/Requests/Transport/Curl.php

When updating to 5.9-beta1 via wp-cli on a system that uses a case-insensitive filesystem (not only Windows, but that's the most obvioous example) via the wp core update --version=5.9-beta command, all those files that have been renamed in case only get incorrectly deleted (in ​Core_Command::cleanup_extra_files()) which results in PHP fatal error(s). This was discovered during the 5.9-beta1 release party, and an ​issue was created against that wp-cli command.

Core has it's own means on deleting files from previous releases, but it doesn't come into play until a major release package to built. wp-admin/includes/update-core.php defines the $_old_files array that contains files from all previous WP versions that should be deleted by core after an update has been performed. That array is not amended until just before the major version package is created (see ​Releasing Major Versions in the Core Handbook].

I've been experimenting and I think the exact same thing will happen when 5.9 is is released and sites are updated via the Dashboard.

To test this hypothesis I did the following on a Windows machine (with basically a WAMP stack):

1. Installed 5.8.2
2. Edited wp-admin/includes/class-core-upgrader.php and added global $_old_files; $_old_files[] = 'wp-includes/Requests/IRI.php'; just before line 172. This simulates that file being added to the $_old_files array in wp-admin/includes/update-core.php of the 5.9 release package.
3. Added define( 'WP_AUTO_UPDATE_CORE', 'beta' ); to wp-config.php. This allows core to update to 5.9-beta1 in Dashboard > Updates (without the need for wp-cli nor the ​Beta Tester plugin.
4. Went to Dashboard > Updates and clicked on the Update to version 5.9-beta1 button
5. When the update finished, I looked on disk and found that neither wp-includes/Requests/IRI.php nor wp-includes/Requests/Iri.php existed
6. Went back to Dashboard > Updates (which causes ​wp_version_check(), and thus wp_remote_get(), to be called...resulting in a fatal error (Fatal error: Uncaught Error: Class 'WpOrg\Requests\Iri' not found in ABSPATH\wp-includes\Requests\Requests.php on line 682)

One might thing that to avoid this problem, any files whose name case in case only just shouldn'd be added to $_old_files. Unfortunately, that would be bad for sites with case-senstive filesystems, because then files with both filenames would exist on disk after the update (e.g., both wp-includes/Requests/Iri.php and /wp-includes/Requests-IRI.php`).

To prove this to myself, I then did the following on a machine with a case-sensitive filesystem (centos 7):

1. installed 5.8.2
2. Added define( 'WP_AUTO_UPDATE_CORE', 'beta' ); to wp-config.php
3. Went to Dashboard > Updates and clicked on the Update to version 5.9-beta1 button
4. When the update finished, I looked on disk and found that both wp-includes/Requests/IRI.php and wp-includes/Requests/Iri.php existed
5. Rolled back to 5.8.2
6. Edited wp-admin/includes/class-core-upgrader.php and added global $_old_files; $_old_files[] = 'wp-includes/Requests/IRI.php';` just before line 172
7. Went to Dashboard > Updates and clicked on the Update to version 5.9-beta1 button
8. When the update finished, I looked on disk and found that only wp-includes/Requests/Iri.php existed, as it should be

As mentioned, I've only tested these hypotheses on Windows and linux (centos). It would be great if someone could test on MacOS (in both it's case-sensitive and case-insensitive filesystem configurations)!!!

What do I think should be done to solve this? The "delete old files" routines in both core and wp-cli should check whether the filesystem is case-insensitive and, if so, not delete files whose names have changed only in case (note: the current ​PR against wp-cli's wp core update command doesn't yet do this). I've been experimenting with how to detect whether the filesystem is case-insensitive (hint: just checking the OS is not sufficient, for a number of reasons). I'll add some comments about this after I finish opening this ticket.

Related: #54546, #54547, ​https://github.com/wp-cli/core-command/issues/195

[pbiron]

Here's what I currently have for the check whether the filesystem is case-insensitive. I'm thing this could be added as a method to WP_Filesystem_Direct:

function is_filesystem_case_insenstive() {
        $temp_dir = get_temp_dir();
        if ( ! is_dir( $temp_dir) ) {
                mkdir( $temp_dir );
        }

        $mixed_case_filename = tempnam( $temp_dir, 'MixedCase' );
        $lower_case_filename = strtolower( $mixed_case_filename );
        touch( $lower_case_filename );

        // Compare the Inodes: if they are the same, then filesystem is case-insensitive.
        $filesystem_case_insensitive = fileinode( $mixed_case_filename ) === fileinode( $lower_case_filename );

        unlink( $mixed_case_filename );
        if ( ! $filesystem_case_insensitive ) {
                unlink( $lower_case_filename );
        }

        return $filesystem_case_insensitive;
}

It seems to work as expected, but needs some error checking added (e.g., test that tempnam() and fileinode() succeed). Not yet sure what to do if those fail, i.e., what should it return in that case.

My thought with comparing the Inodes is that is more foolproof than doing something like a glob() and counting the number of results returned...but I could certainly overthinking it. Would love feedback on that.

I've tested this in Windows and 2 different linux environments (centos a public server and centos running within WSL on Windows) and it reports correctly in all 3 cases. Would love to have someone test in MacOS (both case-sensitive and case-insensitive configs).

[pbiron]

Replying to pbiron:

My thought with comparing the Inodes is that is more foolproof than doing something like a glob() and counting the number of results returned...but I could certainly overthinking it. Would love feedback on that.

@wojsmol has pointed me to how ​Joomla deals with case-insensitivity.

I note that they also compare inodes. I've updated is_filesystem_case_insenstive() to use ​fileinode() instead of calling stat to get the inode like Joomla does...I didn't know fileinode() function existed in PHP :-)

Not being familiar with Joomla, I'll have to take a while to look over their code in more detail to see if there is anything else we can "borrow" from it.

[afragen]

Forgive my ignorance but if a filesystem is case-insensitive wouldn't

$mixed_case_filename === $lower_case_filename

[pbiron]

Replying to afragen:

Forgive my ignorance but if a filesystem is case-insensitive wouldn't

$mixed_case_filename === $lower_case_filename

Not necessarily. Take MacOS, for example, which by default is case-insensitive by case-preserving.

That is, if you do:

touch( 'Foo.txt' );
touch( 'foo.txt' );

There will only be 1 file on the filesystem (with filename Foo.txt).

So, strtolower( 'Foo.txt' ) === strtolower( 'foo.txt' ) doesn't tell you whether to filesystem is, in fact, case-insensitive :-)

[costdev]

@pbiron It seems to work as expected, but needs some error checking added (e.g., test that tempnam() and fileinode() succeed). Not yet sure what to do if those fail, i.e., what should it return in that case.

Some thoughts from a chat on Slack about this:

If the method should always default to a value:

• @pbiron thinks it should default to false and treat the filesystem as case sensitive. I concur.
• This would maintain BC behaviour.
• This would reduce support tickets from users who receive an error.

However, if it's determined that the method should return null or WP_Error on failure:

• I think that returning null is fine, but it isn't informative.
• Alternatively, returning WP_Error is informative. ​See here for a similar WP_Error but for wp_tempnam().

[schlessera]

As far as I can tell from documentation, retrieving the fileinode will always return 0 for all files on Windows for versions of PHP below 7.4.

From ​https://www.php.net/manual/en/function.fileinode.php:

As documented in ​https://www.php.net/manual/en/function.stat.php#refsect1-function.stat-returnvalues:
On Windows, as of PHP 7.4.0, this is the identifier associated with the file, which is a 64-bit unsigned integer, so may overflow. Previously, it was always 0.

It appears that fileinode shares the same underlying implementation.

[pbiron]

Replying to schlessera:

As far as I can tell from documentation, retrieving the fileinode will always return 0 for all files on Windows for versions of PHP below 7.4.

This has been confirmed. I installed PHP 7.3.33 on my local and am getting 0 for both fileinode() and stat()['ino'].

Windows strikes again :-(

[hellofromTonya]

​Decision was made to revert Requests 2.0.0 and punt it and all associated tickets to WordPress 6.0. Why? Requests library revealed 2 issues in the upgrader process that need resolution, including this issue reported in this ticket and the related #54547 in WP-CLI.

Moving this ticket to 6.0 and marking as early. A multitiered testing strategy will be needed early in the development process to validate and refine the implementation that resolves the case-insensitivity detection issue.

[schlessera]

Just for reference, this is what I ended up using for WP-CLI v2.6.0 for now: ​https://github.com/wp-cli/core-command/blob/v2.1.1/src/Core_Command.php#L1375-L1463

It was taken from Joomla, but then adapted, as the scenario is a bit different.

[pbiron]

I just tested the case-insensitive file cleanup code from wp-cli 2.6.0 and it seems to work great! I did the following:

1. fresh install of 5.8.3 on Windows (WAMP stack, w/ PHP 7.4.16)
2. wp core update --version=5.8-beta1
   • this step attempted to recreate the conditions that caused me to open this ticket originally
   • the correct files were delete and wp_remote_get() worked fine
3. wp core update --version=5.8-beta2
   • the correct files were delete and wp_remote_get() worked fine
   • this step also showed that the new code file cleanup code in wp-cli 2.6.0 meant that the "​2 step update process" from the 5.9-beta2 announcement post isn't necessary with this new code

So, at this time, it seems the approach taken in wp-cli 2.6.0 is a good candidate for a patch for core (for updating from the dashboard).

[peterwilsoncc]

As this doesn't have a patch and is tagged early (actually early).

I'm going to bump this from 6.0 as we're a couple of weeks from beta and it doesn't appear anyone has bandwidth to work on this issue.

[audrasjb]

As per today's bugscrub, let's move this ticket back to Future Release for the same reason it was moved outside of milestone 6.1.

This is an initial attempt to adapt the code from WP-CLI's Core_Command::cleanup_extra_files() method to avoid unintentionally deleting files on case-insensitive filesystems when a core update changes the case of a file.

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

[SergeyBiryukov]

The ​linked PR is an initial attempt from a coding session with @poena, @aristath, and @SergeyBiryukov to adapt the code from WP-CLI's ​PR #196 where this problem was solved. This is not tested yet, as the correct testing steps need to be figured out. Any help in testing appreciated :)

This is an initial attempt to adapt the code from WP-CLI's Core_Command::cleanup_extra_files() method to avoid unintentionally deleting files on case-insensitive filesystems when a core update changes the case of a file.

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

Closed in favor of #3110.

[costdev]

@pbiron, do you have bandwidth at the moment to test ​PR 3110?

[pbiron]

Replying to costdev:

@pbiron, do you have bandwidth at the moment to test ​PR 3110?

I try to set some time aside this weekend. thanx for the ping.

[costdev]

Thanks @pbiron! 😊

[pbiron]

see ​https://github.com/WordPress/wordpress-develop/pull/3110/files#r956790145 for one change necessary to the PR.

This is pretty hard to test, but I think it works.

My brain is pretty fried at the moment (working 7 days a week will do that to you :-)). Let me get some sleep and I'll post a more complete testing report in the morning.

[costdev]

I did a couple of test runs on Local for Windows and the patch seems to work just fine.

Testing Instructions

These steps define how to test ​PR 3110, and indicate the expected behavior.

Setup

1. Set up a new test site. Do not test this on wordpress-develop.
2. Create the file wp-content/mu-plugins/wpcore-force-custom-core-package.php with the following contents:

   <?php
   
   /**
    * Plugin Name: WP Core - Force custom Core package.
    * Description: Forces a custom Core upgrade package for testing.
    * Author:      WordPress Core Contributors
    * Author URI:  https://make.wordpress.org/core
    * License:     GPLv2 or later
    * Version:     1.0.0
    */
   
   add_filter( 'upgrader_pre_download', 'wpcore_force_custom_core_package', 10, 2 );
   function wpcore_force_custom_core_package( $reply, $package ) {
           global $wp_version;
           $core_update_str = 'downloads.wordpress.org/release/wordpress-';
           $not_core_update = false === strpos( $package, $core_update_str );
           if ( $not_core_update ) return false;
           return __DIR__ . '/custom-package.zip';
   }
   

3. Download ​WordPress 5.9-beta1.zip.
4. Copy the ZIP file to wp-content/mu-plugins/.
5. Rename the ZIP file to custom-package.zip.
6. Open the ZIP file and replace the contents of wp-admin/includes/update-core.php with ​these contents.
7. Replace _cleanup_old_files() with _cleanup_old_files();.
8. Replace global $_old_files; with global $_old_files, $wp_filesystem;.
9. Add the following to the bottom of $_old_files:

   <?php
   'wp-includes/Requests/IRI.php',
   'wp-includes/Requests/SSL.php',
   'wp-includes/Requests/IPv6.php',
   'wp-includes/Requests/Exception/HTTP.php',
   'wp-includes/Requests/Exception/Transport/cURL.php',
   'wp-includes/Requests/Proxy/HTTP.php',
   'wp-includes/Requests/Transport/fsockopen.php',
   'wp-includes/Requests/Transport/cURL.php',
   

10. Save the wp-admin/includes/update-core.php file back into the ZIP.
11. Make a backup copy of the ZIP (the upgrader will delete the ZIP after each update).

Steps to Test

1. Navigate to Dashboard > Updates and click the update button. Don't worry what version it says.
2. After the update, navigate to wp-includes/Requests/

Expected Results

• ✅ The following files should exist and the filename case should match these exactly:

  <?php
  'wp-includes/Requests/Iri.php'
  'wp-includes/Requests/Ssl.php'
  'wp-includes/Requests/Ipv6.php'
  'wp-includes/Requests/Exception/Http.php'
  'wp-includes/Requests/Exception/Transport/Curl.php'
  'wp-includes/Requests/Proxy/Http.php'
  'wp-includes/Requests/Transport/Fsockopen.php'
  'wp-includes/Requests/Transport/Curl.php'
  

Additional Notes

• When repeating tests, remember to copy the backup ZIP file to custom-package.zip.
• Always retain the backup for future tests.

[hellofromTonya]

In 54997:

External Libraries: Update Requests library to version 2.0.0.

This is a major release and contains breaking changes.

Most important changes to be aware of for this release:

• All code is now namespaced. Though there is a full backward compatibility layer available and the old class names are still supported, using them will generate a deprecation notice (which can be silenced by plugins if they'd need to support multiple WP versions). See the ​upgrade guide for more details.
• A lot of classes have been marked final. This should generally not affect userland code as care has been taken to not apply the final keyword to classes which are known to be extended in userland code.
• Extensive input validation has been added to Requests. When Requests is used as documented though, this will be unnoticable.
• A new WpOrg\Requests\Requests::has_capabilities() method has been introduced which can be used to address #37708.
• A new WpOrg\Requests\Response::decode_body() method has been introduced which may be usable to simplify some of the WP native wrapper code.
• Remaining PHP 8.0 compatibility fixed (support for named parameters).
• PHP 8.1 compatibility.

Release notes: ​https://github.com/WordPress/Requests/releases/tag/v2.0.0

For a full list of changes in this update, see the Requests GitHub:
​https://github.com/WordPress/Requests/compare/v1.8.1...v2.0.0

This commit also resolves 2 blocking issues which previously caused the revert of [52244]:

• New Requests files are loaded into wp-includes/Requests/src/, matching the location of the library. In doing so, filesystems that are case-insensitive are not impacted (see #54582).
• Preload: During a Core update, the old Requests files are preloaded into memory before the update deletes the files. Preloading avoids fatal errors noted in #54562.

Follow-up to [50842], [51078], [52244], [52315], [52327], [52328].

Props jrf, schlessera, datagutten, wojsmol, dustinrue, soulseekah, szepeviktor. costdev, sergeybiryukov, peterwilsoncc, ironprogrammer, antonvlasenko, hellofromTonya, swissspidy, dd32, azaozz, TobiasBg, audrasjb.
Fixes #54504.
See #54582, #54562.

Refreshes #3110, incorporating feedback and additional testing on case-insensitive filesystems:

This is an initial attempt to adapt the code from WP-CLI's Core_Command::cleanup_extra_files() method to avoid unintentionally deleting files on case-insensitive filesystems when a core update changes the case of a file.

Props SergeyBiryukov, pbiron, mukeshpanchal27, costdev

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

[audrasjb]

As per today's bug scrub, let's move this ticket to Future Release to give it more time for refining/testing.