[5.4] Add fresh method on Eloquent\Collection. - #19616
Conversation
Add full tests for Model::fresh() and Collection::fresh() methods.
|
Isn't this failed on empty Collection since |
|
@decadence can you explain a bit more your issue? (some code?) |
| * @return static | ||
| */ | ||
| public function fresh($with = []) | ||
| { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Add $users = new Collection(); in your test before assertion and it will fail with Error: Call to a member function newQueryWithoutScopes() on null
There was a problem hiding this comment.
You should add in start of the method something like:
if ($this->isEmpty()) {
return new static; // or return $this;
}There was a problem hiding this comment.
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
There was a problem hiding this comment.
Sometimes you can't say if Collection will be empty or not. Therefore users have to do it check manually which is wrong.
There was a problem hiding this comment.
@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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ok, it's done (and I've updated tests too).
* commit '05060b183dd09cee6dce498da5afde9e66fd1a29': Apply fixes from StyleCI (laravel#19659) [5.4] Add ability to remove a global scope with another global scope (laravel#19657) [5.4] Add fresh method on Eloquent\Collection. (laravel#19616) support multiple fields to validateDifferent (laravel#19637) escape default value of yield blade directive (laravel#19643) [5.4] Add "avg" and "average" to higher order proxy. (laravel#19628) tagged v5.4.27 release notes version Add missing dependency. (laravel#19623) update timestamp on soft delete only when its used (laravel#19627) Add new `diffAssoc()` method to collection (laravel#19604) Fix tests code coverage config. (laravel#19609) # Conflicts: # src/Illuminate/Database/Eloquent/Collection.php
3 participants
Hi,
This PR adds this simple handy method which permit to
freshall the models in an Eloquent collection.It also adds two tests: one for the actual
Model::fresh()method, and one for the brand newCollection::fresh()one.Wait for your comments (or your merge :P)!
Matt'