Remove file number ("yeet (1).zip") from plugin name to fix import - #2489
Remove file number ("yeet (1).zip") from plugin name to fix import#2489Boy132 wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughPlugin imports now use a shared helper to normalize uploaded ZIP filenames. The helper removes the ChangesPlugin import handling
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Services/Helpers/PluginService.php`:
- Line 365: Extract the filename normalization currently assigned to $pluginName
in downloadPluginFromFile() into a shared helper, then reuse that helper in
PluginResource’s duplicate check and downloadPluginFromFile(). Ensure both paths
normalize names such as modpack-manager(1).zip to the same value before
evaluating import_exists.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7bade0e6-68fb-4da9-ab2f-ebd8135cda5d
📒 Files selected for processing (1)
app/Services/Helpers/PluginService.php
| } | ||
|
|
||
| $pluginName = str($file->getClientOriginalName())->basename()->before('.zip')->toString(); | ||
| $pluginName = str($file->getClientOriginalName())->basename()->before('.zip')->replaceMatches('/\(\d+\)(?=\.[^.]+$|$)/', '')->trim()->toString(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep duplicate detection consistent with filename normalization.
app/Filament/Admin/Resources/Plugins/PluginResource.php:262-288 still checks the unnormalized name. For modpack-manager(1).zip, the admin path checks modpack-manager(1), while this line imports as modpack-manager. An existing plugin can bypass import_exists.
Move this normalization into a shared helper and use it in both the duplicate check and downloadPluginFromFile().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/Services/Helpers/PluginService.php` at line 365, Extract the filename
normalization currently assigned to $pluginName in downloadPluginFromFile() into
a shared helper, then reuse that helper in PluginResource’s duplicate check and
downloadPluginFromFile(). Ensure both paths normalize names such as
modpack-manager(1).zip to the same value before evaluating import_exists.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/helpers.php`:
- Around line 170-176: Update get_plugin_name_from_zip to remove only the final
.zip extension using an end-anchored, case-insensitive match, preserving other
filename extensions and occurrences. After stripping that extension, remove only
a trailing numeric copy suffix, including whitespace before the suffix, so names
such as foo(1).bar.zip, foo.ZIP, and foo (1) .zip normalize correctly.
- Around line 172-175: Update get_plugin_name_from_zip() to assign the
normalized filename result to $pluginName, then reject empty values, “.”, “..”,
path separators, and absolute-path prefixes before returning it. Ensure invalid
names are handled before plugin_path() or any extraction logic can use them.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 51e0ade3-af32-47d1-b15c-e06d63c69982
📒 Files selected for processing (3)
app/Filament/Admin/Resources/Plugins/PluginResource.phpapp/Services/Helpers/PluginService.phpapp/helpers.php
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Services/Helpers/PluginService.php
|
|
||
| if (!function_exists('get_plugin_name_from_zip')) { | ||
| function get_plugin_name_from_zip(UploadedFile $file): string | ||
| { | ||
| return str($file->getClientOriginalName())->basename()->before('.zip')->replaceMatches('/\(\d+\)(?=\.[^.]+$|$)/', '')->trim()->toString(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Normalize only the final .zip extension and copy suffix.
before('.zip') is case-sensitive and removes the first occurrence. After that call, the regex can remove (1) before another extension, so foo(1).bar.zip becomes foo.bar. It also leaves foo.ZIP unchanged and misses foo (1) .zip.
Use an end-anchored, case-insensitive extension match, then remove only a trailing numeric suffix.
Proposed fix
- return str($file->getClientOriginalName())->basename()->before('.zip')->replaceMatches('/\(\d+\)(?=\.[^.]+$|$)/', '')->trim()->toString();
+ return str($file->getClientOriginalName())
+ ->basename()
+ ->replaceMatches('/\.zip$/i', '')
+ ->replaceMatches('/\s*\(\d+\)\s*$/', '')
+ ->trim()
+ ->toString();📝 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.
| if (!function_exists('get_plugin_name_from_zip')) { | |
| function get_plugin_name_from_zip(UploadedFile $file): string | |
| { | |
| return str($file->getClientOriginalName())->basename()->before('.zip')->replaceMatches('/\(\d+\)(?=\.[^.]+$|$)/', '')->trim()->toString(); | |
| } | |
| } | |
| if (!function_exists('get_plugin_name_from_zip')) { | |
| function get_plugin_name_from_zip(UploadedFile $file): string | |
| { | |
| return str($file->getClientOriginalName()) | |
| ->basename() | |
| ->replaceMatches('/\.zip$/i', '') | |
| ->replaceMatches('/\s*\(\d+\)\s*$/', '') | |
| ->trim() | |
| ->toString(); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/helpers.php` around lines 170 - 176, Update get_plugin_name_from_zip to
remove only the final .zip extension using an end-anchored, case-insensitive
match, preserving other filename extensions and occurrences. After stripping
that extension, remove only a trailing numeric copy suffix, including whitespace
before the suffix, so names such as foo(1).bar.zip, foo.ZIP, and foo (1) .zip
normalize correctly.
| function get_plugin_name_from_zip(UploadedFile $file): string | ||
| { | ||
| return str($file->getClientOriginalName())->basename()->before('.zip')->replaceMatches('/\(\d+\)(?=\.[^.]+$|$)/', '')->trim()->toString(); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 'function[[:space:]]+plugin_path|plugin_path\(' app
rg -n -C 8 'get_plugin_name_from_zip|\$pluginName|File::deleteDirectory' \
app/helpers.php \
app/Services/Helpers/PluginService.php \
app/Filament/Admin/Resources/Plugins/PluginResource.phpRepository: pelican-dev/panel
Length of output: 31129
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '75,110p' app/helpers.php
sed -n '330,380p' app/Services/Helpers/PluginService.php
printf '\n--- path helpers definitions/usages nearby ---\n'
sed -n '80,105p' app/helpers.php
printf '\n--- deterministic filename normalization probe for reported examples ---\n'
python3 - <<'PY'
import re
def get_plugin_name_from_zip(filename: str) -> str:
basename = filename.rpartition('/')[-1]
head = basename.rpartition('.')[0]
text = re.sub(r'(\d+)(?=\.[^.]+$|$)', r'\1', '')
text = re.sub(r'\s+', ' ', text).strip()
return head.replace(r'\1', '')
for name in ['(1).zip', '..(1).zip', '.zip', '( 1 ).zip']:
normalized = re.sub(r'\(\d+\)(?=\.[^.]+$|$)', '', name.rsplit(".", 1)[0]).strip()
print(name, "=>", repr(normalized))
PYRepository: pelican-dev/panel
Length of output: 3577
Reject empty and traversal plugin names before extracting.
get_plugin_name_from_zip() accepts client-controlled names and can return '' or ..; plugin_path() then joins these into relative paths (plugins, plugins/../), so plugin.json lookup and extraction can target the plugin directory root or a parent directory. Return after validating $pluginName is non-empty, not . or .., and contains no /, \, or absolute prefix.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/helpers.php` around lines 172 - 175, Update get_plugin_name_from_zip() to
assign the normalized filename result to $pluginName, then reject empty values,
“.”, “..”, path separators, and absolute-path prefixes before returning it.
Ensure invalid names are handled before plugin_path() or any extraction logic
can use them.
1 participant
Closes #2486
Honestly, I asked ChatGPT to create the regex because no one likes regex