WordPress.org

Make WordPress Core

Opened 7 weeks ago

Last modified 6 weeks ago

#54582 new defect (bug)

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

Reported by: pbiron Owned by:
Milestone: 6.0 Priority: high
Severity: major Version:
Component: Upgrade/Install Keywords: needs-patch needs-testing early early-like-actually-early
Focuses: Cc:

Description

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

Change History (11)

This ticket was mentioned in Slack in #cli by pbiron. View the logs.


7 weeks ago

#2 follow-up: @pbiron
7 weeks ago

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).

Last edited 7 weeks ago by pbiron (previous) (diff)

#3 in reply to: ↑ 2 @pbiron
7 weeks ago

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.

#4 follow-up: @afragen
6 weeks ago

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

$mixed_case_filename === $lower_case_filename

#5 in reply to: ↑ 4 @pbiron
6 weeks ago

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 :-)

#6 @costdev
6 weeks ago

@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().

#7 follow-up: @schlessera
6 weeks ago

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.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


6 weeks ago

#9 in reply to: ↑ 7 @pbiron
6 weeks ago

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 :-(

#10 @hellofromTonya
6 weeks ago

  • Keywords early added
  • Milestone changed from 5.9 to 6.0

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.

#11 @hellofromTonya
6 weeks ago

  • Keywords early-like-actually-early added
Note: See TracTickets for help on using tickets.