fix: prevent cascade delete of OptionItem records by jsirish · Pull Request #420 · dynamic/foxystripe

jsirish · 2026-02-13T21:14:13Z

Summary

Fixes #419 — Unpublishing or deleting a ProductPage permanently destroys all associated OptionItem records and corrupts historical order data.

Root Cause

ProductPage::onBeforeDelete() checked $this->Status != 'Published' and unconditionally deleted all ProductOptions. In Silverstripe 5:

$this->Status is unreliable (not a real DB field on versioned objects)
OptionItem was unversioned, so deletion was permanent
OptionItem has a belongs_many_many to OrderDetail, so deletion corrupted order history

Changes

`ProductPage.php`

Removed the destructive onBeforeDelete() method that cascade-deleted all option items

`OptionItem.php`

Added Versioned extension so options follow the same publish/unpublish lifecycle as ProductPage
Added protective onBeforeDelete() guard that prevents deletion if the option is linked to any OrderDetail records (preserves order history integrity)

`PublishOptionItemsTask.php` (NEW)

BuildTask to publish all existing OptionItem records to the Live stage after the Versioned extension is applied
Must be run once after deploying this change: sake dev/tasks/publish-option-items

Post-Deploy Steps

After merging and deploying:

Run sake dev/build "flush=1" to create the OptionItem_Versions and OptionItem_Live tables
Run sake dev/tasks/publish-option-items to publish existing records to Live

Testing

PHPCS passes on all modified files
Manual verification of the protective guard logic

- Remove destructive onBeforeDelete() from ProductPage that unconditionally deleted all associated OptionItem records - Add Versioned extension to OptionItem so options follow the same publish/unpublish lifecycle as ProductPage - Add protective onBeforeDelete() guard on OptionItem to prevent deletion of records linked to historical OrderDetail records - Add PublishOptionItemsTask BuildTask to publish existing OptionItem records after Versioned extension is applied Fixes dynamic#419

Copilot

Pull request overview

This PR fixes a critical data loss bug where unpublishing or deleting ProductPages permanently destroyed all associated OptionItem records and corrupted historical order data. The root cause was a faulty onBeforeDelete() method in ProductPage that relied on the unreliable $this->Status property and unconditionally deleted all ProductOptions. Since OptionItem was unversioned and had a belongs_many_many relationship to OrderDetail, this permanently destroyed order history.

Changes:

Removed destructive cascade delete logic from ProductPage that was causing data loss
Added Versioned extension to OptionItem so options follow the same draft/live lifecycle as ProductPages
Added protective guard to prevent deletion of OptionItems linked to historical orders
Created migration task to publish existing OptionItem records after deploying the Versioned extension

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/Page/ProductPage.php	Removed destructive `onBeforeDelete()` logic that cascade-deleted ProductOptions; added missing backslash prefix to FoxyCart_Helper call for proper namespace resolution
src/Model/OptionItem.php	Added Versioned extension and protective `onBeforeDelete()` guard to preserve order history; fixed code style (cast operator spacing, indentation) to match codebase conventions
src/Migration/PublishOptionItemsTask.php	New BuildTask to publish all existing OptionItem records to Live stage after applying Versioned extension

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-13T21:19:53Z

+    public function onBeforeDelete()
+    {
+        parent::onBeforeDelete();
+
+        if ($this->OrderDetails()->exists()) {
+            throw new ValidationException(
+                'This option cannot be deleted as it is part of one or more past orders.'
+            );
+        }
+    }


The new protective guard in onBeforeDelete lacks test coverage. Consider adding a test that verifies: (1) OptionItems linked to OrderDetails cannot be deleted and throw a ValidationException, and (2) OptionItems not linked to any OrderDetails can be deleted successfully. Additionally, the existing testProductDraftOptionDeletion test in ProductPageTest.php may need updating as it expects OptionItems to be deleted when ProductPages are removed, which will no longer happen with the Versioned extension and removal of cascade delete behavior.

- Add ProductOptions to ProductPage $owns for cascade publish/unpublish - Remove redundant empty onBeforeDelete from ProductPage - Mirror OrderDetails guard in OptionItem::canDelete() for better CMS UX - Move validation before parent::onBeforeDelete() call - Fix trailing space in $extensions array - Add OptionItemTest with 10 test methods covering: - Versioned extension presence and publish/unpublish - canDelete guard for order-linked vs unlinked items - onBeforeDelete ValidationException for order-linked items - Cascade publish/unpublish via $owns - Options survive product deletion (core bug fix) - Update testProductDraftOptionDeletion to assert options survive deletion - Add ordered OptionItem fixture linked to OrderDetail

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-13T22:08:14Z

+            }
+            else {


The else statement should be on the same line as the closing brace, following the codebase convention of } else { rather than placing it on a new line. This pattern is consistently used throughout the codebase (e.g., in FoxyStripeController.php, FoxyCart.php, Order.php).

Suggested change

}

