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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[8.x] Added sole() and soleWhere() methods for Collections #37034

Merged
merged 15 commits into from Apr 20, 2021

Conversation

ash-jc-allen
Copy link
Contributor

@ash-jc-allen ash-jc-allen commented Apr 18, 2021

Hey! This PR should hopefully add new sole() and soleWhere() methods to Collections and LazyCollections. I've taken inspiration from the similar sole() method for the database queries and think that these could be pretty handy additions.

The sole() method should work in a similar way to the first() method does but does not require a default value as a second parameter. Likewise, the soleWhere() method should work in a similar way to the firstWhere() method.

If either of the methods are executed and there are no items to be returned, an Illuminate\Collections\ItemNotFoundException will be thrown. If the either of the methods are executed are there is more than one item to be returned, an Illuminate\Collections\MultipleItemsFoundException will be thrown.

Here's a couple of quick examples of how they could be used:

sole():

$collection = collect([
    ['name' => 'foo'],
    ['name' => 'bar'],
    ['name' => 'bar'],
]);

// $result will be equal to: ['name' => 'foo']
$result = $collection->where('name', 'foo')->sole();

// $result will be equal to: ['name' => 'foo']
$result = $collection->sole(function ($value) {
    return $value['name'] === 'foo';
});

// This will throw an ItemNotFoundException
$collection->where('name', 'INVALID')->sole();

// This will throw a MultipleItemsFoundException
$collection->where('name', 'bar')->sole();

soleWhere():

$collection = collect([
    ['name' => 'foo'],
    ['name' => 'bar'],
    ['name' => 'bar'],
]);

// $result will be equal to: ['name' => 'foo']
$result = $collection->soleWhere('name', 'foo');

// This will throw an ItemNotFoundException
$collection->soleWhere('name', 'INVALID');

// This will throw a MultipleItemsFoundException
$collection->soleWhere('name', 'bar');

If there's anything that might need changing on these, please let me know 馃槃

@driesvints
Copy link
Member

Ping @JosephSilber

@JosephSilber
Copy link
Member

I was never a fan of the sole method, but I guess that ship has sailed.

Comment on lines 1062 to 1063
* @throws ItemNotFoundException
* @throws MultipleItemsFoundException
Copy link
Member

Choose a reason for hiding this comment

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

We always use fully qualified class names in DocBlocks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh okay, sorry I wasn't aware of that. I'll get them changed to be fully qualified

Comment on lines 823 to 835
public function sole(callable $callback = null);

/**
* Get the first item by the given key value pair, but only if
* exactly one item matches the criteria. Otherwise, throw
* an exception.
*
* @param string $key
* @param mixed $operator
* @param mixed $value
* @return mixed
*/
public function soleWhere($key, $operator = null, $value = null);
Copy link
Member

Choose a reason for hiding this comment

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

We can't add methods to an interface mid-cycle.

Remove these from the interface. After it's merged, submit a separate PR to master to add them to the interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense thinking about it. I'll take this out and make a separate PR if this one gets merged

* @throws ItemNotFoundException
* @throws MultipleItemsFoundException
*/
public function sole(callable $callback = null)
Copy link
Member

Choose a reason for hiding this comment

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

This implementation is way more complex than it needs to be.

Something like this (not tested) should suffice:

