cascade_delete deletes end of relationship rather than join record #9612
Comments
|
Is this a dup of silverstripe/silverstripe-versioned#200? |
|
Without any core changes you should be able to create a 2nd has_many relationship on your data object that references the through-object instead of the foreign-object. You can then specify cascade_deletes on that relationship instead. I'd suggest doing this in the short-term because I feel like we're treading on dangerous territory and wouldn't want to squeeze a quick PR in. |
It's definitely related, but I'd say this bug is more focused in scope. It only concerns itself with the ORM functionality, not Versioned or GridField.
I'm not smart enough to think through the ramnifications across the entire ORM surface around adding a pseudo-has_many relationship for this ;) I could do this as a custom getter to avoid any ORM interference though. Anyway, it doesn't change the fact that this is a bug, right? We could clarify this workaround in the docs as a "known limitation" of course. |
|
There are no bugs or features in this land, just the code's implications :-P The current implementation matches what happens in many-many land AFAIK. I agree it's confusing and potentially dangerous, but not necessarily a bug. Or more, simply "fixing" it isn't necessarily going to cause other problems. |
|
One possible concern here: given that the "through" record is now a DataObject in its own right, can we say with absolute certainty that deleting it automatically is safe? I can’t think of a possible use case where you’d have multiple relations running through the same join record, but I like to throw spanners at things Would/could it be possible to delete the join records, but not the end record? |
|
This is why cascade_deletes needs to let developers configure these things. The question is how to distinguish a relation that returns the foreign objects from a relation that returns the intermediary objects. I'm not convinced that "(relname).Join" is the most intuitive way to refer to the intermediary relationship, as it makes it seem "further" than the foreign objects when it's actually "closer". If, for example, you were to traverse a list of the join objects you'd probably not link to the foreign object at all. It would behave exactly like a has-many, sense my suggestion to Ingo. On option would be to create a pair of relation methods for a manymany-through: "(relname)" and "(relname)Join". There a little bit of risk in such a string concatenation that someone overrides that relation name, but I think that's the kind of edge case that people use as a feature sometimes, so it's okay. Not that I'm not suggesting a mere hack for cascade_deletes. You could, for example, Chuck "(relname)Join" into a gridfield. I'm not sure what the purpose of that would be - my motivation is more to keep a consistent API. The other thing you could do is simply force users to manually create a has_many. Ingo was worried about side-effects of this, but I'm not - it's in keeping with the ORM's design to do that. |
To be clear, I'm not proposing that we change the current API or behaviour, even though I think it's not accounting for the most common scenario (deleting the join record rather than the other end of the relationship).
We've got
OK, I'll let you write the PR for this if you want to push for it as a solution ;) Lazy loading, caching, introspection, schema changes, ownership, snapshots, cascade_duplicates - it's just too much to think through for me, sorry. I'd also argue that it's counter-intuitive to developers - in 99% of cases, you'll want to ensure that join records are cleaned up when one side of the relationship is deleted, and this sounds like a lot of hoops to jump through to achieve that. Option 1: Built-in (object)->(relname)Joins() as pseudo has_many on cascade_deletesUse the ORM's built in logic to delete the through objects, with auto-mapped relationships. Wouldn't work on <?php
class ParentObject extends DataObject
{
private static $many_many = [
'Items' => [
'through' => ThroughObject::class,
'from' => 'Parent',
'to' => 'Child',
]
];
private static $cascade_deletes = [
'ItemsJoins'
];
}Option 2: Custom pseudo has_many on cascade_deletesLike Option 1, but leave it to devs to define this. <?php
class ParentObject extends DataObject
{
private static $many_many = [
'Items' => [
'through' => ThroughObject::class,
'from' => 'Parent',
'to' => 'Child',
]
];
private static $has_many = [
'ThroughObjects' => ThroughObject::class
];
private static $cascade_deletes = [
'ThroughObjects'
];
}Option 3: (object)->relname()->Joins() on cascade_deletesCreate new logic on the ManyManyThroughList. Wouldn't work on <?php
class ParentObject extends DataObject
{
private static $many_many = [
'Items' => [
'through' => ThroughObject::class,
'from' => 'Parent',
'to' => 'Child',
]
];
private static $cascade_deletes = [
'Items.Joins'
];
}Option 4: New onAfterDelete behaviour in coreDo what Damian suggested on a similar discussion around orphaned many_many relationships, and simply move this logic into <?php
class ParentObject extends DataObject
{
private static $many_many = [
'Items' => [
'through' => ThroughObject::class,
'from' => 'Parent',
'to' => 'Child',
]
];
// no cascade_deletes, feature is built-into onAfterWrite()
}Option 5: Custom getter on cascade_deletesRather than as <?php
class ParentObject extends DataObject
{
private static $many_many = [
'Items' => [
'through' => ThroughObject::class,
'from' => 'Parent',
'to' => 'Child',
]
];
private static $cascade_deletes = [
'ThroughObjects'
];
public function ThroughObjects()
{
return ThroughObject::get()->filter('ParentID', $this->ID);
}
} |
|
Somewhat related to this, I just found today that Option 2 is currently required for versioned many-many-through to work properly. Without the extra
More info on why that is the case: silverstripe/silverstripe-versioned#116 (comment) |
|
I came here to add to the conversation, but @sminnee already said everything I was about to say, to +1 to his evaluation. |
|
I'd say at a minimum, the current behaviour needs to be clearly documented which I don't think it currently is. I don't think we can make any functional changes outside of a major release. Going forward though, it's worth looking at our options. The intuitive thing for me (and this could be done in a major release) would be that There should be no reason for this configuration to differentiate between a basic |
This is a workaround until silverstripe/silverstripe-framework#9612 is resolved.
The docs claim that
cascade_deletessupports all types of relationships (inclmany_many). But in the case ofmany_manythrough, it deletes the end of that relationship rather than the join record. In the vast majority of cases, this won't be the correct behaviour.I'd suggest that we solve this by a new
ManyManyThroughList->getJoins()method which returns all the join DataObject records. And then incascade_deletes, we explicitly defineMyManyMany.Joins.See failing unit test below.
PRs
cascade_deletesinformation forhas_manyrelationships developer-docs#523 (CMS 5 docs)The text was updated successfully, but these errors were encountered: