search

hoaproject / Irc

The Hoa\Irc library.
https://hoa-project.net
25 stars 9 forks source link

Add `irc` transport wrapper. #22

Closed shulard closed 7 years ago

shulard commented 8 years ago

A specific Hoa\Irc\Socket definition has been added which contains the transport factory method.

The Socket is autoloaded through composer.json. The Client and Node have been updated to avoid class collision with Hoa\Socket.

The method used here is the same than in Hoa\Websocket. I checked that the default port for irc is 6667 so this is the fallback one.

shulard commented 8 years ago

@Hywan, the source Socket.php file have not been updated before the copy...

I've cleaned the PR :

shulard commented 8 years ago

Any news on that PR, is that change necessary ?

Hywan commented 8 years ago

Here is the scheme I have found about IRC:

I guess it's enought information so far :-).

shulard commented 8 years ago

I think it is 😄 I'll take a look at these two schemes and implement them. Thanks for the docs !

Hywan commented 8 years ago

Thanks for your work!

shulard commented 8 years ago

Hello,

I've pushed the ircs transport wrapper implementation. Like the Hoa\Websocket, the secure state of the socket is determined using transport wrappers.

I've read the docs and have some questions about the required implementation.

Default port

The port number to connect to. If  :<port> is omitted,  the 
default IANA assigned IRC port 194 is used. Clients may use port 
6667 as an alternate port in case connection to the default 
port fails. Clients should also maintain a default port number,
as well as associations of port numbers with specific hosts.

Seems that the standard port today is 6667 but do we need to fallback to 194 when the first connection fails ?

For me it's not the goal of an Irc library to handle that case but...

Question: Is 6667 the default port we must use because it's the most used and not the official default ?

URL Command analysis

The Irc connection URL can contains a lot of details about the connection that must be done : nickname, password, channel...

Do we need retrieving detailed URI informations and store them in the connection ? Maybe it's not in this PR that we need to address these questions...

Just that for the moment we are throwing away all additional informations given (authentication, channel...).

Hywan commented 7 years ago

About default port, I guess it's safe to assume 6667 is the default one today. Maybe Freenode or EFNet accept port 194 too. You should try. But if they not, then add a comment to explain why we have chosen 6667 over 194 as the RFC stipulates.

About the URL command analysis, you are totally right, this is out of the scope of this PR. You should open an issue to not forget to address this point, but this is an interesting feature!

shulard commented 7 years ago

I've created a new issue to address URI analysis: #25

I've also checked freenode and EFNet connection on port 194 with this script :

<?php
require_once 'vendor/autoload.php';

$uri    = 'tcp://chat.freenode.org:194';
$client = new Hoa\Irc\Client(new Hoa\Socket\Client($uri));

$client->run();

Got that error on freenode :

Uncaught Hoa\Socket\Client::_open(): (1) Client returns an error (number 65): No route to host while trying to join tcp://chat.freenode.org:194

And this one on EFNet :

Client returns an error (number 60): Operation timed out while trying to join tcp://irc.du.se:194

So I've added a comment to explain why we choose 6667 as default.

Hywan commented 7 years ago

I have starting this review 11 days ago, but because of a network issue (probably), it has never been sent… sorry!

Hywan commented 7 years ago

I had to commit this patch too, https://github.com/hoaproject/Irc/commit/668b622a4caac830978b09208e60c80a4ba36c71. Nothing important but the API documentation was incorrect.

Thank you very much for your work! The README.md file has been updated too https://github.com/hoaproject/Irc/commit/ab380eb18ea54f64b2e73a50494cbd02d1bb67bb.