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

[5.5] make Collection::where independent on error reporting #22172

Merged
merged 2 commits into from
Nov 22, 2017

Conversation

themsaid
Copy link
Member

@themsaid themsaid commented Nov 22, 2017

In reference to #22106

This change removes the try...catch wrapping so that it's not dependent on the level of error reporting you chose for your project.

if (count(array_filter([$retrieved, $value], 'is_object')) == 1)

Ensures that both variables are objects or non is an object.

@taylorotwell taylorotwell merged commit 16547e2 into laravel:5.5 Nov 22, 2017
@mbardelmeijer
Copy link
Contributor

This broke my code unfortunately. Code to reproduce:

Runs 1 with the old code-base, returns 0 with the new code-base

$user = new \stdClass;
$user->created_at = Carbon::now();
 collect([$user])->where('created_at', '!=', null)->count()

@themsaid
Copy link
Member Author

@mbardelmeijer fixed that here #22256

thanks for the heads up :)

@mbardelmeijer
Copy link
Contributor

@themsaid thank you!

@valorin
Copy link
Contributor

valorin commented Dec 4, 2017

@themsaid this breaks the case when you're trying to compare a object that can be converted to a string against a raw string.

Consider this code:

collect([['value' => new \Illuminate\Support\HtmlString('hello')]])
    ->where('value', 'hello');

Prior to this change, the result of where() would include the record, since the string values match.

After this change, the result of where() is empty.

I would consider this a major breaking change, because it's incredibly subtle and can cause unexpected behaviour in code (such as in our case).

@themsaid
Copy link
Member Author

themsaid commented Dec 4, 2017

@valorin here's a fix: #22295

@valorin
Copy link
Contributor

valorin commented Dec 4, 2017

Awesome, thanks @themsaid 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants