Conversation
|
@damsfx you also need to add the method signature to the File facade ( |
|
This pull request will be closed and archived in 3 days, as there has been no activity in the last 60 days. |
| * winter.txt, [winter_1.txt, winter_2.txt] -> winter_3.txt | ||
| * winter.txt, [winter_1.txt, winter_3.txt] -> winter_4.txt |
There was a problem hiding this comment.
| * 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.
There was a problem hiding this comment.
@LukeTowers If the entry doesn't exist in the array, it must be returned with another index anyway, right?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@LukeTowers So in a such case, the same filename is needed in the array of references to get the next incremented name.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.cmsThere was a problem hiding this comment.
@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?
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
@LukeTowers Do we continue with this PR in draft?
bennothommo
added
maintenance
|
This pull request will be closed and archived in 3 days, as there has been no activity in the last 60 days. |
|
This pull request will be closed and archived in 3 days, as there has been no activity in the last 60 days. |
|
This pull request will be closed and archived in 3 days, as there has been no activity in the last 60 days. |
WalkthroughA new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 2
🧹 Nitpick comments (1)
src/Filesystem/Filesystem.php (1)
249-254: Unanchored regex and missingpreg_quotecan produce silent false matches.The pattern is assembled from raw user-supplied strings without anchors or escaping:
'/' . $info['filename'] . $separator . '(\d*)\.' . $info['extension'] . '/'Two concrete problems:
- No anchors —
/winter_(\d*)\.cms/matchessuperwinter_1.cms(substring match), silently adding a false index to the pool and inflating the returned index.- No
preg_quote— if$info['filename']contains a dot (e.g.,my.archive.tar.gz→filename = my.archive.tar) or$separatoris a regex metacharacter (.,+, etc.), the generated pattern is malformed and matches unintended strings.\d*vs\d+— zero-or-more matches the empty group inwinter_.cms, yielding index0.\d+is a tighter and more intentional constraint.🔧 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)) {🤖 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 regex built in the foreach loop that checks $list items uses unescaped user strings and lacks anchors and a proper digit quantifier, causing false matches; update the pattern used around the block that references $info['filename'], $separator, and $info['extension'] to preg_quote the filename, separator and extension, anchor the pattern with ^ and $ (or use start/end delimiters), and change (\d*) to (\d+) so only one-or-more digits are captured before casting to int and appending to $indexes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/Filesystem/FilesystemTest.php`:
- Around line 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'.
- 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).
---
Nitpick comments:
In `@src/Filesystem/Filesystem.php`:
- Around line 249-254: The regex built in the foreach loop that checks $list
items uses unescaped user strings and lacks anchors and a proper digit
quantifier, causing false matches; update the pattern used around the block that
references $info['filename'], $separator, and $info['extension'] to preg_quote
the filename, separator and extension, anchor the pattern with ^ and $ (or use
start/end delimiters), and change (\d*) to (\d+) so only one-or-more digits are
captured before casting to int and appending to $indexes.
|
|
||
| public function testUnique() | ||
| { | ||
| $cases = [ |
There was a problem hiding this comment.
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.
| $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).
| '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 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/matcheswinter_1.cms→$indexes = [1] max([1]) + 1 = 2→ returnswinter_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'.
4 participants
Restore :
Summary by CodeRabbit