$wp_filesystem->exists('') returns a wrong answer (when FS_METHOD='ftpext').

[Zdrobau]

If the method $wp_filesystem->exists() is called with an empty string and FS_METHOD = 'ftpext' then the result is true, I think it is wrong, because when FS_METHOD is defined as 'direct' returns false.

[dd32]

Personally I think calling with an empty string is the incorrect behaviour here.

FTP is most likely returning that the current directory exists (Since that's what leaving the parameter off the ls -la calls will do)

[Zdrobau]

I want to use the method WP_Upgrader::install_package() with

$args['clear_destination'] = true

In it exists the following code that causes problems:

$removed = true;
if ( $wp_filesystem->exists( $remote_destination ) ) {
    $removed = $wp_filesystem->delete( $remote_destination, true );
}

[dd32]

If $remote_destination is an empty string, you've got deeper problems.

Sounds like the destination parameter to WP_Upgrader::install_package() is invalid, doesn't exist, or something else around that.

[Zdrobau]

I got this error because I use the following methods:

WP_Upgrader::download_package(); WP_Upgrader::unpack_package()

, and I want to install a theme from the zip archive that have a name like

theme-name-.-templates-.- 1.0.1-.-md5.zip

in a folder like

wp-content/themes/theme-name-parent

None of them has the option to rename this. When I call

WP_Upgrader::install_package()

with

$args['destination'] = WP_CONTENT_DIR . '/ themes /' . 'theme-name-parent'

and

FS_METHOD = 'direct'

this is possible. But not when

FS_METHOD = 'ftpext'

I think that

WP_Upgrader::install_package()

method should have the same behavior in both cases.

[dd32]

Is it possible to attach a plugin which shows the behaviour you're seeing?

[mkox]

I was able to see the problem @Zdrobau might have been experiencing here. The behavior for the different FS_METHOD values is inconsistent for wp_filesystem->exists(). This is due to different implementations of the exists methods.

In ​https://www.ietf.org/rfc/rfc959.txt page 32 you can see that the ftp_nlist is based on ftp_list. If that method receives an empty path, it will check the current working directory. So the ftpext implementation of exists may return true (maybe always for empty paths), but direct returns false.

The exists method should never be called empty, yes. But exists should behave equally in both impls.

[mkox]

Check for empty filepath arg in exists method

[mkox]

The patch will return false for empty filepath args, so both implementations behave equally.

When ftp_nlist() or nlist() receive an empty path, they check the current working directory and may return true.

For consistency with the WP_Filesystem_Direct filesystem abstraction class, this returns false when an empty path is provided to WP_Filesystem_FTPext::exists() or WP_Filesystem_ftpsockets::exists().

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

[costdev]

Test plugin.

[costdev]

After some testing, it appears that both the FTPext and ftpsockets ::exists() methods return true for an empty path.

​PR 4224 updates the ::exists() methods for both WP_Filesystem_FTPext and WP_Filesystem_ftpsockets to return false when passed ''.

The PR uses a strict check on '' rather than empty(), to prevent an existing path of '0' returning false.

I've also attached a test plugin, 33058_test_filesystem_exists.zip, which shows the results for the direct, ftpext and ftpsockets filesystem methods. The test plugin requires that you have an FTP server installed and running.

[costdev]

Testing Instructions

These steps define how to reproduce the issue or test a patch, and indicate the expected behavior.

Steps to Test

1. Set up an FTP server on your local system. Make a note of the user and password.
2. Download, install and activate the 33058_test_filesystem_exists.zip plugin.
3. In wp-content/plugins/33058_test_filesystem_exists/33058_test_filesystem_exists.php, make sure that the FTP_BASE, FTP_USER and FTP_PASS constants are set to the appropriate values for your environment.
4. 🐞 Note the results for each Empty path exists: entry.
5. Apply ​PR 4224.
6. Refresh the admin page.
7. ✅ Note the results for each Empty path exists: entry.

Expected Results

When reproducing a bug:

• ❌ The admin notice shows "No" for direct, and "Yes" for ftpext and ftpsockets.

When testing a patch to validate it works as expected:

• ✅ The admin notice should show "No" for direct, ftpext and ftpsockets.

Hi @mukeshpanchal27, unfortunately we don't have an FTP docker image available yet to test this properly.

Ok Thanks. @costdev for quick reply.

[pbiron]

Test Report

Windows 11
Apache 2.4.53
FileZilla Server 1.6.6 (FTP)

Before Patch Applied

Filesystem method: direct
Empty path exists: No

Filesystem method: ftpext
Empty path exists: Yes

Filesystem method: ftpsockets
Empty path exists: Yes

After Patch Applied

Filesystem method: direct
Empty path exists: No

Filesystem method: ftpext
Empty path exists: No

Filesystem method: ftpsockets
Empty path exists: No

LGTM

[costdev]

Thanks @pbiron! As this change is a simple guard and doesn't employ any FTP functionality, adding for commit consideration.

[azaozz]

Replying to costdev:

this change is a simple guard and doesn't employ any FTP functionality

Wondering if any of these methods support the ​FTP command CWD? If yes, seems an empty exists() may be expected to return true as it will be checking the current working directory?

[pbiron]

Replying to azaozz:

Replying to costdev:

this change is a simple guard and doesn't employ any FTP functionality

Wondering if any of these methods support the ​FTP command CWD? If yes, seems an empty exists() may be expected to return true as it will be checking the current working directory?

Thanx for looking at this @azaozz . The point of this ticket is that the current behavior of ::exists('') for the FTP filesytems is different than that for the direct filesystem.

And that incongruity could cause problems (not likely for core's use of the filesystem classes, but for extenders' uses).

[costdev]

Wondering if any of these methods support the FTP command CWD? If yes, seems an empty exists() may be expected to return true as it will be checking the current working directory?

Some thoughts:

• The filesystem classes have a ::cwd() method that returns the CWD (via ftp_pwd() and such). I'm not sure why someone would want to do an exists on an empty string as opposed to the better (IMO) ::exists( ::cwd() ), except for the specific reason that FTP checks the working directory as a sort of "quirk". It certainly wouldn't be a portable approach to use ::exists( '' ) if a site didn't use an FTP filesystem.
• Let's say I'm going to delete something. I'm going to check if ( ::exists( $possible_empty_string ) ). If that returns true, I'm going to delete it. That means potentially deleting the current working directory if I didn't realise I was passing ''. Instead, I'd prefer a situation where if I run if ( ::exists( $possible_empty_string ) ), I get a "false" false, and when trying to create that directory, get a warning that /your/current/working/directory/ already exists, so I know I need to check something.
• The direct filesystem is by far the most used filesystem. For consistency, the other classes should generally aim to mimic its behaviour, rather than the other way around.
• Some documentation in the FTP ::exists() methods' docblocks might be a useful addition here to explain the reasoning.

[SergeyBiryukov]

In 55556:

Filesystem API: Return false for empty paths in FTP ::exists() methods.

When ftp_nlist() receives an empty path, it checks the current working directory and may return true.

This affects:

• WP_Filesystem_FTPext::exists()
• WP_Filesystem_ftpsockets::exists()

As the purpose of the API is to provide a consistent interface for various filesystem implementations, this commit updates the affected methods to returns false when an empty path is provided, bringing consistency with the other filesystem abstraction classes, specifically WP_Filesystem_Direct and WP_Filesystem_SSH2.

Follow-up to [6779], [11821], [25274], [31815].

Props mkox, costdev, Zdrobau, dd32, pbiron, azaozz, mukesh27, SergeyBiryukov.
Fixes #33058.

Thanks for the PR! Merged in r55556.

[azaozz]

Replying to costdev:

I'm not sure why someone would want to do an exists on an empty string as opposed to the better (IMO) ::exists( ::cwd() )

makes sense.

• Some documentation in the FTP ::exists() methods' docblocks might be a useful addition here to explain the reasoning.

Yep, think so too. At least have to mention that doing ::cwd( $path ) followed by ::exists('') would return false for consistency with the other filesystem methods.