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 | Owned by: | 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)
Change History (37)
#2
@
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
@
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
@
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:
↓ 6
@
5 years ago
I've found who makes this happen: WPML Multilingual CMS in combination with RankMath.
#6
in reply to:
↑ 5
@
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
@
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).
#8
@
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
@
5 years ago
Проблемный плагин у меня Pirateforms, последнее обновление WP его не поддерживает ;)
#10
in reply to:
↑ 1
@
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()
andftp_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:
↓ 12
@
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
@
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.
#13
@
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.
#15
@
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.
#16
@
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.
#18
@
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.
#20
@
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
@
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
@
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
@
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)
#25
follow-up:
↓ 26
@
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
? Isnull
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
@
13 months ago
Replying to hellofromTonya:
Should the file link property be documented as
resource|null
? Isnull
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
@
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
@
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
@
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
@
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.
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()
andftp_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.