fix: P2 consistency findings across v0.4.9.9-v0.5.2#96
Open
fabiodalez-dev wants to merge 19 commits intomainfrom
Open
fix: P2 consistency findings across v0.4.9.9-v0.5.2#96fabiodalez-dev wants to merge 19 commits intomainfrom
fabiodalez-dev wants to merge 19 commits intomainfrom
Conversation
1. descrizione_plain: catalog search and admin grid now use COALESCE(descrizione_plain, descrizione) for LIKE conditions (FULLTEXT MATCH unchanged - requires index columns) 2. ISSN: added to Schema.org JSON-LD ($bookSchema['issn']) and PublicApiController SELECT + response mapping 3. CollaneController::rename(): explicit execute() check on collane sync with RuntimeException on failure 4. LibraryThing import: descrizione_plain (strip_tags), ISSN normalization (XXXX-XXXX), AuthorNormalizer on traduttore — all 4 INSERT/UPDATE SQL variants updated
Owner
Author
|
@CodeRabbit 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 descrizione_plain-aware search, hardens collane sync error propagation, enriches LibraryThing parsing/persistence (contributors, descrizione_plain, ISSN/EAN), propagates locale through auth/profile/remember-me, exposes ISSN in APIs/views, registers a new GoodLib plugin with admin UI/hooks, and adds extensive E2E tests and a PHP test harness. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Admin as Admin UI
participant Controller as PluginController
participant PM as PluginManager
participant DB as MySQL
participant Plugin as GoodLibPlugin
participant View as Book Page
Admin->>Controller: POST GoodLib settings (sources, domains)
Controller->>PM: normalize domains & setSetting(...)
PM->>DB: upsert plugin_settings rows
DB-->>PM: OK
PM-->>Controller: success
Controller-->>Admin: JSON success
View->>Plugin: renderFrontendBadges($book)
Plugin->>PM: load settings
PM->>DB: select plugin_settings
DB-->>PM: settings rows
PM-->>Plugin: settings
Plugin->>View: include badges.php with $sources and $query
View-->>Client: HTML with external links
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 5
🧹 Nitpick comments (1)
app/Controllers/LibriApiController.php (1)
45-48: LGTM! Correctly implementsdescrizione_plainsearch support.The defensive column check via
hasTableColumn()and the COALESCE fallback pattern align with the approach inFrontendController::descriptionExpr(). The prepared statement placeholder count remains correct (5 LIKE params), and the interpolated$descExpris derived from trusted internal logic.Consider extracting the description expression logic into a shared trait or utility class to reduce duplication with
FrontendController::descriptionExpr(). This would centralize the column-existence check and make future changes easier to maintain.Example shared trait approach
// app/Support/DescriptionExprTrait.php trait DescriptionExprTrait { private static ?bool $hasDescrizionePlain = null; protected static function descriptionExpr(mysqli $db): string { if (self::$hasDescrizionePlain === null) { $result = $db->query("SHOW COLUMNS FROM libri LIKE 'descrizione_plain'"); self::$hasDescrizionePlain = $result && $result->num_rows > 0; } return self::$hasDescrizionePlain ? 'COALESCE(l.descrizione_plain, l.descrizione)' : 'l.descrizione'; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibriApiController.php` around lines 45 - 48, Extract the duplicated description-expression logic into a shared helper (e.g., create a DescriptionExprTrait with a cached descriptionExpr(mysqli $db): string method) and have LibriApiController use that trait instead of inlining the hasTableColumn/COALESCE logic; mirror FrontendController::descriptionExpr() behavior and return the same expression ('COALESCE(l.descrizione_plain, l.descrizione)' or 'l.descrizione') while caching the column-exists check, then replace the inline $descExpr assignment in LibriApiController with a call to the new descriptionExpr($db) helper.
🤖 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/CollaneController.php`:
- Around line 276-284: In rename(), ensure the collane sync prepare failure is
handled so the transaction cannot partially commit: after $stmtSync =
$db->prepare("UPDATE collane SET nome = ? WHERE nome = ?") check if $stmtSync is
false and immediately throw a \RuntimeException (including $db->error) or
otherwise abort/rollback the transaction before proceeding; keep the existing
bind_param/execute/close path for $stmtSync and only allow commit when prepare
and execute succeed so $db, $stmtSync, $newName and $oldName updates remain
atomic.
In `@app/Controllers/LibraryThingImportController.php`:
- Line 1150: The SQL update/insert statements in LibraryThingImportController
that unconditionally include "descrizione_plain" (e.g., the fragment "genere_id
= ?, descrizione = ?, descrizione_plain = ?, formato = ?, prezzo = ?, editore_id
= ?") will fail on upgraded instances missing that column; change the logic to
only include and bind descrizione_plain when the schema/migration has been
applied—detect this with a schema check or migration/version flag (e.g.,
hasColumn('descrizione_plain') or checking the controller's upgrade state) and
build the SQL and parameter list conditionally. Apply the same conditional
inclusion to the other occurrences referenced in the comment so none of the four
SQL branches reference descrizione_plain unless the column exists.
- Line 1162: The UPDATE statements that target the libri table currently use
"WHERE id = ?" and must include the soft-delete guard to avoid racing with
findExistingBook(); update both SQL statements (the ones using "WHERE id = ?" in
LibraryThingImportController) to append "AND deleted_at IS NULL" so they become
"WHERE id = ? AND deleted_at IS NULL", keeping the same bound parameters and
error handling; apply this change to the occurrence around the first update
(shown at the diff) and the second occurrence noted (also at the other update).
- Around line 700-708: The normalized author list may contain duplicates after
calling \App\Support\AuthorNormalizer::normalize for 'Primary Author' and
'Secondary Author', causing duplicate libri_autori rows in processChunk(); to
fix, deduplicate the $authors array after normalization (e.g., use array_unique
or build a keyed set) before setting $result['autori'], so only unique canonical
names are joined with implode('|') and assigned to $result['autori'].
- Around line 791-793: The current assignment to $result['descrizione_plain']
uses strip_tags($result['descrizione']) which glues adjacent blocks and leaves
HTML entities encoded; instead normalize block boundaries and decode entities:
replace common block-level tags (e.g. </p>, <br>, </div>, </li>, headings) with
a space or newline in $result['descrizione'], then strip tags, run
html_entity_decode on the result, collapse repeated whitespace to single spaces
and trim, and assign that cleaned string to $result['descrizione_plain']; refer
to $result['descrizione'], $result['descrizione_plain'] and the existing
strip_tags call to locate where to apply these steps.
---
Nitpick comments:
In `@app/Controllers/LibriApiController.php`:
- Around line 45-48: Extract the duplicated description-expression logic into a
shared helper (e.g., create a DescriptionExprTrait with a cached
descriptionExpr(mysqli $db): string method) and have LibriApiController use that
trait instead of inlining the hasTableColumn/COALESCE logic; mirror
FrontendController::descriptionExpr() behavior and return the same expression
('COALESCE(l.descrizione_plain, l.descrizione)' or 'l.descrizione') while
caching the column-exists check, then replace the inline $descExpr assignment in
LibriApiController with a call to the new descriptionExpr($db) helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4f76c506-f7b0-4368-b903-00d225a7d72c
📒 Files selected for processing (6)
app/Controllers/CollaneController.phpapp/Controllers/FrontendController.phpapp/Controllers/LibraryThingImportController.phpapp/Controllers/LibriApiController.phpapp/Controllers/PublicApiController.phpapp/Views/frontend/book-detail.php
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
app/Controllers/CollaneController.php (1)
276-284:⚠️ Potential issue | 🔴 CriticalAbort transaction when collane sync
prepare()failsLine 276 still allows a silent
prepare()failure path (if ($stmtSync)), which can commit a partial rename (libriupdated,collaneunchanged). This is the same unresolved atomicity issue flagged earlier.Proposed fix
if ($this->hasCollaneTable($db)) { $stmtSync = $db->prepare("UPDATE collane SET nome = ? WHERE nome = ?"); - if ($stmtSync) { - $stmtSync->bind_param('ss', $newName, $oldName); - $ok = $stmtSync->execute(); - $stmtSync->close(); - if (!$ok) { - throw new \RuntimeException('Collane sync failed: ' . $db->error); - } - } + if (!$stmtSync) { + throw new \RuntimeException('Collane sync prepare failed: ' . $db->error); + } + $stmtSync->bind_param('ss', $newName, $oldName); + $ok = $stmtSync->execute(); + $stmtSync->close(); + if (!$ok) { + throw new \RuntimeException('Collane sync failed: ' . $db->error); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/CollaneController.php` around lines 276 - 284, The current prepare() for $stmtSync can fail silently due to the if ($stmtSync) guard, allowing the transaction to commit with only the libri updates; change the flow so prepare() returning false triggers an immediate rollback and exception: after calling $db->prepare("UPDATE collane ...") check for a falsy result and call $db->rollback() then throw a RuntimeException (including $db->error), otherwise proceed to bind/execute/close and let the outer transaction logic continue; ensure the rollback call targets the same DB connection used for the libri update so the rename remains atomic.app/Controllers/LibraryThingImportController.php (3)
1146-1163:⚠️ Potential issue | 🟠 MajorAdd
deleted_at IS NULLto bothlibriupdates.
findExistingBook()filters deleted rows, but theseUPDATE libri ... WHERE id = ?writes can still race a soft-delete and mutate a deleted record.🛡️ Proposed fix
- WHERE id = ? + WHERE id = ? AND deleted_at IS NULLApply the same change to the second
UPDATE libribranch as well. As per coding guidelines,Every query on libri table MUST include soft-delete condition: AND deleted_at IS NULL.Also applies to: 1229-1237
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibraryThingImportController.php` around lines 1146 - 1163, The UPDATE statements that write to the libri table (the prepared statement building "UPDATE libri SET ... WHERE id = ?") lack the soft-delete guard and can race a soft-delete; modify both UPDATE branches (the one shown and the other UPDATE around the 1229-1237 area) to append "AND deleted_at IS NULL" to the WHERE clause so every libri update includes the soft-delete condition; locate the UPDATE SQL in LibraryThingImportController (the prepared statements) and add the soft-delete predicate to each WHERE id = ?.
790-793:⚠️ Potential issue | 🟠 MajorBuild real plain text for
descrizione_plain.
strip_tags()still turns adjacent blocks into glued text and leaves entities encoded, so the new LIKE search can still miss summaries such as<p>foo</p><p>bar</p>orl'amour.🛠️ Proposed fix
$result['descrizione'] = !empty($data['Summary']) ? trim($data['Summary']) : ''; if (!empty($result['descrizione'])) { - $result['descrizione_plain'] = strip_tags($result['descrizione']); + $plain = preg_replace('/<[^>]+>/', ' ', $result['descrizione']) ?? $result['descrizione']; + $plain = html_entity_decode($plain, ENT_QUOTES | ENT_HTML5, 'UTF-8'); + $result['descrizione_plain'] = trim(preg_replace('/\s+/u', ' ', $plain) ?? $plain); }In PHP, does strip_tags() preserve whitespace between removed block elements or decode HTML entities like '?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibraryThingImportController.php` around lines 790 - 793, The current generation of descrizione_plain using strip_tags(trim($data['Summary'])) loses spacing between block elements and keeps HTML entities; update the logic that sets $result['descrizione_plain'] (currently computed from $result['descrizione'] after strip_tags) to: 1) decode HTML entities with html_entity_decode(..., ENT_QUOTES|ENT_HTML5, 'UTF-8'); 2) replace common block-level tags (e.g., p, div, br, li, h1..h6, blockquote) with a single space or newline using preg_replace before stripping tags; 3) run strip_tags() and then normalize internal whitespace (preg_replace('/\s+/',' ', ...)) and trim the result so descrizione_plain has real plain text spacing and decoded entities. Ensure changes touch the code that assigns $result['descrizione'] and $result['descrizione_plain'] and keep trim() behavior for the original descrizione.
1146-1180:⚠️ Potential issue | 🟠 MajorDon't hard-require
descrizione_plainin every SQL branch.All four
INSERT/UPDATEvariants now binddescrizione_plain, including the two “plugin not installed” paths. If an upgraded instance has not applied that column yet, this import path fails withUnknown column 'descrizione_plain'. Build the column/value/type fragments conditionally off a schema check or a migration/version flag.Expect either a guaranteed migration path for
libri.descrizione_plain, or conditional SQL assembly here that omits the column when absent.#!/bin/bash set -euo pipefail # Verify whether the repository guarantees libri.descrizione_plain before this controller can run. rg -n -C2 --type=php '\bdescrizione_plain\b|\bfunction\s+isInstalled\b|\bisInstalled\s*\(' rg -n -C2 --glob '*.sql' 'descrizione_plain|ALTER TABLE libri|CREATE TABLE libri'Also applies to: 1229-1253, 1287-1332, 1384-1409
🧹 Nitpick comments (2)
app/Controllers/FrontendController.php (1)
22-30: Use the blank-safe fallback in the shared description expression.This helper feeds every new catalog
LIKEbranch below, so the sameCOALESCEcaveat applies here:descrizione_plain = ''suppresses fallback tol.descrizione. Switching the helper toCOALESCE(NULLIF(l.descrizione_plain, ''), l.descrizione)makes the search path resilient for legacy rows too.💡 Suggested change
- return self::$hasDescrizionePlain - ? 'COALESCE(l.descrizione_plain, l.descrizione)' - : 'l.descrizione'; + return self::$hasDescrizionePlain + ? "COALESCE(NULLIF(l.descrizione_plain, ''), l.descrizione)" + : 'l.descrizione';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/FrontendController.php` around lines 22 - 30, The descriptionExpr helper currently returns 'COALESCE(l.descrizione_plain, l.descrizione)' which treats empty strings as valid values and prevents falling back; update the return expression in descriptionExpr (and keep using the cached self::$hasDescrizionePlain logic) to use a blank-safe fallback: 'COALESCE(NULLIF(l.descrizione_plain, ''), l.descrizione)' so empty descrizione_plain values will fall back to l.descrizione everywhere this helper is used.app/Controllers/LibriApiController.php (1)
45-48: UseCOALESCE(NULLIF(..., ''), ...)to treat empty-string descriptions as NULL.
COALESCE(l.descrizione_plain, l.descrizione)works correctly whendescrizione_plainis NULL (the default), butstrip_tags()in BookRepository and LibraryThingImportController can produce empty strings when the originaldescrizionecontains only HTML tags or whitespace. In such cases, COALESCE returns''instead of falling back tol.descrizione. Wrapping withNULLIF(l.descrizione_plain, '')ensures consistent fallback behavior during data migrations and mixed-version scenarios.💡 Suggested change
- $descExpr = $this->hasTableColumn($db, 'libri', 'descrizione_plain') - ? 'COALESCE(l.descrizione_plain, l.descrizione)' + $descExpr = $this->hasTableColumn($db, 'libri', 'descrizione_plain') + ? "COALESCE(NULLIF(l.descrizione_plain, ''), l.descrizione)" : 'l.descrizione';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibriApiController.php` around lines 45 - 48, The COALESCE expression for description should treat empty strings as NULL so fallback to l.descrizione happens when descrizione_plain is ''. Update the $descExpr construction in the block that calls hasTableColumn($db, 'libri', 'descrizione_plain') to use COALESCE(NULLIF(l.descrizione_plain, ''), l.descrizione) instead of COALESCE(l.descrizione_plain, l.descrizione) so queries (and the WHERE clause built with $descExpr) will ignore empty-string descrizione_plain values; adjust the same expression used in the EXISTS subquery string interpolation to keep behavior consistent.
🤖 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/LibraryThingImportController.php`:
- Around line 700-708: The current import logic in LibraryThingImportController
collects Primary and Secondary Author into $authors and always stores them into
$result['autori'], which causes translator entries (per Secondary Author Roles)
to be misclassified and never assigned to $result['traduttore']; update the
block that builds $authors to check the 'Secondary Author Roles' value for the
secondary entry and, when that role indicates a translator, push the normalized
name (using \App\Support\AuthorNormalizer::normalize(trim(...))) into
$result['traduttore'] (concatenated with '|' when multiple) instead of into
$authors, and only push non-translator secondaries into $authors; ensure
$result['autori'] remains the '|' join of $authors and $result['traduttore'] is
set to '' when empty.
---
Duplicate comments:
In `@app/Controllers/CollaneController.php`:
- Around line 276-284: The current prepare() for $stmtSync can fail silently due
to the if ($stmtSync) guard, allowing the transaction to commit with only the
libri updates; change the flow so prepare() returning false triggers an
immediate rollback and exception: after calling $db->prepare("UPDATE collane
...") check for a falsy result and call $db->rollback() then throw a
RuntimeException (including $db->error), otherwise proceed to bind/execute/close
and let the outer transaction logic continue; ensure the rollback call targets
the same DB connection used for the libri update so the rename remains atomic.
In `@app/Controllers/LibraryThingImportController.php`:
- Around line 1146-1163: The UPDATE statements that write to the libri table
(the prepared statement building "UPDATE libri SET ... WHERE id = ?") lack the
soft-delete guard and can race a soft-delete; modify both UPDATE branches (the
one shown and the other UPDATE around the 1229-1237 area) to append "AND
deleted_at IS NULL" to the WHERE clause so every libri update includes the
soft-delete condition; locate the UPDATE SQL in LibraryThingImportController
(the prepared statements) and add the soft-delete predicate to each WHERE id =
?.
- Around line 790-793: The current generation of descrizione_plain using
strip_tags(trim($data['Summary'])) loses spacing between block elements and
keeps HTML entities; update the logic that sets $result['descrizione_plain']
(currently computed from $result['descrizione'] after strip_tags) to: 1) decode
HTML entities with html_entity_decode(..., ENT_QUOTES|ENT_HTML5, 'UTF-8'); 2)
replace common block-level tags (e.g., p, div, br, li, h1..h6, blockquote) with
a single space or newline using preg_replace before stripping tags; 3) run
strip_tags() and then normalize internal whitespace (preg_replace('/\s+/',' ',
...)) and trim the result so descrizione_plain has real plain text spacing and
decoded entities. Ensure changes touch the code that assigns
$result['descrizione'] and $result['descrizione_plain'] and keep trim() behavior
for the original descrizione.
---
Nitpick comments:
In `@app/Controllers/FrontendController.php`:
- Around line 22-30: The descriptionExpr helper currently returns
'COALESCE(l.descrizione_plain, l.descrizione)' which treats empty strings as
valid values and prevents falling back; update the return expression in
descriptionExpr (and keep using the cached self::$hasDescrizionePlain logic) to
use a blank-safe fallback: 'COALESCE(NULLIF(l.descrizione_plain, ''),
l.descrizione)' so empty descrizione_plain values will fall back to
l.descrizione everywhere this helper is used.
In `@app/Controllers/LibriApiController.php`:
- Around line 45-48: The COALESCE expression for description should treat empty
strings as NULL so fallback to l.descrizione happens when descrizione_plain is
''. Update the $descExpr construction in the block that calls
hasTableColumn($db, 'libri', 'descrizione_plain') to use
COALESCE(NULLIF(l.descrizione_plain, ''), l.descrizione) instead of
COALESCE(l.descrizione_plain, l.descrizione) so queries (and the WHERE clause
built with $descExpr) will ignore empty-string descrizione_plain values; adjust
the same expression used in the EXISTS subquery string interpolation to keep
behavior consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 10460516-a394-43f7-87fa-bc59dc59bd62
📒 Files selected for processing (6)
app/Controllers/CollaneController.phpapp/Controllers/FrontendController.phpapp/Controllers/LibraryThingImportController.phpapp/Controllers/LibriApiController.phpapp/Controllers/PublicApiController.phpapp/Views/frontend/book-detail.php
- CollaneController::rename(): abort on prepare() failure (was silent) - NULLIF in COALESCE for empty descrizione_plain fallback - Better plain text: html_entity_decode + tag→space + whitespace normalize - Soft-delete guard on all LT import UPDATE queries - descrizione_plain conditional in all 4 LT SQL variants - Secondary Author Roles routed to traduttore when translator
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/Controllers/LibraryThingImportController.php (1)
1179-1239:⚠️ Potential issue | 🔴 CriticalDon't treat a guarded
UPDATEthat matched no live row as a successful upsert.With
AND deleted_at IS NULL, a concurrent soft-delete turns this into a silent no-op.upsertBook()still returnsupdated, andprocessChunk()then deletes/recreateslibri_autorifor a book that no longer has a livelibrirow. Please fail the row in this case instead of continuing.Also applies to: 1254-1285
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibraryThingImportController.php` around lines 1179 - 1239, The guarded UPDATE using "AND deleted_at IS NULL" can match zero rows (e.g. due to concurrent soft-delete) but is currently treated as a successful upsert, causing processChunk() to proceed and touch libri_autori for a non-live libri row; change upsertBook() so it distinguishes a no-op UPDATE from a real update: after executing the UPDATE statement check the statement's affected row count (e.g. PDOStatement::rowCount()) and only return "updated" when rowCount() > 0; if the UPDATE affected 0 rows, return a failure (or special flag) so processChunk() will treat the row as failed instead of continuing to delete/recreate libri_autori; apply the same fix for the other upsert path referenced around lines 1254-1285 so all guarded UPDATEs behave the same.
🧹 Nitpick comments (1)
app/Controllers/LibraryThingImportController.php (1)
1152-1160: Cache thedescrizione_plainschema probe.
SHOW COLUMNS FROM libri LIKE 'descrizione_plain'is duplicated here and runs for every book upsert. A small cached helper would remove the extra metadata round-trips on long imports and keep the insert/update branches from drifting.Also applies to: 1301-1310
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibraryThingImportController.php` around lines 1152 - 1160, The code repeatedly runs SHOW COLUMNS FROM libri LIKE 'descrizione_plain' for every upsert (using $checkCol/$hasDescPlain/$descPlainSet/$descPlainType), causing extra DB round-trips; move this probe out of the per-book loop and cache the boolean once (e.g., compute $hasDescPlain once at start of the import or in a small helper method) and then reuse the cached $hasDescPlain to build $descPlainSet/$descPlainType for all inserts/updates (also apply the same change for the duplicate probe around lines referencing the same logic at 1301-1310).
🤖 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/LibraryThingImportController.php`:
- Around line 700-712: Export loses translator-only secondary authors because
import stores translators in result['traduttore'] but formatLibraryThingRow()
still builds "Secondary Author" only from autori_nomi and leaves "Secondary
Author Roles" empty; update formatLibraryThingRow() to check
result['traduttore'] and, when present and not already in autori_nomi, populate
the "Secondary Author" field with that name and set "Secondary Author Roles" to
"translator" (or localized equivalent), and ensure autori_nomi/Authors output
includes the primary author plus the secondary when appropriate so an imported
translator is preserved on re-export.
---
Outside diff comments:
In `@app/Controllers/LibraryThingImportController.php`:
- Around line 1179-1239: The guarded UPDATE using "AND deleted_at IS NULL" can
match zero rows (e.g. due to concurrent soft-delete) but is currently treated as
a successful upsert, causing processChunk() to proceed and touch libri_autori
for a non-live libri row; change upsertBook() so it distinguishes a no-op UPDATE
from a real update: after executing the UPDATE statement check the statement's
affected row count (e.g. PDOStatement::rowCount()) and only return "updated"
when rowCount() > 0; if the UPDATE affected 0 rows, return a failure (or special
flag) so processChunk() will treat the row as failed instead of continuing to
delete/recreate libri_autori; apply the same fix for the other upsert path
referenced around lines 1254-1285 so all guarded UPDATEs behave the same.
---
Nitpick comments:
In `@app/Controllers/LibraryThingImportController.php`:
- Around line 1152-1160: The code repeatedly runs SHOW COLUMNS FROM libri LIKE
'descrizione_plain' for every upsert (using
$checkCol/$hasDescPlain/$descPlainSet/$descPlainType), causing extra DB
round-trips; move this probe out of the per-book loop and cache the boolean once
(e.g., compute $hasDescPlain once at start of the import or in a small helper
method) and then reuse the cached $hasDescPlain to build
$descPlainSet/$descPlainType for all inserts/updates (also apply the same change
for the duplicate probe around lines referencing the same logic at 1301-1310).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: be38ce1d-2208-47a6-9ab1-77e40dda91f0
📒 Files selected for processing (4)
app/Controllers/CollaneController.phpapp/Controllers/FrontendController.phpapp/Controllers/LibraryThingImportController.phpapp/Controllers/LibriApiController.php
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Controllers/FrontendController.php
- formatLibraryThingRow(): export traduttore as Secondary Author with role "Translator" — completes import/export roundtrip (CodeRabbit #7) - AuthController: add locale to SELECT, store in $_SESSION on login - RememberMeMiddleware: same locale fix for auto-login via cookie - ProfileController: read/save locale, apply to session immediately - profile/index.php: language selector dropdown (visible when >1 locale) - full-test.spec.js: test 18.11 sets both descrizione and descrizione_plain Closes #97
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/Controllers/LibraryThingImportController.php (2)
1152-1159: Remove unused variable$descPlainType.The variable is assigned but never used since
inferBindTypes()dynamically determines bind types. This was flagged by static analysis.🧹 Proposed fix
$hasDescPlain = false; $checkCol = $db->query("SHOW COLUMNS FROM libri LIKE 'descrizione_plain'"); if ($checkCol && $checkCol->num_rows > 0) { $hasDescPlain = true; } $descPlainSet = $hasDescPlain ? ', descrizione_plain = ?' : ''; - $descPlainType = $hasDescPlain ? 's' : '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibraryThingImportController.php` around lines 1152 - 1159, Remove the unused variable $descPlainType from the LibraryThingImportController import block: keep the logic that detects the descrizione_plain column and the $descPlainSet string, but delete the assignment "$descPlainType = $hasDescPlain ? 's' : ''" and any dead references to $descPlainType so that bind types continue to be handled by inferBindTypes() only; ensure no other code relies on $descPlainType before committing.
1296-1305: Remove unused variable$descPlainType.Same issue as in
updateBook()- this variable is assigned but never used.🧹 Proposed fix
$hasDescPlain = false; $checkCol = $db->query("SHOW COLUMNS FROM libri LIKE 'descrizione_plain'"); if ($checkCol && $checkCol->num_rows > 0) { $hasDescPlain = true; } $descPlainCol = $hasDescPlain ? ', descrizione_plain' : ''; $descPlainVal = $hasDescPlain ? ', ?' : ''; - $descPlainType = $hasDescPlain ? 's' : '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibraryThingImportController.php` around lines 1296 - 1305, The variable $descPlainType is never used and should be removed to avoid dead code; locate the block that sets $hasDescPlain, $descPlainCol, $descPlainVal and the unused $descPlainType in LibraryThingImportController (and mirror the same change in updateBook() if present), delete the $descPlainType assignment and any references to it, and ensure any parameter-type strings or bind operations continue to work using only the actually-used variables ($descPlainVal and other type strings) so the prepared statement parameter handling remains correct.
🤖 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/ProfileController.php`:
- Around line 165-167: The session locale is only set when $locale !== null so a
removed preference leaves $_SESSION['locale'] stale; in the ProfileController
method containing the if ($locale !== null) { $_SESSION['locale'] = $locale; }
branch, update the logic to remove the session key when $locale is null (e.g.
unset($_SESSION['locale']) or assign null/empty) so the session mirrors the
cleared DB value; ensure you reference and modify $_SESSION['locale'] in that
same conditional block.
In `@app/Middleware/RememberMeMiddleware.php`:
- Around line 94-97: RememberMeMiddleware currently writes the user's locale
into $_SESSION['locale'] but does not apply it immediately, so the current
request still uses the earlier bootstrapped locale; after setting
$_SESSION['locale'] in RememberMeMiddleware (handle method), also apply the
locale to the runtime translator/locale system used by the app (for example call
the same bootstrap locale initializer or invoke the translator/app locale setter
— e.g. translator->setLocale($row['locale']) or App::setLocale($row['locale']) /
Locale::setDefault($row['locale']) depending on your app) so the remainder of
this request renders with the new language. Ensure you reference and call the
same mechanism the bootstrap uses to avoid duplicate logic.
---
Nitpick comments:
In `@app/Controllers/LibraryThingImportController.php`:
- Around line 1152-1159: Remove the unused variable $descPlainType from the
LibraryThingImportController import block: keep the logic that detects the
descrizione_plain column and the $descPlainSet string, but delete the assignment
"$descPlainType = $hasDescPlain ? 's' : ''" and any dead references to
$descPlainType so that bind types continue to be handled by inferBindTypes()
only; ensure no other code relies on $descPlainType before committing.
- Around line 1296-1305: The variable $descPlainType is never used and should be
removed to avoid dead code; locate the block that sets $hasDescPlain,
$descPlainCol, $descPlainVal and the unused $descPlainType in
LibraryThingImportController (and mirror the same change in updateBook() if
present), delete the $descPlainType assignment and any references to it, and
ensure any parameter-type strings or bind operations continue to work using only
the actually-used variables ($descPlainVal and other type strings) so the
prepared statement parameter handling remains correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 78b910b8-b934-4e2f-90b9-511129d58236
📒 Files selected for processing (6)
app/Controllers/AuthController.phpapp/Controllers/LibraryThingImportController.phpapp/Controllers/ProfileController.phpapp/Middleware/RememberMeMiddleware.phpapp/Views/profile/index.phptests/full-test.spec.js
- Remove unused $descPlainType in updateBook() and insertBook() - unset($_SESSION['locale']) when profile locale cleared to null - Apply I18n::setLocale() immediately in RememberMeMiddleware
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/full-test.spec.js (1)
2375-2384:⚠️ Potential issue | 🟠 MajorMake this fixture prove
descrizione_plainis the field being searched.With the same token in both columns, this still passes if the API keeps using
descrizione. Seed HTML indescrizionethat breaks the contiguous search term, and keep the searchable plain text only indescrizione_plain.🧪 Example adjustment
test.skip(!targetId, 'No seeded book id available'); - const marker = `desc-${RUN_ID}`; - dbQuery(`UPDATE libri SET descrizione='${marker}', descrizione_plain='${marker}' WHERE id=${targetId}`); + const searchTerm = `desc ${RUN_ID}`; + dbQuery( + `UPDATE libri SET descrizione='<p>desc <strong>${RUN_ID}</strong></p>', descrizione_plain='${searchTerm}' WHERE id=${targetId}` + ); await page.goto(`${BASE}/admin/libri`); await page.waitForLoadState('domcontentloaded'); - const searchTerm = marker; const apiResp = await page.request.get(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/full-test.spec.js` around lines 2375 - 2384, Change the test fixture so it proves the API searches descrizione_plain, not descrizione: when calling dbQuery update (currently using marker and targetId), set descrizione to an HTML string that breaks the contiguous token (e.g., insert tags or whitespace inside the token) and set descrizione_plain to the searchable marker string; keep the searchTerm and the API request to /api/libri unchanged so the test will only pass if the server searches descrizione_plain rather than the HTML-broken descrizione.
♻️ Duplicate comments (2)
app/Controllers/ProfileController.php (1)
132-167:⚠️ Potential issue | 🟠 MajorDon’t treat an omitted
localefield as a locale reset.
app/Views/profile/index.phponly postslocalewhen more than one locale is available. In single-locale installs this branch writesNULLtoutenti.locale, and the success block leaves the previous$_SESSION['locale']behind. Preserve the stored value unless the field is actually posted; when it is posted, update/unset the session and apply it immediately via the same I18n setter before building the redirect.💡 One way to guard the update/session flow
- $locale = trim(strip_tags((string) ($data['locale'] ?? ''))); + $localeProvided = array_key_exists('locale', $data); + $locale = $localeProvided ? trim(strip_tags((string) $data['locale'])) : null; ... // Validate locale - only allow known locales $availableLocales = \App\Support\I18n::getAvailableLocales(); - $locale = (!empty($locale) && isset($availableLocales[$locale])) ? $locale : null; + if ($localeProvided) { + $locale = ($locale !== '' && isset($availableLocales[$locale])) ? $locale : null; + }- $stmt = $db->prepare("UPDATE utenti SET nome = ?, cognome = ?, telefono = ?, data_nascita = ?, cod_fiscale = ?, sesso = ?, indirizzo = ?, locale = ? WHERE id = ?"); - $stmt->bind_param('ssssssssi', $nome, $cognome, $telefono, $data_nascita, $cod_fiscale, $sesso, $indirizzo, $locale, $uid); + if ($localeProvided) { + $stmt = $db->prepare("UPDATE utenti SET nome = ?, cognome = ?, telefono = ?, data_nascita = ?, cod_fiscale = ?, sesso = ?, indirizzo = ?, locale = ? WHERE id = ?"); + $stmt->bind_param('ssssssssi', $nome, $cognome, $telefono, $data_nascita, $cod_fiscale, $sesso, $indirizzo, $locale, $uid); + } else { + $stmt = $db->prepare("UPDATE utenti SET nome = ?, cognome = ?, telefono = ?, data_nascita = ?, cod_fiscale = ?, sesso = ?, indirizzo = ? WHERE id = ?"); + $stmt->bind_param('sssssssi', $nome, $cognome, $telefono, $data_nascita, $cod_fiscale, $sesso, $indirizzo, $uid); + } ... - if ($locale !== null) { - $_SESSION['locale'] = $locale; - } + if ($localeProvided) { + if ($locale !== null) { + \App\Support\I18n::setLocale($locale); + $_SESSION['locale'] = $locale; + } else { + unset($_SESSION['locale']); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/ProfileController.php` around lines 132 - 167, The code treats an omitted locale as an explicit reset to NULL; change the flow in ProfileController so you only touch utenti.locale and session locale when the locale field was actually POSTed: detect presence via the $data array (e.g., isset($data['locale'])), and if present validate and include locale in the UPDATE and in the session (calling the I18n setter used elsewhere), otherwise omit locale from the SQL UPDATE (or build a different prepared statement without the locale column) and leave $_SESSION['locale'] and the DB value unchanged; ensure the bind_param list and $stmt->execute() call match the chosen SQL variant and apply the I18n setter immediately when you do update the locale.app/Middleware/RememberMeMiddleware.php (1)
94-97:⚠️ Potential issue | 🟠 MajorApply the remembered locale before continuing the request.
Writing only
$_SESSION['locale']is too late for the response currently being built, so the first remember-me page can still render with the previous language. Call the same locale-application path used by the normal locale switch flow here as well.💡 Minimal fix
- // Load user's preferred locale into session - if (!empty($row['locale'])) { - $_SESSION['locale'] = $row['locale']; - } + // Apply the remembered locale for this same request + if (!empty($row['locale'])) { + $locale = (string) $row['locale']; + \App\Support\I18n::setLocale($locale); + $_SESSION['locale'] = $locale; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Middleware/RememberMeMiddleware.php` around lines 94 - 97, You currently only set $_SESSION['locale'] in RememberMeMiddleware.php, which is too late for the current response; after setting the session key call the same locale-application routine used by the normal locale switch flow (i.e. invoke the function that applies the session locale to the current request/response instead of only writing $_SESSION['locale']). Locate the existing locale application entrypoint used elsewhere (e.g. the function/class that handles the normal locale switch—call it applyLocaleFromSession/applyLocale/setLocaleFromSession) and invoke it immediately after setting $_SESSION['locale'] so the remembered locale is applied for the current request.
🧹 Nitpick comments (2)
app/Controllers/LibraryThingImportController.php (2)
1159-1159: Remove unused$descPlainTypevariable.The variable
$descPlainTypeis assigned but never used sinceinferBindTypes()dynamically infers types. This dead code should be cleaned up.🧹 Proposed fix
$hasDescPlain = true; } $descPlainSet = $hasDescPlain ? ', descrizione_plain = ?' : ''; - $descPlainType = $hasDescPlain ? 's' : '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibraryThingImportController.php` at line 1159, Remove the dead variable assignment for $descPlainType in LibraryThingImportController.php: locate the line that sets $descPlainType = $hasDescPlain ? 's' : '' and delete it because inferBindTypes() already infers types dynamically and $descPlainType is never used; ensure no other code references $descPlainType (search for $descPlainType) and run tests to confirm no regressions.
1304-1304: Remove unused$descPlainTypevariable.Same issue as in
updateBook()— this variable is assigned but never referenced.🧹 Proposed fix
$hasDescPlain = true; } $descPlainCol = $hasDescPlain ? ', descrizione_plain' : ''; $descPlainVal = $hasDescPlain ? ', ?' : ''; - $descPlainType = $hasDescPlain ? 's' : '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibraryThingImportController.php` at line 1304, The variable $descPlainType is assigned but never used in LibraryThingImportController (same as in updateBook()); remove the unused assignment ($descPlainType = $hasDescPlain ? 's' : '') and any other dead references or related unused variables in the same method (and remove the duplicate unused assignment if present in updateBook()) so there are no leftover unused variables or meaningless logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/full-test.spec.js`:
- Around line 2375-2384: Change the test fixture so it proves the API searches
descrizione_plain, not descrizione: when calling dbQuery update (currently using
marker and targetId), set descrizione to an HTML string that breaks the
contiguous token (e.g., insert tags or whitespace inside the token) and set
descrizione_plain to the searchable marker string; keep the searchTerm and the
API request to /api/libri unchanged so the test will only pass if the server
searches descrizione_plain rather than the HTML-broken descrizione.
---
Duplicate comments:
In `@app/Controllers/ProfileController.php`:
- Around line 132-167: The code treats an omitted locale as an explicit reset to
NULL; change the flow in ProfileController so you only touch utenti.locale and
session locale when the locale field was actually POSTed: detect presence via
the $data array (e.g., isset($data['locale'])), and if present validate and
include locale in the UPDATE and in the session (calling the I18n setter used
elsewhere), otherwise omit locale from the SQL UPDATE (or build a different
prepared statement without the locale column) and leave $_SESSION['locale'] and
the DB value unchanged; ensure the bind_param list and $stmt->execute() call
match the chosen SQL variant and apply the I18n setter immediately when you do
update the locale.
In `@app/Middleware/RememberMeMiddleware.php`:
- Around line 94-97: You currently only set $_SESSION['locale'] in
RememberMeMiddleware.php, which is too late for the current response; after
setting the session key call the same locale-application routine used by the
normal locale switch flow (i.e. invoke the function that applies the session
locale to the current request/response instead of only writing
$_SESSION['locale']). Locate the existing locale application entrypoint used
elsewhere (e.g. the function/class that handles the normal locale switch—call it
applyLocaleFromSession/applyLocale/setLocaleFromSession) and invoke it
immediately after setting $_SESSION['locale'] so the remembered locale is
applied for the current request.
---
Nitpick comments:
In `@app/Controllers/LibraryThingImportController.php`:
- Line 1159: Remove the dead variable assignment for $descPlainType in
LibraryThingImportController.php: locate the line that sets $descPlainType =
$hasDescPlain ? 's' : '' and delete it because inferBindTypes() already infers
types dynamically and $descPlainType is never used; ensure no other code
references $descPlainType (search for $descPlainType) and run tests to confirm
no regressions.
- Line 1304: The variable $descPlainType is assigned but never used in
LibraryThingImportController (same as in updateBook()); remove the unused
assignment ($descPlainType = $hasDescPlain ? 's' : '') and any other dead
references or related unused variables in the same method (and remove the
duplicate unused assignment if present in updateBook()) so there are no leftover
unused variables or meaningless logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 581e6923-9e71-4257-82d1-d772a6fb6230
📒 Files selected for processing (11)
app/Controllers/AuthController.phpapp/Controllers/CollaneController.phpapp/Controllers/FrontendController.phpapp/Controllers/LibraryThingImportController.phpapp/Controllers/LibriApiController.phpapp/Controllers/ProfileController.phpapp/Controllers/PublicApiController.phpapp/Middleware/RememberMeMiddleware.phpapp/Views/frontend/book-detail.phpapp/Views/profile/index.phptests/full-test.spec.js
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/Controllers/ProfileController.php (1)
171-171: UseSecureLogger::error()instead oferror_log()for consistency.Line 171 uses
error_log()while the rest of this controller correctly usesSecureLogger::error()for database errors (lines 64-67, 99-102). This context involves user ID which warrants the secure logger.Suggested fix
- error_log("Profile update error for user $uid: " . $stmt->error); + SecureLogger::error('ProfileController: profile update failed', [ + 'user_id' => $uid, + 'db_error' => $stmt->error + ]);As per coding guidelines: "Use
SecureLogger::error()noterror_log()for sensitive contexts".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/ProfileController.php` at line 171, Replace the direct call to error_log with the project's secure logging utility: use SecureLogger::error(...) in the ProfileController where the DB error is handled (replace the error_log("Profile update error for user $uid: " . $stmt->error) call). Ensure the message includes the same context (the $uid and $stmt->error) and follows the existing SecureLogger::error usage pattern used elsewhere in this controller so sensitive data is logged consistently and securely.app/Controllers/FrontendController.php (1)
20-31: Extract this schema probe into a shared helper.
LibriApiControllernow carries a second cached column-existence path for the samedescrizione_plainfallback. Keeping these checks duplicated is an easy way for admin and catalog search to drift again later; a shared helper/service would keep the behavior aligned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/FrontendController.php` around lines 20 - 31, Extract the duplicated "descrizione_plain" existence probe into a shared helper/service so both FrontendController::descriptionExpr and LibriApiController can reuse it; create a single utility (e.g., a SchemaHelper or DescriptionFallback service) with a cached method like hasColumn(mysqli $db, string $table, string $column) or descriptionExpression(mysqli $db) that performs the SHOW COLUMNS check and caches the boolean, then replace the inline logic in FrontendController::descriptionExpr and the corresponding method in LibriApiController to call the shared helper's method to return the same COALESCE/NULLIF expression or 'l.descrizione'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/full-test.spec.js`:
- Line 2376: The test currently runs dbQuery(`UPDATE libri SET
descrizione='${marker}', descrizione_plain='${marker}' WHERE id=${targetId}`)
which assumes the descrizione_plain column exists; instead run a schema check
(e.g., via dbQuery("PRAGMA table_info('libri')") or equivalent
information_schema query) to detect whether 'descrizione_plain' is present and
only include the descrizione_plain=... assignment when that check returns true,
otherwise issue an UPDATE that sets only descrizione; keep using the existing
variables marker and targetId and the dbQuery helper so the test exercises the
same compatibility fallback path as production.
---
Nitpick comments:
In `@app/Controllers/FrontendController.php`:
- Around line 20-31: Extract the duplicated "descrizione_plain" existence probe
into a shared helper/service so both FrontendController::descriptionExpr and
LibriApiController can reuse it; create a single utility (e.g., a SchemaHelper
or DescriptionFallback service) with a cached method like hasColumn(mysqli $db,
string $table, string $column) or descriptionExpression(mysqli $db) that
performs the SHOW COLUMNS check and caches the boolean, then replace the inline
logic in FrontendController::descriptionExpr and the corresponding method in
LibriApiController to call the shared helper's method to return the same
COALESCE/NULLIF expression or 'l.descrizione'.
In `@app/Controllers/ProfileController.php`:
- Line 171: Replace the direct call to error_log with the project's secure
logging utility: use SecureLogger::error(...) in the ProfileController where the
DB error is handled (replace the error_log("Profile update error for user $uid:
" . $stmt->error) call). Ensure the message includes the same context (the $uid
and $stmt->error) and follows the existing SecureLogger::error usage pattern
used elsewhere in this controller so sensitive data is logged consistently and
securely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e19a3cf3-a053-4972-9461-a75d644a52b7
📒 Files selected for processing (11)
app/Controllers/AuthController.phpapp/Controllers/CollaneController.phpapp/Controllers/FrontendController.phpapp/Controllers/LibraryThingImportController.phpapp/Controllers/LibriApiController.phpapp/Controllers/ProfileController.phpapp/Controllers/PublicApiController.phpapp/Middleware/RememberMeMiddleware.phpapp/Views/frontend/book-detail.phpapp/Views/profile/index.phptests/full-test.spec.js
- ProfileController: only touch locale when field is POSTed (single-locale installs don't render the select, so omitted locale was resetting to NULL) - ProfileController: error_log → SecureLogger::error per project rules - Test 18.11: HTML in descrizione breaks token, only descrizione_plain is searchable — proves COALESCE fallback works correctly - Test 18.11: schema-aware UPDATE for descrizione_plain column
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/ProfileController.php`:
- Around line 160-167: The code calls $db->prepare(...) and immediately
$stmt->bind_param(...) in both the $localeProvided and else branches without
checking for prepare() failure; update ProfileController to verify that $stmt is
a valid statement (non-false) after each $db->prepare(...) call and handle
failures (log the error via your logger or $db->error and return/throw an
appropriate response) before calling $stmt->bind_param(...); ensure both the
branch creating a statement with locale and the branch without locale perform
this check and short-circuit on failure to avoid calling bind_param on a false
value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 225f8b70-dc38-49ab-bee1-2b90d46862f3
📒 Files selected for processing (2)
app/Controllers/ProfileController.phptests/full-test.spec.js
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Check $stmt before bind_param() to avoid fatal error on prepare failure, consistent with existing pattern in changePassword().
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
app/Controllers/AuthController.php (1)
109-112: MissingI18n::setLocale()call creates inconsistency with RememberMeMiddleware.The locale is stored in
$_SESSION['locale']butI18n::setLocale()is not called, unlikeRememberMeMiddleware(lines 94-98 in that file) which does both. This means any content rendered in the login response (e.g., flash messages, redirected page content on same request) will use the default locale rather than the user's preference.While the bootstrap code in
public/index.phpwill apply the locale on subsequent requests, this asymmetry could cause the first post-login response to render in the wrong language if it includes localized content.♻️ Suggested fix to match RememberMeMiddleware behavior
// Load user's preferred locale into session if (!empty($row['locale'])) { + $locale = (string) $row['locale']; + \App\Support\I18n::setLocale($locale); - $_SESSION['locale'] = $row['locale']; + $_SESSION['locale'] = $locale; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/AuthController.php` around lines 109 - 112, When storing the user's preferred locale in $_SESSION within AuthController (when $row['locale'] is set), also call I18n::setLocale($row['locale']) so the current request uses the user's locale (matching RememberMeMiddleware's behavior); update the block that checks $row['locale'] to both set $_SESSION['locale'] and call I18n::setLocale with that value to ensure flash messages and immediate response rendering use the correct locale.app/Controllers/ProfileController.php (1)
227-227: Inconsistent error logging: useSecureLogger::error()for sensitive contexts.This method uses
error_log()while theupdate()method correctly usesSecureLogger::error(). For consistency and to comply with the coding guideline about sensitive contexts, consider updating this and the other API methods (revokeSession,revokeAllSessions) to useSecureLogger::error().♻️ Suggested fix
- error_log("getSessions error for user $uid: " . $e->getMessage()); + SecureLogger::error('ProfileController: getSessions failed', [ + 'user_id' => $uid, + 'error' => $e->getMessage() + ]);Similar changes for lines 272 and 307.
As per coding guidelines: "Use
SecureLogger::error()noterror_log()for sensitive contexts"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/ProfileController.php` at line 227, Replace the use of PHP's global error_log() with SecureLogger::error() in the controller methods that handle sensitive session operations: getSessions, revokeSession, and revokeAllSessions; locate the error_log(...) calls (e.g., the one inside getSessions that logs "$uid: " . $e->getMessage()) and change them to call SecureLogger::error() with the same contextual message and include the exception message/metadata so logs remain informative while conforming to the secure logging guideline.app/Controllers/LibraryThingImportController.php (1)
1152-1159: Cache thedescrizione_plainschema probe.
SHOW COLUMNS FROM libri LIKE 'descrizione_plain'now runs on every book write. On larger LibraryThing imports that turns schema introspection into a hot path for no functional benefit; cache the result once per request/controller and reuse it in both methods.Also applies to: 1295-1303
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibraryThingImportController.php` around lines 1152 - 1159, The schema probe "SHOW COLUMNS FROM libri LIKE 'descrizione_plain'" is being executed on every book write; compute it once and cache the boolean on the controller instance (e.g. add a private property $hasDescPlain and a getter like getHasDescPlain() that performs the query lazily) so both places use the cached value instead of re-running the query; replace local $hasDescPlain and repeated query logic with calls to the cached property/getter and keep $descPlainSet = $this->getHasDescPlain() ? ', descrizione_plain = ?' : '' where needed.
🤖 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/FrontendController.php`:
- Around line 22-31: The schema probe in descriptionExpr currently assumes
$db->query will return false on failure but with MYSQLI_REPORT_STRICT enabled it
can throw mysqli_sql_exception; wrap the $db->query(...) call in a try/catch
(catch \Throwable) around the block that sets self::$hasDescrizionePlain so that
on any exception you set self::$hasDescrizionePlain = false and proceed to
return the fallback 'l.descrizione'; ensure you catch exceptions around the
specific query call inside descriptionExpr and do not change the returned string
logic.
In `@app/Controllers/LibraryThingImportController.php`:
- Around line 797-800: When handling scraped descriptions, ensure the normalized
plain text is written to the descrizione_plain column instead of only storing
raw HTML in descrizione: compute the same HTML→plain normalization you already
use (same preg_replace/html_entity_decode/trim flow) and assign it to
$result['descrizione_plain'] when hasDescrizionePlainColumn() returns true; also
reuse the cached schema probe behind hasDescrizionePlainColumn() so updateBook()
/ insertBook() will include descrizione_plain in their DB writes (apply the same
fix for the other occurrence around the 1555–1562 block).
- Around line 705-712: The current logic treats the entire $data['Secondary
Author'] string as one token and classifies it as a translator if any of the
combined roles contains "translator", which drops co-authors; instead split the
secondary authors and their roles pairwise: explode $data['Secondary Author'] on
';' into an array, explode $data['Secondary Author Roles'] on ';' into a
parallel array, trim each element, then iterate by index and for each pair check
the role string (e.g. $secondaryRolesParts[$i]) for 'translator'/'traduttore'
and push the corresponding normalized author into $result['traduttore'] or into
$authors accordingly (handling unequal lengths by treating missing role as
non-translator); apply the same change to the other identical block that uses
the same variables ($secondaryAuthor, $secondaryRoles, $result['traduttore'],
$authors).
---
Nitpick comments:
In `@app/Controllers/AuthController.php`:
- Around line 109-112: When storing the user's preferred locale in $_SESSION
within AuthController (when $row['locale'] is set), also call
I18n::setLocale($row['locale']) so the current request uses the user's locale
(matching RememberMeMiddleware's behavior); update the block that checks
$row['locale'] to both set $_SESSION['locale'] and call I18n::setLocale with
that value to ensure flash messages and immediate response rendering use the
correct locale.
In `@app/Controllers/LibraryThingImportController.php`:
- Around line 1152-1159: The schema probe "SHOW COLUMNS FROM libri LIKE
'descrizione_plain'" is being executed on every book write; compute it once and
cache the boolean on the controller instance (e.g. add a private property
$hasDescPlain and a getter like getHasDescPlain() that performs the query
lazily) so both places use the cached value instead of re-running the query;
replace local $hasDescPlain and repeated query logic with calls to the cached
property/getter and keep $descPlainSet = $this->getHasDescPlain() ? ',
descrizione_plain = ?' : '' where needed.
In `@app/Controllers/ProfileController.php`:
- Line 227: Replace the use of PHP's global error_log() with
SecureLogger::error() in the controller methods that handle sensitive session
operations: getSessions, revokeSession, and revokeAllSessions; locate the
error_log(...) calls (e.g., the one inside getSessions that logs "$uid: " .
$e->getMessage()) and change them to call SecureLogger::error() with the same
contextual message and include the exception message/metadata so logs remain
informative while conforming to the secure logging guideline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3af0875b-7e04-4796-8817-a7a5ad1a83e0
📒 Files selected for processing (11)
app/Controllers/AuthController.phpapp/Controllers/CollaneController.phpapp/Controllers/FrontendController.phpapp/Controllers/LibraryThingImportController.phpapp/Controllers/LibriApiController.phpapp/Controllers/ProfileController.phpapp/Controllers/PublicApiController.phpapp/Middleware/RememberMeMiddleware.phpapp/Views/frontend/book-detail.phpapp/Views/profile/index.phptests/full-test.spec.js
- AuthController: call I18n::setLocale() on login (consistent with RememberMe) - FrontendController: try/catch on schema probe (MYSQLI_REPORT_STRICT) - ProfileController: error_log → SecureLogger::error in 3 API methods - LibraryThingImportController: cache hasDescPlain via instance method - LibraryThingImportController: scraping path generates descrizione_plain
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/Controllers/FrontendController.php (1)
20-38: Consider sharing thedescrizione_plaincolumn probe.
FrontendControllernow has a second schema-detection path for the same feature thatapp/Controllers/LibriApiController.php::hasTableColumn()already handles. Pulling this behind a shared helper would reduce drift between frontend and admin search behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/FrontendController.php` around lines 20 - 38, The descriptionExpr method duplicates schema-probing logic already implemented by LibriApiController::hasTableColumn; extract or reuse that probe so both controllers share one implementation. Replace the inline probe in FrontendController::descriptionExpr (and remove the static $hasDescrizionePlain) with a call to the shared helper (e.g. SchemaInspector::hasTableColumn or LibriApiController::hasTableColumn) to check for the libri.descrizione_plain column, and use its boolean result to return the same COALESCE expression; ensure the helper accepts a mysqli (or wraps connection handling) and caches results to preserve current behavior.
🤖 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/book-detail.php`:
- Around line 254-256: Replace the current use of $bookSchema["issn"] when
$book['issn'] is set by adding an identifier entry on the Book JSON-LD object
using a PropertyValue structure: set $bookSchema["identifier"] to an object with
"@type" = "PropertyValue", "propertyID" = "ISSN" and "value" = $book['issn'];
remove the $bookSchema["issn"] assignment and ensure this runs in the same
conditional that checks !empty($book['issn']) in book-detail.php so the ISSN is
represented as a schema.org identifier rather than an issn property.
---
Nitpick comments:
In `@app/Controllers/FrontendController.php`:
- Around line 20-38: The descriptionExpr method duplicates schema-probing logic
already implemented by LibriApiController::hasTableColumn; extract or reuse that
probe so both controllers share one implementation. Replace the inline probe in
FrontendController::descriptionExpr (and remove the static $hasDescrizionePlain)
with a call to the shared helper (e.g. SchemaInspector::hasTableColumn or
LibriApiController::hasTableColumn) to check for the libri.descrizione_plain
column, and use its boolean result to return the same COALESCE expression;
ensure the helper accepts a mysqli (or wraps connection handling) and caches
results to preserve current behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a98fe829-6527-4088-b72e-8a7c7e5502b4
📒 Files selected for processing (11)
app/Controllers/AuthController.phpapp/Controllers/CollaneController.phpapp/Controllers/FrontendController.phpapp/Controllers/LibraryThingImportController.phpapp/Controllers/LibriApiController.phpapp/Controllers/ProfileController.phpapp/Controllers/PublicApiController.phpapp/Middleware/RememberMeMiddleware.phpapp/Views/frontend/book-detail.phpapp/Views/profile/index.phptests/full-test.spec.js
Schema.org "issn" property is defined for serial publications (CreativeWorkSeries, Periodical), not Book. Use "identifier" with PropertyValue structure instead.
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Controllers/LibraryThingImportController.php (1)
1368-1369:⚠️ Potential issue | 🟠 MajorAbort the import when the guarded
libriupdate no longer matches an active row.With the new
AND deleted_at IS NULLpredicates, a book soft-deleted afterfindExistingBook()but beforeupdateBook()turns this execute into a no-op.upsertBook()still reportsupdated, andprocessChunk()then rebuilds author links for that deleted row in the same transaction. Detect the “row no longer active” case here and throw so the transaction rolls back instead of committing a partial update.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibraryThingImportController.php` around lines 1368 - 1369, After calling $stmt->execute() in the guarded libri update (the block reached from upsertBook()/updateBook() after findExistingBook()), check the statement’s affected rows (e.g. $stmt->affected_rows or $mysqli->affected_rows) immediately before $stmt->close(); if it is 0, throw an exception (RuntimeException or similar) so the outer transaction aborts instead of continuing; this ensures processChunk() will not rebuild author links for a row that was soft-deleted between findExistingBook() and updateBook().
♻️ Duplicate comments (1)
app/Views/admin/plugins.php (1)
220-231:⚠️ Potential issue | 🟠 MajorResolve plugin settings endpoints with a named route, not a hardcoded path string.
Wrapping
'/admin/plugins/.../settings'inurl()still hardcodes the route. If admin routes are translated or renamed, these settings modals break again. Please emit the endpoint viaroute_path(...)orRouteTranslator::route(...)instead of concatenating the path here.As per coding guidelines,
Never use hardcoded routes. Use route_path('key') or RouteTranslator::route('key') instead of hardcoded paths like /accedi or /login.Also applies to: 244-245, 262-262, 286-286
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/admin/plugins.php` around lines 220 - 231, The data-settings-url attribute is built from a hardcoded path string; replace the url('/admin/plugins/.../settings') usage with a named-route resolver (route_path with the admin plugins settings route key or RouteTranslator::route) and pass the plugin id as a parameter so the settings endpoint is emitted from the router, e.g. use route_path('admin.plugins.settings', ['id' => (int)$plugin['id']]) (or RouteTranslator::route equivalent) for the data-settings-url used by the openPluginSettingsModal button, and apply the same change to the other occurrences noted (around the other data-settings-url attributes at the referenced blocks).
🧹 Nitpick comments (4)
tests/helpers/e2e-fixtures.js (2)
87-92:getPluginIdByNamereturnsNaNif plugin not found.When the plugin doesn't exist,
dbQueryreturns an empty string, andNumber('')returns0(notNaNas I initially thought). However,0is a falsy value that may not clearly indicate "not found" vs. a plugin with id=0.Consider returning
nullor throwing when the plugin isn't found to make the failure mode explicit:💡 Optional improvement
function getPluginIdByName(name) { const result = dbQuery( `SELECT id FROM plugins WHERE name = '${escapeSql(name)}' LIMIT 1` ); - return Number(result); + if (!result || result.trim() === '') { + return null; + } + return Number(result); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/helpers/e2e-fixtures.js` around lines 87 - 92, getPluginIdByName currently converts dbQuery result to Number which yields 0 for empty string and masks "not found"; update getPluginIdByName to explicitly handle empty/falsey dbQuery responses from dbQuery( ... escapeSql(name) ... ) by checking the raw result first and then either returning null (or throwing an explicit Error) when no row is returned, otherwise parse and return Number(id); keep the function name getPluginIdByName and the dbQuery/escapeSql usage unchanged while making the not-found branch explicit.
70-74:escapeSqlhandles basic cases but consider edge cases for robustness.The current implementation escapes backslashes and single quotes, which covers common cases. However, it doesn't handle null bytes (
\0) or other special characters that could cause issues in MySQL.For test fixtures with controlled inputs, this is acceptable. If you ever need to handle arbitrary data, consider using a more complete escape implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/helpers/e2e-fixtures.js` around lines 70 - 74, The escapeSql function only escapes backslashes and single quotes; update escapeSql to also handle null bytes and other MySQL special characters (e.g., \0, \n, \r, \x1a) by replacing them with their escaped equivalents, or switch to using a proven library escape function (e.g., mysql.escape) or parameterized queries; locate the function escapeSql in tests/helpers/e2e-fixtures.js and either extend its replace chain to include replacements for \0, \n, \r, and \x1a or call the library escape helper so arbitrary input is safely escaped for MySQL in tests.app/Controllers/LibriApiController.php (1)
663-701: Consider extracting shared column detection logic.Both
LibriApiController::hasTableColumn()andFrontendController::descriptionExpr()implement column existence detection but use different approaches (INFORMATION_SCHEMA.COLUMNSvsSHOW COLUMNS). While both are functionally correct, consolidating into a shared utility would improve maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibriApiController.php` around lines 663 - 701, Extract the column-detection logic used in LibriApiController::hasTableColumn and FrontendController::descriptionExpr into a single reusable utility (e.g., SchemaUtils::getTableColumns or DBSchema::columnExists) that accepts a mysqli, table name and returns cached column names or a boolean, centralizes the whitelist check, and uses a single safe query strategy (INFORMATION_SCHEMA or SHOW COLUMNS) with prepared statements; update LibriApiController::hasTableColumn and FrontendController::descriptionExpr to call the new utility and remove duplicated code while preserving existing caching and logging behavior (use the existing tableColCache semantics moved into the utility).app/Controllers/PluginController.php (1)
392-421: Domain validation excludes localhost and IP addresses.The hostname regex requires a valid domain with TLD (at least one dot), which means
localhost,127.0.0.1, or internal IPs won't be accepted. This appears intentional for external mirror domains but may limit testing scenarios.If local testing with custom hosts is needed, consider documenting this restriction or adding an exception for development environments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/PluginController.php` around lines 392 - 421, normalizeGoodLibDomain currently rejects 'localhost' and IP addresses because the hostname regex enforces a dotted TLD domain; update normalizeGoodLibDomain to allow localhost and numeric IPs by adding an early-accept branch: after parsing $parts and obtaining $host, if $host === 'localhost' or it matches an IPv4/IPv6 pattern accept and return $host (with port if present via $port), otherwise fall back to the existing domain regex; optionally gate the localhost/IP acceptance behind a dev check (e.g., APP_ENV === 'local') if you want to restrict it to development.
🤖 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/AuthController.php`:
- Around line 109-114: The session locale is being set from the DB even if
\App\Support\I18n::setLocale($locale) fails; modify the block around
AuthController:: (the code using $row['locale'], \App\Support\I18n::setLocale
and $_SESSION['locale']) so that you only assign $_SESSION['locale'] after
confirming setLocale succeeded — either by checking a boolean return value from
setLocale (if it returns true/false) or by wrapping the call in try/catch and
setting $_SESSION['locale'] inside the success path (or after no exception is
thrown); ensure you use the same $locale value and do not persist the raw DB
value on failure.
In `@app/Controllers/LibraryThingImportController.php`:
- Around line 746-760: normalizeContributorField currently combines multiple
translators into a semicolon-joined string which breaks consumers expecting a
single name; to avoid regressing callers (e.g., the frontend book-detail
template that treats libri.traduttore as one Person) change
normalizeContributorField to preserve the single-value contract: use
splitContributorList($value, '/\s*[;|]\s*/u') and AuthorNormalizer::normalize on
parts as you already do, but return only the first non-empty, unique normalized
name (or null) instead of imploding all names; alternatively, if you
intentionally want multi-value behavior, update downstream consumers to split on
';' and render each name as a separate Person—pick one approach and make
matching changes to normalizeContributorField or the consumer rendering to keep
behavior consistent.
In `@locale/en_US.json`:
- Around line 972-975: The English translation for the key "Mostra badge nella
pagina dettaglio libro" uses singular "Show badge on the book detail page" and
should be changed to the plural form to match the other labels; update that
value to "Show badges on the book detail page" so it aligns with the related
entries ("Mostra i badge nella pagina dettaglio libro del catalogo", "Mostra i
badge nella scheda libro dell'area amministrazione") and maintains consistent UI
copy.
In `@locale/it_IT.json`:
- Line 95: Replace the misspelled Italian word in the JSON value for the string
"Sono accettati anche domini personalizzati; se incolli un URL completo verra
salvato solo l'host." by changing "verra" to "verrà" so the value reads "Sono
accettati anche domini personalizzati; se incolli un URL completo verrà salvato
solo l'host."; update the same key/value entry in locale/it_IT.json ensuring
proper UTF-8 encoding and escaping if needed.
In `@tests/branch-fix-consistency.spec.js`:
- Around line 6-8: The test uses a fallback default for BASE_URL which can mask
misconfiguration; update the BASE_URL initialization in
tests/branch-fix-consistency.spec.js (referencing the existing HARNESS constant
and BASE_URL variable) to require an explicit env var: if neither
process.env.E2E_BASE_URL nor process.env.APP_URL is set, throw an informative
error and exit the test run instead of assigning 'http://localhost:8081'; this
ensures the harness that mutates the real DB fails fast when the E2E base URL is
missing.
In `@tests/helpers/branch-fix-harness.php`:
- Around line 292-300: The LibraryThing scenarios call reflected private methods
(from getLibraryThingMethods()) that rely on
LibraryThingInstaller::isInstalled($db); before invoking
insertBook()/updateBook() via the reflected methods, ensure the plugin schema is
installed/asserted by running the installer check/install path (call the
installer install/assert method for LibraryThingInstaller against $db) so the
tests exercise the LT schema branch; add this install/assert step immediately
after creating the test DB and before calling $parse->invoke(...) and before the
other reflected calls (also add the same install/assert at the other referenced
spot around the code near the 438-439 mention).
---
Outside diff comments:
In `@app/Controllers/LibraryThingImportController.php`:
- Around line 1368-1369: After calling $stmt->execute() in the guarded libri
update (the block reached from upsertBook()/updateBook() after
findExistingBook()), check the statement’s affected rows (e.g.
$stmt->affected_rows or $mysqli->affected_rows) immediately before
$stmt->close(); if it is 0, throw an exception (RuntimeException or similar) so
the outer transaction aborts instead of continuing; this ensures processChunk()
will not rebuild author links for a row that was soft-deleted between
findExistingBook() and updateBook().
---
Duplicate comments:
In `@app/Views/admin/plugins.php`:
- Around line 220-231: The data-settings-url attribute is built from a hardcoded
path string; replace the url('/admin/plugins/.../settings') usage with a
named-route resolver (route_path with the admin plugins settings route key or
RouteTranslator::route) and pass the plugin id as a parameter so the settings
endpoint is emitted from the router, e.g. use
route_path('admin.plugins.settings', ['id' => (int)$plugin['id']]) (or
RouteTranslator::route equivalent) for the data-settings-url used by the
openPluginSettingsModal button, and apply the same change to the other
occurrences noted (around the other data-settings-url attributes at the
referenced blocks).
---
Nitpick comments:
In `@app/Controllers/LibriApiController.php`:
- Around line 663-701: Extract the column-detection logic used in
LibriApiController::hasTableColumn and FrontendController::descriptionExpr into
a single reusable utility (e.g., SchemaUtils::getTableColumns or
DBSchema::columnExists) that accepts a mysqli, table name and returns cached
column names or a boolean, centralizes the whitelist check, and uses a single
safe query strategy (INFORMATION_SCHEMA or SHOW COLUMNS) with prepared
statements; update LibriApiController::hasTableColumn and
FrontendController::descriptionExpr to call the new utility and remove
duplicated code while preserving existing caching and logging behavior (use the
existing tableColCache semantics moved into the utility).
In `@app/Controllers/PluginController.php`:
- Around line 392-421: normalizeGoodLibDomain currently rejects 'localhost' and
IP addresses because the hostname regex enforces a dotted TLD domain; update
normalizeGoodLibDomain to allow localhost and numeric IPs by adding an
early-accept branch: after parsing $parts and obtaining $host, if $host ===
'localhost' or it matches an IPv4/IPv6 pattern accept and return $host (with
port if present via $port), otherwise fall back to the existing domain regex;
optionally gate the localhost/IP acceptance behind a dev check (e.g., APP_ENV
=== 'local') if you want to restrict it to development.
In `@tests/helpers/e2e-fixtures.js`:
- Around line 87-92: getPluginIdByName currently converts dbQuery result to
Number which yields 0 for empty string and masks "not found"; update
getPluginIdByName to explicitly handle empty/falsey dbQuery responses from
dbQuery( ... escapeSql(name) ... ) by checking the raw result first and then
either returning null (or throwing an explicit Error) when no row is returned,
otherwise parse and return Number(id); keep the function name getPluginIdByName
and the dbQuery/escapeSql usage unchanged while making the not-found branch
explicit.
- Around line 70-74: The escapeSql function only escapes backslashes and single
quotes; update escapeSql to also handle null bytes and other MySQL special
characters (e.g., \0, \n, \r, \x1a) by replacing them with their escaped
equivalents, or switch to using a proven library escape function (e.g.,
mysql.escape) or parameterized queries; locate the function escapeSql in
tests/helpers/e2e-fixtures.js and either extend its replace chain to include
replacements for \0, \n, \r, and \x1a or call the library escape helper so
arbitrary input is safely escaped for MySQL in tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 44961bb8-686f-43ff-80f9-c6910d56803e
📒 Files selected for processing (29)
.gitignoreapp/Controllers/AuthController.phpapp/Controllers/CollaneController.phpapp/Controllers/FrontendController.phpapp/Controllers/LibraryThingImportController.phpapp/Controllers/LibriApiController.phpapp/Controllers/LibriController.phpapp/Controllers/PluginController.phpapp/Controllers/ProfileController.phpapp/Controllers/PublicApiController.phpapp/Middleware/RememberMeMiddleware.phpapp/Support/PluginManager.phpapp/Views/admin/plugins.phpapp/Views/frontend/book-detail.phpapp/Views/libri/scheda_libro.phpapp/Views/profile/index.phplocale/de_DE.jsonlocale/en_US.jsonlocale/it_IT.jsonstorage/plugins/goodlib/GoodLibPlugin.phpstorage/plugins/goodlib/plugin.jsonstorage/plugins/goodlib/views/badges.phpstorage/plugins/goodlib/views/settings.phpstorage/plugins/goodlib/wrapper.phptests/branch-fix-consistency.spec.jstests/full-test.spec.jstests/goodlib-custom-domains.spec.jstests/helpers/branch-fix-harness.phptests/helpers/e2e-fixtures.js
- Add missing i18n keys: "Cerca su:" and 'Cerca "%s" su %s' (it/en/de) - badges.php: replace inline style with Tailwind text-[0.6rem] - settings.php: inline bg colors → Tailwind bg-red-500/bg-blue-500/bg-green-600 - settings.php: escape checkbox name attr, add maxlength/pattern on domain inputs - TLD regex: disallow leading/trailing hyphens (RFC compliance) - RememberMeMiddleware: match AuthController order (setLocale before session) - Test: replace hardcoded /accedi and /libro/ routes with locale-independent URLs
- AuthController: persist session locale only when setLocale() succeeds - LT normalizeContributorField: return first name only (single-value contract) - LT updateBook: throw on affected_rows=0 (race with soft-delete) - i18n: fix "verra" → "verrà" typo in it/en/de (key + value + view) - i18n: "badge" → "badges" in en_US for consistency - branch-fix-consistency: fail-fast when E2E_BASE_URL missing - branch-fix-harness: install LT schema before LT scenarios
When a book has ISBN13 or ISBN10, Anna's Archive and Z-Library badges link to the ISBN search (exact edition match). Gutenberg always uses title+author (ISBN search not supported). Falls back to title+author when no ISBN is available.
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
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 (1)
app/Support/PluginManager.php (1)
45-52:⚠️ Potential issue | 🟠 MajorAdding
goodlibhere auto-enables it on upgrade.
autoRegisterBundledPlugins()doesn't just make bundled plugins discoverable: it inserts missing records withis_active = 1and immediately runsonInstall()/onActivate(). Because this PR also shipsstorage/plugins/goodlib/, upgrading will turn GoodLib on everywhere instead of merely making it available in the admin UI. Keep optional plugins out of the auto-activated bundled list, or split bundled registration from default activation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Support/PluginManager.php` around lines 45 - 52, The BUNDLED_PLUGINS constant currently includes 'goodlib' which causes autoRegisterBundledPlugins() to insert and immediately activate it (calling onInstall()/onActivate()); remove 'goodlib' from the BUNDLED_PLUGINS array or modify autoRegisterBundledPlugins() to distinguish "bundled but not auto-activate" plugins so that bundled discovery and DB insertion does not set is_active = 1 or call onActivate() for optional plugins (refer to BUNDLED_PLUGINS constant and the autoRegisterBundledPlugins(), onInstall(), onActivate() code paths).
♻️ Duplicate comments (3)
locale/en_US.json (1)
975-975:⚠️ Potential issue | 🟡 MinorUse plural wording for badge visibility label.
This string is inconsistent with neighboring badge labels that use plural phrasing.
✏️ Proposed text fix
- "Mostra badge nell'area amministrazione": "Show badge in the admin area", + "Mostra badge nell'area amministrazione": "Show badges in the admin area",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@locale/en_US.json` at line 975, Update the English translation value for the JSON entry keyed by "Mostra badge nell'area amministrazione" to use plural phrasing; replace "Show badge in the admin area" with "Show badges in the admin area" so it matches neighboring plural badge labels.app/Middleware/RememberMeMiddleware.php (1)
94-99:⚠️ Potential issue | 🟡 MinorOnly persist the locale when
setLocale()succeeds.Regular login already guards
$_SESSION['locale']behindI18n::setLocale(), but remember-me still writes the raw DB value unconditionally. If the stored locale is invalid or has been removed, the translator keeps the old locale while the session claims a different one.💡 Suggested fix
if (!empty($row['locale'])) { $locale = (string) $row['locale']; - \App\Support\I18n::setLocale($locale); - $_SESSION['locale'] = $locale; + if (\App\Support\I18n::setLocale($locale)) { + $_SESSION['locale'] = $locale; + } else { + unset($_SESSION['locale']); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Middleware/RememberMeMiddleware.php` around lines 94 - 99, RememberMeMiddleware currently writes $_SESSION['locale'] directly from $row['locale'] even if \App\Support\I18n::setLocale($locale) fails; change the code to call I18n::setLocale((string)$row['locale']) and only assign $_SESSION['locale'] when that call returns truthy (or succeeds), and optionally catch any exception from I18n::setLocale to avoid persisting an invalid locale; this ensures $_SESSION['locale'] is updated only when setLocale actually applied the locale.app/Views/admin/plugins.php (1)
218-221:⚠️ Potential issue | 🟠 MajorReplace the raw
/admin/plugins/.../settingspaths with route helpers.These
data-settings-urlattributes hardcode the admin path again, so any route translation/rename breaks modal saves. Build them fromroute_path(...)/RouteTranslator::route(...)instead of embedding the path literal. If there isn't a route key yet, add one rather than keeping the literal in the view.As per coding guidelines,
Never use hardcoded routes. Use route_path('key') or RouteTranslator::route('key') instead of hardcoded paths like /accedi or /login.Also applies to: 229-232, 241-245, 260-262, 282-286
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/admin/plugins.php` around lines 218 - 221, Replace hardcoded data-settings-url values that build "/admin/plugins/{id}/settings" with a route helper call: construct the URL via route_path('admin.plugins.settings', ['id' => (int) $plugin['id']]) or RouteTranslator::route('admin.plugins.settings', ['id' => (int) $plugin['id']]) and use htmlspecialchars on that output; update each occurrence that sets data-settings-url (the attributes in the same block that call onclick="openPluginSettingsModal(this)") and if the named route key 'admin.plugins.settings' doesn't exist, add that route key in your routes config so the view can call the helper instead of hardcoding the path.
🧹 Nitpick comments (5)
app/Controllers/LibraryThingImportController.php (1)
885-888: Extractdescrizione_plainnormalization into one helper.The same HTML→plain-text pipeline now exists in both import parsing and scrape enrichment. One private method will keep those paths aligned when the normalization rules change.
Also applies to: 1646-1652
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibraryThingImportController.php` around lines 885 - 888, Extract the HTML→plain-text normalization into a single private helper on the LibraryThingImportController (e.g. private function normalizeDescrizione(?string $html): ?string) that: returns null for empty input, strips tags (preg_replace '/<[^>]+>/' → ' '), runs html_entity_decode(..., ENT_QUOTES|ENT_HTML5, 'UTF-8'), collapses whitespace with preg_replace('/\s+/u', ' ', ...) and returns trim(...) or null; then replace the inline logic that sets $result['descrizione_plain'] (the spot using $result['descrizione'] and the duplicate occurrence in the scrape enrichment code) to call $this->normalizeDescrizione($result['descrizione']) and assign the result..gitignore (1)
279-285: Prefer a pattern-based allowlist for top-level specs.These rules already miss
tests/full-test.spec.js; tracked files keep working, but every new top-level Playwright spec now needs another.gitignoreedit and is easy to forget.♻️ Suggested cleanup
tests/* -!tests/branch-fix-consistency.spec.js -!tests/goodlib-custom-domains.spec.js +!tests/*.spec.js !tests/helpers/ tests/helpers/* !tests/helpers/branch-fix-harness.php !tests/helpers/e2e-fixtures.js🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore around lines 279 - 285, The .gitignore currently ignores tests/* and then individually negates specific top-level spec files (e.g., tests/branch-fix-consistency.spec.js, tests/goodlib-custom-domains.spec.js), which forces manual updates for each new top-level Playwright spec; update the ignore rules to use a pattern-based allowlist instead (replace the repeated single-file negations with a single negation like allowing all top-level spec patterns such as !tests/*.spec.js or a more specific glob you want), keeping the existing helpers handling (tests/helpers/ and tests/helpers/* and the explicit helper exceptions) intact so helper files remain ignored/allowed as before.app/Controllers/LibriApiController.php (1)
45-48: Please de-duplicate the description fallback SQL.
FrontendControlleralready has adescriptionExpr()helper for this exactdescrizione_plain/descrizionefallback. Keeping a second copy here makes catalog and admin search easy to drift apart again the next time this logic changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibriApiController.php` around lines 45 - 48, Replace the duplicated descrizione_plain/descrizione fallback in LibriApiController by reusing the existing helper instead of re-implementing it: remove the local $descExpr assignment that calls hasTableColumn and instead obtain the expression via FrontendController::descriptionExpr() (or the instance helper method descriptionExpr()) and use that result in the $where clause where {$descExpr} is referenced; ensure you import or reference FrontendController correctly so the single canonical descriptionExpr() is used rather than duplicating the fallback logic.tests/goodlib-custom-domains.spec.js (1)
54-55: Don't couple this spec to the exact mirror inventory.These
toHaveCount()checks will fail every time a preset domain is rotated or added, even if the custom-domain flow still works. Assert the presence of__custom__and the specific preset(s) this scenario depends on instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/goodlib-custom-domains.spec.js` around lines 54 - 55, Replace the brittle toHaveCount assertions on '#goodlib_anna_domain_select option' and '#goodlib_zlib_domain_select option' with presence checks for the custom-domain sentinel and the specific preset(s) this scenario requires: assert that an option with value "__custom__" exists for each select (e.g., '#goodlib_anna_domain_select option[value="__custom__"]' and '#goodlib_zlib_domain_select option[value="__custom__"]') and additionally assert presence of the particular preset option(s) you depend on by value or visible text (use the option[value="presetName"] or option:has-text("Preset Label") selectors). This keeps the test resilient to inventory rotations while ensuring the custom-domain flow and required presets are present.storage/plugins/goodlib/GoodLibPlugin.php (1)
304-340: Minor inconsistency:gutenberg_domainin getSettings but not in view docblock.
getSettings()returnsgutenberg_domain(line 317), butstorage/plugins/goodlib/views/settings.phpdocblock only declares 7 keys withoutgutenberg_domain. This is fine since Gutenberg only has one domain (no mirror selection), but consider adding it to the docblock for completeness or adding a comment explaining why it's excluded.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/goodlib/GoodLibPlugin.php` around lines 304 - 340, The docblock for the settings passed to the view is inconsistent with GoodLibPlugin::getSettings(): getSettings() returns a 'gutenberg_domain' key but the view's docblock in storage/plugins/goodlib/views/settings.php omits it; update the view docblock to include 'gutenberg_domain' (or add a short comment explaining its intentional omission) so the declared shape matches the returned array from getSettings(), referencing the gutenberg_domain key and the getSettings() method.
🤖 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/LibraryThingImportController.php`:
- Around line 1013-1020: The current LibraryThingImportController code leaves
malformed ISSNs in $result['issn'] (after attempting to normalize via
$issnCompact), which then gets emitted to JSON-LD/public API; change the logic
so that after normalization you only assign to $result['issn'] when the pattern
matches, otherwise set $result['issn'] = null (or unset it) and, if you still
need the original input, store it under a separate field such as
$result['source_issn'] or $result['raw_issn']; update the block that computes
$issnCompact and the assignment to $result['issn'] to implement this
safe-normalize-or-null behavior.
- Around line 1370-1376: The current check uses $stmt->affected_rows to decide
if the row was soft-deleted, but UPDATE returns 0 both for no-matching-row and
for unchanged-row; after the existing UPDATE that yields $stmt->affected_rows
=== 0, run an explicit existence query (e.g., SELECT id FROM books WHERE id = ?)
using the same DB connection to determine whether the book still exists: if the
SELECT finds the row treat the situation as "no-op/unchanged" and continue,
otherwise treat it as soft-deleted and throw the RuntimeException; ensure you
properly close/cleanup the original $stmt before or after the existence check
and reference the same $bookId variable in the SELECT.
In `@app/Controllers/PluginController.php`:
- Around line 354-370: The code in PluginController currently writes boolean
settings via $this->pluginManager->setSetting() before validating GoodLib
domains, causing partial persistence on invalid domains; change the flow in the
method handling $settings/$pluginId to first validate all GoodLib domains using
self::normalizeGoodLibDomain against self::GOODLIB_DEFAULT_DOMAINS and collect
their normalized values (return 400 immediately if any domain is invalid) and
only after all validations pass call $this->pluginManager->setSetting() for the
boolean keys and then for each domain key using the validated normalized domain
values; reference normalizeGoodLibDomain, GOODLIB_DEFAULT_DOMAINS, setSetting,
$settings and $pluginId to locate and reorder logic accordingly.
In `@app/Views/admin/plugins.php`:
- Around line 1822-1832: The success handler currently writes raw form values
into the config button datasets (configBtn.dataset.goodlibAnnaDomain,
goodlibZlibDomain, etc.) which can differ from the normalized domain the backend
saved; instead, after data.success use the canonical domain fields returned in
the response (e.g. data.goodlib_anna_domain, data.goodlib_zlib_domain,
data.goodlib_gutenberg_domain, data.goodlib_frontend_domain,
data.goodlib_admin_domain or whatever keys the API returns) to populate
configBtn.dataset.goodlibAnnaDomain, configBtn.dataset.goodlibZlibDomain,
configBtn.dataset.goodlibGutenbergDomain,
configBtn.dataset.goodlibFrontendDomain and configBtn.dataset.goodlibAdminDomain
so the cached values reflect the normalized persisted values rather than the raw
form inputs.
In `@app/Views/profile/index.php`:
- Around line 434-452: Add an explicit "use site default" option to the locale
<select> so an empty or unknown $userLocale doesn't implicitly select the first
available locale; insert an <option value=""><?= __("Use site default")
?></option> (or similar label) as the first option and set it selected when
$userLocale is empty or when $userLocale is not present in the keys of
$availableLocales (use array_key_exists or isset on $availableLocales to detect
this). Update the selection logic around $userLocale and the foreach so the
empty value is selected when appropriate and keep htmlspecialchars for the
option rendering.
In `@tests/helpers/e2e-fixtures.js`:
- Around line 87-92: The getPluginIdByName function currently converts
dbQuery(...) directly to Number(result), which yields 0 when no row is returned;
change it to inspect the dbQuery return (the row object from dbQuery), extract
the id field (from the result of dbQuery(`SELECT id FROM plugins WHERE name =
'${escapeSql(name)}' LIMIT 1`)), and if the id is missing/undefined throw an
explicit Error (e.g., `throw new Error('Plugin not found: ' + name)`), otherwise
return Number(id) so callers never receive 0 as a missing-plugin sentinel.
- Around line 116-129: The restorePluginSettings function currently performs
DELETE then multiple INSERTs via separate dbExec calls, which is non-atomic and
can leave partially restored state on failure; change it to perform the whole
operation inside a single transaction: begin a transaction, run the DELETE for
pluginId and then a single batched INSERT (or multiple INSERT statements
concatenated) for all rows using escapeSql for values, then COMMIT; if any error
occurs, ROLLBACK. Use the existing restorePluginSettings and dbExec identifiers
(and escapeSql for escaping) so the code runs the DELETE/INSERTs within
BEGIN/COMMIT/ROLLBACK in one dbExec call (or use dbExec per step but ensure
rollback on catch) to make the restoration atomic.
---
Outside diff comments:
In `@app/Support/PluginManager.php`:
- Around line 45-52: The BUNDLED_PLUGINS constant currently includes 'goodlib'
which causes autoRegisterBundledPlugins() to insert and immediately activate it
(calling onInstall()/onActivate()); remove 'goodlib' from the BUNDLED_PLUGINS
array or modify autoRegisterBundledPlugins() to distinguish "bundled but not
auto-activate" plugins so that bundled discovery and DB insertion does not set
is_active = 1 or call onActivate() for optional plugins (refer to
BUNDLED_PLUGINS constant and the autoRegisterBundledPlugins(), onInstall(),
onActivate() code paths).
---
Duplicate comments:
In `@app/Middleware/RememberMeMiddleware.php`:
- Around line 94-99: RememberMeMiddleware currently writes $_SESSION['locale']
directly from $row['locale'] even if \App\Support\I18n::setLocale($locale)
fails; change the code to call I18n::setLocale((string)$row['locale']) and only
assign $_SESSION['locale'] when that call returns truthy (or succeeds), and
optionally catch any exception from I18n::setLocale to avoid persisting an
invalid locale; this ensures $_SESSION['locale'] is updated only when setLocale
actually applied the locale.
In `@app/Views/admin/plugins.php`:
- Around line 218-221: Replace hardcoded data-settings-url values that build
"/admin/plugins/{id}/settings" with a route helper call: construct the URL via
route_path('admin.plugins.settings', ['id' => (int) $plugin['id']]) or
RouteTranslator::route('admin.plugins.settings', ['id' => (int) $plugin['id']])
and use htmlspecialchars on that output; update each occurrence that sets
data-settings-url (the attributes in the same block that call
onclick="openPluginSettingsModal(this)") and if the named route key
'admin.plugins.settings' doesn't exist, add that route key in your routes config
so the view can call the helper instead of hardcoding the path.
In `@locale/en_US.json`:
- Line 975: Update the English translation value for the JSON entry keyed by
"Mostra badge nell'area amministrazione" to use plural phrasing; replace "Show
badge in the admin area" with "Show badges in the admin area" so it matches
neighboring plural badge labels.
---
Nitpick comments:
In @.gitignore:
- Around line 279-285: The .gitignore currently ignores tests/* and then
individually negates specific top-level spec files (e.g.,
tests/branch-fix-consistency.spec.js, tests/goodlib-custom-domains.spec.js),
which forces manual updates for each new top-level Playwright spec; update the
ignore rules to use a pattern-based allowlist instead (replace the repeated
single-file negations with a single negation like allowing all top-level spec
patterns such as !tests/*.spec.js or a more specific glob you want), keeping the
existing helpers handling (tests/helpers/ and tests/helpers/* and the explicit
helper exceptions) intact so helper files remain ignored/allowed as before.
In `@app/Controllers/LibraryThingImportController.php`:
- Around line 885-888: Extract the HTML→plain-text normalization into a single
private helper on the LibraryThingImportController (e.g. private function
normalizeDescrizione(?string $html): ?string) that: returns null for empty
input, strips tags (preg_replace '/<[^>]+>/' → ' '), runs
html_entity_decode(..., ENT_QUOTES|ENT_HTML5, 'UTF-8'), collapses whitespace
with preg_replace('/\s+/u', ' ', ...) and returns trim(...) or null; then
replace the inline logic that sets $result['descrizione_plain'] (the spot using
$result['descrizione'] and the duplicate occurrence in the scrape enrichment
code) to call $this->normalizeDescrizione($result['descrizione']) and assign the
result.
In `@app/Controllers/LibriApiController.php`:
- Around line 45-48: Replace the duplicated descrizione_plain/descrizione
fallback in LibriApiController by reusing the existing helper instead of
re-implementing it: remove the local $descExpr assignment that calls
hasTableColumn and instead obtain the expression via
FrontendController::descriptionExpr() (or the instance helper method
descriptionExpr()) and use that result in the $where clause where {$descExpr} is
referenced; ensure you import or reference FrontendController correctly so the
single canonical descriptionExpr() is used rather than duplicating the fallback
logic.
In `@storage/plugins/goodlib/GoodLibPlugin.php`:
- Around line 304-340: The docblock for the settings passed to the view is
inconsistent with GoodLibPlugin::getSettings(): getSettings() returns a
'gutenberg_domain' key but the view's docblock in
storage/plugins/goodlib/views/settings.php omits it; update the view docblock to
include 'gutenberg_domain' (or add a short comment explaining its intentional
omission) so the declared shape matches the returned array from getSettings(),
referencing the gutenberg_domain key and the getSettings() method.
In `@tests/goodlib-custom-domains.spec.js`:
- Around line 54-55: Replace the brittle toHaveCount assertions on
'#goodlib_anna_domain_select option' and '#goodlib_zlib_domain_select option'
with presence checks for the custom-domain sentinel and the specific preset(s)
this scenario requires: assert that an option with value "__custom__" exists for
each select (e.g., '#goodlib_anna_domain_select option[value="__custom__"]' and
'#goodlib_zlib_domain_select option[value="__custom__"]') and additionally
assert presence of the particular preset option(s) you depend on by value or
visible text (use the option[value="presetName"] or option:has-text("Preset
Label") selectors). This keeps the test resilient to inventory rotations while
ensuring the custom-domain flow and required presets are present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 728d79e2-aa1d-43a5-a198-45b8f811cda1
📒 Files selected for processing (29)
.gitignoreapp/Controllers/AuthController.phpapp/Controllers/CollaneController.phpapp/Controllers/FrontendController.phpapp/Controllers/LibraryThingImportController.phpapp/Controllers/LibriApiController.phpapp/Controllers/LibriController.phpapp/Controllers/PluginController.phpapp/Controllers/ProfileController.phpapp/Controllers/PublicApiController.phpapp/Middleware/RememberMeMiddleware.phpapp/Support/PluginManager.phpapp/Views/admin/plugins.phpapp/Views/frontend/book-detail.phpapp/Views/libri/scheda_libro.phpapp/Views/profile/index.phplocale/de_DE.jsonlocale/en_US.jsonlocale/it_IT.jsonstorage/plugins/goodlib/GoodLibPlugin.phpstorage/plugins/goodlib/plugin.jsonstorage/plugins/goodlib/views/badges.phpstorage/plugins/goodlib/views/settings.phpstorage/plugins/goodlib/wrapper.phptests/branch-fix-consistency.spec.jstests/full-test.spec.jstests/goodlib-custom-domains.spec.jstests/helpers/branch-fix-harness.phptests/helpers/e2e-fixtures.js
- RememberMeMiddleware: guard session locale on setLocale() success - PluginController: validate domains before persisting any settings - PluginController: return normalized domains in response - Admin JS: use server-normalized domains for cached data attributes - LT import: affected_rows=0 → explicit existence check (not just throw) - LT import: null malformed ISSN instead of keeping raw value - Profile locale: add "Predefinita del sito" default option - en_US: "badge" → "badges" for admin area label - e2e-fixtures: getPluginIdByName throws on not-found - e2e-fixtures: restorePluginSettings atomic (single transaction) - .gitignore: pattern-based test file allowlist - i18n: add "Predefinita del sito" in it/en/de
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/helpers/branch-fix-harness.php (1)
433-478:⚠️ Potential issue | 🟡 MinorISSN scenario should ensure LibraryThing schema is installed.
This scenario uses
$insert->invoke()fromgetLibraryThingMethods()which branches onLibraryThingInstaller::isInstalled($db). UnlikescenarioLibraryThingParseAndPersist(line 293), this function doesn't explicitly install the schema first. If the database lacks LT columns, the INSERT may silently skipissn.💡 Suggested fix
function scenarioPublicApiAndFrontendIssn(mysqli $db): array { $_SESSION = []; $factory = new ServerRequestFactory(); $publicApi = new PublicApiController(); + // Ensure LibraryThing plugin schema is installed so issn column exists + if (!\App\Support\LibraryThingInstaller::isInstalled($db)) { + $installer = new \App\Support\LibraryThingInstaller($db); + $installer->install(); + } [$controller, $parse, $insert] = getLibraryThingMethods();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/helpers/branch-fix-harness.php` around lines 433 - 478, The scenarioPublicApiAndFrontendIssn test calls $insert->invoke(...) which assumes LibraryThing schema is present but does not ensure it; call the same installation step used in scenarioLibraryThingParseAndPersist before invoking $insert (e.g., check LibraryThingInstaller::isInstalled($db) and if false run the installer or call the helper that installs the LibraryThing schema) so the insert will include issn; locate getLibraryThingMethods()/LibraryThingInstaller usage in this file to mirror the install/check logic already used in scenarioLibraryThingParseAndPersist.
🧹 Nitpick comments (3)
app/Controllers/FrontendController.php (1)
22-37: Consider sharing the column-introspection helper.
FrontendController::descriptionExpr()now carries its own cacheddescrizione_plainprobe, whileapp/Controllers/LibriApiController.phpalready has a separate cachedhasTableColumn()path. Keeping both detection strategies in sync makes catalog and API search behavior easier to drift again. Extracting one shared helper would reduce that risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/FrontendController.php` around lines 22 - 37, FrontendController::descriptionExpr duplicates column-introspection logic already present in LibriApiController (cached probe for descrizione_plain); extract a single shared helper (e.g. a static method like SchemaInspector::hasTableColumn(mysqli $db, string $table, string $column)) that implements the cached SHOW COLUMNS probe and freeing the result, then have FrontendController::descriptionExpr call that helper instead of using its own self::$hasDescrizionePlain cache and detection logic; update LibriApiController to use the same helper so both callers share one cache/implementation and avoid drift.storage/plugins/goodlib/GoodLibPlugin.php (1)
210-239: Domain normalization duplicates PluginController logic.
GoodLibPlugin::normalizeDomain()mirrorsPluginController::normalizeGoodLibDomain()(see context snippet 1 fromapp/Controllers/PluginController.php:399-428). Consider extracting to a shared utility to avoid divergence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/goodlib/GoodLibPlugin.php` around lines 210 - 239, GoodLibPlugin::normalizeDomain duplicates logic in PluginController::normalizeGoodLibDomain; extract the validation/normalization into a single shared utility (e.g., DomainNormalizer::normalize or Utils::normalizeDomain) and have both GoodLibPlugin::normalizeDomain and PluginController::normalizeGoodLibDomain call that single implementation; update references in both classes to use the shared function, remove the duplicated code body from GoodLibPlugin::normalizeDomain (or make it a thin wrapper) and add unit tests if present to ensure behavior remains identical.storage/plugins/goodlib/plugin.json (1)
10-12:max_app_versionis declared in plugin.json but not enforced by PluginManager.The PluginManager only validates
requires_phpduring plugin installation; neitherrequires_appnormax_app_versionare checked. This means the plugin will activate regardless of app version, making the"max_app_version": "1.0.0"declaration effectively advisory-only.Low risk for a bundled plugin, but worth implementing max_app_version validation in a future refactor to prevent compatibility issues as the application evolves.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/goodlib/plugin.json` around lines 10 - 12, PluginManager currently only checks requires_php during plugin install; add validation to also enforce the plugin.json fields requires_app and max_app_version (e.g., in PluginManager.installPlugin and PluginManager.activatePlugin or a new helper validatePluginRequirements) by parsing the version strings (use existing semver utility or add a semver compare) and rejecting installation/activation when currentAppVersion < requires_app or currentAppVersion > max_app_version, returning/logging a clear error that includes the offending plugin id and the expected vs actual app version.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/helpers/e2e-fixtures.js`:
- Around line 7-14: The helper currently falls back to non-E2E sources
(envFromFile, APP_URL, .env) which can run destructive fixtures against the real
app; change the logic in loadDotEnv/this file so BASE_URL, DB_HOST, DB_USER,
DB_PASS, DB_NAME and DB_SOCKET are read only from their E2E_* environment
variables (process.env.E2E_BASE_URL, process.env.E2E_DB_HOST, etc.), and if any
required E2E_* var is missing throw/log a clear fatal error and exit (fail fast)
instead of using APP_URL or values from envFromFile; keep a clear error message
naming the missing E2E_* keys so CI/local run scripts (e.g., /tmp/run-e2e.sh)
must set them.
- Around line 120-137: The restorePluginSettings function currently quotes all
setting_value fields, turning JS null into the string 'null'; change the VALUES
construction so when row.setting_value === null you insert the SQL token NULL
(no quotes) instead of quoting escapeSql(row.setting_value), e.g., produce
${row.setting_value === null ? 'NULL' : `'${escapeSql(row.setting_value)}'`};
keep using escapeSql for non-null values, and leave autoload and other fields
unchanged; update the INSERT generation in restorePluginSettings (refer to
restorePluginSettings, escapeSql, dbExec) so snapshotPluginSettings-null values
roundtrip correctly.
---
Duplicate comments:
In `@tests/helpers/branch-fix-harness.php`:
- Around line 433-478: The scenarioPublicApiAndFrontendIssn test calls
$insert->invoke(...) which assumes LibraryThing schema is present but does not
ensure it; call the same installation step used in
scenarioLibraryThingParseAndPersist before invoking $insert (e.g., check
LibraryThingInstaller::isInstalled($db) and if false run the installer or call
the helper that installs the LibraryThing schema) so the insert will include
issn; locate getLibraryThingMethods()/LibraryThingInstaller usage in this file
to mirror the install/check logic already used in
scenarioLibraryThingParseAndPersist.
---
Nitpick comments:
In `@app/Controllers/FrontendController.php`:
- Around line 22-37: FrontendController::descriptionExpr duplicates
column-introspection logic already present in LibriApiController (cached probe
for descrizione_plain); extract a single shared helper (e.g. a static method
like SchemaInspector::hasTableColumn(mysqli $db, string $table, string $column))
that implements the cached SHOW COLUMNS probe and freeing the result, then have
FrontendController::descriptionExpr call that helper instead of using its own
self::$hasDescrizionePlain cache and detection logic; update LibriApiController
to use the same helper so both callers share one cache/implementation and avoid
drift.
In `@storage/plugins/goodlib/GoodLibPlugin.php`:
- Around line 210-239: GoodLibPlugin::normalizeDomain duplicates logic in
PluginController::normalizeGoodLibDomain; extract the validation/normalization
into a single shared utility (e.g., DomainNormalizer::normalize or
Utils::normalizeDomain) and have both GoodLibPlugin::normalizeDomain and
PluginController::normalizeGoodLibDomain call that single implementation; update
references in both classes to use the shared function, remove the duplicated
code body from GoodLibPlugin::normalizeDomain (or make it a thin wrapper) and
add unit tests if present to ensure behavior remains identical.
In `@storage/plugins/goodlib/plugin.json`:
- Around line 10-12: PluginManager currently only checks requires_php during
plugin install; add validation to also enforce the plugin.json fields
requires_app and max_app_version (e.g., in PluginManager.installPlugin and
PluginManager.activatePlugin or a new helper validatePluginRequirements) by
parsing the version strings (use existing semver utility or add a semver
compare) and rejecting installation/activation when currentAppVersion <
requires_app or currentAppVersion > max_app_version, returning/logging a clear
error that includes the offending plugin id and the expected vs actual app
version.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c1f54edc-34e6-4782-af1b-780c20018ecb
📒 Files selected for processing (29)
.gitignoreapp/Controllers/AuthController.phpapp/Controllers/CollaneController.phpapp/Controllers/FrontendController.phpapp/Controllers/LibraryThingImportController.phpapp/Controllers/LibriApiController.phpapp/Controllers/LibriController.phpapp/Controllers/PluginController.phpapp/Controllers/ProfileController.phpapp/Controllers/PublicApiController.phpapp/Middleware/RememberMeMiddleware.phpapp/Support/PluginManager.phpapp/Views/admin/plugins.phpapp/Views/frontend/book-detail.phpapp/Views/libri/scheda_libro.phpapp/Views/profile/index.phplocale/de_DE.jsonlocale/en_US.jsonlocale/it_IT.jsonstorage/plugins/goodlib/GoodLibPlugin.phpstorage/plugins/goodlib/plugin.jsonstorage/plugins/goodlib/views/badges.phpstorage/plugins/goodlib/views/settings.phpstorage/plugins/goodlib/wrapper.phptests/branch-fix-consistency.spec.jstests/full-test.spec.jstests/goodlib-custom-domains.spec.jstests/helpers/branch-fix-harness.phptests/helpers/e2e-fixtures.js
👮 Files not reviewed due to content moderation or server errors (4)
- locale/en_US.json
- tests/goodlib-custom-domains.spec.js
- storage/plugins/goodlib/views/badges.php
- app/Controllers/PluginController.php
- Fail-fast when E2E_BASE_URL, E2E_DB_USER, E2E_DB_NAME are missing - restorePluginSettings preserves SQL NULL (not string 'null')
Without this, the updater's updateBundledPlugins() would not copy the goodlib plugin directory during upgrades from pre-goodlib versions.
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/helpers/e2e-fixtures.js (1)
7-20:⚠️ Potential issue | 🟠 MajorRequire dedicated
E2E_*config in this helper.This still falls back to
APP_URLand repo.envvalues, so a missing test variable can make these destructive fixtures hit the regular app environment instead of failing fast.Based on learnings, E2E tests with Playwright require `/tmp/run-e2e.sh` to set DB/admin credentials as env vars.🔒 Suggested guard
-const envFromFile = loadDotEnv(path.join(ROOT, '.env')); - // Require explicit E2E env vars — these fixtures create/delete data -const BASE_URL = process.env.E2E_BASE_URL || process.env.APP_URL; +const BASE_URL = process.env.E2E_BASE_URL; if (!BASE_URL) { - throw new Error('E2E_BASE_URL or APP_URL must be set (use /tmp/run-e2e.sh)'); + throw new Error('E2E_BASE_URL must be set (use /tmp/run-e2e.sh)'); } -const DB_HOST = process.env.E2E_DB_HOST || envFromFile.DB_HOST || ''; -const DB_USER = process.env.E2E_DB_USER || envFromFile.DB_USER || ''; -const DB_PASS = process.env.E2E_DB_PASS ?? envFromFile.DB_PASS ?? ''; -const DB_NAME = process.env.E2E_DB_NAME || envFromFile.DB_NAME || ''; -const DB_SOCKET = process.env.E2E_DB_SOCKET || envFromFile.DB_SOCKET || ''; -if (!DB_USER || !DB_NAME) { - throw new Error('E2E_DB_USER and E2E_DB_NAME must be set (use /tmp/run-e2e.sh)'); +const DB_HOST = process.env.E2E_DB_HOST || ''; +const DB_USER = process.env.E2E_DB_USER || ''; +const DB_PASS = process.env.E2E_DB_PASS ?? ''; +const DB_NAME = process.env.E2E_DB_NAME || ''; +const DB_SOCKET = process.env.E2E_DB_SOCKET || ''; +if (!DB_USER || !DB_NAME || (!DB_HOST && !DB_SOCKET)) { + throw new Error('Missing dedicated E2E_DB_* configuration (use /tmp/run-e2e.sh)'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/helpers/e2e-fixtures.js` around lines 7 - 20, The helper currently falls back to non-E2E sources (APP_URL and envFromFile) causing tests to accidentally target real environments; update the logic in the block that sets BASE_URL, DB_HOST, DB_USER, DB_PASS, DB_NAME, and DB_SOCKET (and the loadDotEnv usage) to only read from explicit E2E_* environment variables (no fallback to process.env.APP_URL or envFromFile), and throw clear errors if any required E2E_* var (E2E_BASE_URL, E2E_DB_USER, E2E_DB_NAME, etc.) is missing so fixtures fail fast.tests/helpers/branch-fix-harness.php (1)
433-445:⚠️ Potential issue | 🟡 MinorMissing LibraryThingInstaller check before using LT methods.
scenarioPublicApiAndFrontendIssninvokesinsertBookvia reflection (line 445) but does not verify that the LibraryThing plugin schema is installed. On a clean test DB,LibraryThingInstaller::isInstalled($db)may return false, causinginsertBookto use the basic code path that omits LT-specific columns (likeissn). This would make the test pass/fail based on harness setup rather than product behavior.
scenarioLibraryThingParseAndPersistcorrectly includes this check at lines 293-296.🛠️ Proposed fix
function scenarioPublicApiAndFrontendIssn(mysqli $db): array { + // Ensure LibraryThing plugin schema is installed so LT-specific columns exist + if (!\App\Support\LibraryThingInstaller::isInstalled($db)) { + $installer = new \App\Support\LibraryThingInstaller($db); + $installer->install(); + } + $_SESSION = []; $factory = new ServerRequestFactory(); $publicApi = new PublicApiController();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/helpers/branch-fix-harness.php` around lines 433 - 445, The scenarioPublicApiAndFrontendIssn helper calls LT-specific methods (via getLibraryThingMethods and invoking insertBook) without checking whether the LibraryThing plugin schema is present; add the same guard used in scenarioLibraryThingParseAndPersist by calling LibraryThingInstaller::isInstalled($db) before invoking the parse/insert reflection methods and skip or adjust the LT-specific assertions when not installed. Locate scenarioPublicApiAndFrontendIssn and wrap the code that uses $parse/$insert (from getLibraryThingMethods) in an if (LibraryThingInstaller::isInstalled($db)) { ... } branch (or early return) so insertBook is not invoked against a DB missing LT schema and the test behavior matches the harness setup.
🧹 Nitpick comments (8)
app/Controllers/LibraryThingImportController.php (3)
35-55: Per-instance caching may diverge from FrontendController's static cache.
FrontendController::descriptionExpr()(lines 20-38 in app/Controllers/FrontendController.php) uses a static property$hasDescrizionePlainto cache the column check, while this controller uses a per-instance property$cachedHasDescPlain. If a migration adds the column mid-process, different controller instances or request paths could disagree on whether the column exists.Consider aligning caching strategies across controllers (e.g., using a shared static helper) to ensure consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibraryThingImportController.php` around lines 35 - 55, The per-instance cache $cachedHasDescPlain in hasDescrizionePlainColumn() can diverge from FrontendController::descriptionExpr() which uses static $hasDescrizionePlain; replace the instance-level caching with a shared static/cache approach so all controllers agree (e.g., move the column-existence check into a single static helper or reuse FrontendController::descriptionExpr() logic), update hasDescrizionePlainColumn() to consult and set that shared static cache instead of $cachedHasDescPlain, and preserve the existing query/exception handling and result freeing behavior when computing the shared value.
508-521: PreferSecureLogger::error()overerror_log()for potentially sensitive contexts.Lines 508, 515, 521 use
error_log()directly to log import history persistence failures. As per coding guidelines, sensitive contexts should useSecureLogger::error()instead.♻️ Proposed fix
if (!$persisted) { - error_log("[LibraryThingImportController] Failed to persist import history to database"); + \App\Support\SecureLogger::error("[LibraryThingImportController] Failed to persist import history to database"); // Mark as failed so the record doesn't stay stuck in 'processing' $importLogger->fail('Failed to persist import history', $importData['total_rows']); } } catch (\Throwable $e) { // Log error but don't fail the import (already completed) // Catches \Error/TypeError too (strict_types=1 can throw TypeError) - error_log("[LibraryThingImportController] Failed to persist import history (" . get_class($e) . "): " . $e->getMessage()); + \App\Support\SecureLogger::error("[LibraryThingImportController] Failed to persist import history (" . get_class($e) . "): " . $e->getMessage()); // Mark as failed so the record doesn't stay stuck in 'processing' if (isset($importLogger)) { try { $importLogger->fail($e->getMessage(), $importData['total_rows']); } catch (\Throwable $inner) { - error_log("[LibraryThingImportController] Also failed to mark import as failed: " . $inner->getMessage()); + \App\Support\SecureLogger::error("[LibraryThingImportController] Also failed to mark import as failed: " . $inner->getMessage()); } } }As per coding guidelines,
Use SecureLogger::error() not error_log() for sensitive contexts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibraryThingImportController.php` around lines 508 - 521, Replace direct error_log() calls in LibraryThingImportController's import history persistence error paths with SecureLogger::error() so sensitive data is logged via the secure logger; specifically update the three occurrences inside the persistence failure branch and the outer and inner catch blocks (the messages that include get_class($e) and $e->getMessage(), and the inner->getMessage()) to call SecureLogger::error() with the same contextual strings, ensure SecureLogger is imported/available in the class scope, and keep the existing calls to $importLogger->fail(...) unchanged.
1650-1650: Minor:error_logusage in cover download error handling.Line 1650 uses
error_log()for cover download failures. While less sensitive than import history errors, consider usingSecureLogger::error()for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibraryThingImportController.php` at line 1650, Replace the direct PHP error_log call used for cover download failures in LibraryThingImportController with the application's SecureLogger error method to maintain consistent logging; locate the cover download error handling where error_log("[LT Import] Cover download failed for book $bookId: " . $e->getMessage()) is called and change it to call SecureLogger::error (or the controller's injected secure logger) with the same message/context and include the exception details so the bookId and $e->getMessage() are preserved.app/Controllers/LibriApiController.php (1)
45-48: Extract the description expression into shared search code.
app/Controllers/FrontendController.php:22-38already owns the sameCOALESCE(NULLIF(...), ...)+ schema-probe logic. Keeping a second copy here makes the frontend catalog and admin grid easy to drift apart again on the next search tweak.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibriApiController.php` around lines 45 - 48, Extract the duplicated description-expression logic into a shared helper used by both controllers: move the COALESCE(NULLIF(...), ...) + schema-probe logic currently duplicated in FrontendController and the $descExpr calculation in LibriApiController into a single function (e.g., a static helper like getDescriptionExpression or a method on a shared SearchHelper/Libri model). Replace the inline $descExpr construction in LibriApiController (which currently uses hasTableColumn($db, 'libri', 'descrizione_plain')) with a call to that shared function and update FrontendController to call the same function so both controllers use the identical expression for searches.app/Support/Updater.php (1)
39-46: Consider a single source of truth for bundled plugins.Line 43 is correct, but
Updater::BUNDLED_PLUGINSnow duplicates the list also maintained inapp/Support/PluginManager.php(Line 45-52). This duplication is easy to drift again; centralizing the list would reduce future consistency regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Support/Updater.php` around lines 39 - 46, Updater::BUNDLED_PLUGINS duplicates the same plugin list maintained in PluginManager, causing drift; remove the duplicate constant from Updater and have Updater read the canonical list from PluginManager (e.g., reference the PluginManager constant or accessor method that holds the bundled plugins), updating any Updater code that used BUNDLED_PLUGINS to use PluginManager::... or PluginManager::getBundledPlugins() instead so there is a single source of truth.tests/helpers/branch-fix-harness.php (3)
307-310: Consider adding soft-delete condition for guideline compliance.Per coding guidelines, queries on the
libritable should includeAND deleted_at IS NULL. While these test scenarios create and immediately query records that are never soft-deleted, adding the condition would maintain consistency with production code patterns.Note: The query at line 339 intentionally omits this condition to verify that soft-deleted records aren't modified.
♻️ Suggested change
$created = $db->query( 'SELECT titolo, descrizione_plain, collana, numero_serie, traduttore, issn, copie_totali, copie_disponibili - FROM libri WHERE id = ' . $bookId + FROM libri WHERE id = ' . $bookId . ' AND deleted_at IS NULL' )->fetch_assoc();As per coding guidelines: "Every query on
libritable MUST include soft-delete condition:AND deleted_at IS NULL"Also applies to: 320-322
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/helpers/branch-fix-harness.php` around lines 307 - 310, The SELECT against the libri table in the test uses $db->query(...)->fetch_assoc() and must include the soft-delete filter; update the query string that builds the SELECT (the one selecting titolo, descrizione_plain, collana, numero_serie, traduttore, issn, copie_totali, copie_disponibili for $bookId) to append "AND deleted_at IS NULL". Also apply the same change to the other similar query referenced around the $db->query call at lines noted (the earlier query at 320-322) so all queries against the libri table in this file include the soft-delete condition.
252-263: Reflection access to private methods is inherently fragile.
getLibraryThingMethods()uses reflection to access private methods (parseLibraryThingRow,insertBook,updateBook). If these method names or signatures change inLibraryThingImportController, the tests will fail at runtime with cryptic reflection errors rather than clear compile-time feedback.This is acceptable for a test harness, but consider documenting the expected signatures in a comment to aid future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/helpers/branch-fix-harness.php` around lines 252 - 263, The helper uses fragile reflection to access private methods parseLibraryThingRow, insertBook, and updateBook on LibraryThingImportController; add a concise doc comment above getLibraryThingMethods() that lists the expected method names and their signatures (parameters and return types) for parseLibraryThingRow, insertBook, and updateBook so maintainers know what to update if those methods change and to reduce runtime surprises when reflection fails.
480-481: Unused$dbparameter in export/roundtrip scenarios.
scenarioLibraryThingExportTranslatorRoundtripandscenarioLibraryThingSecondaryAuthorRolePairingdon't use the$dbparameter since they only test parsing/formatting logic without database operations. Consider removing the parameter or adding a PHPStan ignore annotation for signature consistency.♻️ Option A: Remove unused parameter
-function scenarioLibraryThingExportTranslatorRoundtrip(mysqli $db): array +function scenarioLibraryThingExportTranslatorRoundtrip(): array {Then update the match expression at line 757 accordingly.
♻️ Option B: Add PHPStan ignore
+/** `@phpstan-ignore-next-line` */ function scenarioLibraryThingExportTranslatorRoundtrip(mysqli $db): arrayAlso applies to: 518-519
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/helpers/branch-fix-harness.php` around lines 480 - 481, Two helper scenario functions, scenarioLibraryThingExportTranslatorRoundtrip and scenarioLibraryThingSecondaryAuthorRolePairing, declare an unused mysqli $db parameter; either remove the unused $db parameter from both function signatures and update every call site (including the match expression that builds the scenarios) to stop passing $db, or keep the signatures and add a PHPStan ignore annotation to suppress the unused-parameter warning (e.g. add a docblock like /** `@phpstan-ignore-next-line` */ or /** `@param` mysqli $db */ with an explicit ignore) so signature consistency remains; update the scenario registry/match expression that references these functions accordingly to match the chosen option.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/helpers/e2e-fixtures.js`:
- Around line 173-177: The helper currently converts dbQuery(...) to Number
which yields 0 on no rows; update the logic around the id assignment that calls
dbQuery and escapeSql so you first capture the raw query result (e.g., const row
= dbQuery(...)), verify that row is present and contains an id, and throw a
descriptive Error if not (e.g., "fixture lookup failed for email ..."), then
convert the found value to Number for return; apply the same
presence/check-and-throw fix to the other occurrence referenced (the block
around lines 186-189).
---
Duplicate comments:
In `@tests/helpers/branch-fix-harness.php`:
- Around line 433-445: The scenarioPublicApiAndFrontendIssn helper calls
LT-specific methods (via getLibraryThingMethods and invoking insertBook) without
checking whether the LibraryThing plugin schema is present; add the same guard
used in scenarioLibraryThingParseAndPersist by calling
LibraryThingInstaller::isInstalled($db) before invoking the parse/insert
reflection methods and skip or adjust the LT-specific assertions when not
installed. Locate scenarioPublicApiAndFrontendIssn and wrap the code that uses
$parse/$insert (from getLibraryThingMethods) in an if
(LibraryThingInstaller::isInstalled($db)) { ... } branch (or early return) so
insertBook is not invoked against a DB missing LT schema and the test behavior
matches the harness setup.
In `@tests/helpers/e2e-fixtures.js`:
- Around line 7-20: The helper currently falls back to non-E2E sources (APP_URL
and envFromFile) causing tests to accidentally target real environments; update
the logic in the block that sets BASE_URL, DB_HOST, DB_USER, DB_PASS, DB_NAME,
and DB_SOCKET (and the loadDotEnv usage) to only read from explicit E2E_*
environment variables (no fallback to process.env.APP_URL or envFromFile), and
throw clear errors if any required E2E_* var (E2E_BASE_URL, E2E_DB_USER,
E2E_DB_NAME, etc.) is missing so fixtures fail fast.
---
Nitpick comments:
In `@app/Controllers/LibraryThingImportController.php`:
- Around line 35-55: The per-instance cache $cachedHasDescPlain in
hasDescrizionePlainColumn() can diverge from
FrontendController::descriptionExpr() which uses static $hasDescrizionePlain;
replace the instance-level caching with a shared static/cache approach so all
controllers agree (e.g., move the column-existence check into a single static
helper or reuse FrontendController::descriptionExpr() logic), update
hasDescrizionePlainColumn() to consult and set that shared static cache instead
of $cachedHasDescPlain, and preserve the existing query/exception handling and
result freeing behavior when computing the shared value.
- Around line 508-521: Replace direct error_log() calls in
LibraryThingImportController's import history persistence error paths with
SecureLogger::error() so sensitive data is logged via the secure logger;
specifically update the three occurrences inside the persistence failure branch
and the outer and inner catch blocks (the messages that include get_class($e)
and $e->getMessage(), and the inner->getMessage()) to call SecureLogger::error()
with the same contextual strings, ensure SecureLogger is imported/available in
the class scope, and keep the existing calls to $importLogger->fail(...)
unchanged.
- Line 1650: Replace the direct PHP error_log call used for cover download
failures in LibraryThingImportController with the application's SecureLogger
error method to maintain consistent logging; locate the cover download error
handling where error_log("[LT Import] Cover download failed for book $bookId: "
. $e->getMessage()) is called and change it to call SecureLogger::error (or the
controller's injected secure logger) with the same message/context and include
the exception details so the bookId and $e->getMessage() are preserved.
In `@app/Controllers/LibriApiController.php`:
- Around line 45-48: Extract the duplicated description-expression logic into a
shared helper used by both controllers: move the COALESCE(NULLIF(...), ...) +
schema-probe logic currently duplicated in FrontendController and the $descExpr
calculation in LibriApiController into a single function (e.g., a static helper
like getDescriptionExpression or a method on a shared SearchHelper/Libri model).
Replace the inline $descExpr construction in LibriApiController (which currently
uses hasTableColumn($db, 'libri', 'descrizione_plain')) with a call to that
shared function and update FrontendController to call the same function so both
controllers use the identical expression for searches.
In `@app/Support/Updater.php`:
- Around line 39-46: Updater::BUNDLED_PLUGINS duplicates the same plugin list
maintained in PluginManager, causing drift; remove the duplicate constant from
Updater and have Updater read the canonical list from PluginManager (e.g.,
reference the PluginManager constant or accessor method that holds the bundled
plugins), updating any Updater code that used BUNDLED_PLUGINS to use
PluginManager::... or PluginManager::getBundledPlugins() instead so there is a
single source of truth.
In `@tests/helpers/branch-fix-harness.php`:
- Around line 307-310: The SELECT against the libri table in the test uses
$db->query(...)->fetch_assoc() and must include the soft-delete filter; update
the query string that builds the SELECT (the one selecting titolo,
descrizione_plain, collana, numero_serie, traduttore, issn, copie_totali,
copie_disponibili for $bookId) to append "AND deleted_at IS NULL". Also apply
the same change to the other similar query referenced around the $db->query call
at lines noted (the earlier query at 320-322) so all queries against the libri
table in this file include the soft-delete condition.
- Around line 252-263: The helper uses fragile reflection to access private
methods parseLibraryThingRow, insertBook, and updateBook on
LibraryThingImportController; add a concise doc comment above
getLibraryThingMethods() that lists the expected method names and their
signatures (parameters and return types) for parseLibraryThingRow, insertBook,
and updateBook so maintainers know what to update if those methods change and to
reduce runtime surprises when reflection fails.
- Around line 480-481: Two helper scenario functions,
scenarioLibraryThingExportTranslatorRoundtrip and
scenarioLibraryThingSecondaryAuthorRolePairing, declare an unused mysqli $db
parameter; either remove the unused $db parameter from both function signatures
and update every call site (including the match expression that builds the
scenarios) to stop passing $db, or keep the signatures and add a PHPStan ignore
annotation to suppress the unused-parameter warning (e.g. add a docblock like
/** `@phpstan-ignore-next-line` */ or /** `@param` mysqli $db */ with an explicit
ignore) so signature consistency remains; update the scenario registry/match
expression that references these functions accordingly to match the chosen
option.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b47482a2-c39f-4816-b7e7-534737ee16a7
📒 Files selected for processing (30)
.gitignoreapp/Controllers/AuthController.phpapp/Controllers/CollaneController.phpapp/Controllers/FrontendController.phpapp/Controllers/LibraryThingImportController.phpapp/Controllers/LibriApiController.phpapp/Controllers/LibriController.phpapp/Controllers/PluginController.phpapp/Controllers/ProfileController.phpapp/Controllers/PublicApiController.phpapp/Middleware/RememberMeMiddleware.phpapp/Support/PluginManager.phpapp/Support/Updater.phpapp/Views/admin/plugins.phpapp/Views/frontend/book-detail.phpapp/Views/libri/scheda_libro.phpapp/Views/profile/index.phplocale/de_DE.jsonlocale/en_US.jsonlocale/it_IT.jsonstorage/plugins/goodlib/GoodLibPlugin.phpstorage/plugins/goodlib/plugin.jsonstorage/plugins/goodlib/views/badges.phpstorage/plugins/goodlib/views/settings.phpstorage/plugins/goodlib/wrapper.phptests/branch-fix-consistency.spec.jstests/full-test.spec.jstests/goodlib-custom-domains.spec.jstests/helpers/branch-fix-harness.phptests/helpers/e2e-fixtures.js
- e2e-fixtures: remove .env/APP_URL fallback, require E2E_* vars only - e2e-fixtures: fail-fast on createTempAdminUser/createTempBook (ID=0) - branch-fix-harness: install LT schema in scenarioPublicApiAndFrontendIssn - LibraryThingImportController: error_log → SecureLogger::error (4 occurrences)
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Controllers/ProfileController.php (1)
20-24:⚠️ Potential issue | 🟠 MajorGuard the
prepare()call inshow()beforebind_param().Line 20's query now selects the
localecolumn; ifprepare()fails (schema mismatch on partial migration, DB error, etc.), Line 21 dereferencesfalsewithbind_param()and causes a fatal error. Mirror theif (!$stmt)guard you already use inchangePassword()(line 63) andupdate()(line 166), logging the failure withSecureLogger::error().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/ProfileController.php` around lines 20 - 24, In the show() method, guard the $db->prepare(...) result before calling $stmt->bind_param() by checking if $stmt is falsy; if prepare() returns false, call SecureLogger::error() with a clear message (include context like the query or uid) and return/handle gracefully (same pattern used in changePassword() and update()) to avoid dereferencing $stmt when prepare fails.
♻️ Duplicate comments (4)
tests/helpers/e2e-fixtures.js (2)
173-189:⚠️ Potential issue | 🟡 MinorFail fast when fixture row lookup returns no ID.
Number(dbQuery(...))can still yield0on empty output in both temp-user and temp-book creation paths; throw immediately when lookup fails.Suggested guard checks
- const id = Number(dbQuery( - `SELECT id FROM utenti WHERE email = '${escapeSql(email)}' LIMIT 1` - )); + const userIdRaw = dbQuery( + `SELECT id FROM utenti WHERE email = '${escapeSql(email)}' LIMIT 1` + ); + const id = Number(userIdRaw); + if (!Number.isInteger(id) || id <= 0) { + throw new Error(`Failed to resolve temp admin user id: ${email}`); + } @@ - const id = Number(dbQuery( - `SELECT id FROM libri WHERE titolo = '${escapeSql(title)}' ORDER BY id DESC LIMIT 1` - )); + const bookIdRaw = dbQuery( + `SELECT id FROM libri WHERE titolo = '${escapeSql(title)}' ORDER BY id DESC LIMIT 1` + ); + const id = Number(bookIdRaw); + if (!Number.isInteger(id) || id <= 0) { + throw new Error(`Failed to resolve temp book id: ${title}`); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/helpers/e2e-fixtures.js` around lines 173 - 189, Both temp-entity creation paths currently use Number(dbQuery(...)) which can yield 0 for missing rows; update the lookup in the user creation (the query that selects id from utenti by email) and createTempBook (the query that selects id from libri by titolo) to validate the dbQuery result, coerce to a Number, and immediately throw a clear error if the resulting id is NaN or <= 0 (include the email or title in the error message) instead of returning a 0 id; locate and modify the id extraction logic in the functions that perform these queries to add the guard and throw.
10-20:⚠️ Potential issue | 🟠 MajorRequire strict
E2E_*config only for destructive fixtures.This still falls back to
APP_URLand repo.env, so a misconfigured run can mutate a non-test environment. MakeBASE_URLand DB connection values E2E-only and fail fast if missing.Based on learnings, E2E tests with Playwright require `/tmp/run-e2e.sh` to set DB/admin credentials as env vars.Suggested hardening
-const envFromFile = loadDotEnv(path.join(ROOT, '.env')); - -const BASE_URL = process.env.E2E_BASE_URL || process.env.APP_URL; +const BASE_URL = process.env.E2E_BASE_URL || ''; if (!BASE_URL) { - throw new Error('E2E_BASE_URL or APP_URL must be set (use /tmp/run-e2e.sh)'); + throw new Error('E2E_BASE_URL must be set (use /tmp/run-e2e.sh)'); } -const DB_HOST = process.env.E2E_DB_HOST || envFromFile.DB_HOST || ''; -const DB_USER = process.env.E2E_DB_USER || envFromFile.DB_USER || ''; -const DB_PASS = process.env.E2E_DB_PASS ?? envFromFile.DB_PASS ?? ''; -const DB_NAME = process.env.E2E_DB_NAME || envFromFile.DB_NAME || ''; -const DB_SOCKET = process.env.E2E_DB_SOCKET || envFromFile.DB_SOCKET || ''; -if (!DB_USER || !DB_NAME) { - throw new Error('E2E_DB_USER and E2E_DB_NAME must be set (use /tmp/run-e2e.sh)'); +const DB_HOST = process.env.E2E_DB_HOST || ''; +const DB_USER = process.env.E2E_DB_USER || ''; +const DB_PASS = process.env.E2E_DB_PASS ?? ''; +const DB_NAME = process.env.E2E_DB_NAME || ''; +const DB_SOCKET = process.env.E2E_DB_SOCKET || ''; +if (!DB_USER || !DB_NAME || (!DB_HOST && !DB_SOCKET)) { + throw new Error('Missing required E2E_DB_* configuration'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/helpers/e2e-fixtures.js` around lines 10 - 20, Change the fixture env parsing to require E2E-only variables and fail fast: stop falling back to APP_URL or envFromFile for BASE_URL and DB_*—read only process.env.E2E_BASE_URL, process.env.E2E_DB_HOST, process.env.E2E_DB_USER, process.env.E2E_DB_PASS, process.env.E2E_DB_NAME, process.env.E2E_DB_SOCKET and throw an Error if any required E2E_* (at least E2E_BASE_URL, E2E_DB_USER and E2E_DB_NAME) are missing; update the code that defines BASE_URL, DB_HOST, DB_USER, DB_PASS, DB_NAME and DB_SOCKET to remove the non-E2E fallbacks so tests fail immediately when E2E credentials are not provided.app/Views/admin/plugins.php (1)
220-220:⚠️ Potential issue | 🟠 MajorStop hardcoding admin routes in these attributes.
These buttons now read their endpoint from
data-settings-url, but the new attributes still embed/admin/...literals. If admin routes are translated or renamed, the modal buttons break again. Resolve them throughroute_path()orRouteTranslator::route()instead. As per coding guidelines,Never use hardcoded routes. Use route_path('key') or RouteTranslator::route('key') instead of hardcoded paths like /accedi or /login.Also applies to: 231-231, 244-244, 262-262, 276-276, 286-286
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/admin/plugins.php` at line 220, Replace all hardcoded "/admin/..." route fragments used in data attributes (e.g., data-settings-url, other data-* attributes around the plugin actions that build URLs using (int) $plugin['id']) with framework route helpers: call route_path('route_name', ['id' => (int) $plugin['id']]) or RouteTranslator::route('route_name', ['id' => (int) $plugin['id']]) to generate the URL; update every occurrence that currently concatenates "/admin/plugins/" + (int) $plugin['id'] + "/settings" (and the other similar endpoints at the noted locations) to use the appropriate named route key (the route name for plugin settings/actions) and pass the id param so the attributes read a resolved, non-hardcoded URL.tests/helpers/branch-fix-harness.php (1)
433-445:⚠️ Potential issue | 🟠 MajorInstall/assert the LT schema before this reflected insert path too.
insertBook()still branches on\App\Support\LibraryThingInstaller::isInstalled($db)inapp/Controllers/LibraryThingImportController.php:1393-1403. On a clean test DB this scenario can take the non-LT branch, soissndisappears before the API/frontend assertions even run.🛠️ Proposed fix
function scenarioPublicApiAndFrontendIssn(mysqli $db): array { $_SESSION = []; $factory = new ServerRequestFactory(); $publicApi = new PublicApiController(); + + if (!\App\Support\LibraryThingInstaller::isInstalled($db)) { + $installer = new \App\Support\LibraryThingInstaller($db); + $installer->install(); + } + [$controller, $parse, $insert] = getLibraryThingMethods(); $runId = randomRunId(); $bookId = 0; $baseUrl = rtrim(getenv('BASE_URL') ?: 'http://localhost:8081', '/');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/helpers/branch-fix-harness.php` around lines 433 - 445, The scenarioPublicApiAndFrontendIssn test harness currently calls the reflected insert via getLibraryThingMethods() and insert->invoke(...) without ensuring the LibraryThing schema is present, so insertBook() may take the non-LT branch; before invoking $insert (or immediately after creating $db) call or assert the installer so \App\Support\LibraryThingInstaller::isInstalled($db) returns true—i.e., run the same install/assert routine used by other tests (or call the LibraryThingInstaller install/assert method) so the reflected insert path uses the LT branch and the issn field remains present for the API/frontend assertions.
🧹 Nitpick comments (2)
storage/plugins/goodlib/views/settings.php (1)
16-130: Avoid maintaining a second GoodLib settings UI.This template now overlaps almost 1:1 with the modal in
app/Views/admin/plugins.phplines 707-848, but the two copies already diverge (datalisttext inputs here vs. preset selects/custom fields there). Please collapse them to one shared partial before they drift further.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/goodlib/views/settings.php` around lines 16 - 130, The settings UI blocks (the Sources/Visibility/Domains markup using $sourcesList, $settings, and the anna_domain_options / zlib_domain_options datalists) are duplicated between this settings.php and the admin plugins modal—extract those repeated blocks into a single reusable partial (e.g., a view fragment) that accepts $settings and the domain option arrays, replace the duplicated markup here and in the admin modal with a single include/echo of that partial, and ensure the partial preserves the same input attributes (name, pattern, maxlength, htmlspecialchars usage and datalist IDs) so behavior remains identical and divergence is avoided.tests/helpers/branch-fix-harness.php (1)
214-214: Use route helpers in the harness instead of literal paths.These scenarios hardcode both request URIs and an expected redirect target, so route translation or route-key changes will break the harness even when the controller behavior is still correct. Resolve the paths through
route_path()/RouteTranslator::route()before building the request and when asserting redirects.As per coding guidelines,
Never use hardcoded routes. Use route_path('key') or RouteTranslator::route('key') instead of hardcoded paths like /accedi or /login.Also applies to: 403-403, 418-418, 448-448, 456-456, 572-572, 583-583, 615-615, 666-666
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/helpers/branch-fix-harness.php` at line 214, Replace hardcoded URIs used to build requests and to assert redirects with route resolution helpers: instead of passing '/admin/collane/rinomina' into ->createServerRequest(), call route_path('route.key.for.rinomina') or RouteTranslator::route('route.key.for.rinomina') to obtain the URI and use that same resolved value for any redirect assertions; update the createServerRequest call and any subsequent redirect assertions in tests/helpers/branch-fix-harness.php (and the other listed occurrences) to use route_path() or RouteTranslator::route() so route-key changes won’t break the harness.
🤖 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/LibraryThingImportController.php`:
- Around line 1248-1252: The update/insert SQL still writes the ean column and
can hit the unique index; in upsertBook() add the same pre-upsert
conflict-clearing for ean as done for isbn10/isbn13: if $data['ean'] is
non-empty run an UPDATE libri SET ean = NULL WHERE ean = ? AND deleted_at IS
NULL (and if $existingBookId !== null use AND id != ?), execute and close the
statement before performing the INSERT/UPDATE that writes ean so any other live
book with that barcode is nulled first.
In `@app/Views/admin/plugins.php`:
- Around line 258-264: The button's data-enable-server attribute is reading the
wrong key; update the admin view (plugin modal seed) to read
$z39Settings['server_enabled'] (which PluginController::updateSettings() now
saves as 'true'/'false') and convert it to the expected '1'/'0' value for the
attribute (e.g., treat 'true' as '1' and anything else as '0'), replacing the
existing ($z39Settings['enable_server'] ?? '0') === '1' check so the modal
reflects the persisted server_enabled state correctly.
In `@storage/plugins/goodlib/GoodLibPlugin.php`:
- Around line 181-207: dbSetSetting currently swallows DB prepare/execute
failures and returns void, causing saveSettings to report success even when
writes fail; change dbSetSetting to return a boolean success flag (true on
successful set, false on any failure), ensure the fallback path checks prepare()
and execute() return values and returns false on error (or rethrow), and update
all call sites (including saveSettings and the other occurrence around lines
468-495) to check the returned bool and propagate/fail saveSettings accordingly
so the admin flow accurately reports persistence failures.
- Around line 390-391: The ISBN assignment for $isbn in GoodLibPlugin.php uses
the null coalescing operator which doesn't fall back when isbn13 is an empty
string; change the logic in the $isbn assignment (where $bookData['isbn13'] and
$bookData['isbn10'] are used) to explicitly prefer a non-empty isbn13 and fall
back to isbn10 (for example using a conditional that checks trim/empty or the ?:
operator) before casting and trimming, so an empty isbn13 won't block using
isbn10.
In `@storage/plugins/goodlib/views/badges.php`:
- Line 18: The translated string __("Cerca su:") is output raw inside the span
with class "font-medium", creating an XSS risk; change the echo to escape the
translation for HTML text context by wrapping the call to __() with the
project's HTML-escaping helper (e.g., h(), e(), or htmlspecialchars) so the
content is safely encoded before rendering.
In `@tests/branch-fix-consistency.spec.js`:
- Around line 24-31: The test currently calls JSON.parse(output) which hides raw
harness output on parse failure; wrap the JSON.parse(output) call in a try/catch
and on any parse error re-throw an Error that includes the original output (the
variable `output`) or attach it to the thrown error so the original PHP harness
snippet/warning/HTML is preserved for debugging; update the block surrounding
the existing catch that sets `output` (the same `output` variable referenced in
JSON.parse) so parse failures surface the raw output instead of only the JSON
parsing message.
In `@tests/goodlib-custom-domains.spec.js`:
- Around line 24-27: Ensure the test fails fast if the GoodLib plugin ID is
missing: after calling getPluginIdByName('goodlib') (assigned to pluginId) add a
guard that checks pluginId is truthy and throws or fails the test with a clear
message (e.g., "GoodLib plugin ID not found") before calling
snapshotPluginSettings(pluginId) or any other functions that depend on it; this
makes the failure immediate and more actionable.
- Line 14: The test uses .describe.serial but there's no enforced way to run it
with /tmp/run-e2e.sh and --workers=1 as required by CLAUDE.md; add a dedicated
npm script (e.g. "test:e2e" or "e2e") in package.json that invokes
/tmp/run-e2e.sh --workers=1 and update CI to call that script (or create a CI
workflow that runs /tmp/run-e2e.sh --workers=1), or alternatively add a
prominent header comment in tests/goodlib-custom-domains.spec.js referencing the
required invocation and CLAUDE.md so maintainers know to run the suite with
/tmp/run-e2e.sh and --workers=1.
---
Outside diff comments:
In `@app/Controllers/ProfileController.php`:
- Around line 20-24: In the show() method, guard the $db->prepare(...) result
before calling $stmt->bind_param() by checking if $stmt is falsy; if prepare()
returns false, call SecureLogger::error() with a clear message (include context
like the query or uid) and return/handle gracefully (same pattern used in
changePassword() and update()) to avoid dereferencing $stmt when prepare fails.
---
Duplicate comments:
In `@app/Views/admin/plugins.php`:
- Line 220: Replace all hardcoded "/admin/..." route fragments used in data
attributes (e.g., data-settings-url, other data-* attributes around the plugin
actions that build URLs using (int) $plugin['id']) with framework route helpers:
call route_path('route_name', ['id' => (int) $plugin['id']]) or
RouteTranslator::route('route_name', ['id' => (int) $plugin['id']]) to generate
the URL; update every occurrence that currently concatenates "/admin/plugins/" +
(int) $plugin['id'] + "/settings" (and the other similar endpoints at the noted
locations) to use the appropriate named route key (the route name for plugin
settings/actions) and pass the id param so the attributes read a resolved,
non-hardcoded URL.
In `@tests/helpers/branch-fix-harness.php`:
- Around line 433-445: The scenarioPublicApiAndFrontendIssn test harness
currently calls the reflected insert via getLibraryThingMethods() and
insert->invoke(...) without ensuring the LibraryThing schema is present, so
insertBook() may take the non-LT branch; before invoking $insert (or immediately
after creating $db) call or assert the installer so
\App\Support\LibraryThingInstaller::isInstalled($db) returns true—i.e., run the
same install/assert routine used by other tests (or call the
LibraryThingInstaller install/assert method) so the reflected insert path uses
the LT branch and the issn field remains present for the API/frontend
assertions.
In `@tests/helpers/e2e-fixtures.js`:
- Around line 173-189: Both temp-entity creation paths currently use
Number(dbQuery(...)) which can yield 0 for missing rows; update the lookup in
the user creation (the query that selects id from utenti by email) and
createTempBook (the query that selects id from libri by titolo) to validate the
dbQuery result, coerce to a Number, and immediately throw a clear error if the
resulting id is NaN or <= 0 (include the email or title in the error message)
instead of returning a 0 id; locate and modify the id extraction logic in the
functions that perform these queries to add the guard and throw.
- Around line 10-20: Change the fixture env parsing to require E2E-only
variables and fail fast: stop falling back to APP_URL or envFromFile for
BASE_URL and DB_*—read only process.env.E2E_BASE_URL, process.env.E2E_DB_HOST,
process.env.E2E_DB_USER, process.env.E2E_DB_PASS, process.env.E2E_DB_NAME,
process.env.E2E_DB_SOCKET and throw an Error if any required E2E_* (at least
E2E_BASE_URL, E2E_DB_USER and E2E_DB_NAME) are missing; update the code that
defines BASE_URL, DB_HOST, DB_USER, DB_PASS, DB_NAME and DB_SOCKET to remove the
non-E2E fallbacks so tests fail immediately when E2E credentials are not
provided.
---
Nitpick comments:
In `@storage/plugins/goodlib/views/settings.php`:
- Around line 16-130: The settings UI blocks (the Sources/Visibility/Domains
markup using $sourcesList, $settings, and the anna_domain_options /
zlib_domain_options datalists) are duplicated between this settings.php and the
admin plugins modal—extract those repeated blocks into a single reusable partial
(e.g., a view fragment) that accepts $settings and the domain option arrays,
replace the duplicated markup here and in the admin modal with a single
include/echo of that partial, and ensure the partial preserves the same input
attributes (name, pattern, maxlength, htmlspecialchars usage and datalist IDs)
so behavior remains identical and divergence is avoided.
In `@tests/helpers/branch-fix-harness.php`:
- Line 214: Replace hardcoded URIs used to build requests and to assert
redirects with route resolution helpers: instead of passing
'/admin/collane/rinomina' into ->createServerRequest(), call
route_path('route.key.for.rinomina') or
RouteTranslator::route('route.key.for.rinomina') to obtain the URI and use that
same resolved value for any redirect assertions; update the createServerRequest
call and any subsequent redirect assertions in
tests/helpers/branch-fix-harness.php (and the other listed occurrences) to use
route_path() or RouteTranslator::route() so route-key changes won’t break the
harness.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a56f6fe2-7dab-4f30-a951-696c448e226b
📒 Files selected for processing (30)
.gitignoreapp/Controllers/AuthController.phpapp/Controllers/CollaneController.phpapp/Controllers/FrontendController.phpapp/Controllers/LibraryThingImportController.phpapp/Controllers/LibriApiController.phpapp/Controllers/LibriController.phpapp/Controllers/PluginController.phpapp/Controllers/ProfileController.phpapp/Controllers/PublicApiController.phpapp/Middleware/RememberMeMiddleware.phpapp/Support/PluginManager.phpapp/Support/Updater.phpapp/Views/admin/plugins.phpapp/Views/frontend/book-detail.phpapp/Views/libri/scheda_libro.phpapp/Views/profile/index.phplocale/de_DE.jsonlocale/en_US.jsonlocale/it_IT.jsonstorage/plugins/goodlib/GoodLibPlugin.phpstorage/plugins/goodlib/plugin.jsonstorage/plugins/goodlib/views/badges.phpstorage/plugins/goodlib/views/settings.phpstorage/plugins/goodlib/wrapper.phptests/branch-fix-consistency.spec.jstests/full-test.spec.jstests/goodlib-custom-domains.spec.jstests/helpers/branch-fix-harness.phptests/helpers/e2e-fixtures.js
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 (2)
app/Controllers/AuthController.php (1)
51-56:⚠️ Potential issue | 🟠 MajorWrap the database query in a try-catch block to handle exceptions.
With
MYSQLI_REPORT_STRICTenabled in ConfigStore.php,prepare(),bind_param(), andexecute()throw exceptions on failure rather than returning false. The current code has no exception handling, so a database error becomes an unhandled exception and produces a 500 error instead of a controlled login failure path. Catch\Throwableand return an appropriate auth error response.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/AuthController.php` around lines 51 - 56, Wrap the database query (the sequence using $db->prepare(...), $stmt->bind_param('s', $email), $stmt->execute(), $stmt->get_result() and $res->fetch_assoc(), and $stmt->close()) in a try-catch that catches \Throwable; on any caught exception close/free the statement if set and return the controlled authentication error response used by this controller (the same path used for invalid credentials) instead of letting the exception bubble up. Ensure the catch block prevents leaking exception details and maps failures during prepare/bind/execute/get_result to the normal auth failure response.app/Controllers/LibraryThingImportController.php (1)
1198-1227:⚠️ Potential issue | 🟠 MajorClear
eanconflicts before writes to avoid unique constraint violations.The conflict-clearing prelude nullifies
isbn13andisbn10on other rows before upsert, buteanis still written without the same conflict-clearing step. A row whose barcode already belongs to another live book will still hit the unique index and roll back the chunk.🛠️ Proposed fix (add EAN conflict clearing)
if ($conflictsCleared > 0) { $this->log("[upsertBook] Cleared ISBN10 '{$data['isbn10']}' from $conflictsCleared conflicting book(s)"); } } + if (!empty($data['ean'])) { + $stmt = $db->prepare("UPDATE libri SET ean = NULL WHERE ean = ? AND id != ? AND deleted_at IS NULL"); + $stmt->bind_param('si', $data['ean'], $existingBookId); + $stmt->execute(); + $conflictsCleared = $stmt->affected_rows; + $stmt->close(); + if ($conflictsCleared > 0) { + $this->log("[upsertBook] Cleared EAN '{$data['ean']}' from $conflictsCleared conflicting book(s)"); + } + } $this->updateBook($db, $existingBookId, $data, $editorId, $genreId);For the INSERT path (when
$existingBookIdis null), use:if (!empty($data['ean'])) { $stmt = $db->prepare("UPDATE libri SET ean = NULL WHERE ean = ? AND deleted_at IS NULL"); $stmt->bind_param('s', $data['ean']); $stmt->execute(); $stmt->close(); }Based on learnings,
Nullify unique-indexed columns (isbn10, isbn13, ean) on soft-delete of libri table records.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibraryThingImportController.php` around lines 1198 - 1227, The upsertBook flow clears conflicting isbn13/isbn10 values but omits ean, which can trigger unique-index violations; update upsertBook to mirror the isbn handling by nullifying conflicting ean values on other live rows before writing (in the existingBookId != null branch do an UPDATE libri SET ean = NULL WHERE ean = ? AND id != ? AND deleted_at IS NULL and log affected rows) and also add the same pre-write ean conflict-clear step in the INSERT path (when findExistingBook returns null) using UPDATE libri SET ean = NULL WHERE ean = ? AND deleted_at IS NULL so ean uniqueness won't block the insert; touch the same helper flow that precedes calling updateBook/insert logic and reference $data['ean'], upsertBook, findExistingBook, and updateBook when making the changes.
♻️ Duplicate comments (8)
app/Controllers/ProfileController.php (1)
20-24:⚠️ Potential issue | 🟠 MajorHandle
prepare()failure inshow()before binding.This lookup now selects
locale, but the method still assumes statement creation succeeded. A prepare failure here becomes a fatal error on the profile page instead of a handled server failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/ProfileController.php` around lines 20 - 24, The prepare() call in ProfileController::show (the $stmt = $db->prepare(...) line) can return false and must be checked before calling $stmt->bind_param, otherwise a fatal error occurs; update show() to verify $stmt !== false, and if prepare failed log the DB error (or $db->error), return/throw a handled server error response (e.g. render an error page or send a 500) and avoid calling bind_param/execute/close; keep existing logic for successful prepares (bind_param, execute, fetch_assoc, close) unchanged.storage/plugins/goodlib/views/badges.php (1)
18-18:⚠️ Potential issue | 🟠 MajorEscape translated text before rendering in HTML.
Line 18 outputs
__("Cerca su:")raw. If translation content is ever compromised, this can inject markup/script in page content.Suggested fix
- <span class="font-medium"><?= __("Cerca su:") ?></span> + <span class="font-medium"><?= htmlspecialchars(__("Cerca su:"), ENT_QUOTES, 'UTF-8') ?></span>Based on learnings: Use
htmlspecialchars(url(...), ENT_QUOTES, 'UTF-8')in ALL HTML attributes for view escaping.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/goodlib/views/badges.php` at line 18, The translated string output __("Cerca su:") in badges.php is rendered raw; wrap the translation with an HTML-escaping helper (e.g., call htmlspecialchars(__(...), ENT_QUOTES, 'UTF-8') or the framework's h() escape helper) before echoing so the span renders escaped text and prevents injected markup/scripts.app/Views/admin/plugins.php (2)
258-264:⚠️ Potential issue | 🟠 MajorSeed the Z39 modal from
server_enabled, notenable_server.
PluginController::updateSettings()now persistsserver_enabledastrue/false. Readingenable_server === '1'here reopens the modal in the wrong state, and the next save can silently disable the server.Minimal fix
- data-enable-server="<?= ($z39Settings['enable_server'] ?? '0') === '1' ? '1' : '0' ?>" + data-enable-server="<?= ($z39Settings['server_enabled'] ?? 'false') === 'true' ? '1' : '0' ?>"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/admin/plugins.php` around lines 258 - 264, The modal seed uses the old string flag; update the data attribute to read the persisted boolean field instead: replace uses of ($z39Settings['enable_server'] ?? '0') === '1' with checking ($z39Settings['server_enabled'] ?? false) (or equivalent truthy check) when setting data-enable-server on the button in admin/plugins.php so the UI reflects the true persisted state (also verify data-enable-client remains correct and that PluginController::updateSettings() persists server_enabled).
215-221:⚠️ Potential issue | 🟠 MajorUse route helpers instead of hardcoded
/admin/plugins/...paths.These
data-settings-urlattributes still bake the literal settings path into the view. If admin routes are translated or renamed, all of these modals break again.As per coding guidelines,
Never use hardcoded routes. Use route_path('key') or RouteTranslator::route('key') instead of hardcoded paths like /accedi or /login.Also applies to: 226-232, 239-245, 258-262, 283-286
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/admin/plugins.php` around lines 215 - 221, Replace the hardcoded settings URL in the data-settings-url attribute with a route helper call (e.g., RouteTranslator::route(...) or route_path(...)) so the plugin settings URL is generated from the named admin route rather than "/admin/plugins/{id}/settings"; update the same change for all other similar attributes in this file (the other button blocks referenced) and ensure you pass the plugin id (from $plugin['id']) as the route parameter so openPluginSettingsModal receives a correct, translatable URL.tests/goodlib-custom-domains.spec.js (1)
23-27:⚠️ Potential issue | 🟡 MinorFail fast if GoodLib isn't registered.
Without a guard after
getPluginIdByName('goodlib'), the suite snapshots/restores settings for0and the real problem shows up much later.Suggested hardening
test.beforeAll(() => { pluginId = getPluginIdByName('goodlib'); + if (!pluginId) { + throw new Error('GoodLib plugin not found; cannot run custom-domain E2E test.'); + } originalSettings = snapshotPluginSettings(pluginId); adminUser = createTempAdminUser('it_IT'); book = createTempBook(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/goodlib-custom-domains.spec.js` around lines 23 - 27, The test setup in test.beforeAll calls getPluginIdByName('goodlib') into pluginId and proceeds without verifying it, causing later failures when pluginId is 0; update test.beforeAll to check the returned pluginId immediately (e.g., if falsy or 0) and fail fast by throwing or calling test.skip with a clear message that GoodLib is not registered so snapshotPluginSettings/restore and subsequent operations are not executed for an invalid pluginId.tests/branch-fix-consistency.spec.js (1)
24-31:⚠️ Potential issue | 🟡 MinorKeep the raw harness output when parsing fails.
The final parse step turns a PHP warning or HTML error page into
Unexpected token ..., even thoughoutputwas already captured above.Suggested hardening
- return JSON.parse(output); + try { + return JSON.parse(output); + } catch { + throw new Error(`Invalid harness output for "${name}": ${output.slice(0, 500)}`); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/branch-fix-consistency.spec.js` around lines 24 - 31, The code currently does JSON.parse(output) unguarded so a parse error (e.g. PHP warning/HTML) loses the captured raw harness output; wrap the JSON.parse(output) in a try/catch and on parse failure rethrow an Error (or augment the thrown error) that includes the original raw output string (variable output) so callers see the raw harness output alongside the parse error; locate the JSON.parse call and the output variable in this file (tests/branch-fix-consistency.spec.js) to implement the safe-parse and preserve-output behavior.storage/plugins/goodlib/GoodLibPlugin.php (2)
181-207:⚠️ Potential issue | 🟠 MajorMake
dbSetSetting()report failures.The
PluginManagerpath ignores the boolean return fromsetSetting(), and the fallback SQL path ignoresprepare()/execute()failures. That leavesonActivate()andsaveSettings()unable to tell whether anything was actually persisted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/goodlib/GoodLibPlugin.php` around lines 181 - 207, dbSetSetting currently returns void and ignores failures from PluginManager::setSetting and from the direct SQL path; change dbSetSetting to return bool (true on success, false on any failure), use the boolean return value from \App\Support\PluginManager::setSetting($this->pluginId, $key, $value, true) and immediately return that result, and in the fallback path check that $this->db->prepare(...) returned a statement, that bind_param and execute succeed (return false on any failure), and close the statement before returning true; update callers like onActivate() and saveSettings() to check the new boolean return and handle/report failures accordingly.
390-391:⚠️ Potential issue | 🟡 MinorUse
isbn10whenisbn13is blank.An empty
isbn13still wins here, so books withisbn13 = ''and a populatedisbn10lose the precise ISBN search and fall back to the broader title/author query.Minimal fix
- $isbn = trim((string) ($bookData['isbn13'] ?? $bookData['isbn10'] ?? '')); + $isbn13 = trim((string) ($bookData['isbn13'] ?? '')); + $isbn10 = trim((string) ($bookData['isbn10'] ?? '')); + $isbn = $isbn13 !== '' ? $isbn13 : $isbn10;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/plugins/goodlib/GoodLibPlugin.php` around lines 390 - 391, The current selection for $isbn in GoodLibPlugin.php uses the null coalescing operator on $bookData['isbn13'] which still selects an empty string when isbn13 exists but is blank; change the logic that sets $isbn (the line creating $isbn from $bookData) to prefer a non-empty trimmed isbn13 and only fall back to isbn10 when isbn13 is empty or whitespace (e.g., compute trimmed values and choose isbn13 if !== '' else isbn10) so precise ISBN searches use isbn10 when isbn13 is blank.
🧹 Nitpick comments (1)
tests/helpers/branch-fix-harness.php (1)
58-68: Consider using prepared statements for consistency.While the
inttype hints provide protection against SQL injection, using prepared statements would be more consistent with the rest of the codebase and serve as a better reference pattern.♻️ Optional refactor
function deleteBook(mysqli $db, int $bookId): void { - $db->query('DELETE FROM copie WHERE libro_id = ' . $bookId); - $db->query('DELETE FROM libri WHERE id = ' . $bookId); + $stmt = $db->prepare('DELETE FROM copie WHERE libro_id = ?'); + $stmt->bind_param('i', $bookId); + $stmt->execute(); + $stmt->close(); + + $stmt = $db->prepare('DELETE FROM libri WHERE id = ?'); + $stmt->bind_param('i', $bookId); + $stmt->execute(); + $stmt->close(); } function deleteUser(mysqli $db, int $userId): void { - $db->query('DELETE FROM user_sessions WHERE utente_id = ' . $userId); - $db->query('DELETE FROM utenti WHERE id = ' . $userId); + $stmt = $db->prepare('DELETE FROM user_sessions WHERE utente_id = ?'); + $stmt->bind_param('i', $userId); + $stmt->execute(); + $stmt->close(); + + $stmt = $db->prepare('DELETE FROM utenti WHERE id = ?'); + $stmt->bind_param('i', $userId); + $stmt->execute(); + $stmt->close(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/helpers/branch-fix-harness.php` around lines 58 - 68, The deleteBook and deleteUser functions currently build SQL with string concatenation which, despite int type hints, is inconsistent with the codebase; change both functions (deleteBook(mysqli $db, int $bookId) and deleteUser(mysqli $db, int $userId)) to use prepared statements: prepare the DELETE queries with placeholders for libro_id/id and utente_id/id, bind the integer parameter(s) (bookId or userId) using mysqli_stmt::bind_param, execute and close the statement for each query (both DELETEs in each function) to mirror the existing prepared-statement pattern used elsewhere.
🤖 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/PluginController.php`:
- Around line 372-384: The current loop in PluginController ignores the return
value of pluginManager->setSetting for both the $boolKeys and
$normalizedDomains, always returning success:true; update the logic to check
each pluginManager->setSetting($pluginId, $key, $value, true) and
pluginManager->setSetting($pluginId, $domainKey, $domain, true) call, collect
any failures (or short-circuit on first failure), and return success:false with
an appropriate message and the failing key(s) instead of emitting success:true
when any write fails; ensure the response->getBody()->write payload reflects
failure and includes which setting(s) failed (use $pluginId, $boolKeys,
$normalizedDomains, and the pluginManager->setSetting return values to build the
error info).
In `@app/Controllers/ProfileController.php`:
- Around line 180-192: The success flash ($_SESSION['success_message']) is set
before the locale change, so after a language update the confirmation shows in
the old language; in ProfileController, after you call $stmt->execute() move the
assignment of $_SESSION['success_message'] to occur after the locale update
block (the logic that checks $localeProvided and calls
\App\Support\I18n::setLocale and updates/unsets $_SESSION['locale']) so the
message is generated with the new locale; keep the session name update
($_SESSION['user']['name']) where appropriate or also move it together if you
want all session changes applied after the locale switch.
In `@storage/plugins/goodlib/plugin.json`:
- Around line 11-12: The manifest declares requires_app and max_app_version but
the system doesn't enforce them; update the plugin registration path in
PluginManager (e.g., registerBundledPlugin/registerPlugin methods) to parse and
validate the plugin.json fields requires_app and max_app_version against the
running app version (using semver range checks) and reject or skip registration
when the app version is out of range, logging a clear message; alternatively, if
you prefer not to add enforcement, remove the requires_app and max_app_version
keys from plugin.json so the manifest does not advertise unsupported bounds.
In `@storage/plugins/goodlib/views/settings.php`:
- Around line 92-101: The HTML input for the Anna domain (input id/name
"anna_domain" in settings.php) uses a restrictive pattern that only allows bare
host[:port], but the backend normalizers accept full URLs (with scheme, path,
query) and strip them; update the input's validation to match the backend by
removing or relaxing the pattern attribute (or replace it with a pattern that
allows an optional scheme (http|https):// and optional path/query, and optional
port) so strings like "https://annas-archive.custom.test/path?q=ignored" are
accepted; apply the same change to the sibling input(s) around lines 110-119
that use the same pattern.
---
Outside diff comments:
In `@app/Controllers/AuthController.php`:
- Around line 51-56: Wrap the database query (the sequence using
$db->prepare(...), $stmt->bind_param('s', $email), $stmt->execute(),
$stmt->get_result() and $res->fetch_assoc(), and $stmt->close()) in a try-catch
that catches \Throwable; on any caught exception close/free the statement if set
and return the controlled authentication error response used by this controller
(the same path used for invalid credentials) instead of letting the exception
bubble up. Ensure the catch block prevents leaking exception details and maps
failures during prepare/bind/execute/get_result to the normal auth failure
response.
In `@app/Controllers/LibraryThingImportController.php`:
- Around line 1198-1227: The upsertBook flow clears conflicting isbn13/isbn10
values but omits ean, which can trigger unique-index violations; update
upsertBook to mirror the isbn handling by nullifying conflicting ean values on
other live rows before writing (in the existingBookId != null branch do an
UPDATE libri SET ean = NULL WHERE ean = ? AND id != ? AND deleted_at IS NULL and
log affected rows) and also add the same pre-write ean conflict-clear step in
the INSERT path (when findExistingBook returns null) using UPDATE libri SET ean
= NULL WHERE ean = ? AND deleted_at IS NULL so ean uniqueness won't block the
insert; touch the same helper flow that precedes calling updateBook/insert logic
and reference $data['ean'], upsertBook, findExistingBook, and updateBook when
making the changes.
---
Duplicate comments:
In `@app/Controllers/ProfileController.php`:
- Around line 20-24: The prepare() call in ProfileController::show (the $stmt =
$db->prepare(...) line) can return false and must be checked before calling
$stmt->bind_param, otherwise a fatal error occurs; update show() to verify $stmt
!== false, and if prepare failed log the DB error (or $db->error), return/throw
a handled server error response (e.g. render an error page or send a 500) and
avoid calling bind_param/execute/close; keep existing logic for successful
prepares (bind_param, execute, fetch_assoc, close) unchanged.
In `@app/Views/admin/plugins.php`:
- Around line 258-264: The modal seed uses the old string flag; update the data
attribute to read the persisted boolean field instead: replace uses of
($z39Settings['enable_server'] ?? '0') === '1' with checking
($z39Settings['server_enabled'] ?? false) (or equivalent truthy check) when
setting data-enable-server on the button in admin/plugins.php so the UI reflects
the true persisted state (also verify data-enable-client remains correct and
that PluginController::updateSettings() persists server_enabled).
- Around line 215-221: Replace the hardcoded settings URL in the
data-settings-url attribute with a route helper call (e.g.,
RouteTranslator::route(...) or route_path(...)) so the plugin settings URL is
generated from the named admin route rather than "/admin/plugins/{id}/settings";
update the same change for all other similar attributes in this file (the other
button blocks referenced) and ensure you pass the plugin id (from $plugin['id'])
as the route parameter so openPluginSettingsModal receives a correct,
translatable URL.
In `@storage/plugins/goodlib/GoodLibPlugin.php`:
- Around line 181-207: dbSetSetting currently returns void and ignores failures
from PluginManager::setSetting and from the direct SQL path; change dbSetSetting
to return bool (true on success, false on any failure), use the boolean return
value from \App\Support\PluginManager::setSetting($this->pluginId, $key, $value,
true) and immediately return that result, and in the fallback path check that
$this->db->prepare(...) returned a statement, that bind_param and execute
succeed (return false on any failure), and close the statement before returning
true; update callers like onActivate() and saveSettings() to check the new
boolean return and handle/report failures accordingly.
- Around line 390-391: The current selection for $isbn in GoodLibPlugin.php uses
the null coalescing operator on $bookData['isbn13'] which still selects an empty
string when isbn13 exists but is blank; change the logic that sets $isbn (the
line creating $isbn from $bookData) to prefer a non-empty trimmed isbn13 and
only fall back to isbn10 when isbn13 is empty or whitespace (e.g., compute
trimmed values and choose isbn13 if !== '' else isbn10) so precise ISBN searches
use isbn10 when isbn13 is blank.
In `@storage/plugins/goodlib/views/badges.php`:
- Line 18: The translated string output __("Cerca su:") in badges.php is
rendered raw; wrap the translation with an HTML-escaping helper (e.g., call
htmlspecialchars(__(...), ENT_QUOTES, 'UTF-8') or the framework's h() escape
helper) before echoing so the span renders escaped text and prevents injected
markup/scripts.
In `@tests/branch-fix-consistency.spec.js`:
- Around line 24-31: The code currently does JSON.parse(output) unguarded so a
parse error (e.g. PHP warning/HTML) loses the captured raw harness output; wrap
the JSON.parse(output) in a try/catch and on parse failure rethrow an Error (or
augment the thrown error) that includes the original raw output string (variable
output) so callers see the raw harness output alongside the parse error; locate
the JSON.parse call and the output variable in this file
(tests/branch-fix-consistency.spec.js) to implement the safe-parse and
preserve-output behavior.
In `@tests/goodlib-custom-domains.spec.js`:
- Around line 23-27: The test setup in test.beforeAll calls
getPluginIdByName('goodlib') into pluginId and proceeds without verifying it,
causing later failures when pluginId is 0; update test.beforeAll to check the
returned pluginId immediately (e.g., if falsy or 0) and fail fast by throwing or
calling test.skip with a clear message that GoodLib is not registered so
snapshotPluginSettings/restore and subsequent operations are not executed for an
invalid pluginId.
---
Nitpick comments:
In `@tests/helpers/branch-fix-harness.php`:
- Around line 58-68: The deleteBook and deleteUser functions currently build SQL
with string concatenation which, despite int type hints, is inconsistent with
the codebase; change both functions (deleteBook(mysqli $db, int $bookId) and
deleteUser(mysqli $db, int $userId)) to use prepared statements: prepare the
DELETE queries with placeholders for libro_id/id and utente_id/id, bind the
integer parameter(s) (bookId or userId) using mysqli_stmt::bind_param, execute
and close the statement for each query (both DELETEs in each function) to mirror
the existing prepared-statement pattern used elsewhere.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1bcd8b23-ad05-4b98-9422-515615d88dbe
📒 Files selected for processing (30)
.gitignoreapp/Controllers/AuthController.phpapp/Controllers/CollaneController.phpapp/Controllers/FrontendController.phpapp/Controllers/LibraryThingImportController.phpapp/Controllers/LibriApiController.phpapp/Controllers/LibriController.phpapp/Controllers/PluginController.phpapp/Controllers/ProfileController.phpapp/Controllers/PublicApiController.phpapp/Middleware/RememberMeMiddleware.phpapp/Support/PluginManager.phpapp/Support/Updater.phpapp/Views/admin/plugins.phpapp/Views/frontend/book-detail.phpapp/Views/libri/scheda_libro.phpapp/Views/profile/index.phplocale/de_DE.jsonlocale/en_US.jsonlocale/it_IT.jsonstorage/plugins/goodlib/GoodLibPlugin.phpstorage/plugins/goodlib/plugin.jsonstorage/plugins/goodlib/views/badges.phpstorage/plugins/goodlib/views/settings.phpstorage/plugins/goodlib/wrapper.phptests/branch-fix-consistency.spec.jstests/full-test.spec.jstests/goodlib-custom-domains.spec.jstests/helpers/branch-fix-harness.phptests/helpers/e2e-fixtures.js
Comment on lines
+372
to
+384
| foreach ($boolKeys as $key) { | ||
| $value = isset($settings[$key]) && $settings[$key] === '1' ? '1' : '0'; | ||
| $this->pluginManager->setSetting($pluginId, $key, $value, true); | ||
| } | ||
| foreach ($normalizedDomains as $domainKey => $domain) { | ||
| $this->pluginManager->setSetting($pluginId, $domainKey, $domain, true); | ||
| } | ||
|
|
||
| $response->getBody()->write(json_encode([ | ||
| 'success' => true, | ||
| 'message' => __('Impostazioni GoodLib salvate correttamente.'), | ||
| 'data' => $normalizedDomains, | ||
| ])); |
There was a problem hiding this comment.
Don't return success when a GoodLib write fails.
This branch ignores every PluginManager::setSetting() result and still emits success: true. If any write fails, the config can be partially updated while the modal says everything was saved.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Controllers/PluginController.php` around lines 372 - 384, The current
loop in PluginController ignores the return value of pluginManager->setSetting
for both the $boolKeys and $normalizedDomains, always returning success:true;
update the logic to check each pluginManager->setSetting($pluginId, $key,
$value, true) and pluginManager->setSetting($pluginId, $domainKey, $domain,
true) call, collect any failures (or short-circuit on first failure), and return
success:false with an appropriate message and the failing key(s) instead of
emitting success:true when any write fails; ensure the
response->getBody()->write payload reflects failure and includes which
setting(s) failed (use $pluginId, $boolKeys, $normalizedDomains, and the
pluginManager->setSetting return values to build the error info).
Comment on lines
+11
to
+12
| "requires_app": "0.4.0", | ||
| "max_app_version": "1.0.0", |
There was a problem hiding this comment.
requires_app / max_app_version are declarative only right now.
Bundled-plugin registration currently stores these fields but does not enforce them, so GoodLib will still auto-register outside this declared range. Either add compatibility checks in PluginManager or drop the unsupported bounds from this manifest for now.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@storage/plugins/goodlib/plugin.json` around lines 11 - 12, The manifest
declares requires_app and max_app_version but the system doesn't enforce them;
update the plugin registration path in PluginManager (e.g.,
registerBundledPlugin/registerPlugin methods) to parse and validate the
plugin.json fields requires_app and max_app_version against the running app
version (using semver range checks) and reject or skip registration when the app
version is out of range, logging a clear message; alternatively, if you prefer
not to add enforcement, remove the requires_app and max_app_version keys from
plugin.json so the manifest does not advertise unsupported bounds.
- ProfileController::show(): prepare() guard before bind_param
- ProfileController: flash message after locale switch (correct language)
- badges.php: escape __("Cerca su:") with htmlspecialchars
- Z39 plugins.php: read server_enabled (not enable_server) for modal state
- GoodLib ISBN: prefer non-empty isbn13 over isbn10 (?: not ??)
- LT upsertBook: clear EAN conflicts before insert/update (like isbn13/isbn10)
- GoodLib dbSetSetting: return bool, propagate failures in saveSettings
- branch-fix-consistency: safe JSON.parse with raw output on failure
- settings.php: remove restrictive pattern on domain inputs (backend normalizes)
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 (2)
app/Controllers/AuthController.php (1)
51-56:⚠️ Potential issue | 🟠 MajorGuard
prepare()failure before callingbind_param()(Line 51).If
mysqli::prepare()returnsfalse, the login flow fails fatally onbind_param(). Other controllers in the codebase guard this condition; implement the same pattern here.Proposed fix
- $stmt = $db->prepare("SELECT id, email, password, tipo_utente, email_verificata, stato, nome, cognome, locale FROM utenti WHERE LOWER(email) = LOWER(?) LIMIT 1"); - $stmt->bind_param('s', $email); + $stmt = $db->prepare("SELECT id, email, password, tipo_utente, email_verificata, stato, nome, cognome, locale FROM utenti WHERE LOWER(email) = LOWER(?) LIMIT 1"); + if ($stmt === false) { + Log::security('login.db_prepare_failed', [ + 'ip' => $_SERVER['REMOTE_ADDR'] ?? 'unknown', + 'db_error' => $db->error, + ]); + return $response->withHeader('Location', RouteTranslator::route('login') . '?error=server')->withStatus(302); + } + $stmt->bind_param('s', $email); $stmt->execute();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/AuthController.php` around lines 51 - 56, The code calls $db->prepare(...) and immediately uses $stmt->bind_param(...), but $db->prepare can return false causing a fatal error; update the AuthController login flow to check the result of $db->prepare (the $stmt variable) before calling $stmt->bind_param, mirroring other controllers: if $stmt === false, log or handle the DB error (using existing logger or return an appropriate response), and only proceed to bind_param, execute, get_result and fetch_assoc when $stmt is a valid statement; ensure $stmt->close() is only called when $stmt is a valid object.app/Controllers/LibraryThingImportController.php (1)
1267-1284:⚠️ Potential issue | 🟠 MajorAdd error handling to all mysqli
prepare()andexecute()calls in these write paths.The code calls
prepare(),bind_param(), andexecute()without checking return values. Ifprepare()fails with strict reporting off, callingbind_param()onfalsewill fatal. Ifexecute()fails silently, code continues using unreliableaffected_rowsorinsert_id, creating orphaned or invalid records.The pattern appears throughout
updateBook(),insertBook(), and the conflict-clearing operations inupsertBook(). Only one check exists (line 1395:if ($checkStmt)), proving the risk was recognized but not applied consistently.🛠️ Minimal hardening pattern
$stmt = $db->prepare("UPDATE libri SET ..."); +if (!$stmt) { + throw new \RuntimeException('Failed to prepare statement: ' . $db->error); +} $stmt->bind_param(...); -$stmt->execute(); +if (!$stmt->execute()) { + $error = $stmt->error; + $stmt->close(); + throw new \RuntimeException('Failed to execute: ' . $error); +} $stmt->close();Applies to: 1210–1249 (conflict clearing), 1267–1284, 1348–1356, 1391–1393, 1432–1461, 1526–1535, 1572–1573.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibraryThingImportController.php` around lines 1267 - 1284, The prepare()/bind_param()/execute() calls in updateBook(), insertBook(), and upsertBook() (e.g. the $stmt prepared in the UPDATE block and the conflict-clearing statements) lack error handling: check that $db->prepare(...) returns a valid statement before calling bind_param(), and verify that ->execute() returns true; on failure log or throw with $db->error and/or $stmt->error and abort the write path (return false or propagate exception) to avoid calling methods on false or proceeding with bad affected_rows/insert_id; apply this pattern to every mysqli prepare/execute use in the listed regions (conflict clearing, the UPDATE stmt around $stmt, and the other insert/update statements).
♻️ Duplicate comments (3)
app/Controllers/PluginController.php (1)
372-378:⚠️ Potential issue | 🟠 MajorDo not report success when a GoodLib setting write fails.
Line 374 and Line 377 ignore
setSetting()outcomes, so the response at Line 381 can returnsuccess: trueafter partial write failures.🛠️ Suggested fix
// All validated — now persist + $failedKeys = []; foreach ($boolKeys as $key) { $value = isset($settings[$key]) && $settings[$key] === '1' ? '1' : '0'; - $this->pluginManager->setSetting($pluginId, $key, $value, true); + if (!$this->pluginManager->setSetting($pluginId, $key, $value, true)) { + $failedKeys[] = $key; + } } foreach ($normalizedDomains as $domainKey => $domain) { - $this->pluginManager->setSetting($pluginId, $domainKey, $domain, true); + if (!$this->pluginManager->setSetting($pluginId, $domainKey, $domain, true)) { + $failedKeys[] = $domainKey; + } + } + + if ($failedKeys !== []) { + $response->getBody()->write(json_encode([ + 'success' => false, + 'message' => __('Errore durante il salvataggio delle impostazioni.'), + 'failed_keys' => $failedKeys, + ])); + return $response->withHeader('Content-Type', 'application/json')->withStatus(500); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/PluginController.php` around lines 372 - 378, The success response is returned even if some writes via $this->pluginManager->setSetting(...) fail; update the loops in PluginController so you capture and check each setSetting() boolean return (both in the boolKeys loop and normalizedDomains loop), accumulate an overall $allSaved flag (start true, set false on any failure), and use that flag when building the response instead of unconditionally returning success; also consider short-circuiting or collecting failed keys for clearer error messages.tests/goodlib-custom-domains.spec.js (1)
23-27:⚠️ Potential issue | 🟡 MinorFail fast when GoodLib plugin ID is not resolved.
Line 24 feeds directly into Line 25 without validation; if the plugin is missing, failures surface later with less-clear diagnostics.
💡 Suggested hardening
test.beforeAll(() => { pluginId = getPluginIdByName('goodlib'); + if (!pluginId) { + throw new Error('GoodLib plugin ID not found; cannot run custom-domain E2E test.'); + } originalSettings = snapshotPluginSettings(pluginId); adminUser = createTempAdminUser('it_IT'); book = createTempBook(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/goodlib-custom-domains.spec.js` around lines 23 - 27, After calling getPluginIdByName('goodlib') in the test.beforeAll block, immediately validate the returned pluginId and fail fast if it's missing: check the pluginId variable (from pluginId = getPluginIdByName('goodlib')) and throw a clear error or call the test framework's fail/assert before calling snapshotPluginSettings(pluginId), createTempAdminUser, or createTempBook so missing plugin resolution surfaces with a precise diagnostic.app/Views/admin/plugins.php (1)
220-220:⚠️ Potential issue | 🟠 MajorUse named route helpers for these modal endpoints.
These
data-settings-urlvalues still hardcode/admin/...path segments viaurl(). If admin routes are translated or renamed, the modals will post to dead URLs even though the page itself still renders. Resolve each settings endpoint from a named route instead of embedding literal path strings in the view.As per coding guidelines,
Never use hardcoded routes. Use route_path('key') or RouteTranslator::route('key') instead of hardcoded paths like /accedi or /login.Also applies to: 231-231, 244-244, 262-262, 286-286
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/admin/plugins.php` at line 220, Replace the hardcoded admin plugin settings URLs in the data-settings-url attributes with a named-route resolver: instead of building "/admin/plugins/{id}/settings" with url(), call the route helper (e.g. route_path('admin.plugins.settings', ['id' => (int)$plugin['id']]) or RouteTranslator::route('admin.plugins.settings', ['id' => (int)$plugin['id']])) and keep the htmlspecialchars encoding around the result; apply the same change to the other instances referenced (lines where data-settings-url is set for the same plugin id).
🧹 Nitpick comments (5)
app/Support/Updater.php (1)
39-46: Consider a single source of truth for bundled plugins.
BUNDLED_PLUGINSis now maintained in multiple classes (App\Support\UpdaterandApp\Support\PluginManager). Centralizing it (shared config/constant) will reduce future drift risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Support/Updater.php` around lines 39 - 46, The BUNDLED_PLUGINS constant is duplicated between App\Support\Updater and App\Support\PluginManager; create a single source of truth (either a config entry like config/bundled_plugins.php or a dedicated class constant, e.g. App\Support\BundledPlugins::LIST) and replace the local BUNDLED_PLUGINS definitions with references to that new source from both Updater and PluginManager; update all usages to read from the shared constant/method (e.g., BundledPlugins::LIST or config('bundled_plugins')) and remove the old duplicated constant declarations, then run tests to ensure no references remain to the removed constant.app/Controllers/FrontendController.php (1)
22-34: Consider: Different caching strategies exist between controllers.
FrontendControlleruses static variable caching withSHOW COLUMNS, whileLibriApiController(per relevant snippets at lines 664-702) uses instance-level caching withINFORMATION_SCHEMA.COLUMNS. Both approaches work correctly, but the inconsistency could lead to subtle differences:
- Static cache persists across requests in long-running processes (e.g., PHP-FPM workers)
- Instance cache is isolated per-request
This isn't a defect—both detect the column reliably—but unifying the approach could improve maintainability if schema detection is needed more broadly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/FrontendController.php` around lines 22 - 34, The code in FrontendController::descriptionExpr uses a static cache (self::$hasDescrizionePlain) with SHOW COLUMNS, which is inconsistent with LibriApiController's instance-level cache using INFORMATION_SCHEMA.COLUMNS; change FrontendController to use the same instance-level caching strategy as LibriApiController: replace the static self::$hasDescrizionePlain with an instance property (e.g., $this->hasDescrizionePlain), switch the column check to the same INFORMATION_SCHEMA.COLUMNS query or call the shared helper used by LibriApiController, and ensure descriptionExpr (or a refactored helper method) uses the instance property so schema detection is per-request and consistent across controllers.app/Controllers/PluginController.php (1)
399-428: Avoid duplicated domain-normalization logic across controller and plugin.
PluginController::normalizeGoodLibDomain()mirrorsstorage/plugins/goodlib/GoodLibPlugin.php::normalizeDomain(). Consider extracting a shared normalizer to prevent future drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/PluginController.php` around lines 399 - 428, The domain-normalization logic in PluginController::normalizeGoodLibDomain duplicates storage/plugins/goodlib/GoodLibPlugin::normalizeDomain; extract the shared logic into a single utility (e.g., a static method normalize in a new DomainNormalizer or GoodLibDomainNormalizer class) and have both PluginController::normalizeGoodLibDomain and GoodLibPlugin::normalizeDomain delegate to that new shared method; ensure the new normalizer preserves current behavior (trim, default handling, scheme prefix, parse_url host/port validation and regex) and update call sites to use the shared utility to avoid duplication and drift.tests/helpers/branch-fix-harness.php (1)
486-522: Consider removing unused$dbparameter for clarity.The
$dbparameter is unused inscenarioLibraryThingExportTranslatorRoundtrip()andscenarioLibraryThingSecondaryAuthorRolePairing()(line 524) since these scenarios only test parsing/formatting logic via reflection without database operations.However, keeping the parameter maintains a uniform signature across all scenarios, simplifying the dispatcher. This is acceptable for test harness code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/helpers/branch-fix-harness.php` around lines 486 - 522, The mysqli $db parameter is unused in scenarioLibraryThingExportTranslatorRoundtrip and scenarioLibraryThingSecondaryAuthorRolePairing; remove the unused parameter from both function signatures (delete "mysqli $db"), update any callers/dispatcher to stop passing the DB argument (or adjust the dispatcher to call these scenarios without an argument), and leave the reflection/format/parse logic inside scenarioLibraryThingExportTranslatorRoundtrip unchanged (keep references to getLibraryThingMethods, formatLibraryThingRow, getLibraryThingHeaders, and the parsing assertions intact).app/Controllers/LibriApiController.php (1)
45-47: ReusehasColumn()cache forlibri.descrizione_plaindetection.This works functionally, but using
hasColumn($db, 'descrizione_plain')here would avoid an extra schema lookup path for the same table.♻️ Suggested simplification
- $descExpr = $this->hasTableColumn($db, 'libri', 'descrizione_plain') + $descExpr = $this->hasColumn($db, 'descrizione_plain') ? "COALESCE(NULLIF(l.descrizione_plain, ''), l.descrizione)" : 'l.descrizione';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibriApiController.php` around lines 45 - 47, The libri.descrizione_plain existence check currently calls hasTableColumn($db, 'libri', 'descrizione_plain'), causing an extra schema lookup; replace that call with the cached check hasColumn($db, 'descrizione_plain') where $descExpr is set (the assignment using COALESCE/NULLIF) so the controller reuses the cached column result; ensure you remove the hasTableColumn(...) invocation in this expression and use hasColumn(...) consistently in LibriApiController for detecting descrizione_plain.
🤖 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/LibriController.php`:
- Line 11: The global exception handler currently logs every exception at error
level before checking for HttpNotFoundException, which turns normal 404s into
noisy error-level stack traces; update the handler in public/index.php to first
detect instances of Slim\Exception\HttpNotFoundException (or check $exception
instanceof HttpNotFoundException) and either handle them without calling
SecureLogger::error() or log them at a lower level, and replace any raw
error_log() calls with SecureLogger::error() for sensitive contexts; also apply
the same change to the other handler locations referenced (around lines noted
for LibriController.php and the other occurrences) so 404s are classified before
error-level logging.
In `@app/Controllers/ProfileController.php`:
- Around line 191-201: The runtime locale isn't reset when the user clears their
preference, so the success flash is still translated with the old locale; in the
else branch of the locale handling in ProfileController (where you currently
unset $_SESSION['locale']) call the I18n runtime reset (e.g.
\App\Support\I18n::setLocale(null) or the library's
resetLocale()/setDefaultLocale() method) before unsetting the session key so the
rest of the current request uses the default locale, and keep the
$_SESSION['success_message'] assignment after that change.
In `@tests/branch-fix-consistency.spec.js`:
- Around line 38-124: This test suite ("Branch Fix Consistency" using
runScenario) mutates shared DB state and must run serially; update your
Playwright configuration to set workers: 1 (add workers: 1 to the exported
config object in your Playwright config) or, alternatively, change the test npm
invocation to pass --workers=1 (add --workers=1 to the Playwright test script in
package.json) so all tests using runScenario execute with a single worker.
In `@tests/helpers/e2e-fixtures.js`:
- Around line 198-207: The teardown helpers deleteTempAdminUser and
deleteTempBook miss non-cascading dependents (prenotazioni, prestiti), causing
FK failures and leaked state; update both functions to first delete dependent
rows in prenotazioni and prestiti that reference the target (for
deleteTempAdminUser delete from prenotazioni and prestiti where utente_id =
Number(userId) before deleting from user_sessions and utenti; for deleteTempBook
delete from prenotazioni and prestiti where libro_id = Number(bookId), and also
delete prestiti/prenotazioni that reference copie if applicable, before deleting
from copie and libri), using the existing dbExec calls and keeping the
Number(...) casting to avoid SQL errors.
---
Outside diff comments:
In `@app/Controllers/AuthController.php`:
- Around line 51-56: The code calls $db->prepare(...) and immediately uses
$stmt->bind_param(...), but $db->prepare can return false causing a fatal error;
update the AuthController login flow to check the result of $db->prepare (the
$stmt variable) before calling $stmt->bind_param, mirroring other controllers:
if $stmt === false, log or handle the DB error (using existing logger or return
an appropriate response), and only proceed to bind_param, execute, get_result
and fetch_assoc when $stmt is a valid statement; ensure $stmt->close() is only
called when $stmt is a valid object.
In `@app/Controllers/LibraryThingImportController.php`:
- Around line 1267-1284: The prepare()/bind_param()/execute() calls in
updateBook(), insertBook(), and upsertBook() (e.g. the $stmt prepared in the
UPDATE block and the conflict-clearing statements) lack error handling: check
that $db->prepare(...) returns a valid statement before calling bind_param(),
and verify that ->execute() returns true; on failure log or throw with
$db->error and/or $stmt->error and abort the write path (return false or
propagate exception) to avoid calling methods on false or proceeding with bad
affected_rows/insert_id; apply this pattern to every mysqli prepare/execute use
in the listed regions (conflict clearing, the UPDATE stmt around $stmt, and the
other insert/update statements).
---
Duplicate comments:
In `@app/Controllers/PluginController.php`:
- Around line 372-378: The success response is returned even if some writes via
$this->pluginManager->setSetting(...) fail; update the loops in PluginController
so you capture and check each setSetting() boolean return (both in the boolKeys
loop and normalizedDomains loop), accumulate an overall $allSaved flag (start
true, set false on any failure), and use that flag when building the response
instead of unconditionally returning success; also consider short-circuiting or
collecting failed keys for clearer error messages.
In `@app/Views/admin/plugins.php`:
- Line 220: Replace the hardcoded admin plugin settings URLs in the
data-settings-url attributes with a named-route resolver: instead of building
"/admin/plugins/{id}/settings" with url(), call the route helper (e.g.
route_path('admin.plugins.settings', ['id' => (int)$plugin['id']]) or
RouteTranslator::route('admin.plugins.settings', ['id' => (int)$plugin['id']]))
and keep the htmlspecialchars encoding around the result; apply the same change
to the other instances referenced (lines where data-settings-url is set for the
same plugin id).
In `@tests/goodlib-custom-domains.spec.js`:
- Around line 23-27: After calling getPluginIdByName('goodlib') in the
test.beforeAll block, immediately validate the returned pluginId and fail fast
if it's missing: check the pluginId variable (from pluginId =
getPluginIdByName('goodlib')) and throw a clear error or call the test
framework's fail/assert before calling snapshotPluginSettings(pluginId),
createTempAdminUser, or createTempBook so missing plugin resolution surfaces
with a precise diagnostic.
---
Nitpick comments:
In `@app/Controllers/FrontendController.php`:
- Around line 22-34: The code in FrontendController::descriptionExpr uses a
static cache (self::$hasDescrizionePlain) with SHOW COLUMNS, which is
inconsistent with LibriApiController's instance-level cache using
INFORMATION_SCHEMA.COLUMNS; change FrontendController to use the same
instance-level caching strategy as LibriApiController: replace the static
self::$hasDescrizionePlain with an instance property (e.g.,
$this->hasDescrizionePlain), switch the column check to the same
INFORMATION_SCHEMA.COLUMNS query or call the shared helper used by
LibriApiController, and ensure descriptionExpr (or a refactored helper method)
uses the instance property so schema detection is per-request and consistent
across controllers.
In `@app/Controllers/LibriApiController.php`:
- Around line 45-47: The libri.descrizione_plain existence check currently calls
hasTableColumn($db, 'libri', 'descrizione_plain'), causing an extra schema
lookup; replace that call with the cached check hasColumn($db,
'descrizione_plain') where $descExpr is set (the assignment using
COALESCE/NULLIF) so the controller reuses the cached column result; ensure you
remove the hasTableColumn(...) invocation in this expression and use
hasColumn(...) consistently in LibriApiController for detecting
descrizione_plain.
In `@app/Controllers/PluginController.php`:
- Around line 399-428: The domain-normalization logic in
PluginController::normalizeGoodLibDomain duplicates
storage/plugins/goodlib/GoodLibPlugin::normalizeDomain; extract the shared logic
into a single utility (e.g., a static method normalize in a new DomainNormalizer
or GoodLibDomainNormalizer class) and have both
PluginController::normalizeGoodLibDomain and GoodLibPlugin::normalizeDomain
delegate to that new shared method; ensure the new normalizer preserves current
behavior (trim, default handling, scheme prefix, parse_url host/port validation
and regex) and update call sites to use the shared utility to avoid duplication
and drift.
In `@app/Support/Updater.php`:
- Around line 39-46: The BUNDLED_PLUGINS constant is duplicated between
App\Support\Updater and App\Support\PluginManager; create a single source of
truth (either a config entry like config/bundled_plugins.php or a dedicated
class constant, e.g. App\Support\BundledPlugins::LIST) and replace the local
BUNDLED_PLUGINS definitions with references to that new source from both Updater
and PluginManager; update all usages to read from the shared constant/method
(e.g., BundledPlugins::LIST or config('bundled_plugins')) and remove the old
duplicated constant declarations, then run tests to ensure no references remain
to the removed constant.
In `@tests/helpers/branch-fix-harness.php`:
- Around line 486-522: The mysqli $db parameter is unused in
scenarioLibraryThingExportTranslatorRoundtrip and
scenarioLibraryThingSecondaryAuthorRolePairing; remove the unused parameter from
both function signatures (delete "mysqli $db"), update any callers/dispatcher to
stop passing the DB argument (or adjust the dispatcher to call these scenarios
without an argument), and leave the reflection/format/parse logic inside
scenarioLibraryThingExportTranslatorRoundtrip unchanged (keep references to
getLibraryThingMethods, formatLibraryThingRow, getLibraryThingHeaders, and the
parsing assertions intact).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 688070b2-d4c9-4191-90ea-b3f3846755e2
📒 Files selected for processing (30)
.gitignoreapp/Controllers/AuthController.phpapp/Controllers/CollaneController.phpapp/Controllers/FrontendController.phpapp/Controllers/LibraryThingImportController.phpapp/Controllers/LibriApiController.phpapp/Controllers/LibriController.phpapp/Controllers/PluginController.phpapp/Controllers/ProfileController.phpapp/Controllers/PublicApiController.phpapp/Middleware/RememberMeMiddleware.phpapp/Support/PluginManager.phpapp/Support/Updater.phpapp/Views/admin/plugins.phpapp/Views/frontend/book-detail.phpapp/Views/libri/scheda_libro.phpapp/Views/profile/index.phplocale/de_DE.jsonlocale/en_US.jsonlocale/it_IT.jsonstorage/plugins/goodlib/GoodLibPlugin.phpstorage/plugins/goodlib/plugin.jsonstorage/plugins/goodlib/views/badges.phpstorage/plugins/goodlib/views/settings.phpstorage/plugins/goodlib/wrapper.phptests/branch-fix-consistency.spec.jstests/full-test.spec.jstests/goodlib-custom-domains.spec.jstests/helpers/branch-fix-harness.phptests/helpers/e2e-fixtures.js
| use Psr\Http\Message\ServerRequestInterface as Request; | ||
| use App\Support\SecureLogger; | ||
| use App\Support\LibraryThingInstaller; | ||
| use Slim\Exception\HttpNotFoundException; |
There was a problem hiding this comment.
Avoid turning expected 404s into error-level stack traces.
These HttpNotFoundExceptions now send missing-book cases through the global handler in public/index.php, and that handler currently logs every exception before it checks whether the exception is a 404. That will make normal not-found requests show up as noisy error logs with stack traces. Please classify HttpNotFoundException first there, or skip error-level logging for 404s altogether.
As per coding guidelines, "Use SecureLogger::error() not error_log() for sensitive contexts".
Also applies to: 473-475, 1139-1140, 1174-1175, 2399-2400
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Controllers/LibriController.php` at line 11, The global exception handler
currently logs every exception at error level before checking for
HttpNotFoundException, which turns normal 404s into noisy error-level stack
traces; update the handler in public/index.php to first detect instances of
Slim\Exception\HttpNotFoundException (or check $exception instanceof
HttpNotFoundException) and either handle them without calling
SecureLogger::error() or log them at a lower level, and replace any raw
error_log() calls with SecureLogger::error() for sensitive contexts; also apply
the same change to the other handler locations referenced (around lines noted
for LibriController.php and the other occurrences) so 404s are classified before
error-level logging.
Comment on lines
+38
to
+124
| test.describe('Branch Fix Consistency', () => { | ||
| test('collana rename rolls back when target already exists', () => { | ||
| const result = runScenario('collana-rename-rollback'); | ||
| expect(result.ok, JSON.stringify(result)).toBe(true); | ||
| expect(result.bookCollana).toBe(result.source); | ||
| expect(result.sourceExists).toBe(1); | ||
| expect(result.targetExists).toBe(1); | ||
| }); | ||
|
|
||
| test('librarything import normalizes and persists translator, series, descrizione_plain and issn', () => { | ||
| const result = runScenario('librarything-parse-and-persist'); | ||
| expect(result.ok, JSON.stringify(result)).toBe(true); | ||
| expect(result.parsed.autori).toBe('George Orwell'); | ||
| expect(result.parsed.traduttore).toBe('Barbara Pym'); | ||
| expect(result.parsed.descrizione_plain).toBe('Alpha & Omega'); | ||
| expect(result.parsed.collana).toBe('Branch Saga'); | ||
| expect(result.parsed.numero_serie).toBe('7'); | ||
| expect(result.parsed.issn).toBe('1234-567X'); | ||
| expect(result.created.issn).toBe('1234-567X'); | ||
| expect(result.updated.collana).toBe('Branch Saga Updated'); | ||
| expect(result.softDeleted.descrizione_plain).toBe('before soft delete'); | ||
| }); | ||
|
|
||
| test('librarything export keeps translator roundtrip-compatible', () => { | ||
| const result = runScenario('librarything-export-translator-roundtrip'); | ||
| expect(result.ok, JSON.stringify(result)).toBe(true); | ||
| expect(result.row.secondaryAuthor).toBe('Barbara Pym'); | ||
| expect(result.row.secondaryAuthorRoles).toBe('Translator'); | ||
| expect(result.parsed.autori).toBe('George Orwell'); | ||
| expect(result.parsed.traduttore).toBe('Barbara Pym'); | ||
| }); | ||
|
|
||
| test('librarything keeps secondary author roles paired during roundtrip', () => { | ||
| const result = runScenario('librarything-secondary-author-role-pairing'); | ||
| expect(result.ok, JSON.stringify(result)).toBe(true); | ||
| expect(result.row.secondaryAuthor).toBe('Jane Austen; Barbara Pym'); | ||
| expect(result.row.secondaryAuthorRoles).toBe('; Translator'); | ||
| expect(result.parsed.autori).toBe('George Orwell|Jane Austen'); | ||
| expect(result.parsed.traduttore).toBe('Barbara Pym'); | ||
| }); | ||
|
|
||
| test('admin and frontend search use descrizione_plain', () => { | ||
| const result = runScenario('descrizione-plain-search'); | ||
| expect(result.ok, JSON.stringify(result)).toBe(true); | ||
| expect(result.apiTitles).toContain(result.title); | ||
| expect(result.catalogFound).toBe(true); | ||
| }); | ||
|
|
||
| test('public api and frontend detail expose issn with schema identifier', () => { | ||
| const result = runScenario('public-api-and-frontend-issn'); | ||
| expect(result.ok, JSON.stringify(result)).toBe(true); | ||
| expect(result.apiIssn).toBe('1234-567X'); | ||
| expect(result.detailHasIssn).toBe(true); | ||
| expect(result.detailHasSchemaIdentifier).toBe(true); | ||
| }); | ||
|
|
||
| test('login applies user locale immediately', () => { | ||
| const result = runScenario('auth-login-loads-locale'); | ||
| expect(result.ok, JSON.stringify(result)).toBe(true); | ||
| expect(result.location).toBe('/admin/dashboard'); | ||
| expect(result.sessionLocale).toBe('en_US'); | ||
| expect(result.currentLocale).toBe('en_US'); | ||
| }); | ||
|
|
||
| test('profile locale update persists and updates session locale', () => { | ||
| const result = runScenario('profile-locale-update'); | ||
| expect(result.ok, JSON.stringify(result)).toBe(true); | ||
| expect(result.dbLocale).toBe('it_IT'); | ||
| expect(result.sessionLocale).toBe('it_IT'); | ||
| expect(result.currentLocale).toBe('it_IT'); | ||
| }); | ||
|
|
||
| test('profile update without locale keeps the stored locale intact', () => { | ||
| const result = runScenario('profile-locale-omitted-keeps-value'); | ||
| expect(result.ok, JSON.stringify(result)).toBe(true); | ||
| expect(result.dbLocale).toBe('en_US'); | ||
| expect(result.sessionLocale).toBe('en_US'); | ||
| expect(result.currentLocale).toBe('en_US'); | ||
| }); | ||
|
|
||
| test('remember-me middleware restores user locale on auto-login', () => { | ||
| const result = runScenario('remember-me-loads-locale'); | ||
| expect(result.ok, JSON.stringify(result)).toBe(true); | ||
| expect(result.sessionLocale).toBe('de_DE'); | ||
| expect(result.currentLocale).toBe('de_DE'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
# Expect to find a global worker constraint or an invocation that passes --workers=1.
fd 'playwright\.config'
rg -n -C2 'workers\s*:\s*1' .
rg -n -C2 -- '--workers=1' .Repository: fabiodalez-dev/Pinakes
Length of output: 92
🏁 Script executed:
cat -n tests/playwright.config.jsRepository: fabiodalez-dev/Pinakes
Length of output: 463
🏁 Script executed:
cat -n package.json | head -100Repository: fabiodalez-dev/Pinakes
Length of output: 1409
Configure Playwright to run tests serially with --workers=1.
The test suite mutates shared DB state through tests/helpers/branch-fix-harness.php. Without a worker constraint, Playwright will spawn multiple parallel workers (defaulting to CPU core count), causing race conditions when this suite runs alongside other E2E tests against the same database.
Add workers: 1 to tests/playwright.config.js or pass --workers=1 to the test invocation in package.json.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/branch-fix-consistency.spec.js` around lines 38 - 124, This test suite
("Branch Fix Consistency" using runScenario) mutates shared DB state and must
run serially; update your Playwright configuration to set workers: 1 (add
workers: 1 to the exported config object in your Playwright config) or,
alternatively, change the test npm invocation to pass --workers=1 (add
--workers=1 to the Playwright test script in package.json) so all tests using
runScenario execute with a single worker.
- AuthController: prepare() guard before bind_param in login - ProfileController: reset runtime locale to installation default on clear - PluginController: check setSetting return, report failure - plugins.php: Z39 read server_enabled (not enable_server) - e2e-fixtures: delete prenotazioni/prestiti in teardown (FK safety) - goodlib-custom-domains: explicit pluginId guard
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
Fixes 4 P2 consistency findings identified in a cross-version review (v0.4.9.9 → v0.5.2):
COALESCE(l.descrizione_plain, l.descrizione)for LIKE conditions. MATCH() unchanged (requires FULLTEXT index columns).issnproperty to Schema.org Book JSON-LD and to PublicApiController SELECT + response mapping.execute()result check withRuntimeExceptionon failure (was relying on MYSQLI_REPORT_STRICT catch only).descrizione_plain(strip_tags), ISSN normalization (XXXX-XXXX format), andAuthorNormalizeron traduttore to all 4 INSERT/UPDATE SQL variants.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests
Chores