Closed alexrussell closed 11 years ago
Following discussion in IRC with @anlutro, the idea appears to be that Laravel should only be editing models from the side of the relationship that the developer is calling. So a way to remove from a hasMany
set is bad in that regard as you call it on Post
but the models affected are actually Comment
s due to the relationship being defined on them with the foreign key.
So if the proposal above generally goes against the idea of editing a different model to the one actually being referenced (I hear the logic, although I vehemently disagree with it), at least a disassociate
method on a belongsTo
relationship would eb useful - that doesn't break the rule, but saves the hacky (un-Eloquent) job of setting $model->foreign_id = null
way you'd have to do it.
Just set the foreign keys to null... I don't know. This sounds like a weird case of just detaching stuff from its parents.
I think this is a topic worth discussing. Simply doing a foreign_id = NULL
then model events aren't fired and timestamps don't get updated. Seems like there should be a better way than doing this manually.
I'm currently working on a piece of code, where I'd also like to see a way to disassociate/re-associate in a clean way. Due to the fact that this issue is still closed and other users don't seem to demand this heavily, I'm wondering whether my code is simply wrong and there are better ways to solve parent->child relation switches...
FWIW I think it's a great idea to have code that does this. I don't see anything inherently wrong with it and it adds to the eloquence of Eloquent to be able to detach/disassociate.
That said, as I mentioned above I do feel that to get the best out of this, the API should be updated to be more consistent, so you don't have to remember the different methods depending on your relationship type.
I think that the argument for not having this in Eloquent by default is that Eloquent seems to be against messing with the far side of a relationship (in general, at least). (See my first comment in this thread for details.) I don't really think it's a big deal to mess with your foreign model, though, personally. It's no different to doing $category->posts()->save($post)
which, while called on Category
, ends up setting category_id
on the instance of Post
.
I too would like to see more consistency in attaching relationships. I am constantly scouring documentation trying to remember which one to use. Let's just say it's not my favorite and file it next to the Laravel implementation of facades.
A disassociate method would also be appreciated, as it would fit with associate.
It seems like most of the laravel related cases I search for a solution to end in an unresolved closed issue.
Just saw that this is now actually implemented...
$category->post()->dissociate();
I just opened a pull request to add this to the docs...
Hurrah! I knew it was a good idea really. Just took Taylor some time to come round...
Hallelujah
It's actually possible since 4.2, just wasn't documented...
Unless I'm mistaken (I've looked in both the docs and the relevant
Relationship
classes), there isn't a way to 'detach' a child from a parent in a hasOne/hasMany/belongsTo relationship.Currently:
belongsToMany
has the ability toattach
anddetach
, as well assync
hasMany
only hassave
andsaveMany
hasOne
only hassave
andsaveMany
(although I feel thathasOne
should not havesaveMany
)belongsTo
only hasassociate
(and it must be passed an instance ofModel
)I feel that
hasMany
should also have thesync
method (why not?), as well as some method to remove a model from the set.Similarly,
belongsTo
should provide a way to remove the parent (setting the field toNULL
is the standard way of modelling this). My assumption was to maybe pass nothing (orfalse
ornull
) toassociate
but from the look of the method signature, aModel
instance must be passed to it. So maybe the method can be changed to either not force aModel
to be passed for removal of the 'parent'.Finally,
hasOne
is a bit weird at the moment - it currently acts just like hasMany (I understand, it's a 'has'-type relationship) but is actually more like abelongsTo
in terms of how you use it. So really, it should behave, from an API point of view, likebelongsTo
.Proposal (ideal, but seriously breaks current API):
belongsToMany
keepsattach
,detach
andsync
hasMany
should removesave
andsaveMany
in favour of havingattach
,detach
andsync
for continuity (attach and detach, when not using an array, can accept aModel
instance so it doesn't stop people using models entirely here)hasOne
should actually probably use theassociate
method as hasOne is a lot likebelongsTo
in that regard, albeit from the opposite end of the relationship. It should then also provide a way to sever the relationship, so seebelongsTo
proposal below.belongsTo
should either:associate
and allow the method to be called with no parameters to blank the field (like callingdetach
on abelongsToMany
relationship with no parameters)detach
could work here:$comment->post()->detach()
, or maybedisassociate
)Proposal (just adds to current API, but is less ideal):
belongsToMany
keepsattach
,detach
andsync
hasMany
should provide methods to remove 'child' models form itshasMany
set, not sure what they would be called though, as there's not really an inverse ofsave
hasOne
keepssave
andsaveMany
(as weird as that is) but also adds a method to break the relationship ashasMany
now does (see point above)belongsTo
should provide a method to simply sever the relationship which takes no parameters (seebelongsTo
proposal from first list)Sorry if this has been discussed before - I found it hard to think of the right search terms to search through the issues, but in the search I did do, I didn't see any discussion of this.
Also, I'd be happy to try to code something up for this, but I have a feeling the code involved is something best done by Taylor if the request/proposal is an acceptable one. I can certainly do a kind of detach/disassociate method for
belongsTo
, but the other proposals here would require quite a change to the way thehasMany
andhasOne
methods work (depending on the path chosen) and I'm not sure I'd necessarily do it quite the way Taylor would like!But first, let's discuss...