Make WordPress Core

Opened 5 years ago

Last modified 3 months ago

#48689 assigned defect (bug)

PHP warnings after updating to WP 5.3: ftp_nlist() and ftp_pwd() expect missing parameters

Reported by: hinjiriyo's profile Hinjiriyo Owned by: costdev's profile costdev
Milestone: 6.7 Priority: normal
Severity: minor Version: 5.3
Component: Filesystem API Keywords: php80 needs-unit-tests has-patch 2nd-opinion changes-requested dev-feedback
Focuses: php-compatibility Cc:

Description

I updated several websites to WP 5.3 without any problems. But on one wesite I got these PHP warnings both in the backend and in the website:

Warning: ftp_nlist() expects parameter 1 to be resource, null given in /wp-admin/includes/class-wp-filesystem-ftpext.php on line 402
Warning: ftp_pwd() expects parameter 1 to be resource, null given in /wp-admin/includes/class-wp-filesystem-ftpext.php on line 226
Warning: ftp_pwd() expects parameter 1 to be resource, null given in /wp-admin/includes/class-wp-filesystem-ftpext.php on line 226
Warning: ftp_pwd() expects parameter 1 to be resource, null given in /wp-admin/includes/class-wp-filesystem-ftpext.php on line 226
Warning: ftp_nlist() expects parameter 1 to be resource, null given in /wp-admin/includes/class-wp-filesystem-ftpext.php on line 402
Warning: ftp_pwd() expects parameter 1 to be resource, null given in /wp-admin/includes/class-wp-filesystem-ftpext.php on line 226
Warning: ftp_pwd() expects parameter 1 to be resource, null given in /wp-admin/includes/class-wp-filesystem-ftpext.php on line 226
Warning: ftp_pwd() expects parameter 1 to be resource, null given in /wp-admin/includes/class-wp-filesystem-ftpext.php on line 681
Warning: ftp_pwd() expects parameter 1 to be resource, null given in /wp-admin/includes/class-wp-filesystem-ftpext.php on line 226
Warning: ftp_pwd() expects parameter 1 to be resource, null given in /wp-admin/includes/class-wp-filesystem-ftpext.php on line 226

Warning: Cannot modify header information - headers already sent by (output started at /wp-admin/includes/class-wp-filesystem-ftpext.php:402) in /wp-includes/functions.php on line 5946
Warning: Cannot modify header information - headers already sent by (output started at /wp-admin/includes/class-wp-filesystem-ftpext.php:402) in /wp-admin/includes/misc.php on line 1252
Warning: Cannot modify header information - headers already sent by (output started at /wp-admin/includes/class-wp-filesystem-ftpext.php:402) in /wp-admin/admin-header.php on line 9

I suppressed the ouput by "muting" the function calls with '@' in the file 'class-wp-filesystem-ftpext.php', i.e. changed ftp_nlist() to @ftp_nlist() etc..

I will be glad if any reason can be found and fixed until the next WP upgrade.

Attachments (1)

48689.diff (1.5 KB) - added by adamsilverstein 3 years ago.

Download all attachments as: .zip

Change History (37)

#1 follow-up: @SergeyBiryukov
5 years ago

Hi there, thanks for the report!

Related: [45611/trunk/src/wp-admin/includes/class-wp-filesystem-ftpext.php], which removed error suppression for ftp_nlist() and ftp_pwd() here. It's likely that the issue comes from somewhere else, but is now more visible.

I could not reproduce it on a clean install though, some more details about the environment and the steps to reproduce would be helpful.

#2 @Hinjiriyo
5 years ago

Due to the many websites on many servers I maintain (~15), it is difficult to see anything special about the respective WP installation. The debugging is further complicated by the fact that nowhere in this website the class WP_Filesystem_FTPext seems to be called and used.

#3 @Hinjiriyo
5 years ago

Additional comment: if a plugin is upgraded this text appears on the "Update Plugins" page between "Enabling Maintenance mode..." and "Updating Plugin {x} ({m}/{n})":

Warning: ftp_rmdir(): /.maintenance: File or directory not found in /wp-admin/includes/class-wp-filesystem-ftpext.php on line 381

#4 @Hinjiriyo
5 years ago

  • Severity changed from critical to minor

