Skip to content

Add soft delete observable events #224

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
damsfx wants to merge 9 commits into from
+72 −0
Closed

Add soft delete observable events #224

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
45d0748
Restore File::unique() method
damsfx Aug 1, 2024
0f2b4c4
Retore FileSystem unique test
damsfx Aug 1, 2024
c9e9095
Fix code quality check
damsfx Aug 1, 2024
0c9dbc5
Merge branch 'wintercms:develop' into develop
damsfx Nov 21, 2024
bf4762e
Merge commit 'f04400144927db674d32a5edb9bf5a0e2019ae97' into develop
damsfx Dec 5, 2024
d66cc3f
Add unique() method signature to the File facade
damsfx Dec 5, 2024
43ba814
Update tests/Filesystem/FilesystemTest.php
LukeTowers Feb 4, 2025
dfc23f7
Merge branch 'wintercms:develop' into develop
damsfx Feb 19, 2026
28c0ec6
Add force delete observable events
damsfx Feb 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions src/Database/Traits/SoftDelete.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,28 @@ public static function restored($callback)
static::registerModelEvent('restored', $callback);
}

/**
* Register a "forceDeleting" model event callback with the dispatcher.
*
* @param \Closure|string $callback
* @return void
*/
public static function forceDeleting($callback)
{
static::registerModelEvent('forceDeleting', $callback);
}

/**
* Register a "forceDeleted" model event callback with the dispatcher.
*
* @param \Closure|string $callback
* @return void
*/
public static function forceDeleted($callback)
{
static::registerModelEvent('forceDeleted', $callback);
}
Comment on lines +306 to +326
Copy link

@coderabbitai coderabbitai bot Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

forceDeleting and forceDeleted events are never dispatched — registered callbacks will never fire

The two new registration methods correctly call static::registerModelEvent(), but the Winter Storm forceDelete() override (lines 90–97) only calls $this->delete() — which dispatches deleting/deleted — and never calls fireModelEvent('forceDeleting') or fireModelEvent('forceDeleted'). Laravel 9.x documents forceDeleting and forceDeleted as distinct lifecycle events, fired explicitly by the framework's own SoftDeletes::forceDelete(). Because Winter Storm overrides that method, it must replicate the event firing itself.

Compare the existing restore() method (lines 180, 193), which explicitly calls fireModelEvent('restoring') (halting) and fireModelEvent('restored', false) (non-halting). The same pattern must be applied to forceDelete().

🐛 Proposed fix — fire the events in forceDelete() 
 public function forceDelete()
 {
     $this->forceDeleting = true;

+    if ($this->fireModelEvent('forceDeleting') === false) {
+        $this->forceDeleting = false;
+        return false;
+    }
+
     $this->delete();

     $this->forceDeleting = false;
+
+    $this->fireModelEvent('forceDeleted', false);
 }
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/Database/Traits/SoftDelete.php` around lines 306 - 326, The forceDelete()
override currently only calls $this->delete() so registered
forceDeleting/forceDeleted callbacks never run; update forceDelete() to mimic
restore() by first calling $this->fireModelEvent('forceDeleting') and aborting
if it returns false (halting), then perform the actual deletion (keep the
existing $this->delete() call), and finally call
$this->fireModelEvent('forceDeleted', false) to dispatch non-halting post-delete
listeners; reference the existing restore() implementation and the
fireModelEvent() method when making the changes.


/**
* Get the name of the "deleted at" column.
*
Expand Down
28 changes: 28 additions & 0 deletions src/Filesystem/Filesystem.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,34 @@ public function isLocalDisk(\Illuminate\Filesystem\FilesystemAdapter $disk): boo
return ($disk->getAdapter() instanceof \League\Flysystem\Local\LocalFilesystemAdapter);
}

/**
* Apply a unique index to a filename from provided list i.e.
* winter.txt, [winter_1.txt, winter_2.txt] -> winter_3.txt
* winter.txt, [winter_1.txt, winter_3.txt] -> winter_4.txt
*/
public function unique(string $str, array $list, string $separator = '_', int $starting = 1, int $step = 1): string
{
$indexes = [];

$info = pathinfo($str);

if (empty($info['filename']) || empty($info['extension'])) {
throw new \InvalidArgumentException('$str must be a file name');
}

foreach ($list as $item) {
if (!preg_match('/' . $info['filename'] . $separator . '(\d*)\.' . $info['extension'] . '/', $item, $matches)) {
continue;
}

$indexes[] = (int) $matches[1];
Comment on lines +249 to +254
Copy link

@coderabbitai coderabbitai bot Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Regex metacharacter injection and zero-length digit match

Two correctness issues on the preg_match pattern:

  1. $info['filename'], $separator, and $info['extension'] are interpolated directly into the regex without preg_quote(). A filename like file.v2.cms produces filename = 'file.v2', causing the unescaped . to match any character. Any regex metacharacter in the filename or a custom separator will silently corrupt the pattern (or cause a preg_last_error failure for invalid syntax like * as separator).

  2. (\d*) matches zero or more digits, so an item like winter_.cms is accepted with index 0, which incorrectly skews max($indexes). Use (\d+) to require at least one digit.

🐛 Proposed fix 
-        if (!preg_match('/' . $info['filename'] . $separator . '(\d*)\.' . $info['extension'] . '/', $item, $matches)) {
+        if (!preg_match(
+            '/^' . preg_quote($info['filename'], '/') . preg_quote($separator, '/') . '(\d+)\.' . preg_quote($info['extension'], '/') . '$/',
+            $item,
+            $matches
+        )) {

Note: anchoring with ^/$ also prevents subwinter_1.cms from falsely matching a pattern for winter.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/Filesystem/Filesystem.php` around lines 249 - 254, The preg_match usage
in Filesystem.php is vulnerable: it interpolates $info['filename'], $separator,
and $info['extension'] directly into the regex and uses (\d*) which permits
zero-length matches; update the pattern construction used in the foreach over
$list so you escape those dynamic values with preg_quote($info['filename'], '/')
and preg_quote($info['extension'], '/') (and preg_quote for $separator if it can
contain metacharacters), change the digit capture to (\d+) to require at least
one digit, and anchor the pattern with ^ and $ so only exact
filename+separator+index+extension matches are accepted before populating
$matches and casting to int into $indexes.

}

return empty($indexes)
? $info['filename'] . $separator . $starting . '.' . $info['extension']
: $info['filename'] . $separator . (max($indexes) + $step) . '.' . $info['extension'];
}

/**
* Finds the path of a given class.
*
Expand Down
1 change: 1 addition & 0 deletions src/Support/Facades/File.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
* @method static bool fileNameMatch(string $fileName, string $pattern)
* @method static bool copyBetweenDisks(string|FilesystemAdapter $sourceDisk, string|FilesystemAdapter $destinationDisk, string $filePath, ?string $targetPath = null)
* @method static bool moveBetweenDisks(string|FilesystemAdapter $sourceDisk, string|FilesystemAdapter $destinationDisk, string $filePath, ?string $targetPath = null)
* @method static string unique(string $str, array $list, string $separator = '_', int $starting = 1, int $step = 1)
*
* @see \Winter\Storm\Filesystem\Filesystem
*/
Expand Down
21 changes: 21 additions & 0 deletions tests/Filesystem/FilesystemTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,27 @@
$this->filesystem = new Filesystem();
}

public function testUnique()
{
$cases = [
Copy link

@coderabbitai coderabbitai bot Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

PHPCS: incorrect indentation

The $cases array is indented with 6 spaces; the pipeline requires at least 8 spaces inside a method body.

🐛 Proposed fix 
-      $cases = [
+        $cases = [
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
$cases = [
$cases = [
🧰 Tools
🪛 GitHub Actions: Code Quality

[error] 16-16: PHPCS: Line indented incorrectly; expected at least 8 spaces

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@tests/Filesystem/FilesystemTest.php` at line 16, In the FilesystemTest class
adjust the indentation of the $cases array so it has at least 8 spaces inside
the method body; locate the $cases assignment in the test method within
Filesystem/FilesystemTest.php and re-indent the line (and any subsequent lines
of the array) to use 8 spaces so it conforms to the PHPCS method-body
indentation rule.

// File exists, make it unique
'winter_1.cms' => ['winter.cms', ['winter.cms', 'test_5']],

// File already unique, return original
'winter.cms' => ['winter.cms', ['winter_1.cms']],

// Last index available is incremented
'winter_4.cms' => ['winter.cms', ['winter_1.cms', 'test_5', 'winter_3.cms']],
'winter_98.cms' => ['winter.cms', ['winter_97.cms', 'test_5', 'winter_1.cms']],

// Separator as space
'winter 1.cms' => ['winter.cms', ['winter_1.cms', 'test_5', 'winter_3.cms'], ' '],
Comment on lines +18 to +28
Copy link

@coderabbitai coderabbitai bot Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Test case 2's expected value causes the pipeline assertion failure

The comment says "File already unique, return original", but the list ['winter_1.cms'] contains an indexed version of winter.cms. The regex /winter_(\d*)\.cms/ matches winter_1.cms → $indexes = [1] → the implementation correctly returns winter_2.cms. The expected value 'winter.cms' is wrong, which is the direct cause of the Failed asserting that two strings are identical pipeline failure on line 31.

Change the expected key to 'winter_2.cms' to reflect the actual intended behavior (consistent with cases 3a and 3b). Alternatively, if the intent truly is "return original when $str is not itself present in $list", then the implementation logic needs to be revisited — but note that cases 3a/3b also omit winter.cms from their lists yet expect indexed output, making that semantic self-contradictory.

🐛 Proposed fix (align test with implementation) 
-            // File already unique, return original
-            'winter.cms' => ['winter.cms', ['winter_1.cms']],
+            // Next index when indexed versions already exist
+            'winter_2.cms' => ['winter.cms', ['winter_1.cms']],
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@tests/Filesystem/FilesystemTest.php` around lines 18 - 28, Update the test
case in tests/Filesystem/FilesystemTest.php where the array entry "'winter.cms'
=> ['winter.cms', ['winter_1.cms']]" is defined: change the expected returned
filename from 'winter.cms' to 'winter_2.cms' so the entry becomes "'winter.cms'
=> ['winter_2.cms', ['winter_1.cms']]" to match the implementation behavior used
in the other cases (see the array entry keys and values around 'winter_1.cms',
'winter_3.cms', and 'winter_97.cms').

];
foreach ($cases as $output => $config) {
$this->assertSame($output, $this->filesystem->unique(...$config));

Check failure on line 31 in tests/Filesystem/FilesystemTest.php

View workflow job for this annotation

GitHub Actions / windows-latest / PHP 8.3 

Failed asserting that two strings are identical.

Check failure on line 31 in tests/Filesystem/FilesystemTest.php

View workflow job for this annotation

GitHub Actions / ubuntu-latest / PHP 8.3 

Failed asserting that two strings are identical.
}
}

/**
* @dataProvider providePathsForIsAbsolutePath
* @see Symfony\Component\Filesystem\Tests\FilesystemTest::testIsAbsolutePath
Expand Down
Loading