Skip to content
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

Add implementation and tests for array_key_first, array_key_last and array_value_first, array_value_last #3256

Closed
wants to merge 4 commits into from

Conversation

@wol-soft
Copy link

@wol-soft wol-soft commented May 29, 2018

Hello Internals,

I'd like to ask for feedback for my idea.
Some years ago a pull request came up to add array_key_first(), array_key_last(), and array_key_index() functions (#347).
In the meantime a RFC waspublished to integrate the functions (https://wiki.php.net/rfc/array_key_first_last_index) which was discussed in the mailing list two and a half years ago (https://externals.io/message/89955).

Since the pull request and the RFC doesn't sustain a real progress and in my opinion at least the functions array_key_first(array $a) and array_key_last(array $a) would be handy in some cases I'd like to propose using a smaller functional scope as a first step including only this two functions.
Additional features like returning the value of the requested element or the function array_key_index() could be implemented in a later stage if required.

I implemented those two functions in a different implementation approach which tries to use as much existing code as possible available at wol-soft@ec2332b.

Regards,
Enno

- array_key_first(array $a)
  Returns the key of the first element or null
- array_key_last(array $a)
  Returns the key of the last element or null
@cmb69
Copy link
Contributor

@cmb69 cmb69 commented Jun 8, 2018

It seems to me that we should have an RFC for this. Either https://wiki.php.net/rfc/array_key_first_last_index should be updated respectively, or a new RFC should be pursued.

Loading

@wol-soft
Copy link
Author

@wol-soft wol-soft commented Jun 11, 2018

I've requested a PHP.net wiki account at http://news.php.net/php.internals/102225 to create a new RFC due to the different functional scope of this PR in comparison to the existing RFC.

EDIT:
Link to the RFC for this PR: https://wiki.php.net/rfc/array_key_first_last

Loading

@linaori
Copy link

@linaori linaori commented Jun 12, 2018

According to your RFC, NULL will be returned if nothing was found. Does this work?

$array = [null => true];
if (null !== array_key_first($array) && null !== array_key_last($array)) {
    echo 'hi';
}

Reading the code, I would expect that there is a collision in the return value. I know it's an edge-case, but I've seen this code being written before. The var_dump output of this gives an empty string though, so it's different behavior than you'd expect when reading the code as a new developer.

$array = [null => true];
var_dump($array);

array(1) { [""] => bool(true) }

Loading

@wol-soft
Copy link
Author

@wol-soft wol-soft commented Jun 12, 2018

The functions will return null only for empty arrays. The provided array is not empty and thus a given key will be returned. As a key initialized with null will be casted to an empty string, the empty string will be returned:

$array = [null => true];
var_dump(array_key_first($array));

string(0) ""

In the code example you provided the code inside the if will be executed and "hi" will be echoed.

It's an unexpected behavior at first glance but it's an already existing PHP behavior, see array_keys for example:

var_dump(array_keys([null => true]));

array(1) {
  [0]=>
  string(0) ""
}

Loading

@Pierstoval
Copy link

@Pierstoval Pierstoval commented Jun 14, 2018

We could even propose an RFC to deprecate null as a key assignment, WDYT?

Loading

@krakjoe krakjoe added the RFC label Jun 14, 2018
@wol-soft
Copy link
Author

@wol-soft wol-soft commented Jun 14, 2018

deprecate null as an array key could help to avoid errors. But I think it would be a huge deal to update large legacy applications due to something like

$array[$randomVariable] = $bla;

where $randomVariable is not always defined. Have seen it sometimes.

Should be feasible with an appropriate time as a warning.

Loading

@Pierstoval
Copy link

@Pierstoval Pierstoval commented Jun 14, 2018

Yep, that's the goal of a deprecation: just send a E_DEPRECATED error without breaking the script 😉

Loading

* array_value_first($array)
* array_value_last($array)
to handle the values of the outer array elements
@wol-soft wol-soft changed the title Add implementation and tests for array_key_first and array_key_last Add implementation and tests for array_key_first, array_key_last and array_value_first, array_value_last Jun 19, 2018
@wol-soft
Copy link
Author

@wol-soft wol-soft commented Jun 19, 2018

As I extended the scope of the RFC to provide a complete set of functions for handling the outer array elements I updated the PR to also implement and add tests for the methods

$firstValue = array_value_first($array);
$lastValue = array_value_last($array);

Loading

@imbrish
Copy link

@imbrish imbrish commented Jun 19, 2018

Is the _value part added due to backward compatibility? It would make sense as these two functions are often implemented in the user land. Otherwise I think it would be neat to have just short names array_first and array_last as the _value part is nowhere to be seen in other array functions. Replacing user defined functions with the same functionality shouldn't be a big issue.

Loading

@wol-soft
Copy link
Author

@wol-soft wol-soft commented Jun 19, 2018

Primary I've chosen the names to have consistent function names for the group of functions to handle array keys/values:

array_keys();
array_key_first();
array_key_last();

array_values();
array_value_first();
array_value_last();

In my opinion array_value_first describes the function better than array_first which provides less context if used in user land code and may be mistaken as array_key_first.

Loading

@amcsi
Copy link

@amcsi amcsi commented Jul 9, 2018

array_value_first() returning NULL is still ambiguous though, because the first value could be NULL, or the array could be empty. :/

Loading

* Get a value out of a HashTable at the given position and copy the value into return_value.
*/
static inline
void array_value_result_helper(HashTable *ht, HashPosition *pos, zval *return_value)
Copy link
Contributor

@derickr derickr Jul 9, 2018

Choose a reason for hiding this comment

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

This should not split over two lines.

Loading

Copy link
Author

@wol-soft wol-soft Jul 11, 2018

Choose a reason for hiding this comment

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

This seems to be inconsistently but I'll gladly fix this to an oneliner

Loading

@@ -418,10 +418,27 @@ ZEND_BEGIN_ARG_INFO_EX(arginfo_array_keys, 0, 0, 1)
ZEND_ARG_INFO(0, strict)
ZEND_END_ARG_INFO()

ZEND_BEGIN_ARG_INFO(arginfo_array_key_first, 0)
ZEND_ARG_INFO(0, arg) /* ARRAY_INFO(0, arg, 0) */
Copy link
Contributor

@derickr derickr Jul 9, 2018

Choose a reason for hiding this comment

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

Why is the comment ARRAY_INFO, instead of just using ZEND_ARG_ARRAY_INFO ?

Loading

Copy link
Contributor

@cmb69 cmb69 Jul 9, 2018

Choose a reason for hiding this comment

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

Since we don't use the typed arginfo parameters, yet. @nikic recently suggested to stick with that for now to avoid duplicate checks, IIRC.

Loading

Copy link
Author

@wol-soft wol-soft Jul 11, 2018

Choose a reason for hiding this comment

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

I adopted it from existing functions for consistency so I guess keep it?

Loading


/* Various combinations of arrays to be used for the test */
$mixed_array = array(
array(),
Copy link
Contributor

@derickr derickr Jul 9, 2018

Choose a reason for hiding this comment

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

You should add test cases with:

  • A single element here: array( "foo" );
  • An array where there is one element, and it doesn't start with index 0: array( 1 => "42" ),

Loading

Copy link
Author

@wol-soft wol-soft Jul 11, 2018

Choose a reason for hiding this comment

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

Good additional cases, I'll add them to the tests for all four functions.

Thanks for the input!

Loading

Copy link
Author

@wol-soft wol-soft Jul 17, 2018

Choose a reason for hiding this comment

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

I've added the cases to the accepted two functions

Loading

Add additional test cases for array_key_first() and array_key_last()
@php-pulls
Copy link

@php-pulls php-pulls commented Jul 17, 2018

Comment on behalf of cmb at php.net:

Thanks! Applied via 50516a6.

Loading

@php-pulls php-pulls closed this Jul 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
10 participants