I could detect the plugin which was the reason of this error. So I would close this ticket. But I think "muting" more PHP file function calls with '@' in the file 'class-wp-filesystem-ftpext.php' makes still sense, since many other function calls in this file are set as muted.

#5 follow-up: @SirLouen
5 years ago

I've found who makes this happen: WPML Multilingual CMS in combination with RankMath.

Last edited 5 years ago by SirLouen (previous) (diff)

#6 in reply to: ↑ 5 @Hinjiriyo
5 years ago

Replying to SirLouen:

I've found who makes this happen: WPML Multilingual CMS in combination with RankMath.

In the case of my website it was the plugin "Amazon Associates Link Builder". Since I deactivated it, there are no error messages. Was it a combination with another plugin? I don't know and didn't research that.

#7 @SirLouen
5 years ago

I have inyected a little bit of extra verbosity through some php error logging function to discover that Rankmath was trying to access to certain files (.htaccess and robots.txt) without success. I'm not sure why WPML (also happened by activating AMP for WP plugin in combination with Rankmath) was hindering Rankmath from this access...

The function that was being hindered is exists from WP Filesystem.
So probably any plugin in combination with WPML or AMP for WP may have this issue?
https://developer.wordpress.org/reference/classes/wp_filesystem_base/exists/
Not 100% sure.

This is the rankmath snippet which was making this misbehaviour.

<?php
public static function get_htaccess_data() {
                $wp_filesystem = WordPress::get_filesystem();
                $htaccess_file = get_home_path() . '.htaccess';

                return ! $wp_filesystem->exists( $htaccess_file ) ? false : [
                        'content'  => $wp_filesystem->get_contents( $htaccess_file ),
                        'writable' => $wp_filesystem->is_writable( $htaccess_file ),
                ];
        }

I'm not a code maintainer of Rankmath. In fact I have uninstalled from a production site, and left it in a staging server. But just wanted to inform about this because it could be useful for WP maintainers (I'm not sure).

Last edited 5 years ago by SirLouen (previous) (diff)

#8 @erikgeurtsplatformiqcom
5 years ago

This changeset https://core.trac.wordpress.org/changeset/45611/trunk/src/wp-admin/includes/class-wp-filesystem-ftpext.php (for example in line 226) unearths these PHP warnings by removing the @ symbol. It means the warning is no longer suppressed, but it's been an issue before this change.

In my case, it was also the addition of a plugin, but not the same as the previous comments, that brought out the warnings. The cause is not in these plugins directly, but in the fact that certain plugins in certain environments make an attempt to use the WP filesystem in a way that apparently is not expected.

The actual issue in https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/class-wp-filesystem-ftpext.php?rev=45611 since that change in July 2019 is that $this->link doesn't get a value assigned first, so any attempt to use the variable in this code throws the warning.

It seems that the if statement in https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/class-wp-filesystem-ftpext.php?rev=45611#L86 is where things go wrong.

In my case, the problem may come from the fact that I'm used to defining the FTP hostname, user and password in the wp-config.php, yet doing this incorrectly or with something missing. So in fact this may be a documentation issue (i.e. not having followed the documentation to the letter).

Where can the documentation for defining the FTP credentials in the wp-config.php be found?

#9 @aiko2011
5 years ago

Проблемный плагин у меня Pirateforms, последнее обновление WP его не поддерживает ;)

Last edited 5 years ago by aiko2011 (previous) (diff)

#10 in reply to: ↑ 1 @aiko2011
5 years ago

Replying to SergeyBiryukov:

Hi there, thanks for the report!

Related: [45611/trunk/src/wp-admin/includes/class-wp-filesystem-ftpext.php], which removed error suppression for ftp_nlist() and ftp_pwd() here. It's likely that the issue comes from somewhere else, but is now more visible.

I could not reproduce it on a clean install though, some more details about the environment and the steps to reproduce would be helpful.

Привет!
Были такие же проблемы.
Проблемный плагин у меня Pirateforms, последнее обновление WP его не поддерживает ;)

#11 follow-up: @orenwolf
5 years ago

It appears that this problem occurs whenever a plugin tries to access a file, but doesn't have write access, and tries instead to use FTP to access it.

#12 in reply to: ↑ 11 @aiko2011
5 years ago

