Allow opt-in to recursive unpublish

[chillu]

Acceptance Criteria

• Unpublish and delete only affect the record they're called on by default (regardless of ownership relations)
• Developers can opt-in to recursive unpublish and delete through explicit declarations (not inferred from ownership)
• Developers can opt in on a per-record, per-relationship level
• The behaviour can be applied both to versioned and non-versioned records
• The behaviour works with all types of relationships: has_one, has_many, many_many
• The behaviour tracks deletes in ChangeSets
• The behaviour is well documented and discoverable by developers

Notes

• Does not require separate behaviours for unpublish and delete
• Should not change existing ownership API
• Makes not assumptions about exclusivity of an ownership relation
• Excludes any UI warnings around the impact of an unpublish/delete action
• Assumes this does not require changes to the existing ChangeSet model tracking
• This was raised by @pitchandtone in GridField support for versioned records silverstripe-framework#7032 (comment)
• The reverse of this has already been implemented (an unpublished object will also unpublish it's owners) - see Don't unpublish owner when unpublishing owned object #25

Tasks

• Implement DataObject.cascade_deletes config
• Ensure ChangeSets adds deleted cascade_deletes (adding item to campaign and deleting it)
• Ensure "unpublish" respects cascade_deletes without changesets
• Update unit tests

PRs

• API Introduce cascade_deletes config feature for recursive delete and unpublish #33
• API Implement cascade_deletes silverstripe-framework#7257
• API Virtual pages now respect cascade_deletes on source page silverstripe-cms#1919

Related

• GridField support for versioned records

[chillu]

Actually I'm not sure if this is a good idea: Even for a straightforward has_one: File, unpublishing the file might have all kinds of side effects on other views. The same file can be in has_one, has_many and many_many relationships on other records, as well as have the file embedded as an image in HTML content on other records. Do we need a way to determine "exclusive" ownership relationships, and throw an error if you try to apply that to more than one of these to a given record type?

[pitchandtone]

If we were relying on has_one or has_many to implicitly control this, it would be dangerous, but if it's explicitly related to using the $owns static then developers are opting-in already.

$owns may need documentation around its power.

Alternatively, you could create multiple static owns_publishes, owns_unpublishes, but I suspect people will just use onAfters to write more complex custom code and avoid owns altogether.

[tractorcow]

So I did have a think about it, and there are two kinds of exclusive relationships:

• Relationships that can ONLY be exclusive. I.e. has_many (since the has_one on the other side can only point to one object).
• Relationships that MAY be currently exclusive. I.e. a many_many to an object which may currently not point to other belongs_many_many than this owner, but it just as easily could.

We probably need an unpublish control similar to below

class MyObject extends DataObject {
	/**
	 * Determine when to unpublish this object:
	 *   - Versioned::EXCLUSIVE_OWNER - Unpublished when only the exclusive owner is unpublished via has_many to this object (default)
	 *   - Versioned::SINGLE_OWNER - Unpublished when the last owner is unpublished
	 *   - Versioned::EXPLICIT - Object must be explicitly unpublished
	 */
	private static $unpublished_by = Versioned::EXCLUSIVE_OWNER;
}

We would set this to EXPLICIT for files, but the default for other dataobjects is exclusive_owner.

We could also use this to replace the unlinkDisownedObjects (doesn't unpublish has_many objects, but sets the has_one on the object to 0 when doing a partial publish). Essentially we would be auto-implement staging of deleted has_many.

Existing code below:

/**
     * Set foreign keys of has_many objects to 0 where those objects were
     * disowned as a result of a partial publish / unpublish.
     * I.e. this object and its owned objects were recently written to $targetStage,
     * but deleted objects were not.
     *
     * Note that this operation does not create any new Versions
     *
     * @param string $sourceStage Objects in this stage will not be unlinked.
     * @param string $targetStage Objects which exist in this stage but not $sourceStage
     * will be unlinked.
     */
    public function unlinkDisownedObjects($sourceStage, $targetStage) {}

[sminnee]

There's something pretty important there:

Auto-unpublish cascade === Auto-delete cascade

I think that focusing on unpublish in isolation is going to lead to an inconsistent model. "unpublish" really just mean "delete from live", after all.

Because of this I think that unpublished_by is the wrong name for the config here. We want something like cascade_deletes.

My preference is that the config goes on the parent object and not the child objects (so "cascade_deletes" and not "cascade_deletes_from") because the latter is too much like aspect-oriented programming.

If cascade_deletes isn't set, then the current behaviour of clearing the has_one record makes sense.

The behaviour of cascade_deletes on a many-many relationship would be a little tricky: my interpretation would be that if you set cascade_deletes on a many-many relationship, then the foreign object would be deleted when the it was linked to no more records on that relationship. I think as long as we're clear about the semantics, developers can decide whether to enable it.

[tractorcow]

Do you want it to apply to non-versioned objects? I had thought of moving ownership out of versioned, and making it something that all objects can use.

[sminnee]

Do you want it to apply to non-versioned objects?

I don't see that we have a choice without creating a fundamentally inconsistent model. Strictly speaking, "cascade_deletes" could be on the base ORM and "owns" could still be part of versioned, as owns controls the cascade of publications, which is a creation even and "cascade_create" doesn't mean any thing outside of a multi-stage data structure.

It's starting to feel like what we really want to do isn't going to be a non-breaking API.

Not really an option, and I don't think it's necessary.

[tractorcow]

It's starting to feel like what we really want to do isn't going to be a non-breaking API.

[tractorcow]

I don't see how something can (or should) cascade delete if it isn't owned.

[sminnee]

I don't see how something can cascade delete if it isn't owned.

By writing code that does it. :-P

If i understand your comment correctly, you mean more "it would seem inconsistent to have an owns property in our schema and not use it to control the behaviour of cascade deletes".

To which I'd say, sure, but:

• $owns is kind of misnamed anyway, it's non-exclusive status means something more like "contains".
• I'm not sure it's worth breaking the API here.
• Cascade deletes is a properly of database relationships and is pretty well understood in the DB community.

[tractorcow]

If i understand your comment correctly, you mean more "it would seem inconsistent to have an owns property in our schema and not use it to control the behaviour of cascade deletes".

What I mean is, I don't know why an object should cascade delete but not cascade publish. I thought cascades would be a subset of owns.

But back to what you are suggesting, you mean owns for publishing, cascade for deletes, and these are independent?

[sminnee]

What I mean is, I don't know why an object should cascade delete but not cascade publish. I thought cascades would be a subset of owns.

I agree that it would be more logical, but it's not worth breaking the API for.

But back to what you are suggesting, you mean owns for publishing, cascade for deletes, and these are independent?

Yes, as a way of introducing the required functionality without breaking APIs, I was suggesting that.

If you can come up with something that ties owns and cascade-deletes together, and doesn't break APIs, I'd support that.

[tractorcow]

ok, I think that sounds like a safe direction; One that doesn't need to rely on versioned right? Could it be a CasecadeDeleteExtension? (so it's not baked into dataobject)

[sminnee]

It could be an extension I supposed - would it be pulled in automatically by Versioned?

However I kind of see it as part of relational integrity and as such it could go into DataObject if needs be.

[tractorcow]

Yeah it could be a part of versioned, maybe, or just SilverStripe\ORM?

[tractorcow]

Question for @chillu

We have looked at two possible directions:

• explicit recursive unpublish; This is a feature of versioned
• explicit recursive delete; This is a feature of dataobject / ORM, and automatically provides unpublish as well.

I don't see either feature being significantly more difficult than one another; Recursive delete may be more useful in more cases, but will have a more significant effect on framework. Given that it'll be opt-in, it shouldn't have any unexpected consequences.

My preference is recursive delete, and we have a deletes config in exactly the same format as ownership, which is a list of strings pointing to relations that trigger delete.

@chillu if you are happy with this direction, I'll write up the new A/C in the body of this issue and write up tasks. Otherwise discuss. :)

[sminnee]

@chillu if you are happy with this direction, I'll write up the new A/C in the body of this issue and write up tasks. Otherwise discuss. :)

I think we should clearly separate "ACs" (which is what's already there) and "recommended implementation" (something new to write). I think that we risk losing context of why when we replace ACs with specific technical implementations.

[chillu]

I've added new ACs reflecting the state of discussion. Hopefully I haven't put too much implementation detail in there. In broad strokes, I think it should be opt-in, there should no inferring of cascades based on relationship/ownership type, and that we don't need to introduce a concept of "exclusive ownership" to make this work.

Original ACs for reference:

• When a record owns other records, calling Versioned->doUnpublish() also unpublishes child records
• Developers can choose to opt in on a per-record, per-relationship level
• This behaviour is well documented and discoverable by developers