[9.x] Fix bug in BelongsToMany where non-related rows are returned

[marcovo]

This PR is preceded by #41881 and #42049 .

Since Laravel 9.0, the methods firstOrNew(), firstOrCreate() and updateOrCreate() received a slight update in behaviour (3e66eb7) where related instance existence is now determined using a clean $this->related-> query instead of the previous $this-> relation query implementation. This introduced an inconsistency in these methods in comparison to the other BelongsToMany methods, where a possibility is introduced that these 3 methods return (or update) a row that is not (was not, is not, will not be) related to the base model. This PR addresses this issue.

---

First, this PR adds 3 tests that (as far as I understand Taylor's comment) describes why the changes in 3e66eb7 were made. This concerns the entirety of the three new tests, except the last assertion of each test. These assertions succeed in the current Laravel 9.* .

Note that this is based on my understanding of the issue, I have not found a reference to the actually issue back then. (Are any references available?)

Second, this PR contains 3 new assertions (the last one of each test mentioned here-above) that fail on the current Laravel 9.*, but which should probably succeed in order for these methods to be consistent with other similar methods. I think this at least is the case for firstOrCreate() and updateOrCreate(). I am not entirely sure on firstOrNew(), more on that below.

Thirdly, this PR contains changes to the implementations of the 3 methods that make sure the 3 new assertions succeed.

---

Updated Motivation
Consider that any method on the BelongsToMany relationship returning a model, returns a model that is related to the base model. This was the case in Laravel 8.*, the three methods either:

• return (or update) an already existing related model
• create and attach a new related model (except for firstOrNew, which cannot attach() due to no primary key being available)

Note that for the *OrCreate methods, we always end up in a state where the returned related model is actually related to the base model. Since Laravel 9.*, one new possibility is introduced; the three methods can:

• return an existing model from the related table, that is not attached to the base model

To my understanding, the calling site cannot distinguish this new situation from the first situation where an already existing related method is returned. Hence, to ensure in a mutating call site that the model from the related table really will be related to the base model, one should always manually call attach(), even if the row was in fact already related. In a retrieving call site, I am not sure what one could do when an existing non-related model is returned.

firstOrCreate()
My understanding of firstOrCreate is that you will always receive a related model instance that already was related ('first') or is newly related ('create') to the base model. In case a new related record is created, attach() is explicitly called. Hence I would assume that in case an existing unrelated model is returned, it should be explicitly attach()ed as well.

firstOrNew()
Similar arguments hold for firstOrNew. Also consider for example the similar method findOrNew(), which only returns existing models that are actually related (unlike the current firstOrNew() implementation). However, compared to firstOrCreate(), firstOrNew() is more nuanced: first returns the first related model, OrNew returns a new unsaved model that cannot be related since it is unsaved. In our new case (returning an existing unrelated model) one can follow two thoughts:

• it should be attached to be consistent with first
• it should not be attached to be consistent with OrNew

As not attaching leads to the call site ending up with no indication whether the model is actually attached or not, I would vote for consistency with first here.

Do note however, that when attach()ing here, we have no access to $joining and $touch variables like we do in the other method.

updateOrCreate
Consider that $model->relation()->update() will always only update rows that are related. Changing it into $model->relation()->updateOrCreate() introduces the possibility that (without clear indication) a row is being updated that is not related to the model at all. One could argue that also in this case (similar to firstOrCreate()), the implementation should automatically attach() a related model if it is not related already.

---

Alternatives
While I am convinced that the three proposed tests indicate a problem in behaviour, I am not convinced that my proposed implementation is actually the best improvement.

One objection would be, it adds a potential extra query to be executed. This could be circumvented by performing a query on $this->related-> and adding an extra select which determines whether the row is related or not. However, this might add a little too much complexity.

Looking at all different cases here, I ultimately feel that perhaps the new behaviour of returning (and attaching) unrelated rows should probably deserve their own dedicated methods, like firstOrCreateAndAttach() and updateOrCreateAndAttach() or something like that, leaving the existing Or methods like they were in Laravel 8.*. However, I do understand that might be a matter of taste.

Conclusion
Long story short, I am concerned that these three methods currently violate the invariant that all similar BelongsToMany methods always act on related data, and these three now do not behave in this same way. I can see what the intention behind the change might have been, but maybe that should have required a different solution? Currently I feel the behaviour can be rather unexpected, especially in some edge cases that are not unlikely to occur in applications. It could lead to hard-to-find bugs and/or vulnerabilities. I certainly do not want to push through any opinion of mine, but I would appreciate if you would give this issue a good thought, to make sure these methods (as seen as part of the greater whole) are evolving in the right direction.

[taylorotwell]

I think it's just unintuitive to me that firstOrNew would attach since findOrNew implies that nothing will actually be saved to the database, when in fact this change would mutate the database in certain situations. And, it's probably too big of a breaking change for firstOrNew to actually instantiate anything.

I think your changes to firstOrCreate and updateOrCreate are fine with me in general. I would leave firstOrNew as it is.

[taylorotwell]

Happy to revisit firstOrNew if you think you can convince me but for now will merge without those changes 😄

[marcovo]

@taylorotwell Thanks for carefully considering these issues.

I can expand a bit on firstOrNew, however I might not be able to convince you since I'm not convinced myself either about any approach here.

In the current code, firstOrNew can end up doing three things:

1. It returns an actual related record (related & existing)
2. It returns an unrelated existing record (not related & existing)
3. It returns a new record (not related & not existing; cannot be related either as by the nature of the method)

In the old situation (Laravel 8.x), you could at the call site distinguish between 1 and 3, by checking ->exists or ->wasRecentlyCreated. In the new situation, you can distinguish 2 from 3 but you cannot distinguish 2 from 1. That is, you cannot (without performing an extra query) determine whether a returned exists-ing record is of type 1 or 2. Hence, when saving a record obtained from firstOrNew, you must always attach() it to the base model for it to remain consistent, while in Laravel 8 you only had to do that if the related model had exists = false.

On the other hand, I do agree with you that the OrNew here implies that no database mutation should be expected. That makes this a difficult situation.

In addition, I feel that some people might find that firstOrNew unexpectedly returns an unrelated existing record in case 2. Some new suggestions could be:

1. An additional parameter $allowUnrelatedExisting = true can be added to firstOrNew, toggling whether to query $this->related or $this->clone()
2. An additional method firstRelatedOrNew could be added, exhibiting the old behaviour.
3. Perhaps using my suggested firstOrNew implementation without the attach() call? This would introduce the possibility to distinguish between situations 1 and 2 by including withPivot() in the relation builder chain.

However, each of these suggestions does have its drawbacks, I feel.

To sum up, while to me the *OrCreate methods were the most important, firstOrNew now still also feels a bit ill-defined. I have no concrete suggestion on what to do with it. Ultimately I feel the $this->related approach may be a bit misplaced here, perhaps deserving a separate method + implementation apart from the original methods. My main goal was to raise awareness on this issue and perhaps contribute a fix, I think this has succeeded. Any well-thought-through decision then is fine with me.

Thanks for your great work.