Setting enable disable ipv6 #1187
Conversation
|
Thanks for Your contribution. This not neccessary solves issue 1169, please remove that tag. Also, we'd like to ask for some improvement. |
| * Const AF_INET6 IPv6 Internet based protocols.<br> | ||
| * null Auto select | ||
| */ | ||
| private $socketProtocolMode = null; |
There was a problem hiding this comment.
that's probably simply socketProtocol? word mode is redundant here
There was a problem hiding this comment.
this is the mode because if you do not install the protocol, automatic will be selected
| */ | ||
| public function setSocketProtocolMode(?int $socketProtocolMode): void | ||
| { | ||
| $this->socketProtocolMode = $socketProtocolMode; |
There was a problem hiding this comment.
This allows to set any integer, while valid values are only 2?
There was a problem hiding this comment.
Yes, function socket_create will give an error "socket_create(): Argument #1 ($domain) must be one of AF_UNIX, AF_INET6, or AF_INET"
PhpAmqpLib/Wire/IO/SocketIO.php
Outdated
| @@ -60,7 +60,7 @@ public function __construct( | |||
| */ | |||
| public function connect() | |||
| { | |||
| $this->sock = socket_create(!$this->isIpv6() ? AF_INET : AF_INET6, SOCK_STREAM, SOL_TCP); | |||
| $this->sock = socket_create($this->selectProtocol(), SOCK_STREAM, SOL_TCP); | |||
PhpAmqpLib/Wire/IO/SocketIO.php
Outdated
| @@ -314,6 +314,15 @@ protected function setErrorHandler(): void | |||
| socket_clear_error($this->sock); | |||
| } | |||
|
|
|||
| private function selectProtocol() :int | |||
| { | |||
| if (is_null($this->config) || is_null($this->config->getSocketProtocolMode())) { | |||
There was a problem hiding this comment.
can we improve readability here?
if ($this->config && $this->config->getSocketProtocol()){
return $this->config->getSocketProtocol();
}
There was a problem hiding this comment.
Why not using a simple flag forcing the AF_INET instead of selecting the version by hand ?
It will probably be easier to understand. What do you think ?
| public function force_connect_ipv4() | ||
| { | ||
| $config = new AMQPConnectionConfig(); | ||
| $config->setSocketProtocolMode(AF_INET); |
There was a problem hiding this comment.
I think if we remove this line, tests won't fail. Seems test case is not testing this feature.
There was a problem hiding this comment.
I'm new to test. Can your help my in wraiting tests?
There was a problem hiding this comment.
I would also add that:
To be run, your two added functions must have the below headers:
/**
* @test
*/
In fact the test will also succeed if you put HOST6 with AF_INET configuration. Or even HOST with AF_INET6.
I am not sure this is a "testable" feature by the way it is done in the code.
No description provided.