implode() refers to wrong parameter name

[adrenth]

Description

The following code:

<?php

private function getRawExecResponseCode($command)
{
    $response = @ftp_raw($this->connection, trim($command));

    return (int) preg_replace('/\D/', '', implode(' ', $response)); // <---- line 577; $response is of type `string`
}

For complete file see: https://github.com/thephpleague/flysystem/blob/1.x/src/Adapter/Ftp.php#L577

Resulted in this output:

In Ftp.php line 577:
 implode(): Argument #1 ($pieces) must be of type array, string given

But I expected this output instead:

$result = $this->getRawExecResponseCode('CWD mydirectory');

echo $result;

// 250

Apparently the native ftp_raw method returns a string as return value instead of array|null.

Setting the connection configuration passive = true fixes the issue for now.
But this kind of type errors should not occur.

Related to: thephpleague/flysystem#1481

PHP Version

PHP 8.0.19

Operating System

CentOS Linux release 7.9.2009 (Core)

[KapitanOczywisty]

Is it likely that ftp_raw returns null, but implode produces confusing error (eval)? Please check $response and try removing @ to see what error is causing that.

[damianwadley]

^ is correct: ftp_raw is failing and returning null, resulting in implode thinking it's being called with the single-argument form of implode(array $array), however that first argument is the string ' ' and thus the error message.

The message also incorrectly cites a "$pieces" parameter. IIRC this was an old name for it - the docs call it "$array" now.

So NAB in the sense that flysystem is not properly checking the return value of ftp_raw.

However there's room for improvement here on the PHP side: mostly in the error message (such as fixing the parameter name) to help indicate what happened. IMO the proper fix is to change behavior based on the number of arguments passed, rather than the legacy style of checking if a parameter is null, but that would have to wait until PHP 9. Until then, the pieces-is-null-but-was-passed-as-an-argument condition could be checked and result in a slightly different message.

[cmb69]

I fully agree with @damianwadley here, except for:

IMO the proper fix is to change behavior based on the number of arguments passed, rather than the legacy style of checking if a parameter is null, […]

In my opinion, we should fade out all function overloads in the long run; they are confusing at best, and always have issues regarding the implementation (ZPP and stubs). So I'd rather deprecate calling the function with a single argument.

[damianwadley]

In my opinion, we should fade out all function overloads in the long run;

Oh yeah, if that's an option then definitely. Let's leave the unusual signatures like function implode(string $separator = "", array $pieces) to Javascript.

[KapitanOczywisty]

Let's leave the unusual signatures like function implode(string $separator = "", array $pieces) to Javascript.

Lol. JS have less weirdness in arguments than PHP...

[Girgias]

Should be resolved by #12683