search

silverstripe / silverstripe-versioned

Versioning for Silverstripe model
BSD 3-Clause "New" or "Revised" License
8 stars 36 forks source link

Allow opt-in to recursive unpublish #23

Closed chillu closed 7 years ago

chillu commented 7 years ago

Acceptance Criteria

Tasks

PRs

Related

chillu commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

tractorcow commented 7 years ago

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

sminnee commented 7 years ago

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:

tractorcow commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

tractorcow commented 7 years ago

Question for @chillu

We have looked at two possible directions:

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 commented 7 years ago

@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 commented 7 years ago

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: