ext/standard: validate mode in array_filter() #15647
Conversation
|
I think this might be safer to go through deprecation first. |
|
Technically it's a BC break as the invalid mode is currently ignored. |
|
Makes sense. I will wait for PHP 8.5. I was following a similar change with |
|
A deprecation is certainly more user friendly, but requiring the RFC process for this appears to be overkill. After all, users are not supposed to pass nonsense to well documented functions. |
|
Well just that something users are not supposed to do, does not mean that it's not a BC break. In my opinion it is a BC break and as such we should go through the deprecation. I agree that doing RFC is overkill but I don't think we need RFC if we get some sort of agreement. |
ext/standard/array.c
Outdated
| zval retval; | ||
| bool have_callback = 0; | ||
| zend_long use_type = 0; | ||
| zend_long mode = ARRAY_FILTER_USE_VALUE; |
There was a problem hiding this comment.
Isn't mode a more relevant name than use_type that suggests boolean value?
This only touches internal variable, so I thought that shouldn't be a problem.
There was a problem hiding this comment.
I think use_type refers to "ARRAY_FILTER_USE_", so mode doesn't seem more descriptive.
There was a problem hiding this comment.
mode is the literal name of the argument in the array_filter function.
https://www.php.net/manual/en/function.array-filter.php
For me, it doesn't make a big difference, rather than alignment, so I can change it tomorrow.
There was a problem hiding this comment.
I reverted the name change
ext/standard/array.c
Outdated
| ZEND_HASH_FOREACH_KEY_VAL(Z_ARRVAL_P(array), num_key, string_key, operand) { | ||
| if (have_callback) { | ||
| if (use_type) { | ||
| if (mode) { |
There was a problem hiding this comment.
If you want to be explicit, you should use mode != ARRAY_FILTER_USE_VALUE, which should end up compiling to the same thing.
jorgsowa
force-pushed
the
check-mode-in-array-filter
branch
from
9245732 to
2111c36
Compare
iluuu1994
left a comment
iluuu1994
left a comment
There was a problem hiding this comment.
LGTM. I wouldn't object to this in a minor version, but @bukka should confirm whether he does. If so, you can just change the error to a deprecation. According to the policy:
https://github.com/php/policies/blob/main/release-process.rst#feature-selection-and-development
However, any change that breaks user-facing backward compatibility MUST go through the RFC process.
From a pedantic PoV, this is breaking. In that case, this would even need an entry in the deprecation RFC.
| RETVAL_EMPTY_ARRAY(); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Nit: Please avoid unrelated whitespace changes.
4 participants
Validates
modein the functionarray_filter()and creates complementaryARRAY_FILTER_USE_VALUEwithout exposing it as a PHP constant.