PHPC-2457 Fix modifiers and other Query options by reference #1694
Conversation
| @@ -333,6 +334,7 @@ bool phongo_query_init(zval* return_value, zval* filter, zval* options) | |||
| if (php_array_existsc(options, "modifiers")) { | |||
| modifiers = php_array_fetchc(options, "modifiers"); | |||
|
|
|||
| ZVAL_DEREF(modifiers); | |||
There was a problem hiding this comment.
Should be done by php_array_fetchc?
| zval *php_array_fetch(zval *zarr, const char *key) { | ||
| return php_array_fetchl(zarr, key, strlen(key)); | ||
| zval *ret = php_array_fetchl(zarr, key, strlen(key)); | ||
| if (ret) { | ||
| ZVAL_DEREF(ret); | ||
| } | ||
|
|
||
| return ret; |
There was a problem hiding this comment.
This is used to read options in Query, BulkWrite and phongo_lient.
There was a problem hiding this comment.
Note that there are a whole family of php_array_fetch functions, so even this change might be incomplete (in terms of fixing the general issue and not just the regression tests).
In this Slack thread, @alcaeus suggested a new method that conditionally applies ZVAL_DEREF(). That sounded reasonable, along with leaving this as-is and just adding our own ZVAL_DEREF() calls as needed.
As for the php_array_zval_to_<type> family of functions, I think those are easy candidates for automatic dereferencing, as @alcaeus did in previous PRs.
There was a problem hiding this comment.
Created #1697 to update php_array_api and fix the deref issue in remaining places.
| --FILE-- | ||
| <?php | ||
|
|
||
| $modifiers = ['$orderby' => ['x' => 1]]; |
There was a problem hiding this comment.
You noted the following error in PHPC-2457:
PHP Fatal error: Uncaught MongoDB\Driver\Exception\InvalidArgumentException: Expected "$orderby" modifier to be array or object, bool given
The exception originates from php_phongo_query_opts_append_document(), but I'm not sure how that's possible as the code calls PHONGO_ZVAL_CLASS_OR_TYPE_NAME_P(value). That macro delegates to zend_get_type_by_const(). IS_REFERENCE isn't even included in that function, and I can't imagine why the type would match for the function returning "bool".
| zval *php_array_fetch(zval *zarr, const char *key) { | ||
| return php_array_fetchl(zarr, key, strlen(key)); | ||
| zval *ret = php_array_fetchl(zarr, key, strlen(key)); | ||
| if (ret) { | ||
| ZVAL_DEREF(ret); | ||
| } | ||
|
|
||
| return ret; |
There was a problem hiding this comment.
Note that there are a whole family of php_array_fetch functions, so even this change might be incomplete (in terms of fixing the general issue and not just the regression tests).
In this Slack thread, @alcaeus suggested a new method that conditionally applies ZVAL_DEREF(). That sounded reasonable, along with leaving this as-is and just adding our own ZVAL_DEREF() calls as needed.
As for the php_array_zval_to_<type> family of functions, I think those are easy candidates for automatic dereferencing, as @alcaeus did in previous PRs.
|
Replaced by #1697 |
Fix PHPC-2457
Similar to #1683.