VersionedCollector doesn't delete versions that were published in the past

[matthewrayner]

What is the reasoning behind this? I see that you mentioned isPublishedInLocale() as a reason but I don't see why.

I have SiteTree objects that have hundreds of different versions but as some of the versions were published they haven't been deleted even if they are hundreds of versions behind.

This is the line of code with the comment I mentioned above.

silverstripe-garbage-collector/src/Collectors/VersionedCollector.php

Lines 282 to 284 in d9a539e

	// Include only draft edits versions
	// as we don't want to delete publish versions because these drive isPublishedInLocale()
	$baseTable . '."WasPublished"' => 0,

[michalkleiner]

Maybe we can adjust the code comment to better explain the primary/basic intentions. My understanding of that is that we can safely remove draft versions (as defined by the delete/keep rules), and not touching any published versions. If you want to go beyond that, the module provides extension points where you can add custom items to the collections.

What's your view on that, @brettt89?

[brettt89]

The original code for this was pulled from a live project, so it was originally quite specific (though I did expand further with extensions etc). Personally my preference is to have the default be the "most common use-case". Having a method for enabling/disabling a filter on WasPublished (so these can be include/excluded as needed) kinda makes sense to me.

The extensions were intially added for support for Versioned extensions (e.g. Fluent) as not all projects use this, but the intent was always that others could also use these extensions to customize as needed.

There is probably a balance to be had here, but the way I kind look at is "If you can do it in Versioned module out of the box, then we should also support it out of the box" - (e.g via a switching method or something).

Does that make sense to you as well? Happy to hear other peoples opinions on this?

[brettt89]

The more I think about, @matthewrayner 's use-case might be the more common approach for Silverstripe websites. We originally took a very cautious approach to deleting these records (which is why WasPublished was always set to 0 as we didn't want to risk deleting anything that was live).

But being able to garbage collect old draft versions, for published pages seems like a pretty straight forward / common use-case for this module. I would probably say that if you wanted to keep ALL your draft content for published pages, that should be an "opt-in" modification (E.g. through extensions or maybe a enable/disable method).

[michalkleiner]

Yep, I'm all for flexibility if the defaults are safe — I'd only allow removing published versions after explicit config change by a dev.