parse_str: new flag to merge duplicated params #14713
Conversation
| @@ -114,6 +114,7 @@ PHPAPI void php_register_variable_ex(const char *var_name, zval *val, zval *trac | |||
| size_t var_len, index_len; | |||
| zval gpc_element, *gpc_element_p; | |||
| bool is_array = 0; | |||
| bool merge_params = 0; | |||
There was a problem hiding this comment.
IMO, this code does not belong in php_register_variable_ex.
There was a problem hiding this comment.
Is there another place i should look for the implementation of parse_str ?
There was a problem hiding this comment.
I think we should have specialised function for this as it seems like this use case doesn't exactly fit to the current logic. It's all a bit messy atm.
| ZEND_PARSE_PARAMETERS_END(); | ||
|
|
||
| arrayArg = zend_try_array_init(arrayArg); | ||
| if (!arrayArg) { | ||
| RETURN_THROWS(); | ||
| } | ||
| Z_EXTRA_P(arrayArg) = (uint16_t)mergeParams; |
There was a problem hiding this comment.
I would prefer not using Z_EXTRA_P for non-VM stuff. It's very easy to run into collisions.
There was a problem hiding this comment.
I understand the concern, indeed just setting the extra value anywhere else than for PARSE_STRING related code, things break..
I see 3 other options for passing the merge_params flag:
- either add a new constant in php_variables.h like PARSE_STRING_MERGE
- or add a new argument to treat_data function in SAPI.h (void (*treat_data)(int arg, char *str, zval *destArray, bool mergeParams);)
- create a new treat_data function in SAPI
The first option seems hacky/not really appropriate to me, but with less code change.
Second option might need some refactoring where treat_data gets called.
Latest option would bring lots of code duplication :/
If you have other ideas or a preference for an option above let me know :)
There was a problem hiding this comment.
Considering that it's kind of a different logic, new constant might be more appropriate here.
There was a problem hiding this comment.
As mentioned, this logic does not exactly fit to the current implementation and we might need some refactoring, so it would be good to at least send email to internals to see if there's a general consensus that this functionality is really needed.
If it was few lines of code changes, then I wouldn't worry about it but this is a different situation and it should be also mentioned in the internals email that this functionality will require some refactoring.
| @@ -114,6 +114,7 @@ PHPAPI void php_register_variable_ex(const char *var_name, zval *val, zval *trac | |||
| size_t var_len, index_len; | |||
| zval gpc_element, *gpc_element_p; | |||
| bool is_array = 0; | |||
| bool merge_params = 0; | |||
There was a problem hiding this comment.
I think we should have specialised function for this as it seems like this use case doesn't exactly fit to the current logic. It's all a bit messy atm.
| ZEND_PARSE_PARAMETERS_END(); | ||
|
|
||
| arrayArg = zend_try_array_init(arrayArg); | ||
| if (!arrayArg) { | ||
| RETURN_THROWS(); | ||
| } | ||
| Z_EXTRA_P(arrayArg) = (uint16_t)mergeParams; |
There was a problem hiding this comment.
Considering that it's kind of a different logic, new constant might be more appropriate here.
This is a POC for parse_str to merge params on duplicated keys without brackets
Related issue: #13802
See also: symfony/symfony#54376