Replying to orenwolf:

It appears that this problem occurs whenever a plugin tries to access a file, but doesn't have write access, and tries instead to use FTP to access it.

https://ru.wordpress.org/support/topic/pirate-forms/

#13 @BearlyDoug
5 years ago

I also was receiving this... I thought it was tied to the WP Astra theme (https://wpastra.com/), since I'm not getting it on sites not running the WP Astra Theme and WP Astra Pro plugin.

The first result I found for this is located HERE

I added

if (!defined('FS_METHOD')) define('FS_METHOD', 'direct');

to my wp-config.php file, and now the error messages actually have stopped.

This issue didn't show up until WordPress 5.3 was released, and is KILLING my Apache log files.

#14 @SergeyBiryukov
4 years ago

#51283 was marked as a duplicate.

#15 @absotively
4 years ago

I see similar warnings, though I've tracked it to a different plugin.

At least on my site, the warnings appear on the site even if WP_DEBUG is set to false.

Edit: Looking more closely at the plugin in question, I realize this may be due to it calling the relevant functions too early, so it may not be relevant to this bug after all.

Last edited 4 years ago by absotively (previous) (diff)

#16 @SirLouen
4 years ago

  • Keywords close added
  • Resolution set to invalid
  • Status changed from new to closed

@BearlyDoug this problem happens when it (a plugin most of the time) tries to use an FTP-based FS_METHOD like ftpext wrongly. Considering that you switched to "direct" then, this 2 functions ftp_nlist and ftp_pwd will not be required more.

Some plugins although, disregard the FS_METHOD and they force using an FTP therefore this messages could appear.

I'm still not 100% sure, but according to my research, operating with FTPext in WP changed some time ago, not sure after which patch, maybe 5.2, 5.3, 5.4 but some plugins have still failed to adapt to the new mechanism.

For example, MainWP did not patch the FTPext method, so now they force using direct to all their clients. If you set ftpext for FS_METHOD you will see these errors.

I don't think this is a bug from WordPress, but a bug from plugin authors so we might close this.

#17 @desrosj
3 years ago

  • Keywords close removed
  • Milestone Awaiting Review deleted

#18 @adamsilverstein
3 years ago

  • Keywords php8 added
  • Resolution invalid deleted
  • Status changed from closed to reopened

I don't think this is purely plugin related. I can reproduce this on my local when visiting the Site Health screen, with no plugins active.

In my case, the web server user is not the owner of the site files and thus WordPress will attempt to use ftp if I try a plugin update for example and apparently on the Site Health screen as well. Fixing the file ownership issue made the problem go away for me.

Worth noting this is a fatal in PHP 8+.

This description seems to identify the underlying issue.

#19 @adamsilverstein
3 years ago

  • Owner set to adamsilverstein
  • Status changed from reopened to assigned

#20 @adamsilverstein
3 years ago

The issue arrises because we pass the file link to several ftp functions without first validating it. In certain conditions (for example, when WordPress lacks the correct file permissions), the file link is null.

Passing null to ftp_pwd, ftp_chdir and ftp_nlist throws a warning in PHP 7.4 and a fatal in PHP 8+. to avoid this I've added some checks to the ftpext functions that were throwing the errors for me. After this patch I can load the Site Health page with PHP 8.

The patch is a POC:

  • not sure this is the best way to fix the issue, didn't look for potential issues with other related classes (eg. WP_Filesystem_ftpsockets)
  • needs tests that validate whatever fix we choose.

#21 @desrosj
2 years ago

  • Keywords needs-unit-tests has-patch needs-testing 2nd-opinion added; needs-patch removed
  • Milestone set to Future Release

Re-adding a milestone for this one.

#22 @ThemeZee
2 years ago

This issue also seems to happen by using the WPTT/webfont-loader packing in some server environments:
https://make.wordpress.org/themes/2020/09/25/new-package-to-allow-locally-hosting-webfonts/

#23 @sc0ttkclark
21 months ago

The patch makes a lot of sense here but it doesn't cover ssh2 which also makes use of $this->link and would exhibit the same type of issue as it uses $this->link in functions that expect a resource.

The WP_Filesystem_ftpsockets class is safe because uses ftp_sockets or ftp_pure classes (depending on sockets use) which already checks to see if the ftp resource is connected before calling the functions that would cause errors like this.

To move forward:

  • Add checks in WP_Filesystem_SSH2 that check $this->link before passing it to a function expecting a resource
  • Add tests for ftpext and ssh2 in cases where a connection could not be made, confirming the methods return as expected (with no errors)

#24 @hellofromTonya
17 months ago

  • Keywords php80 added; php8 removed

#25 follow-up: @hellofromTonya
13 months ago

  • Focuses php-compatibility added
  • Milestone changed from Future Release to 6.4

The issue arrises because we pass the file link to several ftp functions without first validating it. In certain conditions (for example, when WordPress lacks the correct file permissions), the file link is null.

48689.diff adds property validation to silently bail out early. But the $link property is documented to always be a resource. This ticket identifies scenarios where the file link may fail.

I'm thinking about:

  • Should the file link property be documented as resource|null? Is null a valid value?
  • Where these "certain conditions" first happen, is there error logging to alert developers why it failed?
  • Should a failure stop the progression of the work in the filesystem, meaning the code should not get to the methods where the warning / fatal are thrown?
  • If null is a valid value for the file link property, then protecting the methods that act upon it makes sense if those methods are reachable in the call stack.

Silently bailing out without an early error can make debugging harder while potentially leaving users and developers wondering why their action failed.

I'm marking this ticket as a PHP 8.0 php-compatibility issue and pulling it into the 6.4 cycle to hopefully move it forward.

#26 in reply to: ↑ 25 @SergeyBiryukov
13 months ago

Replying to hellofromTonya:

Should the file link property be documented as resource|null? Is null a valid value?

I think not, as pretty much all methods of this class operate under an assumption that the connection was already initialized in WP_Filesystem() via $wp_filesystem->connect().

The only case where $this->link is null is when the connection was for some reason not initialized properly, and that could use some further investigation to determine the cause. It could be missing or incorrect credentials, like in #58940. So I think resource is the correct type here, and null is not a valid value.

The same applies to the WP_Filesystem_SSH2 class too.

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


12 months ago

#28 @nicolefurlan
12 months ago

During our bug scrub today we all discussed what @hellofromTonya mentioned about how silently bailing out early is not ideal. @costdev is going to take a closer look at this!

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


11 months ago

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


11 months ago

#31 @nicolefurlan
11 months ago

  • Milestone changed from 6.4 to 6.5
  • Owner changed from adamsilverstein to costdev

This ticket was discussed in today's bug scrub.

This ticket is getting pushed to 6.5. There was some concern about doing this work late in the cycle without tests or testing.

@hellofromTonya mentioned:

A full suite of tests are coming for the Filesystem. That'll help.

Props to @hellofromTonya and @costdev :)

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


