Make WordPress Core

Opened 2 years ago

Last modified 17 months ago

#54582 accepted defect (bug)

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

Reported by: pbiron's profile pbiron Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: Future Release Priority: high
Severity: major Version:
Component: Upgrade/Install Keywords: needs-testing early early-like-actually-early has-patch changes-requested needs-testing-info
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 (58)

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


2 years ago

#2 follow-up: @pbiron
2 years 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 2 years ago by pbiron (previous) (diff)

#3 in reply to: ↑ 2 @pbiron
2 years 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
2 years 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
2 years 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
2 years 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
2 years 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.


2 years ago

#9 in reply to: ↑ 7 @pbiron
2 years 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
2 years 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
2 years ago

  • Keywords early-like-actually-early added

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


2 years ago

#13 @schlessera
2 years ago

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.

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


2 years ago

#15 @pbiron
2 years ago

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

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


2 years ago

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


2 years ago

This ticket was mentioned in Slack in #core-auto-updates by afragen. View the logs.


2 years ago

#19 @peterwilsoncc
2 years ago

  • Milestone changed from 6.0 to Future Release

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.

This ticket was mentioned in Slack in #core-auto-updates by pbiron. View the logs.


2 years ago

This ticket was mentioned in Slack in #core-auto-updates by pbiron. View the logs.


2 years ago

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


2 years ago

#23 @SergeyBiryukov
2 years ago

  • Milestone changed from Future Release to 6.1

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


2 years ago

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


2 years ago

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


2 years ago

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


2 years ago

This ticket was mentioned in Slack in #core-auto-updates by afragen. View the logs.


2 years ago

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


2 years ago

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


23 months ago

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


22 months ago

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


22 months ago

This ticket was mentioned in Slack in #core-auto-updates by costdev. View the logs.


22 months ago

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


22 months ago

#35 @audrasjb
22 months ago

  • Milestone changed from 6.1 to Future Release

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 ticket was mentioned in PR #3109 on WordPress/wordpress-develop by SergeyBiryukov.


21 months ago
#36

  • Keywords has-patch added; needs-patch removed

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

#37 @SergeyBiryukov
21 months ago

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

Last edited 21 months ago by SergeyBiryukov (previous) (diff)

This ticket was mentioned in PR #3110 on WordPress/wordpress-develop by SergeyBiryukov.


21 months ago
#38

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 commented on PR #3109:


21 months ago
#39

Closed in favor of #3110.

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


21 months ago

#41 follow-up: @costdev
21 months ago

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

#42 in reply to: ↑ 41 @pbiron
21 months ago

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.

#43 @costdev
21 months ago

Thanks @pbiron! 😊

#44 @pbiron
21 months ago

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.

#45 @costdev
21 months ago

  • Keywords changes-requested has-testing-info added

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.

This ticket was mentioned in Slack in #core-auto-updates by costdev. View the logs.


21 months ago

This ticket was mentioned in Slack in #core-auto-updates by costdev. View the logs.


21 months ago

#48 @SergeyBiryukov
19 months ago

  • Milestone changed from Future Release to 6.2

#49 @SergeyBiryukov
19 months ago

  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

This ticket was mentioned in Slack in #core-upgrade-install by wojsmol. View the logs.


19 months ago

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


19 months ago

This ticket was mentioned in Slack in #core-upgrade-install by afragen. View the logs.


19 months ago

This ticket was mentioned in Slack in #core-upgrade-install by afragen. View the logs.


18 months ago

This ticket was mentioned in Slack in #core-upgrade-install by costdev. View the logs.


18 months ago

#55 @hellofromTonya
18 months ago

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.

This ticket was mentioned in PR #3842 on WordPress/wordpress-develop by @ironprogrammer.


17 months ago
#56

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

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


17 months ago

#58 @audrasjb
17 months ago

  • Keywords needs-testing-info added; has-testing-info removed
  • Milestone changed from 6.2 to Future Release

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

Note: See TracTickets for help on using tickets.