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.

Sign up for GitHub

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

[8.x] Collection prevent null being added to resulting collection with pop and shift #38265

Closed
wants to merge 9 commits into from
Closed

[8.x] Collection prevent null being added to resulting collection with pop and shift #38265

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
946a4a4
pop olny return collection items
craig-sen Aug 7, 2021
faa5076
add test - pop only return collection items
craig-sen Aug 7, 2021
a60645f
shift only return collection items
craig-sen Aug 7, 2021
2c6b9fe
add test - shift only return collection items
craig-sen Aug 7, 2021
b9d9e23
if collection is empty or less than one return null
craig-sen Aug 7, 2021
f60ced8
add test return null when no items or less than zero
craig-sen Aug 7, 2021
40ccf2b
whitespace
craig-sen Aug 7, 2021
c907736
return an empty collection when less than 1 or no items
craig-sen Aug 9, 2021
29c6046
return collection when parameter supplied
craig-sen Aug 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/Illuminate/Collections/Collection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -793,10 +793,16 @@ public function only($keys)
*/
public function pop($count = 1)
{
if ($count < 1 || $this->count() === 0) {
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be a new empty collection instead of null to allow for chaining?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can just use return;, there are 2 more spots in this file related to the functions median() and mode() which also have a null return. I'm not sure if returning a new collection is appropriate here when comparing it to the php array_pop()

Copy link
Author

Choose a reason for hiding this comment

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

as mentioned by @bert-w with php the functions array_pop and array_shift returning null in the case of an empty array, when the $count parameter is 1 this implementation seems appropriate.

However, in the case that there is a user supplied parameter which is not 1 things get a little bit more difficult...
Considering the current behaviour of the function it may be better to apply something similar to the following logic

if ($count === 1) {
    return object | null;
}

if ($count < 0 || $this->count() === 0) {
    return empty collection;
}

return $count item collection;

The more I think about it, it is probally more appropriate to seperate the funtionality into classic pop functionality returning object | null, and popMultiple which takes a parameter and returns a collection. (As this is already in production it would be a breaking change?)

}

if ($count === 1) {
return array_pop($this->items);
}

$count = $count > $this->count() ? $this->count() : $count;

$results = [];

foreach (range(1, $count) as $item) {
Expand Down Expand Up @@ -957,12 +963,18 @@ public function search($value, $strict = false)
*/
public function shift($count = 1)
{
if ($count < 1 || $this->count() === 0) {
return null;
}

if ($count === 1) {
return array_shift($this->items);
}

$results = [];

$count = $count > $this->count() ? $this->count() : $count;

foreach (range(1, $count) as $item) {
array_push($results, array_shift($this->items));
}
Expand Down
42 changes: 42 additions & 0 deletions tests/Support/SupportCollectionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,27 @@ public function testPopReturnsAndRemovesLastXItemsInCollection()
$this->assertSame('foo', $c->first());
}

public function testPopOnlyReturnsCollectionItemsWhenParamGreaterThanCollectionLength()
{
$c = new Collection(['foo', 'bar']);
$this->assertEquals(new Collection(['bar', 'foo']), $c->pop(3));
$this->assertEmpty($c);
}

public function testPopReturnsNullWhenParameterLessThanOne()
{
$c = new Collection(['foo', 'bar']);
$this->assertNull($c->pop(0));
$this->assertCount(2, $c);
}

public function testPopReturnsNullWhenNoItemsInCollection()
{
$c = new Collection([]);
$this->assertNull($c->pop());
$this->assertCount(0, $c);
}

public function testShiftReturnsAndRemovesFirstItemInCollection()
{
$data = new Collection(['Taylor', 'Otwell']);
Expand All @@ -251,6 +272,27 @@ public function testShiftReturnsAndRemovesFirstXItemsInCollection()
$this->assertSame('baz', $data->first());
}

public function testShiftOnlyReturnsCollectionItemsWhenParamGreaterThanCollectionLength()
{
$c = new Collection(['foo', 'bar']);
$this->assertEquals(new Collection(['foo', 'bar']), $c->shift(3));
$this->assertEmpty($c);
}

public function testShiftReturnsNullWhenParameterLessThanOne()
{
$c = new Collection(['foo', 'bar']);
$this->assertNull($c->shift(0));
$this->assertCount(2, $c);
}

public function testShiftReturnsNullWhenNoItemsInCollection()
{
$c = new Collection([]);
$this->assertNull($c->shift());
$this->assertCount(0, $c);
}

/**
* @dataProvider collectionClassProvider
*/
Expand Down