Promote warnings to Errors in sockets's extension. by Girgias · Pull Request #5075 · php/php-src

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

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

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).

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

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.

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

Indentation

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

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 c3c5bf0Compare 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 650aa93Compare 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 9d7c7f4Compare February 4, 2020 22:18

Girgias force-pushed the sockets-warnings-to-errors branch from 9d7c7f4 to a336c3cCompare 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 33dbfa2Compare February 26, 2020 02:21

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

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

Girgias force-pushed the sockets-warnings-to-errors branch 2 times, most recently from 9eb8684 to 8a1a9eaCompare 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 421fb30Compare April 18, 2020 12:34

nikic reviewed Apr 21, 2020
View reviewed changes

Comment threadext/sockets/sockets.c Outdated
nikicApr 21, 2020
Copy link
Copy Markdown
Member
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 2869ccbCompare 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 7ff8eaaCompare April 21, 2020 18:41

php-pulls merged commit 7ff8eaa into php:masterApr 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

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants