[wp-trac] [WordPress Trac] #56966: Updating plugins with WP6.1 creates .maintenance file and leaves it

[WordPress Trac]

#56966: Updating plugins with WP6.1 creates .maintenance file and leaves it
-------------------------------------------------+-------------------------
 Reporter:  jsh4                                 |       Owner:  (none)
     Type:  defect (bug)                         |      Status:  new
 Priority:  normal                               |   Milestone:  6.1.1
Component:  Upgrade/Install                      |     Version:  6.1
 Severity:  normal                               |  Resolution:
 Keywords:  has-testing-info needs-testing       |     Focuses:
  needs-patch dev-feedback                       |
-------------------------------------------------+-------------------------
Changes (by costdev):

 * keywords:  has-testing-info needs-testing needs-patch => has-testing-info
     needs-testing needs-patch dev-feedback


Comment:

 I've reproduced the issue using ProFTPD. After looking into this, I'm
 going to step through each part below.

 This is a long one.

 -----

 [https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-
 admin/includes/class-wp-upgrader.php#L894-L907 maintenance_mode()] calls:
 {{{#!php
 <?php
 $file = $wp_filesystem->abspath() . '.maintenance';
 $wp_filesystem->delete( $file );
 }}}

 Note the signature of [https://github.com/wordpress/wordpress-
 develop/blob/2dce57706fa56db2e931be853ee036049921884f/src/wp-
 admin/includes/class-wp-filesystem-ftpext.php#L387
 WP_Filesystem_FTPext::delete()]:
 {{{#!php
 <?php
 public function delete( $file, $recursive = false, $type = false )
 }}}

 `WP_Filesystem_FTPext::delete()` contains this:
 {{{#!php
 <?php
 if ( 'f' === $type || $this->is_file( $file ) ) {
         return ftp_delete( $this->link, $file );
 }
 }}}

 `maintenance_mode()` doesn't pass the `$type` argument, so let's focus on
 `$this->is_file( $file )`.

 [https://github.com/wordpress/wordpress-
 develop/blob/2dce57706fa56db2e931be853ee036049921884f/src/wp-
 admin/includes/class-wp-filesystem-ftpext.php#L437-L439
 WP_Filesystem_FTPext::is_file()]
 {{{#!php
 <?php
 public function is_file( $file ) {
         return $this->exists( $file ) && ! $this->is_dir( $file );
 }
 }}}

 `! $this->is_dir( $file )` returns `true`, which is fine, so let's focus
 on `$this->exists( $file )`.

 [https://github.com/wordpress/wordpress-
 develop/blob/2dce57706fa56db2e931be853ee036049921884f/src/wp-
 admin/includes/class-wp-filesystem-ftpext.php#L421-L427
 WP_Filesystem_FTPext::exists() - 6.1]
 {{{#!php
 <?php
 public function exists( $path ) {
         if ( $this->is_dir( $path ) ) {
                 return true;
         }

         return ! empty( ftp_rawlist( $this->link, $path ) );
 }
 }}}

 Let's go ahead and skip `$this->is_dir( $path )` as we know this returns
 `false`.

 On to `ftp_rawlist()`...

 Most FTP servers have the `-a` enabled by default. This includes hidden
 files, and so for these servers, an existing `.maintenance` file will
 return a non-empty result from `ftp_rawlist()`, meaning the file exists.
 ✅

 However, ProFTPD doesn't include this switch by default, so won't pick up
 the file. ❌

 In addition, including the switch when an FTP server already includes it
 can throw an error, so we can't just call `ftp_rawlist( $this->link, ' -a
 ' . $path )`. ❗

 -----

 Therefore, from what I've seen so far, we need to:

 1. Call `ftp_rawlist()` without the switch. This will cover:
   - Existing non-hidden files. ✅
   - Existing hidden files on FTP servers who enable the switch by default.
 ✅
 2. If the file doesn't exist, we then call `@ftp_rawlist()` ''with'' the
 switch. This will cover:
   - Existing hidden files on FTP servers who don't enable the switch by
 default. ✅
   - **Note:** The `@` operator is needed to suppress errors when `$file`
 doesn't exist on FTP servers that enable the switch by default. I know,
 [https://core.trac.wordpress.org/ticket/24780 we're trying to reduce the
 usage of this operator], and I still need to reproduce the error to see if
 it's a fatal, as `@` [https://php.watch/versions/8.0/fatal-error-
 suppression won't work on PHP8+] in that case.

 This looks like:
 {{{#!diff
 <?php
 public function exists( $path ) {
         if ( $this->is_dir( $path ) ) {
                 return true;
         }

         if ( ! empty( ftp_rawlist( $this->link, $path ) ) ) {
                 return true;
         }

 +       /*
 +        * Include '-a' switch for FTP servers that do not enable it by
 default.
 +        *
 +        * For some FTP servers that enable the switch by default,
 including the
 +        * switch again can produce an error, so the '@' operator must be
 used.
 +        */
 +       if ( ! empty( @ftp_rawlist( $this->link, ' -a ' . $path ) ) ) {
 +               return true;
 +       }

         return false;
 }
 }}}

 Now, that requires confirmation of the type of error thrown, as well as
 discussion on whether this approach is agreeable to committers.

 For those unfamiliar, the aim of #51170 was to make
 `WP_Filesystem_FTPext::exists()` compliant with [https://www.rfc-
 editor.org/rfc/rfc959 RFC 959] (CTRL+F `NAME LIST (NLST)` for
 `ftp_nlist()` - 6.0, `LIST (LIST)` for `ftp_rawlist()` - 6.1). The current
 implementation, and the above code block, are compliant with RFC 959. The
 implementation up to WordPress 6.0 was not.

 I've tested the above code block on vsftpd, PureFTPd and ProFTPD, and it's
 successful on all three.

 For `WP_Filesystem_FTPext`, this means:
 - `::exists()` will continue to be RFC 959 compliant.
 - `::exists()` will work with FTP servers that do/do not enable the `-a`
 switch by default.

 -----

 Adding `dev-feedback`.

 I also still need to find out what type of warning/error is thrown on FTP
 servers that already enable the switch and `-a` is passed with
 `ftp_rawlist()`. FileZilla server does this, so if anyone can set up
 WordPress to use `define( 'FS_METHOD', 'ftpext' )` running FileZilla
 server, implement the above change, remove the `@` operator, and call
 `::exists()` on a file that doesn't exist, that would be great!

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/56966#comment:12>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform