feat(archives): phases 1b–5 — full plugin (#103)#105
Merged
fabiodalez-dev merged 49 commits intomainfrom Apr 22, 2026
Merged
Conversation
Initial scaffolding for Hans Uwe Petersen's request to extend Pinakes
beyond the flat bibliographic `libri` model to handle archival material
(fonds → series → files → items) and authority records for persons /
corporate bodies / families, per the ICA international standards
ISAD(G) and ISAAR(CPF).
Status: SKELETON — schema design + metadata only. No CRUD UI, no
MARCXML I/O, no hook wiring, no unified search integration yet. The
DDL is exposed as string constants so reviewers can audit schema
intent before anything touches a production DB.
Files:
- plugin.json — metadata, marked optional:true + status:skeleton
- wrapper.php — bootstrap, returns ArchivesPlugin instance
- ArchivesPlugin.php — level constants, authority types, plannedHooks
array, three DDL methods (archival_units, authority_records,
archival_unit_authority link table)
- README.md — architecture overview + ISAD(G) → column crosswalk
(mirrors ABA archive format from Copenhagen's labour-movement
archive, per Hans's attached PDFs)
Updates:
- BundledPlugins::LIST grows from 9 to 10 (inserted alphabetically)
- .gitignore whitelist for the new plugin directory
The data model mirrors the ABA mapping of ISAD(G)/ISAAR(CPF) onto an
extended danMARC2, so future MARCXML import/export is a mechanical
translation. Soft-delete (`deleted_at`) is aligned with Pinakes'
existing `libri` convention. FULLTEXT indexes on both tables pave the
way for Phase 3 unified search across libri + archival_units +
authority_records.
Roadmap (see README.md):
Phase 1 — MVP archives CRUD (2-3 weeks)
Phase 2 — Authority records CRUD (1-2 weeks)
Phase 3 — Unified search (1 week)
Phase 4 — MARCXML I/O (2 weeks)
Phase 5 — Photographic items (1-2 weeks)
No DDL is executed by this commit — activating the plugin calls
ensureSchema() which is still a stub. This is deliberate: the plugin
is bundled but optional:true and must be explicitly enabled by an
admin after a design-review pass on the schema.
) Phase 1a of the archives plugin (per the plan in the PR description). The plugin now: - Conforms to PluginManager's instantiation contract (new $className($db, $hookManager) — PluginManager.php:878). - Exposes a global `ArchivesPlugin` proxy in wrapper.php that forwards lifecycle hooks (onActivate/onDeactivate/onInstall/onUninstall) to the namespaced implementation. Mirrors the DiscogsPlugin pattern so both the admin UI install path and direct namespaced loading work. - Implements ensureSchema() against the real mysqli connection, wrapping each CREATE TABLE in try/catch and logging failures via SecureLogger instead of aborting activation. Returns ['created' => [...], 'failed' => [...]] so callers can tell the admin which tables are in place. - Adds getHookManager() as a testable accessor for the injected HookManager (phase 1b will iterate plannedHooks() through it). - Keeps onDeactivate as an explicit no-op — archival data survives deactivation; DROP is reserved for a future explicit "Uninstall archives" admin action with confirmation. Unit test: tests/archives-plugin.unit.php (28 assertions, pure PHP, no DB). Covers the DDL string shape (what will hit production when activated), plannedHooks source inclusion, and the DI contract via Reflection. End-to-end activation will land in the plugin-integrity E2E regression in phase 1b. Plan: phase 1a (THIS PR) — schema backbone + DI + unit tests phase 1b (follow-up) — ArchivesController + routes + views + sidebar phase 1c (follow-up) — i18n (IT/EN/DE) + Playwright E2E Version bumped to 0.1.0, status: phase-1a. Plugin stays optional:true so admins must explicitly enable it from /admin/plugins before any DDL hits their database.
28 regression assertions on the DDL emitted by ArchivesPlugin (the strings that will hit the production database when an admin activates the plugin) + reflection-based checks on the DI contract. The .gitignore whitelist mirrors the one added on feature/discogs-catno-support (PR #102) so future plugin unit tests can live alongside the Playwright *.spec.js files without requiring case-by-case exceptions. Run: php tests/archives-plugin.unit.php → Passed: 28 Failed: 0
Extends the Archives plugin (PR #104 phase 1a) with its first two endpoints, registered entirely from within the plugin via the core `app.routes.register` hook (pattern borrowed from DeweyEditorPlugin): GET /admin/archives — tree-flavoured list of archival_units GET /admin/archives/new — blank create form POST /admin/archives/new — validate + INSERT + 303 redirect Plugin changes - `setPluginId(int)` + `$pluginId` plumbing so the plugin can register/unregister its hooks in the `plugin_hooks` table. - `onActivate()` now also inserts the `app.routes.register` hook. - `onDeactivate()` removes plugin hooks — routes stop responding immediately on deactivation while data is preserved. - `registerRoutes($app)` attaches the three routes with AdminAuthMiddleware (all) + CsrfMiddleware (POST only). - `indexAction` / `newAction` / `storeAction` handlers — Slim PSR-7 signatures. - `validateArchivalUnit()` enforces ISAD(G) 3.1 minimums: reference_code required + ≤64 chars, level must be one of LEVELS, constructed_title required + ≤500 chars, date_start/end within SMALLINT range, end ≥ start, parent_id (if set) must reference a non-deleted row. - `renderView()` helper wraps plugin views with the core `app/Views/layout.php` so sidebar + header stay consistent. Views (new) - `views/index.php` — list with level-coloured badges and visual tree indent via parent_id recursion. Warns with a CTA when the table is empty. Page limit 500 (pagination → phase 1c). - `views/form.php` — all 10 ISAD(G) 3.1 core fields, inline labels cite the ISAD section + the matching ABA MARC field so archivists can cross-reference. Per-field error rendering, sticky values on validation failure, CSRF token wired via existing Csrf::ensureToken() helper. Out of scope for this PR (phase 1c follow-up) - show / edit / update / destroy actions - sidebar menu entry in app/Views/layout.php - i18n stringtable (IT/EN/DE) - Playwright E2E spec - pagination + filters on the index Version bumped to 0.2.0 (status: phase-1b). Plugin remains optional:true so activation is still explicit. Pre-commit: PHPStan level 5 clean on plugin + views; `php -l` clean; existing unit test (28 assertions) still passes.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 24 minutes and 33 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (14)
📒 Files selected for processing (40)
📝 WalkthroughWalkthroughAggiunta completa del plugin "archives": schema DB e migration; nuove view/admin (index, show, form, import, search, authorities); XSD MARC21; manifest/README aggiornati con SRU 1.2; aggiornato layout per hook admin menu; .gitignore aggiornato; ampie localizzazioni e 3 suite E2E Playwright. Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin UI
participant App as App Server
participant DB as Database
participant Hooks as Plugin Hooks
participant XSD as XSD Validator
Admin->>App: POST /admin/archives/new (MARC/ISAD form)
App->>DB: INSERT archival_units
DB-->>App: OK (id)
App-->>Admin: Redirect /admin/archives/{id}
Admin->>App: GET /admin/archives/{id}
App->>DB: SELECT archival_unit + linked authority rows
App->>Hooks: do('admin.menu.render')
DB-->>App: data
App-->>Admin: render show view (typeahead, attach forms)
Admin->>App: POST /admin/archives/{id}/authority (attach)
App->>DB: INSERT archival_unit_authority (role)
DB-->>App: OK
App-->>Admin: Redirect (detail shows link)
Admin->>App: POST /admin/archives/import (MARCXML)
App->>XSD: validate against MARC21slim.xsd (if strict_xsd)
XSD-->>App: validation result
App->>DB: (dry-run? no INSERT : INSERT/UPDATE archival_units & authority_records)
DB-->>App: result counts
App-->>Admin: render import result (parsed/inserted/updated/skipped)
Client->>App: GET /api/archives/sru?operation=searchRetrieve&query=...
App->>DB: MATCH...AGAINST / constrained CQL -> records
DB-->>App: matched records
App-->>Client: SRU XML response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minuti Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Closes the CRUD loop on archival_units. Four new endpoints:
GET /admin/archives/{id} — read-only detail view
GET /admin/archives/{id}/edit — pre-populated edit form
POST /admin/archives/{id}/edit — validate + UPDATE + 303
POST /admin/archives/{id}/delete — soft-delete (deleted_at = NOW())
Plugin changes
- showAction / editAction / updateAction / destroyAction handlers.
- `findById(int): ?array` — single-row fetch with `deleted_at IS NULL`
guard. Centralised so every detail-path gets soft-delete for free.
- `renderNotFound()` — 404 helper used by show/edit/update when
the id is missing or soft-deleted.
- `validateArchivalUnit()` gained an optional `?int $excludeId`
parameter. Currently unused at runtime — reference_code uniqueness
is enforced by the DB's `UNIQUE KEY uq_reference` + errno 1062
surfacing. The param signature lands now so phase 2 can add a
pre-check without breaking callers.
- updateAction also guards against self-parenting (parent_id === id).
Full cycle detection across descendants will arrive with the CTE
helpers in phase 2.
- destroyAction uses plain UPDATE to set `deleted_at = NOW()` — aligned
with the `libri` table soft-delete convention. Children are NOT
cascade-soft-deleted; deleting a fonds with active series below is
a destructive op that needs explicit UI confirmation (roadmap).
Views
- `form.php` now supports `$mode ∈ {create, edit}`. Action URL,
page title, submit label, and breadcrumb switch on the mode.
The create path still dispatches with `$mode` defaulted via the
null-coalesce fallback, so phase 1b behaviour is preserved.
- `show.php` (new) — compact <dl> card with level badge, reference
code, parent link (when set), date range, extent, scope/content,
language, created/updated timestamps. Edit + Delete buttons in
the header. Delete is wrapped in a confirm() to discourage slips.
- `index.php` — reference_code and title cells are now links to the
detail page; a new rightmost column exposes "modifica". The row
structure otherwise unchanged.
Out of scope (→ phase 1d follow-up)
- Sidebar menu entry in app/Views/layout.php (currently reachable
only by typing /admin/archives directly).
- i18n stringtable (IT/EN/DE) — view strings are still hardcoded IT.
- Playwright E2E spec covering list → create → edit → delete.
- Pagination + filters on the index.
Version bumped to 0.3.0 (status: phase-1c). PHPStan level 5 clean;
unit test (28 assertions) still passes.
fabiodalez-dev
changed the title
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
User-facing polish for the archives plugin. The plugin now integrates
with the admin shell via a new plugin hook, ships translations in all
three supported locales, and has end-to-end coverage for the full
CRUD loop.
Sidebar hook
- `app/Views/layout.php` gains a single `\App\Support\Hooks::do('admin.menu.render')`
call between the core "Collocazione" and "Utenti" entries. Any optional
plugin can now hook into this action and echo HTML conforming to the
existing admin-sidebar Tailwind pattern without touching core.
- `ArchivesPlugin::renderAdminMenuEntry()` registers on that hook during
onActivate() and emits an <a> matching the pattern already used for
Collane/Generi/Collocazione — FontAwesome icon `fa-archive`, title
"Archivi", subtitle "Materiale archivistico (ISAD(G))".
- Deactivation removes the hook row → the entry disappears immediately.
i18n
- 13 new string keys added to `locale/{it_IT,en_US,de_DE}.json`:
menu labels ("Archivi", "Reference", "Livello", "Titolo", "Date",
"Estensione", "Azioni"), subtitle, empty-state copy, CTA button,
and the recommended-hierarchy hint. English and German translations
are archival-domain-appropriate (DE uses "Signatur" for reference,
"Bestand" for fonds, "Einzelstück" for item).
- `views/index.php` user-visible strings converted from hardcoded IT
to `__()`. `views/form.php` and `views/show.php` form labels still
ship hardcoded IT — they cite the ISAD(G) section + the matching
ABA MARC field and translating them without a reviewing archivist
would risk drifting from the canonical terminology. Phase 2
follow-up when an archives-domain translator is in the loop.
Playwright E2E
- `tests/archives-crud.spec.js` — 9 serial tests covering:
1. Plugin activation → schema present
2. Index renders (empty-state OR table)
3. Create fonds via form → DB row matches
4. Fonds appears in list
5. Edit fonds → title + extent updated
6. Create child series with parent_id → FK persists
7. Self-parent edit attempt → error shown, no save
8. Soft-delete series → deleted_at set, row gone from list
9. Sidebar link targets /admin/archives
- beforeAll pre-cleans with `DELETE FROM archival_units WHERE reference_code LIKE 'E2E_ARCHIVES_%'`
so a crashed previous run doesn't block the UNIQUE(institution_code, reference_code).
- afterAll hard-deletes the tagged rows — UNIQUE ignores deleted_at,
so soft-delete would leave orphans across runs.
Version bumped to 0.4.0 (status: phase-1d). PHPStan level 5 clean,
`node --check` clean on the spec, existing unit test (28 assertions)
still passes.
fabiodalez-dev
changed the title
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
…Rabbit #104) Two CodeRabbit findings on PR #104 (phase 1a): 1. Major — onActivate() discarded ensureSchema()'s return value, so PluginManager could mark the plugin active with a partial or missing schema. onActivate() now captures the {created, failed} array and throws RuntimeException when failed is non-empty. SecureLogger has already captured the per-table mysqli error inside ensureSchema(), so the exception message only needs to list the tables that couldn't be created — pointing the admin to app.log for the full reason. 2. Minor — README.md line 15 still said "NOT yet executed; review before enabling", a leftover from the initial skeleton-only draft that contradicts line 3 ("Activation creates the three tables"). Rewritten to make it clear the DDL runs when onActivate() fires and keeping the "review before enabling" guidance. Verified: - PHPStan level 5 clean on the plugin. - Unit test (28 assertions) still passes.
# Conflicts: # storage/plugins/archives/ArchivesPlugin.php
Extends the archives plugin with full ISAAR(CPF) authority-records
CRUD and an M:N link UI on the archival-unit detail page. Nine new
endpoints, three new views, one link table exercised end-to-end.
Plugin
- AUTHORITY_ROLES constant — matches the link-table enum
(creator / subject / recipient / custodian / associated),
mirroring MARC 100/600/700/710 semantics in the ABA format.
- Nine registerRoutes() entries, all nested under /admin/archives/
so the plugin owns a single URL prefix.
- Nine handler methods: authorityIndexAction, authorityNewAction,
authorityStoreAction, authorityShowAction, authorityEditAction,
authorityUpdateAction, authorityDestroyAction, attachAuthorityAction,
detachAuthorityAction.
- validateAuthority() — ISAAR 5.1.1 (type ∈ AUTHORITY_TYPES) +
5.1.2 (authorised_form required, ≤500 chars) + 5.2.1 length cap.
Additional ISAAR elements (5.2.3 Places, 5.2.4 Legal status,
5.2.6 Mandates, 5.2.7 Internal structure, 5.2.8 General context)
land in a follow-up PR alongside an archives-domain translator
review — the tables already have the columns, only the form is
phase-2-minimal.
- attachAuthorityAction uses INSERT IGNORE against the composite PK
(archival_unit_id, authority_id, role) so re-posting the same
link is idempotent. detachAuthorityAction removes every role-link
between a given (unit, authority) pair.
- Three small helpers — findAuthorityById (soft-delete-aware fetch),
fetchAuthoritiesForArchivalUnit + fetchArchivalUnitsForAuthority
(M:N joins), listAllAuthorities (populates the attach <select>,
capped at 500 — type-ahead search is a phase-3 task).
- showAction of archival_units now also passes linked_authorities,
available_authorities, and authority_roles to the view so the
unit detail can render the authority section + attach form.
Views (new)
- views/authorities/index.php — list with type-coloured badges
(person/corporate/family), mirror of the archival_units index.
- views/authorities/form.php — 5 core ISAAR fields + dual-mode
(create + edit). Labels cite ISAAR section numbers so archivists
can cross-reference.
- views/authorities/show.php — compact <dl> + list of linked
archival_units with their level badge + role.
- views/show.php (archival_unit) — new "Authority records collegati"
section at the bottom: linked list with per-row unlink form, plus
an attach form with (authority, role) select pair. When no
authorities exist, a CTA links to the create form.
i18n
- 43 new keys in `locale/{it_IT,en_US,de_DE}.json` covering the new
view strings, form labels, ISAAR field titles, and attach/detach
UX copy. DE uses archival-domain terminology: "Normdatensätze",
"Ansetzungsform", "Lebensdaten", "Körperschaft", etc.
Playwright E2E
- `tests/archives-authorities.spec.js`, 9 serial tests covering:
1. Create fonds for linking
2. Authority index renders
3. Create person authority via form
4. Authority appears in list
5. Attach authority to fonds (role=creator)
6. Link renders on both detail pages
7. Edit authority → authorised_form persists
8. Detach link → row gone from archival_unit_authority
9. Soft-delete authority → deleted_at set, disappears from list
- Shared tag prefix (E2E_ARCHAUTH_*) so parallel runs never collide,
pre/post cleanup drops link rows before authority rows before
archival_unit rows (FK order).
Out of scope (→ phase 2b / 3)
- Linkage to the legacy `libri.autori` column
- Biographical ISAAR fields beyond 5.1.1, 5.1.2, 5.2.1, 5.2.2, 5.2.5
- Type-ahead search on the attach <select> (currently LIMIT 500)
- MARCXML I/O (phase 4)
- Unified cross-entity search (phase 3)
Version bumped to 0.5.0 (status: phase-2). PHPStan level 5 clean,
node --check clean on the spec, existing unit test (28 assertions)
still passes.
fabiodalez-dev
changed the title
Owner
Author
|
Phase 2 ( @coderabbitai full review |
|
✅ Actions performedFull review triggered. |
…nified search (#103) Phase 2b New 4th table autori_authority_link (composite PK on autori_id + authority_id, FK on authority_id with ON DELETE CASCADE, no FK on autori_id because the core still hard-deletes and a hard FK would block admin workflows). Added to ensureSchema()'s DDL step list so re-activation provisions it. Authority form grows from 5 to 13 fields — the previously-stored- but-unedited ISAAR columns become editable via a <details> collapsible section: parallel_forms (5.1.3), other_forms (5.1.5), identifiers (5.1.6), places (5.2.3), legal_status (5.2.4), mandates (5.2.6), internal_structure (5.2.7), general_context (5.2.8). Required fields (type + authorised_form) still live above the fold to keep the baseline create flow quick. Refactor: authorityStoreAction + authorityUpdateAction share two new helpers — extractAuthorityPayload() (trims + gathers the 13 form keys) and nullableAuthorityParams() (empty→null for mysqli so COALESCE keeps working downstream). No duplicated bind_param. Two new endpoints: POST /admin/archives/authorities/{id}/autori/link POST /admin/archives/authorities/{id}/autori/{autori_id}/unlink INSERT IGNORE makes relink idempotent. Reconciliation surfaces on authorities/show.php as "Autori di libreria collegati" with name, lifespan, book count, and an attach <select> populated with the first 100 autori rows. A type-ahead for longer libraries is intentionally deferred — see phase 3 below for the JSON API. Phase 3 — unified cross-entity search GET /admin/archives/search?q=... runs three MATCH…AGAINST queries in a single request — archival_units (FULLTEXT on title + scope + history), authority_records (FULLTEXT on name + parallel_forms + history + functions), autori (FULLTEXT on nome, scoped to rows reconciled via autori_authority_link so the view doesn't flood with every library author). Each source is capped at 50 (25 for autori since they're pre-filtered). Results render with level-coloured badges, date ranges, extent, and cross-entity links so you can jump from an authority hit to its archives and to its book-side author in one click. Empty query state hints at MySQL's min-word-length quirk so users understand why "io" returns nothing. New JSON endpoint (scaffold for future glue): GET /admin/archives/api/authorities/search?q=... Returns up to 25 authority_records matching q — shape is {query, results[]} and is ready for an autocomplete frontend on the attach <select> in archival_units show.php. Not wired to a JS consumer yet; that's a small PR on top. /admin/archives index grows a toolbar: "Ricerca", "Authority records", and the existing "Nuovo record archivistico" CTA. i18n +33 new keys in locale/{it_IT,en_US,de_DE}.json covering the search UI, the extended ISAAR field labels, the reconciliation section, and the FULLTEXT min-word hint. DE uses archival terminology consistent with phase 1-2: "Ansetzungsform", "Normdatensätze", "Archiveinheiten", "verknüpfte". Version bumped to 0.7.0 (status: phase-3). PHPStan level 5 clean on the full plugin, `php -l` clean on all new views, unit test (28 assertions) still passes. E2E coverage for the new endpoints will land in a follow-up spec — the existing archives-authorities.spec.js still covers the phase-2 path.
fabiodalez-dev
changed the title
Owner
Author
|
Phases 2b + 3 (commit Phase 2b — 8 extra ISAAR fields in the authority form, new Details in the commit body. +898 lines, +33 i18n keys, PHPStan level 5 clean, 28/28 unit test. @coderabbitai full review |
|
✅ Actions performedFull review triggered. |
Ships round-trip MARCXML for both archival_units and authority_records
using the ABA archive-format crosswalk already documented in README.md.
Output streams through PHP XMLWriter so large fonds don't blow the
memory limit.
Export endpoints
GET /admin/archives/{id}/export.xml
One archival_unit + its attached authorities. Authorities render
as 100/110 (creator), 600/610 (subject), or 700/710 (added entry)
depending on (type, role). Content-Disposition names the file
after the reference_code so downloads land with a stable name.
GET /admin/archives/authorities/{id}/export.xml
One authority record. Uses MARC21 authority tags: 100 (person) /
110 (corporate) for the name, 400 tracings for parallel_forms
and other_forms, and 024/370/368/372/377/373/678 for ISAAR
identifiers / places / legal status / functions / mandates /
internal structure / bio.
GET /admin/archives/export.xml?ids=1,2,3
Bulk collection. When `ids` is omitted, exports every non-deleted
archival_unit (capped at 1000 — larger jobs belong to a CLI batch
script, not a browser request).
Import endpoint
GET /admin/archives/import — upload form (file input + Dry-run
checkbox, checked by default to avoid accidental inserts).
POST /admin/archives/import — parses via SimpleXMLElement with
libxml_use_internal_errors() so parse failures surface as a
user-facing error instead of a 500. Dry-run parses + lists what
WOULD be inserted; unchecking runs the INSERTs.
Phase 4 ingests only `<record type="Bibliographic">` entries —
authority imports land in phase 4b once round-trip validation
is in place.
Implementation notes
- All MARCXML is rooted in <collection xmlns="http://www.loc.gov/MARC21/slim">
so external tools (Koha, Reindex, LC BIBFRAME Editor) can ingest
without a transform.
- `writeMarcDatafield()` skips fields when every subfield is empty —
keeps output compact and avoids `<datafield tag="XXX"/>` noise.
- `parseMarcXml()` falls back to the no-namespace XPath when a producer
emits MARCXML without the slim-namespace declaration (common in
legacy exports).
- `marcSub()` helper wraps the nested-array dereference so PHPStan
level 5 is happy and readers don't have to squint at `??` chains.
- `decodeLevel()` + levelMap invert the ABA 008*c codes a/b/c/d ↔
fonds/series/file/item in both directions.
i18n
+17 keys in it_IT / en_US / de_DE covering the import UI, result
table, dry-run flash, and the new toolbar entries on the index page.
DE uses "MARCXML importieren" / "Testlauf" consistent with archival
terminology.
Out of scope (→ phase 4b)
- Authority-record import
- Round-trip validation + diff-only updates
- XSD schema check
Version bumped to 0.8.0 (status: phase-4). PHPStan level 5 clean on
the full plugin. Unit test (28 assertions) still passes. An E2E
round-trip spec — create fonds → export.xml → re-import → diff —
lands alongside the phase 4 CodeRabbit review, keeping this commit
scoped to the functional slice.
fabiodalez-dev
changed the title
Owner
Author
|
Phase 4 (
+796 lines, PHPStan level 5 clean, 28/28 unit test. Roadmap is now saved in my persistent memory ( @coderabbitai full review |
|
✅ Actions performedFull review triggered. |
|
✅ Actions performedFull review triggered. |
Builds on phase 4's import/export with three round-trip-critical changes:
1. archival_units import is now an UPSERT
New private upsertImportedArchivalUnit() runs an INSERT … ON
DUPLICATE KEY UPDATE keyed on the existing UNIQUE (institution_code,
reference_code). Re-importing a previously-exported file is now
idempotent: rows that already exist report as "updated" instead of
blowing up on the unique constraint. ON DUPLICATE KEY also resets
deleted_at to NULL so a soft-deleted row resurrects on re-import,
which matches the user expectation when round-tripping a backup.
mysqli's affected_rows convention (1=inserted, 2=updated, 0=no-change)
gives us the action label. insert_id is 0 for ON DUPLICATE KEY
matches, so we look the row up by the natural key after the upsert
to get a stable id back to the UI.
2. Authority records import
parseMarcXml() now ALSO emits an `authorities` list, populated by
the new buildAuthorityRowFromMarc() helper. importSubmitAction
iterates that list with a duplicate-skip pre-check via
findAuthorityByName() (lookup on (type, authorised_form)) — ISAAR
has no natural key the way reference_code is for archival_units, so
plain INSERT IGNORE wouldn't help; the explicit pre-check is
transparent and reportable. New rows are persisted via
insertImportedAuthority() which mirrors authorityStoreAction's
13-column payload.
Tag conventions on import mirror the export side:
100/110*a → authorised_form (110 → type=corporate; else person)
100/110*d → dates_of_existence
400*a → parallel_forms (multi-400 collapses to last-write,
documented as a known limitation)
024 370 368 372 377 373 → identifiers / places / legal_status /
functions / mandates / internal_structure
678*a → history (general_context import not split today)
3. Richer import summary
The result struct now carries inserted / updated / skipped /
inserted_authorities / skipped_authorities arrays; the import view
renders two tables (archival_units + authorities) with per-row
status. Dry-run summary now counts both kinds.
i18n
+6 keys in IT/EN/DE for the new statuses ("aggiornato",
"duplicato (saltato)") and table headings.
Roadmap memory updated to reflect phase 4 + 4b done; phase 5
(photographic items) and phase 4c (XSD + multi-400 + Z39.50)
queued.
Version bumped to 0.8.1 (status: phase-4b). PHPStan level 5
clean on the full plugin, unit test (28 assertions) still passes.
A round-trip E2E spec (export → re-import → diff) lands alongside
the phase 4b CodeRabbit pass.
fabiodalez-dev
changed the title
Companion spec to archives-50-elements-crud.spec.js. Same fixture generator (all 4 levels × 15 specific_material × 4 color modes × varied dates/extent/photographer/publisher/collection/local_class), but NO cleanup — rows stay in the DB so they can be inspected on /admin/archives. Distinct TAG prefix (E2E_SEED_ vs E2E_50CRUD_) so the two specs coexist and the CRUD spec's cleanup doesn't accidentally wipe the seed. Each run mints a timestamp-suffixed TAG so repeated runs add cumulative batches. 51/51 passing (1.6m run, 50 CREATE + 1 final assertion). User can now visit http://localhost:8888/admin/archives to see the populated plugin.
…ormat + rename 'authorities' i18n key
- show.php: specific_material (and color_mode) rendered the raw DB
enum value ('drawing', 'bw', ...) — now mapped through \$materialLabelsShow
/ \$colorLabelsShow with __() so Italian admins see "Disegno / opera
grafica (hd)" instead of 'drawing'. Falls back to the raw code if a
future ENUM value isn't in the map.
- show.php: Creato / Ultima modifica dates now pass through format_date()
with separator '/' and include time. Italian locale gets "21/04/2026 22:45"
instead of the MySQL ISO dump.
- 'Gestisci authorities' was literal Italian-English, not a translation
— renamed the i18n key to 'Gestisci record di autorità' in view +
all 3 locale JSONs (EN: "Manage authority records", DE: "Normdatensätze
verwalten").
PHPStan level 5 clean, archives-full 25/25 green.
## Public frontend — /archivio (it) / /archive (en) / /archiv (de)
Two new read-only public routes, rendered inside the standard
/frontend layout (same header/footer/theme as /catalogo, /autore):
- **GET /archivio** — list of root-level archival_units. Card-grid view
with level badge (Fondo/Serie/Fascicolo/Unità), reference code,
formal title, date range, scope preview, extent.
- **GET /archivio/{id}** — single unit detail. Shows full identity +
content area (extent, scope, material, photographer, language,
archival history, access conditions), breadcrumb trail up to the
root fonds, linked authorities grouped by role (creator/subject/
custodian/recipient/associated), and direct children listing with
their own level badges.
Routes are registered in ArchivesPlugin::registerRoutes() without any
auth middleware — archival records are public cultural material.
Implemented as new actions publicIndexAction() + publicShowAction()
and a renderPublic() helper that does the two-pass view → layout
render (same pattern as UserDashboardController).
The localised paths (it/en/de) are resolved through
\App\Support\RouteTranslator::getRouteForLocale so the same
implementation serves all three locales.
## Seed spec extension
tests/archives-seed-50.spec.js now also seeds:
- **15 authority records** (5 person + 5 corporate + 5 family) created
via the admin UI. Real historical figures/organisations to match
the archives plugin's documentary-heritage use case.
- **Link table rows** (~26 M:N entries) wiring 20 archival_units to
one or two authorities each, with varied roles from the enum
(creator/subject/recipient/custodian/associated).
A single seed run now produces a realistically populated archive
the user can browse end-to-end in the admin UI and the public
frontend.
## i18n
17 new keys shipped with translations for EN/DE (archive breadcrumb
labels, empty-state copy, role labels, authority-type labels).
## Routes
locale/routes_{it,en,de}.json gained the `archives` entry so
`RouteTranslator::route('archives')` resolves per locale.
Verification: PHPStan level 5 clean, archives-full 25/25 green,
/archivio and /archivio/{id} both HTTP 200 on localhost:8888.
## Bootstrap 5 restyle
The initial /archivio views used Tailwind utility classes that the
public frontend layout doesn't ship. Rewrote both views (index + show)
with the Bootstrap 5 / custom-CSS primitives used by /catalogo and
/events: container, card, row/col-md, badge text-bg-*, list-group,
breadcrumb. Added minimal scoped <style> blocks for the hero heading
and the ISAD(G) definition-list, so the pages now feel native inside
the public shell.
## Header menu entry
app/Views/frontend/layout.php grows an optional "Archivio" entry
(placed between Catalogo and Eventi in both the desktop nav and the
mobile overlay) that renders only when:
1. The archives plugin is active (plugins.is_active = 1), AND
2. At least one non-deleted archival_unit is published.
The gate uses two LIMIT 1 queries against the existing $container db
connection — negligible overhead per request. A try/catch swallows
the "table doesn't exist" case so the menu entry is simply hidden if
the plugin was never activated.
The link href is resolved through RouteTranslator::route('archives')
so IT/EN/DE installs pick up /archivio, /archive, /archiv respectively.
PHPStan level 5 clean. archives-full 25/25 green. /archivio and
/archivio/{id} both HTTP 200 with Bootstrap styling + header menu
link visible on every public page.
….php
Replaced the flat "breadcrumb + card" detail layout with the same
hero + split-body structure used on /autore/.../{id}:
- Full-bleed coloured hero with large icon box (per-level icon:
archive/folder-open/folder/file-alt), big title, formal subtitle,
date range + extent badges, home-catalog-archive breadcrumb
inside the hero (white text on gradient).
- Body sits -4rem above the hero fold (negative margin trick from
book-hero), rounded-3 cards with shadow, two-column Bootstrap
layout: main (descrizione archivistica + unità discendenti) +
sidebar (soggetti produttori + identificativi).
- Child units now show the per-level icon inside their badge, not
just the text label; hover state highlights the row.
- Authorities sidebar shows role badge (Creatore/Soggetto/…) and
dates_of_existence inline.
- Identifiers card on the side exposes reference_code, institution,
local_classification, collection — the ISAD(G) identity area 3.1.
Verified: /archivio/218 returns HTTP 200 with the new markup.
PHPStan clean (view-only change, no app code touched).
…gn + responsive All 5 review points from the user addressed: 1. **External CSS file**: new storage/plugins/archives/assets/css/archives-public.css served via a plugin route /plugins/archives/assets/css/… (same pattern as digital-library). Removed the inline <style> blocks from both public views — index and show. 2. **Light theme / theme-aware**: dropped the dark gradient hero (the "parte nera" the user flagged). Hero now uses var(--bg-primary) with a border-bottom and the icon-box uses var(--bg-secondary) + var(--color-primary) for the icon — inherits whichever theme (light/dark/custom) the admin picked in Settings → Appearance. 3. **Left-aligned text**: .hero-text is explicitly text-align: left on ≥992px; the responsive block recenters it only below the desktop breakpoint (matches the book-detail pattern). 4. **Mobile responsive**: ported the full @media query ladder from book-detail.php — 1200 / 992 / 768 / 480px breakpoints with the same scaling logic (icon-box shrinks 180→150→120→100px, h1 uses clamp(), breadcrumb wraps + centers, action buttons go vertical on phones, card-body padding shrinks). 5. **Per-unit cover + document download**: view already supports the optional fields cover_image_path, document_path, document_mime, document_filename (next commit adds the migration + admin upload UI). When a document is present: - mime audio/* → green-audio-player (CSS + JS already bundled in the project at /assets/vendor/green-audio-player) - anything else → btn-primary "Scarica documento" with download=filename attribute A cover image, when present, replaces the icon-box in the hero. Verified: /plugins/archives/assets/css/archives-public.css returns HTTP 200 text/css via nginx+php-fpm. PHPStan level 5 clean.
…b/audio/video)
## Schema additions (idempotent migration)
Four new nullable columns on archival_units:
- cover_image_path VARCHAR(500) — e.g. /uploads/archives/covers/218-abcd.jpg
- document_path VARCHAR(500) — e.g. /uploads/archives/documents/218-1234.pdf
- document_mime VARCHAR(100)
- document_filename VARCHAR(255) — original name preserved for download="…"
installer/database/migrations/migrate_0.5.9.sql gets conditional
ALTER TABLE guards (INFORMATION_SCHEMA count) so re-running on an
existing install adds only the missing columns. The CREATE TABLE
block and ArchivesPlugin::ensureSchema() / ddlArchivalUnits() all
include the new fields for fresh installs.
## Admin upload UI
views/show.php grows a "Copertina e documento" card below the ISAD
identity dl, with two independent multipart forms:
- Cover upload — accept image/jpeg|png|webp, max 8 MB
- Document upload — accept pdf/epub/audio(mp3/m4a/ogg/wav)/video(mp4/webm)/image(tiff/jpg/png), max 200 MB
Each side shows a preview (image tag / filename + mime badge) when
an asset is already attached, plus a "Rimuovi …" form that unlinks
the file and nulls the columns.
## Controller
Three new actions on the plugin:
- uploadCoverAction() — thin wrapper calling handleAssetUpload('cover', …)
- uploadDocumentAction()— same for documents
- removeAssetAction() — common entry, dispatches on POST field `type`
handleAssetUpload() is the shared pipeline:
1. PSR-7 getUploadedFiles() + instanceof check
2. getError() === UPLOAD_ERR_OK
3. size cap
4. mime detected via finfo on the temp stream (browser-provided
Content-Type is user-controlled and cannot be trusted)
5. mime whitelist → extension map
6. unlink previous asset (no orphan files)
7. moveTo public/uploads/archives/{covers,documents}/<id>-<rand>.<ext>
8. UPDATE the relevant columns
Routes added under the existing /admin/archives/{id}/ prefix, all
behind CsrfMiddleware + AdminAuthMiddleware — same auth envelope as
the rest of the admin plugin surface.
## Frontend
The public show view (the one rendered on /archivio/{id}) already
supports the fields from the previous commit: cover image replaces
the icon-box in the hero; document triggers either the
green-audio-player (for audio/*) or a "Scarica documento" button
with `download=<original_filename>`.
## Verification
- PHPStan level 5 clean
- archives-full 25/25 green (no regression)
- Migration applied + verified on dev DB: 4 new columns present
- Upload dirs created at public/uploads/archives/{covers,documents}/
Owner
Author
|
@coderabbitai full review Pushed 0f16518 (plus 73d7a02, a1c5493, 4d8bd1f, 5436603, 7d163d2 since the last triggered review). Scope of this batch: App-side
Schema (migrate_0.5.9.sql idempotent ALTERs)
Frontend
i18n~40 new keys across IT/EN/DE (archive menu entry, upload form copy, role labels, level labels, material labels, archival-description strings). Seed / testing
Verification
Please focus on: the upload pipeline ( |
🧩 Analysis chain🏁 Script executed: Length of output: 306521
Verdict
Actionable fixes
PHP (ArchivesPlugin.php) // before unlink, constrain to known prefixes
$allowedPrefixes = $type === 'cover'
? ['/uploads/archives/covers/']
: ['/uploads/archives/documents/'];
$isAllowed = false;
foreach ($allowedPrefixes as $prefix) {
if (str_starts_with($currentPath, $prefix)) { $isAllowed = true; break; }
}
if ($isAllowed) {
$fsPath = __DIR__ . '/../../../public' . $currentPath;
if (is_file($fsPath)) { `@unlink`($fsPath); }
} else {
\App\Support\SecureLogger::warning('[Archives] skip unlink: disallowed path prefix', [
'type' => $type, 'path' => $currentPath
]);
}
PHP (ArchivesPlugin::handleAssetUpload) $stream = $upload->getStream();
$mime = '';
if (function_exists('finfo_open')) {
$f = finfo_open(FILEINFO_MIME_TYPE);
if ($f !== false) {
$tmpPath = $stream->getMetadata('uri');
if (is_string($tmpPath) && `@is_file`($tmpPath)) {
$detected = `@finfo_file`($f, $tmpPath);
if (is_string($detected)) { $mime = $detected; }
}
if ($mime === '') { // fallback to buffer
$contents = (string) $stream;
if ($contents !== '') {
$det = `@finfo_buffer`($f, $contents);
if (is_string($det)) { $mime = $det; }
}
}
finfo_close($f);
}
}
if ($mime === '' || !isset($mimeToExt[$mime])) {
\App\Support\SecureLogger::warning('[Archives] upload rejected — mime not in allow-list', ['kind' => $kind, 'mime' => $mime]);
return $response->withHeader('Location', '/admin/archives/' . $id)->withStatus(303);
}
if (!is_dir($targetDirFs) && !@mkdir($targetDirFs, 0755, true) && !is_dir($targetDirFs)) {
\App\Support\SecureLogger::error('[Archives] failed to create upload dir', ['dir' => $targetDirFs]);
return $response->withHeader('Location', '/admin/archives/' . $id)->withStatus(303);
}ENUM migration note
Public frontend views and theme CSS
Small correctness/readability nits
Let me know if you want me to open a follow-up issue for the unlink guard + finfo_buffer fallback, or draft a patch. 🧠 Learnings used✅ Actions performedReview triggered.
|
… detail page
Three user-reported issues in one commit.
## 1. Menu entry missing from /archivio/{id}
layout.php guarded the "Archivio" nav entry on \$container being in
scope — but plugin-rendered views (renderPublic → include layout)
don't have \$container. The entry was therefore only visible on
core-controller pages (/, /catalogo, /autore/…) and dropped out on
the very pages that most need it.
Fix: renderPublic() now pre-sets \$archivesAvailable = true +
\$archivesRoute before including the layout; layout.php uses
?? null-coalesce on those vars so the fallback probe only runs
when they aren't already set. The Archivio nav link now appears
on every public archive page with an `active` class.
## 2. URLs used the raw id — anti-SEO
The detail URL was /archivio/173 — opaque, no keyword signal for
search engines. Now:
- Canonical: /archivio/{slug}-{id} e.g.
/archivio/seed-archivio-5-drawing-fonds-173
- Legacy: /archivio/{id} (still works)
When the client hits the legacy form (or any slug that no longer
matches the current constructed_title), the action emits a 301 to
the canonical slug+id URL so Google/Bing converge on one result
per archival unit.
Slug generation uses the existing slugify_text() helper from
app/helpers.php (already used by /libro and /autore). Children
listings, breadcrumb links, and the public index grid all switched
to the slug form.
## 3. Schema.org JSON-LD
Added a <script type="application/ld+json"> block to the detail
view with a per-level type mapping:
level=fonds/series/file → ArchiveComponent
level=item → CreativeWork
Properties emitted: @context, @type, name (constructed_title),
alternateName (formal_title), identifier (reference_code), url
(canonical), description (scope_content), inLanguage, temporalCoverage
(date_start/date_end as ISO interval), holdingArchive (ArchiveOrganization
with institution_code), about (Person/Organization for every linked
authority), isPartOf (breadcrumb as ArchiveComponent chain), image
(cover URL when present), associatedMedia (AudioObject / MediaObject
for the downloadable document).
Empty / null properties are pruned before encoding so rich-results
validators don't flag the response.
## Verification
- PHPStan level 5: clean
- Routes tested:
/archivio → 200
/archivio/173 → 301 → slug form
/archivio/seed-archivio-5-drawing-fonds-173 → 200
- Menu entry verified on detail page (active class)
- JSON-LD validated: @type=ArchiveComponent, valid schema.org URLs
The icon-box (or the cover image, when present) was left-aligned
inside the col-lg-4 slot on desktop — it floated against the left
edge of the column instead of sitting centred in the designated
space. The hero-text col-lg-8 is already left-aligned so the visual
balance works best when the media column is centred.
Two small changes:
- storage/plugins/archives/views/public/show.php: the wrapper
<div class="col-lg-4 …"> now uses d-flex justify-content-center
align-items-center so both the icon-box and the archive-cover-large
image sit centred vertically+horizontally.
- assets/css/archives-public.css: .icon-box gets margin: 0 auto on
desktop too (was: margin: 0). Responsive overrides inherit it
automatically.
No behavioural change. /archivio and /archivio/{slug}-{id} still
render identically; just the media column visual layout is tightened.
Owner
Author
|
@coderabbitai full review Pushed further commits since the last review trigger:
Highlights to focus on:
Verification: PHPStan level 5 clean, archives 25/25 + 9/9 + 9/9 all green, routes tested (200/301/200). |
🧩 Analysis chain🏁 Script executed: Length of output: 317476
P1 — Path traversal guard su unlink
// ArchivesPlugin::removeAssetAction (prima di `@unlink`)
$allowedPrefixes = $type === 'cover'
? ['/uploads/archives/covers/']
: ['/uploads/archives/documents/'];
$ok = false;
foreach ($allowedPrefixes as $prefix) {
if (str_starts_with($currentPath, $prefix)) { $ok = true; break; }
}
if ($ok) {
$fsPath = __DIR__ . '/../../../public' . $currentPath;
if (is_file($fsPath)) { `@unlink`($fsPath); }
} else {
\App\Support\SecureLogger::warning('[Archives] skip unlink: disallowed path', [
'type' => $type, 'path' => $currentPath,
]);
}P1 — finfo_buffer fallback su upload temporanei in memoria
// Detect MIME (add buffer fallback)
$stream = $upload->getStream();
$mime = '';
if (function_exists('finfo_open')) {
$fi = finfo_open(FILEINFO_MIME_TYPE);
if ($fi !== false) {
$tmpPath = $stream->getMetadata('uri');
if (is_string($tmpPath) && `@is_file`($tmpPath)) {
$det = `@finfo_file`($fi, $tmpPath);
if (is_string($det)) { $mime = $det; }
}
if ($mime === '') {
$contents = (string) $stream;
if ($contents !== '') {
$det = `@finfo_buffer`($fi, $contents);
if (is_string($det)) { $mime = $det; }
}
}
finfo_close($fi);
}
}
if ($mime === '' || !isset($mimeToExt[$mime])) {
\App\Support\SecureLogger::warning('[Archives] upload rejected — mime not in allow-list', ['kind'=>$kind,'mime'=>$mime]);
return $response->withHeader('Location', '/admin/archives/' . $id)->withStatus(303);
}P2 — mkdir e moveTo: error handling
// mkdir with logging
if (!is_dir($targetDirFs) && !@mkdir($targetDirFs, 0755, true) && !is_dir($targetDirFs)) {
\App\Support\SecureLogger::error('[Archives] failed to create upload dir', ['dir'=>$targetDirFs]);
return $response->withHeader('Location', '/admin/archives/' . $id)->withStatus(303);
}
// moveTo guarded
try {
$upload->moveTo($targetDirFs . '/' . $basename);
} catch (\Throwable $e) {
\App\Support\SecureLogger::error('[Archives] moveTo failed: ' . $e->getMessage(), [
'dest' => $targetDirFs . '/' . $basename, 'kind' => $kind,
]);
return $response->withHeader('Location', '/admin/archives/' . $id)->withStatus(303);
}SEO slug URLs
JSON‑LD
Menu entry “Archivio” nel layout pubblico
<!-- Desktop nav -->
<li>
<?php if (($archivesAvailable ?? false) && !empty($archivesRoute ?? null)): ?>
<a href="<?= htmlspecialchars($archivesRoute, ENT_QUOTES, 'UTF-8') ?>"
class="<?= strpos($_SERVER['REQUEST_URI'] ?? '', $archivesRoute) !== false ? 'active' : '' ?>">
<?= __('Archivio') ?>
</a>
<?php endif; ?>
</li>
<!-- Mobile nav analogo ... -->CSS tema (fallback variabili)
/* storage/plugins/archives/assets/css/archives-public.css */
:root{
--archives-color-primary: var(--color-primary, var(--primary-color, `#0f172a`));
--archives-text-primary: var(--text-primary, var(--text-color, `#0f172a`));
--archives-bg-primary: var(--bg-primary, var(--light-bg, `#f8f9fa`));
}
/* poi usa le --archives-* nelle regole */Serve asset statici
Verifica generale
Vuoi che apra un issue di follow‑up per: (1) unlink path‑guard + (2) finfo_buffer fallback + (3) mkdir/moveTo logging? Posso anche preparare una patch integrata. 🧠 Learnings used✅ Actions performedReview triggered.
|
…or inspection
New spec tests/archives-upload-assets.spec.js drives the full upload
pipeline through Playwright's setInputFiles:
1. JPEG cover image (3 distinct E2E_SEED_* units)
2. PDF document on one unit
3. MP3 audio on another unit
Fixture files are generated outside the test (ffmpeg sine wave for the
MP3, minimal one-page PDF, the existing placeholder.jpg for the cover)
at stable /tmp paths so the spec is deterministic.
Each upload is asserted on three surfaces:
- DB columns populated with the expected relative path + mime +
original filename (cover_image_path, document_path, document_mime,
document_filename)
- Physical file present on disk under public/uploads/archives/
{covers,documents}/<id>-<rand>.<ext>
- Rendered correctly on the public /archivio/{id} page:
* PDF → "Scarica documento" button with download attribute
* Audio → green-audio-player initialised on <audio src="...mp3">
* Cover → served by nginx at /uploads/archives/covers/…
No cleanup — uploads stay on disk and in the DB for manual inspection
via /archivio/{slug}-{id}. 9/9 tests green in 5.4s.
Owner
Author
|
@coderabbitai review New commit c9f64f2 —
No cleanup — seed fixtures + uploaded files kept so /archivio/{slug}-{id} can be inspected manually. 9/9 green (5.4 s). Focus the review on the fixture/mime whitelist contract in the test. |
🧩 Analysis chain🏁 Script executed: Length of output: 9662
✅ Solid
|
…blic The archives plugin CSS (archives-public.css) and the two public views were not being tracked by git — the catch-all rule \`storage/plugins/archives/*\` on line 163 matched the \`assets/\` and \`views/public/\` directories, and no explicit re-include existed for them. As a result: - the stylesheet introduced in commit 73d7a02 was only present on the developer's disk, never pushed, never distributed in the ZIP; - the public /archivio views shipped only because they happened to be added in an earlier commit before the catch-all tightened. Added explicit \`!\` rules for: - storage/plugins/archives/views/public/ + *.php - storage/plugins/archives/assets/ + css/*.css + js/*.js Plus: drop every box-shadow from the archive public pages (flat by design — matches the user's review comment) and add a defensive \`box-shadow: none !important\` block guarding against ambient shadows coming from Bootstrap or the theme card rule (\`0 4px 16px rgba(0,0,0,.07)\`). The .archive-card hover affordance now uses transform + border-color change instead of a drop shadow.
…heme vars Seven findings from the last three CodeRabbit rounds (74287ba + c9f64f2 + 0f16518), all actioned. ## P1 security — unlink path-prefix guard (ArchivesPlugin::removeAssetAction) The unlink path came from the DB; a manipulated \`cover_image_path\` row could have pointed outside /uploads/archives/ and let the action delete arbitrary files under public/. Added a str_starts_with check against the expected prefix before calling unlink; mismatches are logged via SecureLogger and skipped. ## P1 robustness — finfo_buffer fallback (ArchivesPlugin::handleAssetUpload) Some PSR-7 implementations keep the uploaded body in php://temp; finfo_file cannot read that and returned '' → the request was rejected even for legitimate allow-listed MIME types. Now: 1. finfo_file on the temp path (normal \$_FILES + on-disk streams) 2. fall back to finfo_buffer((string) \$stream) if finfo_file fails Both are wrapped in @ and a closed finfo resource. ## P2 observability — mkdir + moveTo error logging \`@mkdir\` hid FS errors (disk full, ENOSPC, permission denied) and \`moveTo\` could throw mid-flight, leaving the DB un-updated without any trace. Now mkdir failure logs via SecureLogger::error and returns early; moveTo is wrapped in try/catch that logs + bails before touching the row, so a failed upload never leaves a DB/fs mismatch. ## CSS theme-var contract — plugin-local aliases with fallbacks The stylesheet used \`var(--bg-primary)\`, \`var(--color-primary)\`, \`var(--text-primary)\` — names that don't exist in the public frontend layout (which ships \`--primary-color\`, \`--text-color\`, \`--light-bg\`). Introduced a :root block that resolves plugin-local \`--archives-*\` aliases via cascading fallbacks (\`var(--bg-primary, var(--light-bg, #ffffff))\`) and rewrote every downstream rule to use the \`--archives-*\` names. Archive pages now pick up the correct theme colours on any Pinakes install. ## Test coverage — two negative MIME-whitelist assertions tests/archives-upload-assets.spec.js grows two tests: 9. Uploads a <?php payload with a .jpg extension; asserts the cover_image_path column is unchanged (whitelist catches it via finfo, not via the client-provided extension). 10. Uploads a fake .exe with the MZ PE header; asserts document_path stays untouched (application/x-dosexec is not on the document mime allow-list). Also dropped the noisy test 9 'Final summary' that only logged, and renamed the misleading \`slug\` variable to use \`ids.\${kind}\` directly — the 301-redirect handling means the numeric id is sufficient for the request anyway. ## Verification - PHPStan level 5: clean - archives-upload-assets: 10/10 green (includes the 2 new negative MIME rejections) - archives-full + archives-crud + archives-authorities: 43/43 green
6 tasks
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Add 20 reusable archive document seeds, Playwright coverage, and fixture assets for admin/public routes. Keep the seed idempotent and persistent so records can be reused across runs.
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
…#105) CodeRabbit PR #105 review flagged two nitpicks asking to clarify test conformance with the project-wide Flatpickr + SweetAlert2 conventions: - Add explanatory comment before page.fill('date_start'/'date_end') in both archives-crud.spec.js and archives-full.spec.js: the form uses <input type="number"> (year-only smallint), NOT Flatpickr, so page.fill is the correct interaction. Note what to switch to if the UI ever migrates to Flatpickr. - swal2 pattern (.swal2-confirm) is already the exclusive confirm flow in both specs (no legacy page.once('dialog') or dialog.accept() remains) — the existing "Destructive confirmations go through SweetAlert2" block comment already documents that contract.
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Resolve ScrapeController hook conflict: keep rawIdentifier in pos 1 (needed by DiscogsPlugin::validateBarcode for Cat# matching), add cleanIsbn as pos 3 extension for plugins that want the normalised form.
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.
Stacked on PR #104 (phase 1a). Merge order: #104 → this.
Covers phases 1b + 1c + 1d of the archives plugin roadmap, accumulated on the same branch to avoid deep-stacking (CodeRabbit disables auto-review on non-default base branches — see comment thread).
Endpoints (all behind
AdminAuthMiddleware; POST alsoCsrfMiddleware)/admin/archives/admin/archives/new/admin/archives/new/admin/archives/{id}/admin/archives/{id}/edit/admin/archives/{id}/edit/admin/archives/{id}/deleteAll routes are registered inside the plugin via
app.routes.register(DeweyEditorPlugin pattern).User-facing polish (phase 1d)
\App\Support\Hooks::do('admin.menu.render')call inapp/Views/layout.phpbetween "Collocazione" and "Utenti"; plugin hooks in and echoes<a href="/admin/archives">…</a>matching the existing Tailwind pattern. Deactivating the plugin removes the hook row → entry disappears.locale/{it_IT,en_US,de_DE}.json; DE uses domain-accurate terminology ("Signatur", "Bestand", "Einzelstück"). Form field labels that cite ISAD(G) sections are deliberately kept hardcoded IT for phase 2 when an archives-domain translator can review them.tests/archives-crud.spec.js, 9 serial tests covering activation, create fonds → appears in list, edit → update, create child series with parent_id, self-parent rejection, soft-delete, sidebar link presence.Plugin glue
setPluginId(int)/$pluginId— lets the plugin CRUD its rows inplugin_hooks.onActivate()→ DDL + two hook rows (app.routes.register,admin.menu.render); idempotent.onDeactivate()→ removes all plugin hook rows; data is preserved.registerRoutes($app)→ attaches all seven routes.renderAdminMenuEntry()→ echoes sidebar<a>conforming to core pattern.Handlers + validation
validateArchivalUnit($values, ?int $excludeId = null):reference_coderequired + ≤64 charslevel∈LEVELSconstructed_titlerequired + ≤500 charsdate_start/date_endwithin SMALLINT range, end ≥ startparent_id(if set) must reference a non-deleted rowupdateActionalso guardsparent_id === idfindById()centralisesdeleted_at IS NULLso every detail-path gets soft-delete for free.destroyActionusesUPDATE … SET deleted_at = NOW()— aligned with thelibritable convention. No cascade; deleting a fonds with active series below needs explicit UI confirm (phase 2).Out of scope
authority_records+archival_unit_authoritytables already exist from phase 1a; UI lands later.Note on CodeRabbit
Auto-review is disabled by CodeRabbit when the PR's base isn't the default branch. Earlier
@coderabbitai full reviewrun (triggered manually at 16:42Z on phase 1b+1c commit) came back SUCCESS with zero findings. Triggering again for the phase 1d additions.Test plan
php -lcleannode --check tests/archives-crud.spec.jscleantests/archives-plugin.unit.php) — 28/28 passes/tmp/run-e2e.sh tests/archives-crud.spec.js --workers=1@coderabbitai full review
Summary by CodeRabbit
Nuove Funzionalità
Documentazione
Localizzazione
Test
Chore