[5.6] Fix uniqueStrict for keyless calls to it

[antonioribeiro]

Looks like array_unique is always strict, so if you are trying to get uniques from a keyless Collection:

$c = new Collection([10, '10']);

Those two unique calls:

$c->uniqueStrict()->values()->all()
$c->unique()->values()->all()

Will both return

[10]

This PR fix this by using valueRetriever also when the user does not pass a key to uniqueStrict().

[Dylan-DPC-zz]

This is a breaking change.

[deleugpn]

Sounds more like a bug fix than a breaking change.

[antonioribeiro]

@deleugpn, I agree, a very old one, which may cause some BC for those who had hacked their code to overcome it, but still a bug, because there are some others out there thinking their non-strict unique results are fine, but they aren't. The way it is, everything is treated as strict when using unique(), if you don't pass a $key.

[Dylan-DPC-zz]

The point that the tests fail is a clear indication that it is a breaking change. There will be people expecting the same behaviour as the tests do.

[antonioribeiro]

Tests are failing only on PHP 7.2, which is not even released yet:

And are failing on a completely unrelated code.

[Dylan-DPC-zz]

Fine. Forgot to check that. But claim can be still made that users see the existing behaviour as expected. Graham has already made it to point to master so that's settled i guess

[deleugpn]

As a bug fix, it should go to 5.5

[Dylan-DPC-zz]

If it is a bug fix, first an issue should be made so that others can reproduce the bug 😛

[deleugpn]

[GrahamCampbell]

@deleugpn That's not what that means. What that means is you may sent a PR with a broken test, and no other changes, to demonstrate a bug.

[deleugpn]

@GrahamCampbell and what is the problem of implementing the fix in the same PR?

[GrahamCampbell]

There is no problem, only if the tests pass after.

[deleugpn]

Then there you have it.

• The PR implement a failing test;
• The PR implement the bug fix;
• The broken test has nothing to do with the PR;

Therefore, it should go to 5.5.

[antonioribeiro]

I just split it in different PRs, one for bug report and this one only to fix it.

[Dylan-DPC-zz]

What if someone is using unique() and assuming the strict behaviour is expected? This will break for him right?

[antonioribeiro]

It's tagged as 5.6 (which is next Laravel's "major" version, since it allows breaking changes on it), mate, so I think we got enough chat about BC, don't you?

Or are you telling us to not fix this bug at all?

[antonioribeiro]

I don't really care if it goes on 5.5 or 5.6, it doesn't look like a security issue, it was not even a problem for me, I only bumped into it because I was literally rewriting all tests for Collection, under the perspective of a new package I'm creating, and I could not make the one for uniqueStrict() pass.

What I think is important here: the bug was identified and at least one fix was created for it, but this PR may not be even the fix Taylor and the Core will merge to 5.6 or 5.5.

[Dylan-DPC-zz]

It's tagged as 5.6 (which is next Laravel's "major" version, since it allows breaking changes on it), mate, so I think we got enough chat about BC, don't you?
Or are you telling us to not fix this bug at all?

I was replying to Deleu's comments about merging this to 5.5

[antonioribeiro]

@taylorotwell Thanks for the merge, but the tests related to this bug are in the PR you just close.

[deleugpn]

@antonioribeiro you should have kept it in the same PR.