else {

} else {

Copilot · 2026-02-13T22:08:14Z

+        }
+        else {


The else statement should be on the same line as the closing brace, following the codebase convention of } else { rather than placing it on a new line. This pattern is consistently used throughout the codebase.

Suggested change

}

else {

} else {

jsirish · 2026-02-13T22:11:49Z

All three Copilot review findings have been addressed:

Test coverage for onBeforeDelete guard — Added comprehensive OptionItemTest.php with 10 test methods including:
- testDeleteThrowsExceptionWhenLinkedToOrder — verifies ValidationException is thrown
- testDeleteSucceedsWhenNotLinkedToOrder — verifies unlinked items can be deleted
- testCanDeleteReturnsFalseWhenLinkedToOrder / testCanDeleteReturnsTrueWhenNotLinkedToOrder
- Updated testProductDraftOptionDeletion to assert options survive product deletion
} else { formatting — Fixed in both ProductPage.php (line 430) and OptionItem.php (line 230) to follow codebase } else { convention.
Additional fixes from PAL review:
- Trailing space in $extensions array
- Unused import removed from test file
- Trailing spaces before commas in test file

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-13T22:19:41Z

+        $option = $this->objFromFixture(OptionItem::class , 'large');
+
+        $this->assertTrue(
+            $option->hasExtension(Versioned::class),
+            'OptionItem should have the Versioned extension'
+        );
+    }
+
+    /**
+     * Test that OptionItem can be published to the Live stage.
+     */
+    public function testOptionItemCanBePublished()
+    {
+        $this->logInWithPermission('ADMIN');
+
+        $option = $this->objFromFixture(OptionItem::class , 'large');
+        $option->publishSingle();
+
+        $this->assertTrue(
+            $option->isPublished(),
+            'OptionItem should be publishable'
+        );
+
+        $liveOption = Versioned::get_by_stage(
+            OptionItem::class ,
+            Versioned::LIVE
+        )->byID($option->ID);
+
+        $this->assertNotNull(
+            $liveOption,
+            'OptionItem should exist on the Live stage after publishing'
+        );
+        $this->assertEquals($option->Title, $liveOption->Title);
+    }
+
+    /**
+     * Test that OptionItem can be unpublished.
+     */
+    public function testOptionItemCanBeUnpublished()
+    {
+        $this->logInWithPermission('ADMIN');
+
+        $option = $this->objFromFixture(OptionItem::class , 'small');
+        $option->publishSingle();
+
+        $this->assertTrue($option->isPublished());
+
+        $option->doUnpublish();
+
+        $this->assertFalse(
+            $option->isPublished(),
+            'OptionItem should no longer be published after unpublishing'
+        );
+    }
+
+    /**
+     * Test that canDelete returns false for OptionItems linked to orders.
+     */
+    public function testCanDeleteReturnsFalseWhenLinkedToOrder()
+    {
+        $this->logInWithPermission('Product_CANCRUD');
+
+        $option = $this->objFromFixture(OptionItem::class , 'ordered');
+
+        $this->assertTrue(
+            $option->OrderDetails()->exists(),
+            'The "ordered" OptionItem should be linked to OrderDetails'
+        );
+
+        $this->assertFalse(
+            $option->canDelete(),
+            'canDelete should return false for OptionItems linked to orders'
+        );
+    }
+
+    /**
+     * Test that canDelete returns true for OptionItems with no orders.
+     */
+    public function testCanDeleteReturnsTrueWhenNotLinkedToOrder()
+    {
+        $this->logInWithPermission('Product_CANCRUD');
+
+        $option = $this->objFromFixture(OptionItem::class , 'large');
+
+        $this->assertFalse(
+            $option->OrderDetails()->exists(),
+            'The "large" OptionItem should not be linked to OrderDetails'
+        );
+
+        $this->assertTrue(
+            $option->canDelete(),
+            'canDelete should return true for OptionItems not linked to orders'
+        );
+    }
+
+    /**
+     * Test that onBeforeDelete throws ValidationException for order-linked items.
+     */
+    public function testDeleteThrowsExceptionWhenLinkedToOrder()
+    {
+        $this->logInWithPermission('ADMIN');
+
+        $option = $this->objFromFixture(OptionItem::class , 'ordered');
+
+        $this->expectException(ValidationException::class);
+        $this->expectExceptionMessage('This option cannot be deleted as it is part of one or more past orders.');
+
+        $option->delete();
+    }
+
+    /**
+     * Test that OptionItems without orders can be deleted normally.
+     */
+    public function testDeleteSucceedsWhenNotLinkedToOrder()
+    {
+        $this->logInWithPermission('Product_CANCRUD');
+
+        $option = $this->objFromFixture(OptionItem::class , 'small');
+        $optionID = $option->ID;
+
+        $this->assertFalse($option->OrderDetails()->exists());
+
+        $option->delete();
+
+        $this->assertNull(
+            OptionItem::get()->byID($optionID),
+            'OptionItem should be deleted when not linked to orders'
+        );
+    }
+
+    /**
+     * Test that publishing a ProductPage cascades to its OptionItems
+     * via the $owns relationship.
+     */
+    public function testProductPagePublishCascadesToOptionItems()
+    {
+        $this->logInWithPermission('ADMIN');
+
+        $holder = $this->objFromFixture(ProductHolder::class , 'default');
+        $holder->publishRecursive();
+
+        $product = $this->objFromFixture(ProductPage::class , 'product1');
+        $product->publishRecursive();
+
+        $this->assertTrue($product->isPublished());
+
+        // Check that owned OptionItems are also published
+        $option = $this->objFromFixture(OptionItem::class , 'large');
+        $liveOption = Versioned::get_by_stage(
+            OptionItem::class ,
+            Versioned::LIVE
+        )->byID($option->ID);
+
+        $this->assertNotNull(
+            $liveOption,
+            'OptionItem should be published when ProductPage is published recursively'
+        );
+    }
+
+    /**
+     * Test that unpublishing a ProductPage cascades to its OptionItems.
+     */
+    public function testProductPageUnpublishCascadesToOptionItems()
+    {
+        $this->logInWithPermission('ADMIN');
+
+        $holder = $this->objFromFixture(ProductHolder::class , 'default');
+        $holder->publishRecursive();
+
+        $product = $this->objFromFixture(ProductPage::class , 'product1');
+        $product->publishRecursive();
+
+        $option = $this->objFromFixture(OptionItem::class , 'large');
+
+        // Verify option is published
+        $this->assertTrue(
+            $option->isPublished(),
+            'OptionItem should be published after ProductPage publishRecursive'
+        );
+
+        // Unpublish the product
+        $product->doUnpublish();
+
+        // Refresh the option from DB
+        $option = OptionItem::get()->byID($option->ID);
+        $liveOption = Versioned::get_by_stage(
+            OptionItem::class ,
+            Versioned::LIVE
+        )->byID($option->ID);
+
+        $this->assertNull(
+            $liveOption,
+            'OptionItem should be unpublished when ProductPage is unpublished'
+        );
+    }
+
+    /**
+     * Test that deleting a ProductPage does NOT cascade-delete its OptionItems.
+     * This is the core bug fix — options must survive product deletion.
+     */
+    public function testProductPageDeleteDoesNotDeleteOptionItems()
+    {
+        $this->logInWithPermission('ADMIN');
+
+        $product = $this->objFromFixture(ProductPage::class , 'product1');
+        $option = $this->objFromFixture(OptionItem::class , 'large');
+        $optionID = $option->ID;
+
+        // Delete the product
+        $product->delete();
+
+        // The option should still exist
+        $remainingOption = OptionItem::get()->byID($optionID);
+
+        $this->assertNotNull(
+            $remainingOption,
+            'OptionItem should NOT be deleted when its ProductPage is deleted'
+        );
+    }
+}


