feat: genre merge/rearrange, book list filters, plugin hardening#68
Merged
fabiodalez-dev merged 19 commits intomainfrom Feb 24, 2026
Merged
feat: genre merge/rearrange, book list filters, plugin hardening#68fabiodalez-dev merged 19 commits intomainfrom
fabiodalez-dev merged 19 commits intomainfrom
Conversation
- Genre merge: combine two genres, moving all books and subgenres to target - Genre rearrange: move genre to different parent via edit form dropdown - Fix update() to handle setting parent_id to NULL (top-level) - Book list: add collana (series) filter with autocomplete - Genre API: add search preview endpoint for autocomplete - Plugin audit: DigitalLibraryPlugin refactor, version bumps - .htaccess: security headers and caching improvements
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds genre management endpoints and admin UI for update/merge/delete with cycle detection; implements GenereRepository update/delete/merge/getAllFlat; centralizes genre-hierarchy resolution in BookRepository; converts hard-deletes to soft-deletes for books; tightens escaping and secure logging; hardens digital-library upload/asset handling; and adds extensive Playwright E2E suites. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GeneriController
participant GenereRepository
participant Database
Client->>GeneriController: POST /admin/generi/{id}/unisci (target_id)
GeneriController->>GenereRepository: merge(sourceId, targetId)
GenereRepository->>Database: BEGIN
GenereRepository->>Database: Validate source & target, check ancestry
GenereRepository->>Database: Rename collisions, UPDATE children.parent_id -> target
GenereRepository->>Database: UPDATE books -> target (including soft-deleted)
GenereRepository->>Database: DELETE source genre
GenereRepository->>Database: COMMIT
GenereRepository-->>GeneriController: {children_moved, books_updated}
GeneriController-->>Client: Redirect + flash message
sequenceDiagram
participant Client
participant LibriApiController
participant BookRepository
participant Database
Client->>LibriApiController: GET /api/libri (genere, collana, keywords)
LibriApiController->>BookRepository: Query(filters incl. collana LIKE, child genres)
BookRepository->>Database: SELECT with expanded WHERE/join
Database-->>BookRepository: rows
BookRepository->>BookRepository: resolveGenreHierarchy(row)
BookRepository-->>LibriApiController: JSON-ready records (hex-escaped)
LibriApiController-->>Client: JSON response
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 9
🧹 Nitpick comments (5)
storage/plugins/digital-library/DigitalLibraryPlugin.php (2)
268-271: Minor redundancy in the audio URL check.
!empty()already handles undefined array keys without warnings, so the null coalescing fallback?? ''is unnecessary. This isn't a bug — just slightly redundant defensive coding.Optional simplification
- if (!empty($book['audio_url'] ?? '')) { + if (!empty($book['audio_url'])) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/digital-library/DigitalLibraryPlugin.php` around lines 268 - 271, The condition checking the audio URL is redundant because !empty() already handles missing keys; change the guard from if (!empty($book['audio_url'] ?? '')) to if (!empty($book['audio_url'])) before the include of __DIR__ . '/views/frontend-player.php' (update inside the same method in DigitalLibraryPlugin.php where $book is used) so the null-coalescing fallback is removed.
490-494:JSON_HEX_TAGis a solid XSS-hardening addition.This prevents
<script>injection if JSON is ever rendered in an HTML context. Consider also addingJSON_THROW_ON_ERRORso encoding failures (e.g., malformed UTF-8 in data) surface as exceptions rather than silently returningfalse, which would cause$response->getBody()->write()to receive a non-string.Optional: add JSON_THROW_ON_ERROR
- $response->getBody()->write(json_encode($data, JSON_UNESCAPED_UNICODE | JSON_UNESCAPED_SLASHES | JSON_HEX_TAG)); + $response->getBody()->write(json_encode($data, JSON_UNESCAPED_UNICODE | JSON_UNESCAPED_SLASHES | JSON_HEX_TAG | JSON_THROW_ON_ERROR));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/digital-library/DigitalLibraryPlugin.php` around lines 490 - 494, The json method currently uses json_encode with JSON_HEX_TAG but not JSON_THROW_ON_ERROR, so encoding failures can return false and pass a non-string to $response->getBody()->write; update the json method to include JSON_THROW_ON_ERROR in the flags passed to json_encode (or use json_encode via the JSON_THROW_ON_ERROR option) and wrap the encoding in a try/catch for \JsonException inside the json function (method json) so you can log/handle the error and return an appropriate error response (e.g., 500) instead of writing a non-string to the response body; ensure the catch returns a JSON error payload and status and still sets Content-Type via the same $response->withHeader(...) chain.package.json (1)
37-37:@playwright/testversion is valid. The latest published version is1.58.2, so the pinned range is correct.Note: the
"test"script (Line 7) still exits with an error. If Playwright tests are intended to run in CI vianpm test, consider wiring the script:💡 Optional: wire Playwright into the test script
- "test": "echo \"Error: no test specified\" && exit 1", + "test": "playwright test",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 37, The package.json currently pins `@playwright/test` correctly but the "test" script still fails; update the scripts.test entry so running npm test executes Playwright (e.g., replace the failing command with "playwright test" or "npx playwright test") and optionally add a CI-friendly script like "test:ci" that runs "npx playwright test --reporter=dot" or similar; change the scripts object (scripts.test and/or add scripts.test:ci) so Playwright is invoked directly using the installed `@playwright/test` package.app/Models/GenereRepository.php (1)
176-252: Merge implementation is well-structured with good transaction safety.The conflict-renaming logic for child genres (lines 209-216), the multi-step update of books and children, and the transaction handling with rollback on failure are all solid. A few observations:
Stats double-counting:
$booksUpdatedsumsaffected_rowsfrom both thegenere_idandsottogenere_idupdates (lines 228, 234). If a book references the source genre in both columns, it gets counted twice. The user sees "N libri aggiornati" which may overcount. This is cosmetic but could confuse admins.Nested merge detection: The
@@autocommitcheck (line 196-197) works formysqli::begin_transaction()but won't detect rawSTART TRANSACTIONSQL. Fine for the current codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/GenereRepository.php` around lines 176 - 252, The reported overcount comes from summing $stmt->affected_rows for both the genere_id and sottogenere_id UPDATEs in merge(), which double-counts books that referenced the source genre in both columns; fix by computing the number of unique books that reference the source before performing the UPDATEs using a single SELECT COUNT(DISTINCT id) FROM libri WHERE deleted_at IS NULL AND (genere_id = ? OR sottogenere_id = ?) bound with $sourceId, store that in $booksUpdated, then run the two UPDATE statements without adding their affected_rows to $booksUpdated (leave childrenMoved logic unchanged).app/Controllers/GeneriApiController.php (1)
129-133: Only direct self-parent cycles are prevented — deeper cycles like A→B→A are not detected.The code checks
$newParent !== $idto prevent direct self-assignment, but the comment claims cycle prevention without validating the full parent chain. If genre A is a child of genre B, setting B's parent to A would create a cycle but pass this check. With shallow hierarchies (2–3 levels), practical risk is low, but consider adding ancestor chain validation for future robustness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/GeneriApiController.php` around lines 129 - 133, The current check only prevents direct self-parenting by comparing $newParent !== $id but doesn't verify ancestor chains, allowing deeper cycles (e.g., A→B→A); update the parent-assignment logic in GeneriApiController (the block using $data['parent_id'], $newParent, $id and setting $updateData['parent_id']) to walk the ancestor chain starting at $newParent (query each genre's parent_id via your Genre model/repository) and abort or ignore the assignment if any ancestor equals $id (also handle nulls and non-existent parents), so the loop terminates when parent_id is null or exceeds a reasonable depth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Controllers/GeneriApiController.php`:
- Around line 143-146: The catch block in GeneriApiController currently writes
$e->getMessage() to the API response, which can leak internal details; change
the handler so that if $e is an instance of InvalidArgumentException you return
a sanitized client-facing message (and appropriate 400 status) but for all other
Throwables log the full exception (stack trace) to your server logger (or
error_log) and return a generic error message like "Internal Server Error" with
a 500 status; keep using $response->getBody()->write(...) and
->withStatus(...)->withHeader('Content-Type','application/json') but do not
expose $e->getMessage() for non-InvalidArgumentException cases.
In `@app/Models/BookRepository.php`:
- Around line 1002-1029: The code currently misses the chainLen === 0 case
causing stale genre fields; add an explicit branch before the existing if/elseif
that handles an empty $chain by clearing/zeroing all cascade and name fields
(set $row['genere_id_cascade'] = 0, $row['sottogenere_id_cascade'] = 0, and
null/empty $row['radice_nome'], $row['genere_nome'], $row['sottogenere_nome']
and corresponding *_id fields as appropriate) so orphaned books don't retain old
values; locate the logic that computes $chain/$chainLen in BookRepository.php
(the block using $chain, $chainLen, $row, $sottogenereId) and insert the
empty-chain handling before the existing chainLen === 1/2/>=3 branches.
- Around line 985-995: The while loop in getById() re-queries the generi table
for IDs already loaded in $row (genere_nome, genere_parent_id, radice_nome,
radice_id, nonno_nome, nonno_id), causing N+1 queries; instead build $chain from
those $row fields first (push root/parent/grandparent entries using those
columns and set $currentId from the last known parent), and only fall back to
the existing DB loop (using $currentId, $maxDepth, $stmt, $chain) if $currentId
remains >0 after consuming the three JOINed levels (i.e., for L4+); keep the
existing loop as the overflow path and ensure types/coalescing when reading
parent IDs from $row.
- Around line 986-994: The loop in BookRepository repeatedly calls
$this->db->prepare(...) creating $stmt on each iteration without closing it; to
fix, move the prepare call for the 'SELECT id, nome, parent_id FROM generi WHERE
id = ?' query outside the loop (create the prepared statement once), inside the
loop update the $currentId variable and call $stmt->bind_param('i', $currentId)
/ $stmt->execute() / $stmt->get_result()->fetch_assoc() as before, and after the
loop call $stmt->close() to release the server-side resource; keep all
references to $stmt and $currentId as in the existing method so the change is
minimal.
In `@app/Views/generi/dettaglio_genere.php`:
- Line 198: The inline JS confirm call in the form (the confirm('<?= __("Sei
sicuro di voler eliminare questo genere?") ?>') inside the form tag) can be
broken by single quotes in translations; replace the raw PHP translation with a
JS-safe encoded string (e.g., use json_encode or htmlspecialchars(...,
ENT_QUOTES) on __("Sei sicuro...") ) so the generated confirm argument is
properly quoted/escaped for JavaScript; update the form's onsubmit attribute in
dettaglio_genere.php to use the encoded translation from the __("…") call.
- Around line 230-239: The confirm prompt in the merge handler
(merge-genre-form) concatenates PHP translation strings with a user-controlled
option label (merge_target_id) using single quotes, creating the same
single-quote injection risk; change the implementation to build the prompt using
JS-safe, escaped strings: emit the translated fragments via JSON encoding (e.g.
use <?= json_encode(__("Sei sicuro di voler unire questo genere con")) ?> and
<?= json_encode(__("Questa azione è irreversibile.")) ?>) and then construct the
message with those safe literals plus a safely-encoded targetName (e.g. use
JSON.stringify(targetName) or replace/escape single quotes in targetName) before
passing to confirm(), ensuring no raw PHP strings are concatenated with JS
single quotes.
In `@app/Views/libri/index.php`:
- Around line 436-482: The URL-driven collana filter is treated as a static
initialCollana value so closing the banner only hides the message while the
actual filter stays active; make the collana filter mutable by introducing a
currentCollana variable (initialized from initialCollana) and update the UI and
filter state when filters are cleared or when the banner close button is
clicked: on closeBtn click, reset currentCollana to '' and clear the
corresponding collana input/select (e.g. document.getElementById('collana') or
the component that applies the collana filter) and trigger the same filter-reset
logic as the "Cancella tutti i filtri" handler; also ensure the global “clear
all filters” routine clears currentCollana and removes the `#url-filter-flash`
banner so the URL-driven filter no longer affects results.
In `@public/.htaccess`:
- Around line 1-19: The .htaccess uses php_value directives inside <IfModule
mod_php.c>, <IfModule mod_php7.c>, and <IfModule mod_php8.c> which only apply to
mod_php and will be ignored for PHP-FPM deployments; verify the actual SAPI and
if PHP-FPM is used remove these php_value lines from .htaccess and instead set
upload_max_filesize/post_max_size (and related limits) in the proper place:
global php.ini, the PHP-FPM pool config using php_admin_value[...] (e.g.,
www.conf), or a per-directory .user.ini for INI_PERDIR settings, ensuring the
values match the intended 512M/520M and execution/input timeouts.
In `@storage/plugins/digital-library/DigitalLibraryPlugin.php`:
- Around line 357-378: The code currently returns HTTP 400 for all upload
errors; change it so server-side problems return 500: after computing $errorCode
(in the same block around the match and return in DigitalLibraryPlugin.php where
$file->getError() is handled), set a default $status = 400 and then if
$errorCode is UPLOAD_ERR_NO_TMP_DIR or UPLOAD_ERR_CANT_WRITE set $status = 500;
finally use that $status in the return $this->json($response, ['success' =>
false, 'message' => $message], $status) so only server-misconfiguration errors
produce HTTP 500.
---
Nitpick comments:
In `@app/Controllers/GeneriApiController.php`:
- Around line 129-133: The current check only prevents direct self-parenting by
comparing $newParent !== $id but doesn't verify ancestor chains, allowing deeper
cycles (e.g., A→B→A); update the parent-assignment logic in GeneriApiController
(the block using $data['parent_id'], $newParent, $id and setting
$updateData['parent_id']) to walk the ancestor chain starting at $newParent
(query each genre's parent_id via your Genre model/repository) and abort or
ignore the assignment if any ancestor equals $id (also handle nulls and
non-existent parents), so the loop terminates when parent_id is null or exceeds
a reasonable depth.
In `@app/Models/GenereRepository.php`:
- Around line 176-252: The reported overcount comes from summing
$stmt->affected_rows for both the genere_id and sottogenere_id UPDATEs in
merge(), which double-counts books that referenced the source genre in both
columns; fix by computing the number of unique books that reference the source
before performing the UPDATEs using a single SELECT COUNT(DISTINCT id) FROM
libri WHERE deleted_at IS NULL AND (genere_id = ? OR sottogenere_id = ?) bound
with $sourceId, store that in $booksUpdated, then run the two UPDATE statements
without adding their affected_rows to $booksUpdated (leave childrenMoved logic
unchanged).
In `@package.json`:
- Line 37: The package.json currently pins `@playwright/test` correctly but the
"test" script still fails; update the scripts.test entry so running npm test
executes Playwright (e.g., replace the failing command with "playwright test" or
"npx playwright test") and optionally add a CI-friendly script like "test:ci"
that runs "npx playwright test --reporter=dot" or similar; change the scripts
object (scripts.test and/or add scripts.test:ci) so Playwright is invoked
directly using the installed `@playwright/test` package.
In `@storage/plugins/digital-library/DigitalLibraryPlugin.php`:
- Around line 268-271: The condition checking the audio URL is redundant because
!empty() already handles missing keys; change the guard from if
(!empty($book['audio_url'] ?? '')) to if (!empty($book['audio_url'])) before the
include of __DIR__ . '/views/frontend-player.php' (update inside the same method
in DigitalLibraryPlugin.php where $book is used) so the null-coalescing fallback
is removed.
- Around line 490-494: The json method currently uses json_encode with
JSON_HEX_TAG but not JSON_THROW_ON_ERROR, so encoding failures can return false
and pass a non-string to $response->getBody()->write; update the json method to
include JSON_THROW_ON_ERROR in the flags passed to json_encode (or use
json_encode via the JSON_THROW_ON_ERROR option) and wrap the encoding in a
try/catch for \JsonException inside the json function (method json) so you can
log/handle the error and return an appropriate error response (e.g., 500)
instead of writing a non-string to the response body; ensure the catch returns a
JSON error payload and status and still sets Content-Type via the same
$response->withHeader(...) chain.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
app/Controllers/GeneriApiController.phpapp/Controllers/GeneriController.phpapp/Controllers/LibriApiController.phpapp/Models/BookRepository.phpapp/Models/GenereRepository.phpapp/Routes/web.phpapp/Views/generi/dettaglio_genere.phpapp/Views/libri/index.phpapp/Views/libri/partials/book_form.phppackage.jsonpublic/.htaccessstorage/plugins/api-book-scraper/plugin.jsonstorage/plugins/dewey-editor/plugin.jsonstorage/plugins/digital-library/DigitalLibraryPlugin.phpstorage/plugins/digital-library/plugin.jsonstorage/plugins/open-library/plugin.jsonstorage/plugins/z39-server/plugin.json
Comment on lines
985
to
995
| while ($currentId > 0 && $maxDepth-- > 0) { | ||
| $stmt = $this->db->prepare('SELECT id, nome, parent_id FROM generi WHERE id = ?'); | ||
| $stmt->bind_param('i', $currentId); | ||
| $stmt->execute(); | ||
| $genre = $stmt->get_result()->fetch_assoc(); | ||
| if (!$genre) { | ||
| break; | ||
| } | ||
| array_unshift($chain, $genre); // prepend: root first | ||
| $currentId = (int)($genre['parent_id'] ?? 0); | ||
| } |
There was a problem hiding this comment.
N+1 queries: resolver re-fetches data already present in $row from the main JOIN.
For every getById() call the while loop issues up to 5 individual SELECT id, nome, parent_id FROM generi WHERE id = ? queries. The main query at lines 53-87 already JOINs the generi table three levels deep (g, gp, gpp) and puts the results directly into $row as genere_nome, genere_parent_id, radice_nome, radice_id, nonno_nome, nonno_id. The resolver discards all of that and redundantly re-fetches the same rows from the DB.
For the standard 1–3 level case (covers the vast majority of real data), the chain can be built from the data already in $row with zero extra queries. Only for genuinely deeper hierarchies (L4+, which the existing three-level JOIN misses) would a fallback query be needed.
⚡ Proposed approach: build chain from already-loaded JOIN data, query only on overflow
- // Walk up the tree from genere_id to collect the full ancestor chain
- $chain = [];
- $currentId = $genereId;
- $maxDepth = 5; // safety limit
-
- while ($currentId > 0 && $maxDepth-- > 0) {
- $stmt = $this->db->prepare('SELECT id, nome, parent_id FROM generi WHERE id = ?');
- $stmt->bind_param('i', $currentId);
- $stmt->execute();
- $genre = $stmt->get_result()->fetch_assoc();
- if (!$genre) {
- break;
- }
- array_unshift($chain, $genre); // prepend: root first
- $currentId = (int)($genre['parent_id'] ?? 0);
- }
+ // Build chain from JOIN data already loaded in $row (zero extra queries for ≤3 levels)
+ $chain = [];
+ // g → $row['genere_id_resolved'] / genere_nome / genere_parent_id
+ // gp → $row['radice_id'] / radice_nome
+ // gpp→ $row['nonno_id'] / nonno_nome
+ if (!empty($row['nonno_id'])) {
+ $chain[] = ['id' => (int)$row['nonno_id'], 'nome' => $row['nonno_nome'] ?? ''];
+ $chain[] = ['id' => (int)$row['radice_id'], 'nome' => $row['radice_nome'] ?? ''];
+ $chain[] = ['id' => (int)($row['genere_id_resolved'] ?? $genereId), 'nome' => $row['genere_nome'] ?? ''];
+ } elseif (!empty($row['radice_id'])) {
+ $chain[] = ['id' => (int)$row['radice_id'], 'nome' => $row['radice_nome'] ?? ''];
+ $chain[] = ['id' => (int)($row['genere_id_resolved'] ?? $genereId), 'nome' => $row['genere_nome'] ?? ''];
+ } else {
+ $chain[] = ['id' => $genereId, 'nome' => $row['genere_nome'] ?? ''];
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Models/BookRepository.php` around lines 985 - 995, The while loop in
getById() re-queries the generi table for IDs already loaded in $row
(genere_nome, genere_parent_id, radice_nome, radice_id, nonno_nome, nonno_id),
causing N+1 queries; instead build $chain from those $row fields first (push
root/parent/grandparent entries using those columns and set $currentId from the
last known parent), and only fall back to the existing DB loop (using
$currentId, $maxDepth, $stmt, $chain) if $currentId remains >0 after consuming
the three JOINed levels (i.e., for L4+); keep the existing loop as the overflow
path and ensure types/coalescing when reading parent IDs from $row.
- GeneriApiController: ancestor-chain cycle detection, sanitized error responses - BookRepository: prepare stmt outside loop + close, handle chainLen===0 - GenereRepository: merge stats use COUNT(DISTINCT id) to avoid double-counting - dettaglio_genere: json_encode confirm() strings to prevent single-quote breakage - DigitalLibraryPlugin: JSON_THROW_ON_ERROR, upload error 500 for server issues - libri/index: collana filter mutable and clearable - package.json: wire npm test to Playwright
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Views/libri/index.php (1)
631-636:⚠️ Potential issue | 🟡 MinorURL-driven
genereandsottogenerefilters cannot be cleared.
initialGenereandinitialSottogenereareconstvalues used as fallbacks in the ajaxdatafunction. After the user clicks "Clear all filters" or dismisses the banner,genere_idis emptied but the fallback|| initialGenerere-activates it.sottogenere_filterhas no dynamic input at all — it always sendsinitialSottogenere.The banner (Lines 454–459) displays these as active filters, implying they can be dismissed, but neither the close button nor "clear all" actually neutralizes them. Consider applying the same mutable-variable pattern used for
collanaFilter:Suggested approach
const initialGenere = parseInt(urlParams.get('genere') || urlParams.get('genere_filter') || '0', 10) || 0; const initialSottogenere = parseInt(urlParams.get('sottogenere') || urlParams.get('sottogenere_filter') || '0', 10) || 0; const initialKeywords = urlParams.get('keywords') || ''; const initialCollana = urlParams.get('collana') || ''; let collanaFilter = initialCollana; + let genereUrlFilter = initialGenere; + let sottogenereUrlFilter = initialSottogenere;Then in the ajax
datafunction:- genere_filter: document.getElementById('genere_id').value || initialGenere || 0, - sottogenere_filter: initialSottogenere || 0, + genere_filter: document.getElementById('genere_id').value || genereUrlFilter || 0, + sottogenere_filter: sottogenereUrlFilter || 0,And in both the banner close handler and clear-filters handler:
collanaFilter = ''; + genereUrlFilter = 0; + sottogenereUrlFilter = 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/libri/index.php` around lines 631 - 636, The URL-driven genere and sottogenere filters use const fallbacks (initialGenere, initialSottogenere) in the AJAX data payload, so clearing the DOM values reverts to those consts and they never clear; change initialGenere and initialSottogenere to mutable let variables (follow the collanaFilter pattern), update the AJAX data builder to use these mutable variables for genere_filter and sottogenere_filter (instead of DOM || initial* or always initial*), and in both the banner close handler and the clear-filters handler set these mutable variables to a neutral value (0 or null) and update the UI so subsequent AJAX requests send a cleared filter. Ensure references: initialGenere, initialSottogenere, genere_filter, sottogenere_filter, collanaFilter, the AJAX data function, banner close handler, and clear-filters handler are updated accordingly.
♻️ Duplicate comments (1)
app/Models/BookRepository.php (1)
985-995: N+1 queries: the resolver still re-fetches data already loaded by the main JOIN.The ancestor-walk loop issues up to 5 individual
SELECTqueries pergetById()call, even though the main query (Lines 53–87) already JOINsgenerithree levels deep and places the results into$row(genere_nome,genere_parent_id,radice_id,radice_nome,nonno_id,nonno_nome). For the common 1–3 level case, the chain can be built from existing$rowdata with zero extra queries, falling back to the DB loop only for L4+.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/BookRepository.php` around lines 985 - 995, The ancestor-walk loop in BookRepository is issuing up to 5 extra SELECTs; instead, build the genre chain from the already-joined $row fields (genere_nome, genere_parent_id, radice_id, radice_nome, nonno_id, nonno_nome, radice_id) for the common 1–3 level case and only fall back to the existing DB loop when the chain extends beyond those levels; update getById() so it first constructs $chain from $row values if present and only executes the while($currentId > 0 ...) prepare/execute loop when $currentId remains after consuming the joined ancestors.
🧹 Nitpick comments (2)
package.json (1)
7-7: Drop thenpxprefix and handle browser installation explicitly.Two separate issues:
- Redundant
npx– npm scripts automatically resolve binaries fromnode_modules/.bin, so usingnpxis unnecessary and can cause the local version pin to be bypassed if the global version is installed. Useplaywright test --config=tests/playwright.config.jsdirectly.Proposed fix
- "test": "npx playwright test --config=tests/playwright.config.js", + "test": "playwright test --config=tests/playwright.config.js",
- Missing browser installation – Playwright requires
npx playwright installto download browsers. No CI configuration exists in the repository, and thepostinstallscript does not include this step. Tests will fail in a clean environment with "browser not found" errors. Add browser installation topostinstall:"postinstall": "node scripts/copy-vendor-assets.js && npx playwright install --with-deps"Alternatively, if tests are only run in CI, document the browser install step there.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 7, Update the package.json scripts: remove the redundant "npx" from the "test" script so it invokes the local Playwright binary (change the "test" script to use "playwright test --config=tests/playwright.config.js"), and ensure Playwright browsers are installed in clean environments by adding a browser install step to the "postinstall" script (append running Playwright's browser installer with deps to the existing "postinstall" that runs scripts/copy-vendor-assets.js), or alternatively document/implement the browser install in CI; target the "test" and "postinstall" script entries in package.json when making the change.app/Controllers/GeneriApiController.php (1)
133-146: Prepared statement created inside the cycle-detection loop on every iteration.
$aStmtis re-prepared on each iteration (up to 10 times) and never explicitly closed. Prepare once before the loop and close after.Suggested fix
if ($newParent !== null) { $ancestorId = $newParent; $depth = 10; + $aStmt = $db->prepare('SELECT parent_id FROM generi WHERE id = ?'); while ($ancestorId > 0 && $depth-- > 0) { if ($ancestorId === $id) { + $aStmt->close(); $response->getBody()->write(json_encode(['error' => __('Impossibile: si creerebbe un ciclo.')])); return $response->withStatus(400)->withHeader('Content-Type', 'application/json'); } - $aStmt = $db->prepare('SELECT parent_id FROM generi WHERE id = ?'); $aStmt->bind_param('i', $ancestorId); $aStmt->execute(); $aRow = $aStmt->get_result()->fetch_assoc(); $ancestorId = $aRow ? (int)($aRow['parent_id'] ?? 0) : 0; } + $aStmt->close(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/GeneriApiController.php` around lines 133 - 146, The loop that checks for cycles prepares the statement $aStmt on every iteration; instead prepare the SELECT parent_id FROM generi WHERE id = ? once before entering the while that uses $ancestorId/$depth/$newParent, bind the parameter inside the loop (or re-bind the value), execute and fetch in each iteration, then close $aStmt after the loop completes to avoid repeated prepares and resource leaks; ensure any early returns still close $aStmt (or use finally-style cleanup) so the prepared statement is always closed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Models/BookRepository.php`:
- Around line 1016-1041: When chainLen === 1 the code sets genere_id_cascade to
0 but does not clear $row['genere_nome'], causing inconsistent data; update the
branch that handles chainLen === 1 in the logic around $row (used in
BookRepository methods) to explicitly set $row['genere_nome'] = null (or an
empty string depending on conventions) alongside setting 'genere_id_cascade' and
'sottogenere_id_cascade', so downstream consumers won't see a leftover L1 name
when no L2 genre is selected.
---
Outside diff comments:
In `@app/Views/libri/index.php`:
- Around line 631-636: The URL-driven genere and sottogenere filters use const
fallbacks (initialGenere, initialSottogenere) in the AJAX data payload, so
clearing the DOM values reverts to those consts and they never clear; change
initialGenere and initialSottogenere to mutable let variables (follow the
collanaFilter pattern), update the AJAX data builder to use these mutable
variables for genere_filter and sottogenere_filter (instead of DOM || initial*
or always initial*), and in both the banner close handler and the clear-filters
handler set these mutable variables to a neutral value (0 or null) and update
the UI so subsequent AJAX requests send a cleared filter. Ensure references:
initialGenere, initialSottogenere, genere_filter, sottogenere_filter,
collanaFilter, the AJAX data function, banner close handler, and clear-filters
handler are updated accordingly.
---
Duplicate comments:
In `@app/Models/BookRepository.php`:
- Around line 985-995: The ancestor-walk loop in BookRepository is issuing up to
5 extra SELECTs; instead, build the genre chain from the already-joined $row
fields (genere_nome, genere_parent_id, radice_id, radice_nome, nonno_id,
nonno_nome, radice_id) for the common 1–3 level case and only fall back to the
existing DB loop when the chain extends beyond those levels; update getById() so
it first constructs $chain from $row values if present and only executes the
while($currentId > 0 ...) prepare/execute loop when $currentId remains after
consuming the joined ancestors.
---
Nitpick comments:
In `@app/Controllers/GeneriApiController.php`:
- Around line 133-146: The loop that checks for cycles prepares the statement
$aStmt on every iteration; instead prepare the SELECT parent_id FROM generi
WHERE id = ? once before entering the while that uses
$ancestorId/$depth/$newParent, bind the parameter inside the loop (or re-bind
the value), execute and fetch in each iteration, then close $aStmt after the
loop completes to avoid repeated prepares and resource leaks; ensure any early
returns still close $aStmt (or use finally-style cleanup) so the prepared
statement is always closed.
In `@package.json`:
- Line 7: Update the package.json scripts: remove the redundant "npx" from the
"test" script so it invokes the local Playwright binary (change the "test"
script to use "playwright test --config=tests/playwright.config.js"), and ensure
Playwright browsers are installed in clean environments by adding a browser
install step to the "postinstall" script (append running Playwright's browser
installer with deps to the existing "postinstall" that runs
scripts/copy-vendor-assets.js), or alternatively document/implement the browser
install in CI; target the "test" and "postinstall" script entries in
package.json when making the change.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/Controllers/GeneriApiController.phpapp/Models/BookRepository.phpapp/Models/GenereRepository.phpapp/Views/generi/dettaglio_genere.phpapp/Views/libri/index.phppackage.jsonstorage/plugins/digital-library/DigitalLibraryPlugin.php
🚧 Files skipped from review as they are similar to previous changes (2)
- app/Models/GenereRepository.php
- storage/plugins/digital-library/DigitalLibraryPlugin.php
…consistency - libri/index: genere/sottogenere URL filters now mutable and clearable - GeneriApiController: prepare cycle-detection stmt once outside loop + close - BookRepository: clear genere_nome/sottogenere_nome when chainLen===1 - package.json: drop redundant npx prefix
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
public/.htaccess (1)
1-19: Duplicate of existing review comment:php_value/ PHP-FPM incompatibility.The concern about
php_valuedirectives being silently ignored under PHP-FPM (even inside<IfModule mod_php*.c>guards) has already been raised and documented in the prior review comment on this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/.htaccess` around lines 1 - 19, The .htaccess contains php_value directives inside <IfModule mod_php.c>, <IfModule mod_php7.c>, and <IfModule mod_php8.c> blocks which will be silently ignored under PHP-FPM; replace these php_value settings (upload_max_filesize, post_max_size, max_execution_time, max_input_time) with a supported configuration approach: either move them to the global php.ini, the FPM pool config (www.conf) using php_admin_value/php_admin_flag, or into a per-directory .user.ini if using FPM with user INI settings enabled; remove the mod_php blocks or add a clear comment pointing to the new location so maintainers know where the limits are managed.app/Views/generi/dettaglio_genere.php (1)
230-241: Past JS injection concern in merge confirmation is now addressed.The merge confirmation prompt now uses
json_encodefor PHP translation strings (Lines 235-236), which correctly handles single quotes and special characters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/generi/dettaglio_genere.php` around lines 230 - 241, The JS injection risk in the merge confirmation is fixed by using json_encode for the PHP translation strings; ensure the code in the mergeForm submit handler (element id 'merge-genre-form', variable mergeForm) continues to retrieve the target select ('merge_target_id') and its textContent.trim() into targetName and builds the confirm message using msgPrefix and msgSuffix (both created via json_encode) before calling confirm; keep using textContent (not innerHTML) and json_encode for translations to prevent injection and maintain the current confirm string concatenation logic.
🧹 Nitpick comments (7)
package.json (2)
37-37:@playwright/test ^1.58.2is the current latest — version is valid.Version 1.58.2 is the latest published release of
@playwright/test. Consider adding anenginesfield topackage.jsonto enforce the Node.js version constraint — Playwright 1.58 requires Node.js 20.x, 22.x, or 24.x.⚙️ Proposed addition
+ "engines": { + "node": ">=20" + }, "type": "commonjs",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 37, Add an "engines" field to package.json to enforce the Node.js versions required by the declared `@playwright/test` dependency (currently "^1.58.2"); specifically set "node": ">=20.x <=20.x || >=22.x <=22.x || >=24.x <=24.x" (or the shorthand "20.x || 22.x || 24.x") so CI and contributors use a compatible Node version when running Playwright; update package.json's top-level object to include this engines entry and ensure any publishing/build scripts respect the constraint.
7-7: Ensure browser binaries are installed beforenpm testruns.Playwright updates bundled browser versions with every release, and
npm installalone does not download them — a separatenpx playwright installstep is required. Without it,npm testwill fail in any clean environment (fresh clone, CI). Consider adding it to thepostinstallscript or ensuring it is an explicit step in your CI pipeline before the test stage.⚙️ Option: add a dedicated `install:browsers` script
"scripts": { "test": "playwright test --config=tests/playwright.config.js", + "install:browsers": "playwright install --with-deps", "build": "cd frontend && npm run build",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 7, The test script can fail in clean environments because Playwright browser binaries aren't downloaded by npm install; add an explicit step to install them—either add a new npm script like "install:browsers" that runs "npx playwright install" and call that in CI before running "npm test", or add a "postinstall" script that runs "npx playwright install" so the browsers are fetched automatically after installs; update package.json scripts accordingly (referencing the "test" script) and adjust CI config to invoke the new script if you choose the explicit CI step.app/Controllers/GeneriApiController.php (1)
155-159: Inconsistent use ofJSON_UNESCAPED_UNICODEacross responses.The success response (line 159) correctly uses
JSON_UNESCAPED_UNICODE, but error responses (lines 116, 123, 140, 163, 167) use plainjson_encode. Since error messages go through__()and may contain non-ASCII characters (Italian text), the unicode escaping will be inconsistent across success/error paths.Proposed fix: add JSON_UNESCAPED_UNICODE to error responses
- $response->getBody()->write(json_encode(['error' => __('Il nome del genere è obbligatorio.')])); + $response->getBody()->write(json_encode(['error' => __('Il nome del genere è obbligatorio.')], JSON_UNESCAPED_UNICODE));Apply similarly to all
json_encodecalls in this method that may contain localized strings.Also applies to: 116-117, 123-124, 140-141, 163-164, 167-167
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/GeneriApiController.php` around lines 155 - 159, The method in GeneriApiController uses JSON_UNESCAPED_UNICODE on the success json_encode but not on several error responses; update every response->getBody()->write(json_encode(...)) in this method (including the error paths that build messages via __('...') around the earlier calls at lines referenced in the review) to pass the JSON_UNESCAPED_UNICODE flag so all outputs use consistent unicode encoding; locate these calls in GeneriApiController (the same method that writes success with 'id','nome','updated') and add the JSON_UNESCAPED_UNICODE option to each json_encode invocation that may contain localized strings.app/Models/GenereRepository.php (2)
111-136: Delete checks are not atomic — minor TOCTOU window.The children/book checks (Lines 114-131) and the actual
DELETE(Line 133) are not wrapped in a transaction, so a concurrent insert could bypass the guards. This is low risk for a single-admin app but worth noting for future hardening.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/GenereRepository.php` around lines 111 - 136, The delete method in GenereRepository has a TOCTOU race between the checks and the DELETE; wrap the entire sequence in a DB transaction to make it atomic: in delete(int $id) call $this->db->begin_transaction(), then run the child and book COUNT checks and the DELETE inside the transaction (optionally use SELECT ... FOR UPDATE or appropriate row/table locking to prevent concurrent inserts), on failure throw after rolling back ($this->db->rollback()), on success execute the DELETE and $this->db->commit(); ensure errors/exceptions also rollback to avoid leaving an open transaction.
195-201: Transaction detection via@@autocommitis fragile.If
autocommitis globally disabled inmy.cnf,@@autocommitreturns0even when no explicit transaction is active, causing the merge to run without its own transaction boundary. A more robust approach is to simply always callbegin_transaction()and handle the "already in transaction" case via a try/catch on the implicit commit, or useSAVEPOINT.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/GenereRepository.php` around lines 195 - 201, The current detection using @@autocommit is fragile; remove the $acResult / $wasInTransaction check and instead always protect the merge with a transactional savepoint: create a unique savepoint name (e.g. using uniqid), call $this->db->query("SAVEPOINT $name") before the merge logic, and on success call $this->db->query("RELEASE SAVEPOINT $name"); on error call $this->db->query("ROLLBACK TO SAVEPOINT $name") and rethrow. This avoids relying on @@autocommit and works whether already in a transaction or not, leaving outer transactions intact; reference the existing $this->db->query(...) and remove the $this->db->begin_transaction() / $wasInTransaction branching.app/Controllers/GeneriController.php (1)
87-96: Bare 404 response has no body.When the genre is not found,
withStatus(404)returns an empty response. Theshow()method on Line 169 does the same, so this is consistent within the controller — but users will see a blank page. Consider redirecting to the genre list with an error flash, matching the pattern used elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/GeneriController.php` around lines 87 - 96, The update() (and similarly show()) returns a bare 404 when GenereRepository->getById($id) is falsy; instead of returning an empty 404 response, set an error flash message and redirect the user to the genre list. Specifically, in the update() method replace the return $response->withStatus(404) branch with code that adds an error flash (use the same flash API used elsewhere in the controller) and returns a redirect response to the genre list route/path (use your router/url helper or set a Location header with a 302 status). Ensure the same change is applied to show() so both methods follow the project’s redirect-with-flash pattern; keep CSRF handling (CsrfMiddleware) as-is.app/Models/BookRepository.php (1)
1032-1041:chainLen >= 3: sottogenere from the chain ignores$row['sottogenere_id'].When
genere_idpoints to L3+, the code maps the deepest chain node as the sottogenere, overriding whateversottogenere_idthe book row carries. This is likely intentional for the cascade display, but if a book has bothgenere_idat L3 and a separatesottogenere_id, the explicit sottogenere is silently dropped. Worth a brief comment in the code if this is by design.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/BookRepository.php` around lines 1032 - 1041, The block in BookRepository.php that handles the else branch for L3+ genres ($row, $chain, $chainLen) currently overwrites any explicit $row['sottogenere_id'] by taking the deepest chain node as the sottogenere; add a concise inline comment immediately above this block explaining that when $row['genere_id'] points to L3+ the code intentionally derives sottogenere from $chain[$chainLen - 1] and therefore ignores $row['sottogenere_id'] (or, if preferred, update the logic to prefer an explicit $row['sottogenere_id'] when present—choose one behavior and document it), referencing the variables $row, $chain and $chainLen so future readers understand the decision.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Controllers/GeneriController.php`:
- Around line 98-117: GeneriController::update currently only blocks
self-parenting but not assigning a descendant as parent, which can create
cycles; implement the same ancestor-walking validation used in
GeneriApiController::update to prevent this: when $data['parent_id'] is set and
non-empty, walk up from the proposed $newParent using the repository (e.g.,
$repo->find($parentId) / parent_id) collecting/visiting ancestors until null,
and if you encounter the current $id treat it as a validation error (set
$_SESSION['error_message'] and skip the repo->update / redirect back) so the
update is rejected; follow the same loop/termination safety as
GeneriApiController::update and ensure you still allow clearing parent_id (null)
and preserve existing behavior for valid moves (call $repo->update($id,
$updateData) and set success message).
In `@app/Models/GenereRepository.php`:
- Around line 218-222: In GenereRepository.php inside the merge logic (the block
using $stmt, $targetId and $sourceId), avoid creating a self-referencing row by
updating the SQL to skip the target row when moving children: change the UPDATE
query from "UPDATE generi SET parent_id = ? WHERE parent_id = ?" to include "AND
id != ?" (or equivalent) and adjust bind_param to pass $targetId, $sourceId,
$targetId so the target row is not re-parented to itself; keep using
$stmt->execute() and $stmt->affected_rows as before.
In `@app/Views/generi/dettaglio_genere.php`:
- Around line 153-178: The current grouping loop skips the current genre
($genereId) which prevents any of its children collected into
$grouped[$genereId]['_children'] from being rendered because later code only
renders groups with a '_self' entry; update the grouping/rendering so children
of the current genre are not silently dropped: in the foreach over $allGeneri
(using $allGeneri, $genereId, $grouped) either stop skipping items whose
parent_id === $genereId or ensure you initialize $grouped[(int)$g['parent_id']]
even when the parent is skipped, then change the rendering condition (the
isset($group['_self']) check and the loop that builds <optgroup>/<option> using
$parentG and $group['_children']) to also render groups that have only
'_children' (render children as flat options or render an optgroup with a
placeholder label), ensuring the merge()-compatible options are available.
In `@app/Views/libri/index.php`:
- Around line 436-491: The genre URL filters (genereUrlFilter,
sottogenereUrlFilter) remain active when the user clears or edits the visible
genre inputs; add logic to clear those URL-driven filter vars when the user
clears the corresponding inputs. Locate the genre/sottogenere input elements and
add event listeners (e.g., 'input' or 'change') that check if the input value is
empty and, if so, set genereUrlFilter = '' (or 0 to match existing usage for
no-filter), sottogenereUrlFilter = '' (or 0), then call table.ajax.reload() to
refresh results; ensure the same behavior is implemented where chips are removed
so the variables are reset consistently (same pattern used in the existing
closeBtn click handler).
In `@storage/plugins/digital-library/DigitalLibraryPlugin.php`:
- Around line 497-504: The fallback json_encode in the catch can still return
false and cause a TypeError when passed to $response->getBody()->write; after
the existing catch assignment to $json, ensure $json is a string (e.g. if ($json
=== false) set $json to a literal JSON string like
'{"success":false,"message":"Internal error"}' or re-encode with error handling)
and optionally log the secondary failure; update the block around
json_encode(...) and $response->getBody()->write(...) in DigitalLibraryPlugin to
guarantee $json is always a string before calling write() (refer to the
json_encode call, the catch that logs via \App\Support\SecureLogger::error, and
the $response->getBody()->write/$status usage).
---
Duplicate comments:
In `@app/Views/generi/dettaglio_genere.php`:
- Around line 230-241: The JS injection risk in the merge confirmation is fixed
by using json_encode for the PHP translation strings; ensure the code in the
mergeForm submit handler (element id 'merge-genre-form', variable mergeForm)
continues to retrieve the target select ('merge_target_id') and its
textContent.trim() into targetName and builds the confirm message using
msgPrefix and msgSuffix (both created via json_encode) before calling confirm;
keep using textContent (not innerHTML) and json_encode for translations to
prevent injection and maintain the current confirm string concatenation logic.
In `@public/.htaccess`:
- Around line 1-19: The .htaccess contains php_value directives inside <IfModule
mod_php.c>, <IfModule mod_php7.c>, and <IfModule mod_php8.c> blocks which will
be silently ignored under PHP-FPM; replace these php_value settings
(upload_max_filesize, post_max_size, max_execution_time, max_input_time) with a
supported configuration approach: either move them to the global php.ini, the
FPM pool config (www.conf) using php_admin_value/php_admin_flag, or into a
per-directory .user.ini if using FPM with user INI settings enabled; remove the
mod_php blocks or add a clear comment pointing to the new location so
maintainers know where the limits are managed.
---
Nitpick comments:
In `@app/Controllers/GeneriApiController.php`:
- Around line 155-159: The method in GeneriApiController uses
JSON_UNESCAPED_UNICODE on the success json_encode but not on several error
responses; update every response->getBody()->write(json_encode(...)) in this
method (including the error paths that build messages via __('...') around the
earlier calls at lines referenced in the review) to pass the
JSON_UNESCAPED_UNICODE flag so all outputs use consistent unicode encoding;
locate these calls in GeneriApiController (the same method that writes success
with 'id','nome','updated') and add the JSON_UNESCAPED_UNICODE option to each
json_encode invocation that may contain localized strings.
In `@app/Controllers/GeneriController.php`:
- Around line 87-96: The update() (and similarly show()) returns a bare 404 when
GenereRepository->getById($id) is falsy; instead of returning an empty 404
response, set an error flash message and redirect the user to the genre list.
Specifically, in the update() method replace the return
$response->withStatus(404) branch with code that adds an error flash (use the
same flash API used elsewhere in the controller) and returns a redirect response
to the genre list route/path (use your router/url helper or set a Location
header with a 302 status). Ensure the same change is applied to show() so both
methods follow the project’s redirect-with-flash pattern; keep CSRF handling
(CsrfMiddleware) as-is.
In `@app/Models/BookRepository.php`:
- Around line 1032-1041: The block in BookRepository.php that handles the else
branch for L3+ genres ($row, $chain, $chainLen) currently overwrites any
explicit $row['sottogenere_id'] by taking the deepest chain node as the
sottogenere; add a concise inline comment immediately above this block
explaining that when $row['genere_id'] points to L3+ the code intentionally
derives sottogenere from $chain[$chainLen - 1] and therefore ignores
$row['sottogenere_id'] (or, if preferred, update the logic to prefer an explicit
$row['sottogenere_id'] when present—choose one behavior and document it),
referencing the variables $row, $chain and $chainLen so future readers
understand the decision.
In `@app/Models/GenereRepository.php`:
- Around line 111-136: The delete method in GenereRepository has a TOCTOU race
between the checks and the DELETE; wrap the entire sequence in a DB transaction
to make it atomic: in delete(int $id) call $this->db->begin_transaction(), then
run the child and book COUNT checks and the DELETE inside the transaction
(optionally use SELECT ... FOR UPDATE or appropriate row/table locking to
prevent concurrent inserts), on failure throw after rolling back
($this->db->rollback()), on success execute the DELETE and $this->db->commit();
ensure errors/exceptions also rollback to avoid leaving an open transaction.
- Around line 195-201: The current detection using @@autocommit is fragile;
remove the $acResult / $wasInTransaction check and instead always protect the
merge with a transactional savepoint: create a unique savepoint name (e.g. using
uniqid), call $this->db->query("SAVEPOINT $name") before the merge logic, and on
success call $this->db->query("RELEASE SAVEPOINT $name"); on error call
$this->db->query("ROLLBACK TO SAVEPOINT $name") and rethrow. This avoids relying
on @@autocommit and works whether already in a transaction or not, leaving outer
transactions intact; reference the existing $this->db->query(...) and remove the
$this->db->begin_transaction() / $wasInTransaction branching.
In `@package.json`:
- Line 37: Add an "engines" field to package.json to enforce the Node.js
versions required by the declared `@playwright/test` dependency (currently
"^1.58.2"); specifically set "node": ">=20.x <=20.x || >=22.x <=22.x || >=24.x
<=24.x" (or the shorthand "20.x || 22.x || 24.x") so CI and contributors use a
compatible Node version when running Playwright; update package.json's top-level
object to include this engines entry and ensure any publishing/build scripts
respect the constraint.
- Line 7: The test script can fail in clean environments because Playwright
browser binaries aren't downloaded by npm install; add an explicit step to
install them—either add a new npm script like "install:browsers" that runs "npx
playwright install" and call that in CI before running "npm test", or add a
"postinstall" script that runs "npx playwright install" so the browsers are
fetched automatically after installs; update package.json scripts accordingly
(referencing the "test" script) and adjust CI config to invoke the new script if
you choose the explicit CI step.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
app/Controllers/GeneriApiController.phpapp/Controllers/GeneriController.phpapp/Controllers/LibriApiController.phpapp/Models/BookRepository.phpapp/Models/GenereRepository.phpapp/Routes/web.phpapp/Views/generi/dettaglio_genere.phpapp/Views/libri/index.phpapp/Views/libri/partials/book_form.phppackage.jsonpublic/.htaccessstorage/plugins/api-book-scraper/plugin.jsonstorage/plugins/dewey-editor/plugin.jsonstorage/plugins/digital-library/DigitalLibraryPlugin.phpstorage/plugins/digital-library/plugin.jsonstorage/plugins/open-library/plugin.jsonstorage/plugins/z39-server/plugin.json
- GeneriController::update: add ancestor-walk cycle detection - GenereRepository::merge: AND id != ? prevents self-referencing - dettaglio_genere: children of current genre shown in merge dropdown - GeneriApiController: JSON_UNESCAPED_UNICODE on all error responses - libri/index: clear URL genre filters on input clear and chip remove - DigitalLibraryPlugin: literal string fallback for json_encode failure
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
> [!CAUTION]
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)storage/plugins/digital-library/DigitalLibraryPlugin.php (3)
174-204:⚠️ Potential issue | 🟡 MinorSELECT prepared statement is silently leaked inside the
registerHooksloop.When
num_rows === 0,$stmt(the SELECT statement) is overwritten at line 186 with the INSERT statement beforeclose()is called.$stmt->close()at line 203 only ever closes the last assigned statement — the SELECT handle leaks for every hook that requires insertion. With 6 hook entries this can leave up to 6 unclosed server-side cursors on the connection.🐛 Proposed fix — close SELECT before reassigning
if ($result->num_rows === 0) { + $stmt->close(); // close SELECT stmt before reassignment // Insert new hook $stmt = $this->db->prepare(" INSERT INTO plugin_hooks🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/digital-library/DigitalLibraryPlugin.php` around lines 174 - 204, In registerHooks (DigitalLibraryPlugin::registerHooks) the SELECT prepared statement assigned to $stmt is overwritten by the INSERT prepared statement before being closed, leaking the SELECT handle; fix by explicitly calling $stmt->close() immediately after finishing and before reassigning $stmt for the INSERT (i.e., close the SELECT statement after $result processing and before preparing the INSERT), and ensure the final $stmt->close() still runs for the INSERT as well.
95-105:⚠️ Potential issue | 🟡 Minor
onInstallwill fatal-error if eitherSHOW COLUMNSquery fails.
mysqli::query()returnsfalseon failure. Calling$result->num_rowsonfalsethrows aTypeError(Attempt to read property "num_rows" on bool), crashing plugin installation silently with a 500. Same issue on both thefile_url(line 96) andaudio_url(line 102) checks.🐛 Proposed fix
$result = $this->db->query("SHOW COLUMNS FROM libri LIKE 'file_url'"); - if ($result->num_rows === 0) { + if ($result instanceof \mysqli_result && $result->num_rows === 0) { // Add file_url column if missing $this->db->query("ALTER TABLE libri ADD COLUMN file_url VARCHAR(500) DEFAULT NULL COMMENT 'eBook file URL' AFTER note_varie"); } $result = $this->db->query("SHOW COLUMNS FROM libri LIKE 'audio_url'"); - if ($result->num_rows === 0) { + if ($result instanceof \mysqli_result && $result->num_rows === 0) { // Add audio_url column if missing $this->db->query("ALTER TABLE libri ADD COLUMN audio_url VARCHAR(500) DEFAULT NULL COMMENT 'Audiobook file URL' AFTER file_url"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/digital-library/DigitalLibraryPlugin.php` around lines 95 - 105, The onInstall method in DigitalLibraryPlugin calls $this->db->query("SHOW COLUMNS ...") and immediately reads $result->num_rows which will throw if query() returns false; update the file_url and audio_url checks to validate the query result (e.g., if $result === false) before accessing num_rows, handle failures by logging or throwing with $this->db->error, and only proceed to ALTER TABLE when the SHOW COLUMNS query succeeded and num_rows === 0; ensure both occurrences (the two SHOW COLUMNS checks in onInstall) follow this pattern so the installation fails gracefully instead of causing a TypeError.
392-407:⚠️ Potential issue | 🟠 MajorFile-upload RCE vector:
application/octet-streamin the ebook allowlist bypasses the extension check.
application/octet-streamis in$allowedMimefor the ebook branch (line 394), soin_array()returnstrueand the entire extension-validation block (lines 399–406) is never reached.generateSafeFilename(line 483) then preserves the original client extension verbatim — including.php. An authenticated admin/staff user can therefore:
- Upload
webshell.phpwithContent-Type: application/octet-streamanddigital_type=ebook.- Receive the public URL (
/uploads/digital/TIMESTAMP_HEX.php) directly in the JSON response.- Execute it, because the
.htaccesscreated inonActivate(line 62) only addsOptions -Indexes— it does not deny PHP execution.Two independent fixes are both needed:
Fix A — always validate the extension, regardless of MIME type (or remove
application/octet-streamfrom the allowlist entirely, since valid PDF/ePub files have unambiguous MIME types):🔒 Fix A: enforce extension check unconditionally
- if (!in_array($clientMediaType, $allowedMime, true)) { - // Allow by extension as fallback - $filename = strtolower($file->getClientFilename()); - $validExt = ($type === 'audio') - ? ['mp3', 'm4a', 'ogg', 'wav'] - : ['pdf', 'epub']; - $ext = pathinfo($filename, PATHINFO_EXTENSION); - if (!in_array($ext, $validExt, true)) { - return $this->json($response, ['success' => false, 'message' => __('Formato file non supportato.')], 400); - } - } + // Always validate extension regardless of reported MIME type + $filename = strtolower($file->getClientFilename()); + $validExt = ($type === 'audio') + ? ['mp3', 'm4a', 'ogg', 'wav'] + : ['pdf', 'epub']; + $ext = pathinfo($filename, PATHINFO_EXTENSION); + if (!in_array($ext, $validExt, true) && !in_array($clientMediaType, $allowedMime, true)) { + return $this->json($response, ['success' => false, 'message' => __('Formato file non supportato.')], 400); + } + if (!in_array($ext, $validExt, true)) { + return $this->json($response, ['success' => false, 'message' => __('Formato file non supportato.')], 400); + }Simpler alternative — just remove
application/octet-streamfrom the ebook allowlist and rely solely on the extension fallback for edge-case browsers:$allowedMime = ($type === 'audio') ? ['audio/mpeg', 'audio/mp4', 'audio/ogg', 'audio/x-m4a', 'audio/wav'] - : ['application/pdf', 'application/epub+zip', 'application/octet-stream']; + : ['application/pdf', 'application/epub+zip'];Fix B — deny PHP execution in the uploads
.htaccess(inonActivate, line 62):🔒 Fix B: block PHP execution in the uploads directory
- file_put_contents($htaccess, "# Protect directory listing\nOptions -Indexes\n"); + file_put_contents($htaccess, implode("\n", [ + '# Protect directory listing', + 'Options -Indexes', + '# Deny execution of any server-side scripts', + '<FilesMatch "\.(php\d?|phtml|phar|pl|py|cgi|sh)$">', + ' Order Deny,Allow', + ' Deny from all', + '</FilesMatch>', + '', + ]));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/digital-library/DigitalLibraryPlugin.php` around lines 392 - 407, The ebook branch currently allows application/octet-stream in $allowedMime which short-circuits the extension fallback and lets generateSafeFilename preserve dangerous extensions; update the upload validation in the method that checks $clientMediaType (the block using $allowedMime, $validExt and pathinfo) so that extension validation runs unconditionally for ebooks (either remove 'application/octet-stream' from $allowedMime or always perform the extension check regardless of MIME), and additionally harden the plugin activation routine (onActivate in DigitalLibraryPlugin) to write an uploads .htaccess that explicitly disables PHP execution (e.g., via RemoveHandler / SetHandler or deny exec of .php) so uploaded .php cannot execute.
♻️ Duplicate comments (4)
app/Views/generi/dettaglio_genere.php (1)
238-249: Merge confirmation prompt properly usesjson_encodefor translation strings.This addresses the prior review concern about single-quote injection in
confirm()calls.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/generi/dettaglio_genere.php` around lines 238 - 249, The confirm dialog currently concatenates translated strings into JavaScript and must use safe JSON-encoded strings to avoid injection; update the merge form script so the msgPrefix and msgSuffix values are produced by PHP's json_encode (as shown) instead of raw echoed strings, targeting the merge-genre-form handler (variable mergeForm) and the merge target select (merge_target_id) and ensure msgPrefix/msgSuffix are JSON-encoded translations before concatenation into the confirm() call and that e.preventDefault() remains when the user cancels.app/Models/GenereRepository.php (1)
218-243:⚠️ Potential issue | 🟠 MajorMerging a parent into one of its children leaves the target with a dangling
parent_id.The
AND id != ?on line 219 correctly prevents the target from self-referencing. However, it also means the target'sparent_id(which still points to the source) is never updated. After the source is deleted on line 241–243,target.parent_idreferences a non-existent row, corrupting the hierarchy.Scenario: Genre "Fiction" (id=1, parent=null) has child "Sci-Fi" (id=2, parent=1). Admin merges Fiction → Sci-Fi. After merge, Sci-Fi still has
parent_id=1, but genre 1 no longer exists.Proposed fix — reparent target to source's parent before deletion
$stmt = $this->db->prepare("UPDATE generi SET parent_id = ? WHERE parent_id = ? AND id != ?"); $stmt->bind_param('iii', $targetId, $sourceId, $targetId); $stmt->execute(); $childrenMoved = $stmt->affected_rows; + // If target was a child of source, reparent target to source's parent + $stmt = $this->db->prepare("UPDATE generi SET parent_id = ? WHERE id = ? AND parent_id = ?"); + $sourceParent = $source['parent_id']; + $stmt->bind_param('iii', $sourceParent, $targetId, $sourceId); + $stmt->execute(); + // Count distinct books referencing source (before updates, to avoid double-counting)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/GenereRepository.php` around lines 218 - 243, The merge leaves the target row pointing at the soon-to-be-deleted source, so before deleting the source (the DELETE FROM generi using $sourceId) fetch the source's parent_id and run an UPDATE generi SET parent_id = ? WHERE id = ? to set the target's parent_id to the source's parent_id (use $sourceParent, $targetId), using the same $this->db->prepare()/bind_param()/execute() pattern; do this after moving children and updating libri but before the DELETE, and wrap the sequence in the existing transaction so the hierarchy update and source deletion are atomic.storage/plugins/digital-library/DigitalLibraryPlugin.php (2)
497-504: json_encode fallback now correctly guards againstfalse— addresses previously flagged concern.The
?: '{"success":false,"message":"Internal error"}'literal at line 501 ensures$response->getBody()->write()always receives astring, resolving theTypeErrorrisk noted in the prior review.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/digital-library/DigitalLibraryPlugin.php` around lines 497 - 504, The JSON encoding fallback must guarantee a string to avoid TypeError when writing to the response body; update the catch in DigitalLibraryPlugin (the block that sets $json and writes to $response->getBody()) so that on JsonException you log the error (using \App\Support\SecureLogger::error) and set $json to a non-false string (e.g. a literal like '{"success":false,"message":"Internal error"}') instead of relying on json_encode(...) which can return false; ensure $response->getBody()->write($json) always receives that string and then return $response->withHeader('Content-Type', 'application/json')->withStatus($status).
357-383: LGTM —UPLOAD_ERR_NO_TMP_DIR/UPLOAD_ERR_CANT_WRITEnow correctly return HTTP 500.The new
$statusmatch (lines 378–381) addresses the previously flagged issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/digital-library/DigitalLibraryPlugin.php` around lines 357 - 383, The upload error handler in DigitalLibraryPlugin was missing proper HTTP status mapping; update the $status match in the upload handling code (the block that checks $file->getError() and builds $message) so that UPLOAD_ERR_NO_TMP_DIR and UPLOAD_ERR_CANT_WRITE return 500 and all other errors return 400 (this is the $status match you added), keep the existing SecureLogger warning and message construction unchanged so the client gets the localized message and the server returns the correct HTTP status.
🧹 Nitpick comments (3)
app/Controllers/GeneriController.php (1)
130-131: Exception message may contain internal details or mixed-language text.
$e->getMessage()is appended to a localized prefix. The repository throws messages in hardcoded Italian (e.g.,'Nome genere richiesto'), while the controller prefix uses__(). If the app is running in a non-Italian locale, this produces mixed-language error messages.Consider wrapping repo exception messages in
__()at the repository level, or catching and re-mapping messages here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/GeneriController.php` around lines 130 - 131, The catch in GeneriController (the block that sets $_SESSION['error_message'] = __('Errore nell\'aggiornamento del genere: ') . $e->getMessage()) must avoid appending raw exception text directly to a localized prefix; instead either translate repository error strings at source or map them here: replace concatenation with a localized, user-facing message (e.g. __('Errore nell\'aggiornamento del genere')) and separately log the raw $e->getMessage() for diagnostics, or run known repo messages through __() (or a mapping array) before appending so languages don't mix; update the catch to use the controller-level mapping/translation logic and move the raw exception into a debug log rather than the session message.app/Controllers/GeneriApiController.php (1)
106-170: Newupdate()method is well-structured with proper validation and cycle detection.The error handling correctly separates
InvalidArgumentException(400) from unexpected errors (500 with generic message + logging), addressing the prior review concern.One minor note: when
$newParent === $id(line 131), the self-reference is silently skipped rather than returning a 400 error. Thenomeupdate still proceeds. Consider returning an explicit error so the caller knows the parent wasn't changed.Optional: return explicit error on self-reference
if ($newParent !== $id) { + } else { + $response->getBody()->write(json_encode(['error' => __('Un genere non può essere genitore di sé stesso.')], JSON_UNESCAPED_UNICODE)); + return $response->withStatus(400)->withHeader('Content-Type', 'application/json'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/GeneriApiController.php` around lines 106 - 170, The update() method currently ignores a self-reference when $newParent === $id (so parent isn't changed silently); modify the logic in GeneriApiController::update to detect $newParent === $id and immediately return a 400 JSON response (Content-Type application/json) with a clear error message (e.g. "Impossibile: il genitore non può essere se stesso.") instead of continuing to perform the nome update; place this check where newParent is computed/compared to $id so it short-circuits before ancestor cycle detection and before calling GenereRepository::update.app/Models/GenereRepository.php (1)
195-201:@@autocommitdetection is fragile and unreliable.Checking
@@autocommitdoesn't reliably detect whether a transaction is active—START TRANSACTIONsets autocommit to 0 for the duration, but so doesSET autocommit=0without an explicitBEGIN. This approach works while callers are controlled, but becomes risky if calling patterns change.
mysqlidoes not provide a built-in transaction state API. For more reliable transaction detection, consider: using PDO withPDO::inTransaction()if switching libraries is feasible, or tracking transaction state manually in your code when callingbegin_transaction()/commit()/rollback().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/GenereRepository.php` around lines 195 - 201, Replace the fragile @@autocommit check with an explicit, local transaction-tracking mechanism: add and use a boolean property (e.g., $inTransaction) on the GenereRepository class that is set to true when begin_transaction() is called and cleared on commit() and rollback(), and use that flag instead of querying @@autocommit to decide whether to call begin_transaction(); update any internal methods that call begin_transaction(), commit(), or rollback() to maintain this flag consistently (ensure the current $wasInTransaction logic uses $this->inTransaction and not the @@autocommit query).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Views/generi/dettaglio_genere.php`:
- Around line 153-186: The merge dropdown currently groups genres by parent but
only renders groups where the parent has parent_id = NULL, which omits
grandchildren; fix by building a full tree (map genres by id from $allGeneri,
attach children into a children array for each node, skipping the node with id
$genereId) and then render the tree recursively (or iteratively flatten)
starting from root nodes so all depths are included; replace the current
$grouped / $orphanChildren logic with the map-and-attach approach and a
recursive renderer that outputs optgroup/option entries for each node and its
descendants.
In `@app/Views/libri/index.php`:
- Around line 635-640: The autocomplete onSelect for genre leaves the global
sottogenereUrlFilter stale, causing the AJAX payload to include the old
sottogenere; in the onSelect handler that sets genere_id (the genre autocomplete
callback), reset sottogenereUrlFilter to 0 (or empty string) and also clear any
sottogenere-related UI/state (e.g., the sottogenere input value and any hidden
sottogenere_id) so subsequent requests send the cleared sottogenere instead of
the previous URL value.
---
Outside diff comments:
In `@storage/plugins/digital-library/DigitalLibraryPlugin.php`:
- Around line 174-204: In registerHooks (DigitalLibraryPlugin::registerHooks)
the SELECT prepared statement assigned to $stmt is overwritten by the INSERT
prepared statement before being closed, leaking the SELECT handle; fix by
explicitly calling $stmt->close() immediately after finishing and before
reassigning $stmt for the INSERT (i.e., close the SELECT statement after $result
processing and before preparing the INSERT), and ensure the final $stmt->close()
still runs for the INSERT as well.
- Around line 95-105: The onInstall method in DigitalLibraryPlugin calls
$this->db->query("SHOW COLUMNS ...") and immediately reads $result->num_rows
which will throw if query() returns false; update the file_url and audio_url
checks to validate the query result (e.g., if $result === false) before
accessing num_rows, handle failures by logging or throwing with
$this->db->error, and only proceed to ALTER TABLE when the SHOW COLUMNS query
succeeded and num_rows === 0; ensure both occurrences (the two SHOW COLUMNS
checks in onInstall) follow this pattern so the installation fails gracefully
instead of causing a TypeError.
- Around line 392-407: The ebook branch currently allows
application/octet-stream in $allowedMime which short-circuits the extension
fallback and lets generateSafeFilename preserve dangerous extensions; update the
upload validation in the method that checks $clientMediaType (the block using
$allowedMime, $validExt and pathinfo) so that extension validation runs
unconditionally for ebooks (either remove 'application/octet-stream' from
$allowedMime or always perform the extension check regardless of MIME), and
additionally harden the plugin activation routine (onActivate in
DigitalLibraryPlugin) to write an uploads .htaccess that explicitly disables PHP
execution (e.g., via RemoveHandler / SetHandler or deny exec of .php) so
uploaded .php cannot execute.
---
Duplicate comments:
In `@app/Models/GenereRepository.php`:
- Around line 218-243: The merge leaves the target row pointing at the
soon-to-be-deleted source, so before deleting the source (the DELETE FROM generi
using $sourceId) fetch the source's parent_id and run an UPDATE generi SET
parent_id = ? WHERE id = ? to set the target's parent_id to the source's
parent_id (use $sourceParent, $targetId), using the same
$this->db->prepare()/bind_param()/execute() pattern; do this after moving
children and updating libri but before the DELETE, and wrap the sequence in the
existing transaction so the hierarchy update and source deletion are atomic.
In `@app/Views/generi/dettaglio_genere.php`:
- Around line 238-249: The confirm dialog currently concatenates translated
strings into JavaScript and must use safe JSON-encoded strings to avoid
injection; update the merge form script so the msgPrefix and msgSuffix values
are produced by PHP's json_encode (as shown) instead of raw echoed strings,
targeting the merge-genre-form handler (variable mergeForm) and the merge target
select (merge_target_id) and ensure msgPrefix/msgSuffix are JSON-encoded
translations before concatenation into the confirm() call and that
e.preventDefault() remains when the user cancels.
In `@storage/plugins/digital-library/DigitalLibraryPlugin.php`:
- Around line 497-504: The JSON encoding fallback must guarantee a string to
avoid TypeError when writing to the response body; update the catch in
DigitalLibraryPlugin (the block that sets $json and writes to
$response->getBody()) so that on JsonException you log the error (using
\App\Support\SecureLogger::error) and set $json to a non-false string (e.g. a
literal like '{"success":false,"message":"Internal error"}') instead of relying
on json_encode(...) which can return false; ensure
$response->getBody()->write($json) always receives that string and then return
$response->withHeader('Content-Type', 'application/json')->withStatus($status).
- Around line 357-383: The upload error handler in DigitalLibraryPlugin was
missing proper HTTP status mapping; update the $status match in the upload
handling code (the block that checks $file->getError() and builds $message) so
that UPLOAD_ERR_NO_TMP_DIR and UPLOAD_ERR_CANT_WRITE return 500 and all other
errors return 400 (this is the $status match you added), keep the existing
SecureLogger warning and message construction unchanged so the client gets the
localized message and the server returns the correct HTTP status.
---
Nitpick comments:
In `@app/Controllers/GeneriApiController.php`:
- Around line 106-170: The update() method currently ignores a self-reference
when $newParent === $id (so parent isn't changed silently); modify the logic in
GeneriApiController::update to detect $newParent === $id and immediately return
a 400 JSON response (Content-Type application/json) with a clear error message
(e.g. "Impossibile: il genitore non può essere se stesso.") instead of
continuing to perform the nome update; place this check where newParent is
computed/compared to $id so it short-circuits before ancestor cycle detection
and before calling GenereRepository::update.
In `@app/Controllers/GeneriController.php`:
- Around line 130-131: The catch in GeneriController (the block that sets
$_SESSION['error_message'] = __('Errore nell\'aggiornamento del genere: ') .
$e->getMessage()) must avoid appending raw exception text directly to a
localized prefix; instead either translate repository error strings at source or
map them here: replace concatenation with a localized, user-facing message (e.g.
__('Errore nell\'aggiornamento del genere')) and separately log the raw
$e->getMessage() for diagnostics, or run known repo messages through __() (or a
mapping array) before appending so languages don't mix; update the catch to use
the controller-level mapping/translation logic and move the raw exception into a
debug log rather than the session message.
In `@app/Models/GenereRepository.php`:
- Around line 195-201: Replace the fragile @@autocommit check with an explicit,
local transaction-tracking mechanism: add and use a boolean property (e.g.,
$inTransaction) on the GenereRepository class that is set to true when
begin_transaction() is called and cleared on commit() and rollback(), and use
that flag instead of querying @@autocommit to decide whether to call
begin_transaction(); update any internal methods that call begin_transaction(),
commit(), or rollback() to maintain this flag consistently (ensure the current
$wasInTransaction logic uses $this->inTransaction and not the @@autocommit
query).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/Controllers/GeneriApiController.phpapp/Controllers/GeneriController.phpapp/Models/GenereRepository.phpapp/Views/generi/dettaglio_genere.phpapp/Views/libri/index.phpstorage/plugins/digital-library/DigitalLibraryPlugin.php
There was a problem hiding this comment.
Actionable comments posted: 7
> [!CAUTION]
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)storage/plugins/digital-library/DigitalLibraryPlugin.php (3)
174-204:⚠️ Potential issue | 🟡 MinorPrepared statement leak: SELECT statement not closed before INSERT overwrites
$stmt.When a hook doesn't already exist (
$result->num_rows === 0), the$stmtprepared at line 176 (SELECT) is overwritten by the newprepare()at line 186 (INSERT) without being closed. Line 203 then only closes the INSERT statement. Over 6 hook iterations, this leaks up to 6 MySQLi statement handles.Proposed fix: close the SELECT statement before preparing the INSERT
if ($result->num_rows === 0) { + $stmt->close(); // Insert new hook $stmt = $this->db->prepare(" INSERT INTO plugin_hooksAlternatively, prepare both statements once outside the loop and reuse them.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/digital-library/DigitalLibraryPlugin.php` around lines 174 - 204, The SELECT prepared statement in the foreach over $hooks is being overwritten by the INSERT prepare call, leaking MySQLi statement handles; fix by closing the SELECT statement ($stmt->close()) after getting $result (before calling $this->db->prepare() for the INSERT), or better: prepare the SELECT and INSERT statements once outside the loop and bind/execute them per $hook (reuse statement handles) to avoid leaks; reference $stmt, $this->db->prepare(), $hook['hook_name'], plugin_hooks table and $this->pluginId when making the change.
95-105:⚠️ Potential issue | 🟡 MinorMissing error handling on
query()results —num_rowsonfalsewillTypeError.
$this->db->query("SHOW COLUMNS FROM libri LIKE 'file_url'")can returnfalseon failure (e.g., table doesn't exist yet). Accessing->num_rowsonfalsethrows aTypeErrorunderstrict_types=1. Same issue on line 101.Proposed fix
$result = $this->db->query("SHOW COLUMNS FROM libri LIKE 'file_url'"); - if ($result->num_rows === 0) { + if ($result && $result->num_rows === 0) { // Add file_url column if missing $this->db->query("ALTER TABLE libri ADD COLUMN file_url VARCHAR(500) DEFAULT NULL COMMENT 'eBook file URL' AFTER note_varie"); } $result = $this->db->query("SHOW COLUMNS FROM libri LIKE 'audio_url'"); - if ($result->num_rows === 0) { + if ($result && $result->num_rows === 0) { // Add audio_url column if missing $this->db->query("ALTER TABLE libri ADD COLUMN audio_url VARCHAR(500) DEFAULT NULL COMMENT 'Audiobook file URL' AFTER file_url"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/digital-library/DigitalLibraryPlugin.php` around lines 95 - 105, In DigitalLibraryPlugin, the calls to $this->db->query for "SHOW COLUMNS FROM libri LIKE 'file_url'" and "...'audio_url'" may return false, so avoid directly accessing ->num_rows; update the checks to first verify $result !== false (or is_bool/$result === false) and handle query failures (log or throw using $this->db->error) before checking $result->num_rows, then only run the ALTER TABLE when the result is valid and num_rows === 0; apply the same pattern for both the file_url and audio_url checks to prevent TypeError under strict_types=1.
79-82:⚠️ Potential issue | 🟡 MinorMissing null-check on
prepare()— willTypeErrorif the query fails.With
strict_types=1, if$this->db->prepare()returnsfalse(e.g., table doesn't exist yet during deactivation), callingbind_paramonfalsethrows aTypeError. The same guard pattern used elsewhere (if ($stmt) { ... }) should be applied here.Proposed fix
$stmt = $this->db->prepare("DELETE FROM plugin_hooks WHERE plugin_id = ?"); + if (!$stmt) { + return; + } $stmt->bind_param("i", $this->pluginId); $stmt->execute(); $stmt->close();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/digital-library/DigitalLibraryPlugin.php` around lines 79 - 82, The code calls $this->db->prepare(...) and then immediately $stmt->bind_param(...), which will throw a TypeError if prepare() returns false; wrap the subsequent bind_param/execute/close logic in the same null-check pattern used elsewhere (e.g., if ($stmt) { $stmt->bind_param("i", $this->pluginId); $stmt->execute(); $stmt->close(); }) so that when $this->db->prepare(...) returns false (such as when the plugin_hooks table is missing) you skip calling methods on $stmt.
♻️ Duplicate comments (8)
public/.htaccess (1)
1-19: LGTM —<IfModule>guards correctly address the prior PHP-FPM concern.All three blocks are gated behind their respective
<IfModule mod_php{,7,8}.c>guards, so non-module PHP deployments will not encounter 500 errors. Thepost_max_size(520 M) correctly exceedsupload_max_filesize(512 M), and the 300 s execution/input timeouts are appropriate for large audiobook uploads. No new issues here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/.htaccess` around lines 1 - 19, No changes required: the three <IfModule> guards (<IfModule mod_php.c>, <IfModule mod_php7.c>, <IfModule mod_php8.c>) correctly prevent PHP-FPM 500 errors and the directives upload_max_filesize (512M), post_max_size (520M), max_execution_time (300), and max_input_time (300) are appropriately set for large uploads—leave the .htaccess blocks as-is.app/Models/BookRepository.php (1)
980-996: N+1 queries concern remains: up to 5SELECTqueries pergetById()call for data already available in the JOIN.The main query already JOINs
generithree levels deep (g,gp,gpp) and loadsgenere_nome,radice_id,radice_nome,nonno_id,nonno_nomeinto$row. The resolver discards this and re-fetches via thewhileloop. For the common 1–3 level case, the chain could be built from$rowdata with zero extra queries, falling back to the loop only for L4+.This is bounded by
maxDepth = 5so it won't cause runaway queries, but it's still redundant work on every book detail page load.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/BookRepository.php` around lines 980 - 996, The loop that builds $chain (using $currentId, $maxDepth and $stmt) causes N+1 queries despite the main SELECT already joining generi into $row; change getById so you first construct the ancestor chain from the existing $row fields (e.g. genere_nome, radice_id/radice_nome, nonno_id/nonno_nome) and only enter the while-loop/fallback that uses $stmt (and variables $currentId/$maxDepth) when the book has deeper ancestry (L4+) or when required IDs are missing from $row, thereby avoiding extra SELECTs for the common 1–3 level case.storage/plugins/digital-library/DigitalLibraryPlugin.php (2)
497-502: JSON encoding safety improvements look correct.
JSON_THROW_ON_ERRORwith try/catch and a hardcoded string fallback addresses the previous review feedback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/digital-library/DigitalLibraryPlugin.php` around lines 497 - 502, The JSON encoding now uses JSON_THROW_ON_ERROR and a try/catch around json_encode in DigitalLibraryPlugin:: (the json_encode($data, JSON_UNESCAPED_UNICODE | JSON_UNESCAPED_SLASHES | JSON_HEX_TAG | JSON_THROW_ON_ERROR) block) and logs the exception via \App\Support\SecureLogger::error with a safe hardcoded fallback payload; no further changes are required—leave the try/catch and fallback string as implemented to ensure safe failure handling and secure logging.
358-383: Upload error handling properly differentiates client vs. server errors now.Server-side errors (
UPLOAD_ERR_NO_TMP_DIR,UPLOAD_ERR_CANT_WRITE) correctly return HTTP 500, while client errors return 400. This addresses the previous review feedback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/digital-library/DigitalLibraryPlugin.php` around lines 358 - 383, The upload error handling in DigitalLibraryPlugin.php correctly maps server-side upload errors (UPLOAD_ERR_NO_TMP_DIR, UPLOAD_ERR_CANT_WRITE) to HTTP 500 and client errors to 400 using the $errorCode and the two match expressions; no code change needed—just mark the PR as approved and remove the duplicate reviewer tag or duplicate comment marker present in the review metadata/comments.app/Views/generi/dettaglio_genere.php (2)
238-249: Merge confirmation prompt now correctly usesjson_encodefor both translation strings.The previously reported single-quote injection risk has been addressed with
json_encodeat lines 243–244.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/generi/dettaglio_genere.php` around lines 238 - 249, The confirmation prompt needs both translation strings to be safely JSON-encoded to prevent quote-injection; ensure the variables msgPrefix and msgSuffix are produced via json_encode(...) (as done) and used when constructing the confirm message in the submit handler attached to mergeForm (element id "merge-genre-form") that reads the selected option text from the select with id "merge_target_id"; keep using target.options[target.selectedIndex].textContent.trim() for targetName and combine msgPrefix + ' "' + targetName + '"? ' + msgSuffix in the confirm call so the prompt is safe and localized.
153-186:⚠️ Potential issue | 🟡 MinorDepth-3+ genres are still silently omitted from the merge dropdown.
The grouping logic assigns
_selfonly to genres whereparent_id === null. Genres at depth ≥ 2 land in$grouped[parentId]['_children']but never get a_selfentry in their own bucket, so theisset($group['_self'])guard at line 168 drops them. This was flagged in a previous review and marked "Addressed in commits 292f158 to d9cec8f," but the current code is unchanged from the problematic version.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/generi/dettaglio_genere.php` around lines 153 - 186, The merge dropdown omits depth-3+ genres because only root genres get a '_self' entry, so buckets for intermediate parents never render; fix by first building an index $byId = [id => genre] from $allGeneri, then when grouping (the code that populates $grouped and $orphanChildren) ensure that whenever you add a genre to $grouped[(int)$g['parent_id']]['_children'] you also set $grouped[(int)$g['parent_id']]['_self'] = $byId[(int)$g['parent_id']] if that parent exists; this guarantees intermediate parents (referenced by $grouped keys) have a '_self' and will pass the isset($group['_self']) check when rendering the optgroups.app/Models/GenereRepository.php (2)
209-216:⚠️ Potential issue | 🟡 MinorRenamed child may still collide after rename.
$newName = $child['nome'] . ' (ex ' . $source['nome'] . ')'is applied without checking whether that exact name already exists among the target's children. If a previous merge left a child named"X (ex Y)"under the target, the collision check at line 210 would pass (the original"X"conflicts), but the new name would also conflict.💡 Suggested safeguard
if (in_array($child['nome'], $targetChildNames, true)) { $newName = $child['nome'] . ' (ex ' . $source['nome'] . ')'; + // Ensure uniqueness with a counter suffix if needed + $base = $newName; + $suffix = 2; + while (in_array($newName, $targetChildNames, true)) { + $newName = $base . ' ' . $suffix++; + } $stmt = $this->db->prepare("UPDATE generi SET nome = ? WHERE id = ?");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/GenereRepository.php` around lines 209 - 216, The rename step in GenereRepository (loop over $sourceChildren) can still produce a collision because $newName = $child['nome'] . ' (ex ' . $source['nome'] . ')' is not checked for uniqueness; change the logic to test whether $newName exists among $targetChildNames (or via DB) and, if it does, append a numeric suffix and increment it until a unique name is found (e.g., " (ex X) 2", " (ex X) 3"), then run the UPDATE prepared statement for that unique name and add the final $newName to $targetChildNames so subsequent iterations see it; ensure you reuse or recreate the prepared statement as needed and update any in-memory name set to avoid race collisions in the same merge.
218-247:⚠️ Potential issue | 🟠 MajorMerging source into its own child leaves the target with a dangling
parent_id.The
AND id != ?guard (line 219) correctly prevents the self-reference bug, but it does not fix the broken hierarchy. When the merge target is a child of the source (reachable via the$orphanChildrenentries in the view's dropdown), after the source is deleted at line 241 the target'sparent_idstill equals the now-deletedsourceId.The second part of the original fix — reparenting the target to the source's own parent before deleting the source — was not applied. Without it, the target ends up with a broken
parent_idpointing to a non-existent row.🐛 Proposed fix — reparent target before deleting source
// Move children from source to target (exclude target itself to prevent self-referencing) $stmt = $this->db->prepare("UPDATE generi SET parent_id = ? WHERE parent_id = ? AND id != ?"); $stmt->bind_param('iii', $targetId, $sourceId, $targetId); $stmt->execute(); $childrenMoved = $stmt->affected_rows; + // If target was a child of source, inherit source's parent to avoid a dangling FK + $srcParent = $source['parent_id']; // null if source was top-level + $stmt = $this->db->prepare("UPDATE generi SET parent_id = ? WHERE id = ? AND parent_id = ?"); + $stmt->bind_param('iii', $srcParent, $targetId, $sourceId); + $stmt->execute();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/GenereRepository.php` around lines 218 - 247, The merge currently moves children and updates books but leaves the target's parent_id pointing to a deleted source when the target is a child of the source; to fix, before deleting the source (before the DELETE FROM generi using $sourceId) fetch the source's parent_id and, if the target's parent_id equals $sourceId, update the target row to set parent_id = that fetched parent (or NULL) using an UPDATE generi SET parent_id = ? WHERE id = ? (reuse $stmt or a new prepared statement) so the target is reparented to the source's parent inside the same transaction (use $sourceId, $targetId and $wasInTransaction context).
🧹 Nitpick comments (1)
app/Models/BookRepository.php (1)
1025-1041:sottogenere_nomeis not explicitly set in thechainLen === 2branch, unlike the other branches.In the
chainLen === 1and>= 3branches,sottogenere_nomeis explicitly assigned. In thechainLen === 2branch it retains whatever the SQL JOIN set. This works correctly because the JOIN'ssg.nomeis the right value, but for consistency and defensive clarity, consider setting it explicitly.Suggestion for consistency
} elseif ($chainLen === 2) { // genere_id points to L2 genre — standard case $row['radice_id'] = $chain[0]['id']; $row['radice_nome'] = $chain[0]['nome']; $row['genere_nome'] = $chain[1]['nome']; $row['genere_id_cascade'] = $chain[1]['id']; + // sottogenere_nome already set by SQL JOIN (sg.nome) — keep as-is $row['sottogenere_id_cascade'] = $sottogenereId;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/BookRepository.php` around lines 1025 - 1041, In the chainLen === 2 branch of the BookRepository mapping (the block that sets $row['radice_id'], $row['radice_nome'], $row['genere_nome'], $row['genere_id_cascade'], and $row['sottogenere_id_cascade']), explicitly set $row['sottogenere_nome'] as well (e.g. from the same source used in other branches) so that 'sottogenere_nome' is consistently populated across all branches and does not rely on the SQL JOIN value implicitly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Controllers/GeneriApiController.php`:
- Around line 154-160: The handler currently ignores the boolean result of
GenereRepository::update() and always returns updated=true; change the logic
around $repo->update($id, $updateData) to capture its return value (e.g., $ok =
$repo->update(...)) and if $ok is false return a 500 response (use
$response->withStatus(500) and a JSON body indicating updated=false and an error
message), otherwise continue to return the success JSON with updated=true;
ensure the Content-Type header remains application/json and keep references to
$repo->update, $id, $nome and the existing response writing flow.
In `@app/Controllers/GeneriController.php`:
- Around line 137-139: The destroy method currently has an unused $request
parameter (public function destroy(Request $request, Response $response, \mysqli
$db, int $id)): either remove $request from the signature if no
middleware/framework contract requires it, or rename it to $_request to silence
PHPMD (e.g., public function destroy($_request, Response $response, \mysqli $db,
int $id)); also update the method docblock/type hints accordingly and run tests
to ensure no callers or framework bindings rely on the original parameter order.
- Around line 87-135: The controller unconditionally sets
$_SESSION['success_message'] even if GenereRepository::update($id, $updateData)
fails; change the logic in update(Request $request, Response $response, \mysqli
$db, int $id) to capture the boolean result of $repo->update($id, $updateData),
and only set $_SESSION['success_message'] when that call returns true; if it
returns false, set $_SESSION['error_message'] (or throw/handle) and avoid the
success message so the UI reflects the real outcome. Ensure you still catch
exceptions in the existing try/catch around $repo->update and keep the final
redirect behavior.
In `@app/Models/GenereRepository.php`:
- Around line 241-243: The DELETE statement in GenereRepository.php (prepared in
the method that handles merging genres) doesn't check the return value of
$stmt->execute(), which can allow a failed DELETE to be ignored while the
transaction still commits; modify the code around the DELETE (the prepared
statement using $this->db->prepare and $stmt->bind_param for $sourceId) to check
if $stmt->execute() returns false and, on failure, throw an exception (including
$stmt->error or $this->db->error) or call rollback before rethrowing so the
outer transaction does not commit partial changes; ensure behavior matches other
methods like create() that validate execute() and throw on failure.
- Around line 231-238: The two UPDATE queries in GenereRepository that set
genere_id and sottogenere_id currently include "AND deleted_at IS NULL", which
leaves soft-deleted libri pointing at the source genre; remove that clause (or
add an additional UPDATE that targets rows WHERE deleted_at IS NOT NULL) so all
libri rows are updated during the merge. Locate the UPDATE statements that
prepare "UPDATE libri SET genere_id = ? WHERE genere_id = ? AND deleted_at IS
NULL" and "UPDATE libri SET sottogenere_id = ? WHERE sottogenere_id = ? AND
deleted_at IS NULL" and either drop the "AND deleted_at IS NULL" filter or add a
follow-up update to nullify/redirect stale references on deleted rows to the
targetId (or NULL) as appropriate.
In `@app/Views/libri/index.php`:
- Around line 447-483: The closeBtn click handler currently only clears
collanaFilter, genereUrlFilter and sottogenereUrlFilter but leaves
initialKeywords and the visible keyword input/chip unchanged; update the handler
(the function attached to closeBtn.addEventListener('click')) to also clear the
keyword filter state (set initialKeywords to empty or the canonical keyword
variable the page uses), clear the prefilled keyword input element (e.g. the
search/input element that shows the URL keyword) and remove any keyword
chip/indicator from the UI so the active filters banner and chips reflect no
keyword filter before calling banner.remove() and table.ajax.reload().
In `@package.json`:
- Line 7: The package.json "test" script references tests/playwright.config.js
which is missing; either add a Playwright config at tests/playwright.config.js
or update the "test" script in package.json to point to the correct existing
config (for example tests/playwright.config.ts or playwright.config.js at repo
root). Ensure the new config is compatible with `@playwright/test`@^1.58.2 and
contains the necessary exports (e.g., default config object) so running the
"test" npm script (the "test" entry in package.json) succeeds.
---
Outside diff comments:
In `@storage/plugins/digital-library/DigitalLibraryPlugin.php`:
- Around line 174-204: The SELECT prepared statement in the foreach over $hooks
is being overwritten by the INSERT prepare call, leaking MySQLi statement
handles; fix by closing the SELECT statement ($stmt->close()) after getting
$result (before calling $this->db->prepare() for the INSERT), or better: prepare
the SELECT and INSERT statements once outside the loop and bind/execute them per
$hook (reuse statement handles) to avoid leaks; reference $stmt,
$this->db->prepare(), $hook['hook_name'], plugin_hooks table and $this->pluginId
when making the change.
- Around line 95-105: In DigitalLibraryPlugin, the calls to $this->db->query for
"SHOW COLUMNS FROM libri LIKE 'file_url'" and "...'audio_url'" may return false,
so avoid directly accessing ->num_rows; update the checks to first verify
$result !== false (or is_bool/$result === false) and handle query failures (log
or throw using $this->db->error) before checking $result->num_rows, then only
run the ALTER TABLE when the result is valid and num_rows === 0; apply the same
pattern for both the file_url and audio_url checks to prevent TypeError under
strict_types=1.
- Around line 79-82: The code calls $this->db->prepare(...) and then immediately
$stmt->bind_param(...), which will throw a TypeError if prepare() returns false;
wrap the subsequent bind_param/execute/close logic in the same null-check
pattern used elsewhere (e.g., if ($stmt) { $stmt->bind_param("i",
$this->pluginId); $stmt->execute(); $stmt->close(); }) so that when
$this->db->prepare(...) returns false (such as when the plugin_hooks table is
missing) you skip calling methods on $stmt.
---
Duplicate comments:
In `@app/Models/BookRepository.php`:
- Around line 980-996: The loop that builds $chain (using $currentId, $maxDepth
and $stmt) causes N+1 queries despite the main SELECT already joining generi
into $row; change getById so you first construct the ancestor chain from the
existing $row fields (e.g. genere_nome, radice_id/radice_nome,
nonno_id/nonno_nome) and only enter the while-loop/fallback that uses $stmt (and
variables $currentId/$maxDepth) when the book has deeper ancestry (L4+) or when
required IDs are missing from $row, thereby avoiding extra SELECTs for the
common 1–3 level case.
In `@app/Models/GenereRepository.php`:
- Around line 209-216: The rename step in GenereRepository (loop over
$sourceChildren) can still produce a collision because $newName = $child['nome']
. ' (ex ' . $source['nome'] . ')' is not checked for uniqueness; change the
logic to test whether $newName exists among $targetChildNames (or via DB) and,
if it does, append a numeric suffix and increment it until a unique name is
found (e.g., " (ex X) 2", " (ex X) 3"), then run the UPDATE prepared statement
for that unique name and add the final $newName to $targetChildNames so
subsequent iterations see it; ensure you reuse or recreate the prepared
statement as needed and update any in-memory name set to avoid race collisions
in the same merge.
- Around line 218-247: The merge currently moves children and updates books but
leaves the target's parent_id pointing to a deleted source when the target is a
child of the source; to fix, before deleting the source (before the DELETE FROM
generi using $sourceId) fetch the source's parent_id and, if the target's
parent_id equals $sourceId, update the target row to set parent_id = that
fetched parent (or NULL) using an UPDATE generi SET parent_id = ? WHERE id = ?
(reuse $stmt or a new prepared statement) so the target is reparented to the
source's parent inside the same transaction (use $sourceId, $targetId and
$wasInTransaction context).
In `@app/Views/generi/dettaglio_genere.php`:
- Around line 238-249: The confirmation prompt needs both translation strings to
be safely JSON-encoded to prevent quote-injection; ensure the variables
msgPrefix and msgSuffix are produced via json_encode(...) (as done) and used
when constructing the confirm message in the submit handler attached to
mergeForm (element id "merge-genre-form") that reads the selected option text
from the select with id "merge_target_id"; keep using
target.options[target.selectedIndex].textContent.trim() for targetName and
combine msgPrefix + ' "' + targetName + '"? ' + msgSuffix in the confirm call so
the prompt is safe and localized.
- Around line 153-186: The merge dropdown omits depth-3+ genres because only
root genres get a '_self' entry, so buckets for intermediate parents never
render; fix by first building an index $byId = [id => genre] from $allGeneri,
then when grouping (the code that populates $grouped and $orphanChildren) ensure
that whenever you add a genre to $grouped[(int)$g['parent_id']]['_children'] you
also set $grouped[(int)$g['parent_id']]['_self'] = $byId[(int)$g['parent_id']]
if that parent exists; this guarantees intermediate parents (referenced by
$grouped keys) have a '_self' and will pass the isset($group['_self']) check
when rendering the optgroups.
In `@public/.htaccess`:
- Around line 1-19: No changes required: the three <IfModule> guards (<IfModule
mod_php.c>, <IfModule mod_php7.c>, <IfModule mod_php8.c>) correctly prevent
PHP-FPM 500 errors and the directives upload_max_filesize (512M), post_max_size
(520M), max_execution_time (300), and max_input_time (300) are appropriately set
for large uploads—leave the .htaccess blocks as-is.
In `@storage/plugins/digital-library/DigitalLibraryPlugin.php`:
- Around line 497-502: The JSON encoding now uses JSON_THROW_ON_ERROR and a
try/catch around json_encode in DigitalLibraryPlugin:: (the json_encode($data,
JSON_UNESCAPED_UNICODE | JSON_UNESCAPED_SLASHES | JSON_HEX_TAG |
JSON_THROW_ON_ERROR) block) and logs the exception via
\App\Support\SecureLogger::error with a safe hardcoded fallback payload; no
further changes are required—leave the try/catch and fallback string as
implemented to ensure safe failure handling and secure logging.
- Around line 358-383: The upload error handling in DigitalLibraryPlugin.php
correctly maps server-side upload errors (UPLOAD_ERR_NO_TMP_DIR,
UPLOAD_ERR_CANT_WRITE) to HTTP 500 and client errors to 400 using the $errorCode
and the two match expressions; no code change needed—just mark the PR as
approved and remove the duplicate reviewer tag or duplicate comment marker
present in the review metadata/comments.
---
Nitpick comments:
In `@app/Models/BookRepository.php`:
- Around line 1025-1041: In the chainLen === 2 branch of the BookRepository
mapping (the block that sets $row['radice_id'], $row['radice_nome'],
$row['genere_nome'], $row['genere_id_cascade'], and
$row['sottogenere_id_cascade']), explicitly set $row['sottogenere_nome'] as well
(e.g. from the same source used in other branches) so that 'sottogenere_nome' is
consistently populated across all branches and does not rely on the SQL JOIN
value implicitly.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
app/Controllers/GeneriApiController.phpapp/Controllers/GeneriController.phpapp/Controllers/LibriApiController.phpapp/Models/BookRepository.phpapp/Models/GenereRepository.phpapp/Routes/web.phpapp/Views/generi/dettaglio_genere.phpapp/Views/libri/index.phpapp/Views/libri/partials/book_form.phppackage.jsonpublic/.htaccessstorage/plugins/api-book-scraper/plugin.jsonstorage/plugins/dewey-editor/plugin.jsonstorage/plugins/digital-library/DigitalLibraryPlugin.phpstorage/plugins/digital-library/plugin.jsonstorage/plugins/open-library/plugin.jsonstorage/plugins/z39-server/plugin.json
app/Controllers/GeneriController.php
Outdated
Comment on lines
137
to
139
| public function destroy(Request $request, Response $response, \mysqli $db, int $id): Response | ||
| { | ||
| // CSRF validated by CsrfMiddleware |
There was a problem hiding this comment.
Remove or rename unused $request parameter.
PHPMD flags this as an unused parameter; consider removing it or renaming to $_request to suppress the warning.
🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 137-137: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Controllers/GeneriController.php` around lines 137 - 139, The destroy
method currently has an unused $request parameter (public function
destroy(Request $request, Response $response, \mysqli $db, int $id)): either
remove $request from the signature if no middleware/framework contract requires
it, or rename it to $_request to silence PHPMD (e.g., public function
destroy($_request, Response $response, \mysqli $db, int $id)); also update the
method docblock/type hints accordingly and run tests to ensure no callers or
framework bindings rely on the original parameter order.
- GenereRepository::merge: update ALL books (incl. soft-deleted) to prevent dangling FK refs after source genre deletion - GenereRepository::merge: check DELETE execute() return, throw on failure - GenereRepository::merge: add collision counter for child rename conflicts - GeneriController::update: check repo->update() return value - GeneriApiController::update: check repo->update() return value - DigitalLibraryPlugin::onDeactivate: guard prepare() against false - libri/index.php: banner close clears keywords + calls updateActiveFilters()
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
app/Models/GenereRepository.php (1)
179-206:⚠️ Potential issue | 🟠 MajorBlock merges into descendants to avoid cyclic hierarchies.
Right now onlysourceId === targetIdis blocked. If the target is a grandchild (or deeper) of the source, the “move children” step can create a cycle (e.g., target ↔ former child). Add a guard that walks target ancestors and rejects if it hits the source.🛡️ Proposed fix (ancestor walk guard)
$target = $this->getById($targetId); if (!$target) { throw new \InvalidArgumentException('Genere di destinazione non trovato'); } + + // Prevent merging into a descendant (would create cycles) + $ancestorId = $targetId; + $depth = 20; + $aStmt = $this->db->prepare('SELECT parent_id FROM generi WHERE id = ?'); + while ($ancestorId > 0 && $depth-- > 0) { + if ($ancestorId === $sourceId) { + $aStmt->close(); + throw new \InvalidArgumentException('Impossibile unire un genere con un suo discendente'); + } + $aStmt->bind_param('i', $ancestorId); + $aStmt->execute(); + $row = $aStmt->get_result()->fetch_assoc(); + $ancestorId = $row ? (int)($row['parent_id'] ?? 0) : 0; + } + $aStmt->close();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/GenereRepository.php` around lines 179 - 206, The merge method allows merging a source into a target but only checks sourceId === targetId; add an ancestor-walk guard in merge (before starting/entering the transaction) to reject merges where the target is a descendant of the source: repeatedly fetch the target's parent via getById (or the repository's parent_id field) and walk up ancestors until null, and if any ancestor id equals sourceId throw an InvalidArgumentException (e.g., "Non è possibile unire un genere in un suo discendente"); keep this check before the transaction/renaming logic so cycles cannot be created by subsequent move of children.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Controllers/GeneriController.php`:
- Around line 140-153: In destroy(), don't assume GenereRepository::delete($id)
throws on failure—check its boolean return value: call $repo->delete($id) and if
it returns true set $_SESSION['success_message'] and redirect to /admin/generi,
otherwise set $_SESSION['error_message'] (include a helpful message or DB error
if available) and redirect back to /admin/generi/{$id}; keep the existing
try/catch for exceptions but treat a false return as a failed deletion.
In `@app/Views/generi/dettaglio_genere.php`:
- Around line 67-74: The parent dropdown currently filters out non-top-level
items (the `if ($g['parent_id'] !== null) continue;` check), causing genres with
non-top parents to be lost on submit; remove that check so the loop over
`$allGeneri` renders every genre except the current one (`if ((int)$g['id'] ===
$genereId) continue;` should remain). Ensure the option selection logic still
uses `(int)($genere['parent_id'] ?? 0) === (int)$g['id']` and keep HTML escaping
via `HtmlHelper::e($g['nome'])`; rely on existing server-side cycle detection to
prevent invalid reparenting.
In `@storage/plugins/digital-library/DigitalLibraryPlugin.php`:
- Around line 62-71: The generated .htaccess written by file_put_contents to
$htaccess uses legacy Apache directives only; update the FilesMatch block
created in the array passed to file_put_contents so it includes the Apache 2.4
directive "Require all denied" alongside the existing "Order Deny,Allow" and
"Deny from all" lines to maintain backward compatibility with older servers
while ensuring modern servers block execution; modify the array in the
file_put_contents call in DigitalLibraryPlugin.php to insert the "Require all
denied" line (with appropriate indentation) inside the <?FilesMatch?> block.
---
Duplicate comments:
In `@app/Models/GenereRepository.php`:
- Around line 179-206: The merge method allows merging a source into a target
but only checks sourceId === targetId; add an ancestor-walk guard in merge
(before starting/entering the transaction) to reject merges where the target is
a descendant of the source: repeatedly fetch the target's parent via getById (or
the repository's parent_id field) and walk up ancestors until null, and if any
ancestor id equals sourceId throw an InvalidArgumentException (e.g., "Non è
possibile unire un genere in un suo discendente"); keep this check before the
transaction/renaming logic so cycles cannot be created by subsequent move of
children.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/Controllers/GeneriApiController.phpapp/Controllers/GeneriController.phpapp/Models/GenereRepository.phpapp/Views/generi/dettaglio_genere.phpapp/Views/libri/index.phpstorage/plugins/digital-library/DigitalLibraryPlugin.php
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Views/generi/dettaglio_genere.php (1)
14-33:⚠️ Potential issue | 🟡 MinorBreadcrumb labels are not wrapped in
__().Lines 18, 26, and 32 contain hardcoded Italian strings ("Home", "Generi", "Dettaglio") that are not passed through the
__()translation function, unlike the rest of the file which consistently uses it.Proposed fix
- <i class="fas fa-home mr-1"></i>Home + <i class="fas fa-home mr-1"></i><?= __("Home") ?>- <i class="fas fa-tags mr-1"></i>Generi + <i class="fas fa-tags mr-1"></i><?= __("Generi") ?>- <li class="text-gray-900 font-medium">Dettaglio</li> + <li class="text-gray-900 font-medium"><?= __("Dettaglio") ?></li>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/generi/dettaglio_genere.php` around lines 14 - 33, Replace the three hardcoded breadcrumb labels with calls to the translation helper so they follow the file's localization pattern: wrap "Home", "Generi" and "Dettaglio" in the __('...') function used elsewhere in the view (keep the surrounding anchor/HTML and icon elements intact and retain existing htmlspecialchars() calls for URLs); update the text nodes inside the anchor for the Home and Generi links and the plain text node for the Dettaglio list item to use __('Home'), __('Generi') and __('Dettaglio') respectively.
♻️ Duplicate comments (5)
public/.htaccess (1)
1-19: LGTM —<IfModule>guards correctly address the prior review concern.The three blocks cover
mod_php.c(legacy),mod_php7.c, andmod_php8.cfor cross-version compatibility; only the active module will match. Thepost_max_size(520M) is correctly sized aboveupload_max_filesize(512M), and the stated 500 MB audiobook ceiling is covered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/.htaccess` around lines 1 - 19, No changes needed: the .htaccess already uses <IfModule> guards for mod_php.c, mod_php7.c and mod_php8.c and correctly sets php_value upload_max_filesize to 512M with post_max_size 520M (ensuring post_max_size > upload_max_filesize) along with max_execution_time and max_input_time; leave the three <IfModule> blocks and the directives upload_max_filesize, post_max_size, max_execution_time, and max_input_time as-is.package.json (1)
7-7:⚠️ Potential issue | 🟠 MajorConfirm the Playwright config path exists.
The test script hardcodes
tests/playwright.config.js. If that file is missing or named differently,npm testwill fail. Please verify the path or update the script accordingly.#!/bin/bash # Verify Playwright config location(s) fd -t f -I 'playwright.config.*' --full-path🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 7, The package.json "test" script hardcodes "tests/playwright.config.js", which will fail if that path/name doesn't exist; verify the actual Playwright config file(s) and update the "test" script (the "test" key in package.json) to reference the correct path (e.g., "playwright.config.js" at project root or "tests/playwright.config.ts"), or remove the explicit --config flag so Playwright uses its default discovery; run the provided fd command (or similar) to find existing playwright.config.* files and update the package.json test value accordingly.app/Controllers/GeneriController.php (1)
140-153:⚠️ Potential issue | 🟠 Major
delete()return value is still unchecked — false is silently treated as success.
GenereRepository::delete()returnsboolfrom$stmt->execute(). If the DELETE fails at the DB level without throwing (e.g., a constraint error that produces a warning rather than an exception),execute()returnsfalse, but line 147 still reports success.Proposed fix
try { - $repo->delete($id); + if (!$repo->delete($id)) { + throw new \RuntimeException('delete() returned false'); + } $_SESSION['success_message'] = __('Genere eliminato con successo!');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/GeneriController.php` around lines 140 - 153, The destroy() action treats any non-exceptional failure as success because it ignores the boolean return of GenereRepository::delete(); call $repo->delete($id) into a variable, check if it === true before setting the success flash and redirecting; if it returns false, set an appropriate error flash (include DB error info available from the repository or $db if exposed) and redirect back to the edit/view page instead of reporting success. Ensure you reference GenereRepository::delete and the $repo->delete($id) call in the destroy() method and handle the false path the same as the catch block (set $_SESSION['error_message'] and redirect).storage/plugins/digital-library/DigitalLibraryPlugin.php (1)
62-71:⚠️ Potential issue | 🟠 MajorApache 2.4 compatibility still missing in generated
.htaccess.The
Order Deny,Allow/Deny from alldirectives are legacy Apache 2.2 syntax. On Apache 2.4+ withoutmod_access_compatloaded, these are silently ignored, leaving PHP execution unblocked in the uploads directory.🔒 Suggested hardening for both Apache 2.2 and 2.4
file_put_contents($htaccess, implode("\n", [ '# Protect directory listing', 'Options -Indexes', '# Deny execution of any server-side scripts', '<FilesMatch "\.(php\d?|phtml|phar|pl|py|cgi|sh)$">', - ' Order Deny,Allow', - ' Deny from all', + ' <IfModule mod_authz_core.c>', + ' Require all denied', + ' </IfModule>', + ' <IfModule mod_access_compat.c>', + ' Order Deny,Allow', + ' Deny from all', + ' </IfModule>', '</FilesMatch>', '', ]));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/digital-library/DigitalLibraryPlugin.php` around lines 62 - 71, The .htaccess content written by file_put_contents(...) in DigitalLibraryPlugin (around the block that builds the array passed to file_put_contents) uses Apache 2.2 directives only; update the generated content to include Apache 2.4-compatible directives (e.g. inside the same <FilesMatch "..."> add a Require all denied clause or wrap 2.4 directives in an <IfModule mod_authz_core.c> and keep the legacy Order/Deny lines inside an <IfModule !mod_authz_core.c> or mod_access_compat fallback) so both Apache 2.2 and 2.4 are covered and PHP/script execution is denied in uploads. Ensure you change the array passed to file_put_contents($htaccess, implode(...)) to include those conditional directives while preserving the existing patterns like <FilesMatch "\.(php\d?|phtml|phar|pl|py|cgi|sh)$"> and closing tags.app/Views/generi/dettaglio_genere.php (1)
66-75: Parent dropdown still excludes non-top-level genres.Line 72 filters out any genre with a non-null
parent_id, so the dropdown only shows top-level genres. If the current genre's parent is itself a subgenre, it won't appear as selected, and submitting the form will silently reparent to top-level. Consider listing all genres (excluding self) and relying on server-side cycle detection, which already exists inGeneriController::update().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/generi/dettaglio_genere.php` around lines 66 - 75, The parent dropdown currently excludes any genre with a non-null parent by skipping entries where $g['parent_id'] !== null; update the template so it lists all genres from $allGeneri except the current one ($genereId) (i.e., remove the "if ($g['parent_id'] !== null) continue;" filter) and keep the existing self-exclusion and selected logic using $genere['parent_id']; rely on GeneriController::update()'s server-side cycle detection to prevent invalid parenting.
🧹 Nitpick comments (2)
app/Controllers/GeneriApiController.php (1)
129-134:parent_id: nullin JSON body won't trigger reparenting to top-level.
isset($data['parent_id'])returnsfalsewhen the value isnull, so an API client sending{"parent_id": null}to make a genre top-level will have the parent unchanged. Sending{"parent_id": 0}or{"parent_id": ""}works. This matches the web controller's behavior but may surprise API consumers.Consider using
array_key_exists('parent_id', $data)instead (matching the pattern inGenereRepository::update()on line 97 of GenereRepository.php) for consistent semantics:Proposed fix
- if (isset($data['parent_id'])) { + if (array_key_exists('parent_id', $data)) {This is low-risk since the form-based flow works either way; only the JSON API path for explicit
nullwould differ.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/GeneriApiController.php` around lines 129 - 134, The API currently checks parent presence with isset($data['parent_id']), which ignores explicit null and prevents reparenting to top-level; replace that isset check with array_key_exists('parent_id', $data) in the controller (the block that computes $newParent and compares it to $id) so that {"parent_id": null} is treated as an explicit request to set parent to null (keep the existing logic that sets $newParent = !empty($data['parent_id']) ? (int)$data['parent_id'] : null and the self-parent validation).storage/plugins/digital-library/DigitalLibraryPlugin.php (1)
419-423: Emptyifblock for MIME validation is a code smell.The MIME check condition evaluates but the body is empty (just a comment). This is confusing to future readers. Either remove the
ifentirely (leaving just the comment) or log a debug warning when the MIME doesn't match but extension is valid.Suggested cleanup
$clientMediaType = $file->getClientMediaType(); - if (!in_array($clientMediaType, $allowedMime, true)) { - // Extension already validated above — allow if extension is safe - // (browsers may report wrong MIME for valid files) - } + // MIME mismatch is tolerated when extension is valid (browsers may report wrong MIME) + if (!in_array($clientMediaType, $allowedMime, true)) { + \App\Support\SecureLogger::debug('[Digital Library] MIME mismatch tolerated', [ + 'filename' => $filename, + 'reported_mime' => $clientMediaType, + 'extension' => $ext, + ]); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/digital-library/DigitalLibraryPlugin.php` around lines 419 - 423, The empty MIME-check if block in DigitalLibraryPlugin (the check using $clientMediaType = $file->getClientMediaType() and if (!in_array($clientMediaType, $allowedMime, true))) is a code smell; replace it with concrete behavior: either remove the if entirely or (preferred) log a debug/warning when the MIME doesn't match but the extension was already validated. Update the block inside the DigitalLibraryPlugin upload/validation method to call the plugin's logger (or appropriate logger used elsewhere in the class) to emit a clear message including $clientMediaType and the file name/extension, so future readers see why mismatched MIME types are allowed. Ensure you keep the comment about browsers reporting wrong MIME types if you keep the log, or delete both the if and comment if you choose to remove the check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Controllers/GeneriController.php`:
- Around line 149-152: In destroy() avoid exposing raw exception text to users:
catch the Throwable, log the full exception (including $e->getMessage(), $e)
using the controller/logger, and set $_SESSION['error_message'] to a generic
safe message (e.g. "Errore durante l'eliminazione") instead of appending
$e->getMessage(); keep the redirect logic intact. Ensure this change covers both
expected RuntimeException from the model's delete() and unexpected Throwables by
using the existing catch block and calling the logger there while returning the
same 302 redirect.
- Around line 155-182: In the merge method's catch block, avoid exposing raw
$e->getMessage() to users; instead detect validation/expected exceptions (e.g.,
InvalidArgumentException or a repository-specific validation exception thrown by
GenereRepository) and use their messages for $_SESSION['error_message'], but for
any other \Throwable log the full exception (stack/message) to the server logger
and set a generic localized message like __('Errore nell\'unione.') for the
user; update the catch in merge() to branch on the exception type, call the
existing logger or error_log($e) for unexpected errors, and only append
$e->getMessage() to session messages for known validation exceptions.
In `@app/Models/BookRepository.php`:
- Around line 985-997: The code assumes $this->db->prepare(...) returns a valid
statement before calling bind_param/execute in the genre chain loop; add a guard
after $stmt = $this->db->prepare('SELECT id, nome, parent_id FROM generi WHERE
id = ?') to check if $stmt is false (prepare failed) and bail (throw an
exception or return an empty chain) with a clear error/log message before
entering the while loop, ensuring $stmt->close() is only called when $stmt is a
valid mysqli_stmt; update the code that uses $stmt in BookRepository (the
prepare/bind_param/execute/get_result sequence) accordingly.
In `@app/Models/GenereRepository.php`:
- Around line 209-223: The UPDATE inside the merge loop (the foreach over
$sourceChildren updating "generi SET nome = ?") does not check $stmt->execute()
result; change this to verify the execute() return and if it fails throw an
exception (include $this->db->error / $stmt->error in the message) so
name-collision failures are surfaced immediately (mirroring the checked DELETE
later). Locate the block that prepares "UPDATE generi SET nome = ?" (the $stmt
for $child['id']) and add a conditional to detect execute() === false and throw
a descriptive RuntimeException/Exception before continuing with the parent_id
updates.
- Around line 160-174: The getAllFlat() method currently assumes
$this->db->query(...) returns a valid mysqli_result and will call
$result->fetch_assoc() unguarded; change it to check the query result for false
(e.g. if ($result === false) { handle error / return [] }) before entering the
fetch loop, mirroring the defensive pattern used in the plugin's onInstall;
ensure you reference the $this->db->query call and the $result->fetch_assoc
usage in getAllFlat() and return an empty array or propagate/log the DB error
instead of calling fetch_assoc on false.
---
Outside diff comments:
In `@app/Views/generi/dettaglio_genere.php`:
- Around line 14-33: Replace the three hardcoded breadcrumb labels with calls to
the translation helper so they follow the file's localization pattern: wrap
"Home", "Generi" and "Dettaglio" in the __('...') function used elsewhere in the
view (keep the surrounding anchor/HTML and icon elements intact and retain
existing htmlspecialchars() calls for URLs); update the text nodes inside the
anchor for the Home and Generi links and the plain text node for the Dettaglio
list item to use __('Home'), __('Generi') and __('Dettaglio') respectively.
---
Duplicate comments:
In `@app/Controllers/GeneriController.php`:
- Around line 140-153: The destroy() action treats any non-exceptional failure
as success because it ignores the boolean return of GenereRepository::delete();
call $repo->delete($id) into a variable, check if it === true before setting the
success flash and redirecting; if it returns false, set an appropriate error
flash (include DB error info available from the repository or $db if exposed)
and redirect back to the edit/view page instead of reporting success. Ensure you
reference GenereRepository::delete and the $repo->delete($id) call in the
destroy() method and handle the false path the same as the catch block (set
$_SESSION['error_message'] and redirect).
In `@app/Views/generi/dettaglio_genere.php`:
- Around line 66-75: The parent dropdown currently excludes any genre with a
non-null parent by skipping entries where $g['parent_id'] !== null; update the
template so it lists all genres from $allGeneri except the current one
($genereId) (i.e., remove the "if ($g['parent_id'] !== null) continue;" filter)
and keep the existing self-exclusion and selected logic using
$genere['parent_id']; rely on GeneriController::update()'s server-side cycle
detection to prevent invalid parenting.
In `@package.json`:
- Line 7: The package.json "test" script hardcodes "tests/playwright.config.js",
which will fail if that path/name doesn't exist; verify the actual Playwright
config file(s) and update the "test" script (the "test" key in package.json) to
reference the correct path (e.g., "playwright.config.js" at project root or
"tests/playwright.config.ts"), or remove the explicit --config flag so
Playwright uses its default discovery; run the provided fd command (or similar)
to find existing playwright.config.* files and update the package.json test
value accordingly.
In `@public/.htaccess`:
- Around line 1-19: No changes needed: the .htaccess already uses <IfModule>
guards for mod_php.c, mod_php7.c and mod_php8.c and correctly sets php_value
upload_max_filesize to 512M with post_max_size 520M (ensuring post_max_size >
upload_max_filesize) along with max_execution_time and max_input_time; leave the
three <IfModule> blocks and the directives upload_max_filesize, post_max_size,
max_execution_time, and max_input_time as-is.
In `@storage/plugins/digital-library/DigitalLibraryPlugin.php`:
- Around line 62-71: The .htaccess content written by file_put_contents(...) in
DigitalLibraryPlugin (around the block that builds the array passed to
file_put_contents) uses Apache 2.2 directives only; update the generated content
to include Apache 2.4-compatible directives (e.g. inside the same <FilesMatch
"..."> add a Require all denied clause or wrap 2.4 directives in an <IfModule
mod_authz_core.c> and keep the legacy Order/Deny lines inside an <IfModule
!mod_authz_core.c> or mod_access_compat fallback) so both Apache 2.2 and 2.4 are
covered and PHP/script execution is denied in uploads. Ensure you change the
array passed to file_put_contents($htaccess, implode(...)) to include those
conditional directives while preserving the existing patterns like <FilesMatch
"\.(php\d?|phtml|phar|pl|py|cgi|sh)$"> and closing tags.
---
Nitpick comments:
In `@app/Controllers/GeneriApiController.php`:
- Around line 129-134: The API currently checks parent presence with
isset($data['parent_id']), which ignores explicit null and prevents reparenting
to top-level; replace that isset check with array_key_exists('parent_id', $data)
in the controller (the block that computes $newParent and compares it to $id) so
that {"parent_id": null} is treated as an explicit request to set parent to null
(keep the existing logic that sets $newParent = !empty($data['parent_id']) ?
(int)$data['parent_id'] : null and the self-parent validation).
In `@storage/plugins/digital-library/DigitalLibraryPlugin.php`:
- Around line 419-423: The empty MIME-check if block in DigitalLibraryPlugin
(the check using $clientMediaType = $file->getClientMediaType() and if
(!in_array($clientMediaType, $allowedMime, true))) is a code smell; replace it
with concrete behavior: either remove the if entirely or (preferred) log a
debug/warning when the MIME doesn't match but the extension was already
validated. Update the block inside the DigitalLibraryPlugin upload/validation
method to call the plugin's logger (or appropriate logger used elsewhere in the
class) to emit a clear message including $clientMediaType and the file
name/extension, so future readers see why mismatched MIME types are allowed.
Ensure you keep the comment about browsers reporting wrong MIME types if you
keep the log, or delete both the if and comment if you choose to remove the
check.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
app/Controllers/GeneriApiController.phpapp/Controllers/GeneriController.phpapp/Controllers/LibriApiController.phpapp/Models/BookRepository.phpapp/Models/GenereRepository.phpapp/Routes/web.phpapp/Views/generi/dettaglio_genere.phpapp/Views/libri/index.phpapp/Views/libri/partials/book_form.phppackage.jsonpublic/.htaccessstorage/plugins/api-book-scraper/plugin.jsonstorage/plugins/dewey-editor/plugin.jsonstorage/plugins/digital-library/DigitalLibraryPlugin.phpstorage/plugins/digital-library/plugin.jsonstorage/plugins/open-library/plugin.jsonstorage/plugins/z39-server/plugin.json
…wn, Apache 2.4 - GenereRepository::merge: ancestor walk to block merging into descendants - GeneriController::destroy: check delete() return value before reporting success - dettaglio_genere: parent dropdown shows all genres (not just top-level), with parent name label, preventing silent reparenting on submit - DigitalLibraryPlugin: .htaccess uses IfModule for Apache 2.4 (Require all denied) + legacy (Order Deny,Allow) backward compatibility
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
- Remove unreliable @@autocommit transaction detection in merge(), use direct begin_transaction/commit/rollback - Increase cycle detection depth from 10 to 100 in both controllers - Add explicit self-parent error in web controller (matching API) - Replace raw exception messages with SecureLogger + generic flash
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
storage/plugins/digital-library/DigitalLibraryPlugin.php (1)
410-440:⚠️ Potential issue | 🟠 MajorGuard null/empty client filenames before
strtolower()andgenerateSafeFilename().PSR-7 specifies that
getClientFilename()returnsstring|nullwhen no filename was provided by the client. Withstrict_types=1enabled in this file, passingnulltogenerateSafeFilename(string $original, ...)will throw aTypeError. Additionally,strtolower(null)may fail depending on the PHP version. Add a null/empty guard and return a 400 error response when the filename is missing.🔧 Proposed fix
+ $clientFilename = $file->getClientFilename(); + if ($clientFilename === null || $clientFilename === '') { + return $this->json($response, ['success' => false, 'message' => __('Nome file non valido.')], 400); + } - $filename = strtolower($file->getClientFilename()); + $filename = strtolower($clientFilename); $validExt = ($type === 'audio') ? ['mp3', 'm4a', 'ogg', 'wav'] : ['pdf', 'epub']; $ext = pathinfo($filename, PATHINFO_EXTENSION); if (!in_array($ext, $validExt, true)) { return $this->json($response, ['success' => false, 'message' => __('Formato file non supportato.')], 400); } @@ - $safeName = $this->generateSafeFilename($file->getClientFilename(), $type); + $safeName = $this->generateSafeFilename($clientFilename, $type);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/digital-library/DigitalLibraryPlugin.php` around lines 410 - 440, Guard against null/empty client filenames before calling strtolower() and generateSafeFilename(): retrieve $file->getClientFilename() into a variable, check if it's null or an empty string, and if so return a 400 JSON response (same shape as other errors) indicating missing filename; otherwise call strtolower() and pass the non-null string into generateSafeFilename($original, $type). Ensure the check prevents passing null to generateSafeFilename() and avoids calling strtolower(null).
♻️ Duplicate comments (6)
package.json (1)
7-7: Ensure the Playwright config path exists.The test script points to
tests/playwright.config.js; verify the file exists or adjust the path to the actual config file.#!/bin/bash # Verify the Playwright config path used by the test script. if [ -f tests/playwright.config.js ]; then echo "Found tests/playwright.config.js" else echo "Missing tests/playwright.config.js" fi # List any Playwright config files to confirm the correct path. fd -t f 'playwright.config.*'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 7, The "test" script in package.json references "tests/playwright.config.js" but that file may not exist; either create the missing Playwright config at tests/playwright.config.js or update the "test" script value to point to the actual config file location (e.g., playwright.config.js or tests/playwright.config.ts). Locate the "test" script entry in package.json to change the path, or add the config file under the tests/ directory with the expected export so Playwright can load it when running "playwright test --config=tests/playwright.config.js".app/Models/BookRepository.php (1)
985-997:⚠️ Potential issue | 🟠 MajorGuard against
prepare()failure before binding.If
prepare()returns false, the subsequentbind_param()andclose()will error. Add a guard and early return/log to avoid fatal failures.🛠️ Suggested fix
- $stmt = $this->db->prepare('SELECT id, nome, parent_id FROM generi WHERE id = ?'); + $stmt = $this->db->prepare('SELECT id, nome, parent_id FROM generi WHERE id = ?'); + if (!$stmt) { + $this->logDebug('resolveGenreHierarchy.prepare_failed', ['error' => $this->db->error]); + return; + } while ($currentId > 0 && $maxDepth-- > 0) { $stmt->bind_param('i', $currentId); $stmt->execute(); $genre = $stmt->get_result()->fetch_assoc(); if (!$genre) { break; } array_unshift($chain, $genre); // prepend: root first $currentId = (int)($genre['parent_id'] ?? 0); } $stmt->close();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/BookRepository.php` around lines 985 - 997, In BookRepository.php around the code that calls $this->db->prepare(...) assign the result to $stmt and guard against a false return before calling $stmt->bind_param(), ->execute(), ->get_result() or ->close(); if prepare() returns false, log or handle the error and return/continue early (do not call methods on $stmt), and only call $stmt->close() when $stmt is a valid mysqli_stmt object; update the loop that uses $stmt (and the surrounding function) to respect this guard so you avoid fatal errors when prepare fails.app/Controllers/GeneriController.php (1)
142-142:⚠️ Potential issue | 🟡 MinorAddress unused
$requestparameter to satisfy PHPMD.
Rename to$_requestor remove if not required by the framework signature.🧹 Suggested tweak
- public function destroy(Request $request, Response $response, \mysqli $db, int $id): Response + public function destroy(Request $_request, Response $response, \mysqli $db, int $id): Response🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/GeneriController.php` at line 142, The destroy method's signature contains an unused $request parameter which triggers PHPMD; update the signature of public function destroy(Request $request, Response $response, \mysqli $db, int $id): Response by renaming $request to $_request (or removing it if framework allows) so the parameter is clearly marked as intentionally unused and PHPMD warnings are resolved; ensure any internal references (if any) are updated to the new name and keep function name destroy unchanged.app/Models/GenereRepository.php (3)
219-231:⚠️ Potential issue | 🟡 MinorCheck the rename UPDATE result inside merge conflict resolution.
If the rename fails, the merge continues and can later hit unique constraints with a confusing error path. Throw on failure for consistency with the DELETE.
🧯 Suggested check
$stmt = $this->db->prepare("UPDATE generi SET nome = ? WHERE id = ?"); $stmt->bind_param('si', $newName, $child['id']); - $stmt->execute(); + if (!$stmt->execute()) { + throw new \RuntimeException('Impossibile rinominare il sottogenere in conflitto: ' . $child['nome']); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/GenereRepository.php` around lines 219 - 231, When renaming colliding children in the merge (loop over $sourceChildren, building $newName and preparing $stmt), check the result of $stmt->execute() and throw/raise an exception (or handle as the DELETE path does) if it returns false; ensure you also check $stmt->affected_rows or $this->db->errno to detect failures and abort the merge to avoid later unique-constraint errors, matching the existing error-handling style used elsewhere in this class.
162-171:⚠️ Potential issue | 🟡 MinorGuard against
query()failure ingetAllFlat().
$this->db->query()can returnfalse, andfetch_assoc()onfalsewill fatal. Add a guard before the fetch loop.✅ Defensive check
$result = $this->db->query(" SELECT g.id, g.nome, g.parent_id, p.nome AS parent_nome FROM generi g LEFT JOIN generi p ON g.parent_id = p.id ORDER BY COALESCE(p.nome, g.nome), g.parent_id IS NOT NULL, g.nome "); + if (!$result instanceof \mysqli_result) { + return []; + } + $rows = []; while ($row = $result->fetch_assoc()) { $rows[] = $row; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/GenereRepository.php` around lines 162 - 171, In getAllFlat(), guard against $this->db->query() returning false before calling $result->fetch_assoc(): after calling $this->db->query(...) (the $result variable), check if $result === false and handle that case (e.g., log/throw or return an empty array) so the while ($row = $result->fetch_assoc()) loop cannot call fetch_assoc on a boolean; update getAllFlat() to perform this defensive check and early return/exception.
123-131:⚠️ Potential issue | 🟠 Major
delete()ignores soft-deleted books, leaving dangling references.Allowing deletion when only soft-deleted rows reference the genre can break restore flows or FK constraints. Consider checking all rows or nullifying references on soft-deleted rows.
🧹 Possible fix (block delete if any book references the genre)
- $stmt = $this->db->prepare("SELECT COUNT(*) as cnt FROM libri WHERE (genere_id = ? OR sottogenere_id = ?) AND deleted_at IS NULL"); + $stmt = $this->db->prepare("SELECT COUNT(*) as cnt FROM libri WHERE (genere_id = ? OR sottogenere_id = ?)"); $stmt->bind_param('ii', $id, $id);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/GenereRepository.php` around lines 123 - 131, The delete() logic in GenereRepository currently ignores soft-deleted books by filtering with "deleted_at IS NULL", which allows deleting a genre while soft-deleted rows still reference it; update the check in the method that prepares the libri COUNT query to consider all rows (remove the deleted_at IS NULL condition) so any book row—soft-deleted or not—blocks deletion, or alternatively perform a cleanup step that nullifies genere_id/sottogenere_id on soft-deleted libri before deleting; locate the query prepared in GenereRepository::delete (the prepared statement using SELECT COUNT(*) as cnt FROM libri ...) and either remove the deleted_at filter or add logic to nullify references on soft-deleted rows prior to removing the genre.
🧹 Nitpick comments (1)
storage/plugins/api-book-scraper/plugin.json (1)
1-24: Consider revisitingmax_app_versionacross all plugin manifests.All five updated plugins still declare
"max_app_version": "1.0.0". As new features land and the app version advances, plugins will be flagged as incompatible before they actually break. Since this PR already touches each manifest, it's a good opportunity to either raise the cap to a higher sentinel (e.g.,"2.0.0") or adopt a convention of maintaining it proactively alongside each version bump.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/api-book-scraper/plugin.json` around lines 1 - 24, The plugin manifest's max_app_version field ("max_app_version": "1.0.0") is too low and will cause false incompatibility flags; update the "max_app_version" value in this plugin.json (and other plugin manifests touched in the PR) to a higher sentinel such as "2.0.0" or to whatever project convention you adopt so plugins aren't prematurely marked incompatible—look for the "max_app_version" property in each manifest (e.g., in storage/plugins/api-book-scraper/plugin.json) and bump it consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Models/GenereRepository.php`:
- Around line 195-208: The ancestor-check in GenereRepository.php currently
stops after a fixed depth (depth = 20) which can miss deep hierarchies; change
the loop in the ancestor-check that uses $aStmt, $ancestorId and $sourceId to
iterate until parent_id is NULL (or 0) rather than a fixed counter, and maintain
a visited set (e.g., an array or hash of visited ancestor IDs) to detect cycles
and abort if an ID repeats; also ensure $aStmt is properly closed in all exit
paths and that you bind/execute/get_result for each ancestor lookup as currently
done.
- Around line 97-106: In update() when 'parent_id' is provided you must guard
against setting parent_id to the record itself or any of its descendants
(reusing the merge-style ancestor check); before preparing the UPDATE, if
$parent_id is not null check $parent_id !== $id and traverse the ancestor chain
for $parent_id (e.g. call the same helper used by merge(), or a
getAncestors/getAncestorIds routine) and ensure $id is not in that ancestor list
— if it is, throw/return a validation error and do not run the UPDATE; keep
allowing null for top-level parents.
- Around line 97-102: The update() path currently uses bind_param('sii', $nome,
$parent_id, $id) which coerces a null parent_id to 0; replicate the create()
handling by normalizing $parent_id with $parent_id = !empty($data['parent_id'])
? (int)$data['parent_id'] : null; then branch the SQL: if $parent_id is null
prepare "UPDATE generi SET nome = ?, parent_id = NULL WHERE id = ?" and
bind_param('si', $nome, $id), otherwise prepare "UPDATE generi SET nome = ?,
parent_id = ? WHERE id = ?" and bind_param('sii', $nome, $parent_id, $id);
update the code in GenereRepository::update() (variables: $parent_id, $stmt,
bind_param) accordingly.
---
Outside diff comments:
In `@storage/plugins/digital-library/DigitalLibraryPlugin.php`:
- Around line 410-440: Guard against null/empty client filenames before calling
strtolower() and generateSafeFilename(): retrieve $file->getClientFilename()
into a variable, check if it's null or an empty string, and if so return a 400
JSON response (same shape as other errors) indicating missing filename;
otherwise call strtolower() and pass the non-null string into
generateSafeFilename($original, $type). Ensure the check prevents passing null
to generateSafeFilename() and avoids calling strtolower(null).
---
Duplicate comments:
In `@app/Controllers/GeneriController.php`:
- Line 142: The destroy method's signature contains an unused $request parameter
which triggers PHPMD; update the signature of public function destroy(Request
$request, Response $response, \mysqli $db, int $id): Response by renaming
$request to $_request (or removing it if framework allows) so the parameter is
clearly marked as intentionally unused and PHPMD warnings are resolved; ensure
any internal references (if any) are updated to the new name and keep function
name destroy unchanged.
In `@app/Models/BookRepository.php`:
- Around line 985-997: In BookRepository.php around the code that calls
$this->db->prepare(...) assign the result to $stmt and guard against a false
return before calling $stmt->bind_param(), ->execute(), ->get_result() or
->close(); if prepare() returns false, log or handle the error and
return/continue early (do not call methods on $stmt), and only call
$stmt->close() when $stmt is a valid mysqli_stmt object; update the loop that
uses $stmt (and the surrounding function) to respect this guard so you avoid
fatal errors when prepare fails.
In `@app/Models/GenereRepository.php`:
- Around line 219-231: When renaming colliding children in the merge (loop over
$sourceChildren, building $newName and preparing $stmt), check the result of
$stmt->execute() and throw/raise an exception (or handle as the DELETE path
does) if it returns false; ensure you also check $stmt->affected_rows or
$this->db->errno to detect failures and abort the merge to avoid later
unique-constraint errors, matching the existing error-handling style used
elsewhere in this class.
- Around line 162-171: In getAllFlat(), guard against $this->db->query()
returning false before calling $result->fetch_assoc(): after calling
$this->db->query(...) (the $result variable), check if $result === false and
handle that case (e.g., log/throw or return an empty array) so the while ($row =
$result->fetch_assoc()) loop cannot call fetch_assoc on a boolean; update
getAllFlat() to perform this defensive check and early return/exception.
- Around line 123-131: The delete() logic in GenereRepository currently ignores
soft-deleted books by filtering with "deleted_at IS NULL", which allows deleting
a genre while soft-deleted rows still reference it; update the check in the
method that prepares the libri COUNT query to consider all rows (remove the
deleted_at IS NULL condition) so any book row—soft-deleted or not—blocks
deletion, or alternatively perform a cleanup step that nullifies
genere_id/sottogenere_id on soft-deleted libri before deleting; locate the query
prepared in GenereRepository::delete (the prepared statement using SELECT
COUNT(*) as cnt FROM libri ...) and either remove the deleted_at filter or add
logic to nullify references on soft-deleted rows prior to removing the genre.
In `@package.json`:
- Line 7: The "test" script in package.json references
"tests/playwright.config.js" but that file may not exist; either create the
missing Playwright config at tests/playwright.config.js or update the "test"
script value to point to the actual config file location (e.g.,
playwright.config.js or tests/playwright.config.ts). Locate the "test" script
entry in package.json to change the path, or add the config file under the
tests/ directory with the expected export so Playwright can load it when running
"playwright test --config=tests/playwright.config.js".
---
Nitpick comments:
In `@storage/plugins/api-book-scraper/plugin.json`:
- Around line 1-24: The plugin manifest's max_app_version field
("max_app_version": "1.0.0") is too low and will cause false incompatibility
flags; update the "max_app_version" value in this plugin.json (and other plugin
manifests touched in the PR) to a higher sentinel such as "2.0.0" or to whatever
project convention you adopt so plugins aren't prematurely marked
incompatible—look for the "max_app_version" property in each manifest (e.g., in
storage/plugins/api-book-scraper/plugin.json) and bump it consistently.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
app/Controllers/GeneriApiController.phpapp/Controllers/GeneriController.phpapp/Controllers/LibriApiController.phpapp/Models/BookRepository.phpapp/Models/GenereRepository.phpapp/Routes/web.phpapp/Views/generi/dettaglio_genere.phpapp/Views/libri/index.phpapp/Views/libri/partials/book_form.phppackage.jsonpublic/.htaccessstorage/plugins/api-book-scraper/plugin.jsonstorage/plugins/dewey-editor/plugin.jsonstorage/plugins/digital-library/DigitalLibraryPlugin.phpstorage/plugins/digital-library/plugin.jsonstorage/plugins/open-library/plugin.jsonstorage/plugins/z39-server/plugin.json
- GenereRepository::update(): branch SQL for NULL parent_id (was coercing to 0) - GenereRepository::update(): add self/descendant cycle guard with visited set - GenereRepository::merge(): replace fixed depth=20 with visited set - GenereRepository::merge(): check rename execute() result - GenereRepository::getAllFlat(): guard query() returning false - GenereRepository::delete(): check ALL books including soft-deleted (dangling FK) - BookRepository::resolveGenreHierarchy: guard prepare() failure - DigitalLibraryPlugin: null/empty client filename guard before strtolower()
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
storage/plugins/digital-library/DigitalLibraryPlugin.php (1)
467-497:⚠️ Potential issue | 🟡 MinorHandle read failures in
serveAssetto avoid caching empty responses.
Iffile_get_contents()fails, the current code still returns 200 with a long cache header, which can cache an empty asset. Consider short‑circuiting with 404/500 on read failure.🩹 Suggested fix
- $response->getBody()->write((string)file_get_contents($filePath)); + $contents = file_get_contents($filePath); + if ($contents === false) { + return $response->withStatus(404); + } + $response->getBody()->write($contents);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/digital-library/DigitalLibraryPlugin.php` around lines 467 - 497, serveAsset currently writes file_get_contents($filePath) directly and will return 200 with a long cache header even if the read fails; change it to read into a variable (e.g. $contents = file_get_contents($filePath)), check if $contents === false and short‑circuit returning a 500 (or 404 if you prefer) without the long Cache-Control header, and only write to the response body and add the long max-age Cache-Control when the read succeeded; reference serveAsset, $filePath, file_get_contents and $response to locate and implement this check.
♻️ Duplicate comments (2)
package.json (1)
7-7: Missing Playwright config file was already flagged.The
tests/playwright.config.jsreferenced by the test script must exist fornpm testto succeed. This was flagged in a prior review.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 7, The package.json "test" script references a missing Playwright config file which prevents npm test from running; add the missing config or update the script: either create the referenced tests/playwright.config.js with the project's Playwright configuration (browsers, testDir, reporter, etc.) or change the "test" npm script (the "test" property in package.json) to point to an existing config path or to use the default Playwright config; ensure tests/playwright.config.js is checked into the repo if created so CI and local npm test succeed.app/Models/BookRepository.php (1)
985-999: N+1 query concern was previously flagged.The resolver still issues up to 5 individual
SELECTqueries pergetById()call, even though the main query already JOINsgenerithree levels deep. The chain for ≤3 levels could be built from$rowdata (genere_nome,radice_id,radice_nome,nonno_id,nonno_nome) with zero extra queries, falling back to the loop only for L4+ depths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/BookRepository.php` around lines 985 - 999, The loop in BookRepository::getById still issues up to 5 separate prepares/executions against $this->db ($stmt, bind_param, execute) to build the genre chain; instead, first assemble up to three levels from the already-fetched $row fields (genere_nome, radice_id, radice_nome, nonno_id, nonno_nome) into $chain (root → child) with zero extra queries, and only fall back to the existing while loop using $stmt/$currentId/$maxDepth when the requested depth exceeds those 3 levels or when a parent id remains unresolved; ensure you still call $stmt->close() only if you prepared it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Controllers/GeneriController.php`:
- Around line 87-100: The update() and merge() methods assume
$request->getParsedBody() returns an array and then do $data['nome'], which can
emit warnings when getParsedBody() is null; change the data assignment to
default to an empty array (e.g., $data = $request->getParsedBody() ?? []; or
cast: $data = (array)$request->getParsedBody();) so subsequent accesses like
$data['nome'] are safe; apply the same change in both update() and merge() to
prevent null parsed-body warnings.
In `@app/Models/GenereRepository.php`:
- Around line 283-287: When reparenting in GenereRepository.php the code uses
$sourceParent = $source['parent_id'] !== null ? (int)$source['parent_id'] : null
and then binds it with 'i', which coerces NULL to 0; change the logic to mirror
update(): if $source['parent_id'] === null prepare the UPDATE to set "parent_id
= NULL WHERE id = ? AND parent_id = ?" (no bound parent param), otherwise
prepare "parent_id = ?" and bind (int)$source['parent_id'] with 'i'; adjust the
$stmt preparation and bind_param calls around $sourceParent/$targetId/$sourceId
so NULL parents are stored as SQL NULL instead of 0.
- Line 107: Guard the prepare() calls in GenereRepository: check the result of
$this->db->prepare('SELECT parent_id FROM generi WHERE id = ?') (the $aStmt
variable used in the ancestor traversal) and the prepare() used in merge() so
you don't call bind_param() on false; follow the same pattern used in
BookRepository::resolveGenreHierarchy (lines referenced) — detect if $aStmt (or
the merge stmt) is false, log or handle the error and return/throw appropriately
instead of continuing to call bind_param()/execute().
In `@app/Views/libri/index.php`:
- Around line 639-645: The payload always sends the initial collanaFilter (set
from the URL) and never reads the current UI value; update the payload
construction to read the current collana input instead of using the stale
collanaFilter (e.g., replace collana: collanaFilter with collana:
document.getElementById('collana_id').value or ensure collanaFilter is updated
in the autocomplete/input change handler), keeping the variable name
collanaFilter or input ID you actually use in the view so the AJAX uses the live
user-selected value.
In `@public/.htaccess`:
- Around line 14-19: Remove the dead Apache config block that uses the incorrect
module identifier "<IfModule mod_php8.c>" — the block containing php_value
upload_max_filesize/post_max_size/max_execution_time/max_input_time — because
PHP 8 uses the unversioned module name; delete that entire <IfModule mod_php8.c>
... </IfModule> section so only the correct "<IfModule mod_php.c>" block
remains.
---
Outside diff comments:
In `@storage/plugins/digital-library/DigitalLibraryPlugin.php`:
- Around line 467-497: serveAsset currently writes file_get_contents($filePath)
directly and will return 200 with a long cache header even if the read fails;
change it to read into a variable (e.g. $contents =
file_get_contents($filePath)), check if $contents === false and short‑circuit
returning a 500 (or 404 if you prefer) without the long Cache-Control header,
and only write to the response body and add the long max-age Cache-Control when
the read succeeded; reference serveAsset, $filePath, file_get_contents and
$response to locate and implement this check.
---
Duplicate comments:
In `@app/Models/BookRepository.php`:
- Around line 985-999: The loop in BookRepository::getById still issues up to 5
separate prepares/executions against $this->db ($stmt, bind_param, execute) to
build the genre chain; instead, first assemble up to three levels from the
already-fetched $row fields (genere_nome, radice_id, radice_nome, nonno_id,
nonno_nome) into $chain (root → child) with zero extra queries, and only fall
back to the existing while loop using $stmt/$currentId/$maxDepth when the
requested depth exceeds those 3 levels or when a parent id remains unresolved;
ensure you still call $stmt->close() only if you prepared it.
In `@package.json`:
- Line 7: The package.json "test" script references a missing Playwright config
file which prevents npm test from running; add the missing config or update the
script: either create the referenced tests/playwright.config.js with the
project's Playwright configuration (browsers, testDir, reporter, etc.) or change
the "test" npm script (the "test" property in package.json) to point to an
existing config path or to use the default Playwright config; ensure
tests/playwright.config.js is checked into the repo if created so CI and local
npm test succeed.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
app/Controllers/GeneriApiController.phpapp/Controllers/GeneriController.phpapp/Controllers/LibriApiController.phpapp/Models/BookRepository.phpapp/Models/GenereRepository.phpapp/Routes/web.phpapp/Views/generi/dettaglio_genere.phpapp/Views/libri/index.phpapp/Views/libri/partials/book_form.phppackage.jsonpublic/.htaccessstorage/plugins/api-book-scraper/plugin.jsonstorage/plugins/dewey-editor/plugin.jsonstorage/plugins/digital-library/DigitalLibraryPlugin.phpstorage/plugins/digital-library/plugin.jsonstorage/plugins/open-library/plugin.jsonstorage/plugins/z39-server/plugin.json
Comment on lines
+639
to
645
| genere_filter: document.getElementById('genere_id').value || genereUrlFilter || 0, | ||
| sottogenere_filter: sottogenereUrlFilter || 0, | ||
| posizione_id: document.getElementById('posizione_id').value || 0, | ||
| anno_from: document.getElementById('anno_from').value, | ||
| anno_to: document.getElementById('anno_to').value | ||
| anno_to: document.getElementById('anno_to').value, | ||
| collana: collanaFilter | ||
| }; |
There was a problem hiding this comment.
Collana filter is frozen to the initial URL value.
The AJAX payload always uses collanaFilter, which is only initialized from the URL and never updated from a UI input. If a collana autocomplete/input exists, user changes won’t affect results. Consider reading the input value (or updating collanaFilter in its handlers).
🛠️ Possible fix (adjust element ID to your actual collana input)
data: function(d) {
+ const collanaInput = document.getElementById('filter_collana');
return {
...d,
@@
- collana: collanaFilter
+ collana: (collanaInput ? collanaInput.value : collanaFilter) || ''
};
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Views/libri/index.php` around lines 639 - 645, The payload always sends
the initial collanaFilter (set from the URL) and never reads the current UI
value; update the payload construction to read the current collana input instead
of using the stale collanaFilter (e.g., replace collana: collanaFilter with
collana: document.getElementById('collana_id').value or ensure collanaFilter is
updated in the autocomplete/input change handler), keeping the variable name
collanaFilter or input ID you actually use in the view so the AJAX uses the live
user-selected value.
- GeneriController: guard getParsedBody() ?? [] in store/update/merge - GenereRepository: guard prepare() in update() and merge() ancestor walks - GenereRepository::merge(): branch SQL for NULL sourceParent (was coercing to 0) - Remove dead mod_php8.c block from .htaccess (PHP 8 uses mod_php.c) - DigitalLibraryPlugin::serveAsset: guard file_get_contents() failure
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
storage/plugins/digital-library/DigitalLibraryPlugin.php (1)
193-218:⚠️ Potential issue | 🟡 Minor
prepare()result not guarded againstfalse— inconsistent withonDeactivate().Both the
SELECT(line 193) andINSERT(line 204) prepared statements can returnfalseon error (e.g., table missing, SQL syntax issue). Without a guard,->bind_param()will cause a fatal error.onDeactivate()already contains theif ($stmt === false) { return; }pattern.🛡️ Proposed fix — add `false` guards in the loop
$stmt = $this->db->prepare(" SELECT id FROM plugin_hooks WHERE plugin_id = ? AND hook_name = ? "); + if ($stmt === false) { + continue; + } $stmt->bind_param("is", $this->pluginId, $hook['hook_name']); $stmt->execute(); $result = $stmt->get_result(); if ($result->num_rows === 0) { $stmt->close(); // close SELECT stmt before reassignment $stmt = $this->db->prepare(" INSERT INTO plugin_hooks (plugin_id, hook_name, callback_class, callback_method, priority, is_active) VALUES (?, ?, ?, ?, ?, ?) "); + if ($stmt === false) { + continue; + } $stmt->bind_param(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/digital-library/DigitalLibraryPlugin.php` around lines 193 - 218, The SELECT and INSERT calls using $this->db->prepare(...) can return false and are not checked, so add guards after each prepare call (for the SELECT and the INSERT) to handle a false $stmt: if $stmt === false then either return or continue the loop consistently with the pattern used in onDeactivate() to avoid calling ->bind_param() on false; ensure you still close any prior statement before reassignment and keep references to $stmt, prepare, bind_param, and the plugin_hooks INSERT/SELECT logic when adding the checks.
♻️ Duplicate comments (3)
package.json (1)
7-7: Confirm the Playwright config file path is valid.The test script references
tests/playwright.config.js; if that file doesn’t exist,npm testwill fail. Please verify the config path or update the script accordingly.#!/bin/bash # Locate Playwright config files to confirm the referenced path exists. fd -t f -i 'playwright.config' --full-path🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 7, The package.json "test" script references tests/playwright.config.js which may not exist; locate the actual Playwright config (search for files named playwright.config.* or playwright.config in the repo) and then either create/move the config to tests/playwright.config.js or update the "test" script to point to the correct path (e.g., change the value of the "test" script in package.json to the actual config path); ensure the referenced symbol is the "test" script in package.json and the target path is tests/playwright.config.js (or the real file you find).app/Views/libri/index.php (1)
439-440:⚠️ Potential issue | 🟡 MinorCollana filter has no UI input — the autocomplete advertised in the PR is missing.
collanaFilteris initialised from the URL and can only be cleared (banner close / "clear all"). There is nofilter_collanaHTML input, nosetupAutocomplete('filter_collana', …)call, and no collana chip inupdateActiveFilters(). The PR description states "added collana (series) filter with autocomplete in the admin book list", but the interactive part is absent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/libri/index.php` around lines 439 - 440, The collana (series) filter is only read from URL into initialCollana/collanaFilter but the UI/input and wiring are missing; add a HTML input with id "filter_collana", initialize it and call setupAutocomplete('filter_collana', …) to populate suggestions, update event handlers to set collanaFilter when a value is chosen, add a "collana" chip in updateActiveFilters() so it appears in the active filters UI, and ensure clearing the banner/clear-all also clears the input and syncs the URL param and collanaFilter state.app/Controllers/GeneriController.php (1)
142-142:⚠️ Potential issue | 🟡 Minor
$requestis still unused — PHPMD still warns.The parameter is present but never referenced in the method body. The past review comment flagging this was marked ✅ addressed, but the current code still carries it.
If the framework requires the parameter in the signature (e.g., Slim route dispatch), rename it to
$_requestto suppress the tooling warning without breaking the contract.🔧 Proposed fix
- public function destroy(Request $request, Response $response, \mysqli $db, int $id): Response + public function destroy(Request $_request, Response $response, \mysqli $db, int $id): Response🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/GeneriController.php` at line 142, The destroy method's signature declares $request but it's unused and PHPMD still warns; to suppress the warning without changing the contract, rename the parameter in the destroy function signature from $request to $_request (keeping the Request type hint) so tooling treats it as intentionally unused, and verify there are no internal references to $request in the destroy method (function destroy(Request $_request, Response $response, \mysqli $db, int $id): Response).
🧹 Nitpick comments (3)
app/Views/libri/partials/book_form.php (1)
2201-2204: Subgenre pre-population lacks the parallel existence check added for genres.The
initialSottogenereblock unconditionally setssottogenereApplied = truewithout verifying the option was actually accepted — mirroring the pre-fix behaviour for genres. If the subgenre isn't in the loaded list (e.g., after a cascade mismatch), the value silently stays at0with no diagnostic.♻️ Proposed symmetry fix
if (!sottogenereApplied && initialSottogenere > 0) { sottogenereSelect.value = String(initialSottogenere); sottogenereApplied = true; + if (parseInt(sottogenereSelect.value, 10) !== initialSottogenere) { + console.warn('Subgenre pre-population: initialSottogenere', initialSottogenere, 'not found in children of genre', parentId); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/libri/partials/book_form.php` around lines 2201 - 2204, The pre-population for subgenre unconditionally marks sottogenereApplied true; change it to first verify the option exists in sottogenereSelect before applying: when initialSottogenere > 0, look for an option with value === String(initialSottogenere) (or check sottogenereSelect.querySelector/option list) and only set sottogenereSelect.value and sottogenereApplied = true if that option is found; otherwise leave sottogenereApplied false (and optionally handle or log the missing option) so behavior mirrors the genre check.storage/plugins/digital-library/DigitalLibraryPlugin.php (2)
428-432: Empty MIME-check block is dead code — remove or replace with a log.The
if (!in_array(...))body is intentionally empty (documented by comment), making this block a no-op. It misleads readers into thinking validation occurs when it doesn't. Either drop the block entirely or emit a debug-level log so MIME mismatches are observable.♻️ Option A — remove dead block
- $clientMediaType = $file->getClientMediaType(); - if (!in_array($clientMediaType, $allowedMime, true)) { - // Extension already validated above — allow if extension is safe - // (browsers may report wrong MIME for valid files) - }♻️ Option B — keep block with a debug log
$clientMediaType = $file->getClientMediaType(); if (!in_array($clientMediaType, $allowedMime, true)) { - // Extension already validated above — allow if extension is safe - // (browsers may report wrong MIME for valid files) + \App\Support\SecureLogger::debug('[Digital Library] MIME mismatch (proceeding via extension)', [ + 'reported_mime' => $clientMediaType, + 'extension' => $ext, + 'type' => $type, + ]); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/digital-library/DigitalLibraryPlugin.php` around lines 428 - 432, The empty MIME-check if blocking ($clientMediaType vs $allowedMime) is dead code; either remove the entire if or replace its body with a debug log so mismatches are observable. Update the block around $clientMediaType and $allowedMime in DigitalLibraryPlugin (the method containing that diff) to call the plugin logger (e.g. $this->logger->debug(...)) with a concise message including $clientMediaType and the allowed list when in_array returns false, or simply delete the if and its comment to avoid the misleading no-op.
498-500: One-yearCache-ControlwithoutETag/Last-Modifiedmeans plugin CSS/JS updates won't reach cached browsers.After a plugin version bump the served file content changes, but cached clients have no way to detect it — no conditional request headers are possible. Adding
Last-Modified(and optionallyETag) allows revalidation and fixes this without shortening the cache TTL.♻️ Proposed fix — add `Last-Modified` and `ETag`
$mime = $type === 'css' ? 'text/css; charset=UTF-8' : 'application/javascript; charset=UTF-8'; $contents = file_get_contents($filePath); if ($contents === false) { return $response->withStatus(500); } + $mtime = filemtime($filePath); $response->getBody()->write($contents); return $response ->withHeader('Content-Type', $mime) - ->withHeader('Cache-Control', 'public, max-age=31536000'); + ->withHeader('Cache-Control', 'public, max-age=31536000, immutable') + ->withHeader('Last-Modified', gmdate('D, d M Y H:i:s', (int)$mtime) . ' GMT') + ->withHeader('ETag', '"' . md5($mtime . $filePath) . '"');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/digital-library/DigitalLibraryPlugin.php` around lines 498 - 500, The response sets a long Cache-Control but lacks revalidation headers; update the DigitalLibraryPlugin response to add Last-Modified (using the served file's filemtime) and an ETag (e.g., hash of filesize+filemtime or file contents) alongside the existing Cache-Control header, and implement conditional request handling: compare If-Modified-Since and If-None-Match from the request to the computed Last-Modified/ETag and return a 304 Not Modified when they match; modify the code around the return $response->withHeader('Content-Type', $mime)->withHeader('Cache-Control', ...) to compute and set Last-Modified and ETag headers and to short-circuit with a 304 response when appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Controllers/GeneriApiController.php`:
- Around line 136-151: The cycle-check loop assumes $db->prepare(...) returns a
valid statement ($aStmt) but prepare can return false; update the block that
creates $aStmt (the SELECT parent_id FROM generi loop) to check if $aStmt ===
false, log or write a JSON error and return a 500 (or appropriate) response
before entering the while loop, and avoid calling
$aStmt->bind_param()/execute()/close() on a non-object; ensure any early returns
close or free resources as needed so the code uses $aStmt only when it is a
valid mysqli_stmt.
In `@app/Controllers/GeneriController.php`:
- Around line 109-126: The controller's cycle-check uses $aStmt =
$db->prepare(...) without guarding for false, so subsequent $aStmt->bind_param()
can fatal; check if $aStmt === false and handle that failure (set
$_SESSION['error_message' ] or log the DB error and return the same redirect as
the user-facing cycle message) before using $aStmt, then close/free it when
done; alternatively remove this duplicated block and let
GenereRepository::update() perform cycle detection (it uses a safe $seen set)
and adjust the catch to distinguish \InvalidArgumentException for cycles versus
other DB errors to preserve the specific user-facing message.
In `@app/Models/GenereRepository.php`:
- Around line 284-313: The transaction in GenereRepository.php performs multiple
$stmt->execute() calls (moving children, reparenting target, updating libri
rows) but does not check their boolean return values, allowing silent failures
to lead to partial commits; update the code (in the method handling the merge)
to check the result of every $stmt->execute() and on any false call
$this->db->rollback() and throw an exception (include $this->db->error or
$stmt->error in the exception message), ensure statements are closed (or freed)
after use, and only call $this->db->commit() if all execute() calls succeeded;
also keep the existing catch block to rollback on thrown exceptions so the
transaction never commits on partial failure.
In `@app/Views/libri/partials/book_form.php`:
- Around line 31-32: The view uses the null-coalescing operator on 'genere_id'
and 'sottogenere_id' which fails when BookRepository sets
'genere_id_cascade'/'sottogenere_id_cascade' to 0; either update BookRepository
to initialize those cascade fields to null instead of 0 (change
'genere_id_cascade' and 'sottogenere_id_cascade' assignments in BookRepository)
or change the view logic in book_form.php to use explicit presence checks (e.g.,
array_key_exists or isset combined with strict !== null) for 'genere_id_cascade'
and 'sottogenere_id_cascade' before falling back to 'genere_id'/'sottogenere_id'
so a zero cascade does not shadow a valid genre id.
---
Outside diff comments:
In `@storage/plugins/digital-library/DigitalLibraryPlugin.php`:
- Around line 193-218: The SELECT and INSERT calls using $this->db->prepare(...)
can return false and are not checked, so add guards after each prepare call (for
the SELECT and the INSERT) to handle a false $stmt: if $stmt === false then
either return or continue the loop consistently with the pattern used in
onDeactivate() to avoid calling ->bind_param() on false; ensure you still close
any prior statement before reassignment and keep references to $stmt, prepare,
bind_param, and the plugin_hooks INSERT/SELECT logic when adding the checks.
---
Duplicate comments:
In `@app/Controllers/GeneriController.php`:
- Line 142: The destroy method's signature declares $request but it's unused and
PHPMD still warns; to suppress the warning without changing the contract, rename
the parameter in the destroy function signature from $request to $_request
(keeping the Request type hint) so tooling treats it as intentionally unused,
and verify there are no internal references to $request in the destroy method
(function destroy(Request $_request, Response $response, \mysqli $db, int $id):
Response).
In `@app/Views/libri/index.php`:
- Around line 439-440: The collana (series) filter is only read from URL into
initialCollana/collanaFilter but the UI/input and wiring are missing; add a HTML
input with id "filter_collana", initialize it and call
setupAutocomplete('filter_collana', …) to populate suggestions, update event
handlers to set collanaFilter when a value is chosen, add a "collana" chip in
updateActiveFilters() so it appears in the active filters UI, and ensure
clearing the banner/clear-all also clears the input and syncs the URL param and
collanaFilter state.
In `@package.json`:
- Line 7: The package.json "test" script references tests/playwright.config.js
which may not exist; locate the actual Playwright config (search for files named
playwright.config.* or playwright.config in the repo) and then either
create/move the config to tests/playwright.config.js or update the "test" script
to point to the correct path (e.g., change the value of the "test" script in
package.json to the actual config path); ensure the referenced symbol is the
"test" script in package.json and the target path is tests/playwright.config.js
(or the real file you find).
---
Nitpick comments:
In `@app/Views/libri/partials/book_form.php`:
- Around line 2201-2204: The pre-population for subgenre unconditionally marks
sottogenereApplied true; change it to first verify the option exists in
sottogenereSelect before applying: when initialSottogenere > 0, look for an
option with value === String(initialSottogenere) (or check
sottogenereSelect.querySelector/option list) and only set
sottogenereSelect.value and sottogenereApplied = true if that option is found;
otherwise leave sottogenereApplied false (and optionally handle or log the
missing option) so behavior mirrors the genre check.
In `@storage/plugins/digital-library/DigitalLibraryPlugin.php`:
- Around line 428-432: The empty MIME-check if blocking ($clientMediaType vs
$allowedMime) is dead code; either remove the entire if or replace its body with
a debug log so mismatches are observable. Update the block around
$clientMediaType and $allowedMime in DigitalLibraryPlugin (the method containing
that diff) to call the plugin logger (e.g. $this->logger->debug(...)) with a
concise message including $clientMediaType and the allowed list when in_array
returns false, or simply delete the if and its comment to avoid the misleading
no-op.
- Around line 498-500: The response sets a long Cache-Control but lacks
revalidation headers; update the DigitalLibraryPlugin response to add
Last-Modified (using the served file's filemtime) and an ETag (e.g., hash of
filesize+filemtime or file contents) alongside the existing Cache-Control
header, and implement conditional request handling: compare If-Modified-Since
and If-None-Match from the request to the computed Last-Modified/ETag and return
a 304 Not Modified when they match; modify the code around the return
$response->withHeader('Content-Type', $mime)->withHeader('Cache-Control', ...)
to compute and set Last-Modified and ETag headers and to short-circuit with a
304 response when appropriate.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
app/Controllers/GeneriApiController.phpapp/Controllers/GeneriController.phpapp/Controllers/LibriApiController.phpapp/Models/BookRepository.phpapp/Models/GenereRepository.phpapp/Routes/web.phpapp/Views/generi/dettaglio_genere.phpapp/Views/libri/index.phpapp/Views/libri/partials/book_form.phppackage.jsonpublic/.htaccessstorage/plugins/api-book-scraper/plugin.jsonstorage/plugins/dewey-editor/plugin.jsonstorage/plugins/digital-library/DigitalLibraryPlugin.phpstorage/plugins/digital-library/plugin.jsonstorage/plugins/open-library/plugin.jsonstorage/plugins/z39-server/plugin.json
…idation Security fixes across 29 files: - XSS: JSON_HEX_TAG on all json_encode in <script>, ENT_QUOTES on HTML attrs, innerHTML→textContent, escapeHtml on DataTables columns - Info disclosure: generic error messages instead of $e->getMessage(), SecureLogger::error() instead of error_log(), displayErrorDetails=false - Soft-delete: bulkDelete now uses UPDATE SET deleted_at + nullify unique cols, bulkStatus/updateBasic add AND deleted_at IS NULL - MIME: finfo_file() magic-byte validation in DigitalLibraryPlugin - Auth: manual-update.php uses env-based key with hash_equals() - Exception→Throwable in all catch blocks (strict_types compatibility) - Directory listing: Options -Indexes in .htaccess - .gitattributes: exclude dev files from release archives - Playwright tests: 29 security + 12 genre tests (45/45 pass)
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/Middleware/CsrfMiddleware.php (1)
13-63:⚠️ Potential issue | 🟠 MajorAvoid logging CSRF token fragments.
Even partial tokens can leak sensitive material if logs are exposed. Log the reason only.🔒 Proposed redaction
- SecureLogger::warning('[CSRF] Validation failed. Reason: ' . $csrfValidation['reason'] . ' Token provided: ' . (is_string($token) ? substr($token, 0, 8) . '...' : '[none]') . ' Session token: ' . (isset($_SESSION['csrf_token']) ? substr($_SESSION['csrf_token'], 0, 8) . '...' : '[none]')); + SecureLogger::warning('[CSRF] Validation failed. Reason: ' . $csrfValidation['reason']);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Middleware/CsrfMiddleware.php` around lines 13 - 63, The SecureLogger::warning call in CsrfMiddleware::process currently logs partial token values and session token; change the log to omit any token fragments and only include the validation reason (from Csrf::validateWithReason result) and safe contextual info (e.g., request path or method if needed), i.e. remove substr($_SERVER/$_SESSION token) and $token bits from the message and call SecureLogger::warning with a message like "[CSRF] Validation failed. Reason: " . $csrfValidation['reason']; keep Csrf::validateWithReason and SecureLogger::warning usage but stop including token material in the logged string.scripts/manual-update.php (1)
441-456:⚠️ Potential issue | 🔴 CriticalBug:
strpos($stmt, '--') === 0skips entire chunks that begin with a SQL comment, including real statements that follow the comment.After
explode(';', $sql), each element can contain a leading-- commentfollowed by actual SQL (e.g.,-- Add column\nALTER TABLE ...). Thestrpos($stmt, '--') === 0check will skip the entire chunk, silently dropping real migration statements.🐛 Proposed fix — strip leading comment lines instead of skipping the whole chunk
foreach ($statements as $stmt) { - if (strpos($stmt, '--') === 0) continue; + // Strip leading SQL comment lines + $stmt = preg_replace('/^\s*--.*$/m', '', $stmt); + $stmt = trim($stmt); + if ($stmt === '') continue; if (!$db->query($stmt)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/manual-update.php` around lines 441 - 456, The loop over $statements currently skips any chunk that begins with '--' (strpos($stmt, '--') === 0), which can discard valid SQL that follows a leading single-line comment; change the handling in the foreach ($statements as $stmt) block to strip leading SQL comment lines instead of skipping the whole chunk: trim leading whitespace, remove any leading lines starting with '--' (or blank lines) until the first real SQL line, then if the resulting $stmt is empty continue, otherwise pass it to $db->query and keep the existing error handling (errno capture and $ignorableErrors array) unchanged.storage/plugins/digital-library/DigitalLibraryPlugin.php (1)
383-465:⚠️ Potential issue | 🟠 MajorAdd missing MIME type variants to prevent rejection of valid EPUB and WAV files.
finfocommonly reports EPUB files asapplication/zip(due to the ZIP container signature) and WAV files asaudio/x-wavoraudio/wavedepending on the system's libmagic database version. Valid uploads will be rejected on these systems. The extension validation provides sufficient protection before MIME checking, so broadening the allowlist is safe.🔧 Suggested adjustment
$allowedMime = ($type === 'audio') - ? ['audio/mpeg', 'audio/mp4', 'audio/ogg', 'audio/x-m4a', 'audio/wav'] - : ['application/pdf', 'application/epub+zip']; + ? ['audio/mpeg', 'audio/mp4', 'audio/ogg', 'audio/x-m4a', 'audio/wav', 'audio/x-wav', 'audio/wave'] + : ['application/pdf', 'application/epub+zip', 'application/zip'];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/digital-library/DigitalLibraryPlugin.php` around lines 383 - 465, The server-side MIME allowlist in the upload handler rejects valid EPUB and WAV files because finfo can report EPUB as "application/zip" and WAV as "audio/x-wav" or "audio/wave"; update the $allowedMime array used before the finfo/file check in the same upload routine (the code around $allowedMime, $validExt, finfo->file, and the MIME validation block) to include 'application/zip' for EPUB and add 'audio/x-wav' and 'audio/wave' alongside existing WAV variants so valid uploads that passed extension validation (generateSafeFilename, $validExt) are not falsely rejected.
♻️ Duplicate comments (4)
package.json (1)
7-7: Missing Playwright config file — verify it was added.The previous review flagged that
tests/playwright.config.jsreferenced in the test script does not exist in the repository. Ensure this file has been created, otherwisenpm testwill fail.#!/bin/bash # Check if the Playwright config file exists fd -t f 'playwright.config' --full-path🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 7, The package.json "test" script references tests/playwright.config.js which doesn't exist; either add the missing Playwright config file at tests/playwright.config.js (exporting the Playwright configuration) or update the "test" script in package.json to point to an existing config or use the default (e.g., remove the --config flag); edit the file(s) that define the test script and/or create tests/playwright.config.js so running npm test/playwright test succeeds.app/Views/libri/index.php (1)
439-440:⚠️ Potential issue | 🟠 MajorCollana filter still tied only to URL state
collanaFilteris initialized from the URL and never updated from the UI, so user changes won’t affect results. Wire it to the collana input/autocomplete (or read the input in the AJAX payload) so the filter is live.💡 Possible fix (adjust input ID as needed)
data: function(d) { + const collanaInput = document.getElementById('filter_collana'); return { ...d, @@ - collana: collanaFilter + collana: (collanaInput ? collanaInput.value : collanaFilter) || '' }; },Also applies to: 639-645
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/libri/index.php` around lines 439 - 440, collanaFilter is only set once from initialCollana and never updated, so hook the UI input/autocomplete to keep it in sync: either add an input/change/autocomplete event listener on the collana input element (the same element referenced for autocomplete) that updates the variable collanaFilter, or stop relying on the global collanaFilter and instead read the current value of the collana input when constructing the AJAX payload (where search/filter params are assembled); apply the same fix for the other occurrences around the block referenced (lines that initialize collanaFilter elsewhere, e.g., the duplicate logic at 639-645) so UI changes actually affect requests.app/Models/BookRepository.php (1)
961-1045: Avoid re-queryinggeneriwhen JOIN data already exists.
The resolver issues extra SELECTs even thoughg/gp/gppare already joined in the main query. Consider building the chain from$rowfor L1–L3 and only querying for deeper levels.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/BookRepository.php` around lines 961 - 1045, The resolver currently always re-queries the generi table; update resolveGenreHierarchy to first build the L1–L3 chain from the already-joined fields in $row (use the joined fields/aliases produced by the main query — e.g. the g/gp/gpp name/id fields or existing keys like genere_id, genere_nome, radice_id/radice_nome, etc.) and map those to radice_id/radice_nome, genere_id_cascade/genere_nome and sottogenere_id_cascade/sottogenere_nome; only fall back to the DB SELECT logic (the prepared statement in resolveGenreHierarchy) when the chain cannot be resolved from $row (i.e. deeper than three levels or missing joined columns). Ensure you keep the existing mapping logic and safety limit but skip the SELECT/while loop when the 3-level data is present in $row.app/Controllers/GeneriController.php (1)
149-149:⚠️ Potential issue | 🟡 MinorUnused
$requestparameter triggers PHPMD warning.
Either remove it (if the framework allows) or rename to$_request.Possible fix
-public function destroy(Request $request, Response $response, \mysqli $db, int $id): Response +public function destroy($_request, Response $response, \mysqli $db, int $id): Response🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/GeneriController.php` at line 149, The destroy method signature currently includes an unused Request $request parameter which triggers PHPMD; update the method declaration for destroy(Request $request, Response $response, \mysqli $db, int $id): Response by either removing the $request parameter entirely if the framework allows or renaming it to $_request to mark it as intentionally unused, and then adjust any callers or type hints accordingly to keep signatures consistent.
🧹 Nitpick comments (2)
app/Views/partials/cookie-banner.php (1)
53-94: InconsistentJSON_HEX_APOS | JSON_HEX_QUOTflags — apply uniformly or drop from Line 94.Line 94 adds
JSON_HEX_APOS | JSON_HEX_QUOTto the flags, but all otherjson_encodecalls in the same<script>block (Lines 53–93) use onlyJSON_HEX_TAG | JSON_HEX_AMP. A URL returned byroute_path('cookies')is the least likely value here to contain quotes, while the configurable HTML text fields (banner_description,preferences_description, etc.) sourced fromConfigStoreare far more likely to.Either extend the extra flags to all calls for consistency:
♻️ Proposed refactor — uniform flags on all json_encode calls
- name: <?= json_encode($cookieBannerTexts['cookie_essential_name'], JSON_HEX_TAG | JSON_HEX_AMP) ?>, - description: <?= json_encode($cookieBannerTexts['cookie_essential_description'], JSON_HEX_TAG | JSON_HEX_AMP) ?>, + name: <?= json_encode($cookieBannerTexts['cookie_essential_name'], JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX_APOS | JSON_HEX_QUOT) ?>, + description: <?= json_encode($cookieBannerTexts['cookie_essential_description'], JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX_APOS | JSON_HEX_QUOT) ?>,(apply the same change to all
json_encodecalls on Lines 53–93)Or, since
JSON_HEX_TAG | JSON_HEX_AMPis already sufficient for XSS prevention in a<script>inline context (standardjson_encodealready escapes"as\"inside a JSON string), simplify Line 94 to match the rest:- statementUrl: <?= json_encode(route_path('cookies'), JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX_APOS | JSON_HEX_QUOT) ?>, + statementUrl: <?= json_encode(route_path('cookies'), JSON_HEX_TAG | JSON_HEX_AMP) ?>,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/partials/cookie-banner.php` around lines 53 - 94, The json_encode flags are inconsistent: most calls use JSON_HEX_TAG | JSON_HEX_AMP but statementUrl uses JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX_APOS | JSON_HEX_QUOT; make them uniform by either adding JSON_HEX_APOS | JSON_HEX_QUOT to all json_encode uses for cookieBannerTexts (references: cookieBannerTexts[...] entries in the script block) and the route_path('cookies') call, or remove the extra JSON_HEX_APOS | JSON_HEX_QUOT from the statementUrl json_encode so it matches the others; update the json_encode invocations around the text object (banner and preferences entries and statementUrl) accordingly.app/Controllers/GeneriController.php (1)
100-141: Add explicit non-empty name validation for UI update.
The API path rejects empty names, but this UI path doesn’t. Consider an early check with a user-facing message for consistency.Proposed tweak
- $updateData = ['nome' => trim((string)($data['nome'] ?? ''))]; + $nome = trim((string)($data['nome'] ?? '')); + if ($nome === '') { + $_SESSION['error_message'] = __('Il nome del genere è obbligatorio.'); + return $response->withHeader('Location', "/admin/generi/{$id}")->withStatus(302); + } + $updateData = ['nome' => $nome];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/GeneriController.php` around lines 100 - 141, Add an explicit non-empty name validation before the DB/cycle logic: after computing $updateData (or immediately after $data['nome'] is read) check if trim((string)($data['nome'] ?? '')) === '' and if so set $_SESSION['error_message'] = __('Il nome non può essere vuoto.'), then return a 302 redirect to "/admin/generi/{$id}" without calling $repo->update; this mirrors the API behavior and prevents empty names from reaching GeneriController::update and $repo->update.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Views/frontend/home.php`:
- Around line 891-900: The frontend is showing a hardcoded emoji for available
books; fix by adding an availability count to the API and consuming it: modify
FrontendController::catalogAPI() to compute the available books count (similar
to getFilterOptions() logic) and include it in the JSON response under
pagination.available_books, then update the frontend fetch handler (the block
that calls fetch(API_CATALOG_ROUTE) and references totalBooksEl and
availableBooksEl) to set availableBooksEl.textContent =
data.pagination.available_books instead of the emoji; ensure error handling
still sets sensible defaults.
In `@scripts/manual-update.php`:
- Around line 18-30: The fallback .env parsing can capture surrounding quotes
causing mismatch with getenv() and the script only checks one .env location;
update the $secretKey fallback so the preg_match result is trimmed of
surrounding single or double quotes (strip leading/trailing quotes after
extracting via preg_match) before returning, and also check both potential .env
locations (e.g., __DIR__ . '/../.env' and __DIR__ . '/.env') when loading the
file so the manual-update script finds the root .env regardless of where it's
invoked; ensure the final $secretKey used in the hash_equals comparison is the
cleaned, unquoted string.
In `@tests/genre-bugs.spec.js`:
- Around line 4-6: Replace the hardcoded constants BASE, ADMIN_EMAIL, ADMIN_PASS
with environment variables (e.g. process.env.BASE_URL, process.env.ADMIN_EMAIL,
process.env.ADMIN_PASS) and make the test suite skip when the required creds are
not provided; locate the constants BASE, ADMIN_EMAIL, ADMIN_PASS in
tests/genre-bugs.spec.js, read their values from process.env, and if ADMIN_EMAIL
or ADMIN_PASS are missing, call the test framework's skip mechanism (e.g. wrap
or call describe.skip / test.skip or early-return in a top-level beforeAll) so
tests are not run when secrets are absent.
In `@tests/security-hardening.spec.js`:
- Around line 4-6: Remove the hardcoded credentials (BASE, ADMIN_EMAIL,
ADMIN_PASS) from tests/security-hardening.spec.js and instead read them from
environment variables (e.g., process.env.TEST_BASE, process.env.ADMIN_EMAIL,
process.env.ADMIN_PASS); if the required env vars are missing, make the test
suite skip/exit gracefully (use the test framework's skip or before hook to
bail) and document that credentials must be supplied via CI secrets or a local
.env (not committed). Ensure all refs to ADMIN_EMAIL/ADMIN_PASS in the file use
the env-derived variables so no real credentials remain in source.
---
Outside diff comments:
In `@app/Middleware/CsrfMiddleware.php`:
- Around line 13-63: The SecureLogger::warning call in CsrfMiddleware::process
currently logs partial token values and session token; change the log to omit
any token fragments and only include the validation reason (from
Csrf::validateWithReason result) and safe contextual info (e.g., request path or
method if needed), i.e. remove substr($_SERVER/$_SESSION token) and $token bits
from the message and call SecureLogger::warning with a message like "[CSRF]
Validation failed. Reason: " . $csrfValidation['reason']; keep
Csrf::validateWithReason and SecureLogger::warning usage but stop including
token material in the logged string.
In `@scripts/manual-update.php`:
- Around line 441-456: The loop over $statements currently skips any chunk that
begins with '--' (strpos($stmt, '--') === 0), which can discard valid SQL that
follows a leading single-line comment; change the handling in the foreach
($statements as $stmt) block to strip leading SQL comment lines instead of
skipping the whole chunk: trim leading whitespace, remove any leading lines
starting with '--' (or blank lines) until the first real SQL line, then if the
resulting $stmt is empty continue, otherwise pass it to $db->query and keep the
existing error handling (errno capture and $ignorableErrors array) unchanged.
In `@storage/plugins/digital-library/DigitalLibraryPlugin.php`:
- Around line 383-465: The server-side MIME allowlist in the upload handler
rejects valid EPUB and WAV files because finfo can report EPUB as
"application/zip" and WAV as "audio/x-wav" or "audio/wave"; update the
$allowedMime array used before the finfo/file check in the same upload routine
(the code around $allowedMime, $validExt, finfo->file, and the MIME validation
block) to include 'application/zip' for EPUB and add 'audio/x-wav' and
'audio/wave' alongside existing WAV variants so valid uploads that passed
extension validation (generateSafeFilename, $validExt) are not falsely rejected.
---
Duplicate comments:
In `@app/Controllers/GeneriController.php`:
- Line 149: The destroy method signature currently includes an unused Request
$request parameter which triggers PHPMD; update the method declaration for
destroy(Request $request, Response $response, \mysqli $db, int $id): Response by
either removing the $request parameter entirely if the framework allows or
renaming it to $_request to mark it as intentionally unused, and then adjust any
callers or type hints accordingly to keep signatures consistent.
In `@app/Models/BookRepository.php`:
- Around line 961-1045: The resolver currently always re-queries the generi
table; update resolveGenreHierarchy to first build the L1–L3 chain from the
already-joined fields in $row (use the joined fields/aliases produced by the
main query — e.g. the g/gp/gpp name/id fields or existing keys like genere_id,
genere_nome, radice_id/radice_nome, etc.) and map those to
radice_id/radice_nome, genere_id_cascade/genere_nome and
sottogenere_id_cascade/sottogenere_nome; only fall back to the DB SELECT logic
(the prepared statement in resolveGenreHierarchy) when the chain cannot be
resolved from $row (i.e. deeper than three levels or missing joined columns).
Ensure you keep the existing mapping logic and safety limit but skip the
SELECT/while loop when the 3-level data is present in $row.
In `@app/Views/libri/index.php`:
- Around line 439-440: collanaFilter is only set once from initialCollana and
never updated, so hook the UI input/autocomplete to keep it in sync: either add
an input/change/autocomplete event listener on the collana input element (the
same element referenced for autocomplete) that updates the variable
collanaFilter, or stop relying on the global collanaFilter and instead read the
current value of the collana input when constructing the AJAX payload (where
search/filter params are assembled); apply the same fix for the other
occurrences around the block referenced (lines that initialize collanaFilter
elsewhere, e.g., the duplicate logic at 639-645) so UI changes actually affect
requests.
In `@package.json`:
- Line 7: The package.json "test" script references tests/playwright.config.js
which doesn't exist; either add the missing Playwright config file at
tests/playwright.config.js (exporting the Playwright configuration) or update
the "test" script in package.json to point to an existing config or use the
default (e.g., remove the --config flag); edit the file(s) that define the test
script and/or create tests/playwright.config.js so running npm test/playwright
test succeeds.
---
Nitpick comments:
In `@app/Controllers/GeneriController.php`:
- Around line 100-141: Add an explicit non-empty name validation before the
DB/cycle logic: after computing $updateData (or immediately after $data['nome']
is read) check if trim((string)($data['nome'] ?? '')) === '' and if so set
$_SESSION['error_message'] = __('Il nome non può essere vuoto.'), then return a
302 redirect to "/admin/generi/{$id}" without calling $repo->update; this
mirrors the API behavior and prevents empty names from reaching
GeneriController::update and $repo->update.
In `@app/Views/partials/cookie-banner.php`:
- Around line 53-94: The json_encode flags are inconsistent: most calls use
JSON_HEX_TAG | JSON_HEX_AMP but statementUrl uses JSON_HEX_TAG | JSON_HEX_AMP |
JSON_HEX_APOS | JSON_HEX_QUOT; make them uniform by either adding JSON_HEX_APOS
| JSON_HEX_QUOT to all json_encode uses for cookieBannerTexts (references:
cookieBannerTexts[...] entries in the script block) and the
route_path('cookies') call, or remove the extra JSON_HEX_APOS | JSON_HEX_QUOT
from the statementUrl json_encode so it matches the others; update the
json_encode invocations around the text object (banner and preferences entries
and statementUrl) accordingly.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
.gitattributesapp/Controllers/Admin/LanguagesController.phpapp/Controllers/CmsController.phpapp/Controllers/CollocazioneController.phpapp/Controllers/GeneriApiController.phpapp/Controllers/GeneriController.phpapp/Controllers/LibriApiController.phpapp/Controllers/LibriController.phpapp/Controllers/SettingsController.phpapp/Middleware/CsrfMiddleware.phpapp/Models/BookRepository.phpapp/Models/GenereRepository.phpapp/Routes/web.phpapp/Views/admin/settings.phpapp/Views/autori/index.phpapp/Views/cms/edit-home.phpapp/Views/editori/index.phpapp/Views/frontend/book-detail.phpapp/Views/frontend/catalog.phpapp/Views/frontend/home.phpapp/Views/generi/dettaglio_genere.phpapp/Views/layout.phpapp/Views/libri/index.phpapp/Views/libri/partials/book_form.phpapp/Views/partials/cookie-banner.phpapp/Views/prestiti/index.phppackage.jsonpublic/.htaccesspublic/index.phpscripts/manual-update.phpstorage/plugins/api-book-scraper/plugin.jsonstorage/plugins/dewey-editor/plugin.jsonstorage/plugins/digital-library/DigitalLibraryPlugin.phpstorage/plugins/digital-library/plugin.jsonstorage/plugins/open-library/plugin.jsonstorage/plugins/z39-server/plugin.jsontests/genre-bugs.spec.jstests/security-hardening.spec.js
Comment on lines
891
to
+900
| fetch(API_CATALOG_ROUTE) | ||
| .then(response => response.json()) | ||
| .then(data => { | ||
| totalBooksEl.innerHTML = data.pagination.total_books; | ||
| availableBooksEl.innerHTML = '📚'; | ||
| totalBooksEl.textContent = data.pagination.total_books; | ||
| availableBooksEl.textContent = '\uD83D\uDCDA'; | ||
| }) | ||
| .catch(error => { | ||
| console.error('Error loading stats:', error); | ||
| totalBooksEl.innerHTML = '📚'; | ||
| availableBooksEl.innerHTML = '✓'; | ||
| totalBooksEl.textContent = '\uD83D\uDCDA'; | ||
| availableBooksEl.textContent = '\u2713'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the HTML elements and understand what they're supposed to show
rg -n "total-books|available-books" -SRepository: fabiodalez-dev/Pinakes
Length of output: 977
🏁 Script executed:
# Search for the element references in JavaScript
rg -n "totalBooksEl|availableBooksEl" -S appRepository: fabiodalez-dev/Pinakes
Length of output: 714
🏁 Script executed:
# Check the home.php file around the context to see the full function
sed -n '880,910p' app/Views/frontend/home.phpRepository: fabiodalez-dev/Pinakes
Length of output: 1013
🏁 Script executed:
# Look for API endpoint definition
rg -n "API_CATALOG_ROUTE" -SRepository: fabiodalez-dev/Pinakes
Length of output: 381
🏁 Script executed:
# Check git history to see what this looked like before
git log --oneline -n 20 -- app/Views/frontend/home.phpRepository: fabiodalez-dev/Pinakes
Length of output: 150
🏁 Script executed:
# Check if there's a git diff for the recent commit
git show 8cfea8b --statRepository: fabiodalez-dev/Pinakes
Length of output: 50379
🏁 Script executed:
# Look for the API controller that returns pagination data
rg -n "pagination" -S app --type php | head -20Repository: fabiodalez-dev/Pinakes
Length of output: 1920
🏁 Script executed:
# Find the API catalog controller
find app -name "*catalog*" -o -name "*Catalog*" | grep -E "\.(php|js)$"Repository: fabiodalez-dev/Pinakes
Length of output: 132
🏁 Script executed:
# Check the controller that handles the API_CATALOG_ROUTE
rg -n "apiCatalogRoute" -S appRepository: fabiodalez-dev/Pinakes
Length of output: 633
🏁 Script executed:
# Find the catalog API controller
find app/Controllers -name "*Api*" | xargs grep -l "catalog" -iRepository: fabiodalez-dev/Pinakes
Length of output: 50
🏁 Script executed:
# Look for the route definition to understand which controller handles api_catalog
rg -n "api_catalog" -S appRepository: fabiodalez-dev/Pinakes
Length of output: 864
🏁 Script executed:
# Check what the API endpoint returns - search for pagination data structure in the controller
rg -n "pagination" -S app/Controllers --type php -A 5 | head -40Repository: fabiodalez-dev/Pinakes
Length of output: 2980
🏁 Script executed:
# Search for the controller that returns book statistics/pagination
rg -n "total_books" -S app/ControllersRepository: fabiodalez-dev/Pinakes
Length of output: 937
🏁 Script executed:
# Get the full context of the API catalog route handler
sed -n '2170,2200p' app/Routes/web.phpRepository: fabiodalez-dev/Pinakes
Length of output: 1831
🏁 Script executed:
# Look for the catalog API method in FrontendController
rg -n "public function.*catalog" -S app/Controllers/FrontendController.php -A 30 | head -60Repository: fabiodalez-dev/Pinakes
Length of output: 2647
🏁 Script executed:
# Check if there's an available_books field anywhere in the API response
rg -n "available" -S app/Controllers/FrontendController.phpRepository: fabiodalez-dev/Pinakes
Length of output: 463
🏁 Script executed:
# Let's see the full context of the home.php file around the loadStats function, including any comments about expected API structure
sed -n '870,920p' app/Views/frontend/home.phpRepository: fabiodalez-dev/Pinakes
Length of output: 1772
🏁 Script executed:
# Get the full catalogAPI method to see the complete response structure
sed -n '466,570p' app/Controllers/FrontendController.phpRepository: fabiodalez-dev/Pinakes
Length of output: 4050
🏁 Script executed:
# Check if catalogAPI response includes availability data
rg -n "catalogAPI" -S app/Controllers/FrontendController.php -A 100 | grep -E "(available|return.*json)" -B 2 -A 2 | head -30Repository: fabiodalez-dev/Pinakes
Length of output: 224
🏁 Script executed:
# Check git history to see what this code looked like in previous commits
git log --oneline -n 5 -- app/Views/frontend/home.phpRepository: fabiodalez-dev/Pinakes
Length of output: 150
🏁 Script executed:
# Let's check if there's another stat API or if available books should come from catalogAPI
rg -n "available.*book" -S app/Controllers/FrontendController.php -iRepository: fabiodalez-dev/Pinakes
Length of output: 149
🏁 Script executed:
# Check the HTML template to understand what these stats should show
sed -n '50,70p' app/Views/frontend/home-sections/hero.phpRepository: fabiodalez-dev/Pinakes
Length of output: 1243
🏁 Script executed:
# Check if the catalogAPI should include available_books - look at the full context around line 974
sed -n '960,1010p' app/Controllers/FrontendController.phpRepository: fabiodalez-dev/Pinakes
Length of output: 2139
🏁 Script executed:
# Search for any other API that returns availability data
rg -n "disponibil|available" -S app/Controllers/FrontendController.php -i -B 2 -A 2 | head -50Repository: fabiodalez-dev/Pinakes
Length of output: 2174
Add available books count to the API response or update the frontend to use it.
Line 895 sets availableBooksEl to a hardcoded emoji instead of displaying a count from the API. The element is labeled "Disponibili" (available) and should show the number of available books, not an emoji. The catalogAPI endpoint does not currently include an available_books field in its pagination data—only total_books. Either extend the API response to include availability counts or retrieve available books separately.
🛠️ Suggested fix (update catalogAPI to include availability stats)
Update the API response in FrontendController::catalogAPI() to include available books count:
$data = [
'html' => $html,
'pagination' => [
'current_page' => $page,
'total_pages' => $total_pages,
'total_books' => $total_books,
+ 'available_books' => $availableCount,
'start' => $offset + 1,
'end' => min($offset + $limit, $total_books)
],
...
];Then update the frontend to use it:
- availableBooksEl.textContent = '\uD83D\uDCDA';
+ availableBooksEl.textContent = data.pagination.available_books ?? '0';(You'll need to calculate $availableCount in the method, similar to how it's done in getFilterOptions().)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Views/frontend/home.php` around lines 891 - 900, The frontend is showing
a hardcoded emoji for available books; fix by adding an availability count to
the API and consuming it: modify FrontendController::catalogAPI() to compute the
available books count (similar to getFilterOptions() logic) and include it in
the JSON response under pagination.available_books, then update the frontend
fetch handler (the block that calls fetch(API_CATALOG_ROUTE) and references
totalBooksEl and availableBooksEl) to set availableBooksEl.textContent =
data.pagination.available_books instead of the emoji; ensure error handling
still sets sensible defaults.
Comment on lines
18
to
30
| // or with a secret key for emergency access (must be set in .env) | ||
| $secretKey = getenv('MANUAL_UPDATE_KEY') ?: ( | ||
| file_exists(__DIR__ . '/../.env') ? | ||
| (function() { | ||
| $env = file_get_contents(__DIR__ . '/../.env'); | ||
| preg_match('/^MANUAL_UPDATE_KEY=(.*)$/m', $env, $m); | ||
| return trim($m[1] ?? ''); | ||
| })() : '' | ||
| ); | ||
|
|
||
| session_start(); | ||
| $isAdmin = isset($_SESSION['user']['tipo_utente']) && $_SESSION['user']['tipo_utente'] === 'admin'; | ||
| $hasKey = isset($_GET['key']) && $_GET['key'] === $secretKey; | ||
| $hasKey = !empty($secretKey) && isset($_GET['key']) && hash_equals($secretKey, $_GET['key']); |
There was a problem hiding this comment.
Timing-safe key check and env-based secret — good hardening.
Two minor observations:
- The regex
/^MANUAL_UPDATE_KEY=(.*)$/m(line 23) will capture surrounding quotes if the.envvalue is quoted (e.g.,MANUAL_UPDATE_KEY="mysecret"→ captured as"mysecret"), whilegetenv()typically strips them. This mismatch can causehash_equalsto fail for the fallback path. - The
.envpath__DIR__ . '/../.env'is correct when the script runs fromscripts/, but the doc comment (line 6) tells users to upload it to the root — where it would resolve one level above.
🔧 Proposed fix for quoted .env values
$env = file_get_contents(__DIR__ . '/../.env');
preg_match('/^MANUAL_UPDATE_KEY=(.*)$/m', $env, $m);
- return trim($m[1] ?? '');
+ return trim(trim($m[1] ?? ''), '"\'');
})() : ''🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/manual-update.php` around lines 18 - 30, The fallback .env parsing
can capture surrounding quotes causing mismatch with getenv() and the script
only checks one .env location; update the $secretKey fallback so the preg_match
result is trimmed of surrounding single or double quotes (strip leading/trailing
quotes after extracting via preg_match) before returning, and also check both
potential .env locations (e.g., __DIR__ . '/../.env' and __DIR__ . '/.env') when
loading the file so the manual-update script finds the root .env regardless of
where it's invoked; ensure the final $secretKey used in the hash_equals
comparison is the cleaned, unquoted string.
- CsrfMiddleware: stop logging partial CSRF token fragments - manual-update.php: strip SQL comment lines instead of skipping entire chunks; strip surrounding quotes from .env value parsing - DigitalLibraryPlugin: add application/zip (EPUB), audio/x-wav, audio/wave to MIME allowlist for finfo compatibility across libmagic versions - GeneriController::update: add empty name validation (consistency with API) - cookie-banner: uniform JSON_HEX_* flags on all json_encode calls - Tests: move hardcoded credentials to env vars (E2E_ADMIN_EMAIL, E2E_ADMIN_PASS), skip gracefully when not set
- 21 serial Playwright tests covering full loan lifecycle: user request, admin approval, pickup, return, rejection - Reservation flow when no copies available, duplicate prevention - DB state verification at every step via MySQL CLI helper - Smoke-install test covering installer wizard + basic CRUD - Playwright config with serial execution and generous timeouts
Owner
Author
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
app/Views/frontend/home.php (1)
895-895:availableBooksElstill shows an emoji placeholder instead of a real count.The API does not return an
available_booksfield, so this element cannot show meaningful data. The past review flagged this same issue and proposed extendingcatalogAPI()to include the count.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/frontend/home.php` at line 895, availableBooksEl is being set to a static emoji because the API response lacks an available_books count; update catalogAPI() to compute and return the available_books count (or the appropriate field) and then use that returned value to set availableBooksEl.textContent instead of the emoji. Locate catalogAPI() and add logic to derive available_books from the returned catalog data (e.g., filter by availability or sum counts), update its response shape to include available_books, and then change the assignment where availableBooksEl.textContent = ... to use the new response.available_books value.app/Models/BookRepository.php (1)
970-1044: Consider reusing the already-joined genre data to avoid extra queries.
The resolver still walks the tree with per-level SELECTs even thoughg/gp/gppare already in the main JOIN for the common 1–3 level case. Using those first and only falling back for deeper chains would cut DB round-trips.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/BookRepository.php` around lines 970 - 1044, The resolver resolveGenreHierarchy currently always issues per-level SELECTs to build $chain; instead first attempt to build the 1–3 level chain from the already-joined genre columns (the typical g/gp/gpp fields present in $row — e.g. root/genre/subgenre ids/names your query adds) and populate radice_*, genere_*, sottogenere_* from those values, and only fall back to the prepared SELECT loop (the existing $stmt and while/currentId logic) when the joined fields are missing or when the genre depth exceeds three; implement this by checking the joined keys on $row before running the DB loop and keep existing variables (resolveGenreHierarchy, $row, $chain, $sottogenereId) to minimize changes.app/Views/libri/index.php (1)
639-645:⚠️ Potential issue | 🟠 MajorCollana filter still appears frozen to the initial URL value.
collanais sent fromcollanaFilter, which is only initialized from the URL and never updated from UI input. If a collana autocomplete/input exists, wire it into the payload (or updatecollanaFilteron change). Otherwise, consider removing this param to avoid a stale filter.🛠️ Suggested wiring (adjust input id as needed)
- collana: collanaFilter + collana: (document.getElementById('filter_collana')?.value || collanaFilter) || ''🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/libri/index.php` around lines 639 - 645, The collana payload uses collanaFilter which is only set from the URL and never updated from the UI, so wire the UI input/autocomplete into the filter or remove the param; locate the collanaFilter variable and the payload object where collana: collanaFilter is built in the function that prepares the request, and update code to read the current value from the UI control (e.g., document.getElementById('collana_id').value or the autocomplete's selected value) before sending, or attach an onchange handler to update collanaFilter whenever the input changes; if no UI exists, remove the collana property from the payload to avoid sending a stale filter.app/Controllers/GeneriController.php (1)
154-155:⚠️ Potential issue | 🟡 MinorUnused
$requestparameter still present indestroy().PHPMD will continue to flag this. Consider removing it or renaming to
$_request.🔧 Minimal fix
-public function destroy(Request $request, Response $response, \mysqli $db, int $id): Response +public function destroy(Request $_request, Response $response, \mysqli $db, int $id): Response🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/GeneriController.php` around lines 154 - 155, The destroy method signature currently includes an unused Request $request parameter which triggers PHPMD; update the method signature for destroy(Request $request, Response $response, \mysqli $db, int $id) by either removing the $request parameter entirely (and adjust any internal calls or callers if they expect four args) or renaming it to an intentionally unused variable like $_request to silence the linter, and ensure no references to $request remain inside the destroy() implementation.
🧹 Nitpick comments (2)
.gitattributes (1)
6-21: LGTM — sensible export-ignore additions.All exclusions are appropriate for stripping dev, CI, tooling, and test artifacts from
git archivereleases.One minor inconsistency worth noting:
/frontend/is anchored to the repository root (leading/), while every other directory pattern (tests/,.github/,.vscode/, etc.) is unanchored and would match at any depth. Iffrontend/only exists at the root this is functionally equivalent, but for consistency you may want to either anchor all entries or remove the leading/from/frontend/.♻️ Align path anchoring for consistency
-/frontend/ export-ignore +frontend/ export-ignore🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitattributes around lines 6 - 21, The .gitattributes list uses an anchored "/frontend/" entry while other directory patterns are unanchored (e.g., "tests/", ".github/", ".vscode/"); update the frontend pattern for consistency by removing the leading slash so it reads "frontend/"; locate the "/frontend/" entry in the .gitattributes diff and replace it with "frontend/" so all directory patterns follow the same unanchored convention.public/index.php (1)
406-408: Dead call —forceContentTypeon a handler that is immediately replaced.Lines 406–408 retrieve the default error handler and call
forceContentType('text/html')on it, but line 465 discards that handler by installing$customErrorHandler. TheforceContentTypecall has no effect.♻️ Proposed cleanup
- /** `@var` \Slim\Handlers\ErrorHandler $errorHandler */ - $errorHandler = $errorMiddleware->getDefaultErrorHandler(); - $errorHandler->forceContentType('text/html'); - $customErrorHandler = function (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/index.php` around lines 406 - 408, The call to forceContentType('text/html') on $errorHandler (returned from $errorMiddleware->getDefaultErrorHandler()) is dead because you replace that handler later with $customErrorHandler; either remove the redundant $errorHandler retrieval and its forceContentType call, or move/apply the call to the actual handler you register (i.e., call $customErrorHandler->forceContentType('text/html') after you create/register $customErrorHandler) so the content type change affects the handler in use.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Views/libri/partials/book_form.php`:
- Around line 31-32: Accessing $book['genere_id_cascade'] and
$book['sottogenere_id_cascade'] can trigger undefined index notices when $book
is empty; update the expressions in book_form.php to use null-coalescing first
(e.g. use ($book['genere_id_cascade'] ?? $book['genere_id'] ?? 0) and
($book['sottogenere_id_cascade'] ?? $book['sottogenere_id'] ?? 0)) and cast the
final result to int so the fallback logic no longer relies on the
undefined-index-prone ?: operator.
In `@public/index.php`:
- Line 400: The code now defaults displayErrorDetails to false when reading
$container->get('settings')['displayErrorDetails'] ?? false, which hides stack
traces in dev; update docs and dev settings so development environments
explicitly set displayErrorDetails: true (or add it into the example/dev
settings file) and ensure any deployment or staging config files used for local
testing include the explicit displayErrorDetails key so developers get stack
traces during development.
In `@storage/plugins/digital-library/DigitalLibraryPlugin.php`:
- Around line 202-205: Guard the call to $stmt->get_result() and avoid unguarded
access to ->num_rows: after calling $stmt->execute() and $stmt->get_result()
check that $result is an instance of \mysqli_result (or !== false) before
reading $result->num_rows, mirroring the pattern used in onInstall(); if
get_result() returned false, handle the failure path (log the error,
throw/return early) instead of accessing properties on a boolean.
In `@tests/loan-reservation.spec.js`:
- Around line 5-23: Remove hardcoded secrets by reading ADMIN_EMAIL and
ADMIN_PASS from env vars (replace ADMIN_EMAIL/ADMIN_PASS defaults) and make
dbQuery use env-driven DB_USER, DB_PASS, DB_SOCKET, DB_NAME (instead of literal
'fabiodal_biblioteca_user', password, socket, and DB name); if any required env
var for E2E (admin credentials or DB connection) is missing, mark/skip the tests
early (e.g., call Mocha's this.skip or conditionally skip in the top-level
describe) and avoid running execFileSync with hardcoded credentials; update the
dbQuery function signature or its callers to rely on these env vars so CI is not
brittle.
In `@tests/smoke-install.spec.js`:
- Around line 4-7: Replace hard-coded credential defaults in tests by making
BASE, ADMIN_EMAIL, ADMIN_PASS (and the similar vars in loan-reservation.spec.js)
use environment variables with empty-string fallbacks (e.g., const ADMIN_EMAIL =
process.env.E2E_ADMIN_EMAIL || ''), then add a guard that skips the smoke and
loan tests when required credentials are missing (use the same skip/abort
pattern used in security-hardening.spec.js and genre-merge-rearrange.spec.js).
Update the test setup that uses BASE, ADMIN_EMAIL, ADMIN_PASS (and DB secrets
referenced in loan-reservation.spec.js) to check for empty values and call the
test skip/abort helper so tests are not run without explicit env configuration.
---
Duplicate comments:
In `@app/Controllers/GeneriController.php`:
- Around line 154-155: The destroy method signature currently includes an unused
Request $request parameter which triggers PHPMD; update the method signature for
destroy(Request $request, Response $response, \mysqli $db, int $id) by either
removing the $request parameter entirely (and adjust any internal calls or
callers if they expect four args) or renaming it to an intentionally unused
variable like $_request to silence the linter, and ensure no references to
$request remain inside the destroy() implementation.
In `@app/Models/BookRepository.php`:
- Around line 970-1044: The resolver resolveGenreHierarchy currently always
issues per-level SELECTs to build $chain; instead first attempt to build the 1–3
level chain from the already-joined genre columns (the typical g/gp/gpp fields
present in $row — e.g. root/genre/subgenre ids/names your query adds) and
populate radice_*, genere_*, sottogenere_* from those values, and only fall back
to the prepared SELECT loop (the existing $stmt and while/currentId logic) when
the joined fields are missing or when the genre depth exceeds three; implement
this by checking the joined keys on $row before running the DB loop and keep
existing variables (resolveGenreHierarchy, $row, $chain, $sottogenereId) to
minimize changes.
In `@app/Views/frontend/home.php`:
- Line 895: availableBooksEl is being set to a static emoji because the API
response lacks an available_books count; update catalogAPI() to compute and
return the available_books count (or the appropriate field) and then use that
returned value to set availableBooksEl.textContent instead of the emoji. Locate
catalogAPI() and add logic to derive available_books from the returned catalog
data (e.g., filter by availability or sum counts), update its response shape to
include available_books, and then change the assignment where
availableBooksEl.textContent = ... to use the new response.available_books
value.
In `@app/Views/libri/index.php`:
- Around line 639-645: The collana payload uses collanaFilter which is only set
from the URL and never updated from the UI, so wire the UI input/autocomplete
into the filter or remove the param; locate the collanaFilter variable and the
payload object where collana: collanaFilter is built in the function that
prepares the request, and update code to read the current value from the UI
control (e.g., document.getElementById('collana_id').value or the autocomplete's
selected value) before sending, or attach an onchange handler to update
collanaFilter whenever the input changes; if no UI exists, remove the collana
property from the payload to avoid sending a stale filter.
---
Nitpick comments:
In @.gitattributes:
- Around line 6-21: The .gitattributes list uses an anchored "/frontend/" entry
while other directory patterns are unanchored (e.g., "tests/", ".github/",
".vscode/"); update the frontend pattern for consistency by removing the leading
slash so it reads "frontend/"; locate the "/frontend/" entry in the
.gitattributes diff and replace it with "frontend/" so all directory patterns
follow the same unanchored convention.
In `@public/index.php`:
- Around line 406-408: The call to forceContentType('text/html') on
$errorHandler (returned from $errorMiddleware->getDefaultErrorHandler()) is dead
because you replace that handler later with $customErrorHandler; either remove
the redundant $errorHandler retrieval and its forceContentType call, or
move/apply the call to the actual handler you register (i.e., call
$customErrorHandler->forceContentType('text/html') after you create/register
$customErrorHandler) so the content type change affects the handler in use.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
.gitattributesapp/Controllers/Admin/LanguagesController.phpapp/Controllers/CmsController.phpapp/Controllers/CollocazioneController.phpapp/Controllers/GeneriApiController.phpapp/Controllers/GeneriController.phpapp/Controllers/LibriApiController.phpapp/Controllers/LibriController.phpapp/Controllers/SettingsController.phpapp/Middleware/CsrfMiddleware.phpapp/Models/BookRepository.phpapp/Models/GenereRepository.phpapp/Routes/web.phpapp/Views/admin/settings.phpapp/Views/autori/index.phpapp/Views/cms/edit-home.phpapp/Views/editori/index.phpapp/Views/frontend/book-detail.phpapp/Views/frontend/catalog.phpapp/Views/frontend/home.phpapp/Views/generi/dettaglio_genere.phpapp/Views/layout.phpapp/Views/libri/index.phpapp/Views/libri/partials/book_form.phpapp/Views/partials/cookie-banner.phpapp/Views/prestiti/index.phppackage.jsonpublic/.htaccesspublic/index.phpscripts/manual-update.phpstorage/plugins/api-book-scraper/plugin.jsonstorage/plugins/dewey-editor/plugin.jsonstorage/plugins/digital-library/DigitalLibraryPlugin.phpstorage/plugins/digital-library/plugin.jsonstorage/plugins/open-library/plugin.jsonstorage/plugins/z39-server/plugin.jsontests/genre-bugs.spec.jstests/genre-merge-rearrange.spec.jstests/loan-reservation.spec.jstests/playwright.config.jstests/security-hardening.spec.jstests/smoke-install.spec.js
- tests: externalize all credentials to env vars, skip when missing - book_form: null-coalesce cascade fields to prevent undefined index - DigitalLibraryPlugin: guard get_result() with instanceof check - index.php: remove dead forceContentType on replaced error handler - .gitattributes: unanchor /frontend/ for consistency
33 Playwright tests covering: - Admin settings: general, email, contacts, privacy, labels, advanced tabs - CMS homepage editing: hero section, visibility toggle, section reorder - User self-registration: form validation, weak password, activation flow - Book creation with ISBN scraping attempt - Concurrent loan prevention (2 users, 1 copy) - Collana (series) filter on book list - Author and publisher CRUD with SweetAlert handling All 54 E2E tests pass (33 admin-features + 21 loan-reservation).
Owner
Author
|
@coderabbitai full review |
Owner
Author
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
app/Views/frontend/home.php (1)
894-900: Show a real availability count instead of placeholdersLine 895 and Line 900 still set the "Disponibili" stat to emoji placeholders, so the UI never shows an actual available-books count. Either add
available_booksto the API response or adjust the UI to hide this stat when data is unavailable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/frontend/home.php` around lines 894 - 900, The UI currently uses emoji placeholders for the available-books stat (setting availableBooksEl.textContent to '📚' or '✓' in the fetch success/error handlers) so the real count never appears; either update the API response to include available_books and set availableBooksEl.textContent = data.pagination.available_books in the success branch (and handle missing/undefined by hiding the element or showing 'N/A'), or if backend changes aren’t possible, hide or remove the availableBooksEl when data.pagination.available_books is absent and avoid setting emoji placeholders in both the success and .catch handlers (update code that references totalBooksEl and availableBooksEl accordingly).public/index.php (1)
400-400: Line 400: dev error details default change.
Same concern as earlier: ensure dev/staging configs explicitly setdisplayErrorDetails=trueto keep stack traces available during development.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/index.php` at line 400, The current line sets $displayErrorDetails to false by default which can hide stack traces in dev/staging; change this so you do not silently default to false—either require an explicit settings['displayErrorDetails'] value or set the default to true for non-production environments. Update the assignment around $displayErrorDetails (the $container->get('settings')['displayErrorDetails'] ?? false expression) to read the explicit config or default to true for dev/staging, and ensure your dev/staging configuration files explicitly set settings['displayErrorDetails'] = true rather than relying on the fallback.scripts/manual-update.php (1)
18-25:⚠️ Potential issue | 🟡 MinorFallback .env lookup can miss root installs.
If the script is uploaded to the project root (per the header),
__DIR__ . '/../.env'points above the root, so the fallback MANUAL_UPDATE_KEY won’t be found and emergency access can fail. Consider checking both likely .env locations.🛠️ Suggested fix
-$secretKey = getenv('MANUAL_UPDATE_KEY') ?: ( - file_exists(__DIR__ . '/../.env') ? - (function() { - $env = file_get_contents(__DIR__ . '/../.env'); - preg_match('/^MANUAL_UPDATE_KEY=(.*)$/m', $env, $m); - return trim(trim($m[1] ?? ''), '"\''); - })() : '' -); +$secretKey = getenv('MANUAL_UPDATE_KEY') ?: (function () { + $paths = [__DIR__ . '/../.env', __DIR__ . '/.env']; + foreach ($paths as $path) { + if (!file_exists($path)) { + continue; + } + $env = file_get_contents($path); + if ($env === false) { + continue; + } + preg_match('/^MANUAL_UPDATE_KEY=(.*)$/m', $env, $m); + return trim(trim($m[1] ?? ''), '"\''); + } + return ''; +})();#!/bin/bash rg -n "manual-update.php|MANUAL_UPDATE_KEY" -S🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/manual-update.php` around lines 18 - 25, The fallback .env lookup in scripts/manual-update.php assigns $secretKey using getenv('MANUAL_UPDATE_KEY') and only checks __DIR__ . '/../.env', which misses cases where the script lives in the project root; update the lookup to try both likely locations (e.g., __DIR__ . '/.env' and __DIR__ . '/../.env') before returning an empty string, preserving the current parsing logic (preg_match on MANUAL_UPDATE_KEY and trim) and the getenv() precedence so emergency access works whether the script is installed in the root or a subdirectory.app/Controllers/GeneriController.php (1)
154-169: Avoid unused$requestparameter to satisfy PHPMD.PHPMD flags Line 154 as an unused parameter. If framework contracts allow, rename to
$_requestor remove it to silence the warning.🧹 Minimal tweak
-public function destroy(Request $request, Response $response, \mysqli $db, int $id): Response +public function destroy($_request, Response $response, \mysqli $db, int $id): Response🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/GeneriController.php` around lines 154 - 169, The destroy method in GeneriController currently has an unused $request parameter which triggers PHPMD; to fix, either remove the parameter from the method signature if the framework's controller contract allows, or rename it to $_request to indicate intentional unused status and silence PHPMD, making sure to keep the other parameters (\mysqli $db, int $id) and existing logic that calls GenereRepository->delete($id) and the error handling in GeneriController::destroy unchanged.app/Views/libri/index.php (1)
629-645:⚠️ Potential issue | 🟠 MajorCollana filter still reads only the initial URL value.
Line 644 sends
collana: collanaFilter, which is initialized from the URL and never updated from any UI input. If a collana autocomplete exists, user selections won’t affect results. Consider reading the current input value or updatingcollanaFilterin its handlers.🛠️ Possible fix (use the actual collana input ID)
data: function(d) { + const collanaInput = document.getElementById('filter_collana'); // adjust to actual ID return { ...d, @@ - collana: collanaFilter + collana: (collanaInput ? collanaInput.value : collanaFilter) || '' }; },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/libri/index.php` around lines 629 - 645, The collana filter is using the stale variable collanaFilter (initialized from the URL) instead of the current UI value; update the object to read the live input or ensure collanaFilter is updated on selection. Replace collana: collanaFilter with reading the input (e.g., document.getElementById('collana_id').value or the actual collana input ID) or wire the collana autocomplete/handler to set collanaFilter when selections change so the return payload in the function that builds this object uses the current collana selection.
🧹 Nitpick comments (5)
app/Views/admin/settings.php (1)
468-482: Avoid appending an empty<ul>when there are no placeholders.This keeps the UI cleaner (no empty list spacing) and avoids adding a redundant node.
🧩 Suggested tweak
- const ul = document.createElement('ul'); - ul.className = 'list-disc pl-5'; - if (template.placeholders && template.placeholders.length > 0) { - template.placeholders.forEach(ph => { - const li = document.createElement('li'); - const code = document.createElement('code'); - code.textContent = '{{' + ph + '}}'; - li.appendChild(code); - ul.appendChild(li); - }); - } - placeholderContainer.appendChild(ul); + if (template.placeholders && template.placeholders.length > 0) { + const ul = document.createElement('ul'); + ul.className = 'list-disc pl-5'; + template.placeholders.forEach(ph => { + const li = document.createElement('li'); + const code = document.createElement('code'); + code.textContent = '{{' + ph + '}}'; + li.appendChild(code); + ul.appendChild(li); + }); + placeholderContainer.appendChild(ul); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/admin/settings.php` around lines 468 - 482, The current code always creates and appends an empty <ul> into the element with id "template-placeholders" even when template.placeholders is empty; change the logic in the block that builds placeholders so you only create and append the <ul> (and its <li>/<code> children) when template.placeholders && template.placeholders.length > 0, and otherwise leave placeholderContainer empty (still clear existing content via placeholderContainer.textContent = '' but do not append the ul). Refer to the identifiers placeholderContainer, template.placeholders, ul, li and code to find and update the snippet.tests/playwright.config.js (2)
4-13: AddforbidOnlyto prevent accidental.onlytests from blocking CI.Without
forbidOnly: !!process.env.CI, a straytest.onlycommitted to the branch will silently run only that test in CI without any warning.♻️ Proposed change
module.exports = defineConfig({ testDir: '.', + forbidOnly: !!process.env.CI, timeout: 120_000,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/playwright.config.js` around lines 4 - 13, Add a forbidOnly flag to the Playwright config object so CI fails if a stray test.only is committed: inside the object passed to defineConfig (the module.exports assignment), add the property forbidOnly: !!process.env.CI (or equivalent boolean derived from CI) alongside testDir/timeout/expect/reporter/use/retries to ensure CI sets forbidOnly true while local runs keep it false.
10-10: MakebaseURLconfigurable via environment variable.Hardcoding
http://localhost:8081prevents targeting different environments (CI, staging) without editing the config file. Using an environment variable with a sensible default enables flexibility across environments.Suggested change
- baseURL: 'http://localhost:8081', + baseURL: process.env.BASE_URL || 'http://localhost:8081',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/playwright.config.js` at line 10, The baseURL in the Playwright config is hardcoded; update the config object (the baseURL property in tests/playwright.config.js) to read from an environment variable (e.g. process.env.PLAYWRIGHT_BASE_URL or process.env.BASE_URL) with a fallback of 'http://localhost:8081' so tests can target CI/staging without editing the file; ensure the value is used where baseURL is exported in the config so existing test runs keep the default.app/Controllers/LibriController.php (2)
1718-1719: Good security fix — minor coupling risk with the'jpg'fallbackUsing magic bytes instead of the URL path is the right call; it prevents extension spoofing (e.g., a URL ending in
.phpbeing served as a JPEG).The
?? 'jpg'fallback is effectively dead code today becauseisValidImageMimeType()(line 1707) andgetExtensionFromMagicBytes()cover exactly the same format set (JPEG, PNG, GIF, WebP, AVIF, BMP). However, if a future format is added toisValidImageMimeType()without a matching entry ingetExtensionFromMagicBytes(), this fallback would silently save the file with a.jpgextension — the same silent-failure risk thathandleCoverUploadavoids by rejecting (returning early) instead of falling back. Consider aligning the two methods for consistency:♻️ Optional: match `handleCoverUpload`'s strict rejection pattern
- $ext = $this->getExtensionFromMagicBytes($img) ?? 'jpg'; + $ext = $this->getExtensionFromMagicBytes($img); + if ($ext === null) { + $this->logCoverDebug('handleCoverUrl.security.unknown_format', ['bookId' => $bookId, 'url' => $url]); + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibriController.php` around lines 1718 - 1719, The code falls back to 'jpg' when getExtensionFromMagicBytes($img) returns null, which can silently accept unknown/unsupported formats; update the logic so getExtensionFromMagicBytes and isValidImageMimeType remain consistent or reject instead of defaulting: either add the missing format mappings to getExtensionFromMagicBytes to match isValidImageMimeType (ensuring formats like JPEG, PNG, GIF, WebP, AVIF, BMP are all covered) or remove the 'jpg' fallback and make the caller (e.g., handleCoverUpload) return an error/early exit when getExtensionFromMagicBytes returns null so unknown formats are rejected rather than saved with a wrong extension.
1722-1728: Missing$stmt->close()after UPDATE inhandleCoverUrlThe prepared statement created at line 1724 is never closed. The same omission exists at lines 1564–1567 and 1579–1583 within this method. While MySQLi reclaims statements on GC, explicit closure is preferred to avoid holding server-side cursor resources unnecessarily under high concurrency.
🔧 Proposed fix
$stmt = $db->prepare('UPDATE libri SET copertina_url=?, updated_at=NOW() WHERE id=?'); $stmt->bind_param('si', $cover, $bookId); $stmt->execute(); + $stmt->close(); $this->logCoverDebug('handleCoverUrl.download.ok', ['bookId' => $bookId, 'stored' => $cover]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibriController.php` around lines 1722 - 1728, In handleCoverUrl, the prepared mysqli statement ($stmt) used for the UPDATE is never closed; locate the UPDATE block that prepares 'UPDATE libri SET copertina_url=?, updated_at=NOW() WHERE id=?' (and the similar prepare/execute blocks earlier around the method) and call $stmt->close() after $stmt->execute() (and after any conditional/early-return paths) to release server-side resources; ensure each $stmt created in handleCoverUrl is closed once you're done using it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Controllers/LibriApiController.php`:
- Around line 34-35: The controller reads/filtering on columns that may be
absent (e.g., l.collana, l.parole_chiave, l.sottogenere_id) causing 500s; update
LibriApiController to guard any access or SQL condition that references those
columns (including where you build $collana and the search/genre/collana filter
blocks) by checking column existence first (reuse the existing
BookRepository::hasColumn or the schema builder check) and only read
$q['collana'] or add the corresponding WHERE/SELECT clauses when the column
exists; ensure all places that reference l.parole_chiave, l.sottogenere_id, and
l.collana (the code around $collana, the search filter, genre filter, and the
block at lines ~131–135) follow this pattern so older schemas don’t trigger
queries against missing columns.
- Around line 619-644: The bulk-delete SQL in LibriApiController currently only
nulls isbn10/isbn13/ean; update the $sql UPDATE statement used before
$db->prepare(...) to also set scaffale_id = NULL, mensola_id = NULL,
posizione_progressiva = NULL, and collocazione = NULL so it matches
BookRepository::delete() behavior; keep the same WHERE id IN ($placeholders) AND
deleted_at IS NULL and leave the existing prepare/ bind_param/ execute/ error
handling in place.
In `@package.json`:
- Line 7: The package.json test script ("test": "playwright test
--config=tests/playwright.config.js") points to a non-existent config file;
either create the config file at tests/playwright.config.js or update the script
to use the correct config path (for example remove --config or point to an
existing playwright.config.js). Locate the "test" script in package.json and
either add the missing tests/playwright.config.js with your Playwright
configuration or change the script to "playwright test" or
"--config=playwright.config.js" so the npm test command runs against an actual
config. Ensure the chosen path matches the repository layout and that
`@playwright/test` remains in devDependencies.
In `@tests/loan-reservation.spec.js`:
- Around line 111-114: The password hash generation is using a hard-coded
'Test1234!' while the test uses the overridable TEST_PASS (from E2E_TEST_PASS);
update the execFileSync call that builds the PHP password_hash command to use
the TEST_PASS variable instead of the literal string so the hashed password
matches the login step (refer to TEST_PASS and the execFileSync invocation that
runs "php -r \"echo password_hash(...);\"").
In `@tests/security-hardening.spec.js`:
- Around line 39-41: The test string "wrong_key_12345" triggers gitleaks; update
the test in the "returns 403 with wrong key" test to use a non key-looking
placeholder or an obfuscated construction to avoid the detector — e.g. replace
the literal in the request URL `${BASE}/manual-update.php?key=wrong_key_12345`
with a safer value like "wrong_key_placeholder" or build it at runtime (e.g.
['wrong','key','12345'].join('_')) so the symbol BASE and the request call
remain the same but the static pattern no longer matches.
---
Duplicate comments:
In `@app/Controllers/GeneriController.php`:
- Around line 154-169: The destroy method in GeneriController currently has an
unused $request parameter which triggers PHPMD; to fix, either remove the
parameter from the method signature if the framework's controller contract
allows, or rename it to $_request to indicate intentional unused status and
silence PHPMD, making sure to keep the other parameters (\mysqli $db, int $id)
and existing logic that calls GenereRepository->delete($id) and the error
handling in GeneriController::destroy unchanged.
In `@app/Views/frontend/home.php`:
- Around line 894-900: The UI currently uses emoji placeholders for the
available-books stat (setting availableBooksEl.textContent to '📚' or '✓' in the
fetch success/error handlers) so the real count never appears; either update the
API response to include available_books and set availableBooksEl.textContent =
data.pagination.available_books in the success branch (and handle
missing/undefined by hiding the element or showing 'N/A'), or if backend changes
aren’t possible, hide or remove the availableBooksEl when
data.pagination.available_books is absent and avoid setting emoji placeholders
in both the success and .catch handlers (update code that references
totalBooksEl and availableBooksEl accordingly).
In `@app/Views/libri/index.php`:
- Around line 629-645: The collana filter is using the stale variable
collanaFilter (initialized from the URL) instead of the current UI value; update
the object to read the live input or ensure collanaFilter is updated on
selection. Replace collana: collanaFilter with reading the input (e.g.,
document.getElementById('collana_id').value or the actual collana input ID) or
wire the collana autocomplete/handler to set collanaFilter when selections
change so the return payload in the function that builds this object uses the
current collana selection.
In `@public/index.php`:
- Line 400: The current line sets $displayErrorDetails to false by default which
can hide stack traces in dev/staging; change this so you do not silently default
to false—either require an explicit settings['displayErrorDetails'] value or set
the default to true for non-production environments. Update the assignment
around $displayErrorDetails (the
$container->get('settings')['displayErrorDetails'] ?? false expression) to read
the explicit config or default to true for dev/staging, and ensure your
dev/staging configuration files explicitly set settings['displayErrorDetails'] =
true rather than relying on the fallback.
In `@scripts/manual-update.php`:
- Around line 18-25: The fallback .env lookup in scripts/manual-update.php
assigns $secretKey using getenv('MANUAL_UPDATE_KEY') and only checks __DIR__ .
'/../.env', which misses cases where the script lives in the project root;
update the lookup to try both likely locations (e.g., __DIR__ . '/.env' and
__DIR__ . '/../.env') before returning an empty string, preserving the current
parsing logic (preg_match on MANUAL_UPDATE_KEY and trim) and the getenv()
precedence so emergency access works whether the script is installed in the root
or a subdirectory.
---
Nitpick comments:
In `@app/Controllers/LibriController.php`:
- Around line 1718-1719: The code falls back to 'jpg' when
getExtensionFromMagicBytes($img) returns null, which can silently accept
unknown/unsupported formats; update the logic so getExtensionFromMagicBytes and
isValidImageMimeType remain consistent or reject instead of defaulting: either
add the missing format mappings to getExtensionFromMagicBytes to match
isValidImageMimeType (ensuring formats like JPEG, PNG, GIF, WebP, AVIF, BMP are
all covered) or remove the 'jpg' fallback and make the caller (e.g.,
handleCoverUpload) return an error/early exit when getExtensionFromMagicBytes
returns null so unknown formats are rejected rather than saved with a wrong
extension.
- Around line 1722-1728: In handleCoverUrl, the prepared mysqli statement
($stmt) used for the UPDATE is never closed; locate the UPDATE block that
prepares 'UPDATE libri SET copertina_url=?, updated_at=NOW() WHERE id=?' (and
the similar prepare/execute blocks earlier around the method) and call
$stmt->close() after $stmt->execute() (and after any conditional/early-return
paths) to release server-side resources; ensure each $stmt created in
handleCoverUrl is closed once you're done using it.
In `@app/Views/admin/settings.php`:
- Around line 468-482: The current code always creates and appends an empty <ul>
into the element with id "template-placeholders" even when template.placeholders
is empty; change the logic in the block that builds placeholders so you only
create and append the <ul> (and its <li>/<code> children) when
template.placeholders && template.placeholders.length > 0, and otherwise leave
placeholderContainer empty (still clear existing content via
placeholderContainer.textContent = '' but do not append the ul). Refer to the
identifiers placeholderContainer, template.placeholders, ul, li and code to find
and update the snippet.
In `@tests/playwright.config.js`:
- Around line 4-13: Add a forbidOnly flag to the Playwright config object so CI
fails if a stray test.only is committed: inside the object passed to
defineConfig (the module.exports assignment), add the property forbidOnly:
!!process.env.CI (or equivalent boolean derived from CI) alongside
testDir/timeout/expect/reporter/use/retries to ensure CI sets forbidOnly true
while local runs keep it false.
- Line 10: The baseURL in the Playwright config is hardcoded; update the config
object (the baseURL property in tests/playwright.config.js) to read from an
environment variable (e.g. process.env.PLAYWRIGHT_BASE_URL or
process.env.BASE_URL) with a fallback of 'http://localhost:8081' so tests can
target CI/staging without editing the file; ensure the value is used where
baseURL is exported in the config so existing test runs keep the default.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (43)
.gitattributesapp/Controllers/Admin/LanguagesController.phpapp/Controllers/CmsController.phpapp/Controllers/CollocazioneController.phpapp/Controllers/GeneriApiController.phpapp/Controllers/GeneriController.phpapp/Controllers/LibriApiController.phpapp/Controllers/LibriController.phpapp/Controllers/SettingsController.phpapp/Middleware/CsrfMiddleware.phpapp/Models/BookRepository.phpapp/Models/GenereRepository.phpapp/Routes/web.phpapp/Views/admin/settings.phpapp/Views/autori/index.phpapp/Views/cms/edit-home.phpapp/Views/editori/index.phpapp/Views/frontend/book-detail.phpapp/Views/frontend/catalog.phpapp/Views/frontend/home.phpapp/Views/generi/dettaglio_genere.phpapp/Views/layout.phpapp/Views/libri/index.phpapp/Views/libri/partials/book_form.phpapp/Views/partials/cookie-banner.phpapp/Views/prestiti/index.phppackage.jsonpublic/.htaccesspublic/index.phpscripts/manual-update.phpstorage/plugins/api-book-scraper/plugin.jsonstorage/plugins/dewey-editor/plugin.jsonstorage/plugins/digital-library/DigitalLibraryPlugin.phpstorage/plugins/digital-library/plugin.jsonstorage/plugins/open-library/plugin.jsonstorage/plugins/z39-server/plugin.jsontests/admin-features.spec.jstests/genre-bugs.spec.jstests/genre-merge-rearrange.spec.jstests/loan-reservation.spec.jstests/playwright.config.jstests/security-hardening.spec.jstests/smoke-install.spec.js
Comment on lines
+34
to
35
| $collana = trim((string) ($q['collana'] ?? '')); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, search for column definitions in migrations and schema files
rg -n "parole_chiave|sottogenere_id|collana" -g '*.sql' -g '*.php' | head -50Repository: fabiodalez-dev/Pinakes
Length of output: 6277
🏁 Script executed:
# Search for hasColumn and hasTableColumn method definitions
rg -n "hasColumn|hasTableColumn" -g '*.php' -A 3 | head -100Repository: fabiodalez-dev/Pinakes
Length of output: 8002
🏁 Script executed:
# Check the actual LibriApiController.php file around the mentioned lines
fd -e php -path '*LibriApiController.php' -x wc -l {}Repository: fabiodalez-dev/Pinakes
Length of output: 234
🏁 Script executed:
# Get the content of LibriApiController.php to understand the actual implementation
fd -e php -path '*LibriApiController.php' -x cat -n {}Repository: fabiodalez-dev/Pinakes
Length of output: 234
🏁 Script executed:
fd -type f -name "LibriApiController.php" | head -1 | xargs wc -lRepository: fabiodalez-dev/Pinakes
Length of output: 255
🏁 Script executed:
fd -type f -name "LibriApiController.php" | head -1 | xargs cat -n | sed -n '1,150p'Repository: fabiodalez-dev/Pinakes
Length of output: 236
🏁 Script executed:
fd -type f -name "LibriApiController.php" | head -1 | xargs cat -n | sed -n '30,80p'Repository: fabiodalez-dev/Pinakes
Length of output: 236
🏁 Script executed:
fd -type f -name "LibriApiController.php" | head -1 | xargs cat -n | sed -n '125,145p'Repository: fabiodalez-dev/Pinakes
Length of output: 236
🏁 Script executed:
# Also check the hasColumn method implementation in the parent class or trait
rg -n "function hasColumn" -A 10 app/Repository: fabiodalez-dev/Pinakes
Length of output: 2458
🏁 Script executed:
find app/Controllers -name "LibriApiController.php" -exec wc -l {} \;Repository: fabiodalez-dev/Pinakes
Length of output: 108
🏁 Script executed:
find app/Controllers -name "LibriApiController.php" -exec cat -n {} \; | sed -n '1,50p'Repository: fabiodalez-dev/Pinakes
Length of output: 2618
🏁 Script executed:
find app/Controllers -name "LibriApiController.php" -exec cat -n {} \; | sed -n '30,80p'Repository: fabiodalez-dev/Pinakes
Length of output: 2735
🏁 Script executed:
find app/Controllers -name "LibriApiController.php" -exec cat -n {} \; | sed -n '125,150p'Repository: fabiodalez-dev/Pinakes
Length of output: 1408
Guard optional columns in filters to prevent SQL errors on installations with older schemas.
l.parole_chiave, l.sottogenere_id, and l.collana are referenced unconditionally in the search, genre filter, and collana filter. While these columns exist in the current schema, other parts of the codebase (e.g., BookRepository) use hasColumn() to tolerate installations without them. If any are missing, the list endpoint will return 500 errors when the corresponding filter is used.
✅ Suggested guard pattern
if ($search_text !== '') {
$nameExpr = $this->hasTableColumn($db, 'autori', 'cognome')
? "CONCAT(a.nome, ' ', a.cognome)"
: 'a.nome';
- $where .= " AND (l.titolo LIKE ? OR l.sottotitolo LIKE ? OR l.parole_chiave LIKE ? OR EXISTS (SELECT 1 FROM libri_autori la JOIN autori a ON la.autore_id=a.id WHERE la.libro_id=l.id AND $nameExpr LIKE ?)) ";
+ $hasParole = $this->hasColumn($db, 'parole_chiave');
+ $where .= " AND (l.titolo LIKE ? OR l.sottotitolo LIKE ?"
+ . ($hasParole ? " OR l.parole_chiave LIKE ?" : "")
+ . " OR EXISTS (SELECT 1 FROM libri_autori la JOIN autori a ON la.autore_id=a.id WHERE la.libro_id=l.id AND $nameExpr LIKE ?)) ";
$searchParam = '%' . $search_text . '%';
$params[] = $searchParam;
$params[] = $searchParam;
- $params[] = $searchParam;
+ if ($hasParole) { $params[] = $searchParam; }
$params[] = $searchParam;
- $types .= 'ssss';
+ $types .= $hasParole ? 'ssss' : 'sss';
}
if ($genere_id) {
- $where .= ' AND (l.genere_id = ? OR l.sottogenere_id = ? OR l.genere_id IN (SELECT id FROM generi WHERE parent_id = ?))';
+ $hasSottogenere = $this->hasColumn($db, 'sottogenere_id');
+ $where .= ' AND (l.genere_id = ?'
+ . ($hasSottogenere ? ' OR l.sottogenere_id = ?' : '')
+ . ' OR l.genere_id IN (SELECT id FROM generi WHERE parent_id = ?))';
$params[] = $genere_id;
- $params[] = $genere_id;
+ if ($hasSottogenere) { $params[] = $genere_id; }
$params[] = $genere_id;
- $types .= 'iii';
+ $types .= $hasSottogenere ? 'iii' : 'ii';
}
- if ($collana !== '') {
+ if ($collana !== '' && $this->hasColumn($db, 'collana')) {
$where .= " AND l.collana LIKE ?";
$params[] = '%' . $collana . '%';
$types .= 's';
}Also applies to: lines 45, 66, 131–135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Controllers/LibriApiController.php` around lines 34 - 35, The controller
reads/filtering on columns that may be absent (e.g., l.collana, l.parole_chiave,
l.sottogenere_id) causing 500s; update LibriApiController to guard any access or
SQL condition that references those columns (including where you build $collana
and the search/genre/collana filter blocks) by checking column existence first
(reuse the existing BookRepository::hasColumn or the schema builder check) and
only read $q['collana'] or add the corresponding WHERE/SELECT clauses when the
column exists; ensure all places that reference l.parole_chiave,
l.sottogenere_id, and l.collana (the code around $collana, the search filter,
genre filter, and the block at lines ~131–135) follow this pattern so older
schemas don’t trigger queries against missing columns.
Comment on lines
619
to
644
| // Soft-delete: set deleted_at and nullify unique-indexed columns | ||
| // to avoid unique constraint violations on future inserts. | ||
| // Related records are kept for history/integrity (same as single delete). | ||
| $sql = "UPDATE libri SET deleted_at = NOW(), isbn10 = NULL, isbn13 = NULL, ean = NULL WHERE id IN ($placeholders) AND deleted_at IS NULL"; | ||
| $stmt = $db->prepare($sql); | ||
| if (!$stmt) { | ||
| AppLog::error('libri.bulk_delete.prepare_failed', ['error' => $db->error]); | ||
| $response->getBody()->write(json_encode([ | ||
| 'success' => false, | ||
| 'error' => __('Errore interno del database') | ||
| ], JSON_UNESCAPED_UNICODE)); | ||
| return $response->withStatus(500)->withHeader('Content-Type', 'application/json'); | ||
| } | ||
|
|
||
| $stmt->bind_param($types, ...$cleanIds); | ||
| if (!$stmt->execute()) { | ||
| throw new \Exception('Failed to execute book delete: ' . $stmt->error); | ||
| } | ||
| $affected = $stmt->affected_rows; | ||
| $stmt->bind_param($types, ...$cleanIds); | ||
| if (!$stmt->execute()) { | ||
| AppLog::error('libri.bulk_delete.execute_failed', ['error' => $stmt->error]); | ||
| $stmt->close(); | ||
|
|
||
| // Commit transaction | ||
| $db->commit(); | ||
| } catch (\Throwable $e) { | ||
| $db->rollback(); | ||
| AppLog::error('libri.bulk_delete.transaction_failed', ['error' => $e->getMessage()]); | ||
| $response->getBody()->write(json_encode([ | ||
| 'success' => false, | ||
| 'error' => __('Errore interno del database') | ||
| ], JSON_UNESCAPED_UNICODE)); | ||
| return $response->withStatus(500)->withHeader('Content-Type', 'application/json'); | ||
| } | ||
| $affected = $stmt->affected_rows; | ||
| $stmt->close(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for schema definitions and constraints on shelf/position fields
rg -n "collocazione|scaffale_id|mensola_id|posizione_progressiva" --type-list | head -20Repository: fabiodalez-dev/Pinakes
Length of output: 455
🏁 Script executed:
# Look for the schema file and any constraint definitions
find . -type f \( -name "*.sql" -o -name "*migration*" -o -name "*schema*" \) | head -30Repository: fabiodalez-dev/Pinakes
Length of output: 897
🏁 Script executed:
# Locate BookRepository class and its delete method
fd -e php | xargs rg -l "BookRepository" | head -10Repository: fabiodalez-dev/Pinakes
Length of output: 208
🏁 Script executed:
# Search for the single-delete implementation in BookRepository
rg -A 20 "function delete\(|public function delete" --type-list phpRepository: fabiodalez-dev/Pinakes
Length of output: 455
🏁 Script executed:
# Search for schema definitions with shelf fields and constraints
rg -n "collocazione|scaffale_id|mensola_id|posizione_progressiva" installer/database/schema.sqlRepository: fabiodalez-dev/Pinakes
Length of output: 1304
🏁 Script executed:
# Look for unique constraints on these fields
rg -n "UNIQUE|PRIMARY KEY" installer/database/schema.sql | grep -i "libri"Repository: fabiodalez-dev/Pinakes
Length of output: 48
🏁 Script executed:
# Extract the BookRepository delete method
cat -n app/Models/BookRepository.php | sed -n '1,50p'Repository: fabiodalez-dev/Pinakes
Length of output: 1979
🏁 Script executed:
# Search for the delete method in BookRepository
rg -n "function delete\|public.*delete" app/Models/BookRepository.phpRepository: fabiodalez-dev/Pinakes
Length of output: 48
🏁 Script executed:
# Get the full schema for libri table
rg -A 30 "CREATE TABLE.*libri" installer/database/schema.sqlRepository: fabiodalez-dev/Pinakes
Length of output: 5642
🏁 Script executed:
# Search for delete method in BookRepository with extended context
rg -n "public.*delete" app/Models/BookRepository.phpRepository: fabiodalez-dev/Pinakes
Length of output: 111
🏁 Script executed:
# Get the full BookRepository file to find the delete method
wc -l app/Models/BookRepository.phpRepository: fabiodalez-dev/Pinakes
Length of output: 100
🏁 Script executed:
# Extract the delete method from BookRepository
sed -n '1,500p' app/Models/BookRepository.php | tail -300Repository: fabiodalez-dev/Pinakes
Length of output: 12865
🏁 Script executed:
# Extract the delete method starting from line 900
sed -n '900,950p' app/Models/BookRepository.phpRepository: fabiodalez-dev/Pinakes
Length of output: 2697
Bulk delete should clear shelf-location fields to match single-delete behavior.
BookRepository::delete() nullifies shelf fields (scaffale_id, mensola_id, posizione_progressiva, collocazione) to free positions. The bulk delete path only clears ISBN/EAN, creating an inconsistency that leaves stale shelf locations and could cause data integrity issues. Align the bulk update with single-delete behavior by adding the shelf fields.
Suggested alignment with single delete
- $sql = "UPDATE libri SET deleted_at = NOW(), isbn10 = NULL, isbn13 = NULL, ean = NULL WHERE id IN ($placeholders) AND deleted_at IS NULL";
+ $sql = "UPDATE libri SET deleted_at = NOW(), isbn10 = NULL, isbn13 = NULL, ean = NULL,
+ scaffale_id = NULL, mensola_id = NULL, posizione_progressiva = NULL, collocazione = NULL
+ WHERE id IN ($placeholders) AND deleted_at IS NULL";📝 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
| // Soft-delete: set deleted_at and nullify unique-indexed columns | |
| // to avoid unique constraint violations on future inserts. | |
| // Related records are kept for history/integrity (same as single delete). | |
| $sql = "UPDATE libri SET deleted_at = NOW(), isbn10 = NULL, isbn13 = NULL, ean = NULL WHERE id IN ($placeholders) AND deleted_at IS NULL"; | |
| $stmt = $db->prepare($sql); | |
| if (!$stmt) { | |
| AppLog::error('libri.bulk_delete.prepare_failed', ['error' => $db->error]); | |
| $response->getBody()->write(json_encode([ | |
| 'success' => false, | |
| 'error' => __('Errore interno del database') | |
| ], JSON_UNESCAPED_UNICODE)); | |
| return $response->withStatus(500)->withHeader('Content-Type', 'application/json'); | |
| } | |
| $stmt->bind_param($types, ...$cleanIds); | |
| if (!$stmt->execute()) { | |
| throw new \Exception('Failed to execute book delete: ' . $stmt->error); | |
| } | |
| $affected = $stmt->affected_rows; | |
| $stmt->bind_param($types, ...$cleanIds); | |
| if (!$stmt->execute()) { | |
| AppLog::error('libri.bulk_delete.execute_failed', ['error' => $stmt->error]); | |
| $stmt->close(); | |
| // Commit transaction | |
| $db->commit(); | |
| } catch (\Throwable $e) { | |
| $db->rollback(); | |
| AppLog::error('libri.bulk_delete.transaction_failed', ['error' => $e->getMessage()]); | |
| $response->getBody()->write(json_encode([ | |
| 'success' => false, | |
| 'error' => __('Errore interno del database') | |
| ], JSON_UNESCAPED_UNICODE)); | |
| return $response->withStatus(500)->withHeader('Content-Type', 'application/json'); | |
| } | |
| $affected = $stmt->affected_rows; | |
| $stmt->close(); | |
| // Soft-delete: set deleted_at and nullify unique-indexed columns | |
| // to avoid unique constraint violations on future inserts. | |
| // Related records are kept for history/integrity (same as single delete). | |
| $sql = "UPDATE libri SET deleted_at = NOW(), isbn10 = NULL, isbn13 = NULL, ean = NULL, | |
| scaffale_id = NULL, mensola_id = NULL, posizione_progressiva = NULL, collocazione = NULL | |
| WHERE id IN ($placeholders) AND deleted_at IS NULL"; | |
| $stmt = $db->prepare($sql); | |
| if (!$stmt) { | |
| AppLog::error('libri.bulk_delete.prepare_failed', ['error' => $db->error]); | |
| $response->getBody()->write(json_encode([ | |
| 'success' => false, | |
| 'error' => __('Errore interno del database') | |
| ], JSON_UNESCAPED_UNICODE)); | |
| return $response->withStatus(500)->withHeader('Content-Type', 'application/json'); | |
| } | |
| $stmt->bind_param($types, ...$cleanIds); | |
| if (!$stmt->execute()) { | |
| AppLog::error('libri.bulk_delete.execute_failed', ['error' => $stmt->error]); | |
| $stmt->close(); | |
| $response->getBody()->write(json_encode([ | |
| 'success' => false, | |
| 'error' => __('Errore interno del database') | |
| ], JSON_UNESCAPED_UNICODE)); | |
| return $response->withStatus(500)->withHeader('Content-Type', 'application/json'); | |
| } | |
| $affected = $stmt->affected_rows; | |
| $stmt->close(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Controllers/LibriApiController.php` around lines 619 - 644, The
bulk-delete SQL in LibriApiController currently only nulls isbn10/isbn13/ean;
update the $sql UPDATE statement used before $db->prepare(...) to also set
scaffale_id = NULL, mensola_id = NULL, posizione_progressiva = NULL, and
collocazione = NULL so it matches BookRepository::delete() behavior; keep the
same WHERE id IN ($placeholders) AND deleted_at IS NULL and leave the existing
prepare/ bind_param/ execute/ error handling in place.
| "main": "index.js", | ||
| "scripts": { | ||
| "test": "echo \"Error: no test specified\" && exit 1", | ||
| "test": "playwright test --config=tests/playwright.config.js", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the Playwright config path referenced by npm script exists
fd -t f 'playwright.config.js'Repository: fabiodalez-dev/Pinakes
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Check the package.json content around the test script and line 37
cat -n package.json | head -50Repository: fabiodalez-dev/Pinakes
Length of output: 1409
🏁 Script executed:
#!/bin/bash
# Search for `@playwright/test` in devDependencies
rg -n "@playwright/test" package.jsonRepository: fabiodalez-dev/Pinakes
Length of output: 103
Playwright devDependency added correctly, but test script references missing config file.
The @playwright/test devDependency on line 37 is correctly specified at version ^1.58.2. However, the test script on line 7 references tests/playwright.config.js, which does not exist in the repository. This will cause the npm test command to fail. Create the config file at the specified path or update the script to reference the correct location.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 7, The package.json test script ("test": "playwright
test --config=tests/playwright.config.js") points to a non-existent config file;
either create the config file at tests/playwright.config.js or update the script
to use the correct config path (for example remove --config or point to an
existing playwright.config.js). Locate the "test" script in package.json and
either add the missing tests/playwright.config.js with your Playwright
configuration or change the script to "playwright test" or
"--config=playwright.config.js" so the npm test command runs against an actual
config. Ensure the chosen path matches the repository layout and that
`@playwright/test` remains in devDependencies.
Comment on lines
111
to
114
| // 1. Hash password via PHP | ||
| const hash = execFileSync('php', [ | ||
| '-r', "echo password_hash('Test1234!', PASSWORD_DEFAULT);", | ||
| ], { encoding: 'utf-8' }).trim(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, get the full context of the file around lines 111-114
head -120 tests/loan-reservation.spec.js | tail -20Repository: fabiodalez-dev/Pinakes
Length of output: 826
🏁 Script executed:
#!/bin/bash
# Search for TEST_PASS and password_hash usage
rg -n "TEST_PASS" tests/loan-reservation.spec.js -B 2 -A 2Repository: fabiodalez-dev/Pinakes
Length of output: 665
🏁 Script executed:
#!/bin/bash
# Search for password_hash usage
rg -n "password_hash" tests/loan-reservation.spec.js -B 2 -A 2Repository: fabiodalez-dev/Pinakes
Length of output: 261
🏁 Script executed:
#!/bin/bash
# Get the beginning of the file to see how TEST_PASS is initialized
head -50 tests/loan-reservation.spec.jsRepository: fabiodalez-dev/Pinakes
Length of output: 2056
Password hash must use overridable TEST_PASS constant.
TEST_PASS can be overridden via the E2E_TEST_PASS environment variable (line 9), but the password hash at line 113 is hard-coded to 'Test1234!'. When the test user logs in at line 193, it will use the potentially overridden password, causing authentication to fail if E2E_TEST_PASS differs from the hard-coded value.
🛠️ Suggested fix
- const hash = execFileSync('php', [
- '-r', "echo password_hash('Test1234!', PASSWORD_DEFAULT);",
- ], { encoding: 'utf-8' }).trim();
+ const passLiteral = JSON.stringify(TEST_PASS);
+ const hash = execFileSync('php', [
+ '-r', `echo password_hash(${passLiteral}, PASSWORD_DEFAULT);`,
+ ], { encoding: 'utf-8' }).trim();📝 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
| // 1. Hash password via PHP | |
| const hash = execFileSync('php', [ | |
| '-r', "echo password_hash('Test1234!', PASSWORD_DEFAULT);", | |
| ], { encoding: 'utf-8' }).trim(); | |
| // 1. Hash password via PHP | |
| const passLiteral = JSON.stringify(TEST_PASS); | |
| const hash = execFileSync('php', [ | |
| '-r', `echo password_hash(${passLiteral}, PASSWORD_DEFAULT);`, | |
| ], { encoding: 'utf-8' }).trim(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/loan-reservation.spec.js` around lines 111 - 114, The password hash
generation is using a hard-coded 'Test1234!' while the test uses the overridable
TEST_PASS (from E2E_TEST_PASS); update the execFileSync call that builds the PHP
password_hash command to use the TEST_PASS variable instead of the literal
string so the hashed password matches the login step (refer to TEST_PASS and the
execFileSync invocation that runs "php -r \"echo password_hash(...);\"").
Comment on lines
39
to
41
| test('returns 403 with wrong key', async ({ request }) => { | ||
| const resp = await request.get(`${BASE}/manual-update.php?key=wrong_key_12345`); | ||
| expect([403, 404, 302, 301]).toContain(resp.status()); |
There was a problem hiding this comment.
Gitleaks false-positive risk on the test key string.
Static analysis flagged Line 40 as a “generic API key”; if gitleaks is enforced, this can fail CI. Consider using a less key-like placeholder or allowlisting the test string.
🛠️ Suggested tweak
- const resp = await request.get(`${BASE}/manual-update.php?key=wrong_key_12345`);
+ const resp = await request.get(`${BASE}/manual-update.php?key=invalid_key_for_test`);🧰 Tools
🪛 Gitleaks (8.30.0)
[high] 40-40: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/security-hardening.spec.js` around lines 39 - 41, The test string
"wrong_key_12345" triggers gitleaks; update the test in the "returns 403 with
wrong key" test to use a non key-looking placeholder or an obfuscated
construction to avoid the detector — e.g. replace the literal in the request URL
`${BASE}/manual-update.php?key=wrong_key_12345` with a safer value like
"wrong_key_placeholder" or build it at runtime (e.g.
['wrong','key','12345'].join('_')) so the symbol BASE and the request call
remain the same but the static pattern no longer matches.
Cover 7 feature groups: public frontend (catalog, events, contact form, author archive), user profile & dashboard (stats, profile edit, password change, wishlist), admin user management, book management (copies, loan renew), admin events & reviews, infrastructure (shelving, statistics, password reset), and messages & theme.
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
public/.htaccess (1)
70-83:⚠️ Potential issue | 🟠 MajorWildcard CORS + Apache direct-serving of
/uploads/bypasses application-level access control.Lines 25–27 short-circuit all
/uploads/requests to static file serving, meaning PHP never runs for these URLs. Combined withAccess-Control-Allow-Origin: "*"on every audio/video file (lines 70–77) and the entire/uploads/digital/path (lines 80–83), any cross-origin client that knows or guesses a file URL can fetch it directly — no loan check, no authentication.If DigitalLibraryPlugin files (ebooks, audiobooks) should be loan-restricted, direct Apache serving must be replaced with a PHP proxy that validates the session/loan before streaming, or at minimum the CORS wildcard must be scoped to only public/sample content paths rather than all of
/uploads/digital/.🔒 Suggested mitigation options
Option A – PHP streaming proxy (recommended for access-controlled content):
-# Allow direct access to static files in /uploads/ directory -RewriteCond %{REQUEST_URI} ^/uploads/ [NC] -RewriteRule ^ - [L] +# Route digital-library protected content through PHP auth proxy +RewriteCond %{REQUEST_URI} ^/uploads/digital/ [NC] +RewriteRule ^ index.php [QSA,L] + +# Public uploads (covers, etc.) still served directly +RewriteCond %{REQUEST_URI} ^/uploads/ [NC] +RewriteRule ^ - [L]Option B – Restrict CORS to non-restricted paths only (if all digital content is public):
-<If "%{REQUEST_URI} =~ m#^/uploads/digital/#"> - Header set Access-Control-Allow-Origin "*" - Header set Access-Control-Allow-Methods "GET, OPTIONS" -</If> +# Only if digital content is freely accessible (no loan/auth enforcement) +<If "%{REQUEST_URI} =~ m#^/uploads/digital/samples/#"> + Header set Access-Control-Allow-Origin "*" + Header set Access-Control-Allow-Methods "GET, OPTIONS" +</If>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/.htaccess` around lines 70 - 83, The current .htaccess exposes all audio/video under FilesMatch and the /uploads/digital/ If block via Header set Access-Control-Allow-Origin "*", allowing cross-origin access to potentially protected files; either (A) remove the wildcard CORS headers from the FilesMatch and the If "%{REQUEST_URI} =~ m#^/uploads/digital/#" block and instead add CORS only to explicitly public/sample paths (e.g. /uploads/digital/public/ or /uploads/digital/samples/) by creating a new FilesMatch/If that targets those paths, or (B) stop Apache from serving /uploads/digital/ directly and route those requests to a PHP streaming proxy that performs session/loan validation before streaming (disable the static shortcut that currently bypasses PHP), while keeping Accept-Ranges where needed; update/removethe Header set Access-Control-Allow-Origin "*" and related Header set lines accordingly.
♻️ Duplicate comments (17)
storage/plugins/digital-library/DigitalLibraryPlugin.php (5)
197-211: Bothprepare()andget_result()guards are now correctly handled.Logging +
continueon failure, and SELECT stmt is explicitly closed before reassignment at line 211.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/digital-library/DigitalLibraryPlugin.php` around lines 197 - 211, In DigitalLibraryPlugin adjust the resource cleanup: when you call $stmt->get_result() in the hook check code, ensure you free the mysqli_result (use $result->free()) and always close the statement ($stmt->close()) on every early exit path and after successful processing to avoid leaks; reference the $stmt variable and $result from the prepare()/get_result() block in the DigitalLibraryPlugin class and add the missing $result->free() and consistent $stmt->close() calls where you currently continue or finish handling the hook.
62-76: Previously flagged Apache 2.4 compatibility is now correctly addressed.Both
mod_authz_core.c(Require all denied) andmod_access_compat.c(Order Deny,Allow/Deny from all) are present, covering both Apache 2.4+ and legacy 2.2 deployments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/digital-library/DigitalLibraryPlugin.php` around lines 62 - 76, The htaccess generation block using file_put_contents($htaccess, implode("\n", [...])) correctly includes both mod_authz_core.c (Require all denied) and mod_access_compat.c (Order Deny,Allow / Deny from all) inside the '<FilesMatch "\.(php\d?|phtml|phar|pl|py|cgi|sh)$">' section, so no code change is required; leave the file_put_contents call and the FilesMatch block as-is to maintain Apache 2.4+ and legacy 2.2 compatibility.
547-555: JSON fallback now correctly guards againstjson_encodereturningfalse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/digital-library/DigitalLibraryPlugin.php` around lines 547 - 555, The JSON encoding fallback must guard against json_encode returning false; update the error-handling path in the DigitalLibraryPlugin (the try/catch around json_encode and the catch block that calls \App\Support\SecureLogger::error) so that when json_encode(['success' => false, 'message' => 'Internal error']) could return false you instead produce a valid JSON string (e.g. use a coalescing/ternary to fall back to a literal '{"success":false,"message":"Internal error"}') before calling $response->getBody()->write and returning the response with Content-Type application/json and the given $status.
389-414: HTTP 500 vs. 400 distinction for server-side upload errors is now correctly handled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/digital-library/DigitalLibraryPlugin.php` around lines 389 - 414, The HTTP status mapping for upload errors is correct: keep the match on $errorCode that sets $status (using UPLOAD_ERR_NO_TMP_DIR and UPLOAD_ERR_CANT_WRITE => 500, default => 400), preserve the $message match on $errorCode and the SecureLogger::warning call that logs 'error_code' and 'filename', and return via $this->json($response, ['success' => false, 'message' => $message], $status) as-is; no code change required beyond ensuring those symbols ($errorCode, $status, match expressions, SecureLogger::warning, and return $this->json) remain intact.
94-96:prepare()guard on deactivation is now in place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/digital-library/DigitalLibraryPlugin.php` around lines 94 - 96, The prepare() guard currently returns early when $stmt === false but doesn't report or handle the failure; in the DigitalLibraryPlugin::deactivate (or the method that calls prepare on the DB) add error handling when $stmt is false: log or throw a descriptive error using the plugin's logger (or trigger_error) including $this->db->error (or mysqli->error) and then return to avoid proceeding; ensure any later calls that assume $stmt (e.g., $stmt->bind_param, $stmt->execute, $stmt->close) are not executed when prepare() failed.app/Views/frontend/home.php (1)
891-900:⚠️ Potential issue | 🟠 MajorAvailable-books stat still shows a placeholder emoji.
Consider wiring this to an API count (e.g.,
pagination.available_books) or revise the UI to avoid implying a numeric metric.💡 Suggested UI adjustment
- totalBooksEl.textContent = data.pagination.total_books; - availableBooksEl.textContent = '\uD83D\uDCDA'; + totalBooksEl.textContent = data.pagination?.total_books ?? '0'; + availableBooksEl.textContent = data.pagination?.available_books ?? '0';#!/bin/bash # Verify catalog API response includes an available books count rg -n "function\s+catalogAPI" app/Controllers/FrontendController.php -A160 rg -n "available_books" app/Controllers/FrontendController.php🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/frontend/home.php` around lines 891 - 900, The available-books element is still using a placeholder emoji; update the fetch handler that uses API_CATALOG_ROUTE so availableBooksEl.textContent is set from the real API field (e.g., data.pagination.available_books) instead of the hardcoded '\uD83D\uDCDA', and update the .catch fallback to a sensible default (either a numeric 0 or a UI placeholder) so totalBooksEl and availableBooksEl both show real counts or a clear non-numeric fallback; locate the logic around totalBooksEl, availableBooksEl and the fetch(API_CATALOG_ROUTE) promise chain to make this change.public/index.php (1)
399-401:⚠️ Potential issue | 🟡 MinorMake sure dev/staging explicitly set
displayErrorDetails.Defaulting to
falsehides stack traces unless the setting is configured in dev/staging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/index.php` around lines 399 - 401, Currently $displayErrorDetails falls back to false which can hide stack traces; explicitly set displayErrorDetails for dev/staging by reading an environment/app-env and/or enforcing the setting in your configuration instead of a silent default. Update the logic around $displayErrorDetails = $container->get('settings')['displayErrorDetails'] ?? false so that for APP_ENV (or equivalent) values 'development' or 'staging' it yields true (or require the setting to exist and error if missing), and then pass that explicit value into $errorMiddleware = $app->addErrorMiddleware($displayErrorDetails, true, true) so dev/staging always enable detailed errors.package.json (1)
7-7:⚠️ Potential issue | 🟡 MinorVerify the Playwright config path exists.
The test script now points to
tests/playwright.config.js; if that file isn’t present,npm testwill fail. Please confirm the config path is correct.#!/bin/bash # Verify that the Playwright config referenced by package.json exists fd -t f 'playwright.config.js' .Also applies to: 36-38
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 7, The package.json "test" script references tests/playwright.config.js which may not exist; verify that the Playwright config file path is correct and either (a) create or move the Playwright config to tests/playwright.config.js, or (b) update the package.json "test" script value to the actual config path (e.g., playwright.config.js or the correct relative path) so running npm test won't fail; ensure the change is made to the "test" script entry in package.json.scripts/manual-update.php (1)
19-25:⚠️ Potential issue | 🟠 MajorFix .env lookup when the script is uploaded to the project root.
The doc says to upload the script to the root, but the fallback only checks
../.env, so the emergency key won’t be found when.envlives alongside the script. That breaks key-based access in the most common deployment path.🔧 Proposed fix
-$secretKey = getenv('MANUAL_UPDATE_KEY') ?: ( - file_exists(__DIR__ . '/../.env') ? - (function() { - $env = file_get_contents(__DIR__ . '/../.env'); - preg_match('/^MANUAL_UPDATE_KEY=(.*)$/m', $env, $m); - return trim(trim($m[1] ?? ''), '"\''); - })() : '' -); +$secretKey = getenv('MANUAL_UPDATE_KEY') ?: (function () { + $paths = [__DIR__ . '/../.env', __DIR__ . '/.env']; + foreach ($paths as $path) { + if (!file_exists($path)) { + continue; + } + $env = file_get_contents($path); + if ($env === false) { + continue; + } + if (preg_match('/^MANUAL_UPDATE_KEY=(.*)$/m', $env, $m)) { + return trim(trim($m[1] ?? ''), '"\''); + } + } + return ''; +})();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/manual-update.php` around lines 19 - 25, The fallback .env lookup only checks __DIR__ . '/../.env' so when the script is uploaded to the project root it misses a same-directory .env; update the $secretKey resolution (starting at getenv('MANUAL_UPDATE_KEY') ...) to also check __DIR__ . '/.env' (prefer current-dir then parent-dir) and read the MANUAL_UPDATE_KEY from whichever .env exists before falling back to an empty string, keeping the existing preg_match/trim logic to extract the value.tests/loan-reservation.spec.js (1)
111-114:⚠️ Potential issue | 🟠 MajorUse
TEST_PASSwhen generating the password hash.The hash is hard-coded to
"Test1234!"while login uses the overridableTEST_PASS, so overrides will fail authentication.🛠️ Suggested fix
- const hash = execFileSync('php', [ - '-r', "echo password_hash('Test1234!', PASSWORD_DEFAULT);", - ], { encoding: 'utf-8' }).trim(); + const passLiteral = JSON.stringify(TEST_PASS); + const hash = execFileSync('php', [ + '-r', `echo password_hash(${passLiteral}, PASSWORD_DEFAULT);`, + ], { encoding: 'utf-8' }).trim();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/loan-reservation.spec.js` around lines 111 - 114, The password hash generation currently hard-codes "Test1234!" which breaks overrides; change the execFileSync call in tests/loan-reservation.spec.js to use the TEST_PASS value (e.g., process.env.TEST_PASS or the TEST_PASS constant used elsewhere in the test) instead of the literal string so the generated hash matches the credential used by the login flow; keep the same execFileSync invocation, encoding and trim logic but substitute the literal with TEST_PASS.tests/security-hardening.spec.js (1)
39-41:⚠️ Potential issue | 🟠 MajorAvoid a gitleaks‑triggering test key literal.
The
wrong_key_12345string can trip secret scanners; use a less key‑like placeholder.🛠️ Suggested fix
- const resp = await request.get(`${BASE}/manual-update.php?key=wrong_key_12345`); + const resp = await request.get(`${BASE}/manual-update.php?key=invalid_key_for_test`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/security-hardening.spec.js` around lines 39 - 41, Replace the gitleaks‑triggering key literal used in the test named "returns 403 with wrong key" with a non key‑like placeholder constant and use that constant in the request URL; specifically update the string "wrong_key_12345" referenced in the test (inside the request.get call) to a harmless name (e.g., INVALID_KEY or "placeholder-value") so the test still asserts the same statuses via resp.status() but no longer contains a key-like literal.app/Models/BookRepository.php (1)
980-1044: Consider building the hierarchy from already-joined genre fields to avoid extra queries.The resolver now performs per-ancestor SELECTs even though
getById()already joinsg/gp/gpp. Reusing those fields first would avoid extra round-trips for the common ≤3-level case.♻️ Possible refactor sketch
- // Walk up the tree from genere_id to collect the full ancestor chain - $chain = []; - $currentId = $genereId; + // Build chain from JOINed fields first (≤3 levels), then fall back to DB walk + $chain = []; + if (!empty($row['nonno_id'])) { + $chain[] = ['id' => (int) $row['nonno_id'], 'nome' => $row['nonno_nome'] ?? '']; + $chain[] = ['id' => (int) $row['radice_id'], 'nome' => $row['radice_nome'] ?? '']; + $chain[] = ['id' => (int) ($row['genere_id_resolved'] ?? $genereId), 'nome' => $row['genere_nome'] ?? '']; + } elseif (!empty($row['radice_id'])) { + $chain[] = ['id' => (int) $row['radice_id'], 'nome' => $row['radice_nome'] ?? '']; + $chain[] = ['id' => (int) ($row['genere_id_resolved'] ?? $genereId), 'nome' => $row['genere_nome'] ?? '']; + } + $currentId = $genereId;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/BookRepository.php` around lines 980 - 1044, The resolver is issuing extra SELECTs to walk ancestors even though getById() already joins genre ancestors (g/gp/gpp); modify the logic in BookRepository (the method that builds $chain from $genereId and populates $row fields) to first check for the joined genre columns on $row (e.g. g_nome/g_id, gp_nome/gp_id, gpp_nome/gpp_id or whatever the existing joined field names are) and build the $chain and cascade values from those when present (covering the common 1–3 level cases), and only fall back to the current prepared-statement ancestor walk (the $stmt loop that reads generi by id into $chain) if the joined fields are missing or incomplete.app/Controllers/LibriApiController.php (2)
619-644:⚠️ Potential issue | 🟠 MajorBulk delete should clear shelf-location fields to match single-delete behavior.
Otherwise stale collocazione/scaffale data can block reuse of positions.
🛠️ Align bulk delete with single delete
- $sql = "UPDATE libri SET deleted_at = NOW(), isbn10 = NULL, isbn13 = NULL, ean = NULL WHERE id IN ($placeholders) AND deleted_at IS NULL"; + $sql = "UPDATE libri SET deleted_at = NOW(), isbn10 = NULL, isbn13 = NULL, ean = NULL, + scaffale_id = NULL, mensola_id = NULL, posizione_progressiva = NULL, collocazione = NULL + WHERE id IN ($placeholders) AND deleted_at IS NULL";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibriApiController.php` around lines 619 - 644, The bulk-delete SQL only nulls isbn10/isbn13/ean but must also clear shelf-location fields to match single-delete; update the $sql used in LibriApiController (the statement using $placeholders/$stmt) to set the collocazione and scaffale columns to NULL as well (e.g. add ", collocazione = NULL, scaffale = NULL" to the SET clause), keep the existing prepare/bind_param/execute flow with $types and $cleanIds unchanged, and ensure error logging around $stmt remains as-is.
34-35:⚠️ Potential issue | 🟠 MajorGuard optional columns in list filters to avoid SQL errors on older schemas.
l.parole_chiave,l.sottogenere_id, andl.collanaare referenced unconditionally. Installations missing these columns will error when filters are used.🛠️ Suggested guard pattern
+ $hasParole = $this->hasColumn($db, 'parole_chiave'); + $hasSottogenere = $this->hasColumn($db, 'sottogenere_id'); + $hasCollana = $this->hasColumn($db, 'collana'); if ($search_text !== '') { $nameExpr = $this->hasTableColumn($db, 'autori', 'cognome') ? "CONCAT(a.nome, ' ', a.cognome)" : 'a.nome'; - $where .= " AND (l.titolo LIKE ? OR l.sottotitolo LIKE ? OR l.parole_chiave LIKE ? OR EXISTS (SELECT 1 FROM libri_autori la JOIN autori a ON la.autore_id=a.id WHERE la.libro_id=l.id AND $nameExpr LIKE ?)) "; + $where .= " AND (l.titolo LIKE ? OR l.sottotitolo LIKE ?" + . ($hasParole ? " OR l.parole_chiave LIKE ?" : "") + . " OR EXISTS (SELECT 1 FROM libri_autori la JOIN autori a ON la.autore_id=a.id WHERE la.libro_id=l.id AND $nameExpr LIKE ?)) "; $searchParam = '%' . $search_text . '%'; $params[] = $searchParam; $params[] = $searchParam; - $params[] = $searchParam; + if ($hasParole) { $params[] = $searchParam; } $params[] = $searchParam; - $types .= 'ssss'; + $types .= $hasParole ? 'ssss' : 'sss'; } if ($genere_id) { - $where .= ' AND (l.genere_id = ? OR l.sottogenere_id = ? OR l.genere_id IN (SELECT id FROM generi WHERE parent_id = ?))'; + $where .= ' AND (l.genere_id = ?' + . ($hasSottogenere ? ' OR l.sottogenere_id = ?' : '') + . ' OR l.genere_id IN (SELECT id FROM generi WHERE parent_id = ?))'; $params[] = $genere_id; - $params[] = $genere_id; + if ($hasSottogenere) { $params[] = $genere_id; } $params[] = $genere_id; - $types .= 'iii'; + $types .= $hasSottogenere ? 'iii' : 'ii'; } - if ($sottogenere_id) { + if ($sottogenere_id && $hasSottogenere) { $where .= ' AND l.sottogenere_id = ?'; $params[] = $sottogenere_id; $types .= 'i'; } - if ($collana !== '') { + if ($collana !== '' && $hasCollana) { $where .= " AND l.collana LIKE ?"; $params[] = '%' . $collana . '%'; $types .= 's'; }Also applies to: 41-51, 61-75, 131-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibriApiController.php` around lines 34 - 35, In LibriApiController, guard use of optional columns (l.parole_chiave, l.sottogenere_id, l.collana) when building list filters: before adding any WHERE or SELECT that references these columns (e.g. where you read $q and set $collana) check the schema with Schema::hasColumn('libri', 'collana') / Schema::hasColumn('libri', 'sottogenere_id') / Schema::hasColumn('libri', 'parole_chiave') and only append the corresponding filter logic/joins when the column exists and the query param (like $q['collana']) is non-empty; update all filter blocks referenced (the areas around the $collana assignment and the blocks at the other noted ranges) to use these conditional checks so older schemas won't cause SQL errors.app/Views/libri/index.php (2)
639-645:⚠️ Potential issue | 🟠 MajorCollana filter still locked to the initial URL value.
collanais sent fromcollanaFilter, which is only initialized from the URL and never updated from any UI field. If a collana autocomplete/input exists, user changes won’t affect results. Consider reading the live input value (or updatingcollanaFilterin its handlers) and clearing it alongside other filters.🛠️ Possible fix (adjust input ID to the actual collana field)
data: function(d) { + const collanaInput = document.getElementById('filter_collana'); return { ...d, @@ - collana: collanaFilter + collana: (collanaInput ? collanaInput.value : collanaFilter) || '' }; },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/libri/index.php` around lines 639 - 645, The collana filter is always sent from the initial collanaFilter (URL-only) and never reads the live input; update the filter-building code to read the current collana input value (or ensure the collanaFilter variable is updated in the collana input/autocomplete handlers) before sending — locate the place where the filters object is created (uses keys collana: collanaFilter, genere_filter, etc.) and replace or augment collana: collanaFilter with the actual DOM value (e.g., document.getElementById(...) or the component's current value) and also clear/reset collanaFilter together with the other filters where you reset genere_filter/posizione_id/anno_from/anno_to.
477-483:⚠️ Potential issue | 🟡 MinorClosing the URL banner shouldn’t wipe user‑edited keywords.
The close handler clearssearch_textwheneverinitialKeywordsis set, even if the user has since typed a different search. That loses user input unexpectedly. Consider clearing only if the current value still matches the URL‑supplied value.✅ Suggested tweak
closeBtn.addEventListener('click', function() { collanaFilter = ''; genereUrlFilter = 0; sottogenereUrlFilter = 0; - if (initialKeywords) { - document.getElementById('search_text').value = ''; - } + if (initialKeywords) { + const searchInput = document.getElementById('search_text'); + if (searchInput.value === initialKeywords) { + searchInput.value = ''; + } + } banner.remove(); table.ajax.reload(); updateActiveFilters(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/libri/index.php` around lines 477 - 483, The closeBtn.addEventListener handler currently resets collanaFilter, genereUrlFilter, sottogenereUrlFilter and unconditionally clears document.getElementById('search_text').value when initialKeywords is truthy; change this so it only clears the search_text if its current value equals the URL-supplied initialKeywords (e.g. compare document.getElementById('search_text').value to initialKeywords, optionally trim both) so user-typed changes are preserved; keep the rest of the filter resets as-is and only perform the clear when the values match.app/Controllers/GeneriController.php (1)
154-155:⚠️ Potential issue | 🟡 MinorUnused
$requestindestroy()triggers PHPMD.
Rename to$_requestor remove it if the framework allows, to silence the warning.🧹 Minimal fix (if signature must stay)
- public function destroy(Request $request, Response $response, \mysqli $db, int $id): Response + public function destroy(Request $_request, Response $response, \mysqli $db, int $id): Response🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/GeneriController.php` around lines 154 - 155, The destroy() method signature declares a Request $request parameter that is unused and triggers PHPMD; update the method signature for destroy(Request $request, Response $response, \mysqli $db, int $id): Response by either removing the $request parameter entirely if the framework allows or renaming it to $_request (or $_unusedRequest) to silence the PHPMD unused variable warning while keeping the same signature.
🧹 Nitpick comments (6)
storage/plugins/digital-library/DigitalLibraryPlugin.php (2)
229-234:execute()return value for the INSERT hook is unchecked — silent failures.If the INSERT fails (e.g., duplicate key race between two concurrent activations, constraint violation),
$stmt->execute()returnsfalsesilently with no log and no error propagation.🛡️ Proposed fix
$stmt->bind_param( "isssii", $this->pluginId, $hook['hook_name'], $hook['callback_class'], $hook['callback_method'], $hook['priority'], $hook['is_active'] ); - $stmt->execute(); + if (!$stmt->execute()) { + \App\Support\SecureLogger::error('DigitalLibraryPlugin: execute() failed for hook insert', [ + 'hook' => $hook['hook_name'], + 'error' => $stmt->error, + ]); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/digital-library/DigitalLibraryPlugin.php` around lines 229 - 234, The INSERT's $stmt->execute() call in DigitalLibraryPlugin.php is unchecked and can fail silently; update the code around the INSERT (the block that prepares and executes $stmt for the hook activation) to check the boolean return of $stmt->execute(), log the error using available logger/error info (e.g., $stmt->error or $this->logger) and propagate or handle the failure (return false / throw an exception) instead of proceeding as if success—include contextual info (hook id/name and error/errno) in the log so races/constraint violations are diagnosable.
507-510: Preferstr_starts_withoverstrpos(...) !== 0for the path-traversal guard.
strposreturnsint|false; while the strict!== 0comparison is logically correct (sincefalse !== 0istruein strict mode), usingstr_starts_with(available in PHP 8.0+) reads more clearly and better expresses intent.♻️ Proposed refactor
- if ($filePath === false || strpos($filePath, $baseRealPath . DIRECTORY_SEPARATOR) !== 0 || !is_file($filePath)) { + if ($filePath === false || !str_starts_with($filePath, $baseRealPath . DIRECTORY_SEPARATOR) || !is_file($filePath)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/digital-library/DigitalLibraryPlugin.php` around lines 507 - 510, Replace the strpos-based path traversal check in the DigitalLibraryPlugin path-guard with str_starts_with for clarity: after computing $filePath (realpath($baseRealPath . DIRECTORY_SEPARATOR . $filename)), keep the existing checks for $filePath === false and is_file($filePath), but replace the strpos(...) !== 0 condition with !str_starts_with($filePath, $baseRealPath . DIRECTORY_SEPARATOR) so the intent is explicit and safer on PHP 8+; update the conditional that returns $response->withStatus(404) accordingly.tests/playwright.config.js (2)
10-10: Consider makingbaseURLconfigurable via an environment variable for CI portability.♻️ Proposed fix
- baseURL: 'http://localhost:8081', + baseURL: process.env.APP_URL ?? 'http://localhost:8081',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/playwright.config.js` at line 10, The hardcoded baseURL in tests/playwright.config.js should be made configurable via an environment variable; replace the literal 'http://localhost:8081' assigned to baseURL with a value read from process.env (e.g., process.env.PLAYWRIGHT_BASE_URL or process.env.BASE_URL) and fall back to the current default if the env var is not set, updating the exported Playwright config object so tests use the env-driven baseURL.
10-10: HardcodedbaseURL— consider making it configurable via environment variable.
http://localhost:8081won't work in CI environments where the port may differ (e.g.,8080, a random port, or a container hostname).♻️ Proposed fix
- baseURL: 'http://localhost:8081', + baseURL: process.env.APP_URL || 'http://localhost:8081',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/playwright.config.js` at line 10, The config hardcodes baseURL ('http://localhost:8081'), which breaks CI; change the baseURL assignment in tests/playwright.config.js to read from an environment variable (e.g., process.env.PLAYWRIGHT_BASE_URL) with a sensible fallback (like 'http://localhost:8081') so CI or containers can override the host/port; update any references to baseURL if needed and document/validate the env var in test startup/docs.app/Controllers/CollocazioneController.php (1)
364-368:exportCSVhas the samestmt === falseguard without SecureLogger.For consistency with the
getLibrifix, theexportCSVfailure path could also log viaSecureLogger::error.♻️ Proposed fix
$stmt = $db->prepare($sql); if ($stmt === false) { - $response->getBody()->write(__('Errore nella query.')); - return $response->withStatus(500); + \App\Support\SecureLogger::error('CollocazioneController::exportCSV query failed: ' . $db->error); + $response->getBody()->write(__('Errore nella query.')); + return $response->withStatus(500); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/CollocazioneController.php` around lines 364 - 368, The exportCSV method currently checks if $stmt === false and returns a 500 without logging; update CollocazioneController::exportCSV to log the failure via SecureLogger::error (include the SQL and any PDO error info or exception message) before writing the generic error response, similar to the getLibri fix—use the same SecureLogger class and a descriptive message referencing $sql and $db->errorInfo()/PDOException context while leaving the existing response body and status unchanged.tests/genre-bugs.spec.js (1)
272-304: Add cleanup for the “no‑genre book” test.
This test creates a book but never deletes it. In persistent E2E environments that can pollute data and cause later flakiness.🧽 Suggested cleanup snippet
const radiceValue = await page.locator('#radice_select').inputValue(); expect(parseInt(radiceValue, 10)).toBe(0); + + // Cleanup: delete the created book to avoid polluting the test DB + await page.goto(`${BASE}/admin/libri/${bookId}`); + const delBookForm = page.locator('form[action*="/delete"]'); + if (await delBookForm.isVisible({ timeout: 2000 }).catch(() => false)) { + await delBookForm.locator('button[type="submit"]').click(); + await page.waitForURL(/.*libri.*/, { timeout: 10000 }); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/genre-bugs.spec.js` around lines 272 - 304, This test leaves a created book behind—capture the bookId (from match on /libri/(\d+)) and ensure cleanup by deleting that book at the end of the test (or in a finally/afterEach) using the app's admin delete flow or API; for example, call the DELETE book endpoint or navigate to the admin libri list and invoke the delete action for the created bookId (the test already uses page.goto(`${BASE}/admin/libri/modifica/${bookId}`), selector '#radice_select', and variable bookId), and confirm any dialogs so the created record is removed even if assertions fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@storage/plugins/digital-library/DigitalLibraryPlugin.php`:
- Around line 461-469: The current MIME-rejection path in DigitalLibraryPlugin
(inside the block that checks $detectedMime and $allowedMime) uses the
error-suppressing `@unlink`($targetPath) so a failed deletion is silent; update
that code to attempt unlink($targetPath) without the @, check its boolean result
(and file_exists($targetPath) beforehand if desired), and if the deletion fails
call \App\Support\SecureLogger::error with contextual data (target path, client
filename, detected/expected mime, and error_get_last() or a descriptive message)
so failures are logged while still returning the JSON 400 response from
$this->json.
In `@tests/admin-features.spec.js`:
- Around line 269-290: The test "Edit hero section title and subtitle" mutates
shared CMS data and should restore original hero values; before calling
page.fill(...) in that test capture the current hero title/subtitle (e.g., read
from page inputs or via dbQuery("SELECT title, subtitle FROM home_content WHERE
section_key='hero'")), then after the verification step perform a cleanup that
restores those original values (either by filling the inputs and re-submitting
the same form or issuing an update via dbQuery), and ensure the restore runs
even on failure by wrapping the edit/assert/restore sequence in a try/finally or
moving restore logic into an afterEach cleanup for this test.
- Around line 124-157: For both tests ("Contacts tab: update contact info" and
"Privacy tab: update privacy page title") capture the existing values from
system_settings via dbQuery before calling page.fill, then ensure those
originals are restored after the test (use a try/finally inside each test or
move capture/restore into a shared afterEach) so mutated settings are not
leaked; specifically, call dbQuery to read the current setting_value for
category='contacts' & setting_key='contact_email' and for category='privacy' &
setting_key='page_title' prior to updating, and in the finally/cleanup use an
update query to set the values back to the captured originals (or skip restore
if original was null) so the database state is unchanged after the tests.
In `@tests/genre-merge-rearrange.spec.js`:
- Around line 150-166: The test assumes a child genre exists but childGenre can
be undefined; update the test around the genres lookup (variables: genres,
topLevelIds, childGenre) to guard or create a fixture: if childGenre is not
found, create a top-level genre and a child via the same API used in other tests
(or reuse the rearrange test setup), then reload the list and reselect
childGenre before calling page.goto and page.click, or alternatively bail out
the test early with a clear skip/assert when childGenre is missing; ensure the
rest of the test (parentSelect and selectedValue checks) only runs when
childGenre is defined.
---
Outside diff comments:
In `@public/.htaccess`:
- Around line 70-83: The current .htaccess exposes all audio/video under
FilesMatch and the /uploads/digital/ If block via Header set
Access-Control-Allow-Origin "*", allowing cross-origin access to potentially
protected files; either (A) remove the wildcard CORS headers from the FilesMatch
and the If "%{REQUEST_URI} =~ m#^/uploads/digital/#" block and instead add CORS
only to explicitly public/sample paths (e.g. /uploads/digital/public/ or
/uploads/digital/samples/) by creating a new FilesMatch/If that targets those
paths, or (B) stop Apache from serving /uploads/digital/ directly and route
those requests to a PHP streaming proxy that performs session/loan validation
before streaming (disable the static shortcut that currently bypasses PHP),
while keeping Accept-Ranges where needed; update/removethe Header set
Access-Control-Allow-Origin "*" and related Header set lines accordingly.
---
Duplicate comments:
In `@app/Controllers/GeneriController.php`:
- Around line 154-155: The destroy() method signature declares a Request
$request parameter that is unused and triggers PHPMD; update the method
signature for destroy(Request $request, Response $response, \mysqli $db, int
$id): Response by either removing the $request parameter entirely if the
framework allows or renaming it to $_request (or $_unusedRequest) to silence the
PHPMD unused variable warning while keeping the same signature.
In `@app/Controllers/LibriApiController.php`:
- Around line 619-644: The bulk-delete SQL only nulls isbn10/isbn13/ean but must
also clear shelf-location fields to match single-delete; update the $sql used in
LibriApiController (the statement using $placeholders/$stmt) to set the
collocazione and scaffale columns to NULL as well (e.g. add ", collocazione =
NULL, scaffale = NULL" to the SET clause), keep the existing
prepare/bind_param/execute flow with $types and $cleanIds unchanged, and ensure
error logging around $stmt remains as-is.
- Around line 34-35: In LibriApiController, guard use of optional columns
(l.parole_chiave, l.sottogenere_id, l.collana) when building list filters:
before adding any WHERE or SELECT that references these columns (e.g. where you
read $q and set $collana) check the schema with Schema::hasColumn('libri',
'collana') / Schema::hasColumn('libri', 'sottogenere_id') /
Schema::hasColumn('libri', 'parole_chiave') and only append the corresponding
filter logic/joins when the column exists and the query param (like
$q['collana']) is non-empty; update all filter blocks referenced (the areas
around the $collana assignment and the blocks at the other noted ranges) to use
these conditional checks so older schemas won't cause SQL errors.
In `@app/Models/BookRepository.php`:
- Around line 980-1044: The resolver is issuing extra SELECTs to walk ancestors
even though getById() already joins genre ancestors (g/gp/gpp); modify the logic
in BookRepository (the method that builds $chain from $genereId and populates
$row fields) to first check for the joined genre columns on $row (e.g.
g_nome/g_id, gp_nome/gp_id, gpp_nome/gpp_id or whatever the existing joined
field names are) and build the $chain and cascade values from those when present
(covering the common 1–3 level cases), and only fall back to the current
prepared-statement ancestor walk (the $stmt loop that reads generi by id into
$chain) if the joined fields are missing or incomplete.
In `@app/Views/frontend/home.php`:
- Around line 891-900: The available-books element is still using a placeholder
emoji; update the fetch handler that uses API_CATALOG_ROUTE so
availableBooksEl.textContent is set from the real API field (e.g.,
data.pagination.available_books) instead of the hardcoded '\uD83D\uDCDA', and
update the .catch fallback to a sensible default (either a numeric 0 or a UI
placeholder) so totalBooksEl and availableBooksEl both show real counts or a
clear non-numeric fallback; locate the logic around totalBooksEl,
availableBooksEl and the fetch(API_CATALOG_ROUTE) promise chain to make this
change.
In `@app/Views/libri/index.php`:
- Around line 639-645: The collana filter is always sent from the initial
collanaFilter (URL-only) and never reads the live input; update the
filter-building code to read the current collana input value (or ensure the
collanaFilter variable is updated in the collana input/autocomplete handlers)
before sending — locate the place where the filters object is created (uses keys
collana: collanaFilter, genere_filter, etc.) and replace or augment collana:
collanaFilter with the actual DOM value (e.g., document.getElementById(...) or
the component's current value) and also clear/reset collanaFilter together with
the other filters where you reset genere_filter/posizione_id/anno_from/anno_to.
- Around line 477-483: The closeBtn.addEventListener handler currently resets
collanaFilter, genereUrlFilter, sottogenereUrlFilter and unconditionally clears
document.getElementById('search_text').value when initialKeywords is truthy;
change this so it only clears the search_text if its current value equals the
URL-supplied initialKeywords (e.g. compare
document.getElementById('search_text').value to initialKeywords, optionally trim
both) so user-typed changes are preserved; keep the rest of the filter resets
as-is and only perform the clear when the values match.
In `@package.json`:
- Line 7: The package.json "test" script references tests/playwright.config.js
which may not exist; verify that the Playwright config file path is correct and
either (a) create or move the Playwright config to tests/playwright.config.js,
or (b) update the package.json "test" script value to the actual config path
(e.g., playwright.config.js or the correct relative path) so running npm test
won't fail; ensure the change is made to the "test" script entry in
package.json.
In `@public/index.php`:
- Around line 399-401: Currently $displayErrorDetails falls back to false which
can hide stack traces; explicitly set displayErrorDetails for dev/staging by
reading an environment/app-env and/or enforcing the setting in your
configuration instead of a silent default. Update the logic around
$displayErrorDetails = $container->get('settings')['displayErrorDetails'] ??
false so that for APP_ENV (or equivalent) values 'development' or 'staging' it
yields true (or require the setting to exist and error if missing), and then
pass that explicit value into $errorMiddleware =
$app->addErrorMiddleware($displayErrorDetails, true, true) so dev/staging always
enable detailed errors.
In `@scripts/manual-update.php`:
- Around line 19-25: The fallback .env lookup only checks __DIR__ . '/../.env'
so when the script is uploaded to the project root it misses a same-directory
.env; update the $secretKey resolution (starting at getenv('MANUAL_UPDATE_KEY')
...) to also check __DIR__ . '/.env' (prefer current-dir then parent-dir) and
read the MANUAL_UPDATE_KEY from whichever .env exists before falling back to an
empty string, keeping the existing preg_match/trim logic to extract the value.
In `@storage/plugins/digital-library/DigitalLibraryPlugin.php`:
- Around line 197-211: In DigitalLibraryPlugin adjust the resource cleanup: when
you call $stmt->get_result() in the hook check code, ensure you free the
mysqli_result (use $result->free()) and always close the statement
($stmt->close()) on every early exit path and after successful processing to
avoid leaks; reference the $stmt variable and $result from the
prepare()/get_result() block in the DigitalLibraryPlugin class and add the
missing $result->free() and consistent $stmt->close() calls where you currently
continue or finish handling the hook.
- Around line 62-76: The htaccess generation block using
file_put_contents($htaccess, implode("\n", [...])) correctly includes both
mod_authz_core.c (Require all denied) and mod_access_compat.c (Order Deny,Allow
/ Deny from all) inside the '<FilesMatch "\.(php\d?|phtml|phar|pl|py|cgi|sh)$">'
section, so no code change is required; leave the file_put_contents call and the
FilesMatch block as-is to maintain Apache 2.4+ and legacy 2.2 compatibility.
- Around line 547-555: The JSON encoding fallback must guard against json_encode
returning false; update the error-handling path in the DigitalLibraryPlugin (the
try/catch around json_encode and the catch block that calls
\App\Support\SecureLogger::error) so that when json_encode(['success' => false,
'message' => 'Internal error']) could return false you instead produce a valid
JSON string (e.g. use a coalescing/ternary to fall back to a literal
'{"success":false,"message":"Internal error"}') before calling
$response->getBody()->write and returning the response with Content-Type
application/json and the given $status.
- Around line 389-414: The HTTP status mapping for upload errors is correct:
keep the match on $errorCode that sets $status (using UPLOAD_ERR_NO_TMP_DIR and
UPLOAD_ERR_CANT_WRITE => 500, default => 400), preserve the $message match on
$errorCode and the SecureLogger::warning call that logs 'error_code' and
'filename', and return via $this->json($response, ['success' => false, 'message'
=> $message], $status) as-is; no code change required beyond ensuring those
symbols ($errorCode, $status, match expressions, SecureLogger::warning, and
return $this->json) remain intact.
- Around line 94-96: The prepare() guard currently returns early when $stmt ===
false but doesn't report or handle the failure; in the
DigitalLibraryPlugin::deactivate (or the method that calls prepare on the DB)
add error handling when $stmt is false: log or throw a descriptive error using
the plugin's logger (or trigger_error) including $this->db->error (or
mysqli->error) and then return to avoid proceeding; ensure any later calls that
assume $stmt (e.g., $stmt->bind_param, $stmt->execute, $stmt->close) are not
executed when prepare() failed.
In `@tests/loan-reservation.spec.js`:
- Around line 111-114: The password hash generation currently hard-codes
"Test1234!" which breaks overrides; change the execFileSync call in
tests/loan-reservation.spec.js to use the TEST_PASS value (e.g.,
process.env.TEST_PASS or the TEST_PASS constant used elsewhere in the test)
instead of the literal string so the generated hash matches the credential used
by the login flow; keep the same execFileSync invocation, encoding and trim
logic but substitute the literal with TEST_PASS.
In `@tests/security-hardening.spec.js`:
- Around line 39-41: Replace the gitleaks‑triggering key literal used in the
test named "returns 403 with wrong key" with a non key‑like placeholder constant
and use that constant in the request URL; specifically update the string
"wrong_key_12345" referenced in the test (inside the request.get call) to a
harmless name (e.g., INVALID_KEY or "placeholder-value") so the test still
asserts the same statuses via resp.status() but no longer contains a key-like
literal.
---
Nitpick comments:
In `@app/Controllers/CollocazioneController.php`:
- Around line 364-368: The exportCSV method currently checks if $stmt === false
and returns a 500 without logging; update CollocazioneController::exportCSV to
log the failure via SecureLogger::error (include the SQL and any PDO error info
or exception message) before writing the generic error response, similar to the
getLibri fix—use the same SecureLogger class and a descriptive message
referencing $sql and $db->errorInfo()/PDOException context while leaving the
existing response body and status unchanged.
In `@storage/plugins/digital-library/DigitalLibraryPlugin.php`:
- Around line 229-234: The INSERT's $stmt->execute() call in
DigitalLibraryPlugin.php is unchecked and can fail silently; update the code
around the INSERT (the block that prepares and executes $stmt for the hook
activation) to check the boolean return of $stmt->execute(), log the error using
available logger/error info (e.g., $stmt->error or $this->logger) and propagate
or handle the failure (return false / throw an exception) instead of proceeding
as if success—include contextual info (hook id/name and error/errno) in the log
so races/constraint violations are diagnosable.
- Around line 507-510: Replace the strpos-based path traversal check in the
DigitalLibraryPlugin path-guard with str_starts_with for clarity: after
computing $filePath (realpath($baseRealPath . DIRECTORY_SEPARATOR . $filename)),
keep the existing checks for $filePath === false and is_file($filePath), but
replace the strpos(...) !== 0 condition with !str_starts_with($filePath,
$baseRealPath . DIRECTORY_SEPARATOR) so the intent is explicit and safer on PHP
8+; update the conditional that returns $response->withStatus(404) accordingly.
In `@tests/genre-bugs.spec.js`:
- Around line 272-304: This test leaves a created book behind—capture the bookId
(from match on /libri/(\d+)) and ensure cleanup by deleting that book at the end
of the test (or in a finally/afterEach) using the app's admin delete flow or
API; for example, call the DELETE book endpoint or navigate to the admin libri
list and invoke the delete action for the created bookId (the test already uses
page.goto(`${BASE}/admin/libri/modifica/${bookId}`), selector '#radice_select',
and variable bookId), and confirm any dialogs so the created record is removed
even if assertions fail.
In `@tests/playwright.config.js`:
- Line 10: The hardcoded baseURL in tests/playwright.config.js should be made
configurable via an environment variable; replace the literal
'http://localhost:8081' assigned to baseURL with a value read from process.env
(e.g., process.env.PLAYWRIGHT_BASE_URL or process.env.BASE_URL) and fall back to
the current default if the env var is not set, updating the exported Playwright
config object so tests use the env-driven baseURL.
- Line 10: The config hardcodes baseURL ('http://localhost:8081'), which breaks
CI; change the baseURL assignment in tests/playwright.config.js to read from an
environment variable (e.g., process.env.PLAYWRIGHT_BASE_URL) with a sensible
fallback (like 'http://localhost:8081') so CI or containers can override the
host/port; update any references to baseURL if needed and document/validate the
env var in test startup/docs.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (44)
.gitattributesapp/Controllers/Admin/LanguagesController.phpapp/Controllers/CmsController.phpapp/Controllers/CollocazioneController.phpapp/Controllers/GeneriApiController.phpapp/Controllers/GeneriController.phpapp/Controllers/LibriApiController.phpapp/Controllers/LibriController.phpapp/Controllers/SettingsController.phpapp/Middleware/CsrfMiddleware.phpapp/Models/BookRepository.phpapp/Models/GenereRepository.phpapp/Routes/web.phpapp/Views/admin/settings.phpapp/Views/autori/index.phpapp/Views/cms/edit-home.phpapp/Views/editori/index.phpapp/Views/frontend/book-detail.phpapp/Views/frontend/catalog.phpapp/Views/frontend/home.phpapp/Views/generi/dettaglio_genere.phpapp/Views/layout.phpapp/Views/libri/index.phpapp/Views/libri/partials/book_form.phpapp/Views/partials/cookie-banner.phpapp/Views/prestiti/index.phppackage.jsonpublic/.htaccesspublic/index.phpscripts/manual-update.phpstorage/plugins/api-book-scraper/plugin.jsonstorage/plugins/dewey-editor/plugin.jsonstorage/plugins/digital-library/DigitalLibraryPlugin.phpstorage/plugins/digital-library/plugin.jsonstorage/plugins/open-library/plugin.jsonstorage/plugins/z39-server/plugin.jsontests/admin-features.spec.jstests/extra-features.spec.jstests/genre-bugs.spec.jstests/genre-merge-rearrange.spec.jstests/loan-reservation.spec.jstests/playwright.config.jstests/security-hardening.spec.jstests/smoke-install.spec.js
Comment on lines
461
to
469
| if ($detectedMime === false || !in_array($detectedMime, $allowedMime, true)) { | ||
| // Remove the uploaded file — it failed MIME validation | ||
| @unlink($targetPath); | ||
| \App\Support\SecureLogger::warning('[Digital Library] Upload rejected: MIME mismatch', [ | ||
| 'expected' => $allowedMime, | ||
| 'detected' => $detectedMime ?: 'unknown', | ||
| 'filename' => $clientFilename, | ||
| ]); | ||
| return $this->json($response, ['success' => false, 'message' => __('Formato file non supportato.')], 400); |
There was a problem hiding this comment.
@unlink silently suppresses failure — rejected file can remain on disk.
If unlink($targetPath) fails (e.g., a permissions issue or race condition), the MIME-rejected file stays on disk with no indication in the logs that the cleanup failed. This creates a silent security gap.
🐛 Proposed fix
- `@unlink`($targetPath);
- \App\Support\SecureLogger::warning('[Digital Library] Upload rejected: MIME mismatch', [
- 'expected' => $allowedMime,
- 'detected' => $detectedMime ?: 'unknown',
- 'filename' => $clientFilename,
- ]);
+ \App\Support\SecureLogger::warning('[Digital Library] Upload rejected: MIME mismatch', [
+ 'expected' => $allowedMime,
+ 'detected' => $detectedMime ?: 'unknown',
+ 'filename' => $clientFilename,
+ ]);
+ if (!unlink($targetPath)) {
+ \App\Support\SecureLogger::error('[Digital Library] Failed to remove rejected upload', [
+ 'path' => $targetPath,
+ ]);
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@storage/plugins/digital-library/DigitalLibraryPlugin.php` around lines 461 -
469, The current MIME-rejection path in DigitalLibraryPlugin (inside the block
that checks $detectedMime and $allowedMime) uses the error-suppressing
`@unlink`($targetPath) so a failed deletion is silent; update that code to attempt
unlink($targetPath) without the @, check its boolean result (and
file_exists($targetPath) beforehand if desired), and if the deletion fails call
\App\Support\SecureLogger::error with contextual data (target path, client
filename, detected/expected mime, and error_get_last() or a descriptive message)
so failures are logged while still returning the JSON 400 response from
$this->json.
Comment on lines
124
to
157
| test('Contacts tab: update contact info', async () => { | ||
| await page.goto(`${BASE}/admin/settings?tab=contacts`); | ||
| await page.waitForLoadState('networkidle'); | ||
|
|
||
| await page.locator('[data-settings-tab="contacts"]').click(); | ||
| await expect(page.locator('section[data-settings-panel="contacts"]')).toBeVisible(); | ||
|
|
||
| await page.fill('#page_title', `Contattaci ${RUN_ID}`); | ||
| await page.fill('#contact_email', `contacts-${RUN_ID}@example.com`); | ||
|
|
||
| await page.locator('section[data-settings-panel="contacts"] button[type="submit"]').click(); | ||
| await page.waitForLoadState('networkidle'); | ||
|
|
||
| // Verify in DB | ||
| const contactEmail = dbQuery("SELECT setting_value FROM system_settings WHERE category='contacts' AND setting_key='contact_email'"); | ||
| expect(contactEmail).toContain(RUN_ID); | ||
| }); | ||
|
|
||
| test('Privacy tab: update privacy page title', async () => { | ||
| await page.goto(`${BASE}/admin/settings?tab=privacy`); | ||
| await page.waitForLoadState('networkidle'); | ||
|
|
||
| await page.locator('[data-settings-tab="privacy"]').click(); | ||
| await expect(page.locator('section[data-settings-panel="privacy"]')).toBeVisible(); | ||
|
|
||
| await page.fill('#privacy_page_title', `Privacy Policy ${RUN_ID}`); | ||
|
|
||
| await page.locator('section[data-settings-panel="privacy"] button[type="submit"]').first().click(); | ||
| await page.waitForLoadState('networkidle'); | ||
|
|
||
| // Verify in DB | ||
| const privacyTitle = dbQuery("SELECT setting_value FROM system_settings WHERE category='privacy' AND setting_key='page_title'"); | ||
| expect(privacyTitle).toContain(RUN_ID); | ||
| }); |
There was a problem hiding this comment.
Restore settings mutated by the test.
The contacts and privacy tests update system_settings but never restore the original values, which can leak state across suites. Capture the current values and restore them in a finally/cleanup step.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/admin-features.spec.js` around lines 124 - 157, For both tests
("Contacts tab: update contact info" and "Privacy tab: update privacy page
title") capture the existing values from system_settings via dbQuery before
calling page.fill, then ensure those originals are restored after the test (use
a try/finally inside each test or move capture/restore into a shared afterEach)
so mutated settings are not leaked; specifically, call dbQuery to read the
current setting_value for category='contacts' & setting_key='contact_email' and
for category='privacy' & setting_key='page_title' prior to updating, and in the
finally/cleanup use an update query to set the values back to the captured
originals (or skip restore if original was null) so the database state is
unchanged after the tests.
Comment on lines
269
to
290
| test('Edit hero section title and subtitle', async () => { | ||
| await page.goto(`${BASE}/admin/cms/home`); | ||
| await page.waitForLoadState('networkidle'); | ||
|
|
||
| const heroTitle = `Test Library ${RUN_ID}`; | ||
| const heroSubtitle = `Subtitle ${RUN_ID}`; | ||
|
|
||
| await page.fill('input[name="hero[title]"]', heroTitle); | ||
| await page.fill('input[name="hero[subtitle]"]', heroSubtitle); | ||
| await page.fill('input[name="hero[button_text]"]', 'Esplora'); | ||
|
|
||
| // Submit the hero form (first submit button) | ||
| await page.locator('form[action*="cms/home"] button[type="submit"]').first().click(); | ||
| await page.waitForLoadState('networkidle'); | ||
|
|
||
| // Verify success message | ||
| await expect(page.locator('.bg-green-50').first()).toBeVisible({ timeout: 10000 }); | ||
|
|
||
| // Verify in DB | ||
| const dbTitle = dbQuery("SELECT title FROM home_content WHERE section_key='hero'"); | ||
| expect(dbTitle).toBe(heroTitle); | ||
| }); |
There was a problem hiding this comment.
Consider restoring CMS hero content after the test.
The hero title/subtitle changes persist; in shared environments this can affect later suites. Saving the original values and restoring them would keep tests isolated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/admin-features.spec.js` around lines 269 - 290, The test "Edit hero
section title and subtitle" mutates shared CMS data and should restore original
hero values; before calling page.fill(...) in that test capture the current hero
title/subtitle (e.g., read from page inputs or via dbQuery("SELECT title,
subtitle FROM home_content WHERE section_key='hero'")), then after the
verification step perform a cleanup that restores those original values (either
by filling the inputs and re-submitting the same form or issuing an update via
dbQuery), and ensure the restore runs even on failure by wrapping the
edit/assert/restore sequence in a try/finally or moving restore logic into an
afterEach cleanup for this test.
Comment on lines
+150
to
+166
| // Get all genres and find a child whose parent is top-level | ||
| const resp = await page.request.get(`${BASE}/api/generi?limit=100`); | ||
| const genres = await resp.json(); | ||
| const topLevelIds = new Set(genres.filter(g => g.parent_id === null).map(g => g.id)); | ||
| const childGenre = genres.find(g => g.parent_id !== null && topLevelIds.has(g.parent_id)); | ||
| expect(childGenre).toBeTruthy(); | ||
|
|
||
| await page.goto(`${BASE}/admin/generi/${childGenre.id}`); | ||
| await page.click('#btn-edit-genre'); | ||
|
|
||
| const parentSelect = page.locator('#edit_parent_id'); | ||
| await expect(parentSelect).toBeVisible(); | ||
|
|
||
| // Should have current parent pre-selected | ||
| const selectedValue = await parentSelect.inputValue(); | ||
| expect(parseInt(selectedValue, 10)).toBe(childGenre.parent_id); | ||
| }); |
There was a problem hiding this comment.
Guard against missing child‑genre fixtures.
childGenre can be undefined on empty datasets, causing the test to fail. Consider creating a child genre in the test setup (like the rearrange test) or gracefully skipping when none exists.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/genre-merge-rearrange.spec.js` around lines 150 - 166, The test assumes
a child genre exists but childGenre can be undefined; update the test around the
genres lookup (variables: genres, topLevelIds, childGenre) to guard or create a
fixture: if childGenre is not found, create a top-level genre and a child via
the same API used in other tests (or reuse the rearrange test setup), then
reload the list and reselect childGenre before calling page.goto and page.click,
or alternatively bail out the test early with a clear skip/assert when
childGenre is missing; ensure the rest of the test (parentSelect and
selectedValue checks) only runs when childGenre is defined.
- DigitalLibraryPlugin: replace @Unlink with explicit error+SecureLogger, check execute() on hook INSERT, use str_starts_with() for path guard - LibriApiController: nullify shelf-location fields on bulk soft-delete - CollocazioneController: SecureLogger on exportCSV prepare() failure - GeneriController: prefix unused $request param with underscore - manual-update.php: check .env in both __DIR__ and parent directory - .htaccess: move Options -Indexes inside IfModule (shared hosting fix) - libri/index: banner close only clears search if matching initial keywords - Tests: restore originals in settings tabs, use TEST_PASS variable, remove gitleaks-triggering key literal, configurable baseURL, cleanup test book in genre-bugs
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
1 participant
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
update()to correctly handleparent_id = NULL.Closes #64
Test plan
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores