Framework: [5.2] Eloquent : related model not added to the collection in $attributes on save()

[RemiCollin]

It happens when the relations has been previously loaded.

Example :

    $blog->posts->count()

    $post = new Post(['title' => 'post title');

    $blog->posts()->save($post);

    $blog->posts->count(); // 0

On the opposite :

    $post = new Post(['title' => 'post title');

    $blog->posts()->save($post);

    $blog->posts->count(); // 1

[GrahamCampbell]

I don't think this is a bug?

[RemiCollin]

Seems like to me, at least not the behaviour we would expect.

[dvlpp]

Well, I think it might be: the purpose of saving the related object through the relation should be to keep the relation synced, IMO.

[patrickcarlohickman]

This is expected behavior. Once the relationship attribute for a model instance is loaded, it will not be reloaded unless explicitly reloaded.

// relationship attribute lazy loaded here
$blog->posts->count()

$post = new Post(['title' => 'post title');

// typo in OP; save() must be called on relationship method, not relationship attribute
$blog->posts()->save($post);

// relationship already loaded. Collection has not changed.
$blog->posts->count(); // 0

// however, call to database will reflect current count
$blog->posts()->count(); // 1

// reload the relationship attribute
$blog->load('posts');

// relationship collection refreshed; count of relationship attribute will reflect this
$blog->posts->count(); // 1

[RemiCollin]

Thanks for the explanation @patrickcarlohickman

I guess instead of $blog->load('posts') , for consistency I'd be better always refering to $blog->posts() , as the lazy loaded relationship has to be reloaded anyway if we use the magic property to access it...

On the other hand, as the Relationship object is aware of its parent model and the name of the relation, it should be quite easy to perform a check on the parent model and push the freshly saved model to the collection if it has already be lazy loaded. This way we could avoid additionnal queries. What do you think ?

[patrickcarlohickman]

@RemiCollin I think there's too much uncertainty to attempt modifying the Collection. Even though you're relating the post to the blog through the posts() relationship, there is no guarantee that the new post should be loaded into the relationship attribute Collection in the first place.

Imagine, for example, if your relationship were defined like this (silly example, but bear with me):

// only get posts created before today
public function posts() {
    return $this->hasMany(Post::class)->where('created_at', '<', date('Y-m-d'));
}

Given this relationship, since the new post is created today, it would be incorrect if it were simply injected into the existing lazy loaded $blog->posts collection, because it does not meet the added where condition. If you reload this relationship, the new post will still not be in it.

Another example would be this:

// get the posts, newest first
public function posts() {
    return $this->hasMany(Post::class)->orderBy('created_at', 'DESC');
}

Given this relationship, if the new post is added to the end of the existing loaded $blog->posts collection, it will be in the incorrect order. You could argue that it could be added to the beginning, but then it would be in the incorrect order if the relationship were ordered ASC. You would have to parse the relationship query to try to figure out where to add the item.

Now, those are just simple examples. I'm sure there are plenty of very complicated relationships out there where it would be impossible to parse all the constraints and conditions in order to properly insert the new item into the existing collection.

The only real alternative to the existing functionality would be to always force a reload of an existing loaded relationship whenever the relationship is modified, however that could lead to some serious performance issues. It seems best to make sure the developer knows they need to explicitly reload the relationship if needed, rather than to implicitly reload it, whether it is needed or not.

[dvlpp]

@patrickcarlohickman This makes sense, but as you wrote at the very end of your message, I think a good way could be to consider the $blog->posts "magic" call as a cache, and to flush that cache whenever the collection is directly updated with a $blog->posts()->save($post).

The performance issue could be discussed: the SQL request is only made on the next $blog->posts call, and there is one there's anyway a very huge chance that we need the new record in this call (we will do this SQL request ourselves).

Maybe, to let the full decision to the developer, it could be an optional feature, like the dates to Carbon instances one. An instance attribute in the Model like:

protected $syncCollections = ["posts"];

What's your opinion?

[RemiCollin]

@patrickcarlohickman :

You're right, I didn't think of those complex relationships at first.

But, even in those cases, I think it should still be possible to test if there were additionnal constraints added to the 'base' relationship object, by doing some tests to its query builder instance (not sure how easy / hard those test would be though). Then :

• If there was no user constraint added to the relationship : push to relation's collection instance.
• If any user constraint exists : reload lazy loaded relation.

If this would be a performance issue or not really depends on the use case. Assuming the save() method is mostly used on simple relationships than on relationships with constraints, I guess it would be better than having to manually reload a lazy loaded collection when we could simply push to it without making another query.

@dvlpp : Yes, I guess having this behaviour explicitely requested in the model could be a good alternative.

[leichti]

I just ran into the same issue and I totally agree with the idea of @dvlpp. That would be great.

[dsazup]

I spent a good hour trying to figure out what was going on.. :D It makes sense why it is like that, but at least would be nice if it would be documented.

[themsaid]

With the introduction of Model::refresh() you can now reload the parent model along with all its reloaded relations which will provide you with a shortcut on dealing with this behaviour.

Closing since it's not actually a bug.