silverstripe-version-truncator icon indicating copy to clipboard operation
silverstripe-version-truncator copied to clipboard

Does not work with SilverShop module

Open RVXD opened this issue 3 years ago • 4 comments

This module does not work in combination with the SilverShop module. https://github.com/silvershop

The class SilverShop\Model\Variation\Variation is a bit weird. It is versioned but is does not have a _Live table.

See file: https://github.com/silvershop/silvershop-core/blob/master/src/Model/Variation/Variation.php

This causes issues:

ERROR: Uncaught Exception SilverStripe\ORM\Connect\DatabaseException: "Couldn't run query:  SELECT "Version" FROM "SilverShop_Variation_Live" WHERE "ID" = ?  Table 'mydatabase.SilverShop_Variation_Live' doesn't exist"

Is there a way to exclude a model from truncation?

RVXD avatar Jan 07 '22 10:01 RVXD

Hmm, that's a good question @RVXD. It appears SilverShop\Model\Variation uses an odd implementation of Versioned whereby it has stages, but not the ability to save as draft or publish (I'm not sure what the logic is). You could try this in your config:

SilverShop\Model\Variation:
  keep_versions: 0
  keep_drafts: 0

That should skip version pruning altogether (untested) for that model. Ironically however I don't get any errors on my end, however it does not appear to even try truncate versions in Variation (keep in mind that I have literally never used silvershop before and just installed it for the first time). Please let me know if that config change helps?

axllent avatar Jan 07 '22 21:01 axllent

Hi Ralph, thanks for the suggestion. Tried it but didn't work.

Error is caused by the check on ->isLiveVersion();

You could add a check on this.

TruncateVersionTask.php

    private function _prune()
    {
        $classes = $this->_getAllVersionedDataClasses();

        DB::alteration_message('Pruning all DataObjects');

        $total = 0;

        foreach ($classes as $class) {
            $records = Versioned::get_by_stage($class, Versioned::DRAFT);
            $deleted = 0;
            echo($class . "\r\n");

            foreach ($records as $r) {
                // check if stages are present
                if( !$r->hasStages() ){
                    continue;
                }

                if ($r->isLiveVersion()) {
                    $deleted += $r->doVersionCleanup();
                }
            }

            if ($deleted > 0) {
                DB::alteration_message(
                    'Deleted ' . $deleted . ' versioned ' . $class . ' records'
                );

                $total += $deleted;
            }
        }

        DB::alteration_message('Completed, pruned ' . $total . ' records');
    }

Also ran into an error where I was using the Task. It tried to clean up a table: {TableName}__Live__Versions.

Can be fixed in VersionTruncator.php by setting reading mode

    public function doVersionCleanup()
    {
         // make sure Stage is used
        \SilverStripe\Versioned\Versioned::set_reading_mode('Stage.Stage');
        if( !$this->owner->hasStages() ) return;
...

I ended up using some custom code to clean up. I have a versions table with 3.5 million rows, so I ran into memory issues. Below will only select records that actually have multiple versions.

$cleanIds = DB::query("SELECT * FROM ( SELECT RecordID, COUNT(ID) AS VersionCount FROM `".$tableName."_Versions` GROUP BY RecordID ORDER BY VersionCount DESC ) AS t2 WHERE t2.VersionCount > 1;");

RVXD avatar Jan 12 '22 08:01 RVXD

Hi @RVXD, thanks for the feedback & suggestions. I understand the first two, however I'm confused with regards to the last suggestion with DB::query() (as to where you think that should be added). I would have thought that with 3.5 million records you would have been more likely to hit a PHP timeout (30 seconds) than a memory issue - are you sure it was memory that caused it to fail? If you could let me know where/how you think that should be added then I'll consider adding it too.

I've made a commit to the master branch (unreleased) which addresses the first two issues you raised. Does this change fix your issue with SilverShop?

axllent avatar Jan 14 '22 03:01 axllent

@RVXD Just following up here - have you managed to test this change?

axllent avatar May 04 '22 20:05 axllent