Skip to content
This repository has been archived by the owner on Dec 16, 2019. It is now read-only.

Rework of sockets to mimic php-src sockets API #839

Merged
merged 2 commits into from Feb 22, 2018
Merged

Rework of sockets to mimic php-src sockets API #839

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
26eace5
Rework of sockets to mimic php-src sockets API
sirsnyder Feb 22, 2018
47423ce
Merged branch master into rework_sockets
sirsnyder Feb 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 11 additions & 12 deletions classes/socket.h
View file
Open in desktop
Expand Up @@ -114,11 +114,11 @@ ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(Socket_getHost, 0, 0, IS_ARRAY, 0)
ZEND_ARG_TYPE_INFO(0, port, _IS_BOOL, 0)
ZEND_END_ARG_INFO()

ZEND_BEGIN_ARG_INFO_EX(Socket_select, 0, 0, 3)
ZEND_BEGIN_ARG_INFO_EX(Socket_select, 0, 0, 4)
ZEND_ARG_TYPE_INFO(1, read, IS_ARRAY, 1)
ZEND_ARG_TYPE_INFO(1, write, IS_ARRAY, 1)
ZEND_ARG_TYPE_INFO(1, except, IS_ARRAY, 1)
ZEND_ARG_TYPE_INFO(0, sec, IS_LONG, 0)
ZEND_ARG_TYPE_INFO(0, sec, IS_LONG, 1)
ZEND_ARG_TYPE_INFO(0, usec, IS_LONG, 0)
ZEND_ARG_TYPE_INFO(1, error, IS_LONG, 1)
ZEND_END_ARG_INFO()
Expand Down Expand Up @@ -183,7 +183,7 @@ PHP_METHOD(Socket, setOption) {
zend_long value = 0;

if (zend_parse_parameters(ZEND_NUM_ARGS(), "lll", &level, &name, &value) != SUCCESS) {
return;
RETURN_FALSE;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand why you've changed this to return false, but conventionally, when the ZPP API fails to parse the input, it will return null (this null return is usually ignored in the docs, since only the correct usage is documented). The socket_set_option function does the same internally. This leads to the following output:

tpunt:~ tpunt$ php -nr 'var_dump(socket_set_option());'

Warning: socket_set_option() expects exactly 4 parameters, 0 given in Command line code on line 1
NULL

For parity with other internal functions, it would likely be best to keep these as returning null, too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

socket_set_option has no explicit return type defined by ZEND_BEGIN_ARG_INFO_EX, but Socket::setOption uses ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX. Without return false, this leads to the following output:

$socket = new Socket(\Socket::AF_INET, \Socket::SOCK_STREAM, \Socket::SOL_TCP); $socket->getOption();

Warning: Socket::getOption() expects exactly 2 parameters, 0 given in ...

Fatal error: Return value of Socket::getOption() must be of the type integer, null returned in Unknown on line 0

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I do see the problem. This is also why the return type information macros are not particularly pervasive in php-src. I'm not so concerned what solution is chosen here (current solution looks fine).

}

pthreads_socket_set_option(getThis(), level, name, value, return_value);
Expand All @@ -196,7 +196,7 @@ PHP_METHOD(Socket, getOption) {
zend_long name = 0;

if (zend_parse_parameters(ZEND_NUM_ARGS(), "ll", &level, &name) != SUCCESS) {
return;
RETURN_LONG(0);
}

pthreads_socket_get_option(getThis(), level, name, return_value);
Expand All @@ -208,17 +208,17 @@ PHP_METHOD(Socket, bind) {
zend_long port = 0;

if (zend_parse_parameters(ZEND_NUM_ARGS(), "S|l", &host, &port) != SUCCESS) {
return;
RETURN_FALSE;
}

pthreads_socket_bind(getThis(), host, port, return_value);
} /* }}} */

/* {{{ proto bool Socket::listen(int backlog) */
/* {{{ proto bool Socket::listen([int backlog = 0]) */
PHP_METHOD(Socket, listen) {
zend_long backlog = 0;

if (zend_parse_parameters(ZEND_NUM_ARGS(), "l", &backlog) != SUCCESS) {
if (zend_parse_parameters(ZEND_NUM_ARGS(), "|l", &backlog) != SUCCESS) {
return;
}

Expand Down Expand Up @@ -249,13 +249,12 @@ PHP_METHOD(Socket, connect) {
pthreads_socket_connect(getThis(), argc, host, port, return_value);
} /* }}} */

/* {{{ proto int|bool Socket::select(array &read, array &write, array &except [, int sec = 0 [, int usec = 0 [, int &error]]]) */
/* {{{ proto int|bool Socket::select(array &read, array &write, array &except, int sec [, int usec = 0 [, int &error]]) */
PHP_METHOD(Socket, select) {
zval *read, *write, *except, *errorno = NULL;
zend_long sec = 0;
zval *read, *write, *except, *sec, *errorno = NULL;
zend_long usec = 0;

if (zend_parse_parameters(ZEND_NUM_ARGS(), "a/!a/!a/!|llz/", &read, &write, &except, &sec, &usec, &errorno) != SUCCESS) {
if (zend_parse_parameters(ZEND_NUM_ARGS(), "a/!a/!a/!z!|lz/", &read, &write, &except, &sec, &usec, &errorno) != SUCCESS) {
return;
}
pthreads_socket_select(read, write, except, sec, usec, errorno, return_value);
Expand Down Expand Up @@ -333,7 +332,7 @@ PHP_METHOD(Socket, setBlocking) {
zend_bool blocking = 0;

if (zend_parse_parameters(ZEND_NUM_ARGS(), "b", &blocking) != SUCCESS) {
return;
RETURN_FALSE;
}

pthreads_socket_set_blocking(getThis(), blocking, return_value);
Expand Down
4 changes: 2 additions & 2 deletions examples/stub.php
View file
Open in desktop
Expand Up @@ -662,13 +662,13 @@ public function getOption(int $level, int $name) : int{}

public function bind(string $host, int $port = 0) : bool{}

public function listen(int $backlog) : bool{}
public function listen(int $backlog = 0) : bool{}

public function accept($class = self::class){}

public function connect(string $host, int $port = 0) : bool{}

public static function select(array &$read, array &$write, array &$except, int $sec = 0, int $usec = 0, int &$error = null){}
public static function select(array &$read, array &$write, array &$except, int $sec, int $usec = 0, int &$error = null){}
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be ?int $sec

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, do you see any other issues?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nothing off the hop, this is the only thing I noticed.


public function read(int $length, int $flags = 0){}

Expand Down