Promote warnings to Errors in sockets's extension. #5075

Girgias · 2020-01-10T21:28:18Z

There are a bunch of expected array warnings, a better way is too probably ask for a HashTable param instead of a zval.

nikic · 2020-02-04T11:41:31Z

ext/sockets/conversions.c

@@ -241,26 +241,26 @@ static void from_zval_write_aggregation(const zval *container,
 	zval					*elem;

 	if (Z_TYPE_P(container) != IS_ARRAY) {
-		do_from_zval_err(ctx, "%s", "expected an array here");
+		zend_type_error("Expected array");
+		return;
 	}

 	for (descr = descriptors; descr->name != NULL && !ctx->err.has_error; descr++) {
 		if ((elem = zend_hash_str_find(Z_ARRVAL_P(container),
 				descr->name, descr->name_size - 1)) != NULL) {

 			if (descr->from_zval == NULL) {


Convert this to ZEND_ASSERT(descr->from_zval), doesn't seem like it can be hit.

It looks like you converted the wrong condition to an assert here...

Uhhhh, what was I thinking >_>

nikic · 2020-02-04T11:44:51Z

ext/sockets/conversions.c

@@ -365,8 +363,7 @@ void from_zval_write_int(const zval *arr_value, char *field, ser_context *ctx)
 	}


These has_error checks are no longer going to work after some of your changes.

Girgias · 2020-02-27T00:00:09Z

Test failures are FPM related ...

Girgias · 2020-04-17T16:56:21Z

I ignored a large part of warnings in multicast.c currently. Will maybe tackle them later or in a separate PR.

Girgias · 2020-04-17T17:44:25Z

@kocsismate I've updated most of the errors to use the new API, mind checking that I'm using the "standard" error format everywhere?

kocsismate

I had a first look :) If you address the comments, please check for duplicates, because I saw that most of the messages are repeated (and I didn't comment everywhere).

ext/sockets/conversions.c

ext/sockets/sendrecvmsg.c

ext/sockets/sockets.c

kocsismate · 2020-04-18T11:27:29Z

ext/sockets/sockets.c

-				php_error_docref(NULL, E_WARNING, "Socket of type AF_INET6 requires 3 arguments");
-				RETURN_FALSE;
+			if (ZEND_NUM_ARGS() != 3) {
+				zend_argument_count_error("Socket of type AF_INET6 requires port to be specified");


I used an argument_value_error with a message similar to must be specified for the AF_INET6 socket type in a similar situation

Hum, maybe? It feels like a count error is more appropriate as there is a missing argument, but could change it to a ValueError.

My reasoning is that because this parameter is optional, and since it should be nullable in reality (and it will be when I start fixing optional params with UNKNOWN default values in stubs), we should think about this error that the default value is not appropriate in case of the aforementioned socket type. At least this is what I think 😊

Agree with @kocsismate.

ext/sockets/sockets.c

nikic · 2020-04-21T12:40:37Z

ext/sockets/sockets.c

 			close(php_sock->bsd_socket);
-			efree(php_sock);
-			RETURN_FALSE;
+        	efree(php_sock);


Indentation

nikic · 2020-04-21T12:43:15Z

ext/sockets/tests/socket_set_option_in6_pktinfo.phpt

+    var_dump(socket_set_option($s, IPPROTO_IPV6, IPV6_PKTINFO, []));
+} catch (\ValueError $e) {
+    echo $e->getMessage() . \PHP_EOL;
+}


Added try but no change in output?

I'll drop it, as I'm using WSL, I get test failures due to the protocols not being available so I've added it to catch the failure on CI, but seems like there is none.

Ah no I remember why, this was when I still had the changes in conversion.c which I decided to drop

nikic · 2020-04-21T12:44:41Z

Looks fine to me apart from some nits.

Girgias force-pushed the sockets-warnings-to-errors branch from 82e81d3 to c3c5bf0 Compare January 11, 2020 00:16

Girgias marked this pull request as ready for review January 11, 2020 03:32

Girgias force-pushed the sockets-warnings-to-errors branch from f88a4f8 to 650aa93 Compare January 24, 2020 12:02

Girgias requested a review from nikic January 27, 2020 11:45

kocsismate added Warning promotion Waiting on Review labels Jan 28, 2020

nikic reviewed Feb 4, 2020

View reviewed changes

Girgias force-pushed the sockets-warnings-to-errors branch from 650aa93 to 9d7c7f4 Compare February 4, 2020 22:18

Girgias force-pushed the sockets-warnings-to-errors branch from 9d7c7f4 to a336c3c Compare February 25, 2020 11:00

Girgias requested a review from nikic February 25, 2020 11:00

Girgias force-pushed the sockets-warnings-to-errors branch from a336c3c to 33dbfa2 Compare February 26, 2020 02:21

Girgias force-pushed the sockets-warnings-to-errors branch 2 times, most recently from 3bffe06 to 5e745c7 Compare March 27, 2020 01:32

Girgias force-pushed the sockets-warnings-to-errors branch from 5e745c7 to 99045d8 Compare April 5, 2020 19:44

Girgias force-pushed the sockets-warnings-to-errors branch 2 times, most recently from 9eb8684 to 8a1a9ea Compare April 17, 2020 16:41

kocsismate reviewed Apr 18, 2020

View reviewed changes

Girgias force-pushed the sockets-warnings-to-errors branch from 8a1a9ea to 421fb30 Compare April 18, 2020 12:34

nikic reviewed Apr 21, 2020

View reviewed changes

ext/sockets/sockets.c Outdated

close(php_sock->bsd_socket);

efree(php_sock);

RETURN_FALSE;

efree(php_sock);

Copy link

Member

nikic Apr 21, 2020

Choose a reason for hiding this comment

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

Indentation

nikic reviewed Apr 21, 2020

View reviewed changes

kocsismate removed the Waiting on Review label Apr 21, 2020

Girgias force-pushed the sockets-warnings-to-errors branch from 1b6af4a to 2869ccb Compare April 21, 2020 18:07

Promote warnings to Errors in sockets's extension.

7ff8eaa

Girgias force-pushed the sockets-warnings-to-errors branch from 2869ccb to 7ff8eaa Compare April 21, 2020 18:41

php-pulls merged commit 7ff8eaa into php:master Apr 21, 2020

Girgias deleted the sockets-warnings-to-errors branch April 21, 2020 21:42

Girgias mentioned this pull request Aug 13, 2020

Warning to Error (TypeError/ValueError) in extensions php/php-tasks#18

Closed

71 tasks

		@@ -365,8 +363,7 @@ void from_zval_write_int(const zval arr_value, char field, ser_context *ctx)
		}

Promote warnings to Errors in sockets's extension. #5075

Promote warnings to Errors in sockets's extension. #5075

Uh oh!

Conversation

Girgias commented Jan 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Girgias commented Feb 27, 2020

Uh oh!

Girgias commented Apr 17, 2020

Uh oh!

Girgias commented Apr 17, 2020

Uh oh!

kocsismate left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikic commented Apr 21, 2020

Uh oh!

Uh oh!