Skip to content

Conversation

themsaid
Copy link
Member

As reported in #21406

Currently Collection::contains() uses Collection::first(), if first() returned null contains will return false even if null is the desired value.

@@ -217,7 +217,7 @@ public function contains($key, $operator = null, $value = null)
{
if (func_num_args() == 1) {
if ($this->useAsCallable($key)) {
return ! is_null($this->first($key));
return $this->first($key, '__placeholder') != '__placeholder';
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that safe though? What guarantee is there that the actual value isn't __placeholder?

In other places we've used an instance of stdClass for these.

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, we should augment the with helper to allow passing it a callback. Then this would be:

return with(new stdClass, function ($placeholder) use ($key) {
    return $this->first($key, $placeholder) != $placeholder;
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we pleeeease get arrow functions already???

return with(new stdClass, $placeholder => $this->first($key, $placeholder) != $placeholder);

Why can't we have nice things in PHP? 😢

Copy link
Member Author

Choose a reason for hiding this comment

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

switched to using stdClass.

Because php ¯_(ツ)_/¯

@JosephSilber
Copy link
Contributor

I submitted a PR for the with callback #21445

@taylorotwell taylorotwell merged commit 2bb22ed into laravel:5.5 Sep 28, 2017
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.

3 participants