Make WordPress Core

Opened 11 years ago

Last modified 4 months ago

#28013 reopened defect (bug)

WP_Filesystem_FTPext::exists($filename) returns false when file exists on FTP Service ( IIS/6.0 Microsoft & Linux/vsftp )

Reported by: joostdekeijzer's profile joostdekeijzer Owned by: dd32's profile dd32
Milestone: Future Release Priority: normal
Severity: normal Version: 3.9.2
Component: Filesystem API Keywords: close
Focuses: administration Cc:

Description

I'm having trouble using the update/upgrade functions of WordPress on a Windows IIS/6.0 server.

The .maintenance file is never deleted.

I have to use the FTPext method of updating and I've found that WP_Filesystem_FTPext::exists('.maintenance') returns false even though the file exists.

Since the fix for #10060 that function uses ftp_nlist() which returns invalid results on the IIS/6.0 Microsoft FTP Service.

I did some more testing and it turns out that the current function has issues with "dotfiles"

Work correctly
WP_Filesystem_FTPext::exists('index.php')
WP_Filesystem_FTPext::exists('wp-content/debug.log')

Fail
WP_Filesystem_FTPext::exists('.maintenance')
WP_Filesystem_FTPext::exists('wp-content/.maintenance')

(yes, I made sure those files actually existed ;-)

For this particular purpose, couldend WP_Upgrader::maintenance_mode() be modified to test for the existence of .maintenance using is_file( ABSPATH . $file ) and if so execute $wp_filesystem->delete($file, false, 'f'); (setting the f bypases all the tests)?

Attachments (2)

28013.diff (916 bytes) - added by joostdekeijzer 11 years ago.
test for .maintenance using php is_file()
wp.maintenance.patch (760 bytes) - added by hnhn2 7 years ago.
treat .maintenance file deletion as file DELETE instead of RMDIR

Download all attachments as: .zip

Change History (34)

@joostdekeijzer
11 years ago

test for .maintenance using php is_file()

#1 @joostdekeijzer
11 years ago

  • Keywords has-patch added

Added patch to use is_file() to check if there is a .maintenance file.

Tested on IIS/6.0 with FTP and works fine.
Tested on Apache (MacPorts) with "Direct access" (not using ftp) and works fine.

#2 @tellyworth
10 years ago

  • Version changed from trunk to 3.9.2

This applies to 3.9 at least, probably older versions too.

#3 @joostdekeijzer
10 years ago

This issue does not exist on IIS/8.5

Version 0, edited 10 years ago by joostdekeijzer (next)

#4 @jcroucher
9 years ago

  • Summary changed from WP_Filesystem_FTPext::exists($filename) returns false when file exists on IIS/6.0 Microsoft FTP Service to WP_Filesystem_FTPext::exists($filename) returns false when file exists on FTP Service ( IIS/6.0 Microsoft & Linux/vsftp )

This issue still exists but I am finding it on a Linux box using vsftp. I am getting this across multiple boxes.
I am running Wordpress 4.4.2

The issue is in

wp-admin/includes/class-wp-filesystem-ftpext.php

public function exists($file) {
                $list = @ftp_nlist($this->link, $path);

                if ( empty( $list ) && $this->is_dir( $file ) ) {
                        return true; // File is an empty directory.
                }

                return !empty($list); //empty list = no file, so invert.
        }

The problem seems to be that ftp_nlist does not return . files, such as .maintenance
This was causing the site to be stuck in maintenance mode after each update.

I have fixed it by changing the ftp_nlist line to

$list = @ftp_rawlist($this->link, "-al" .$file);

There are numerous posts on other forums mentioning the issue of ftp_nlist not returning hidden files.

Last edited 9 years ago by jcroucher (previous) (diff)

#5 @dd32
9 years ago

  • Keywords early added
  • Milestone changed from Awaiting Review to Future Release
  • Owner set to dd32
  • Status changed from new to accepted

I see no issue with switching to ftp_rawlist here, I guess it varies between servers as to which ones include -a by default. It makes sense that some would have hidden files disabled by default.
I think there's also an argument to be made that we should use wordpress.maintenance instead or something similar.

The actual change here would probably be just to $list = @ftp_rawlist($this->link, "-a" . $file ); (lacking the -l).

Marking as early for 4.4, as I don't want to change upgrade-related code during beta.

#6 @samuelsidler
9 years ago

  • Keywords 4.4-early added

#7 @dd32
9 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 33648:

Upgrades: When upgrading via FTP, use LIST -a to detect if a file exists.
LIST & NLST by default on some servers require the -a flag to view hidden files (ie. .maintenance)
Although we could simply pass the -a flag to NLST, opting to use LIST which we use elsewhere should mean less chance of server incompatibilities.

Props jcroucher.
Fixes #28013.

#8 @dd32
9 years ago

  • Keywords early 4.4-early removed
  • Milestone changed from Future Release to 4.4

This issue still exists but I am finding it on a Linux box using vsftp.

@jcroucher Can you let me know what your configuration of vsftp is? I've been unable to make it work by using either force_dot_files or hide_files directives.. I'd like to actually test this change properly :)

If someone reports strange upgrade behaviour in the 4.4 cycle of it not detecting files properly, please re-open :)

#9 @mmucklo
9 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

This fix did not work for me.

I may be doing something wrong, and I apologize in advance for wasting your time if so.

My update still fails

I patched my 4.2 code actually (been coming across this same issue trying to update a 4.2 installation).

I've been stepping through via XDEBUG and was coming to the same issue.

In includes/update-core.php this line of code is failing.

if ( $wp_filesystem->exists( $from . $root . 'readme.html' ) && $wp_filesystem->exists( $from . $root . 'wp-includes/version.php' ) ) {

This is calling into:
wp-admin/includes/class-wp-filesystem-ftpext.php

Both functions fail:
It fails with ftpn_list and it fails with ftp_rawlist

Maybe because input to command is a file:
The reason it fails is that you're passing in a $file into a command that seems to require a directory as it's argument. I may be wrong here, however when I pass in dirname($file) into the former command, things work fine.

The docs seem to indicate the required parameter is a directory, not a file.
http://php.net/manual/en/function.ftp-nlist.php
http://php.net/manual/en/function.ftp-rawlist.php

Suggestion:
Maybe first try the raw list, if that doesn't work, then do a rawlist of dirname($file) (given dirname($file) is not empty), and then check to see if the list returned contains the file asked for.

Maybe something similar to this:

if (empty($list) && !$this->is_dir($file) && !empty($dirname = dirname($file))) {
    $testList = @ftp_rawlist( $this->link, $dirname );
    if (!empty($testList)) {
        // Find if $file exists in $testList, set some flag, or $list itself, etc.
    }
}

Let me know and I can give you a working example...

#10 @dd32
9 years ago

Thanks for the feedback @mmucklo!

The reason it fails is that you're passing in a $file into a command that seems to require a directory as it's argument. I may be wrong here, however when I pass in dirname($file) into the former command, things work fine.

When a file is presented, a FTP server that respects the protocol is supposed to return information about that file (Ignore whatever the PHP docs suggest here), unfortunately there are a number of FTP servers out there which obviously do not conform to spec.

The alternative solution, as you suggest, is to list the contents of the directory above it to determine if the file is present.
Can you try switching out the exists method for this one?

	public function exists( $file ) {
		$path = dirname( $file );
		$filename = basename( $file );

		$file_list = @ftp_nlist( $this->link, '-a ' . $path );

		return $list && in_array( $filename, $file_list );
	}

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


9 years ago

#12 @SergeyBiryukov
9 years ago

  • Keywords needs-patch added; has-patch removed

#13 @dd32
9 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 34733:

Updates: Try a more compatible method to detect if a file exists when using the FTP Extension.
This change causes it to list the parent directories files, and assets that the node exists within the returned listing, this is a little more compatible than relying upon the FTP server to correctly filter the returned resultset to the specific file/node being requested.
Fixes #28013

#14 @dd32
9 years ago

  • Keywords needs-patch removed
  • Milestone changed from 4.4 to Future Release
  • Resolution fixed deleted
  • Status changed from closed to reopened

Re-opening this ahead of a revert for 4.4.1 & trunk.

See #34976 and #35026 where this has caused an issue. Long story short: Many servers are not happy with this new method.

See my patches on #34976 and comments on #35026 which could be helpful in fixing this in the future.

#15 @dd32
9 years ago

In 35944:

Filesystem: Revert [33648] and [34733] unfortunately these have caused issues for some servers, while fixing it for others.

See #28013

Fixes #34976 for trunk
Fixes #34976 for trunk

#16 @dd32
9 years ago

In 35945:

Filesystem: Revert [33648] and [34733] unfortunately these have caused issues for some servers, while fixing it for others.

See #28013
Fixes #34976 for the 4.4 branch
Fixes #34976 for the 4.4 branch

#17 @dd32
9 years ago

#35967 was marked as a duplicate.

#18 @jcroucher
8 years ago

Is there any update to this? The fix in [34733] does work for me.
I am still manually updating that file on all my installs so it will work.

I should add, that initial bug breaks maintenance mode also. As in, update a plugin. it creates the .maintenance file, but it never deletes it. Adding in the patch from 34733 fixes this issue.

Last edited 2 years ago by SergeyBiryukov (previous) (diff)

#19 @dd32
8 years ago

There's unlikely to be any updates to this in the next few versions.

If it works on your server, great, if it doesn't, unfortunately your only options are to switch FTP servers, FTP configurations, or try to avoid needing to use FTP at all.

#20 @hnhn2
7 years ago

This is rather painful. Seeing this on several 4.9.4 installations (Linux vsftpd) and any plugin updates just bring the sites down!

I don't understand two things
here:

  • Why at :839 bother with ->exists($file) when at :837 there is no such extra care? If the call on a non-existent file has undesired consequences, why it's being handled in one case only? Yes, the call is returning an empty list in this case, but it also seems superfluous.
  • At :841 the call to $wp_filesystem->delete( $file ); invokes RMDIR in vsftp, which fails. So regardless of the failing exists() call, this seems to be the main problem. Shouldn't there instead be $wp_filesystem->delete($file, false, 'f');?

@hnhn2
7 years ago

treat .maintenance file deletion as file DELETE instead of RMDIR

#22 @desrosj
2 years ago

In 54815:

Filesystem: Return FTP/FTP Sockets exists() methods to a previous state.

This partially reverts [53860] and [53862], which refactored the exists() method to rely on ftp_rawlist() instead of ftp_nlist().

[53860] makes a similar attempt to the ones made in [33648] and [34733] (which were also reverted in [35944]). Being compliant with the specifications while continuing to work without issue for all FTP servers continues seem impossible. These little ghosts are the ones we’re scared of the most.

Props jsh4, afragen, costdev, pkolenbr, SergeyBiryukov, dd32, peterwilsoncc, gamecreature, desrosj.
Fixes #56966.
See #51170, #28013.

#23 @desrosj
2 years ago

In 54816:

Filesystem: Return FTP/FTP Sockets exists() methods to a previous state.

This partially reverts [53860] and [53862], which refactored the exists() method to rely on ftp_rawlist() instead of ftp_nlist().

[53860] makes a similar attempt to the ones made in [33648] and [34733] (which were also reverted in [35944]). Being compliant with the specifications while continuing to work without issue for all FTP servers continues seem impossible. These little ghosts are the ones we’re scared of the most.

Props jsh4, afragen, costdev, pkolenbr, SergeyBiryukov, dd32, peterwilsoncc, gamecreature, desrosj.
Merges [54815] to the 6.1 branch.
Fixes #56966.
See #51170, #28013.

#24 @desrosj
2 years ago

  • Keywords close added

This continues to prove very difficult to solve with the problems surfaced through #51170/#56966.

I'm going to mark this with a suggestion to close. @dd32 @costdev do you have any thoughts around trying again in 6.2?

#25 @costdev
2 years ago

I'd like to keep this ticket open so that investigation and attempts can continue.

What's proving most difficult is an inadequate testbase. Being able to run the code against a list of FTP servers would make moving this forward / determining its feasibility much smoother. Each revert reduces contributor/committer confidence in finding a workable solution.

Last edited 2 years ago by costdev (previous) (diff)

#26 @SergeyBiryukov
2 years ago

#51170 was marked as a duplicate.

#27 @zippy1970
18 months ago

Why is this taking so long to fix? First it would be fixed in 6.0, then it was moved to 6.1, now we're in 6.2 and it still hasn't been fixed.

In the meantime, each time I want to update my WordPress sites, I need to manually replace the exists() function with my own version - which, BTW, works flawlessly:

<?php
       public function exists( $file ) {
          $retval = false;

          $list = ftp_nlist( $this->link, $file );
          if( ! empty( $list ) ) {
            // if ftp_nlist returns *something*, the file or directory exists, on any FTP server
            $retval = true;
          } else {
            // if ftp_nlist returns nothing, either the file/dir doesn't exist or it's a file and
            // the FTP server's NLST command doesn't support globbing (i.e. Pure-FTPD > v1.0.47)
            // Check if it's a file
            if( ftp_size( $this->link, $file ) >= 0 ) {
              $retval = true;
            }
          }
          return $retval;

        }

Last edited 18 months ago by zippy1970 (previous) (diff)

#28 @hellofromTonya
11 months ago

#39781 was marked as a duplicate.

#29 @zippy1970
5 months ago

And more than a year later since my last post, this still hasn't been fixed. Why?

#30 @costdev
5 months ago

@zippy1970 Please see this comment explaining that a sufficient testbase is needed to cover a range of FTP servers and environments.

In addition, many contributors are volunteers with limited time to contribute, so we have try to spend this limited time on where we think we can be most effective. It's not that this issue lacks importance though (I've worked on this issue in the past too), we just can't work on all things at once - even after a year.

#31 @zippy1970
4 months ago

@costdev I appreciate that. But this problem hasn't been around for just a year. This problem has been around for over ten years now. And in its current form almost 7 years (since 2017). And as for having a sufficient testbase, isn't that a problem with every suggested change?

#32 @zippy1970
4 months ago

Also, the problem arose when (several) FTP servers dropped support for globbing. My suggested fix works on servers with and without (support for) globbing. And perhaps it will not work on all servers (although I seriously doubt that). But at least it will work on a whole lot more servers than it does now. And it will not break on servers where the current version of the function works. It will still work there as well. So you loose nothing, but gain a lot.

Note: See TracTickets for help on using tickets.