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

[5.4] Add fresh method on Eloquent\Collection. #19616

Merged
merged 3 commits into from
Jun 17, 2017
Merged

[5.4] Add fresh method on Eloquent\Collection. #19616

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
95a123b
Add fresh method on Eloquent/Collection.
mathieutu Jun 15, 2017
ff707c8
Rewrite completely the method to avoid multiple database calls.
mathieutu Jun 15, 2017
7cd6ae2
Update tests for php5.6 and Carbon v1.20.0
mathieutu Jun 15, 2017
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
23 changes: 22 additions & 1 deletion src/Illuminate/Database/Eloquent/Collection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ public function merge($items)
* Run a map over each of the items.
*
* @param callable $callback
* @return \Illuminate\Support\Collection
* @return \Illuminate\Support\Collection|static
*/
public function map(callable $callback)
{
Expand All @@ -138,6 +138,27 @@ public function map(callable $callback)
}) ? $result->toBase() : $result;
}

/**
* Reload a fresh model instance from the database for all the entities.
*
* @param array|string $with
* @return static
*/
public function fresh($with = [])
{
$model = $this->first();
Copy link
Contributor

Choose a reason for hiding this comment

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

I have no code. I just assume if Collection is empty this line returns null and next code will fail with Trying to get property of non-object

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah! I understand now.
But why would you want to fresh an empty collection ? It's about loading all models from database, if you have no model, no need to fresh it ?

But anyway you're right, I will fix it by a check with a isEmpty.

Thank's!

Copy link
Contributor

Choose a reason for hiding this comment

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

Add $users = new Collection(); in your test before assertion and it will fail with Error: Call to a member function newQueryWithoutScopes() on null

Copy link
Contributor

Choose a reason for hiding this comment

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

You should add in start of the method something like:

if ($this->isEmpty()) {
    return new static; // or return $this;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

#19651

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I didn't see your comment and already did PR.
I don't need fresh on empty Collection personally but this check has to be there

Copy link
Contributor

Choose a reason for hiding this comment

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

Sometimes you can't say if Collection will be empty or not. Therefore users have to do it check manually which is wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@decadence Yeah I agree, but be careful, you made the PR on 5.5 version, and not 5.4!
This one was for 5.4...

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right. I think this was for 5.5 because it was there already. Maybe it's worth to make it for 5.4 too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, it's done (and I've updated tests too).


$freshModels = $model->newQueryWithoutScopes()
->with(is_string($with) ? func_get_args() : $with)
->whereIn($model->getKeyName(), $this->modelKeys())
->get()
->getDictionary();

return $this->map(function ($model) use ($freshModels) {
return $model->exists ? $freshModels[$model->getKey()] : null;
});
}

/**
* Diff the collection with the given items.
*
Expand Down
42 changes: 42 additions & 0 deletions tests/Database/DatabaseEloquentIntegrationTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1069,6 +1069,48 @@ public function testIsAfterRetrievingTheSameModel()
$this->assertTrue($saved->is($retrieved));
}

public function testFreshMethodOnModel()
{
$now = \Carbon\Carbon::now();
\Carbon\Carbon::setTestNow($now);

$storedUser1 = EloquentTestUser::create(['id' => 1, 'email' => 'taylorotwell@gmail.com']);
$storedUser1->newQuery()->update(['email' => 'dev@mathieutu.ovh', 'name' => 'Mathieu TUDISCO']);
$freshStoredUser1 = $storedUser1->fresh();

$storedUser2 = EloquentTestUser::create(['id' => 2, 'email' => 'taylorotwell@gmail.com']);
$storedUser2->newQuery()->update(['email' => 'dev@mathieutu.ovh']);
$freshStoredUser2 = $storedUser2->fresh();

$notStoredUser = new EloquentTestUser(['id' => 3, 'email' => 'taylorotwell@gmail.com']);
$freshNotStoredUser = $notStoredUser->fresh();

$this->assertEquals(['id' => 1, 'email' => 'taylorotwell@gmail.com', 'created_at' => $now, 'updated_at' => $now], $storedUser1->toArray());
$this->assertEquals(['id' => 1, 'name' => 'Mathieu TUDISCO', 'email' => 'dev@mathieutu.ovh', 'created_at' => $now, 'updated_at' => $now], $freshStoredUser1->toArray());
$this->assertInstanceOf(EloquentTestUser::class, $storedUser1);

$this->assertEquals(['id' => 2, 'email' => 'taylorotwell@gmail.com', 'created_at' => $now, 'updated_at' => $now], $storedUser2->toArray());
$this->assertEquals(['id' => 2, 'name' => null, 'email' => 'dev@mathieutu.ovh', 'created_at' => $now, 'updated_at' => $now], $freshStoredUser2->toArray());
$this->assertInstanceOf(EloquentTestUser::class, $storedUser2);

$this->assertEquals(['id' => 3, 'email' => 'taylorotwell@gmail.com'], $notStoredUser->toArray());
$this->assertEquals(null, $freshNotStoredUser);
}

public function testFreshMethodOnCollection()
{
EloquentTestUser::create(['id' => 1, 'email' => 'taylorotwell@gmail.com']);
EloquentTestUser::create(['id' => 2, 'email' => 'taylorotwell@gmail.com']);

$users = EloquentTestUser::all()
->add(new EloquentTestUser(['id' => 3, 'email' => 'taylorotwell@gmail.com']));

EloquentTestUser::find(1)->update(['name' => 'Mathieu TUDISCO']);
EloquentTestUser::find(2)->update(['email' => 'dev@mathieutu.ovh']);

$this->assertEquals($users->map->fresh(), $users->fresh());
}

/**
* Helpers...
*/
Expand Down