[7.x] 3.5x faster micro performance enhancement for data_get()

[kevindees]

Replace while:array_shift with foreach:unset. This operation is 3.5x faster. It is arguably more readable as well.

In my isolation tests with 1000 iterations:

• While loop 1.969ms
• Foreach loop 0.527ms

[browner12]

how are you accounting for possible nulls in the keys?

[GrahamCampbell]

how are you accounting for possible nulls in the keys?

That never happens. Keys are either strings or integers.

NB If you do an array access with another type of scalar, PHP will silently convert it (if it's numeric, to an integer then to a string, otherwise, straight to a string), and otherwise, it will raise a warning (or on PHP 8 I think it raises an actual Error).

[browner12]

correct, but $keys is a misleading variable name here. We're not actually pulling "keys" from the array, we're pulling "values" from the $key array.

[kevindees]

how are you accounting for possible nulls in the keys?

I was not sure of the purpose of is_null(). I'm assuming it accounts for calls like this:

    $data = [
        'product-one' => ['name' => ['sku' => 123], 'sku' => 987],
        'product-two' => ['name' => ['sku' => 123], 'sku' => 987],
    ];

    dd(data_get($data, ['*', null, 'sku']));

I have added a check for this but I'd assume this issue is an edge case.

Maybe it should throw an exception instead. I'd imagine anyone sending nulls is not getting the result they desire in the first place unless they are building into the edge case for some odd reason.

If null should be accounted for it is possible !is_string || !is_numeric values should also be checked instead (though this is not in the current feature design).

[browner12]

The purpose of it originally was to figure out when the $key variable was empty.

According to the array_shift docs:

Returns the shifted value, or NULL if array is empty or is not an array.

This way, the while loop would end once array_shift pulled all the items out of $key.

As I'm looking back at the old code, having a null value in your $key array would actually cause a premature termination of the while loop. Based on that, I would say we don't actually have to check for it in the new code, so you can probably revert your latest commit.

while(array_shift([null, 1, null])) {
    echo 'test';
}

In this example, "test" will never be echoed.

[kevindees]

@browner12

Based on my testing adding the null check adds some sanity to the return value (in the edge-case) and the null check has little performance impact allowing the performance to remain intact.

However, on the other hand, the null check can hide more serious bugs because it produces a more reasonable result. Maybe it would be removed in v8 but remain in v7 to keep backward compatibility.

[taylorotwell]

So after this discussion I'm confused. Is there ANY behavior change here at all?

[browner12]

I believe the latest commit that added the NULL check makes this code functionally equivalent to the old code. I ran a couple scenarios, but would love for a second set of eyes on it.

[browner12]

there was a PR awhile ago that switched $var === null to is_null($var).

https://github.com/laravel/framework/pull/15655/files

Might want to switch this line to maintain consistency.

[kevindees]

This PR is in feature parity with the current 7.x state.

[browner12]

I agree the behavior is the same, but to maintain style consistency, we should switch $segment === null to is_null($segment).

Also, FYI. Parity, not parody.

[kevindees]

Done

[kevindees]

So after this discussion I'm confused. Is there ANY behavior change here at all?

There is no behavior change. It is in parity.

[taylorotwell]

Thank you