7 months ago

#33 @chaion07
7 months ago

  • Milestone changed from 6.5 to 6.6

Thanks @Hinjiriyo for reporting this. We reviewed this ticket during a recent bug-scrub and based on the feedback we are updating the milestone to 6.6 since @costdev didn't have adequate time to review this again.

Cheers!

Prop to @mukesh27

#34 @oglekler
3 months ago

  • Keywords changes-requested dev-feedback added; needs-testing removed

WP_Filesystem_SSH2 has

		if ( ! $this->link ) {
			return false;
		}

in one place. Possibly way forward is to add function that generate error and bail like this:

		if ( ! $this->link ) {
			$this->error_missing_link( 'SSH2' );
			return false;
		}

....
	private function error_missing_link( $connection_name ) {
		$this->errors->add(
			'connection_name',
			sprintf(
				/* translators: %s: Connection type. */
				__( 'Missing connection: %s' ),
				$connection_name
			)
		);
	}

This can be done for WP_Filesystem and for WP_Filesystem_SSH2 classes.

This ticket was mentioned in Slack in #core-test by hugod. View the logs.


3 months ago

#36 @hellofromTonya
3 months ago

  • Milestone changed from 6.6 to 6.7

With the last scheduled beta happening in a couple of hours, moving this one to 6.7. However, if a patch is ready and there's consensus for commit, then please feel free to pull it back into 6.6 milestone.

Note: See TracTickets for help on using tickets.