Skip to content
This repository has been archived by the owner on Sep 20, 2021. It is now read-only.

Add irc transport wrapper. #22

Merged
merged 2 commits into from
Oct 14, 2016

Conversation

shulard
Copy link
Contributor

@shulard shulard commented Feb 2, 2016

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.

*
* New BSD License
*
* Copyright © 2007-2015, Hoa community. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2016 Did you start from a fresh master?

@Hywan Hywan self-assigned this Feb 3, 2016
@Jir4 Jir4 mentioned this pull request Feb 3, 2016
@shulard shulard force-pushed the feature/irc-transport-wrapper branch 2 times, most recently from eac4f5e to 4cf26a3 Compare February 3, 2016 10:36
@shulard
Copy link
Contributor Author

shulard commented Feb 3, 2016

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

I've cleaned the PR :

  • Remove the commit about construct from string behaviour
  • Fix copyright date
  • Fix exception message

@shulard
Copy link
Contributor Author

shulard commented Sep 27, 2016

Any news on that PR, is that change necessary ?

);
}

$port = isset($parsed['port'])?$parsed['port']:6667;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, fix CS with hoa devtools:cs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you read https://tools.ietf.org/html/rfc7194. Maybe we can support irc-u:// (if this is a correct scheme)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've applied code style and read RFC7194.

About irc-u:// what the u means for you ? Is it for IRCU ports assignment ? I think irc-u can be considered as a valid scheme.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #22 (comment) for more general considerations.

Copy link
Member

@Hywan Hywan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is definitively useful. Please, continue!

);
}

$port = isset($parsed['port'])?$parsed['port']:6667;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you read https://tools.ietf.org/html/rfc7194. Maybe we can support irc-u:// (if this is a correct scheme)?

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`.
@shulard shulard force-pushed the feature/irc-transport-wrapper branch from 4cf26a3 to 2c02fe4 Compare September 27, 2016 09:59
@Hywan
Copy link
Member

Hywan commented Sep 27, 2016

Here is the scheme I have found about IRC:

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

@shulard
Copy link
Contributor Author

shulard commented Sep 27, 2016

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

@Hywan
Copy link
Member

Hywan commented Sep 27, 2016 via email

@shulard
Copy link
Contributor Author

shulard commented Sep 30, 2016

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
Copy link
Member

Hywan commented Oct 3, 2016

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
Copy link
Contributor Author

shulard commented Oct 3, 2016

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.

@shulard shulard force-pushed the feature/irc-transport-wrapper branch 3 times, most recently from 741b0b5 to 8e9d77e Compare October 3, 2016 09:18
} else {
/**
* https://tools.ietf.org/html/draft-butcher-irc-url-04#section-2.4
* Regarding RFC, port 194 is likely to be a more "authentic"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have written: “Regarding the RFC, the recommended port is 194, however…”.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the comment 👍

@Hywan
Copy link
Member

Hywan commented Oct 14, 2016

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

Also allows to define `secure` status of the socket using the wrapper.
@shulard shulard force-pushed the feature/irc-transport-wrapper branch from 8e9d77e to 64e3a96 Compare October 14, 2016 15:06
@Bhoat Bhoat merged commit 64e3a96 into hoaproject:master Oct 14, 2016
@Hywan Hywan removed the in progress label Oct 14, 2016
@Hywan
Copy link
Member

Hywan commented Oct 14, 2016

I had to commit this patch too, 668b622. Nothing important but the API documentation was incorrect.

Thank you very much for your work! The README.md file has been updated too ab380eb.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

3 participants