First of all, the parameter name in the error message is plain wrong indeed, it uses the old name which we renamed for PHP 8.0.

implode($array, null) mimics the implode(array $array): string signature by avoiding the UNKNOWN default value. In that sense, the null default value makes sense, since the 2nd parameter basically does not exist, and in this case, the $array parameter is the 1st one. So all in all, I'd be more happy to only improve the error message, and not the default value.

P.S. To be honest, this function should have been included to my overloaded signature RFC, but somehow I didn't find it. :/

We can also do that, but my main goal here was to disallow passing null as the second parameter. This was the main point of confusion for me. Allowing implode($array, null) doesn't make sense because it's more likely that if someone passes the second argument, they want the first argument to be a string. With an empty array as the default parameter, we avoid having to use UNKNOWN.

Also, the documentation doesn't help in this case because in the documentation, it's said that the signature is implode(string $separator, array $array): string, which is not true. The second parameter is marked as non-nullable. At least my fix would make the reality match more closely to what is being documented.

Do you see any disadvantages or errors in my bug fix?

Hi,

Maybe the function implementation should switch on ZEND_NUM_ARGS() and act differently depending on wheter it is 1 or 2 ?

This would have the benefit to make the logic easier to follow (I find it currently confusing - as shown by @jorgsowa's comment).

switch ZEND_NUM_ARGS() {
  case 1:
    // this is the 1-argument signature,
    // argument #1 ($array) must be of type array
    break;
  case 2:
    // this is the 2-arguments signature,
    // argument #1 ($separator) must be of type string
    // argument #2 ($array) must be of type array
    break;
  default:
    // already checked by ZPP
    ZEND_UNREACHABLE();
}

I just want to add that whichever way we do it, the error message will be inaccurate. The main thing we should fix is to disallow passing null as the second param.

TBH, I can't remember having ever used the 1-argument form of this function. I agree with @kocsismate that it should just be deprecated.

I disagree with deprecating it - it's a bit unfortunate that the order is $separator, $array instead of $array, $separator.
But apart from that, I'm using the 1-argument form all the time.

it's a bit unfortunate that the order is $separator, $array instead of $array, $separator

What's even more unfortunate is that we deprecated the latter form... :(

The main thing we should fix is to disallow passing null as the second param.

I don't have a good idea, only a somewhat unorthodox one: let's reuse join() for the 1-parameter signature. This would mean that we should deprecate the 2-parameter version of join() + the 1-parameter version of implode(), which is a very arguable decision I know, but at least this would give us clear signatures and error messages...

Maybe the function implementation should switch on ZEND_NUM_ARGS() and act differently depending on wheter it is 1 or 2 ?

Relying on the number of parameters passed would make the default value of the 2nd parameter UNKNOWN IMO which is worse then the current situation.

it's a bit unfortunate that the order is $separator, $array instead of $array, $separator

What's even more unfortunate is that we deprecated the latter form... :(

Well, that's highly subjective... In functional languages for example, the collection to operate on is always passed last (because of partial application). So from my POV, it's rather fortunate that for once PHP chose the correct order instead of having it backwards like in array_filter...

But anyway, consistency and correct error messages matter more than style preferences.

Maybe the function implementation should switch on ZEND_NUM_ARGS() and act differently depending on wheter it is 1 or 2 ?

Relying on the number of parameters passed would make the default value of the 2nd parameter UNKNOWN IMO which is worse then the current situation.

Just to clarify, my comment did not concern the function signature, just how the internal logic was implemented - which was initially so confusing to follow that several people initially thought that the patch was doing something wrong WRT the argument number. Unless I'm missing something, switching on ZEND_NUM_ARGS internally does not make the default value UNKNOWN in any way.

I don't think we can disallow this in the stable version as it might potentially break some apps that are using implode($array, null) (doesn't really matter if it is confusing - we should just not introduce such BC break). I'm fine with this going to 8.3 though.

I rebased this PR against master now. For PHP 8.1, I will only make the tiny change to the text of the error message to use $array instead of $pieces.

Superceded by #12683