Style inconsistency: Space before comma. The existing codebase uses no space before commas (e.g., OptionGroup::class, 'size'). Please remove spaces before commas in all objFromFixture calls to maintain consistency with the rest of the test files.

Copilot · 2026-02-13T22:19:41Z

+    private static $extensions = [
+        Versioned::class ,
+    ];
    /**
     * @var array
     */
    private static $db = array(
        'Title' => 'Text',
        'WeightModifier' => 'Decimal',
        'CodeModifier' => 'Text',
        'PriceModifier' => 'Currency',
        'WeightModifierAction' => "Enum('Add,Subtract,Set','Add')",
        'CodeModifierAction' => "Enum('Add,Subtract,Set','Add')",
        'PriceModifierAction' => "Enum('Add,Subtract,Set','Add')",
        'Available' => 'Boolean',
        'SortOrder' => 'Int',
    );

    /**
     * @var array
     */
    private static $has_one = array(
-        'Product' => ProductPage::class,
-        'ProductOptionGroup' => OptionGroup::class,
+        'Product' => ProductPage::class ,
+        'ProductOptionGroup' => OptionGroup::class ,
    );

    /**
     * @var array
     */
    private static $belongs_many_many = array(
-        'OrderDetails' => OrderDetail::class,
+        'OrderDetails' => OrderDetail::class ,
    );


Style inconsistency: Space before comma after ::class. The existing codebase consistently uses no space before commas in class references (e.g., ProductPage::class,). Please remove the space before the comma in the $extensions, $has_one, and $belongs_many_many arrays to maintain consistency.

Copilot · 2026-02-13T22:19:42Z

