gmane.comp.php.devel

Re: [PHP-DEV] PR 287 - added use_keys argument to array_filter() [Discussion]

Tjerk Meesters — 2013-06-18T23:15:45


On 19 Jun, 2013, at 1:04 AM, Patrick Schaaf <bof< at >bof.de> wrote:

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 :)

I see some potential here, albeit a bit more magical :) Let's see how the discussion progresses.

Re: [PHP-DEV] PR 287 - added use_keys argument to array_filter() [Discussion]

Peter Cowburn — 2013-06-18T23:09:09


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

Re: [PHP-DEV] PR 287 - added use_keys argument to array_filter() [Discussion]

Sherif Ramadan — 2013-06-18T22:21:41


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?

Re: [PHP-DEV] PR 287 - added use_keys argument to array_filter() [Discussion]

Peter Cowburn — 2013-06-18T22:08:22



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.

Re: [PHP-DEV] PR 287 - added use_keys argument to array_filter() [Discussion]

Leigh — 2013-06-18T17:30:03


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 :))

Re: [PHP-DEV] PR 287 - added use_keys argument to array_filter() [Discussion]

Sherif Ramadan — 2013-06-18T17:24:15


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).


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 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.

Re: [PHP-DEV] PR 287 - added use_keys argument to array_filter() [Discussion]

Leigh — 2013-06-18T17:07:58

That look like a fairly decent compromise of all the suggestions so far.

Re: [PHP-DEV] PR 287 - added use_keys argument to array_filter() [Discussion]

Patrick Schaaf — 2013-06-18T17:04:20

That's

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

Re: [PHP-DEV] PR 287 - added use_keys argument to array_filter() [Discussion]

Sherif Ramadan — 2013-06-18T16:42:31

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.

Re: [PHP-DEV] PR 287 - added use_keys argument to array_filter() [Discussion]

Sherif Ramadan — 2013-06-18T16:39:05



I'm not sure how that's a real problem. All built in PHP functions specify
a signature.

Re: [PHP-DEV] PR 287 - added use_keys argument to array_filter() [Discussion]

Sherif Ramadan — 2013-06-18T16:38:25

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.


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).



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.

Re: [PHP-DEV] PR 287 - added use_keys argument to array_filter() [Discussion]

Tjerk Anne Meesters — 2013-06-18T15:37:42


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 ;-)

Re: [PHP-DEV] PR 287 - added use_keys argument to array_filter() [Discussion]

Levi Morrison — 2013-06-18T15:28:41

If `strlen` can't handle extra parameters then I'd say *that* is a real
problem, this PR aside.

Re: [PHP-DEV] PR 287 - added use_keys argument to array_filter() [Discussion]

Leigh — 2013-06-18T15:27:04


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.

Re: [PHP-DEV] PR 287 - added use_keys argument to array_filter() [Discussion]

Sherif Ramadan — 2013-06-18T15:07:24



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.

Re: [PHP-DEV] PR 287 - added use_keys argument to array_filter() [Discussion]

Levi Morrison — 2013-06-18T14:58:07

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.

[PHP-DEV] PR 287 - added use_keys argument to array_filter() [Discussion]

Sherif Ramadan — 2013-06-17T23:20:37

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

[PHP-DEV] Re: [VOTE] Internal operator overloading and GMP improvments

Nikita Popov — 2013-06-17T15:22:58


Vote is closed now. RFC accepted (on both counts).

Nikita

Re: [PHP-DEV] pull reqs broken in bugs.php.net?

Johannes Schlüter — 2013-06-17T09:48:53

Hi,

On Mon, 2013-06-17 at 11:07 +0200, Ferenc Kovacs wrote:

Yes, most likely.


I will look into this. The whole PR thing currently is not nicely done,
I always hoped somebody improves it :)

johannes

Re: [PHP-DEV] pull reqs broken in bugs.php.net?

Ferenc Kovacs — 2013-06-17T09:13:55


I've manually removed the linked PR for #64549 for now.

Re: [PHP-DEV] pull reqs broken in bugs.php.net?

Ferenc Kovacs — 2013-06-17T09:07:43

I suppose somebody added manually (maybe selected a bad PR.
I agree that there should be a button to remove the link to a given PR and
it would be also nice if there would be an entry in the history about who
linked/unlinked a PR.