I'm starting up a thread for discussion on Pull Request 287
https://github.com/php/php-src/pull/287 (allowing array keys to be passed
to the callback function of array_filter through a third optional boolean
argument). I would like to merge this into master and as discussed on IRC
it would probably be a good idea to startup a discussion and make sure
there aren't any objections or clarifications not yet voiced.
The patch has no BC because the third argument is optional and defaults to
false. Personally, I have always thought it would be a good idea to be able
to get the keys into the array_filter callback since I've stumbled across a
few scenarios where that would have made things easier.
I'm not sure if there are any particular down sides to this option being
added, but none that I can find. It currently passes all tests in master
and works as expected.
Thoughts, opinions, objections, concerns?
- Sherif
I'm starting up a thread for discussion on Pull Request 287
https://github.com/php/php-src/pull/287 (allowing array keys to be passed
to the callback function of array_filter through a third optional boolean
argument). I would like to merge this into master and as discussed on IRC
it would probably be a good idea to startup a discussion and make sure
there aren't any objections or clarifications not yet voiced.The patch has no BC because the third argument is optional and defaults to
false. Personally, I have always thought it would be a good idea to be able
to get the keys into the array_filter callback since I've stumbled across a
few scenarios where that would have made things easier.I'm not sure if there are any particular down sides to this option being
added, but none that I can find. It currently passes all tests in master
and works as expected.Thoughts, opinions, objections, concerns?
It might be considered a BC break, but I really think we should drop the
boolean argument; just have it pass the key as parameter 2 always.
On Tue, Jun 18, 2013 at 10:58 AM, Levi Morrison morrison.levi@gmail.comwrote:
I'm starting up a thread for discussion on Pull Request 287
https://github.com/php/php-src/pull/287 (allowing array keys to be passed
to the callback function of array_filter through a third optional boolean
argument). I would like to merge this into master and as discussed on IRC
it would probably be a good idea to startup a discussion and make sure
there aren't any objections or clarifications not yet voiced.The patch has no BC because the third argument is optional and defaults to
false. Personally, I have always thought it would be a good idea to be
able
to get the keys into the array_filter callback since I've stumbled across
a
few scenarios where that would have made things easier.I'm not sure if there are any particular down sides to this option being
added, but none that I can find. It currently passes all tests in master
and works as expected.Thoughts, opinions, objections, concerns?
It might be considered a BC break, but I really think we should drop the
boolean argument; just have it pass the key as parameter 2 always.
See the discussion on github for that PR
https://github.com/php/php-src/pull/287#issuecomment-14175109 unfortunately
we can't do that as it will break lots of userspace code that might be
doing stuff like array_filter(['foo','','bar'], 'strlen') where strlen only
accepts a single argument and in those cases the result will be triggering
lots of warnings and failed code.
On Tue, Jun 18, 2013 at 9:07 AM, Sherif Ramadan theanomaly.is@gmail.comwrote:
On Tue, Jun 18, 2013 at 10:58 AM, Levi Morrison morrison.levi@gmail.comwrote:
I'm starting up a thread for discussion on Pull Request 287
https://github.com/php/php-src/pull/287 (allowing array keys to be
passed
to the callback function of array_filter through a third optional boolean
argument). I would like to merge this into master and as discussed on IRC
it would probably be a good idea to startup a discussion and make sure
there aren't any objections or clarifications not yet voiced.The patch has no BC because the third argument is optional and defaults
to
false. Personally, I have always thought it would be a good idea to be
able
to get the keys into the array_filter callback since I've stumbled
across a
few scenarios where that would have made things easier.I'm not sure if there are any particular down sides to this option being
added, but none that I can find. It currently passes all tests in master
and works as expected.Thoughts, opinions, objections, concerns?
It might be considered a BC break, but I really think we should drop the
boolean argument; just have it pass the key as parameter 2 always.See the discussion on github for that PR
https://github.com/php/php-src/pull/287#issuecomment-14175109 unfortunately
we can't do that as it will break lots of userspace code that might be
doing stuff like array_filter(['foo','','bar'], 'strlen') where strlen only
accepts a single argument and in those cases the result will be triggering
lots of warnings and failed code.
If strlen
can't handle extra parameters then I'd say that is a real
problem, this PR aside.
On Tue, Jun 18, 2013 at 11:28 AM, Levi Morrison morrison.levi@gmail.comwrote:
If
strlen
can't handle extra parameters then I'd say that is a real
problem, this PR aside.
I'm not sure how that's a real problem. All built in PHP functions specify
a signature.
On Tue, Jun 18, 2013 at 11:07 PM, Sherif Ramadan theanomaly.is@gmail.comwrote:
On Tue, Jun 18, 2013 at 10:58 AM, Levi Morrison <morrison.levi@gmail.com
wrote:
I'm starting up a thread for discussion on Pull Request 287
https://github.com/php/php-src/pull/287 (allowing array keys to be
passed
to the callback function of array_filter through a third optional
boolean
argument). I would like to merge this into master and as discussed on
IRC
it would probably be a good idea to startup a discussion and make sure
there aren't any objections or clarifications not yet voiced.The patch has no BC because the third argument is optional and defaults
to
false. Personally, I have always thought it would be a good idea to be
able
to get the keys into the array_filter callback since I've stumbled
across
a
few scenarios where that would have made things easier.I'm not sure if there are any particular down sides to this option being
added, but none that I can find. It currently passes all tests in master
and works as expected.Thoughts, opinions, objections, concerns?
It might be considered a BC break, but I really think we should drop the
boolean argument; just have it pass the key as parameter 2 always.See the discussion on github for that PR
https://github.com/php/php-src/pull/287#issuecomment-14175109unfortunately
we can't do that as it will break lots of userspace code that might be
doing stuff like array_filter(['foo','','bar'], 'strlen') where strlen only
accepts a single argument and in those cases the result will be triggering
lots of warnings and failed code.
The danger actually lurks in the cases whereby the second argument is
accepted and with that (sometimes radically) change the semantics of the
function; in fact, quite a few internal functions have this kind of
switch-by-argument behaviour, such as "array_keys" to randomly name one.
And admittedly, this function when my PR goes through ;-)
--
Tjerk
On Tue, Jun 18, 2013 at 11:37 AM, Tjerk Anne Meesters datibbaw@php.netwrote:
The danger actually lurks in the cases whereby the second argument is
accepted and with that (sometimes radically) change the semantics of the
function; in fact, quite a few internal functions have this kind of
switch-by-argument behaviour, such as "array_keys" to randomly name one.
And admittedly, this function when my PR goes through ;-)
That is a worse outcome, sure, but there is still danger in the former
scenario given that defaulting the callback to take two arguments causes
the wrong result:
var_dump(array_filter(['foo', '', 'bar'], 'strlen', true));
Warning: strlen()
expects exactly 1 parameter, 2 given in - on line 1
Warning: strlen()
expects exactly 1 parameter, 2 given in - on line 1
Warning: strlen()
expects exactly 1 parameter, 2 given in - on line 1
array(0) {
}
Not only do we trigger the error handler (a large enough array causes a
performance issue), but we also get back an empty array as a result. That's
BC and performance loss that's simply unacceptable, so defaulting the
behavior to pass the key is simply out of the question.
--
Tjerk
var_dump(array_filter(['foo', '', 'bar'], 'strlen', true));
Warning:
strlen()
expects exactly 1 parameter, 2 given in - on line 1Not only do we trigger the error handler (a large enough array causes a
performance issue), but we also get back an empty array as a result.
That's
BC and performance loss that's simply unacceptable, so defaulting the
behavior to pass the key is simply out of the question.
Existing code won't pass that third, true, argument to array_filter()
, and
thus will not break.
Nevertheless, I'd say go with a new array_filter_key() function, because
that permits using existing one-parameter functions like that strlen as a
callback, instead of forcing two-parameter callback functions.
Another alternative might be to go with a third argument to array_filter()
,
but make that an integer with ORable constants ARRAY_FILTER_KEY,
ARRAY_FILTER_VAL - only one of them set calls with a single argument, both
set call with two arguments.
best regards
Patrick
Another alternative might be to go with a third argument to
array_filter()
,
but make that an integer with ORable constants ARRAY_FILTER_KEY,
ARRAY_FILTER_VAL - only one of them set calls with a single argument, both
set call with two arguments.
That look like a fairly decent compromise of all the suggestions so far.
var_dump(array_filter(['foo', '', 'bar'], 'strlen', true));
Warning:
strlen()
expects exactly 1 parameter, 2 given in - on line 1Not only do we trigger the error handler (a large enough array causes a
performance issue), but we also get back an empty array as a result.
That's
BC and performance loss that's simply unacceptable, so defaulting the
behavior to pass the key is simply out of the question.Existing code won't pass that third, true, argument to
array_filter()
, and
thus will not break.
Of course not, the premise was that this would be the result if there was
no third argument and the function would pass the key to the callback by
default (in response to the comment above about removing the third
argument).
Nevertheless, I'd say go with a new array_filter_key() function, because
that permits using existing one-parameter functions like that strlen as a
callback, instead of forcing two-parameter callback functions.
The current patch does not force the callback to take two arguments. By
default the existing behavior is maintained since the third argument is
false by default. I still don't hear a good argument for adding a new
function.
Another alternative might be to go with a third argument to
array_filter()
, but make that an integer with ORable constants
ARRAY_FILTER_KEY, ARRAY_FILTER_VAL - only one of them set calls with a
single argument, both set call with two arguments.
I would agree that this is a better solution than the existing patch allows
since the function name does not suggest the filtering of a key or
otherwise. In that case it would be easy to use bit-wise operators to get
both the key and the element into the callback or just one or the other.
The default would be ARRAY_FILTER_VAL, obviously.
I suppose the order of the elements should remain $value, $key in the event
of ARRAY_FILTER_KEY | ARRAY_FILTER_VAL.
best regards
Patrick
I suppose the order of the elements should remain $value, $key in the event
of ARRAY_FILTER_KEY | ARRAY_FILTER_VAL.
This is also the order already used for the callback of array_walk, so I
think it makes sense to have some consistency. (Even if I never liked the
order of the array_walk callback :))
Nevertheless, I'd say go with a new array_filter_key() function, because
that permits using existing one-parameter functions like that strlen as a
callback, instead of forcing two-parameter callback functions.The current patch does not force the callback to take two arguments. By
default the existing behavior is maintained since the third argument is
false by default. I still don't hear a good argument for adding a new
function.
I'm very much on the "new function" side of the fence. Basic (one-word)
arguments for this include familiarity and simplicity. Familiarity, since
we're used to *_key functions for arrays. It will come as absolutely no
surprise to see the _key function spring up out of nowhere and its use is
blindingly obvious for anyone who has ever used array_filter()
. Simplicity
since, err.. what were those flags again? is it $key first or $value? is
there an ARRAY_FILTER_BOTH? Also, I don't pretend my time with PHP covers
everything that everyone has ever done or wanted to do, but I cannot think
of a time when I wanted to filter an array on both the key and the value
at the same time: of course, it's easy to make up use cases.
On Tue, Jun 18, 2013 at 6:08 PM, Peter Cowburn petercowburn@gmail.comwrote:
Nevertheless, I'd say go with a new array_filter_key() function, because
that permits using existing one-parameter functions like that strlen as
a
callback, instead of forcing two-parameter callback functions.The current patch does not force the callback to take two arguments. By
default the existing behavior is maintained since the third argument is
false by default. I still don't hear a good argument for adding a new
function.I'm very much on the "new function" side of the fence. Basic (one-word)
arguments for this include familiarity and simplicity.
Are you saying that it's better to have familiarity over functionality?
I'm not sure I agree with this premise of we shouldn't provide
better functionality just because it would make all the other poorly
written functions look bad. That's setting a lower bar of standards, isn't
it?
Familiarity, since we're used to *_key functions for arrays. It will come
as absolutely no surprise to see the _key function spring up out of nowhere
and its use is blindingly obvious for anyone who has ever used
array_filter()
. Simplicity since, err.. what were those flags again? is it
$key first or $value? is there an ARRAY_FILTER_BOTH? Also, I don't pretend
my time with PHP covers everything that everyone has ever done or wanted to
do, but I cannot think of a time when I wanted to filter an array on both
the key and the value at the same time: of course, it's easy to make up
use cases.
Are you saying that it's better to have familiarity over functionality?
I'm not sure I agree with this premise of we shouldn't provide
better functionality just because it would make all the other poorly
written functions look bad. That's setting a lower bar of standards, isn't
it?
In this case, I have a strong doubt that the "better functionality" will be
used, nor that it is indeed "better" at all. I also much prefer,
aesthetically, the _key() API over appending a flags parameter: though that
is likely, largely due to the familiarity mentioned earlier. Quite frankly
the simpler, more familiar approach looks best to me.
Also, please don't put words into my mouth: nowhere did I say, "we
shouldn't provide better functionality just because it would make all the
other poorly written functions look bad" (emphasis mine).
I appreciate you having an alternative viewpoint, but try not to see what
isn't there. What I was saying is that the other functions are there,
they do a great job and have been for a long time. Why not make use of that
array-function-muscle-memory; put it to our advantage?
To swing over to the other side of the fence, I do see that the flags-based
approach would "work" just as well in terms of getting the functionality
out there. It is something I have thought about adding previously for a
number of other functions. The closest being adding PREG_GREP_KEY to
preg_grep()
, which already has (only) PREG_GREP_INVERT. For that function,
a new flag would be the best option. For array_filter()
, I'm not convinced
at all.
var_dump(array_filter(['foo', '', 'bar'], 'strlen', true));
Warning:
strlen()
expects exactly 1 parameter, 2 given in - on line 1Not only do we trigger the error handler (a large enough array causes a
performance issue), but we also get back an empty array as a result. That's
BC and performance loss that's simply unacceptable, so defaulting the
behavior to pass the key is simply out of the question.Existing code won't pass that third, true, argument to
array_filter()
, and thus will not break.Nevertheless, I'd say go with a new array_filter_key() function, because that permits using existing one-parameter functions like that strlen as a callback, instead of forcing two-parameter callback functions.
I've mirrored the behaviour of this feature from JavaScript as much as possible (but obviously without passing the array itself as the third argument). As such I don't feel there's a need to introduce yet another function just so that the key is passed as the first argument. And to use both key and value forces the developer to use closures instead of just any callback unless this means the second argument passed to the callback is the array :)
Another alternative might be to go with a third argument to
array_filter()
, but make that an integer with ORable constants ARRAY_FILTER_KEY, ARRAY_FILTER_VAL - only one of them set calls with a single argument, both set call with two arguments.
I see some potential here, albeit a bit more magical :) Let's see how the discussion progresses.
best regards
Patrick
Hi,
I've updated my patch to allow a range of values to be passed as the third
argument: pass key, pass value or pass both.
Instead of using OR-able constants, I went with an enumeration type because
there are only going to be three options; it just felt wrong to have
ARRAY_FILTER_USE_KEY | ARRAY_FILTER_USE_VALUE
instead of a simpler
ARRAY_FILTER_USE_BOTH
or even just true
.
Updated examples can be found here: https://github.com/php/php-src/pull/287
Let me know if there are any major objections to the patch.
On Wed, Jun 19, 2013 at 7:15 AM, Tjerk Meesters tjerk.meesters@gmail.comwrote:
var_dump(array_filter(['foo', '', 'bar'], 'strlen', true));
Warning:
strlen()
expects exactly 1 parameter, 2 given in - on line 1Not only do we trigger the error handler (a large enough array causes a
performance issue), but we also get back an empty array as a result.
That's
BC and performance loss that's simply unacceptable, so defaulting the
behavior to pass the key is simply out of the question.Existing code won't pass that third, true, argument to
array_filter()
, and
thus will not break.Nevertheless, I'd say go with a new array_filter_key() function, because
that permits using existing one-parameter functions like that strlen as a
callback, instead of forcing two-parameter callback functions.I've mirrored the behaviour of this feature from JavaScript as much as
possible (but obviously without passing the array itself as the third
argument). As such I don't feel there's a need to introduce yet another
function just so that the key is passed as the first argument. And to use
both key and value forces the developer to use closures instead of just any
callback unless this means the second argument passed to the callback is
the array :)Another alternative might be to go with a third argument to
array_filter()
, but make that an integer with ORable constants
ARRAY_FILTER_KEY, ARRAY_FILTER_VAL - only one of them set calls with a
single argument, both set call with two arguments.I see some potential here, albeit a bit more magical :) Let's see how the
discussion progresses.best regards
Patrick--
Tjerk
Hi,
I've updated my patch to allow a range of values to be passed as the third
argument: pass key, pass value or pass both.Instead of using OR-able constants, I went with an enumeration type because
there are only going to be three options; it just felt wrong to have
ARRAY_FILTER_USE_KEY | ARRAY_FILTER_USE_VALUE
instead of a simpler
ARRAY_FILTER_USE_BOTH
or even justtrue
.
I suggest the following change:
ext/standard/php_array.h:119:
#define
ARRAY_FILTER_USE_BOTH
1
#defineARRAY_FILTER_USE_KEY
2
#define ARRAY_FILTER_USE_VALUE 3
becomes:
ext/standard/php_array.h:119:
#define
ARRAY_FILTER_USE_KEY
1
#define ARRAY_FILTER_USE_VALUE 2
#defineARRAY_FILTER_USE_BOTH
3
Such that we have the best of both worlds, either
ARRAY_FILTER_USE_BOTH
or ARRAY_FILTER_USE_KEY | ARRAY_FILTER_USE_VALUE
:D
--
Andrea Faulds
http://ajf.me/
Hi,
Hi,
I've updated my patch to allow a range of values to be passed as the third
argument: pass key, pass value or pass both.Instead of using OR-able constants, I went with an enumeration type because
there are only going to be three options; it just felt wrong to have
ARRAY_FILTER_USE_KEY | ARRAY_FILTER_USE_VALUE
instead of a simpler
ARRAY_FILTER_USE_BOTH
or even justtrue
.I suggest the following change:
ext/standard/php_array.h:119:
#define
ARRAY_FILTER_USE_BOTH
1
#defineARRAY_FILTER_USE_KEY
2
#define ARRAY_FILTER_USE_VALUE 3becomes:
ext/standard/php_array.h:119:
#define
ARRAY_FILTER_USE_KEY
1
#define ARRAY_FILTER_USE_VALUE 2
#defineARRAY_FILTER_USE_BOTH
3Such that we have the best of both worlds, either
ARRAY_FILTER_USE_BOTH
orARRAY_FILTER_USE_KEY | ARRAY_FILTER_USE_VALUE
:D
I deliberately chose these values so that I can simply specify true
to mean pass both values. Bit masks seem cumbersome in this situation.
--
Andrea Faulds
http://ajf.me/
On Thu, Sep 26, 2013 at 6:51 PM, Tjerk Meesters tjerk.meesters@gmail.comwrote:
I deliberately chose these values so that I can simply specify
true
to
mean pass both values. Bit masks seem cumbersome in this situation.
The default behavior is already to pass the value to the callback.
Requiring the argument only breaks backwards compatibility here. There is
no need for introducing 3 new constants. Simply use PASS_KEY or PASS_BOTH
to solve this problem with the default behavior is pass value if the
argument is omitted.
On Fri, Sep 27, 2013 at 7:11 AM, Sherif Ramadan theanomaly.is@gmail.comwrote:
On Thu, Sep 26, 2013 at 6:51 PM, Tjerk Meesters tjerk.meesters@gmail.comwrote:
I deliberately chose these values so that I can simply specify
true
to
mean pass both values. Bit masks seem cumbersome in this situation.The default behavior is already to pass the value to the callback.
Requiring the argument only breaks backwards compatibility here. There is
no need for introducing 3 new constants. Simply use PASS_KEY or PASS_BOTH
to solve this problem with the default behavior is pass value if the
argument is omitted.
I agree that only two constants are necessary here to accomplish the same
effect; I'll update the code.
I didn't quite understand how the current implementation would have BC
issues, though. Would you care to elaborate on that?
On Thu, Sep 26, 2013 at 7:33 PM, Tjerk Meesters tjerk.meesters@gmail.comwrote:
I didn't quite understand how the current implementation would have BC
issues, though. Would you care to elaborate on that?
I didn't look at your actual implementation. I saw 3 constants and deduced
that one would be required. If it was the case that there was a default to
values then there is no BC, but it's cleaner to just have two constants
here with the default behavior being values.
I'm starting up a thread for discussion on Pull Request 287
https://github.com/php/php-src/pull/287 (allowing array keys to be passed
to the callback function of array_filter through a third optional boolean
argument).
Why not use array_filter_keys?
There are many other array functions that work with keys using this naming
pattern. This would make it clearer in the code what is being filtered, and
no need for the additional parameter.
In this case I think I would still prefer it if the callback took a single
parameter. Values can still be easily looked up with $array[$key] in user
defined callbacks, but all of the built-in functions that take a single
parameter are still available to use.
I'm starting up a thread for discussion on Pull Request 287
https://github.com/php/php-src/pull/287 (allowing array keys to be passed
to the callback function of array_filter through a third optional boolean
argument).Why not use array_filter_keys?
That actually isn't a bad idea. I would consider going that route if this
patch presents any real world problems that could be better solved with
introducing a new function. I just don't see any reason for a new function
now given that this patch does not present any BC and can still solve the
same problem without adding a new function.
There are many other array functions that work with keys using this naming
pattern. This would make it clearer in the code what is being filtered, and
no need for the additional parameter.
The problem with having one callback for just the keys and another for the
array elements is that you're stuck in a situation where you can't possibly
use both. We have that problem with array sort functions that allow a user
defined callback. For example, we have usort and uksort, but there is no
sort function that can offer both. It's plausible that a user may be
relying on both in certain cases (for a example a map of maps).
In this case I think I would still prefer it if the callback took a single
parameter. Values can still be easily looked up with $array[$key] in user
defined callbacks, but all of the built-in functions that take a single
parameter are still available to use.
I would like to solve the problem without introducing new problems. If I
can do that we get PHP to take a step forward and not a step side-ways.