Repository navigation
Release 0.9.0-rc.1 - #514
Merged
Merged
Conversation
|
Important Review skippedToo many files! This PR contains 659 files, which is 559 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 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.
The explicit section-to-test map stopped at the fines ledger, so the three sections this branch adds to migrate_0.9.0-rc.1.sql failed the runner as untested. Each now names its own behavioural test.
Re-recorded with --record after the UNIMARC export started marking a serial with bibliographic level s (leader/07) and its ISSN in 011. The serial goldens differ only in those two places, plus the record length and directory that follow from them; every other golden is unchanged.
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 ｜ 𝕏
importTriggers() counted a trigger only when the chunk between two $$ delimiters started with CREATE TRIGGER, but every chunk starts with comments and a DROP TRIGGER. The count stayed at 0, so every successful install ended on the "manual action required" page, blaming database permissions while all the triggers were in place. The final page now also shows the database error behind a warning, since the fixed permissions text hid the real cause whatever it was.
Owner
Author
|
@coderabbitai review |
Action not completed
|
CodeRabbit refuses to review a pull request with more than 100 files and now reports that as a skipped check run instead of a successful status. A release PR that carries a stacked series always crosses the limit, so the prerelease gate vetoed v0.9.0-rc.1 even though every PR in the series was reviewed on its own. The gate now reads the CodeRabbit app's check run on the tagged commit and accepts the skip only when every run comes from the coderabbitai app, targets that exact commit, and carries the file-limit title. A rate-limit skip, a skip on another commit or a run posted by another app still vetoes, and the skip never excuses any other failing check.
The test dropped and recreated <DB_NAME>_migtrg with DROP DATABASE, so a database that already carried that name lost its data without warning. It now follows the other sandbox suites: the name can be set with INSTALLER_TRIGGER_SANDBOX_DB and is refused when it is the installation's own database, the database is created only if missing, and a database that already holds tables without this test's marker table is refused. Between scenarios and at the end only the tables are emptied.
…ing' into release/0.9.0-rc.1
There was a problem hiding this comment.
ℹ️ No blocking issues. One minor wording point on the installer's final page is inline.
Reviewed changes
This run covers the four non-merge commits since the previous Pullfrog review at 93c25855. Both new tests pass locally: tests/installer-trigger-import.unit.php gave 9/0 against a temporary MySQL 8.4, and tests/release-source-policy.test.sh gave 31/0.
- Trigger count fixed:
Installer::importTriggers()now matchesCREATE TRIGGERat the start of any line of a$$chunk, not only at the start of the chunk. Each chunk opens with comments,SETandDROP TRIGGER, so the count used to stay at 0 and every install ended with the "Nessun trigger valido" warning. A real failure is still reported: the multi-statementexec()surfaces the failingCREATE, and the healthy triggers stay in place. - Warnings shown on the final page:
step7.phpnow lists each trigger warning, escaped withhtmlspecialchars. - Sandbox test: the new DB test runs the real importer and the real
triggers.sqlin a<DB_NAME>_migtrgsandbox. It refuses a database that holds tables without its marker, and it never drops the sandbox. It covers a fresh import, a re-import, and a missingprestititable. - Release gate:
ci-verify-release-source.shnow accepts a requiredCodeRabbitcheck in theskippingbucket, but only when everyCodeRabbitcheck run on$GITHUB_SHAcomes from thecoderabbitaiapp and is a skip titledReview skipped: N files exceed the limit of M. A rate-limit skip, a run from another app, a different SHA, or a failure in another check still vetoes. The fakeghcovers each of these cases.
claude-opus-5-5 ｜ 𝕏
The final page opened the trigger warning with "the database user lacks the permissions to create triggers", and since the previous commit it shows the real database error right under it. When a trigger fails for another reason, such as a missing table, the two lines contradicted each other. The opening sentence now only says that some triggers could not be created, in all five locales, and the CHANGELOG entry says both lines are shown.
The final page opened the trigger warning with "the database user lacks the permissions to create triggers", and since the previous commit it shows the real database error right under it. When a trigger fails for another reason, such as a missing table, the two lines contradicted each other. The opening sentence now only says that some triggers could not be created, in all five locales.
…ing' into release/0.9.0-rc.1
There was a problem hiding this comment.
✅ No new issues found. The installer's final page no longer blames the database user's permissions for every trigger failure.
Reviewed changes
This run covers the two non-merge commits since the previous Pullfrog review at 23fa2dd9.
- Neutral trigger warning:
installer/steps/step7.phpnow opens the warning with "Some database triggers could not be created", followed by the escaped database messages. The new string is translated in all five locales, andscripts/ci-check-locales.pypasses. The old permissions string remains in the locale files but nothing uses it. - CHANGELOG: the Fixes entry now says that the page shows both the general sentence and the database's own message.
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 was referenced Nov 1, 2026
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.