ext/session: session_start() options arguments type checks. #17388
Conversation
ext/session/session.c
Outdated
| @@ -2667,7 +2672,8 @@ PHP_FUNCTION(session_start) | |||
| case IS_FALSE: | |||
| case IS_LONG: | |||
| if (zend_string_equals_literal(str_idx, "read_and_close")) { | |||
| read_and_close = zval_get_long(value); | |||
| zend_long tmp = zval_get_long_ex(value, true); | |||
There was a problem hiding this comment.
let me know if you think zval_try_get_long is more appropriate, so we forgo "true"/"false" for example.
There was a problem hiding this comment.
I think zval_get_long() here is fine, except for the string case, probably best to handle this manually with one of the numeric string APIs.
| @@ -26,7 +26,7 @@ foreach ($valuesDisablingReadAndClose as $value) { | |||
|
|
|||
| try { | |||
| session_start(["read_and_close" => 1.0]); | |||
| } catch (Throwable $e) { | |||
| } catch (TypeError $e) { | |||
There was a problem hiding this comment.
Because with the change I find it vague now we throw an explicit TypeError but ok with your other suggestions.
ext/session/session.c
Outdated
| @@ -2659,6 +2659,11 @@ PHP_FUNCTION(session_start) | |||
|
|
|||
| /* set options */ | |||
| if (options) { | |||
| if (UNEXPECTED(HT_IS_PACKED(Z_ARRVAL_P(options)))) { | |||
| zend_argument_type_error(1, "must be of type array with keys as string"); | |||
There was a problem hiding this comment.
This seems more like a ValueError than a TypeError? As we are correctly passing an array.
Moreover, you are not checking this condition in the foreach loop (e.g. an array of the shape ["key" => "val1", "val2"] would not error. As such the HT_IS_PACKED(Z_ARRVAL_P(options)) pre-check seems a bit pointless? Just integrate it into the foreach loop (and change to FOREACH_STR_KEY_VAL as well).
ext/session/session.c
Outdated
| @@ -2667,7 +2672,8 @@ PHP_FUNCTION(session_start) | |||
| case IS_FALSE: | |||
| case IS_LONG: | |||
| if (zend_string_equals_literal(str_idx, "read_and_close")) { | |||
| read_and_close = zval_get_long(value); | |||
| zend_long tmp = zval_get_long_ex(value, true); | |||
There was a problem hiding this comment.
I think zval_get_long() here is fine, except for the string case, probably best to handle this manually with one of the numeric string APIs.
ext/session/session.c
Outdated
| if (str_idx) { | ||
| switch(Z_TYPE_P(value)) { |
There was a problem hiding this comment.
Maybe swap the conditions around and do if (UNEXPECTED(str_index == NULL)) { /* throw error */ } and then you don't need to indent the happy path.
| } catch (ValueError $exception) { | ||
| echo $exception->getMessage() . "\n"; | ||
| } |
There was a problem hiding this comment.
| } catch (ValueError $exception) { | |
| echo $exception->getMessage() . "\n"; | |
| } | |
| } catch (ValueError $e) { | |
| echo $e::class, ': ', $e->getMessage(), PHP_EOL; | |
| } |
devnexen
force-pushed
the
session_start_upd
branch
from
3b798a9 to
d2c7052
Compare
|
Oh very nice, thanks @devnexen! :) |
No description provided.