Skip to content

add ability to use array keys with array_filter() #287

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Oct 30, 2013
Merged

add ability to use array keys with array_filter() #287

merged 5 commits into from
Oct 30, 2013

Conversation

datibbaw
Copy link
Contributor

This adds a third (optional) argument to array_filter() that will determine what gets passed to the callback, the array key, value or both.

    array_filter(array(1 => 'a', 2 => 'b', a' => 1, 'b' => 2), 'is_numeric', ARRAY_FILTER_USE_KEY);
    // returns: [1 => 'a', 2 => 'b']

    array_filter(array(1 => 'a', 2 => 'b', a' => 1, 'b' => 2), 'is_numeric');
    // returns: ['a' => 1, 'b' => 2]

    array_filter(array(1 => 'a', 2 => 'b', a' => 1, 'b' => 2), function($value, $key) {
        return is_numeric($key) && $value == 'b';
    }, ARRAY_FILTER_USE_BOTH);
    // returns: [2 => 'b']

@davidkuridza
Copy link

+1, it would really simplify my life :)

shouldn't the example provided return [5, 6, 7]?

@datibbaw
Copy link
Contributor Author

@davidkuridza That's if array keys were 1-indexed :)

@davidkuridza
Copy link

of course, stupid me :)

@weltling
Copy link
Contributor

Tested on windows, it's ok (except see the comment about the test).

Another thing - actually the third arg isn't needed, you could just pass two args to the func by default. If the filter callback has only one arg in the signature, so the second is just ignored. That means the older code is still compatible while no need to add the third arg to the array_filter signature.

@datibbaw
Copy link
Contributor Author

@weltling I've discussed dropping the third arg on the mailing list; basically, functions with optional arguments may start to break if we always send the key.

@weltling
Copy link
Contributor

True, passing two args unconditionally is a BC breach, sigh! The test is ok now, also all the other array_filter_* tests pass. Good work!

@datibbaw
Copy link
Contributor Author

@weltling Great! I'm a bit new with this, do I close the pull request?

@weltling
Copy link
Contributor

May be someone else will take a look yet. Please wait until it's merged then. The person who merges it will close it.

@dsp
Copy link
Member

dsp commented Sep 16, 2013

Thanks for your patience. Things aren't always going as fast as hoped for :/.

This thread is stalled atm :/. Can you resend a list to the ML and ping me back in 2 weeks? If there are no objections, I would like to have that change and I don't think an RFC is necessary (even though people might disagree), but I would like to see it being discussed before merging it into master.

@hikari-no-yume
Copy link
Contributor

Can we have the bitmask option? I like it best.

@srgoogleguy
Copy link
Contributor

Merged to master, you can close this now

@hikari-no-yume
Copy link
Contributor

@srgoogleguy It looks to me as if there's a piece missing. It doesn't seem to actually define ARRAY_FILTER_USE_VALUE as a constant, it's just a fake constant as a default. That seems strange. Even if specifying it explicitly is unnecessary, it should exist as a proper constant.

@php-pulls php-pulls merged commit 75ba75e into php:master Oct 30, 2013
@datibbaw datibbaw deleted the array_filter_with_keys branch October 30, 2013 23:47
@srgoogleguy
Copy link
Contributor

@TazeTSchnitzel If you followed the discussion on Internals you should know that the implementation has changed to not define an ARRAY_FILTER_USE_VALUE constant, since this is the default behavior anyway. It doesn't make much sense to define a constant for behavior the function does regardless of the argument supplied.

See http://news.php.net/php.internals/69341

You were probably looking at https://github.com/php/php-src/pull/287/files#diff-9a4220f442d2acbb4519beb4481d13e9R5 which mentions the constant name in the test file comment. Admittedly this may confuse a few folks. Perhaps we should delete that comment as not to confuse anyone for now.

@datibbaw
Copy link
Contributor Author

And this is why I don't like documenting my code ;-)

@dunglas dunglas mentioned this pull request Sep 17, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@datibbaw @davidkuridza @weltling @dsp @hikari-no-yume @srgoogleguy @php-pulls