Configurable event image layout (closes #137)#139
Closed
fabiodalez-dev wants to merge 9 commits into
Closed
Conversation
Issue #137 (HansUwe52): the featured image on the public event detail page renders at 100% container width with no max-height constraint, so sproportioned uploads (e.g. a 1791x927 book cover used as an event image) dominate the viewport. Adds a CMS-tab setting 'cms.event_image_layout' with four options: - contained (default) max-height: clamp(280px, 50vw, 480px) + object-fit: contain. Solves the reported case while preserving the previous look for reasonably-sized images. - banner aspect-ratio: 16/9 + object-fit: cover. Crops to a uniform hero banner. - full Legacy 100%-width-no-constraint behaviour, for users who depended on it. - thumb Right-floated 3:4 portrait thumbnail that wraps the event body text. Collapses to full-width on viewports under 640px. The setting lives in the existing system_settings KV store (no schema migration needed) and is wired through the existing pattern used by 'updateLabels' / 'updateSharingSettings': - SettingsController::updateEventSettings (validated allow-list) - resolveEventSettings() exposed to the settings view - POST /admin/settings/events route protected by CsrfMiddleware + AdminAuthMiddleware - select rendered inside the existing 'Eventi' card on the CMS tab, so the admin doesn't have to discover a new tab FrontendController::event() reads and re-validates the setting against the same allow-list before passing it to the view; event-detail.php narrows once more before composing the CSS class, so a future controller refactor cannot leak an unknown layout into the DOM. i18n: 8 new keys added to all four locale files (it_IT/en_US/de_DE/fr_FR) in the same commit, per project policy. Tests: tests/issue-137-event-image-layout.spec.js — 5 cases covering the four explicit layouts plus the default fallback. Each test mutates cms.event_image_layout in the KV store directly, requests the public event page, and asserts the figure carries the matching CSS modifier and data-event-cover-layout attribute. All 5 pass. PHPStan: clean (no new findings; pre-existing storage/plugins issues unchanged at 11).
Self-review feedback on the initial layout palette:
- 'contained' was using an arbitrary max-height: 480px that produced
uneven silhouettes depending on the source aspect ratio. Replaced
with aspect-ratio: 3/2 + object-fit: cover so the cover always
lands on a predictable photographic crop.
- 'thumb' was using CSS float: right, which caused the figure to
escape the parent .event-card whenever the body content was short
(e.g. a one-paragraph event) — visually the image then overlapped
the page footer. Refactored to a CSS grid layout driven by a new
.event-card--thumb-layout modifier on the parent article: the
figure now lives in its own column/row and is geometrically
constrained inside the card. On viewports below 768px the grid
collapses to a vertical stack.
- Bonus on desktop: the side-thumbnail sticks while the reader
scrolls long event descriptions (position: sticky).
UI labels updated to match the new design ('Foto 3:2' instead of
'Adattato (max 480px)') in all four locale files.
Tests: added a 6th case to tests/issue-137-event-image-layout.spec.js
that creates a very short event content and asserts
figure.bottom <= card.bottom at desktop width — the exact invariant
the float bug used to violate. All 6 cases pass.
Previous iterations all rendered at 100% container width and only
differed by aspect-ratio, so the original complaint (a 1791x927 image
dominating the event page) was still visible in three of four presets.
The whole point of the setting is to LIMIT the image footprint.
Resized accordingly:
full 100% x auto legacy enormous (opt-in)
banner 100% x 220px max low decorative strip
contained 420px max, centred small poster (DEFAULT, solves
the reported case directly)
thumb 240px wide side thumbnail next to body text
Source aspect ratio is preserved on contained/thumb-mobile — shrinking
the footprint is the goal, forcing crops would just trade one issue
for another.
UI labels rewritten so the size is explicit ('Piccola centrata
(max 420px)', 'Miniatura affiancata al testo (240px)', etc.) and
the librarian can pick at a glance. All 4 locale files updated.
Tests: added a measurement-based regression that asserts each preset
renders at a distinct, capped size:
contained.width <= 420px AND < 70% of card.width
thumb.width <= 245px
banner.height <= 225px
full.width > 85% of card.width
Catches the whole class of regression where four CSS modifiers exist
but render identically. All 7 tests pass.
User feedback: small images should sit on the left, not centred. - contained: margin-left: 0 (was: margin-left/right: auto) - thumb on mobile: margin: 0 0 1.5rem 0 (was: 0 auto 1.5rem auto) Small images centred in a wide card read as a 'centred hero' which is the same visual problem we are trying to escape: the image dominates its own row. Left-aligned, the image becomes part of the body flow. UI label updated: 'Piccola a sinistra (max 420px)' replaces 'Piccola centrata (max 420px)'. All four locale files updated. Test guard added: the effective-size test now also asserts that figure.x is within 100px of card.x (i.e. only inner padding) for both 'contained' and 'thumb'. Catches any future refactor that reintroduces margin: auto. All 7 cases pass.
The edit form has separate controls: a 'remove_image' checkbox and a
'featured_image' file input. When the user ticks both (replace the
old image AND clear the slot in one submit), the existing if/elseif
processed 'remove' first and skipped the upload branch — the new
file was discarded and featured_image ended up NULL.
Fix:
- Invert the priority: handle the file upload first; only run the
remove path when no upload was submitted. This matches user
intent (a new file always means 'replace').
- Look up the previous featured_image before the UPDATE and unlink
the orphan file from disk after the row is updated. Path-
traversal-safe via realpath() + base-directory containment, the
same contract handleImageUpload() upholds when writing.
Test: tests/issue-137-event-image-layout.spec.js — added an
end-to-end case that drives the actual admin form (login, edit
event, tick the remove checkbox, attach a new file, submit) and
asserts the DB row points at a NEW upload path (regex matches
handleImageUpload()'s naming scheme) — not NULL, not the old
sentinel. Catches the regression at the controller layer where the
historic bug lived. All 8 cases pass.
📝 WalkthroughWalkthroughAggiunge setting admin per il layout dell'immagine evento (full, banner, contained, thumb), risolve/valida il valore in frontend, implementa varianti CSS/markup, assicura lifecycle sicuro per upload/update/delete delle immagini, introduce traduzioni e test E2E di regressione. ChangesEvent Image Layout Configuration and Display
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin User
participant AdminUI as Settings UI
participant SettingsCtrl as SettingsController
participant Repository as SettingsRepository
participant ConfigStore as ConfigStore
participant DB as Database
participant FrontendCtrl as FrontendController
participant FrontendView as Event Detail View
participant Browser as Browser
Admin->>AdminUI: Seleziona layout e invia form
AdminUI->>SettingsCtrl: POST /admin/settings/events
SettingsCtrl->>SettingsCtrl: Normalizza e valida vs whitelist
SettingsCtrl->>Repository: Salva cms.event_image_layout
SettingsCtrl->>ConfigStore: Salva cms.event_image_layout
SettingsCtrl-->>AdminUI: Redirect + success_message
Browser->>FrontendCtrl: GET /event/{slug}
FrontendCtrl->>Repository: Leggi cms:event_image_layout
Repository-->>FrontendCtrl: Ritorna layout
FrontendCtrl->>FrontendView: Pass eventImageLayout
FrontendView-->>Browser: Rende `figure.event-cover--{layout}` con `data-event-cover-layout`
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // Reset content to a long enough body so 'banner' / 'contained' | ||
| // have something below them to measure against. | ||
| const longContent = '<p>' + 'Test event description. '.repeat(40) + '</p>'; | ||
| const sqlSafeLong = longContent.replace(/'/g, "\\'"); |
|
|
||
| // Shrink the event content so a CSS float would expose the bug. | ||
| const shortContent = '<p>Breve.</p>'; | ||
| const sqlSafe = shortContent.replace(/'/g, "\\'"); |
Owner
Author
Code reviewBranch: Found 29 findings across all lanes:
Deep lane — correctness & security✓ Auto-fixable (5)
Cross-cutting group G1: F005 + F006 + F025 — Three findings all describe the same root cause: the event image lifecycle is one-sided — only the successful UPDATE path cleans up the old file. Three additional disk-cleanup sites must be wired to call deleteUploadedImageFile() so the lifecycle is symmetric: (a) inside the else branch of $stmt->execute() in update() — unlink the newly-uploaded $featuredImagePath (F005); (b) inside the if(!empty($errors)) short-circuit — unlink $featuredImagePath when $updateImage is true and the path was just created by this request (F006); (c) inside delete() — read the current featured_image before DELETE, then call deleteUploadedImageFile() with the captured path (F025). Apply them together so any future change to the lifecycle (e.g., adding a new path) considers all four cleanup sites instead of just one. Add a brief docblock to deleteUploadedImageFile() listing the four callsites for traceability. Details and fix proposalsF005 — Orphan-file regression on UPDATE failure: when a new image was successfully uploaded but UPDATE returns false, the just-uploaded file at $featuredImagePath is never cleaned up. The new cleanup block runs only inside the success branch and deletes $oldImagePath. The DB row continues to reference the old path while a brand-new orphan file persists.File: Evidence:
Approach: Symmetric cleanup on UPDATE failure: unlink the newly-uploaded file when the DB write rolls back. Files to modify:
Verification:
Edge cases to preserve:
Latest fix attempt (fixrun_20260521T090648Zc0df58): fixed and verified F006 — Orphan-file gap on early validation-error redirect: handleImageUpload() moves the uploaded file to its final path before any non-image validation errors are checked. When the validation block finds title/date/time errors AFTER a successful upload, the controller redirects without DB write and without unlinking the freshly-moved $featuredImagePath.File: Evidence:
Approach: Cleanup orphan upload on validation-error short-circuit. Files to modify:
Verification:
Edge cases to preserve:
Latest fix attempt (fixrun_20260521T090648Zc0df58): fixed and verified F025 — Parallel paths whose invariants diverged — update() now cleans up orphans, but delete() still DELETEs an event row WITHOUT unlinking its featured_image. PR introduces a reusable cleanup helper but never wires it into delete().File: Evidence:
Approach: Symmetrize the event lifecycle: delete() must also unlink the file, matching update()'s cleanup pattern. Files to modify:
Verification:
Edge cases to preserve:
Latest fix attempt (fixrun_20260521T090648Zc0df58): fixed and verified F032 — Cross-locale assumption: test hardcodes '/eventi/' URL prefix (Italian). If the installation is en_US (routes /events) or de_DE (different localized route) the test silently 404s.File: Latest fix attempt (fixrun_20260521T090648Zc0df58): fixed and verified F033 — Test pollution: beforeAll INSERTs events_page_enabled=1 via ON DUPLICATE KEY UPDATE but afterAll never restores the original value. Test permanently enables the public events page after teardown.File: Evidence:
Approach: Snapshot + restore the events_page_enabled setting around the test suite. Files to modify:
Verification:
Edge cases to preserve:
Latest fix attempt (fixrun_20260521T090648Zc0df58): fixed and verified Light lane — ux, policy, architectureLight-lane findings — including rows labeled auto-fixable — aren't applied by
Promoted findings — detailsF007 — Silent UX regression on upgrade for existing installations: the new default cms.event_image_layout='contained' caps every event hero image at max-width:420px / max-height:320px. Existing librarians who previously curated wide hero images (rendered at 100% before this PR) will see images visibly shrink and re-align after upgrade without any migration notice. No 'preserve legacy on upgrade' default exists.File: Polish — below threshold, clustered (4)Below-gate findings (score < 45) that cluster in the same area — not worth surfacing individually, but dense enough that a human pass may catch something the pipeline filtered out.
Fix runsRun
|
| Finding | Group | Outcome | phase_9_finding |
|---|---|---|---|
| F005 | FG-1 | ✓ fixed and verified | |
| F006 | FG-1 | ✓ fixed and verified | |
| F009 | FG-2 | ✓ fixed and verified | |
| F010 | FG-2 | ✓ fixed and verified | |
| F011 | FG-2 | ✓ fixed and verified | |
| F012 | FG-3 | ✓ fixed and verified | |
| F013 | FG-4 | ✓ fixed and verified | |
| F025 | FG-1 | ✓ fixed and verified | |
| F032 | FG-2 | ✓ fixed and verified | |
| F033 | FG-2 | ✓ fixed and verified |
Run fixrun_20260521T103644Z7b1556 — 2026-05-21T10:43:39Z
- Outcomes: 4 fixed and verified
- Commits:
28e1e00
| Finding | Group | Outcome | phase_9_finding |
|---|---|---|---|
| F015 | FG-1 | ✓ fixed and verified | |
| F018 | FG-2 | ✓ fixed and verified | |
| F020 | FG-2 | ✓ fixed and verified | |
| F031 | FG-3 | ✓ fixed and verified |
🤖 Generated with Adam's Claude Code Review Command
Applied by /adamsreview:fix run_id=fixrun_20260521T090648Zc0df58 at threshold=60.
Cross-cutting group G1 (EventsController orphan-file lifecycle):
- F005: cleanup new upload when UPDATE rolls back
- F006: cleanup new upload when validation-error short-circuits
- F025: wire deleteUploadedImageFile() into delete() lifecycle
Stale documentation fixes:
- F009: spec.js comment 'banner' was '16:9 cropped', now matches 220px cap
- F010: spec.js comment 'contained' was 'max-height 480px', now 320px
- F011: spec.js comment 'thumb' was 'right-floated', now grid
- F012: event-detail.php block comment 'centred' → 'left-aligned'
- F013: SettingsController updateEventSettings() docblock — was inverted
(said 'contained' preserves legacy 100%-width; that is 'full', not 'contained')
Test robustness:
- F032: locale-aware EVENT_URL_PREFIX (was hardcoded /eventi/, silently
404'd on en_US / de_DE environments)
- F033: snapshot+restore events_page_enabled around the suite (was
permanently enabling it on every run)
Sanity: PHPStan clean on the 3 modified PHP files; php -l clean on all 4.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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/Controllers/EventsController.php`:
- Line 480: Rimuovi l'operatore di soppressione sullo chiamata unlink() in
deleteUploadedImageFile(), controlla il valore di ritorno di
unlink($realCandidate) e in caso di fallimento registra l'errore con
SecureLogger::error() fornendo il percorso ($realCandidate) e i dettagli
dell'errore (es. error_get_last() o message generico) per avere visibilità su
permessi/path; assicurati inoltre di non ignorare il risultato e di non usare
error_log().
- Around line 413-421: The code assumes $stmt->execute() == true means a row was
updated, which can leave an uploaded image orphaned; change the logic in
EventsController around handleImageUpload(), $stmt->execute(), and the cleanup
using deleteUploadedImageFile() so you verify the UPDATE actually affected a row
(e.g. check mysqli_stmt_affected_rows() or $stmt->affected_rows) and only set
$_SESSION['success_message'] and keep the new image when affected_rows > 0; if
affected_rows === 0, treat it as a failure: delete the newly uploaded image
(using deleteUploadedImageFile() or the same cleanup path), set an error
message, and do not claim success; alternatively, perform a SELECT to confirm
the target row exists before calling handleImageUpload().
In `@app/Controllers/SettingsController.php`:
- Line 969: The redirect in SettingsController currently returns a hardcoded URL
string; replace that with the centralized route resolver (use route_path(...) or
RouteTranslator::route(...)) so routing stays consistent. Locate the redirect
call in SettingsController (the method that returns $this->redirect(...)) and
change the target to use route_path or RouteTranslator::route with the
appropriate route key and query/tab parameter (e.g., route key for admin
settings with tab=cms) so the redirect is generated from the app's routing
system instead of a literal "/admin/settings?tab=cms".
In `@app/Views/frontend/event-detail.php`:
- Around line 464-467: Replace uses of HtmlHelper::e() with
htmlspecialchars(..., ENT_QUOTES, 'UTF-8') for all HTML attributes in this view:
change the escaping for class on the <article> (currently using
HtmlHelper::e(implode(' ', $cardClasses))), the data-event-cover-layout and
class suffix on <figure> (HtmlHelper::e($coverLayout)), and the src and alt
attributes on the <img> (currently HtmlHelper::e(url($event['featured_image']))
and HtmlHelper::e($event['title'])); ensure you call htmlspecialchars with
ENT_QUOTES and 'UTF-8' for each attribute value and leave non-attribute contexts
unchanged.
In `@app/Views/settings/index.php`:
- Line 475: Sostituisci l'uso di HtmlHelper::e(...) nella view con
htmlspecialchars(..., ENT_QUOTES, 'UTF-8'): recupera il token tramite
Csrf::ensureToken() (come già fatto) e passalo a htmlspecialchars con ENT_QUOTES
e charset 'UTF-8' invece di HtmlHelper::e; aggiorna l'input hidden che
attualmente usa HtmlHelper::e(Csrf::ensureToken()) per usare
htmlspecialchars(Csrf::ensureToken(), ENT_QUOTES, 'UTF-8') in modo da rispettare
la policy delle view.
In `@tests/issue-137-event-image-layout.spec.js`:
- Around line 149-163: The teardown in test.afterAll is leaving global DB state
and orphan files because it always DELETEs the cms.event_image_layout row
instead of restoring the pre-test value and it removes the event row directly
(bypassing controller file cleanup), so change the cleanup to restore the
original event_image_layout setting (use originalEventsImageLayout or check
existence and UPDATE/INSERT rather than unconditional DELETE) and ensure
uploaded files are removed by invoking the same controller cleanup used in the
test or explicitly deleting files associated with EVENT_SLUG after
dbExec(`DELETE FROM events...`) (or better: call the controller's delete method
for the event to run its file cleanup); also keep the existing restore logic for
events_page_enabled using originalEventsPageEnabled and the
eventsPageEnabledWasAbsent flag.
- Around line 377-380: After submitting the form you must wait for and click the
SweetAlert confirm button before asserting the redirect; replace the current
Promise.all usage so that after calling submitButton.click() you first await
page.waitForSelector('.swal2-confirm', { timeout: 15000 }) and click that
element ('.swal2-confirm') and only then await
page.waitForURL(/\/admin\/cms\/events(\?|$)/, { timeout: 15000 }); this ensures
the SweetAlert confirmation is handled reliably instead of racing straight to
page.waitForURL with page.waitForURL and submitButton.click.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1bcf5364-236b-461a-bc67-1e98a7f00d9d
📒 Files selected for processing (11)
app/Controllers/EventsController.phpapp/Controllers/FrontendController.phpapp/Controllers/SettingsController.phpapp/Routes/web.phpapp/Views/frontend/event-detail.phpapp/Views/settings/index.phplocale/de_DE.jsonlocale/en_US.jsonlocale/fr_FR.jsonlocale/it_IT.jsontests/issue-137-event-image-layout.spec.js
Comment on lines
413
to
+421
| if ($stmt->execute()) { | ||
| $_SESSION['success_message'] = __('Evento aggiornato con successo!'); | ||
| // Cleanup orphan file on disk now that the DB row no longer | ||
| // references it. Skipped when the old path matches the new | ||
| // one (defensive — should not happen, but unlinking would | ||
| // delete a still-referenced file). | ||
| if ($updateImage && $oldImagePath !== null && $oldImagePath !== $featuredImagePath) { | ||
| $this->deleteUploadedImageFile($oldImagePath); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and print relevant sections around the reported lines in EventsController.php
FILE="app/Controllers/EventsController.php"
if [ ! -f "$FILE" ]; then
echo "Missing $FILE"
exit 1
fi
echo "---- file: $FILE ----"
echo "---- lines 360-470 ----"
nl -ba "$FILE" | sed -n '360,470p'
echo "---- search for handleImageUpload usage ----"
rg -n "handleImageUpload|deleteUploadedImageFile|affected_rows|affectedRows|execute\(" "$FILE"
echo "---- inspect DB stmt type hints / creation near execute ----"
# show area where $stmt is prepared/created for context
rg -n "\$stmt\s*=\s*\$?db.*prepare|\$stmt\s*=\s*.*prepare|->prepare\(" "$FILE"Repository: fabiodalez-dev/Pinakes
Length of output: 205
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="app/Controllers/EventsController.php"
if [ ! -f "$FILE" ]; then
echo "Missing $FILE"
exit 1
fi
echo "---- file: $FILE ----"
nl -ba "$FILE" | sed -n '380,460p'
echo "---- nearby: 320-420 for ordering ----"
nl -ba "$FILE" | sed -n '320,420p'
echo "---- search for relevant symbols ----"
rg -n "handleImageUpload|deleteUploadedImageFile|affected_rows|affectedRows|execute\(|WHERE id|UPDATE" "$FILE"Repository: fabiodalez-dev/Pinakes
Length of output: 181
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="app/Controllers/EventsController.php"
if [ ! -f "$FILE" ]; then
echo "Missing $FILE"
exit 1
fi
echo "---- file: $FILE ----"
nl -ba "$FILE" | sed -n '390,440p'
echo "---- lines 330-390 for ordering/handle upload ----"
nl -ba "$FILE" | sed -n '330,390p'
echo "---- search for relevant symbols ----"
rg -n "handleImageUpload|deleteUploadedImageFile|affected_rows|affectedRows|execute\(|WHERE id|UPDATE" "$FILE"Repository: fabiodalez-dev/Pinakes
Length of output: 180
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="app/Controllers/EventsController.php"
echo "---- file: $FILE (exist?) ----"
ls -la "$FILE" || exit 1
echo "---- lines 380-460 ----"
cat -n "$FILE" | sed -n '380,460p'
echo "---- lines 320-400 ----"
cat -n "$FILE" | sed -n '320,400p'
echo "---- search symbols ----"
rg -n "handleImageUpload|deleteUploadedImageFile|affected_rows|affectedRows|execute\(|WHERE id|UPDATE" "$FILE"Repository: fabiodalez-dev/Pinakes
Length of output: 11423
Non basarti su if ($stmt->execute()): può lasciare file orfani e mostrare successo falso.
In app/Controllers/EventsController.php l’upload avviene prima dell’UPDATE (handleImageUpload()), e il file nuovo viene cancellato solo nel ramo else di if ($stmt->execute()). Con mysqli execute() può tornare true anche quando WHERE id = ? non trova righe (id inesistente), quindi qui si imposta success_message e si lascia sul disco un’immagine senza riferimento DB. Serve verificare l’esistenza della riga prima dell’upload oppure distinguere “nessun match” dopo l’UPDATE (e quindi fare cleanup del file appena caricato).
🤖 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/Controllers/EventsController.php` around lines 413 - 421, The code
assumes $stmt->execute() == true means a row was updated, which can leave an
uploaded image orphaned; change the logic in EventsController around
handleImageUpload(), $stmt->execute(), and the cleanup using
deleteUploadedImageFile() so you verify the UPDATE actually affected a row (e.g.
check mysqli_stmt_affected_rows() or $stmt->affected_rows) and only set
$_SESSION['success_message'] and keep the new image when affected_rows > 0; if
affected_rows === 0, treat it as a failure: delete the newly uploaded image
(using deleteUploadedImageFile() or the same cleanup path), set an error
message, and do not claim success; alternatively, perform a SELECT to confirm
the target row exists before calling handleImageUpload().
| ConfigStore::set('cms.event_image_layout', $layout); | ||
|
|
||
| $_SESSION['success_message'] = __('Impostazioni eventi aggiornate.'); | ||
| return $this->redirect($response, '/admin/settings?tab=cms'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Evita percorso hardcoded nel redirect.
In Line 969 il redirect usa una URL hardcoded; instrada tramite resolver di route centralizzato (route_path(...) / RouteTranslator::route(...)) per mantenere coerenza con il routing applicativo.
As per coding guidelines, Route: mai hardcodare percorsi URL, usare route_path('key') o RouteTranslator::route('key').
🤖 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/Controllers/SettingsController.php` at line 969, The redirect in
SettingsController currently returns a hardcoded URL string; replace that with
the centralized route resolver (use route_path(...) or
RouteTranslator::route(...)) so routing stays consistent. Locate the redirect
call in SettingsController (the method that returns $this->redirect(...)) and
change the target to use route_path or RouteTranslator::route with the
appropriate route key and query/tab parameter (e.g., route key for admin
settings with tab=cms) so the redirect is generated from the app's routing
system instead of a literal "/admin/settings?tab=cms".
Comment on lines
+464
to
467
| <article class="<?= HtmlHelper::e(implode(' ', $cardClasses)) ?>"> | ||
| <?php if ($hasCoverImage): ?> | ||
| <figure class="event-cover event-cover--<?= HtmlHelper::e($coverLayout) ?>" data-event-cover-layout="<?= HtmlHelper::e($coverLayout) ?>"> | ||
| <img src="<?= HtmlHelper::e(url($event['featured_image'])) ?>" alt="<?= HtmlHelper::e($event['title']) ?>"> |
There was a problem hiding this comment.
Sostituisci HtmlHelper::e() con htmlspecialchars(..., ENT_QUOTES, 'UTF-8') negli attributi HTML.
Nelle Line 464-467 stai facendo escaping attributi con HtmlHelper::e(), ma nelle view il progetto richiede esplicitamente htmlspecialchars(...) (in particolare per url(...) in attributi come src).
Patch proposta
- <article class="<?= HtmlHelper::e(implode(' ', $cardClasses)) ?>">
+ <article class="<?= htmlspecialchars(implode(' ', $cardClasses), ENT_QUOTES, 'UTF-8') ?>">
<?php if ($hasCoverImage): ?>
- <figure class="event-cover event-cover--<?= HtmlHelper::e($coverLayout) ?>" data-event-cover-layout="<?= HtmlHelper::e($coverLayout) ?>">
- <img src="<?= HtmlHelper::e(url($event['featured_image'])) ?>" alt="<?= HtmlHelper::e($event['title']) ?>">
+ <figure class="event-cover event-cover--<?= htmlspecialchars($coverLayout, ENT_QUOTES, 'UTF-8') ?>" data-event-cover-layout="<?= htmlspecialchars($coverLayout, ENT_QUOTES, 'UTF-8') ?>">
+ <img src="<?= htmlspecialchars(url($event['featured_image']), ENT_QUOTES, 'UTF-8') ?>" alt="<?= htmlspecialchars((string)($event['title'] ?? ''), ENT_QUOTES, 'UTF-8') ?>">
</figure>
<?php endif; ?>As per coding guidelines app/Views/**: - CRITICO: htmlspecialchars(url(...), ENT_QUOTES, 'UTF-8') in TUTTI gli attributi HTML (href, action, src) - Mai usare HtmlHelper::e() nelle view — usare htmlspecialchars(..., ENT_QUOTES, 'UTF-8').
🤖 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/Views/frontend/event-detail.php` around lines 464 - 467, Replace uses of
HtmlHelper::e() with htmlspecialchars(..., ENT_QUOTES, 'UTF-8') for all HTML
attributes in this view: change the escaping for class on the <article>
(currently using HtmlHelper::e(implode(' ', $cardClasses))), the
data-event-cover-layout and class suffix on <figure>
(HtmlHelper::e($coverLayout)), and the src and alt attributes on the <img>
(currently HtmlHelper::e(url($event['featured_image'])) and
HtmlHelper::e($event['title'])); ensure you call htmlspecialchars with
ENT_QUOTES and 'UTF-8' for each attribute value and leave non-attribute contexts
unchanged.
| ?> | ||
| <div class="mt-5 pt-5 border-t border-gray-200"> | ||
| <form action="<?= htmlspecialchars(url('/admin/settings/events'), ENT_QUOTES, 'UTF-8') ?>" method="post" class="space-y-3"> | ||
| <input type="hidden" name="csrf_token" value="<?= HtmlHelper::e(Csrf::ensureToken()) ?>"> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Nelle view non usare HtmlHelper::e() per il token CSRF.
In Line 475 sostituisci HtmlHelper::e(...) con htmlspecialchars(..., ENT_QUOTES, 'UTF-8') per aderire alla policy delle view.
As per coding guidelines, Mai usare HtmlHelper::e() nelle view — usare htmlspecialchars(..., ENT_QUOTES, 'UTF-8').
🤖 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/Views/settings/index.php` at line 475, Sostituisci l'uso di
HtmlHelper::e(...) nella view con htmlspecialchars(..., ENT_QUOTES, 'UTF-8'):
recupera il token tramite Csrf::ensureToken() (come già fatto) e passalo a
htmlspecialchars con ENT_QUOTES e charset 'UTF-8' invece di HtmlHelper::e;
aggiorna l'input hidden che attualmente usa HtmlHelper::e(Csrf::ensureToken())
per usare htmlspecialchars(Csrf::ensureToken(), ENT_QUOTES, 'UTF-8') in modo da
rispettare la policy delle view.
Comment on lines
+377
to
+380
| await Promise.all([ | ||
| page.waitForURL(/\/admin\/cms\/events(\?|$)/, { timeout: 15000 }), | ||
| submitButton.click(), | ||
| ]); |
There was a problem hiding this comment.
Dopo il submit manca la conferma SweetAlert obbligatoria.
Aspetti subito il redirect, ma in questo repo dopo il submit va verificato e cliccato .swal2-confirm. Senza quel passaggio il test diventa fragile o può bloccarsi quando la dialog compare.
As per coding guidelines, SweetAlert: dopo form submit, verificare e cliccare .swal2-confirm``.
🤖 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 `@tests/issue-137-event-image-layout.spec.js` around lines 377 - 380, After
submitting the form you must wait for and click the SweetAlert confirm button
before asserting the redirect; replace the current Promise.all usage so that
after calling submitButton.click() you first await
page.waitForSelector('.swal2-confirm', { timeout: 15000 }) and click that
element ('.swal2-confirm') and only then await
page.waitForURL(/\/admin\/cms\/events(\?|$)/, { timeout: 15000 }); this ensures
the SweetAlert confirmation is handled reliably instead of racing straight to
page.waitForURL with page.waitForURL and submitButton.click.
Owner
Author
Walkthrough decisions
Walking the Qualifying scope: of 5 non-auto-eligible finding(s), 0 auto-accepted, 5 promoted, 0 skipped, 0 stopped. Promoted findings will be picked up by the next Promoted
Decisions log: this comment is append-only audit — it's never edited in place. Each |
…ld=40)
Applied by /adamsreview:fix run_id=fixrun_20260521T103644Z7b1556.
- F015 (settings/index.php): swap 'Salva impostazioni eventi' button from
bg-purple-600 to bg-gray-900 — aligns with the libri/autori/editori
Tailwind chrome guideline (memory feedback_ui_style_and_i18n_blocking).
Kept bg-purple-100 on the Eventi card icon (category marker).
- F018 (event-detail.php): document the intentional asymmetry on
.event-cover--contained (no border/background while other variants
keep the .event-cover base chrome) — comment-only change.
- F020 (event-detail.php): bound the sticky thumb to the viewport with
max-height: calc(100vh - 9rem) + overflow: hidden inside the >=768px
grid media query. Prevents the figure from spilling past the article
on short-body events.
- F031 (EventsController.php): rewrite the misleading comment above the
pre-UPDATE SELECT featured_image. Old text said 'BEFORE the error
short-circuit so $oldImagePath is always populated'; in reality only
the success branch consumes $oldImagePath — the error-short-circuit
and UPDATE-fail branches unlink $featuredImagePath.
Sanity: PHPStan clean on EventsController + event-detail; php -l clean
on all 3 modified files.
Triage of the CodeRabbit review pass: VALID (fixed): - EventsController::deleteUploadedImageFile (line 482): the @Unlink silenced any I/O failure. Replaced with a checked unlink() call that logs failures via SecureLogger::error with the resolved path, errno from error_get_last(), and message — orphan files now surface in the operator log instead of accumulating silently. - tests/issue-137-event-image-layout.spec.js teardown: previously DELETEd cms.event_image_layout unconditionally, destroying an admin's pre-existing custom choice on every run. Snapshot/restore pattern extended (mirrors the existing events_page_enabled handling). Captures original value in beforeAll, INSERTs back or DELETEs in afterAll. SKIPPED (with reason): - affected_rows guard around UPDATE: race condition is real but pre-existing across every updateXxx controller in the repo (libri, autori, editori). Touching only EventsController would create inconsistency for a marginal edge case; the id is already 404-validated by edit() before the form posts. - SettingsController::updateEventSettings redirect target: /admin/... routes are not in routes_*.json (only public routes are translated). Every other settings-update redirect in the same file uses the same literal pattern; changing only this one introduces asymmetry. - HtmlHelper::e() in event-detail.php / settings/index.php: the pre-commit hook explicitly accepts both htmlspecialchars and HtmlHelper::e (which calls the same escape internally). 20+ uses of HtmlHelper::e in the same files predate this PR. - .swal2-confirm wait after the admin events form submit: that form has no SweetAlert handler (verified by grep on app/Views/events/form.php). The current Promise.all([waitForURL, click]) pattern passes 8/8 — adding swal2-confirm wait would hang the test. Tests: tests/issue-137-event-image-layout.spec.js — 8/8 pass after the teardown change. PHPStan + php -l clean on EventsController.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/issue-137-event-image-layout.spec.js (1)
172-175:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIl teardown lascia ancora orfano il file caricato dal test.
DELETE FROM events ...pulisce solo il DB. Il testadmin update...crea un upload reale in/uploads/events, quindi a fine suite l’immagine resta sul disco. Recuperafeatured_imageprima delDELETEe rimuovilo esplicitamente, oppure passa dalla stessa delete applicativa che esegue la cleanup.🤖 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 `@tests/issue-137-event-image-layout.spec.js` around lines 172 - 175, Il teardown attuale esegue solo dbExec(`DELETE FROM events WHERE slug='${sqlEscape(EVENT_SLUG)}'`) lasciando il file caricato in /uploads/events; modifica il teardown in tests/issue-137-event-image-layout.spec.js per prima cosa leggere il valore di featured_image del record identificato da EVENT_SLUG (es. SELECT featured_image FROM events WHERE slug=...), se presente rimuovere esplicitamente il file corrispondente in /uploads/events/<featured_image> (o invocare la stessa API di delete usata dall'applicazione) e solo dopo eseguire la dbExec DELETE; assicurati di gestire l'assenza del file senza fallire il teardown.
🤖 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.
Duplicate comments:
In `@tests/issue-137-event-image-layout.spec.js`:
- Around line 172-175: Il teardown attuale esegue solo dbExec(`DELETE FROM
events WHERE slug='${sqlEscape(EVENT_SLUG)}'`) lasciando il file caricato in
/uploads/events; modifica il teardown in
tests/issue-137-event-image-layout.spec.js per prima cosa leggere il valore di
featured_image del record identificato da EVENT_SLUG (es. SELECT featured_image
FROM events WHERE slug=...), se presente rimuovere esplicitamente il file
corrispondente in /uploads/events/<featured_image> (o invocare la stessa API di
delete usata dall'applicazione) e solo dopo eseguire la dbExec DELETE;
assicurati di gestire l'assenza del file senza fallire il teardown.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3e2b7588-ccdd-46bb-bed0-6d28e3e606f1
📒 Files selected for processing (2)
app/Controllers/EventsController.phptests/issue-137-event-image-layout.spec.js
…low-up) CodeRabbit re-flagged tests/issue-137-event-image-layout.spec.js afterAll: the previous fix restored the event_image_layout setting but still bypassed the controller's delete() — the admin-update test uses page.setInputFiles with a real JPEG buffer, which the admin controller moves to /uploads/events/event_YYYYMMDD_HHMMSS_<hex>.jpg via handleImageUpload(). Without the controller, DELETE FROM events left that file orphan on disk every run. Fix: before the dbExec(DELETE FROM events ...), SELECT the current featured_image, and if it starts with /uploads/events/ (i.e. it's a real upload, not the static seed /assets/books.jpg), unlinkSync the file. ENOENT is tolerated (the controller may have already cleaned it on the success path of the admin-update test). Added 'fs' and 'path' to the spec's requires. Validation: ran the suite — 8/8 pass; the /uploads/events directory now stays at the same orphan count across runs instead of growing.
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 `@tests/issue-137-event-image-layout.spec.js`:
- Around line 190-193: La guardia attuale con
currentImage.startsWith('/uploads/events/') non impedisce path traversal e
path.join può risolvere fuori dalla cartella prevista; sostituisci il check con
una normalizzazione e confronto assoluto: calcola absBase =
path.resolve(__dirname, '..', 'public', 'uploads', 'events') e absPath =
path.resolve(__dirname, '..', 'public', currentImage) (o rimuovi il prefisso
slash da currentImage prima di join), poi verifica che
absPath.startsWith(absBase + path.sep) prima di chiamare fs.unlinkSync(absPath);
usa queste variabili (currentImage, absPath, absBase, fs.unlinkSync,
path.resolve) per localizzare e applicare la correzione.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e3c65f28-b0dd-4b9b-a878-790374be99f5
📒 Files selected for processing (1)
tests/issue-137-event-image-layout.spec.js
Comment on lines
+190
to
+193
| if (currentImage && currentImage.startsWith('/uploads/events/')) { | ||
| const absPath = path.join(__dirname, '..', 'public', currentImage); | ||
| try { | ||
| fs.unlinkSync(absPath); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
node <<'NODE'
const path = require('path');
const legacyBase = path.join('/repo/tests', '..', 'public');
const uploadsRoot = path.resolve('/repo/public/uploads/events');
const crafted = '/uploads/events/../../../../tmp/pwned.txt';
const legacyPath = path.join(legacyBase, crafted);
const safePath = path.resolve(uploadsRoot, crafted.slice('/uploads/events/'.length));
const escaped = path.relative(uploadsRoot, safePath);
console.log({ legacyPath, safePath, escaped });
console.log('legacy escapes uploads root:', !legacyPath.startsWith(uploadsRoot));
console.log('safe guard should reject:', escaped.startsWith('..') || path.isAbsolute(escaped));
NODERepository: fabiodalez-dev/Pinakes
Length of output: 236
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="tests/issue-137-event-image-layout.spec.js"
# show lines around teardown unlink logic
nl -ba "$FILE" | sed -n '160,240p'Repository: fabiodalez-dev/Pinakes
Length of output: 109
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="tests/issue-137-event-image-layout.spec.js"
awk 'NR>=160 && NR<=240 {printf "%d:%s\n", NR, $0}' "$FILE"Repository: fabiodalez-dev/Pinakes
Length of output: 4298
🏁 Script executed:
#!/bin/bash
node <<'NODE'
const path = require('path');
const legacyBase = path.join('/repo/tests', '..', 'public');
const uploadsRoot = path.resolve('/repo/public/uploads/events');
const crafted = '/uploads/events/../../../../tmp/pwned.txt';
const legacyPath = path.join(legacyBase, crafted);
const safePath = path.resolve(uploadsRoot, crafted.slice('/uploads/events/'.length));
const escaped = path.relative(uploadsRoot, safePath);
console.log({ legacyPath, safePath, escaped });
console.log('legacy escapes uploads root:', !legacyPath.startsWith(uploadsRoot));
console.log('safe guard should reject:', escaped.startsWith('..') || path.isAbsolute(escaped));
NODERepository: fabiodalez-dev/Pinakes
Length of output: 236
🏁 Script executed:
#!/bin/bash
node <<'NODE'
const path = require('path');
const legacyBase = path.join('/repo/tests', '..', 'public');
const uploadsRoot = path.resolve('/repo/public/uploads/events');
const crafted = '/uploads/events/../../../../tmp/pwned.txt';
const legacyPath = path.join(legacyBase, crafted);
const safePath = path.resolve(uploadsRoot, crafted.slice('/uploads/events/'.length));
const escaped = path.relative(uploadsRoot, safePath);
console.log({ legacyPath, safePath, escaped });
console.log('legacy escapes uploads root:', !legacyPath.startsWith(uploadsRoot));
console.log('safe guard should reject:', escaped.startsWith('..') || path.isAbsolute(escaped));
NODERepository: fabiodalez-dev/Pinakes
Length of output: 236
🏁 Script executed:
#!/bin/bash
node <<'NODE'
const path = require('path');
const uploadsRoot = path.resolve('/repo/public/uploads/events');
const crafted = '/uploads/events/../../../../tmp/pwned.txt';
const safePath = path.resolve(uploadsRoot, crafted.slice('/uploads/events/'.length));
const escaped = path.relative(uploadsRoot, safePath);
console.log({ safePath, escaped });
console.log('safe guard should reject:', escaped.startsWith('..') || path.isAbsolute(escaped));
NODERepository: fabiodalez-dev/Pinakes
Length of output: 166
🏁 Script executed:
#!/bin/bash
node <<'NODE'
const path = require('path');
const uploadsRoot = path.resolve('/repo/public/uploads/events');
const crafted = '/uploads/events/../../../../tmp/pwned.txt';
const safePath = path.resolve(uploadsRoot, crafted.slice('/uploads/events/'.length));
const escaped = path.relative(uploadsRoot, safePath);
console.log({ safePath, escaped });
console.log('safe guard should reject:', escaped.startsWith('..') || path.isAbsolute(escaped));
NODERepository: fabiodalez-dev/Pinakes
Length of output: 166
🏁 Script executed:
#!/bin/bash
node <<'NODE'
const path = require('path');
const uploadsRoot = path.resolve('/repo/public/uploads/events');
const crafted = '/uploads/events/../../../../tmp/pwned.txt';
const safePath = path.resolve(uploadsRoot, crafted.slice('/uploads/events/'.length));
const escaped = path.relative(uploadsRoot, safePath);
console.log({ safePath, escaped });
console.log('safe guard should reject:', escaped.startsWith('..') || path.isAbsolute(escaped));
NODERepository: fabiodalez-dev/Pinakes
Length of output: 166
🏁 Script executed:
#!/bin/bash
node <<'NODE'
const path = require('path');
const uploadsRoot = path.resolve('/repo/public/uploads/events');
const crafted = '/uploads/events/../../../../tmp/pwned.txt';
const safePath = path.resolve(uploadsRoot, crafted.slice('/uploads/events/'.length));
const escaped = path.relative(uploadsRoot, safePath);
console.log({ safePath, escaped });
console.log('safe guard should reject:', escaped.startsWith('..') || path.isAbsolute(escaped));
NODERepository: fabiodalez-dev/Pinakes
Length of output: 166
🏁 Script executed:
#!/bin/bash
node <<'NODE'
const path = require('path');
const uploadsRoot = path.resolve('/repo/public/uploads/events');
const crafted = '/uploads/events/../../../../tmp/pwned.txt';
const safePath = path.resolve(uploadsRoot, crafted.slice('/uploads/events/'.length));
const escaped = path.relative(uploadsRoot, safePath);
console.log({ safePath, escaped });
console.log('safe guard should reject:', escaped.startsWith('..') || path.isAbsolute(escaped));
NODERepository: fabiodalez-dev/Pinakes
Length of output: 166
🏁 Script executed:
#!/bin/bash
node <<'NODE'
const path = require('path');
const legacyBase = path.join('/repo/tests', '..', 'public');
const uploadsRoot = path.resolve('/repo/public/uploads/events');
const crafted = '/uploads/events/../../../../tmp/pwned.txt';
const legacyPath = path.join(legacyBase, crafted);
const safePath = path.resolve(uploadsRoot, crafted.slice('/uploads/events/'.length));
const escaped = path.relative(uploadsRoot, safePath);
console.log({ legacyPath, safePath, escaped });
console.log('legacy escapes uploads root:', !legacyPath.startsWith(uploadsRoot));
console.log('safe guard should reject:', escaped.startsWith('..') || path.isAbsolute(escaped));
NODERepository: fabiodalez-dev/Pinakes
Length of output: 236
🏁 Script executed:
#!/bin/bash
node <<'NODE'
const path = require('path');
const uploadsRoot = path.resolve(process.cwd(), 'public', 'uploads', 'events');
const crafted = '/uploads/events/../../../../tmp/pwned.txt';
const safePath = path.resolve(uploadsRoot, crafted.slice('/uploads/events/'.length));
const escaped = path.relative(uploadsRoot, safePath);
console.log({ safePath, escaped });
console.log('safe guard should reject:', escaped.startsWith('..') || path.isAbsolute(escaped));
NODERepository: fabiodalez-dev/Pinakes
Length of output: 184
🏁 Script executed:
#!/bin/bash
node <<'NODE'
const path = require('path');
const uploadsRoot = path.resolve('/repo/public/uploads/events');
const crafted = '/uploads/events/../../../../tmp/pwned.txt';
const safePath = path.resolve(uploadsRoot, crafted.slice('/uploads/events/'.length));
const escaped = path.relative(uploadsRoot, safePath);
console.log({ safePath, escaped });
console.log('safe guard should reject:', escaped.startsWith('..') || path.isAbsolute(escaped));
NODERepository: fabiodalez-dev/Pinakes
Length of output: 166
Teardown: impedire path traversal prima di fs.unlinkSync (cancellazioni fuori da public/uploads/events)
Nel teardown di tests/issue-137-event-image-layout.spec.js (~190-193) la guardia currentImage.startsWith('/uploads/events/') non impedisce traversal con ../../..; inoltre path.join(__dirname, '..', 'public', currentImage) con currentImage assoluto può risolvere fuori da public/uploads/events, rendendo unlinkSync potenzialmente distruttivo.
🔒 Fix minimale
- if (currentImage && currentImage.startsWith('/uploads/events/')) {
- const absPath = path.join(__dirname, '..', 'public', currentImage);
- try {
- fs.unlinkSync(absPath);
- } catch (e) {
- // ENOENT (already gone) is acceptable — the controller's
- // own cleanup may have unlinked it on the success path.
- // Any other error we surface to the test log but do not
- // fail the teardown — the suite has finished otherwise.
- if (e && e.code !== 'ENOENT') {
- console.warn(`teardown: could not unlink ${absPath}: ${e.message}`);
- }
- }
- }
+ if (currentImage && currentImage.startsWith('/uploads/events/')) {
+ const uploadsRoot = path.resolve(__dirname, '..', 'public', 'uploads', 'events');
+ const relativeUploadPath = currentImage.slice('/uploads/events/'.length);
+ const absPath = path.resolve(uploadsRoot, relativeUploadPath);
+ const escaped = path.relative(uploadsRoot, absPath);
+ if (escaped.startsWith('..') || path.isAbsolute(escaped)) {
+ console.warn(`teardown: refusing to unlink path outside uploads root: ${currentImage}`);
+ } else {
+ try {
+ fs.unlinkSync(absPath);
+ } catch (e) {
+ if (e && e.code !== 'ENOENT') {
+ console.warn(`teardown: could not unlink ${absPath}: ${e.message}`);
+ }
+ }
+ }
+ }🤖 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 `@tests/issue-137-event-image-layout.spec.js` around lines 190 - 193, La
guardia attuale con currentImage.startsWith('/uploads/events/') non impedisce
path traversal e path.join può risolvere fuori dalla cartella prevista;
sostituisci il check con una normalizzazione e confronto assoluto: calcola
absBase = path.resolve(__dirname, '..', 'public', 'uploads', 'events') e absPath
= path.resolve(__dirname, '..', 'public', currentImage) (o rimuovi il prefisso
slash da currentImage prima di join), poi verifica che
absPath.startsWith(absBase + path.sep) prima di chiamare fs.unlinkSync(absPath);
usa queste variabili (currentImage, absPath, absBase, fs.unlinkSync,
path.resolve) per localizzare e applicare la correzione.
5 tasks
fabiodalez-dev
added a commit
that referenced
this pull request
May 22, 2026
…able tests CodeRabbit fix on PR #141 ------------------------- attachSwalConfirm's confirmText default was unconditionally 'Elimina', even when data-swal-confirm-kind="action" routed the dialog through SwalApp.confirm (the neutral/gray helper). A form opting into kind="action" without an explicit data-swal-confirm-button would render a gray button labelled "Elimina" — the wrong destructive cue. Default now follows the kind: action→Conferma, destructive→Elimina. Explicit data-swal-confirm-button still wins for both. UX follow-ups (3 non-destructive forms) --------------------------------------- - admin/languages/index.php: set-default-language → kind=action - utenti/dettagli_utente.php: activate-directly → kind=action - utenti/index.php: activate-directly → kind=action These three flows are reversible (re-set language, deactivate user) and were showing a red destructive dialog. Now neutral/gray. 49 reusable Playwright tests ---------------------------- - tests/sweetalert2-comprehensive.spec.js (25 tests): Block A — window.SwalApp bus contract (6) Block B — attachSwalConfirm auto-wire / idempotence / proceed-marker (5) Block C — kind routing + confirmText defaults (3, validates this fix) Block D — i18n & data-attr propagation (3) Block E — refactored views per-page assertions (8, login-gated, skip-clean) - tests/pr-139-event-image-additional.spec.js (11 tests): Block S — controller orphan-cleanup symmetry + path-traversal hardening + locale completeness + settings/route contract (9) Block H — admin-side HTTP contract checks (2, login-gated) - tests/pr-136-archives-ric-cm-additional.spec.js (13 tests): Migration version-le-release invariant (project rule #6), ensureSchema() pattern compliance, plugin.json bumps, RiC-O entity coverage, README freshness. Reads files from origin/feat/ric-cm-archives via 'git show', so the suite works from any branch without checking out. Validation ---------- - 39 no-login tests run green in ~30s - 10 login-gated tests skip cleanly when E2E_ADMIN_PASS is absent - All 13 pr-136 tests pass against origin/feat/ric-cm-archives Refs: PR #141 (closes #140), PR #139 (closes #137), PR #136 (closes #122)
4 tasks
Owner
Author
|
Superseded by #144 (combined/release-unified) — the unified release branch merges this work alongside the other two open PRs into a single release. All commits from this branch are preserved in the merge; CI green on the combined branch. This PR is closed, not merged — the branch |
ctariel
pushed a commit
to ctariel/Pinakes
that referenced
this pull request
Jun 4, 2026
…#141 + fabiodalez-dev#139) into unified release Brings in: - PR fabiodalez-dev#141 (closes fabiodalez-dev#140): full SweetAlert2 popup unification — every native window.alert/confirm/prompt routed through a single SwalApp helper bus with defensive native fallback. 11+ form/onsubmit/onclick refactors via the `data-swal-confirm` attribute pattern. - PR fabiodalez-dev#139 (closes fabiodalez-dev#137): admin-configurable event image layout — 4 presets (full / banner / contained / thumb) on /eventi/<slug>, picker in admin/settings?tab=cms. - Validation work that landed during the review pass: - CodeRabbit-driven fix: confirmText default follows the kind (action→Conferma, destructive→Elimina) instead of always defaulting to Elimina (commit 383b0a2) - UX follow-ups: set-default-language + activate-user → kind=action (commit 450ed8e) - 49 reusable Playwright regression tests: sweetalert2-comprehensive.spec.js (25) pr-139-event-image-additional.spec.js (11) pr-136-archives-ric-cm-additional.spec.js (13) - Block E selector hardening for real admin URLs + DB seed state (commit 5701c58) - Phase 21 (full-test.spec.js) language-switch tests adapted to the SwalApp confirmation modal (commit 318117b) - CodeRabbit-driven fix on firstDeletableId: check ALL dep keys (children_count AND libri_count for generi) rather than just one (commit dcf98a0) PR fabiodalez-dev#139 commits are already included in fabiodalez-dev#141's stack — no duplication. Refs: PR fabiodalez-dev#141 (closes fabiodalez-dev#140), PR fabiodalez-dev#139 (closes fabiodalez-dev#137) # Conflicts: # locale/de_DE.json # locale/en_US.json # locale/fr_FR.json # locale/it_IT.json
ctariel
pushed a commit
to ctariel/Pinakes
that referenced
this pull request
Jun 4, 2026
…+ fabiodalez-dev#139 + fabiodalez-dev#136 Adds tests/unified-release-regression.spec.js — a focused regression suite that targets the user-observable contract each of the 3 combined PRs introduces. Designed to sit alongside the existing per-PR specs (sweetalert2-comprehensive, pr-139-event-image-additional, pr-136-archives-ric-cm-additional, full-test, etc.) as a fast, re-runnable safety net for the release branch. Distribution (25 tests): Block U-SWAL (8) — PR fabiodalez-dev#141 popup unification S1: SwalApp method surface S2: confirmText default branches on isAction (CodeRabbit fix) S3: defensive native fallback when Swal absent S4: one-shot data-swal-proceed marker (not attribute mutation) S5: 3 non-destructive forms set kind="action" (UX follow-up) S6: it_IT.json carries SwalApp default copy H1: window.SwalApp initialised on admin pages H2: data-swal-confirm forms auto-attached on /admin/utenti Block U-EVT (8) — PR fabiodalez-dev#139 event image layout (fabiodalez-dev#137) S1: 4-preset enum in settings/index.php S2: event-detail.php renders event-cover--<preset> S3: SettingsController validates with in_array S4: 4 deleteUploadedImageFile lifecycle callsites S5: path-traversal hardening (prefix + realpath containment) S6: @media restyle for thumb layout H1: /admin/settings?tab=cms renders the layout picker H2: form posts to /admin/settings/events with CSRF Block U-RIC (9) — PR fabiodalez-dev#136 RiC-CM full roadmap (fabiodalez-dev#122) S1: phase 2/3/4 + cleanup migration files S2: version.json ≥ highest migration version S3: 0.7.09.sql creates Phase 3 tables idempotently S4: 0.7.10.sql creates Phase 4 places + relations S5: ArchivesPlugin ensureSchema() called from BOTH hooks S6: RicJsonLdBuilder covers RecordResource/Agent/Activity/Place S7: OaiPmhServerPlugin handles metadataPrefix=ric-o S8: archives plugin.json ≥ 1.5.0, oai-pmh-server ≥ 1.1.0 H1: /archives/collection.ric.json returns JSON-LD Design choices: - 16 tests run without admin login (static checks on repo files + one no-auth HTTP smoke for the public RiC-O endpoint). - 9 tests require login — they test.skip() cleanly on missing credentials, wrong password, or missing seed state (legitimate skip, never fail). - No state mutation — every assertion reads HTML / JSON / static files. Re-runnable in any order, parallel-safe. Run: /tmp/run-e2e.sh tests/unified-release-regression.spec.js \ --config=tests/playwright.config.js --workers=1 Validated locally with E2E_ADMIN_PASS set: 24 passed, 1 skipped (U-SWAL-H2 when /admin/utenti has no pending users — legitimate seed state, not a test bug).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #137.
Adds an admin-controlled layout setting for the featured image on the public event detail page (
/eventi/<slug>). The previous CSS rendered every uploaded image at 100% container width with no height constraint, so a 1791×927 cover (the case @HansUwe52 reported) dominated the viewport.A new setting
cms.event_image_layoutlives in the existing CMS settings tab. Four presets, in increasing visual footprint:All four labels are translated in
it_IT,en_US,de_DE,fr_FRin the same commit (per i18n policy).Architecture
system_settings(category=cms, key=event_image_layout). No migration needed.POST /admin/settings/events(CSRF + AdminAuth middleware), pattern identical toupdateLabels/updateSharingSettings.Bonus fix in the same branch
While reviewing the events admin form a separate bug was found (last commit): when the admin submits the edit form with BOTH a new
featured_imageupload ANDremove_image=1checked, the previousif/elseifevaluatedremove_imagefirst and silently discarded the upload. The branch flips the priority (upload wins) and adds best-effort cleanup of the orphaned file frompublic/uploads/events/, path-traversal-safe.Test plan
8 cases in
tests/issue-137-event-image-layout.spec.js, all passing:cms.event_image_layoutis unset, the page rendersevent-cover--containedfull→event-cover--fullclass +data-event-cover-layout='full'banner→event-cover--bannercontained→event-cover--containedthumb→event-cover--thumbcontained.width ≤ 420px AND < 70% card.width,thumb.width ≤ 245px,banner.height ≤ 225px,full.width > 85% card.width. Also assertscontainedandthumbare left-aligned (figure.x within 100px of card.x).figure.bottom ≤ card.bottom— the invariant the originalfloat: rightdesign violated.handleImageUpload()'s naming scheme, not NULL and not the original.Run locally:
PHPStan: clean on the 5 PHP files touched (pre-existing
storage/plugins/{archives,bibframe-linked-data,discogs}/...warnings onmainremain at 11 and are out of scope here).Summary by CodeRabbit
New Features
UI / Frontend
Bug Fixes
Tests
Localization