Skip to content

Restore File::unique() helper #186

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

Open
damsfx wants to merge 8 commits into
base: develop
Choose a base branch
from
+50 −0
Open

Restore File::unique() helper #186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 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
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
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
Comment on lines +236 to +237
Copy link
Member

@LukeTowers LukeTowers Feb 4, 2025

Choose a reason for hiding this comment

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

Suggested change
* winter.txt, [winter_1.txt, winter_2.txt] -> winter_3.txt
* winter.txt, [winter_1.txt, winter_3.txt] -> winter_4.txt
* winter.txt, [winter.txt] -> winter_1.txt
* winter.txt, [winter.txt, winter_1.txt, winter_2.txt] -> winter_3.txt
* winter.txt, [winter.txt, winter_1.txt, winter_3.txt] -> winter_4.txt
* winter.txt, [winter_1.txt, winter_3.txt] -> winter.txt

@damsfx it should probably allow the input to be returned unmodified if it doesn't exist within the array.

Copy link
Contributor Author

@damsfx damsfx Feb 4, 2025

Choose a reason for hiding this comment

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

@LukeTowers If the entry doesn't exist in the array, it must be returned with another index anyway, right?

Copy link
Member

@LukeTowers LukeTowers Feb 6, 2025

Choose a reason for hiding this comment

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

@damsfx I'm making the suggestion that if it doesn't exist in the array it should be returned unmodified. The method is intended to give you a unique filename provided the input of a desired filename and a list of already existing options. If the filename provided is already unique, then there's no need to modify it.

Copy link
Contributor Author

@damsfx damsfx Feb 6, 2025

Choose a reason for hiding this comment

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

@LukeTowers So in a such case, the same filename is needed in the array of references to get the next incremented name.

Copy link
Member

@LukeTowers LukeTowers Feb 6, 2025

Choose a reason for hiding this comment

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

The suggested change above should list all the cases that I think the method should need to handle, I've already updated the tests so it should just require a minor change in the method itself.

Copy link
Contributor Author

@damsfx damsfx Feb 6, 2025

Choose a reason for hiding this comment

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

@LukeTowers Your modifications in tests goes to a failure for me.

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

FAILED !
The function return wintert_2.cms in that case.

What I don't understand is why you want to return the unmodified value if it's unique.
In any case, we should return the incremented value! 🤯

(new Filesystem())->unique('winter.cms', ['test.cms']);   // winter_1.cms
(new Filesystem())->unique('winter.cms', []);             // winter_1.cms

Copy link
Member

@LukeTowers LukeTowers Feb 8, 2025

Choose a reason for hiding this comment

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

@damsfx The tests are failing because the method doesn't currently have the logic to allow the name to be returned unmodified. The method claims to return a unique filename given the input of a desired filename and a list of existing filenames; why does it have to always return an incremented value if the desired filename isn't present?

Copy link
Contributor Author

@damsfx damsfx Feb 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@LukeTowers Apply a unique index to a filename from provided list ...
Maybe we should then say Apply a unique index to a filename from provided list ONLY if it is found in the list ... or change the name of the method which is perhaps the source of the confusion.

In any case, the current function respects the logic of the original one:
68bb7ac

Copy link
Contributor Author

@damsfx damsfx May 27, 2025

Choose a reason for hiding this comment

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

@LukeTowers Do we continue with this PR in draft?

*/
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];
}

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

Fix indentation — confirmed code-quality pipeline failure.

Line 16 uses 6 spaces; the pipeline reports expected at least 8 spaces. Align with the rest of the 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: ERR: 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, The line defining the $cases
array is indented with 6 spaces and fails the code-quality rule expecting at
least 8 spaces; update the indentation of the $cases declaration to match the
rest of the method body (use 8 spaces) so it aligns with surrounding statements
in the same function (look for the $cases variable in the test method in
FilesystemTest and adjust its leading spaces accordingly).

// 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 +29
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

Test case B's expected value contradicts the implementation and causes the confirmed pipeline failure (line 31).

The failing assertion is case B:

'winter.cms' => ['winter.cms', ['winter_1.cms']],  // expected: 'winter.cms'

which calls unique('winter.cms', ['winter_1.cms']).

Tracing through the implementation:

  • Pattern /winter_(\d*)\.cms/ matches winter_1.cms → $indexes = [1]
  • max([1]) + 1 = 2 → returns winter_2.cms
  • Test asserts winter.cms → FAIL ✓ (confirmed by pipeline)

Cases B and C have the same precondition (winter.cms absent from the list) but opposite expectations:

Case $list Expected
B ['winter_1.cms'] winter.cms
C ['winter_1.cms', 'test_5', 'winter_3.cms'] winter_4.cms

There is no single consistent rule that produces both results. The docblock examples (winter.txt, [winter_1.txt, winter_2.txt] → winter_3.txt) match the current implementation (always return an indexed name when indexed variants exist), not case B's expectation.

Most likely fix: case B's expected value should be winter_2.cms and its comment should be revised:

🔧 Proposed fix 
-            // File already unique, return original
-            'winter.cms' => ['winter.cms', ['winter_1.cms']],
+            // Only one indexed variant exists; next index is returned
+            'winter_2.cms' => ['winter.cms', ['winter_1.cms']],

If instead the intent is that unique() returns $str unchanged when $str is not in $list, then the implementation needs an early-return guard and cases C and D (where winter.cms is also absent from the list yet an indexed name is expected) would need to be revised or replaced. Please cross-check against the original commits referenced in the PR description (68bb7ac and 76a4302) to confirm the intended algorithm.

🤖 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 - 29, The test
expectation for the case keyed 'winter.cms' is inconsistent with the unique()
implementation: update the expected result from 'winter.cms' to 'winter_2.cms'
in the test row "'winter.cms' => ['winter.cms', ['winter_1.cms']]" (and update
its inline comment) so it matches the unique() behavior (which parses indexes
like winter_1.cms and returns the next indexed name); alternatively, if you
intend unique() to return the original when not present, change the unique()
implementation to early-return $str in the unique() function instead—but the
quick fix is to change the test expectation and comment to 'winter_2.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