silverstripe / developer-docs

Developer documentation for Silverstripe CMS
Other
6 stars 57 forks source link

keep_archived_assets docs could be better #17

Open emteknetnz opened 3 years ago

emteknetnz commented 3 years ago

There are multiple separate references to configuring file archiving via keep_archived_assets

I'm not sure how many of these you actually need to set to true in order to get keep_archived_assets to function correctly. Possibly some of these are previous implementations and no longer do anything. Seems like we should at have a single source of truth in the docs for how to enable this feature.

Undocumented:

https://github.com/silverstripe/silverstripe-assets/blob/1/src/AssetControlExtension.php#L47

    private static $keep_archived_assets = false;

Documented:

https://github.com/silverstripe/silverstripe-framework/blob/4/docs/en/02_Developer_Guides/14_Files/01_File_Management.md#configuring-file-archiving

SilverStripe\Assets\File:
  keep_archived_assets: true

https://github.com/silverstripe/silverstripe-framework/blob/4/docs/en/02_Developer_Guides/14_Files/03_File_Security.md#configuring-archive-behaviour

class MyVersiondObject extends DataObject 
{
    /** Ensure assets are archived along with the DataObject */
    private static $keep_archived_assets = true;

https://github.com/silverstripe/silverstripe-framework/blob/4/docs/en/02_Developer_Guides/14_Files/04_File_Storage.md#versioned-and-archived-files-archived

SilverStripe\Assets\Flysystem\FlysystemAssetStore:
  keep_archived_assets: true
emteknetnz commented 3 years ago

A quick test, indicative though not conclusive

        Config::modify()->set(File::class, 'keep_archived_assets', true); // works by itself
        Config::modify()->set(AssetControlExtension::class, 'keep_archived_assets', true); // works by itself
        Config::modify()->set(FlysystemAssetStore::class, 'keep_archived_assets', true); // does nothing
maxime-rainville commented 3 years ago

The optimal way of enabling keep archive would be to set keep_archived_assets to true on File. That's what the doc should say.

The FlysystemAssetStore way is non-sense. The AssetControlExtension way while working is probably more confusing than it needs to be.