Skip to content

ext/standard: validate mode in array_filter() #15647

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jorgsowa
Copy link
Contributor

Validates mode in the function array_filter() and creates complementary ARRAY_FILTER_USE_VALUE without exposing it as a PHP constant.

@bukka
Copy link
Member

bukka commented Sep 2, 2024

I think this might be safer to go through deprecation first.

@bukka
Copy link
Member

bukka commented Sep 2, 2024

Technically it's a BC break as the invalid mode is currently ignored.

@jorgsowa
Copy link
Contributor Author

jorgsowa commented Sep 2, 2024

Makes sense. I will wait for PHP 8.5. I was following a similar change with round() which didn't need RFC: #12252

@cmb69
Copy link
Member

cmb69 commented Oct 30, 2024

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.

@bukka
Copy link
Member

bukka commented Nov 2, 2024

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.

@@ -6500,7 +6500,7 @@ PHP_FUNCTION(array_filter)
zval args[2];
zval retval;
bool have_callback = 0;
zend_long use_type = 0;
zend_long mode = ARRAY_FILTER_USE_VALUE;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please avoid the rename.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think use_type refers to "ARRAY_FILTER_USE_", so mode doesn't seem more descriptive.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -6533,15 +6544,15 @@ PHP_FUNCTION(array_filter)

ZEND_HASH_FOREACH_KEY_VAL(Z_ARRVAL_P(array), num_key, string_key, operand) {
if (have_callback) {
if (use_type) {
if (mode) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to be explicit, you should use mode != ARRAY_FILTER_USE_VALUE, which should end up compiling to the same thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants