Promote warnings to Error in SNMP extension #6124
Conversation
Girgias
force-pushed
the
snmp-warning-to-error
branch
4 times, most recently
from
982b362 to
dcc0b41
Compare
ext/snmp/snmp.c
Outdated
| } else { | ||
| php_error_docref(NULL, E_WARNING, "max_oids should be positive integer or NULL, got " ZEND_LONG_FMT, lval); | ||
| if (lval <= 0) { | ||
| zend_value_error("max_oids must be greater than 0 or NULL"); |
There was a problem hiding this comment.
Is there an existing function instead of zval_get_long() which behaves more consistently with usual ZPP rules? E.g. won't return 0 or 1 in case of an array new_zval, depending on if it is empty or not. :D
otherwise, I would say something like Property SNMP::$max_oids must be greater than 0. AFAIR, null is neither mentioned explicitly in case of nullable parameters in similar situations (e.g. shm_attach()).
There was a problem hiding this comment.
I don't think there is, but NULL is handled explicitly above, and as this is a property hook I have no clue about a better approach for handling the type.
There was a problem hiding this comment.
This looks okay to me, but possibly lowercase null would be more common now?
Girgias
force-pushed
the
snmp-warning-to-error
branch
2 times, most recently
from
8760c06 to
745465a
Compare
ext/snmp/snmp.c
Outdated
| } else { | ||
| php_error_docref(NULL, E_WARNING, "max_oids should be positive integer or NULL, got " ZEND_LONG_FMT, lval); | ||
| if (lval <= 0) { | ||
| zend_value_error("max_oids must be greater than 0 or NULL"); |
There was a problem hiding this comment.
This looks okay to me, but possibly lowercase null would be more common now?
Girgias
force-pushed
the
snmp-warning-to-error
branch
2 times, most recently
from
7831901 to
777a00f
Compare
ext/snmp/tests/snmp2_set.phpt
Outdated
| try { | ||
| $z = snmp2_set($hostname, $communityWrite, $oid1, 's', $newvalue1, $timeout, $retries); | ||
| var_dump($z); | ||
| var_dump((snmpget($hostname, $communityWrite, $oid1, $timeout, $retries) === $newvalue1)); |
There was a problem hiding this comment.
Here again, some of these var_dumps should be outside the try/catch.
Girgias
force-pushed
the
snmp-warning-to-error
branch
from
777a00f to
62c20c6
Compare
No description provided.