Fix trailing slash of untold torment and suffering#2319
Conversation
Signed-off-by: Nikolaos Karaolidis <nick@karaolidis.com>
📝 WalkthroughWalkthroughThe ChangesEditFiles URL Generation
Possibly Related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/Filament/Server/Resources/Files/Pages/EditFiles.php (1)
262-265: ⚡ Quick win
getUrl()override is now a no-op — consider removing it entirely.After stripping the trailing-slash concatenation, this override unconditionally delegates to
parent::getUrl()with identical arguments and adds no behaviour. Keeping it is misleading (readers will wonder what it overrides) and means future changes to the parent signature must be mirrored here manually.♻️ Proposed refactor — remove the redundant override
- public static function getUrl(array $parameters = [], bool $isAbsolute = true, ?string $panel = null, ?Model $tenant = null, bool $shouldGuessMissingParameters = false, ?string $configuration = null): string - { - return parent::getUrl($parameters, $isAbsolute, $panel, $tenant, $shouldGuessMissingParameters, $configuration); - } - public static function route(string $path): PageRegistration🤖 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/Filament/Server/Resources/Files/Pages/EditFiles.php` around lines 262 - 265, The getUrl method override in the EditFiles page is now a no-op and should be removed: delete the public static function getUrl(...) definition so the class inherits parent::getUrl() directly; verify there are no internal references to EditFiles::getUrl elsewhere (search for getUrl calls on this class) and run tests/compile to ensure no signature-dependent code required the override.
🤖 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.
Nitpick comments:
In `@app/Filament/Server/Resources/Files/Pages/EditFiles.php`:
- Around line 262-265: The getUrl method override in the EditFiles page is now a
no-op and should be removed: delete the public static function getUrl(...)
definition so the class inherits parent::getUrl() directly; verify there are no
internal references to EditFiles::getUrl elsewhere (search for getUrl calls on
this class) and run tests/compile to ensure no signature-dependent code required
the override.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e541b915-7254-4d04-9d75-c3ea57667d6a
📒 Files selected for processing (1)
app/Filament/Server/Resources/Files/Pages/EditFiles.php
I'll leave this one up to the maintainers. edit: i.e. the decision, not the cleanup |
|
It is needed else you can't edit .env files or .php files for example. |
rmartinoscar
left a comment
There was a problem hiding this comment.
Hey thanks for your contribution but im afraid without that you won't be able to edit files that start with a dot or are processed by your webserver such as .html, .php etc...
When Pelican is deployed behind a reverse proxy (e.g., Traefik, nginx) that terminates TLS and forwards plain HTTP to Apache, navigating to the file editor triggers a mixed content block in the browser.
Why?
EditFiles::getUrl()appends a trailing/to the URL. The.htaccessrewrite rule then strips it with a 301 Redirect. When Apache constructs the redirect URL, it uses the scheme of the incoming connection (plainhttp://), not the external-facing scheme. The browser sees a redirect fromhttps://tohttp://and blocks it as mixed active content.The fix simply removes the trailing slash concatenation. The redirect was always a no-op from the user's perspective (append slash, immediately redirect to remove it), and Laravel's front controller handles routing regardless of trailing slashes.
I have run out of hairs to pull out debugging this.
Possibly relevant to #1685.