ircWhois results in Notice and Warning

[hashworks]

$queue->ircWhois('', 'hashworks');

PHP Warning:  Missing argument 2 for Phergie\Irc\Client\React\WriteStream::ircWhois() in phergie/phergie-irc-client-react/src/WriteStream.php on line 434
PHP Notice:  Undefined variable: nickmasks in phergie-irc-client-react/src/WriteStream.php on line 436

[elazar]

This appears to be an issue with the first parameter, $server, not being considered optional by ircWhois() as it should be per RFC 1459 and RFC 2812.

[hashworks]

According to https://tools.ietf.org/html/rfc2812#section-3.6.2 you don't have to set a target server.

[elazar]

Correct. This is an issue in both WriteStream->ircWhois() and in GeneratorInterface and its Generator implementation. Filing an issue against that repo as well.

[elazar]

So, there are two ways to approach addressing this:

1. Maintain the current parameter order of ircWhois(), make the $nickmasks parameter optional, and require the method implementation to handle swapping the values of $server and $nickmasks if only a single parameter value is specified when ircWhois() is called. This maintains backward compatibility, but requires a bit more of the implementation and makes it slightly less clean.
2. Swap the parameter order of ircWhois() such that $server is last and as such can be defaulted to null. getIrcMessage() already handles filtering out null values. This means a cleaner implementation, but breaks backward compatibility because calls to the method have to change their parameter order.

Thoughts? @hashworks?

[Renegade334]

Option 1 is closer to the way it's implemented at a server level.

There is the Third Option™ which would be a filthy dose of func_get_args()...

[hashworks]

I'll answer next Sunday. I'm drunk till Saturday.

[elazar]

@hashworks Fair enough.

@Renegade334 I'd prefer to keep the solution as clean and simple as possible. Much as I don't care for the two options I've detailed above, they do meet both criteria.

[Renegade334]

Agreed. I just enjoy throwing spanners.

[elazar]

@stil Want to throw your hat in the ring on how this should be resolved? Your affected code.

[elazar]

Assuming we change the argument order, EventQueue in the bot repo will be affected as well.

[Renegade334]

I personally don't think that any write command should require its parameters to specified in a different order to how they will be sent.

[elazar]

@Renegade334 I believe there's only one other case where we've done just that, because of parameter order coupled with optionality.

[hashworks]

Is there any clean PHPDoc definition available for swapable variables? However it agitates me to do something like that in PHP. If we can change the parameter order in one go we should do it, it doesn't affect a ton of code. Additionally there are only a few people who use the ircWhois command.

My vote goes for solution number 2.

[stil]

As for me, I also would keep it clean as possible. Breaking backwards compatibility is ok if it's reflected in version change.

[elazar]

Question: does the affected ircWhois() method, as it stands now, work correctly under any circumstances? If not, we wouldn't even necessarily need to call it a BC break; it's a bug.

[hashworks]

@elazar: It works correctly. The issue is just about the warning, the whois works just fine.

[elazar]

@Renegade334 Sounds like the popular vote is option 2. Any thoughts before we proceed on that route?

[Renegade334]

Nope, I agree with keeping it consistent if there are other cases that already use reversed parameter order where the first parameter is optional.