+        }
+        else {


Style inconsistency: else should be on the same line as the closing brace. The existing codebase consistently uses } else { (closing brace, space, else, space, opening brace on the same line), but this change introduces else { on a new line. Please change to } else { to maintain consistency.

Suggested change

}

else {

} else {

Copilot · 2026-02-13T22:19:42Z

+        $default = $this->objFromFixture(ProductCategory::class , 'default');
+        $holder = $this->objFromFixture(ProductHolder::class , 'default');
+        $product1 = $this->objFromFixture(ProductPage::class , 'product1');


Style inconsistency: Space before comma in objFromFixture calls. The existing codebase uses no space before commas (e.g., OptionGroup::class, 'size'), but these changes introduce a space before the comma (e.g., ProductCategory::class , 'default'). Please remove the space before the comma to maintain consistency with the rest of the test files.

Copilot · 2026-02-13T22:19:42Z

@@ -353,13 +362,13 @@
     */
    public function getGeneratedTitle()
    {
-        $modPrice = ($this->PriceModifier) ? (string) $this->PriceModifier : '0';
+        $modPrice = ($this->PriceModifier) ? (string)$this->PriceModifier : '0';


Style inconsistency: Type casts should have a space after them. The existing codebase consistently uses (string) $variable with a space after the type cast, but these changes introduce (string)$variable without a space. Please add a space after the type casts to maintain consistency.

Copilot · 2026-02-13T22:19:42Z

+        $modPrice = ($this->PriceModifier) ? (string)$this->PriceModifier : '0';
        $title = $this->Title;
-        $title .= ($this->PriceModifier != 0) ?
+        $title .= ($this->PriceModifier != 0) ? 


Trailing whitespace detected at the end of the line. This should be removed as it's generally considered bad practice and can cause issues with version control and code linters.

Suggested change

$title .= ($this->PriceModifier != 0) ?

$title .= ($this->PriceModifier != 0) ?

Copilot · 2026-02-13T22:19:43Z

+                }
                    ;


Formatting issue: The closing brace and semicolon should be on the same line as the closing brace, or the semicolon should be on the same line as the closing brace. The current formatting with the semicolon on a separate line with extra indentation is unconventional and makes the code less readable. Please reformat to match standard PHP conventions.

Suggested change

}

;

};

Copilot · 2026-02-13T22:19:43Z

+            $this->PriceModifierAction,
+            $returnWithOnlyPlusMinus = true


Formatting degradation: The indentation of the function parameters has been reduced, making the code less readable. The parameters on lines 369-370 should be indented to align with the opening of the function call. Please restore proper indentation.

Suggested change

$this->PriceModifierAction,

$returnWithOnlyPlusMinus = true

$this->PriceModifierAction,

$returnWithOnlyPlusMinus = true

jsirish · 2026-02-13T22:22:45Z

Copilot Re-Review (Review 3) — Disposition

All 8 new comments from the latest Copilot review are about pre-existing code style inconsistencies, not issues introduced by this PR:

#	Finding	Disposition
4	Space before comma in `OptionItemTest.php` objFromFixture	Pre-existing convention — matches `ProductPageTest.php` style
5	Space before comma in `OptionItem.php` `$extensions`/`$has_one`/`$belongs_many_many`	Pre-existing — 10+ occurrences across `src/`
6	`} else {` formatting in `OptionItem.php:230`	✅ Already fixed in this commit
7	Space before comma in `ProductPageTest.php:37`	Pre-existing — not touched by this PR
8	Type cast spacing `(string)$variable`	Pre-existing code on untouched line
9	Trailing whitespace `OptionItem.php:367`	Pre-existing code on untouched line
10	Semicolon formatting `ProductPage.php:237`	Pre-existing code on untouched line
11	Parameter indentation `OptionItem.php:370`	Pre-existing code on untouched line

All pre-existing style issues tracked in #421 for a dedicated cleanup pass.

…ion; add regression tests

Copilot

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-13T23:18:11Z

