[5.2] Fix Arr::get($array, null) when $array is empty

[thomasruiz]

Following #12958, #12946 and #12945.

Basically, in that particular use case, when the model is created, attributes and original are both set to empty arrays. In Arr::get, we have if (! $array) { return $default; }. However, in PHP, when an array is empty, (! $array) === true.
This PR just adds to the condition the && ! is_array($array) to fix the problem.

[vlakoff]

I wasn't expecting we would go as far as empty array + null key. But we do.

A better implementation would be to move up the is_null($key) check.

[thomasruiz]

Depends whether we want Arr::get(null, null, 'foo') to return null or foo. I think foo would make more sense?

[vlakoff]

Should be more like "Test $array is empty and $key is null"

[vlakoff]

Depends whether we want Arr::get(null, null, 'foo') to return null or foo. I think foo would make more sense?

Does this make sense at all? :p

So the alternatives are:

• null/false array --> return default
• null key --> return input array (so default is irrelevant)

[vlakoff]

Your pick, just please add a test for the choosen one :)

[taylorotwell]

Is this supposed to be an or check? Seems weird to be and?

[vlakoff]

It means "falsy, but not an empty array".

[vlakoff]

We should be ok with only:

if (! is_array($array)) {
    return value($default);
}

if (is_null($key)) {
    return $array;
}

Tests:

$this->assertSame([], Arr::get([], null));
$this->assertSame([], Arr::get([], null, 'default'));
$this->assertEquals('default', Arr::get(null, null, 'default'));

[vlakoff]

It should rather use static::accessible:

if (! static::accessible($array)) {
    return value($default);
}

if (is_null($key)) {
    return $array;
}

[thomasruiz]

I tried with only ! is_array, it doesn't work properly when using a key not null.

[vlakoff]

I had edited my message, can you please check again?

[thomasruiz]

@vlakoff Yep, working properly with accessible. I added the tests you recommended, they make sense.

[vlakoff]

Please use assertSame here. I'm tired of loose typing enough :\

[dciancu]

@thomasruiz You changed perms on the test file, you should revert them back to 644.

[vlakoff]

To summarize:

Previous behaviour:

• if array is not an array, return default.
• if array is empty, return default (as there can't be a match).
• then, if key is null, return whole array.

Fixed behaviour:

• if array is not an array, return default.
• if array is empty and a key is specified, return default (no match).
• if array is empty and key is null, return empty array. <-- that's the change
• if key is null, return whole array.

[taylorotwell]

Why is this so complicated? Arr::get has been the same for years and over the past month it seems like we have been screwing with it endlessly? Can we please stop and just make it work how it has worked for the past 4 years?

[taylorotwell]

Does this make it work how it used to work?

[vlakoff]

I understand this can annoy you (you're not alone ;)

Problem is, the framework is relying on edge, real tricky cases of Arr::get, which weren't really determined, nor enforced by tests.

Bad news, without surprise it easily broke, good news, it gets reinforced in the process.

[vlakoff]

Does this make it work how it used to work?

We do our best.

[crynobone]

Can we delay this to 5.3?

[thomasruiz]

I'm pretty sure the code works as intended now.

[vlakoff]

Can we delay this to 5.3?

We are restoring previous behaviour for the case Arr::get([], null), so it has to go to 5.2.

[vlakoff]

For codebase consistency, we might make a similar change in Arr::has. This would produce the same results. More function calls, but codebase consistency with Arr::get. I let you decide on that.

Apart from that, PR seems good to go.

[mark86092]

Why we need Arr::get accept null or false, as it is not designed to do so.

Arr means working on array (or array like)

[taylorotwell]

STOP ARGUING ABOUT WHAT YOU THINK IT SHOULD BE. This function has been the same since like Laravel 2.0 and has NEVER caused a problem until people started screwing around with it like a month ago. STOP! Just make it how it used to work in Laravel 1, 2, 3, 4, 5.1, 5.2 until people started messing with it.

[mark86092]

Oh, update my word, I mean it works GOOD now, other special cases (not designed) change is not the point as Taylor said