public function sole(callable $callback = null)
{
    return $this
        ->when($callback)
        ->filter($callback)
        ->take(2)
        ->collect()
        ->sole();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do agree that it's more complex. I was looking at some of the existing code for things like the first() method and just assumed that it had been done like that for a specific reason. I've not really used LazyCollections too much so I wasn't 100% sure on the best way to implement it. Thanks for the advice on this one

/**
* @dataProvider collectionClassProvider
*/
public function testSoleReturnsFirstItemInCollectionIfOnlyOneExists($collection)
Copy link
Member

Choose a reason for hiding this comment

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

Please also add tests in SupportLazyCollectionIsLazyTest.php ensuring this method is lazy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah sure, I didn't realise those tests were there, so I'll add some to it

Comment on lines 466 to 469
public function soleWhere($key, $operator = null, $value = null)
{
return $this->sole($this->operatorForWhere(...func_get_args()));
}
Copy link
Member

Choose a reason for hiding this comment

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

Since, unlike the first method, sole doesn't take a $default parameter , there's no need for a separate soleWhere method. Constraints can be passed into the sole method directly, like we do in contains.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense. So do you suggest removed the soleWhere method completely?

*/
public function sole(callable $callback = null)
{
$items = $this->filter($callback);
Copy link
Member

Choose a reason for hiding this comment

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

The code as is will filter out nulls when no $callback has been provided.

Suggested change
$items = $this->filter($callback);
$items = $this->when($callback)->filter($callback);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh I must have missed that, cheers

@JosephSilber
Copy link
Member

It's a pity the exceptions aren't shared (maybe down the line in 9.x). Then this:

$result = $this->take(2)->get($columns);
if ($result->isEmpty()) {
throw new RecordsNotFoundException;
}
if ($result->count() > 1) {
throw new MultipleRecordsFoundException;
}
return $result->first();

could be changed to simply:

return $this->take(2)->get($columns)->sole(); 

@ash-jc-allen
Copy link
Contributor Author

Thanks for the feedback on all of this @JosephSilber! I'll make all of the changes for it now 馃槃

And I do agree that it'd be cool to have the shared functionality like in your suggestion.

@ash-jc-allen
Copy link
Contributor Author

I've made the changes that you suggested @JosephSilber and from what I can see, they all seem to working. One thing thing I'm not too sure about though is the new test I've added. I have a feeling that I might not have done it right; apologies if that's the case.

Copy link
Member

@JosephSilber JosephSilber left a comment

Choose a reason for hiding this comment

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

Looking better.

Seems like you forgot to add operator support to sole.

Comment on lines 980 to 989
public function testSoleIsLazy()
{
$data = $this->make([['a' => 1], ['a' => 2], ['a' => 3]]);

$this->assertEnumeratesCollection($data, 3, function ($collection) {
$collection->sole(function ($item) {
return $item['a'] === 2;
});
});
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public function testSoleIsLazy()
{
$data = $this->make([['a' => 1], ['a' => 2], ['a' => 3]]);
$this->assertEnumeratesCollection($data, 3, function ($collection) {
$collection->sole(function ($item) {
return $item['a'] === 2;
});
});
}
public function testSoleIsLazy()
{
$this->assertEnumerates(2, function ($collection) {
try {
$collection->sole();
} catch (MultipleItemsFoundException $e) {
//
}
});
$this->assertEnumeratesOnce(function ($collection) {
$collection->sole(function ($item) {
return $item === 1;
});
});
$this->assertEnumerates(4, function ($collection) {
try {
$collection->sole(function ($item) {
return $item % 2 === 0;
});
} catch (MultipleItemsFoundException $e) {
//
}
});
}

Be sure to also import MultipleItemsFoundException.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion again, I've just pushed up the new test changes :)

@JosephSilber
Copy link
Member

Seems like you forgot to add operator support to sole.

If it's not added now, we won't be able to add it until 9.x, since it would be a breaking change in the function signature.

@ash-jc-allen
Copy link
Contributor Author

@JosephSilber In your opinion, do you think it's something that should be added now? I can try and add it if so

@JosephSilber
Copy link
Member

Would be nice.

If you can't figure it out, let me know, and I'll try to find a few minutes to do it for you 馃憤

@ash-jc-allen
Copy link
Contributor Author

Sweet, I'll take a look at it now then. Thanks again for all the help on this one, I really appreciate it and it's giving me a chance to learn a bit more at the same time 馃槃

@browner12
Copy link
Contributor

god, this is a lot of extra code to avoid saying

if($collection->count() === 1){
    return $collection->first();
}

@ash-jc-allen
Copy link
Contributor Author

@JosephSilber I think I've added the operator support, but please let me know if I've missed anything

Comment on lines 1067 to 1084
public function sole($key = null, $operator = null, $value = null)
{
if (func_num_args() <= 1) {
$items = $this->when($key)->filter($key);

if ($items->isEmpty()) {
throw new ItemNotFoundException;
}

if ($items->count() > 1) {
throw new MultipleItemsFoundException;
}

return $items->first();
}

return $this->sole($this->operatorForWhere(...func_get_args()));
}
Copy link
Member

Choose a reason for hiding this comment

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

A ternary would be a little clearer, and allows dedenting the whole method:

Suggested change
public function sole($key = null, $operator = null, $value = null)
{
if (func_num_args() <= 1) {
$items = $this->when($key)->filter($key);
if ($items->isEmpty()) {
throw new ItemNotFoundException;
}
if ($items->count() > 1) {
throw new MultipleItemsFoundException;
}
return $items->first();
}
return $this->sole($this->operatorForWhere(...func_get_args()));
}
public function sole($key = null, $operator = null, $value = null)
{
$filter = func_num_args() > 1
? $this->operatorForWhere(...func_get_args())
: $key;
$items = $this->when($filter)->filter($filter);
if ($items->isEmpty()) {
throw new ItemNotFoundException;
}
if ($items->count() > 1) {
throw new MultipleItemsFoundException;
}
return $items->first();
}

Comment on lines 1025 to 1037
public function sole($key = null, $operator = null, $value = null)
{
if (func_num_args() <= 1) {
return $this
->when($key)
->filter($key)
->take(2)
->collect()
->sole();
}

return $this->sole($this->operatorForWhere(...func_get_args()));
}
Copy link
Member

Choose a reason for hiding this comment

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

Same here:

Suggested change
public function sole($key = null, $operator = null, $value = null)
{
if (func_num_args() <= 1) {
return $this
->when($key)
->filter($key)
->take(2)
->collect()
->sole();
}
return $this->sole($this->operatorForWhere(...func_get_args()));
}
public function sole($key = null, $operator = null, $value = null)
{
$filter = func_num_args() > 1
? $this->operatorForWhere(...func_get_args())
: $key;
return $this
->when($filter)
->filter($filter)
->take(2)
->collect()
->sole();
}

@ash-jc-allen
Copy link
Contributor Author

@JosephSilber Ah yeah, the ternary makes it look much cleaner, thanks! 馃槃

@ash-jc-allen
Copy link
Contributor Author

It looks like 2 of the test checks failed, but I don't think I can rerun them myself?

Copy link
Member

@JosephSilber JosephSilber left a comment

Choose a reason for hiding this comment

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

馃憤

@taylorotwell taylorotwell merged commit 9c42ab1 into laravel:8.x Apr 20, 2021
@ash-jc-allen ash-jc-allen deleted the feature/collection-sole branch April 20, 2021 19:34
$items = $this->when($filter)->filter($filter);

if ($items->isEmpty()) {
throw new ItemNotFoundException;
Copy link
Contributor

Choose a reason for hiding this comment

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

would be nice if the query builder etc would throw the exact same exceptions when calling sole()

Copy link
Member

Choose a reason for hiding this comment

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

@driesvints
Copy link
Member

This PR apparently introduced a new Illuminate\Collections namespace which doesn't exists. It's not autoloaded. Only the Illuminate\Support namespace exists. I'll send a PR to fix this.

@ash-jc-allen
Copy link
Contributor Author

@driesvints Sorry, my bad here! When I made the exceptions, I must have assumed at the time that the namespace was the same as the file path. I should have double-checked this before pushing 馃様

@driesvints
Copy link
Member

driesvints commented Aug 19, 2021

@ash-jc-allen don't worry about it. I've sent in a PR to fix this: #38449

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.

None yet

6 participants