Make WordPress Core

Opened 8 months ago

Closed 8 months ago

#58121 closed defect (bug) (wontfix)

WP_Filesystem_SSH2::put_contents() assumes that PHP's file_put_contents() always returns in int

Reported by: pbiron's profile pbiron Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Filesystem API Keywords:
Focuses: Cc:

Description

See https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/class-wp-filesystem-ssh2.php#L285.

The PHP manual for file_put_contents() says

This function returns the number of bytes that were written to the file, or false on failure.

I'm not sure it file_put_contents() could always return a boolean false or whether than is a PHP 8 thing, but we should make sure that the Filesystem API accounts for it

Change History (4)

#1 @afragen
8 months ago

From php.net:file_put_contents

This function returns the number of bytes that were written to the file, or false on failure.

Warning
This function may return Boolean false, but may also return a non-Boolean value which evaluates to false. Please read the section on Booleans for more information. Use the === operator for testing the return value of this function.

#2 @afragen
8 months ago

Are we trying to mimic the response of file_put_contents() in $wp_filesystem::put_contents()?

#3 @pbiron
8 months ago

No. And WP_Filesystem_SSH2::put_contents() does use strict comparison when checking the return value from file_put_contents() so I don't think any errors/warnings would be raised if file_put_contents() were to return false.

So, maybe there's no change needed, but I just wanted to get other opinions about whether we should change the conditional to:

<?php
if ( false === $ret || strlen( $contents ) !== $ret ) {
   return false;
}

to avoid the call to strlen() when it's not necessary for an early return.

Last edited 8 months ago by pbiron (previous) (diff)

#4 @pbiron
8 months ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Just looked into how PHP's strlen() is implemented.

It's O(1) (there's a len property associated with every string and that's what's returned), so I guess there's not a big performance hit for a needless call to strlen().

So, I'll close this as wontfix.

Note: See TracTickets for help on using tickets.