Add soft delete observable events#224
Conversation
WalkthroughThis pull request adds soft-delete model event registration methods to the SoftDelete trait, introduces a filename uniqueness utility method to the Filesystem class with facade support, and includes tests for the new functionality. The changes extend framework capabilities for handling force deletion events and generating unique filenames. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/Filesystem/FilesystemTest.php (1)
14-33: Consider adding@dataProviderand anInvalidArgumentExceptiontest caseEvery other test in this file uses
@dataProviderfor data-driven cases. This test uses a manualforeachloop instead, which produces a less informative failure message (no per-case label) and is inconsistent with the file's established pattern.Additionally, there's no test for the
InvalidArgumentExceptionthrown when$strhas no extension.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Filesystem/FilesystemTest.php` around lines 14 - 33, Refactor testUnique to use a data provider: create a method (e.g., uniqueProvider) that returns the array of cases (including the separator-case) and annotate testUnique with `@dataProvider` uniqueProvider so each case runs separately and yields per-case failure messages; update testUnique signature to accept parameters matching the provider and call $this->filesystem->unique(...$params) and assertSame. Also add a new test method (e.g., testUniqueThrowsOnNoExtension) that calls $this->filesystem->unique with a filename having no extension and asserts that it throws InvalidArgumentException. Ensure you reference the existing filesystem->unique method and use InvalidArgumentException in the new test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Database/Traits/SoftDelete.php`:
- Around line 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.
In `@src/Filesystem/Filesystem.php`:
- Around line 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.
In `@tests/Filesystem/FilesystemTest.php`:
- 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.
- Around line 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').
---
Nitpick comments:
In `@tests/Filesystem/FilesystemTest.php`:
- Around line 14-33: Refactor testUnique to use a data provider: create a method
(e.g., uniqueProvider) that returns the array of cases (including the
separator-case) and annotate testUnique with `@dataProvider` uniqueProvider so
each case runs separately and yields per-case failure messages; update
testUnique signature to accept parameters matching the provider and call
$this->filesystem->unique(...$params) and assertSame. Also add a new test method
(e.g., testUniqueThrowsOnNoExtension) that calls $this->filesystem->unique with
a filename having no extension and asserts that it throws
InvalidArgumentException. Ensure you reference the existing filesystem->unique
method and use InvalidArgumentException in the new test.
| /** | ||
| * 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); | ||
| } |
There was a problem hiding this comment.
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.
| foreach ($list as $item) { | ||
| if (!preg_match('/' . $info['filename'] . $separator . '(\d*)\.' . $info['extension'] . '/', $item, $matches)) { | ||
| continue; | ||
| } | ||
|
|
||
| $indexes[] = (int) $matches[1]; |
There was a problem hiding this comment.
Regex metacharacter injection and zero-length digit match
Two correctness issues on the preg_match pattern:
-
$info['filename'],$separator, and$info['extension']are interpolated directly into the regex withoutpreg_quote(). A filename likefile.v2.cmsproducesfilename = '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 apreg_last_errorfailure for invalid syntax like*as separator). -
(\d*)matches zero or more digits, so an item likewinter_.cmsis accepted with index0, which incorrectly skewsmax($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.
|
|
||
| public function testUnique() | ||
| { | ||
| $cases = [ |
There was a problem hiding this comment.
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.
| $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.
| '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'], ' '], |
There was a problem hiding this comment.
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').
2 participants
Summary by CodeRabbit
New Features
Tests