Repository navigation
Release 0.9.0-rc.1 - #514
Open
fabiodalez-dev wants to merge 625 commits into
Open
fabiodalez-dev wants to merge 625 commits into
fabiodalez-dev wants to merge 625 commits into
Conversation
|
Important Review skippedToo many files! This PR contains 654 files, which is 554 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. Usage-priced reviews support at most 300 files.
 |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues. There is one small regression in the contributor cache normaliser; see the inline comment.
Reviewed changes
The stacked feature PRs (#502 → #513) were already reviewed on their own PRs. This review covers only what landed after the last reviewed #512 head (029b19a7): 42 files, up to and including the release commit.
- Release metadata:
version.jsonis now0.9.0-rc.1. The CHANGELOG heading is the exact## [0.9.0-rc.1]thatcreate-release.shneeds, and README has## What's New in v0.9.0-rc.1. Every bundledrequires_appis ≤0.9.0-rc.1; mobile-api is the only plugin pinned to the RC. - Migration renamed for the RC:
migrate_0.9.0.sql→migrate_0.9.0-rc.1.sqlwith identical content. All section tests, seed comments andversion_compareguards point at the new name. The dispatchertests/migration-0.9.0-rc.1.unit.phpnow skips itself, since it matches its ownmigration-0.9.0-*glob.scripts/verify-schema.shalso runs the RC migration's test whenversion.jsonis the stable X.Y.Z, and fails if that test is missing. - Contributor cache lists:
AuthorNormalizer::normalizeStored()now splits a;/|list and normalises each name, so inverted names and per-name quotes come out right. LibraryThing import andBookRepository::updateOptionals()use it for thelibri.*cache columns. - Vanished holiday ids:
HolidayPresets::missingIds()lists the chosen ids that the rule table or dataset no longer knows. The calendar page shows them in a warning box, with locale strings in all five languages, a unit test and an E2E test.
ℹ️ The RC migration will not run again on the upgrade from the RC to 0.9.0
The PR body says migrate_0.9.0-rc.1.sql "runs on the RC and again on the later 0.9.0 upgrade". However, Updater::shouldRunMigration() needs the migration version to be greater than $fromVersion, and isMigrationExecuted() also skips it. So on an RC install that upgrades to 0.9.0, the file never runs again. It does still run once for libraries that go straight from 0.8.x to 0.9.0. This means any schema fix made after the RC has to ship in a new file, such as migrate_0.9.0.sql or migrate_0.9.0-rc.2.sql, not as an edit to this one.
Technical details
# RC migration is not re-run on RC → stable
## Affected sites
- app/Support/Updater.php:3827 — `shouldRunMigration()` uses `version_compare($migrationVersion, $fromVersion, '>')`; `'0.9.0-rc.1' > '0.9.0-rc.1'` is false.
- scripts/verify-schema.sh:55 — comment "That migration still runs on the upgrade to the stable release" is true only for upgrades from < 0.9.0-rc.1.
## Required outcome
- Release notes / PR body describe the actual behaviour; any change to the 0.9.0 schema after this RC ships in a new migration file.claude-opus-5-5 ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found. The contributor cache fix and the MARC dedup change both look correct.
Reviewed changes
This run covers what changed since the previous Pullfrog review at 4ffc58b8. The fixes were made on the stack branches and merged forward into this branch.
- Entity-safe list split:
AuthorNormalizer::normalizeStored()now looks for the list separator on the decoded value and splits withContributorSync::splitNames(). As a result,O'Brien, Flannstays one name, and the cache uses the same rule as the entity sync. Two new checks cover a single name with an entity and a list that contains one. - MARC title + author dedup: when a record has no ISBN,
MarcBookImporter::findExisting()now resolves the first author withAuthorRepository::findByCanonicalName(). That is the same lookup thatCsvImportController::getOrCreateAuthor()uses when it links the author. The query then matchesla.autore_idtogether with the accent-exact title. Before, aHaffnerrecord never matched a book filed underHäffner, so re-importing it created a duplicate. New DB checks cover four cases: the fold-to-existing match, a double import that leaves one book, the exact spelling winning once both authors are on file, and accent-distinct titles.
The earlier note on the migration still applies, and the PR body has not changed. An RC install that upgrades to 0.9.0 will not run migrate_0.9.0-rc.1.sql again, so any schema fix after the RC needs a new migration file.
claude-opus-5-5 ｜ 𝕏
The close panel claimed that book availability is recalculated whatever the operator picks, but leaving the missing copies as they are touches no copy and recalculates nothing. The hint now says availability changes only when the copies are marked as lost, in every locale.
The settings persistence test now saves a QR code and the A4 sheet format and asserts both rows in system_settings, plus that a card PDF without ?format uses the stored sheet default. The A4 sheet test reads word positions with pdftotext -bbox and checks that the back text sits in the mirrored column of its front.
The 0.9.0 migration entry point only compared the number of SQL sections with the number of section tests, so one extra test could hide a section that has none. It now carries an explicit section-to-test map (stocktake included, no slug derivation) and fails unless every section in the SQL has its mapped test file.
The afterAll cleanup no longer deletes every closure above an id taken at start, which could also remove rows a concurrent writer stored meanwhile; every closure the spec creates carries the run tag, so that tag is the only criterion.
Before importing the Italian holidays the test makes the yearly Ferragosto closure absent (pre-existing rows are made non-yearly and restored in the teardown), then asserts the import inserted exactly that holiday instead of relying on the total count of holidays.
The UNIMARC builder always wrote bibliographic level m in label/07, so a periodical exported as a monograph and its ISSN was lost. A serial now gets level s and its ISSN in field 011, mirroring the MARC 21 leader/07 and 022 logic; monographs are unchanged.
The deleted-book duplicate case soft-deleted the book with a direct UPDATE, so it proved nothing about what the real delete path leaves behind. It now deletes through the admin bulk delete endpoint and asserts the soft delete cleared the identifiers before re-importing.
Instead of accepting any start date after today, the test walks the stored library-wide closures (one-off and yearly) from tomorrow and asserts the reservation starts on the first day none of them covers, so a pre-existing closure can no longer hide a wrong date.
The default-port test only covered MarcCoverFetcher::effectivePort. LocalSruClient::isLocalUrl now has its own checks: the installation's /api/sru is recognised with the HTTPS port implicit or written out, on either the canonical URL or the target, while another port or plain http on 443 is not.
The whole suite was skipped when the bibframe-linked-data plugin was inactive, although only the BIBFRAME checks need it: now only those tests skip, and the form, storage, page and clear-override tests always run. The DVD case also checks the derived video media label, the post-clear BIBFRAME check asserts the derived audio media type, and a new pair of tests saves a content type override and verifies it in the database, the reloaded form, the book page and BIBFRAME.
expectCardSize read only the first /MediaBox, so a multi-card PDF with a wrongly sized later page still passed. It now collects every MediaBox, requires at least one and checks width and height of each against 125 x 75 mm.
The schema.sql check confirmed the presenze table and columns but not the delete rule of fk_presenze_utente. It now also requires ON DELETE SET NULL there, so a fresh install cannot drift from the migration and drop visits when a member is deleted.
The test now dispatches a real POST to /admin/calendar/rotate-link through the registered routes and middleware, with a valid CSRF token, as a custom role holding settings.manage and as staff. Both pass the middleware, are refused by the route and leave the library feed secret unchanged. A failed check now throws so the role and user fixtures are always removed.
Both modules are off on an upgraded install, so the 0.9.0 write checks ran against modules that refuse to write. The test now switches them on for those checks and also asserts the opposite: with the modules off a save keeps the stored values and an import batch assigns no supplier.
With loss penalties off the backfill never completes and chargeLossPenalty() re-arms the marker, so debtBalanceCents() and debtBalanceSql() kept adding every unconverted prestiti.sanzione: patrons were refused loans for amounts the library chose not to mirror, while the balance shown to staff and patrons stayed under the threshold. legacyPenaltiesPending() now answers false when loss penalties are off, so the debt block reads the ledger alone in PHP and SQL.
importCodes() keeps one transaction for the whole file, so the locking reads record() takes for a single scan held the copie and prestiti locks of every recorded code until the last one: returns waited for the upload, and a desk loan of a copy further down the file could deadlock with it. The upload now reads the committed loan state without locks; a single scan, in its own short transaction, keeps the locking reads that wait for a return in progress.
A cancel whose history entry failed hid the resume panel but left the upload step hidden too (it is rendered hidden while a run is resumable), so the page showed only the warning and no way to load a new file until a reload. The upload step is now shown under the warning.
There was a problem hiding this comment.
✅ No new issues found. The stocktake upload no longer holds circulation locks for the whole file, and the user import recovers cleanly after an unlogged cancel.
Reviewed changes
This run covers the two non-merge commits since the previous Pullfrog review at 4411496e.
- Stocktake upload without circulation locks: when
record()runs insideimportCodes()'s single transaction, it now reads the loan and the copy state with plain consistent reads. A singlescan()still uses the locking reads (loan, then copy). Before, a long upload held share locks on every recordedprestiti/copierow until the last code, so desk loans and returns of those copies waited, and a loan of a later copy could deadlock. The trade-off is deliberate: a copy returned while the upload runs can be recorded asin_prestito.inventario_scansionihas no foreign key tocopie, so the scanINSERTtakes no parent-row lock either. A new DB test holds a copyFOR UPDATEon a second connection and checks two things: the single scan times out, and the upload records the code. - User import after an unlogged cancel: when abandoning an interrupted import returns a
history_warning, the page now shows the upload form again under the warning, so a new file can be picked without reloading.
The earlier note on the migration still applies: an RC install that upgrades to 0.9.0 will not run migrate_0.9.0-rc.1.sql again.
claude-opus-5-5 ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found. This third hardening pass looks correct. The changed pure unit suites (
library-calendar,stack-090-review-fixes,sru-search,unimarc-standards,copy-cataloging-limits,stocktake-classification/marc-oai-byte-identity,marc-library) all pass locally, and the locale parity check is clean.
Reviewed changes
This run covers the 37 non-merge commits since c66fc170. They were merged forward through the stack branches.
- Debt block fails closed:
PatronLedger::schemaReadyOrFail()/activeOrFail()now throw when the schema probe fails, instead of reading as "no ledger". The debt block (CirculationPolicy,LoanEligibility::debtBlockSql()) and the reader delete guard use them. While the legacy backfill is still pending,debtBalanceSql()adds unconvertedprestiti.sanzioneamounts. It uses the sameNOT EXISTS penalty:<id>predicate as the backfill, so a converted penalty is counted once. - Calendar reads fail closed:
loadClosedWeekdays()andbranchOfCopy()now throw on a failed read, likeloadClosures(). ICSEXDATEnow matches on the DTSTART date and time, and trailing junk in a date is refused. - Calendar admin: an update re-checks that its closure still exists under the lock. ICS import carries the whole-year check state from one event to the next, so it only rescans from a known open day that the new row closes. Coverage rejections now get their own message.
- User import: each block resumes at a saved byte offset with
fgetcsv, using the same controls asCsv::configureReader(). The per-rowseen_*session sets are replaced byexists().send_invitesis now a real boolean. A run closed without its history entry now shows ahistory_warninginstead of claiming it was recorded. - Reader delete: the row is deleted first. Its photo file is removed before the commit, and a failed unlink rolls back the delete. Photo writes now unlink a short or failed write.
- Stocktake: a scan reads the loan, then the copy, with share locks inside its transaction. The CSV report is streamed from a generator, and the PDF re-checks the exact rows it renders (
LengthExceptionaboveMAX_ROWS). Code import reads only the first cell of each line or row. - MARC import: in non-skip modes, a record whose identifiers match more than one book is refused. The title+author fallback now requires a principal author. An overwrite clears another work's ISBN form. ISO 2709 detection checks for the terminator at the base address.
- UNIMARC export: both ISBNs and every language are written. The genre moves from
606 $2 localto608 $2 local(unimarcGenreField()) in the builder and in the SRU formatter, and the mapper reads it back. - Copy cataloguing / SRU: retries carrying the same
import_tokenreplay the first answer, and the MARC detail is capped. SSRF-blocked requests are not retried. The retry budget now includes backoff and rate-limit waits. The bare-isbn fallback only runs after an HTTP 400. - Smaller guards: bulk card print above
MAX_BULKnow returns 422. MARC export refuses anids[]array. The previous MARC preview is kept until the new file is staged. The OAI single download reports a reduced record or cut fields in headers. The book request return date stops at the last loaded availability day.
The earlier note on the migration still applies: an RC install that upgrades to 0.9.0 will not run migrate_0.9.0-rc.1.sql again.
claude-opus-5-5 ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found. With loss penalties off, the debt block now uses the same balance that staff and patrons see.
Reviewed changes
This run covers the single non-merge commit since the previous Pullfrog review at df3e9fdc.
- Legacy penalties when loss penalties are off:
PatronLedger::legacyPenaltiesPending()now returns false whenloss_penaltiesis off. As a result,debtBalanceSql()anddebtBalanceCents()fall back tobalanceSql()andbalanceCents(). Before,chargeLossPenalty()re-armed the marker andbackfillLegacyPenalties()never ran with the module off, so unconvertedprestiti.sanzioneamounts kept blocking loans indefinitely, even though no visible balance included them. Those two methods are the only callers. When the module is switched back on,SettingsControllerre-arms the marker, so the pending amounts count again until the backfill drains. Becauseloss_penaltiesis a child offines, turningfinesoff has the same effect. A new check intests/patron-ledger.unit.phpcovers both the PHP and the SQL paths. It needs a DB, so it did not run here.
The earlier note on the migration still applies: an RC install that upgrades to 0.9.0 will not run migrate_0.9.0-rc.1.sql again.
claude-opus-5-5 ｜ 𝕏
0162a39 stopped the longest return date at the last loaded availability day, but the book page loads 180 days and lets a reader start on almost any of them: a start four months ahead could no longer have its return date in the following month. The public availability route now loads occupancy data past the 180 start days by the longest loan plus the closed-day margin (the same horizon the closed days already use), and returns start_until, the last day a request may start on, which the start picker uses as its maximum. The clamp from 0162a39 stays as a safety net.
There was a problem hiding this comment.
✅ No new issues found. A reader who picks a late start date can now get the full loan window again.
Reviewed changes
This run covers the single non-merge commit since the previous Pullfrog review at 96881d09.
- Availability past the last start day:
getBookAvailabilityData()has a new$coverLoanWindowflag. When the flag is set,daysnow reaches as far asclosed_datesalready did: 180 start days, plus the longest loan, plus the 31-day closed-day margin. The method also always returnsstart_until, the last day a request may start on. Only the public/api/books/{id}/availabilityroute sets the flag, so the mobile, staff and calendar callers are unchanged. - Start picker cap:
book-detail.phpnow usesstart_untilas the start picker'smaxDate, and falls back to the last loaded day when it is missing. ThelastLoadedclamp inloanWindowFrom()stays as a safety net, and it no longer cuts a late start short.tests/loan-window-cap.unit.phpchecks that the plain call ends atstart_untiland that the covered call runs past it by at least the longest loan plus 31 days. - Side effects on the same route:
earliest_availableandunavailable_datesnow cover the longer window. This only matters when all 180 start days are blocked, and then no start can be picked either way. In the admin loan form (crea_prestito.php),isAvailableNowstill means "no blocked day in the loaded window", but the window is now longer. That form takes nomaxDatefromdays.
The earlier note on the migration still applies: an RC install that upgrades to 0.9.0 will not run migrate_0.9.0-rc.1.sql again.
claude-opus-5-5 ｜ 𝕏
This branch has not been deployed
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
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.
Release candidate 0.9.0-rc.1: the 0.9.0 stack (#502 → #504 → #505 → #506 → #507 → #508 → #509 → #510 → #511 → #512 → #513) plus the version bump, the CHANGELOG entry and the README lead.
The prerelease tag goes on the head of this PR once every required check is green; the PR is merged with a merge commit afterwards, so the tagged commit stays in main. #503 (CI runners, TinyMCE 8.10.0) carries the same three CI commits as #502 and is part of this tree.
Upgrade path
migrate_0.9.0-rc.1.sql: a prerelease sorts before its final, somigrate_0.9.0.sqlwould be skipped on the upgrade to the RC; this name runs on the RC and again on the later 0.9.0 upgrade, where it is a no-op.tests/migration-0.9.0-rc.1.unit.phpruns every section test;scripts/verify-schema.shnow also runs the test of amigrate_X.Y.Z-rc.N.sqlat the stable X.Y.Z, and fails when it is missing.0.9.0-rc.1, so it can be activated on the RC.Settings of an upgraded installation
The migration only inserts missing rows. Stocktake, suppliers, the visitor log, RDA overrides and fines start switched off; the other modules are on but change nothing until configured (the seeded Standard patron type inherits every global setting, the closures calendar is empty, no fine is charged, nobody is blocked, no new e-mail is sent). Historic
prestiti.sanzioneamounts become ledger debts only after fines and loss penalties are switched on.Checks run on this head
Local Code Quality mirror on a fresh 0.9.0 database: PHPStan level 5 on the full tree, composer and npm audits, locale and route parity, frontend lint, migration and requires_app guards, schema gate with
version.jsonat 0.9.0-rc.1, 325 of 328 standalone unit tests. The three that do not pass locally fail identically on the unchanged code from a worktree (two maintenance tests hit the main checkout's server; one suite needs to create databases as root) and run in CI.