+        $option = $this->objFromFixture(OptionItem::class , 'large');
+
+        $this->assertTrue(
+            $option->hasExtension(Versioned::class),
+            'OptionItem should have the Versioned extension'
+        );
+    }
+
+    /**
+     * Test that OptionItem can be published to the Live stage.
+     */
+    public function testOptionItemCanBePublished()
+    {
+        $this->logInWithPermission('ADMIN');
+
+        $option = $this->objFromFixture(OptionItem::class , 'large');
+        $option->publishSingle();
+
+        $this->assertTrue(
+            $option->isPublished(),
+            'OptionItem should be publishable'
+        );
+
+        $liveOption = Versioned::get_by_stage(
+            OptionItem::class ,
+            Versioned::LIVE
+        )->byID($option->ID);
+
+        $this->assertNotNull(
+            $liveOption,
+            'OptionItem should exist on the Live stage after publishing'
+        );
+        $this->assertEquals($option->Title, $liveOption->Title);
+    }
+
+    /**
+     * Test that OptionItem can be unpublished.
+     */
+    public function testOptionItemCanBeUnpublished()
+    {
+        $this->logInWithPermission('ADMIN');
+
+        $option = $this->objFromFixture(OptionItem::class , 'small');
+        $option->publishSingle();
+
+        $this->assertTrue($option->isPublished());
+
+        $option->doUnpublish();
+
+        $this->assertFalse(
+            $option->isPublished(),
+            'OptionItem should no longer be published after unpublishing'
+        );
+    }
+
+    /**
+     * Test that canDelete returns false for OptionItems linked to orders.
+     */
+    public function testCanDeleteReturnsFalseWhenLinkedToOrder()
+    {
+        $this->logInWithPermission('Product_CANCRUD');
+
+        $option = $this->objFromFixture(OptionItem::class , 'ordered');
+
+        $this->assertTrue(
+            $option->OrderDetails()->exists(),
+            'The "ordered" OptionItem should be linked to OrderDetails'
+        );
+
+        $this->assertFalse(
+            $option->canDelete(),
+            'canDelete should return false for OptionItems linked to orders'
+        );
+    }
+
+    /**
+     * Test that canDelete returns true for OptionItems with no orders.
+     */
+    public function testCanDeleteReturnsTrueWhenNotLinkedToOrder()
+    {
+        $this->logInWithPermission('Product_CANCRUD');
+
+        $option = $this->objFromFixture(OptionItem::class , 'large');
+
+        $this->assertFalse(
+            $option->OrderDetails()->exists(),
+            'The "large" OptionItem should not be linked to OrderDetails'
+        );
+
+        $this->assertTrue(
+            $option->canDelete(),
+            'canDelete should return true for OptionItems not linked to orders'
+        );
+    }
+
+    /**
+     * Test that onBeforeDelete throws ValidationException for order-linked items.
+     */
+    public function testDeleteThrowsExceptionWhenLinkedToOrder()
+    {
+        $this->logInWithPermission('ADMIN');
+
+        $option = $this->objFromFixture(OptionItem::class , 'ordered');
+
+        $this->expectException(ValidationException::class);
+        $this->expectExceptionMessage('This option cannot be deleted as it is part of one or more past orders.');
+
+        $option->delete();
+    }
+
+    /**
+     * Test that OptionItems without orders can be deleted normally.
+     */
+    public function testDeleteSucceedsWhenNotLinkedToOrder()
+    {
+        $this->logInWithPermission('Product_CANCRUD');
+
+        $option = $this->objFromFixture(OptionItem::class , 'small');
+        $optionID = $option->ID;
+
+        $this->assertFalse($option->OrderDetails()->exists());
+
+        $option->delete();
+
+        $this->assertNull(
+            OptionItem::get()->byID($optionID),
+            'OptionItem should be deleted when not linked to orders'
+        );
+    }
+
+    /**
+     * Test that publishing a ProductPage cascades to its OptionItems
+     * via the $owns relationship.
+     */
+    public function testProductPagePublishCascadesToOptionItems()
+    {
+        $this->logInWithPermission('ADMIN');
+
+        $holder = $this->objFromFixture(ProductHolder::class , 'default');
+        $holder->publishRecursive();
+
+        $product = $this->objFromFixture(ProductPage::class , 'product1');
+        $product->publishRecursive();
+
+        $this->assertTrue($product->isPublished());
+
+        // Check that owned OptionItems are also published
+        $option = $this->objFromFixture(OptionItem::class , 'large');
+        $liveOption = Versioned::get_by_stage(
+            OptionItem::class ,
+            Versioned::LIVE
+        )->byID($option->ID);
+
+        $this->assertNotNull(
+            $liveOption,
+            'OptionItem should be published when ProductPage is published recursively'
+        );
+    }
+
+    /**
+     * Test that unpublishing a ProductPage cascades to its OptionItems.
+     */
+    public function testProductPageUnpublishCascadesToOptionItems()
+    {
+        $this->logInWithPermission('ADMIN');
+
+        // Create manual objects to bypass fixture loading issues
+        $holder = ProductHolder::create();
+        $holder->Title = 'Holder';
+        $holder->URLSegment = 'holder';
+        $holder->write();
+        $holder->publishRecursive();
+
+        $product = ProductPage::create();
+        $product->Title = 'Product';
+        $product->URLSegment = 'product';
+        $product->ParentID = $holder->ID;
+        $product->write();
+        $product->publishRecursive();
+
+        $option = OptionItem::create();
+        $option->Title = 'Option';
+        $option->ProductID = $product->ID;
+        $option->write();
+
+        // Ensure option is published via the product
+        $product->publishRecursive();
+
+        // Verify option is published
+        $this->assertTrue(
+            $option->isPublished(),
+            'OptionItem should be published after ProductPage publishRecursive'
+        );
+
+        // Unpublish the product
+        $product->doUnpublish();
+
+        // Refresh the option from DB
+        $liveOption = Versioned::get_by_stage(
+            OptionItem::class ,
+            Versioned::LIVE
+        )->byID($option->ID);
+
+        $this->assertNull(
+            $liveOption,
+            'OptionItem should be unpublished when ProductPage is unpublished'
+        );
+    }
+
+    /**
+     * Test that deleting a ProductPage does NOT cascade-delete its OptionItems.
+     * This is the core bug fix — options must survive product deletion.
+     */
+    public function testProductPageDeleteDoesNotDeleteOptionItems()
+    {
+        $this->logInWithPermission('ADMIN');
+
+        $product = $this->objFromFixture(ProductPage::class , 'product1');
+        $option = $this->objFromFixture(OptionItem::class , 'large');


Inconsistent spacing: Add a space after the comma, not before. The codebase convention is objFromFixture(ClassName::class, 'fixture') and Versioned::get_by_stage(ClassName::class, Stage) without a space before the comma. This applies to all such calls in this file.

Copilot · 2026-02-13T23:18:11Z

+     * @var array
+     */
+    private static $extensions = [
+        ProductPageExtension::class ,


Inconsistent spacing: Remove space before commas in array definitions. While ProductPage.php appears to have pre-existing spacing inconsistencies, the new line being added should follow the more common codebase convention of 'Key' => ClassName::class, without a space before the comma (as seen in src/Model/Order.php, src/Model/OrderDetail.php, and other files).

Suggested change

ProductPageExtension::class ,

ProductPageExtension::class,

Copilot · 2026-02-13T23:18:11Z

+    /**
+     * @var array
+     */
+    private static $extensions = [
+        ProductPageExtension::class ,
+    ];


Unusual placement of $extensions configuration. By convention, static configuration properties like $extensions should be defined near the top of the class with other static properties (after class declaration, before instance properties and methods). Placing it between onBeforeWrite() and onAfterWrite() methods is unconventional and makes it harder to find. Consider moving it to the top of the class with other static configuration arrays.

Copilot · 2026-02-13T23:18:12Z

+
+class OptionItemTest extends SapphireTest
+{
+


The test class is missing a protected static $fixture_file = 'fixtures.yml'; declaration. Without this, the test will fail because objFromFixture() calls won't be able to load the required test data. Add this property declaration near the top of the class, similar to other test classes in the codebase.

Suggested change

protected static $fixture_file = 'fixtures.yml';

Copilot · 2026-02-13T23:18:12Z

@@ -43,9 +43,9 @@ public function testProductCreation()
    public function testProductDeletion()
    {
        $this->logInWithPermission('Product_CANCRUD');
-        $holder = $this->objFromFixture(ProductHolder::class, 'default');
+        $holder = $this->objFromFixture(ProductHolder::class , 'default');
        $holder->write();
-        $product2 = $this->objFromFixture(ProductPage::class, 'product2');
+        $product2 = $this->objFromFixture(ProductPage::class , 'product2');
        $productID = $product2->ID;

        $product2->write();
@@ -76,10 +76,10 @@ public function testProductTitleLeadingWhiteSpace()
    {
        $this->logInWithPermission('ADMIN');

-        $holder = $this->objFromFixture(ProductHolder::class, 'default');
+        $holder = $this->objFromFixture(ProductHolder::class , 'default');
        $holder->write();

-        $product = $this->objFromFixture(ProductPage::class, 'product1');
+        $product = $this->objFromFixture(ProductPage::class , 'product1');
        $product->Title = ' Test with leading space';
        $product->write();

@@ -93,10 +93,10 @@ public function testProductTitleTrailingWhiteSpace()
    {
        $this->logInWithPermission('ADMIN');

-        $holder = $this->objFromFixture(ProductHolder::class, 'default');
+        $holder = $this->objFromFixture(ProductHolder::class , 'default');
        $holder->write();

-        $product = $this->objFromFixture(ProductPage::class, 'product1');
+        $product = $this->objFromFixture(ProductPage::class , 'product1');
        $product->Title = 'Test with trailing space ';
        $product->write();

@@ -106,7 +106,7 @@ public function testProductTitleTrailingWhiteSpace()
    public function testProductCategoryCreation()
    {
        $this->logInWithPermission('Product_CANCRUD');
-        $category = $this->objFromFixture(ProductCategory::class, 'apparel');
+        $category = $this->objFromFixture(ProductCategory::class , 'apparel');
        $categoryID = $category->ID;

        $productCategory = ProductCategory::get()->filter(array('Code' => 'APPAREL'))->first();
@@ -118,11 +118,11 @@ public function testProductCategoryDeletion()
    {
        $this->logInWithPermission('Product_CANCRUD');

-        $category = $this->objFromFixture(ProductCategory::class, 'default');
+        $category = $this->objFromFixture(ProductCategory::class , 'default');

        $this->assertFalse($category->canDelete());

-        $category2 = $this->objFromFixture(ProductCategory::class, 'apparel');
+        $category2 = $this->objFromFixture(ProductCategory::class , 'apparel');
        $category2ID = $category2->ID;

        $this->assertTrue($category2->canDelete());
@@ -146,7 +146,7 @@ public function testOptionGroupCreation()
    {
        $this->logInWithPermission('Product_CANCRUD');

-        $group = $this->objFromFixture(OptionGroup::class, 'size');
+        $group = $this->objFromFixture(OptionGroup::class , 'size');
        $group->write();

        $this->assertNotNull(OptionGroup::get()->first());
@@ -155,7 +155,7 @@ public function testOptionGroupCreation()
    public function testOptionGroupDeletion()
    {
        $this->logInWithPermission('ADMIN');
-        $group = $this->objFromFixture(OptionGroup::class, 'color');
+        $group = $this->objFromFixture(OptionGroup::class , 'color');
        $group->write();
        $groupID = $group->ID;

@@ -176,7 +176,7 @@ public function testOptionItemCreation()

        $optionGroup = OptionGroup::get()->filter(array('Title' => 'Sample-Group'))->first();

-        $option = $this->objFromFixture(OptionItem::class, 'large');
+        $option = $this->objFromFixture(OptionItem::class , 'large');
        $option->ProductOptionGroupID = $optionGroup->ID;
        $option->write();

@@ -191,10 +191,10 @@ public function testOptionItemDeletion()
    {
        $this->logInWithPermission('ADMIN');

-        $optionGroup = $this->objFromFixture(OptionGroup::class, 'size');
+        $optionGroup = $this->objFromFixture(OptionGroup::class , 'size');
        $optionGroup->write();

-        $option = $this->objFromFixture(OptionItem::class, 'small');
+        $option = $this->objFromFixture(OptionItem::class , 'small');
        $option->write();

        $optionID = $option->ID;
@@ -216,21 +216,21 @@ public function testProductDraftOptionDeletion()

        $this->logInWithPermission('ADMIN');

-        $holder = $this->objFromFixture(ProductHolder::class, 'default');
+        $holder = $this->objFromFixture(ProductHolder::class , 'default');
        //build holder page, ProductPage can't be on root level
        $holder->publishRecursive();

-        $product = $this->objFromFixture(ProductPage::class, 'product1'); //build product page
+        $product = $this->objFromFixture(ProductPage::class , 'product1'); //build product page
        $product->publishRecursive();

        $productID = $product->ID;

-        $optionGroup = $this->objFromFixture(OptionGroup::class, 'size');
+        $optionGroup = $this->objFromFixture(OptionGroup::class , 'size');
        //build the group for the options
        $optionGroup->write();
-        $option = $this->objFromFixture(OptionItem::class, 'small'); //build first option
+        $option = $this->objFromFixture(OptionItem::class , 'small'); //build first option
        $option->write();
-        $option2 = $this->objFromFixture(OptionItem::class, 'large'); //build second option
+        $option2 = $this->objFromFixture(OptionItem::class , 'large'); //build second option
        $option2->write();

        $this->assertTrue($product->isPublished()); //check that product is published
@@ -239,7 +239,7 @@ public function testProductDraftOptionDeletion()

        $this->assertTrue($product->isPublished()); //check product is still published

-        $testOption = $this->objFromFixture(OptionItem::class, 'large');
+        $testOption = $this->objFromFixture(OptionItem::class , 'large');


Inconsistent spacing: Add a space after the comma, not before. The codebase convention is objFromFixture(ClassName::class, 'fixture') without a space before the comma. This applies to all objFromFixture calls in this file that were modified to include a space before the comma.

…Modifieraction since SS4 refactor)

jsirish · 2026-02-17T19:26:16Z

Requesting Copilot code review for this fix. The main changes are: 1) Added Versioned extension to OptionItem, 2) Removed destructive onBeforeDelete from ProductPage, 3) Added protective canDelete guard on OptionItem, 4) Created ProductPageExtension for cascade unpublish, 5) Added PublishOptionItemsTask migration task. Also fixed a pre-existing typo in test fixtures (CodeModifieraction -> CodeModifierAction).

Copilot

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

src/Model/OptionItem.php:460

OptionItem class is missing its final closing brace. The file currently ends after canCreate() without a closing } for the class, which will cause a PHP parse error.

    public function canCreate($member = null, $context = [])
    {
        return Permission::check('Product_CANCRUD');
    }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-17T19:59:44Z

            $fields->addFieldsToTab('Root.Modifiers', [
                HeaderField::create('ModifyHD', _t(
-                    'OptionItem.ModifyHD',
-                    'Product Option Modifiers'
-                ), 2),
+                'OptionItem.ModifyHD',
+                'Product Option Modifiers'
+            ), 2),


The modifier-field setup here depends on $parentPrice/$parentWeight/$parentCode coming from a ProductPage record. Since this PR intentionally allows OptionItem to outlive a deleted ProductPage, ProductPage::get()->byID($this->ProductID) can become null and the earlier $product->... dereferences will fatal. Add a null-check for the product record (and fall back to defaults / skip modifier fields) when the product no longer exists.

Copilot · 2026-02-17T19:59:44Z

    public function canDelete($member = null)
    {
+        if ($this->OrderDetails()->exists()) {
+            return false;
+        }
+
        return Permission::check('Product_CANCRUD');
    }


canDelete($member = null) ignores the $member argument and checks the currently logged-in user only. Other permission methods in this module commonly use Permission::check('Product_CANCRUD', 'any', $member); consider switching to that pattern here so permission checks work correctly when a specific member is passed in.

Copilot · 2026-02-17T19:59:44Z

+use SilverStripe\Dev\SapphireTest;
+
+class OptionItemTest extends SapphireTest
+{
+
+    protected static $use_draft_site = true;


This test class extends SapphireTest but uses logInWithPermission() and objFromFixture(). logInWithPermission() is provided by FunctionalTest, and fixture setup typically requires defining $fixture_file (or extending the existing FS_Test base test). As-is, these tests will fail due to missing helper and/or fixtures.

Suggested change

use SilverStripe\Dev\SapphireTest;

class OptionItemTest extends SapphireTest

{

protected static $use_draft_site = true;

use SilverStripe\Dev\FunctionalTest;

class OptionItemTest extends FunctionalTest

{

protected static $use_draft_site = true;

protected static $fixture_file = 'OptionItemTest.yml';

Copilot · 2026-02-17T19:59:45Z

+        $checkSurvived = OptionItem::get()->filter(array('Title' => 'Large'))->first();
+        //query same option as above - it should survive product deletion

-        $this->assertNull($checkDeleted, 'Option should be deleted when product is fully removed');
+        $this->assertNotNull($checkSurvived, 'Option should survive when product is fully removed (preserves order history)');


This assertion queries for an OptionItem by Title only, which can become ambiguous if multiple options share the same title (or if other tests create additional records). Since the intent is to confirm the specific option survived product deletion, capture the option's ID before deletion and assert by ID instead.

Copilot AI review requested due to automatic review settings February 13, 2026 21:14

Copilot started reviewing on behalf of jsirish February 13, 2026 21:14 View session

Copilot AI reviewed Feb 13, 2026

View reviewed changes

jsirish requested a review from Copilot February 13, 2026 22:01

Copilot started reviewing on behalf of jsirish February 13, 2026 22:02 View session

Copilot AI reviewed Feb 13, 2026

View reviewed changes

style: fix trailing spaces and unused import per PAL review

ed29c05

jsirish requested a review from Copilot February 13, 2026 22:11

Copilot started reviewing on behalf of jsirish February 13, 2026 22:12 View session

Copilot AI reviewed Feb 13, 2026

View reviewed changes

jsirish mentioned this pull request Feb 13, 2026

Code style cleanup: pre-existing inconsistencies flagged by Copilot #421

Open

fix(ProductPage): cascade unpublish OptionItems via ProductPageExtens…

e47285d

…ion; add regression tests

jsirish requested a review from Copilot February 13, 2026 23:11

Copilot started reviewing on behalf of jsirish February 13, 2026 23:12 View session

style: Fix code style issues reported by Copilot

c72674c

Copilot AI reviewed Feb 13, 2026

View reviewed changes

fix: correct CodeModifierAction field name in test fixtures (was Code…

111b1b5

…Modifieraction since SS4 refactor)

jsirish requested a review from Copilot February 17, 2026 19:53

Copilot started reviewing on behalf of jsirish February 17, 2026 19:53 View session

Copilot AI reviewed Feb 17, 2026

View reviewed changes

	$title .= ($this->PriceModifier != 0) ?
	$title .= ($this->PriceModifier != 0) ?

Uh oh!

Conversation

jsirish commented Feb 13, 2026

Summary

Root Cause

Changes

ProductPage.php

OptionItem.php

PublishOptionItemsTask.php (NEW)

Post-Deploy Steps

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

jsirish commented Feb 13, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

jsirish commented Feb 13, 2026

Copilot Re-Review (Review 3) — Disposition

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

`ProductPage.php`

`OptionItem.php`

`PublishOptionItemsTask.php` (NEW)