mirror of
https://github.com/FOSSBilling/FOSSBilling.git
synced 2026-11-08 21:15:33 +09:00
codex/client-area-modernisation
24
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Adam Daley
|
256751b831 |
Allow clients to belong to multiple client groups (#4405)
* Allow clients to belong to multiple client groups (#4387) * Use spellcheck-clean identifiers in multi-group E2E * Renumber multi-group migration to patch125 after main merge * Halve guest logins in multi-group E2E for the shared api_login budget * Only migrate group assignments that reference existing groups |
||
|
Adam Daley
|
e8f375db3b |
Invoice immutability, issuing model, and audit journal (#4423)
* Add invoice cancel and immutability flow Introduce admin invoice cancel/void support with new before/after events, UI actions, and API/service handling for approved unpaid invoices. Consolidate legacy invoice edit/delete toggles into a single `invoice_immutability` setting, tighten issued-invoice edit rules, update expired unpaid invoice cleanup to void issued invoices instead of deleting them. * Rename invoice approval flow to issuing * Refine invoice draft and void workflows * Add invoice journal history tracking Add an invoice event journal to capture lifecycle changes with trimmed snapshots, expose it through the admin API, and replace the invoice billing-parties tab with a history view and JSON download. This also adds upgrade patches and a backfill so existing installs get a baseline journal entry for invoices that predate the feature, plus tests and schema snapshot updates for the new entity. * Fix client page rendering and JSON encoding Move client related-list queries to the tab scope so shared footer blocks can access them under Twig strict_variables, and add a regression test for the manage page render path. Also harden JSON output in invoice/update patch flows by throwing on journal encoding failures and substituting invalid UTF-8 in legacy invoice snapshots so one bad row does not block updates. * Harden invoice journal follow-ups: consumed-keys changes, activity log lines, translatable labels, E2E coverage * Fix datepicker clear button pushing calendar icon over the field value * Use DBAL 4.5 introspectTableByUnquotedName in invoice patch helpers * Harden invoice issuing and payment recording Remove the orphaned tax_setup_eu endpoint left behind by #1826, whitelist draft statuses, guard first issuance to unpaid drafts, snapshot auto-issued funds invoices, and record the paid journal after the admin paid_at override * Harden invoice money paths Manual refunds no longer log, journal, or dispatch as performed: the skip is recorded distinctly. payInvoiceWithCredits returns the real outcome and the batch variant counts covered/skipped/failed with per-invoice warnings. Processed transactions freeze their history fields, refuse deletion, and clean up orphaned balance rows on unprocessed deletes; transaction linkage refuses canceled/refunded/replaced invoices. processInvoice rejects paid invoices, and the expiry cron rechecks deletability under the row lock. rmByClient is documented as intentional erasure and pinned by test. * Remove dead legacy IPN pipeline, serialize transaction processing The legacy process() pipeline (_parseIpnAndApprove, _debit/_refund/_subscribe/ _unsubscribe, amount/currency validation, debitTransaction) had no production callers: live processing runs through gateway adapters via preProcess. Delete ~350 lines of unreachable payment code plus its tests and the now-unused findOneProcessedByTxnId finder, and wire the atomic claimForProcessing into preProcess, createAndProcess, and processAndCatchErrors so concurrent deliveries converge on a single processor. * Expand invoice coverage and finish consistency sweep Number-series fallback paths, non-default currency payment and missing-rate refusal, tax-rate effect on totals, batch activation isolation and batch delete fail-fast, client pay/PDF controller auth matrix, expiry-cron disabled and isolation paths, zero-total and already-paid email skips, and the reissue replacement email variable. Billing-channel debug lines and the prepare instant-issue warning now carry structured context. * Pin no-gap guarantee for rolled-back number reservations * Document journal retention, log base-currency rewrites on issued invoices * Record offline refund documents in manual refund mode Manual mode no longer returns null while the UI reports success: refunding a paid invoice now generates a credit-note-series document marked as an offline refund (note plus journal flag), so money moved outside FOSSBilling leaves a paper trail. Unknown refund-logic values fail loudly instead of silently skipping. * Complete GDPR erasure: anonymize transactions, cascade support cleanup, refuse deletion with active services Detached transactions are anonymized in place on client erasure (IP, raw IPN payloads, notes scrubbed; financial record kept). Support erasure now removes notes, messages, and edit history instead of orphaning them. Client deletion is refused while provisioned service rows exist, forcing explicit cancellation first. Also quotes the reserved table name portably: invoice deletion and the transaction counter/pickup were syntax errors on SQLite. * Add live client-erasure walkthrough test * Bypass rate limiter in development, fix antispam E2E state pollution * Remove stale Invoice draft comments * Fix spellcheck findings: rename UPDATEABLE_FIELDS, reword coinages * Tolerate missing service tables in client-deletion gate * Exercise promo add-remove on drafts in E2E * Apply Rector suggestions in invoice tests * Address PR review: single-claim transactions, frozen seller snapshot, issuance guard * Trim repeated no-reclaim comments in Stripe adapter * Run schema sync before journal backfill on upgrade * Address second review round: claim quoting, patch recovery, bounded journal healing * Serialize journal baseline writes with a per-invoice claim * Surface instant-issue failures instead of returning a silent draft |
||
|
Adam Daley
|
161f1328b8 |
Replace legacy hooks with typed Symfony events (#4400)
* Add typed Symfony events alongside legacy hooks * Move admin login activity to typed event listener * Move client login activity to typed event listener * Add typed events for cron currency and notifications * Add typed extension lifecycle events for widget cache * Avoid duplicate typed listener registration * Add typed events for settings subscriptions and client deletion * Migrate core cron hooks to typed Symfony listeners * Migrate support ticket hooks to typed events * Migrate client and profile lifecycle hooks to typed events * Migrate authentication hooks to typed events * Migrate staff checkout and order lifecycle events * Migrate cart, order batch, invoice, and staff reset hooks * Migrate extension, invoice, system, and service hooks * Retire legacy hook runtime and finish reminder events * Clean up retired hook data during upgrades * Clarify reminder events and complete gateway cancellation * Document current typed event development model * Apply Rector suggestions for typed event changes |
||
|
Adam DaleyandClaude
|
d1d7e69d7a |
Add MySQL/MariaDB/PostgreSQL/SQLite database portability (#4207)
* Add MySQL/MariaDB/PostgreSQL/SQLite database portability
Extends database support beyond MySQL/MariaDB to PostgreSQL and SQLite,
for fresh installs on all four drivers plus a safe upgrade path.
- Driver plumbing: DriverManagerFactory/Requirements/load.php support
pdo_pgsql and pdo_sqlite alongside pdo_mysql, with driver-aware
connection parameter building.
- Runtime raw SQL: portable replacements (RowLock, NamedLock, SqlExpr)
for the ~16 MySQL-only call sites (GET_LOCK, FOR UPDATE, DATE_FORMAT/
DATEDIFF/UNIX_TIMESTAMP, GREATEST) across Stripe, Hook, Stats, Order,
Invoice, Client, and Cart.
- Fresh-install schema: SchemaInstaller/InstallSeeder generate schema
and seed data for PostgreSQL/SQLite from Doctrine entity metadata
(MySQL/MariaDB keeps using install/sql/structure.sql). Renamed 28
Doctrine index names that collided once index scoping stopped being
per-table-only (MySQL) and became per-database (Postgres/SQLite).
Installer UI gained a driver picker with SQLite/network fieldsets.
- Upgrade safety: UpdatePatcher::isMysqlDriver() prevents its ~80
legacy MySQL-only raw-SQL patches from ever running against a
non-MySQL database (a real corruption path if patch-level bookkeeping
is ever missing/stale). SchemaSynchronizer replaces that gap with an
additive-only diff against current entity metadata - new tables,
columns, and indexes only, never drops/alters existing structure, and
never adds a foreign key constraint to an already-existing table
(checked against real MariaDB: FOSSBilling's schema has never had DB-
level FK enforcement, so retrofitting one isn't safe to do blindly).
Runs on every driver now, MySQL/MariaDB included, as a catch-all
after the legacy patches.
Also fixed two latent bugs found along the way: EntityManagerFactory's
module-entity Finder call was silently returning zero paths, and
custom_pages/service_apikey were missing from PostgreSQL/SQLite's
generated schema (their modules create their own table via an
install() hook, invisible to entity-metadata-driven schema builds).
Docs (AGENTS.md) and the installer UI updated to describe current
support accurately: fresh installs on all four drivers, upgrades stay
schema-synced everywhere, but the legacy patches' one-time data
transformations remain MySQL/MariaDB-only.
* Add missing entity indexes found diffing structure.sql vs metadata
Comparing a real MariaDB structure.sql install against a
SchemaInstaller-generated schema (the entity-metadata-driven generator
already used for PostgreSQL/SQLite) surfaced 15 tables where entity
metadata was missing indexes/unique constraints that structure.sql has
had for years, plus two where the annotation was present but silently
broken.
AdminGroupMember and EmailTemplateGroup declared their unique
constraint via Table's $uniqueConstraints constructor param, which
Doctrine ORM 3.6 turned into a documented no-op (see the
Deprecation::trigger() in Doctrine\ORM\Mapping\Table) - it must be a
standalone #[ORM\UniqueConstraint] attribute instead. Verified via
Reflection that the old form produced empty $metadata->table on this
Doctrine version.
The remaining 13 tables get straightforward #[ORM\Index]/
#[ORM\UniqueConstraint] additions matching structure.sql exactly.
Several new index names had to be table-prefixed (unlike
structure.sql's bare names) because SQLite/PostgreSQL require index
names to be unique database-wide, not just per-table like MySQL -
confirmed by the resulting name collisions actually breaking
SchemaInstaller::createSchema() on SQLite.
invoice_item_pending_renewal_idx is deliberately not reproduced:
rel_id is mapped TEXT (unlike structure.sql's varchar(20)), so
matching the index needs a MySQL-only column-length prefix that can't
round-trip through SQLite/PostgreSQL introspection - it made
SchemaSynchronizer::sync() permanently misreport the index as missing
on every sync. Needs rel_id's column type fixed first.
This does not touch MySQL's install path (structure.sql /
install.php) - MySQL still installs from structure.sql unchanged.
These are entity-metadata corrections for the PostgreSQL/SQLite
SchemaInstaller/SchemaSynchronizer path.
Verified against a live MariaDB 10.11 container: re-diffing
structure.sql vs the regenerated metadata schema shows all 15 gaps
closed, with no new gaps introduced. Full Pest suite (2865 tests),
PHPStan, and PHP-CS-Fixer all pass.
* Fix dead cache-invalidation seed in EntityManagerFactory
EntityManagerFactory computed a cacheNamespaceSeed from every entity
file's mtime and size specifically to invalidate stale cached
ClassMetadata when entity attributes change, then passed it to
ORMSetup::createAttributeMetadataConfig() alongside its own
FilesystemAdapter. Doctrine's ORMSetup::createConfig() only ever
consults that seed when it builds a cache instance itself
(createCacheInstance() returns any explicitly-passed $cache
immediately) - so the seed was silently ignored, and the metadata
cache's namespace stayed hardcoded to the literal string 'doctrine'
forever.
Every call path that runs SchemaSynchronizer::sync() happens to wrap
itself in a full PATH_CACHE wipe already (UpdateFinalization::
clearCache(), Update::performManualUpdate()), so that path was
accidentally safe. But the same cache backs every other Doctrine ORM
operation app-wide, with nothing to invalidate it outside those two
call sites - a local entity edit, or a release with no accompanying
UpdatePatcher patch, would silently keep serving pre-edit metadata
indefinitely. Reproduced directly: two separate PHP processes, an
entity index renamed in between, no cache clear - the second process
served the stale name.
Fix: hash the already-computed seed directly into the
FilesystemAdapter's own namespace, reproducing what Doctrine's
internal cache construction would have done. Dropped the
cacheNamespaceSeed argument since it's genuinely inert once an
explicit cache is passed.
Verified the fix closes the exact repro above, and the full Pest
suite (2865 tests), PHPStan, and PHP-CS-Fixer all still pass.
* Fix two more entity/structure.sql mismatches found during the diff
Post.description was mapped VARCHAR(255) while structure.sql has
always had it as TEXT - a real silent-truncation risk on a
metadata-driven install, not just cosmetic column-type noise. Widened
to TEXT to match.
Extension was missing structure.sql's UNIQUE KEY type_name (type,
name) entirely - metadata only had a plain index on type. Added as a
standalone #[ORM\UniqueConstraint], matching
ExtensionRepository::findOneByTypeAndName() and
Service::activateExistingExtension()'s existing check-then-insert
assumption that at most one row exists per (type, name) pair, so this
is a backstop on an invariant the app already enforces, not a
behavior change.
Left InvoiceItem.rel_id (mapped TEXT vs structure.sql's varchar(20))
alone, despite it looking like the same class of bug: ServiceInvoiceItem
::process() json_decode()s rel_id for InvoiceItem::TYPE_HOOK_CALL items,
storing arbitrary event params there - narrowing to varchar(20) would
silently truncate that JSON. That code path has no live caller today
(only exercised by tests), but "safe, mechanical" doesn't apply once
matching structure.sql would regress a real, if dormant, feature -
structure.sql's varchar(20) looks like the one that's actually wrong
here. invoice_item_pending_renewal_idx stays unreproduced as a result.
Verified against a live MariaDB 10.11 container: re-diffing
structure.sql vs regenerated metadata shows both gaps closed, no new
ones introduced. Full Pest suite (2865 tests), PHPStan, and
PHP-CS-Fixer all pass.
* Remove dead InvoiceItem::TYPE_HOOK_CALL feature
No code creates an invoice item with this type anymore (only tests
did, via createEntity(InvoiceItem::class, ['type' => TYPE_HOOK_CALL,
...])) - ServiceInvoiceItem::process()'s handling branch, which
json_decode()s rel_id and fires it as an event, was already dead
weight. Removed the constant and its process() branch.
Correction to my own prior investigation: I'd claimed rel_id was
mapped TEXT "unlike structure.sql's varchar(20)" and that narrowing
it to match would truncate hook-call JSON payloads. That premise was
wrong - the varchar(20) rel_id column in structure.sql belongs to the
unrelated client_balance table; invoice_item.rel_id has always been
TEXT there too. rel_id is left as TEXT, unchanged. The removal here
stands on its own merits (genuinely dead code, no live caller) and is
unrelated to that mixup.
invoice_item_pending_renewal_idx remains unreproduced regardless -
structure.sql needs a MySQL-only column-length index prefix to index
a TEXT column at all (`rel_id(20)` there is an index-key-length
workaround, not a real 20-character data limit), and that prefix
option can't round-trip through SQLite/PostgreSQL introspection
without permanently confusing SchemaSynchronizer::sync(). Updated the
entity's comment to reflect the corrected reasoning.
Rewrote the two attempt-cap regression tests that used
TYPE_HOOK_CALL as their failure vehicle to use the still-live
TYPE_ORDER/order-not-found failure path instead, preserving coverage
of the retry/attempt-cap behavior itself.
Full Pest suite (2864 tests), PHPStan, and PHP-CS-Fixer all pass.
* Fix remaining lower-stakes entity/structure.sql mismatches
Widened three narrower-than-structure.sql columns: Post.status
(varchar(20) -> varchar(30)), Post.section (varchar(100) ->
varchar(255)), ServiceHostingServer.maxAccounts and Product.priority
(both INTEGER -> BIGINT, matching structure.sql's bigint(20) -
ordinary business columns, not id/FK columns, so outside the scope
of the earlier BIGINT id/FK fix).
Added the DB-level defaults structure.sql already has for
Helpdesk.closeAfter (24) and PromoRedemption.phase/status
('checkout'/'reserved') - the app already sets these explicitly on
every insert it makes, so this only matters for a raw insert or an
additive SchemaSynchronizer column-backfill bypassing the entity, not
a behavior change today.
Left two things alone after checking them:
- admin.salt is unmapped in the entity and stays that way - nothing
in the codebase reads or writes it (Config::getProperty('info.salt')
is an unrelated app-wide config value, not this per-admin column);
it looks like dead weight from a pre-password_hash() auth scheme,
and re-adding it to the entity would reintroduce legacy debt rather
than fix anything.
- email_queue.tries/status already declare DB-level defaults
(0/'pending') that structure.sql's NOT NULL columns don't have.
That's the entity being more complete than structure.sql, not a bug
to "fix" by removing them.
Verified against a live MariaDB 10.11 container: re-diffing
structure.sql vs regenerated metadata shows all of the above closed;
remaining differences are cosmetic (TEXT/LONGTEXT length
categorization, decimal default formatting) or previously-known/
out-of-scope (invoice_item_pending_renewal_idx, the cart/extension
1c items). Full Pest suite (2864 tests), PHPStan, and PHP-CS-Fixer
all pass.
* Add invoice_item_pending_renewal_idx with its MySQL length prefix
Reproduces structure.sql's KEY invoice_item_pending_renewal_idx
(rel_id(20), type, task, status, invoice_id) via Doctrine's Index
`options: ['lengths' => [20, null, null, null, null]]`. The prefix is
required on MySQL/MariaDB because InnoDB can't index a full TEXT
column at all - confirmed matching structure.sql's exact DDL byte for
byte against a live MariaDB 10.11 container.
SQLite/PostgreSQL ignore the `lengths` option entirely
(AbstractPlatform::supportsColumnLengthIndexes() is false there) and
apply the full, unprefixed 5-column index - confirmed directly via a
throwaway test against a real SQLite connection, so nothing is
actually narrower there. The one real cost: that option can't
round-trip through SQLite/PostgreSQL schema introspection, so
SchemaSynchronizer::sync() permanently reports this one index as an
"existing structural difference" even against an otherwise fully-
current database. Harmless (skipped items are never applied), but it
means a nonzero skip count on those platforms is no longer sufficient
evidence of real drift on its own.
Updated the two SchemaSynchronizerTest cases that asserted an empty
skip list on a freshly-installed, fully-current schema to expect this
one specific, documented, permanent skip instead.
Full Pest suite (2864 tests), PHPStan, and PHP-CS-Fixer all pass.
* Drop the dead admin.salt column (patch112)
Adam confirmed dropping it for real after reviewing that nothing
reads or writes it: Config::getProperty('info.salt') is an unrelated
app-wide config value, not this per-admin column, and the Admin
entity has never mapped it. Looks like leftover storage from a
pre-password_hash() auth scheme.
- structure.sql: removed the column from the admin table for fresh
installs.
- UpdatePatcher::patch112(): ALTER TABLE admin DROP COLUMN salt for
every upgrading MySQL install, guarded on tableHasColumn() so it's
a no-op if already applied.
- content.sql: bumped last_patch to 112 to match.
Verified end-to-end against a live MariaDB 10.11 container: a fresh
install from the updated structure.sql has no salt column; running
patch112 against a database loaded from the *previous* structure.sql
(still has salt) drops it correctly via a real PDO connection, and
re-running the patch afterward is a safe no-op.
Added three regression tests mirroring the existing patch111/patch97
coverage style: patch number registration, no-op when the column is
already gone, and the drop itself when it still exists.
Full Pest suite (2867 tests), PHPStan, and PHP-CS-Fixer all pass.
* Enforce unique cart.session_id at the DB level (patch113)
Resolves the last 1c item from the unification scoping: the Cart
entity has always declared session_id_idx as a UNIQUE constraint
(matches CartRepository::findBySessionId()'s existing assumption of
at most one cart per session), but structure.sql has only ever had a
plain index. Adam chose to add the real constraint after reviewing
the tradeoff against leaving it metadata-only.
Follows the same dedup-then-constrain pattern already established by
patch102 (custom_pages.slug): before adding the unique index, find
duplicate session_ids and delete every row but the highest-id
(most recently created) one per group, along with their now-orphaned
cart_product rows (cart_product.cart_id has no DB-level foreign key,
so those would otherwise become dangling references). NULL
session_id rows are left untouched - MySQL treats multiple NULLs as
distinct under a UNIQUE index.
structure.sql: cart.session_id_idx is now a UNIQUE KEY for fresh
installs. content.sql: bumped last_patch to 113.
Verified end-to-end against a live MariaDB 10.11 container with
realistic duplicate data (two session_id groups with 3 and 2
duplicate carts respectively, cart_product children on several of
them, plus untouched unique and NULL-session_id rows): patch113
correctly kept the newest cart per group, cascade-deleted the
orphaned cart_product rows with zero remaining orphans, and converted
the index to UNIQUE - confirmed by direct SHOW INDEX/SELECT
inspection, not just a not-null assertion. Re-running the patch
afterward is a safe no-op. A separate fresh install from the updated
structure.sql gets the UNIQUE index directly.
3 new regression tests added (patch numbering, no-op when already
unique, and the reconciliation-then-convert path). Full Pest suite
(2870 tests), PHPStan, and PHP-CS-Fixer all pass.
* Clean up admin_password_reset rows when deleting an admin
Found by the FK-safety audit for the MySQL SchemaInstaller
unification question: admin_password_reset.admin_id would be a real
FK constraint if MySQL ever adopted the entity-metadata-driven schema
generator, but Staff\Service::delete(Admin) never touched those rows
- only the existing admin_group_member cleanup ran. Confirmed against
a live MariaDB 10.11 container with real FK enforcement that this
delete throws today under that constraint.
Added AdminPasswordResetRepository::deleteResetsForAdmin(), mirroring
the existing deleteMembershipsForAdmin() pattern on
AdminGroupMemberRepository, and call it in the same transaction
before removing the admin. This is a real fix regardless of the
unification decision - right now, without FK enforcement, this same
path already silently orphans admin_password_reset rows on every
admin deletion.
Updated the shared test container() helper and the Staff module's own
staffEntityManager() test double to mock the new repository lookup
setDi() now performs. Verified end-to-end against live MariaDB: the
delete now succeeds where it previously threw a real foreign key
violation. New regression coverage on the existing "delete removes
admin account" test asserting deleteResetsForAdmin() is called.
* Clean up client_order_meta rows when deleting an order directly
Found by the same FK-safety audit: client_order_meta.client_order_id
would be a real FK constraint under MySQL SchemaInstaller, but
Order\Service::deleteFromOrder() (the direct/admin single-order
delete path) never removed the order's meta rows - only the
client-cascade path, rmByClient(), already did via the same
getOrderMetaRepository()->deleteByOrderId() call used here. Confirmed
against a live MariaDB 10.11 container with real FK enforcement that
this delete throws today under that constraint.
This is a real fix regardless of the unification decision - right
now, without FK enforcement, deleteFromOrder() already silently
orphans client_order_meta rows every time an order is deleted
directly rather than via a client deletion.
Verified end-to-end against live MariaDB. New regression test
asserting getOrderMetaRepository()->deleteByOrderId() is called with
the order's id during deleteFromOrder().
* Guard payment gateway deletion; detach transactions on invoice removal
Found by the FK-safety audit: 3 of the 6 risky relationships all
stemmed from one gap - ServicePayGateway::delete() had zero usage
checks before removing a gateway, while invoice.gateway_id,
subscription.pay_gateway_id, and transaction.gateway_id would all be
real FK constraints under MySQL SchemaInstaller. A 4th, separate gap:
Invoice\Service::rmInvoice() already nulls client_order.
unpaid_invoice_id before removing an invoice, but never touched
transaction.invoice_id, which would also be a real FK constraint.
Confirmed all 4 against a live MariaDB 10.11 container with real FK
enforcement - each currently throws under that constraint, and this
is a real fix regardless of the unification decision (right now,
without FK enforcement, rmInvoice() already silently orphans
transaction rows).
ServicePayGateway::delete() now blocks (InformationException,
matching the existing "cannot remove X with existing Y" pattern used
elsewhere - e.g. Product::removeProductCategory()) when any invoice,
subscription, or transaction still references the gateway, via new
InvoiceRepository/SubscriptionRepository/TransactionRepository::
existsByGatewayId() methods.
rmInvoice() detaches (not deletes) transactions from the removed
invoice via a new TransactionRepository::detachFromInvoice() -
transaction.invoice_id is nullable, and a transaction is a real
record of a payment attempt/event that shouldn't be destroyed just
because the invoice it once pointed to was, same reasoning as the
existing client_order.unpaid_invoice_id null-out a few lines above it.
Verified end-to-end against live MariaDB: all 4 previously-failing
deletes now succeed (transaction row survives detached with
invoice_id = NULL; pay_gateway deletion correctly blocks while any
dependent row exists and succeeds once they're all cleared). New
regression tests for the guard's three block conditions and the
successful-delete path, plus detachFromInvoice() coverage on the
existing "removes an invoice" test.
Full Pest suite (2874 tests), PHPStan, and PHP-CS-Fixer all pass
throughout this round.
* Make custom_pages/mod_massmailer/service_apikey install() portable
Resolves the extension-module-eagerness question from the unification
scoping, reframed: SchemaInstaller's eagerness (creating every
entity's table on install, active or not) isn't actually the bug -
it's an accidental safety net currently masking a real one. All three
of these modules' own install() hooks ran raw MySQL-only DDL
(backtick identifiers, ENGINE=InnoDB) directly against the
connection, which fails outright on PostgreSQL/SQLite. That never
surfaced because the table already existed by the time install() ran
(SchemaInstaller creates it eagerly, or - for mod_massmailer /
service_apikey - structure.sql/patch111 already did on MySQL), making
the broken SQL a silent no-op everywhere it's actually been tested.
Confirmed directly against a live PostgreSQL 16 container: the old
raw SQL fails immediately with a syntax error on the backtick
identifiers.
Building a "core tables only" filter into SchemaInstaller (the
originally-proposed fix) would have made this worse, not better - it
would remove the safety net and immediately expose these three latent
bugs the moment someone activates one of these extensions on a
non-MySQL platform.
Replaced each install() body with a call to
FOSSBilling\Doctrine\SchemaSynchronizer::sync($di['em']) - the same
portable, additive, entity-metadata-driven mechanism
UpdatePatcher::applyCorePatches() already uses on every request. This
makes activation portable on all three platforms and removes any
reason to build a core-tables filter at all.
Verified end-to-end:
- Real MariaDB 10.11 loaded from the current structure.sql (an
"existing install"): sync() correctly creates only the genuinely
missing custom_pages table (matching the entity, not the old raw
SQL - e.g. id widened to BIGINT, consistent with this branch's
earlier BIGINT convention work) and leaves mod_massmailer/
service_apikey untouched since they already exist via
structure.sql/patch111.
- Real PostgreSQL 16, empty database: sync() creates the full schema
including all three target tables with correct, entity-matching
columns - confirmed via \d custom_pages.
- New unit tests for each module's install() against a real SQLite
connection, confirming table creation (and, for Massmailer, that
the post-creation setConfig() call still happens with the same
arguments).
Full Pest suite (2877 tests), PHPStan, and PHP-CS-Fixer all pass.
* Renumber cart.session_id patch to 114 after rebase collision
Rebasing onto latest main pulled in patch112 (require_transfer_code,
#4200), which this branch's admin.salt drop already used - resolved
during the rebase by shifting it to patch113. That shift collided in
turn with this branch's own patch113 (cart.session_id_idx unique
constraint), which now moves to patch114. Same collision pattern as
#4188/#4189/#4192 - two branches independently claiming the next free
patch number.
Updates the getPatches() dispatch table, content.sql's last_patch
literal, and the corresponding UpdatePatcherTest expectations.
* Fix content.sql seed order for FK-mapped category/registrar tables
Four tables' INSERT blocks were ordered child-before-parent (product
before product_category, tld before tld_registrar, support_pr before
support_pr_category, support_kb_article before its category table) -
an artifact of content.sql being a mostly-alphabetical mysqldump, where
each of these parent tables' name happens to alphabetically sort after
its own child.
This was always latent: MySQL's structure.sql has never had real FK
constraints, so raw-PDO installs never noticed, and content.sql's own
mysqldump header/footer (SET FOREIGN_KEY_CHECKS=0/@OLD...) - which
would have masked it even if it had - is unconditionally stripped by
InstallSeeder::portableize() as mysqldump bookkeeping. Confirmed this
already silently depended on FK enforcement being off: replaying the
pre-fix file through a real SQLite connection with `PRAGMA foreign_keys
= ON` reproduces the same constraint violation a live MariaDB/
PostgreSQL run does.
Reordering the four blocks (parent before child, pure data movement,
no row content changed) fixes it identically on every platform. Adds
a permanent regression test exercising InstallSeeder::seedContent()
against a real FK-enforcing SQLite connection.
* Unify MySQL fresh installs onto SchemaInstaller
MySQL/MariaDB fresh installs no longer replay install/sql/structure.sql
and content.sql almost verbatim via raw PDO. install.php's install()
now runs every driver - MySQL/MariaDB included - through the same
entity-metadata-driven path PostgreSQL/SQLite already used
(SchemaInstaller::createSchema() + InstallSeeder), removing
installMysql() and its raw structure.sql read entirely.
connectMysql() keeps its original raw-PDO pre-flight work (session
charset settings, CREATE DATABASE, since DriverManagerFactory's
connection targets an already-existing dbname rather than creating
one) but now also opens the portable DriverManagerFactory connection
the other two drivers already used, so installPortable() has a
Doctrine DBAL Connection to work with on every driver.
This was the last piece of the DB-portability unification work: prior
rounds already reconciled every entity/structure.sql mismatch, closed
the real FK-safety gaps a live-database audit found (admin_password_
reset, client_order_meta, gateway-referencing invoice/subscription/
transaction rows, transaction/invoice detachment), and made the three
MySQL-only module install() hooks portable. Chose to accept real FK
enforcement on MySQL going forward (matching what PostgreSQL/SQLite
installs already got from SchemaInstaller) rather than suppress it,
now that those gaps are closed.
install/sql/structure.sql is unchanged and remains in place - not as
the fresh-install source anymore, but as the frozen schema definition
UpdatePatcher's legacy MySQL patches still assume for pre-cutover
installs upgrading.
Verified end-to-end against a live MariaDB 10.11 container: schema
creation (62 tables), portable content.sql seeding, admin/currency/
install-nudge seeding, last_patch landing at the latest patch level
immediately (matching the existing PostgreSQL/SQLite fresh-install
behavior), 31 real FK constraints now present and enforced, and a
first-request applyCorePatches() call confirmed as a clean no-op
(no legacy patches run, no schema change attempted). SQLite verified
unaffected. Full Pest suite (2909 tests), PHPStan, and PHP-CS-Fixer
all pass.
A pre-existing, unrelated bug was found incidentally while verifying
this against live PostgreSQL: content.sql's MySQL-style integer
literals (1/0) fail against PostgreSQL's real boolean columns,
breaking fresh PostgreSQL installs at the very first seeded row. Not
fixed here - flagged separately, out of scope for this MySQL-specific
change.
* Fix PostgreSQL boolean literals and sequence desync in InstallSeeder
content.sql (mysqldump output) seeds every boolean-mapped column as a
bare 0/1 literal - harmless on MySQL (tinyint stands in for boolean)
and SQLite (no real boolean type), but rejected outright by
PostgreSQL's genuine boolean columns (SQLSTATE 42804: "column
"protected" is of type boolean but expression is of type integer").
This broke every fresh PostgreSQL install at the very first content.sql
row.
InstallSeeder::portableizeBooleans() now rewrites 0/1 to false/true,
but only at column positions Doctrine's own entity metadata - the same
metadata SchemaInstaller already uses to build the schema - says are
genuinely Types::BOOLEAN for that table. A real integer column that
happens to also hold a 0 or 1 (e.g. product.quantity_in_stock) is left
untouched. This needed an EntityManagerInterface threaded into
seedContent(); install.php's installPortable() call site is updated to
pass the one it already builds.
Also fixed a related but distinct content.sql bug found while
auditing: setting row 31 (hide_company_public) had an unquoted 1 in
its TEXT-typed value column - every other setting row correctly quotes
its value. PostgreSQL rejects that too.
Verifying the boolean fix against a live PostgreSQL 16 container
surfaced a second, independent PostgreSQL-only bug: content.sql's
explicit primary-key inserts desync every seeded table's real Postgres
sequence. MySQL's AUTO_INCREMENT and SQLite's rowid both auto-advance
past an explicit insert; a Postgres sequence does not, so it stays at
1 while the table already holds ids up to content.sql's highest seeded
id. Left alone, the very next auto-generated insert into a seeded
table - including this same install's own seedAdmin()/
seedInstallNudge() calls, or simply the first row a client creates
afterwards - collides with a row content.sql already seeded. Fixed via
InstallSeeder::resyncPostgresSequences(): after seeding, setval()s
every seeded table's sequence to MAX(id), the same step pg_dump's own
--data-only restores append; a no-op on MySQL/SQLite.
Verified end-to-end against a live PostgreSQL 16 container: a full
install (schema + content + admin + currency + install-nudge)
completes cleanly, boolean columns hold true/false, and a genuine
post-install insert lands past content.sql's highest id. Confirmed
both new tests actually fail with the original errors when the fix is
reverted, before restoring it.
Adds regression tests to InstallSeederTest.php: a SQLite spot-check
for the boolean rewrite (proving real integer columns are left
untouched), and two live-PostgreSQL tests (boolean end-to-end and
sequence resync) that skip gracefully via FOSSBILLING_TEST_PGSQL_DSN
when no server is reachable, matching this project's lack of a live-PG
CI lane.
* Add technical terms to cspell dictionary
cutover, mysqldump/mysqldump's, portableize, setval, and nextval are
all genuine terms this branch's install.php/InstallSeeder.php/
InstallSeederTest.php use (mysqldump's own terminology, and the
PostgreSQL functions InstallSeeder now calls) - flagged as unknown
words by the Spellcheck CI check since content.sql itself is
ignore-path'd but these three PHP files aren't.
* Fix fresh installs failing when CacheFactory can't read config.php yet
CI's Live Tests / Playwright Tests checks both failed on this PR with
a 500 from POST /install/install.php?a=install: "The FOSSBilling
configuration file is empty or invalid."
Root cause: since the MySQL-cutover commit, install() runs every
driver through installPortable(), which calls
EntityManagerFactory::create() to build schema/seed data - and that
now always reaches CacheFactory::create() for the Doctrine metadata
cache pool. config.php is only written at the very end of install(),
so this runs with no config file on disk yet on every fresh install.
Config::getProperty() throws a plain \RuntimeException in that case,
which CacheFactory::create()'s catch block didn't catch (it only
caught its own FOSSBilling\Exception), even though this method's own
docblock promises it never throws.
This isn't new to this PR's changes - PostgreSQL/SQLite installs
already took this same path before the MySQL cutover, so it was
already latent there. It only surfaced now because this PR's own Live
Tests/Playwright Tests coverage is the first thing to actually run a
real end-to-end install through the HTTP entry point rather than
calling SchemaInstaller/InstallSeeder directly against an
already-configured local dev environment.
Fixed by broadening CacheFactory::create()'s catch to \Throwable,
matching what its docblock already promised. Added a regression test
that removes config.php entirely and confirms create() still returns
a working filesystem cache pool instead of throwing.
Verified end-to-end against real MariaDB 12.3 and PostgreSQL 16
containers: a full fresh install via the actual install.php HTTP
endpoint (previously reproducing the exact CI failure) now completes
successfully on both, confirmed by reverting the fix and reproducing
the identical error message and stack trace CI reported before
restoring it.
* Address CodeRabbit review: fix real cross-database bugs, defer 2 as follow-up
CodeRabbit's review of this branch flagged 17 threaded findings plus 3
nitpicks. Verified every one against actual current code (via parallel
investigation, not the review's own line numbers/claims) before
touching anything; fixed 15, found 1 already resolved (stale finding),
and deliberately deferred 2 SQLite-concurrency findings that need a
real design decision rather than a quick patch (spawned as a separate
follow-up task).
Real cross-database correctness bugs fixed (same class of bug as the
PostgreSQL boolean/sequence fix this branch already shipped - a raw
SQL literal or MySQL-only construct that MySQL/SQLite tolerate but
PostgreSQL rejects outright):
- Invoice::Service.php, Index::Service.php, Client::Service.php, and
Stats::Service.php all compared/set a Types::BOOLEAN column using
`= 1`/`SET ... = 1` instead of `= true` - 9 sites total across 4
files, including 4 the review didn't list (found by grepping the
whole codebase for the same pattern once the first 5 were
confirmed).
- Order::Service.php's getSoonExpiringActiveOrdersQuery() attached a
non-aggregate WHERE-style predicate via HAVING instead of AND -
PostgreSQL rejects a HAVING clause referencing an ungrouped column
with no GROUP BY; MySQL/SQLite are far more permissive about it.
- Stats::Service.php's getRefunds/getIncome/getTableStats used
SUBSTR(datetimeColumn, 1, 10) to truncate to a date - PostgreSQL has
no implicit timestamp-to-text cast for SUBSTR's argument. Added
SqlExpr::dateOnly() (matching the class's existing portable-SQL-
fragment pattern) and used it at all three sites.
- Serviceapikey::Service.php's uninstall() still had a MySQL-only
backtick-quoted `DROP TABLE IF EXISTS \`service_apikey\`` - the
install() hook right above it was already made portable in an
earlier round, uninstall() was missed.
- NamedLock's PostgreSQL advisory-lock check did `(bool)
$connection->fetchOne(...)` - PDO's pgsql driver returns a boolean
column as the literal string 't'/'f', and `(bool) 'f'` is true (any
non-empty PHP string is truthy), so every lock attempt reported
success on the first poll regardless of whether the lock was
actually free.
Other real bugs fixed, unrelated to database portability:
- InstallSeeder::seedContent() unescaped mysqldump's `\n`/`\"`
sequences on the whole file *before* splitting into statements - a
seeded string ending in a literal `;\n` (backslash-n, two
characters) would become a real `;<newline>` first and get mistaken
for a statement boundary, splitting the INSERT into two garbage
fragments. Reordered to split first (on the still-escaped text, so
no false boundary can appear), then unescape and rewrite booleans
per statement. Confirmed the old order actually breaks (two garbage
fragments) before restoring the fix.
- CacheFactory::clearAll()'s fixed namespace list could never reach
the Doctrine metadata cache pool, which EntityManagerFactory stores
under a namespace hashed from current entity files (by design, so it
self-invalidates on an entity change) - harmless for the filesystem
backend (the PATH_CACHE wipe alongside every clearAll() call already
covers it) but a real gap for Redis/Memcached, where the old
namespace's keys are orphaned forever after every entity-file
change. Exposed EntityManagerFactory::metadataCacheNamespace() and
clear it explicitly at both clearAll() call sites
(Config::setConfig(), System::Service::clearCache()).
- SchemaSynchronizer never carried Doctrine's own `renamedIndexes`
diff through to the schema it actually applies - so on an existing
MySQL install still running structure.sql's legacy short index names
(invoice_id_idx, client_id_idx, admin_id_idx - across 7 entities),
the table-prefixed name several entities were renamed to in an
earlier round is silently never created, on every sync, forever.
Doctrine's comparator represents an unambiguous rename as
getRenamedIndexes(), separate from getAddedIndexes()/
getDroppedIndexes() - unlike a real drop, applying it validates
nothing against existing rows, so it needed no new safety guard.
- Invoice::rmInvoice() ran 4 separate implicit units of work (2 raw
auto-committed UPDATEs, then 2 independent flush() calls) - if the
final flush (removing the invoice) failed, transactions were already
permanently detached from an invoice that was never actually
deleted. Wrapped the whole method in $entityManager->wrapInTransaction().
- Support\Service::helpdeskCreate() passed `null` to setCloseAfter()
when the caller omitted close_after (reachable via the API directly,
even though the stock admin UI form always sends a value) - Doctrine
always includes a mapped column in its INSERT, so this wrote a
literal NULL and bypassed the column's own DB-level default (24),
silently excluding that helpdesk's tickets from ever being
auto-closed by findExpiredOnHold() (NULL propagates through
SqlExpr::addHours()). Added Helpdesk::DEFAULT_CLOSE_AFTER_HOURS as
the single source of truth for both the DB default and this
fallback.
- load.php's hasDatabaseTables() treated an in-memory SQLite
configuration (`db.memory = true`, a real documented config.php
option, distinct from the installer's own SQLite output which never
produces it) as "already installed", since buildDatabaseProbeDsn()
returns null for it and null was otherwise read as "assume
installed" - the opposite of what's true for a database that's
always empty on every process start.
Test-quality nitpicks fixed:
- Client::ServiceTest.php's getSearchQuery test used array_diff_key()
to compare bound params, which only catches extra keys, never wrong
values or missing keys - switched to a direct equality assertion.
- RequirementsTest.php's "does not fail overall" test never actually
called checkCompat() or constructed Requirements, just re-checked
extension_loaded() directly - now asserts against checkCompat()'s
real return value.
- Three test files used native unlink()/is_file()/file_get_contents()
instead of this project's own Filesystem/Path convention
(LoadHasDatabaseTablesTest.php, DriverManagerFactoryTest.php,
InstallSeederTest.php).
- AGENTS.md said "all four drivers" while listing three
(pdo_mysql/pdo_pgsql/pdo_sqlite) - also caught and fixed the rest of
that same paragraph being stale from before this branch's MySQL
cutover (still described MySQL as installing from structure.sql).
One finding (ServiceInvoiceItemTest.php's attempt-cap assertion) was
already fixed in the code the review analyzed - false positive, no
change needed.
Deliberately deferred, spawned as a separate follow-up task rather
than fixed here (both need a real design decision, not a quick
patch):
- NamedLock::acquire() is a safe no-op on SQLite for every caller that
wraps its whole guarded section in one transaction (confirmed: all
RowLock::suffix() callers, Hook::batchConnect()) - but not for
Stripe::withStripeObjectLock(), whose critical section spans
multiple flushes and an external API call. Needs a real
cross-process lock (most likely flock()-based) for SQLite.
- System::Service::reserveNumericParamValue()'s invoice-number counter
can race under SQLite's deferred-transaction locking (RowLock's
FOR UPDATE is a no-op there) - worse than an uncaught exception, it
can silently write duplicate counter values. Needs SQLite's
transaction driven manually via BEGIN IMMEDIATE instead of Doctrine's
transactional() wrapper, verified under real concurrent access.
Full Pest suite (2923 tests, plus the 2 live-PostgreSQL-only tests
that skip without a reachable server), PHPStan, php-cs-fixer, and
cspell all pass.
* Add typed class constants Rector's AddTypeToConstRector now requires
CI's new diff-scoped "Rector & PHP-CS-Fixer" check (added by #4206,
merged into main and picked up by this branch's latest main-merge)
runs rector:check against every PHP file this whole PR's diff touches
- not just this branch's own recent commits - and caught two
pre-existing class constants from this branch's earlier rounds that
predate AddTypeToConstRector's requirement:
- DriverManagerFactory::DEFAULT_PORTS -> private const array
- NamedLock::POLL_INTERVAL_MICROSECONDS -> private const int
Applied via `composer rector:fix`. rector:check, php-cs-fixer, and
PHPStan all clean on both files; DriverManagerFactoryTest.php and
NamedLockTest.php (31 tests) still pass.
* Fix SQLite concurrency gaps flagged by CodeRabbit on PR #4207
Two related races were confirmed real but deliberately left unfixed in
#4207 pending a design decision:
1. NamedLock::acquire()/release() were a silent no-op on SQLite. That's
fine for callers whose whole guarded section is one transaction
(SQLite's write lock covers them), but not for
Stripe::withStripeObjectLock(), which spans several separate
flushes plus an external Stripe API call. On SQLite this let two
concurrent webhook/redirect deliveries both pass the dedup check
and create duplicate transaction rows.
Fixed with a real cross-process lock: flock() on a zero-byte file
under PATH_DATA/locks, one per lock name, polled with
LOCK_EX|LOCK_NB the same way the existing PostgreSQL path polls
pg_try_advisory_lock(). Lock files are deliberately never deleted
(unlinking under a held/about-to-be-opened lock is a TOCTOU race);
the footprint is bounded by the number of distinct lock names an
install ever uses.
2. System\Service::reserveNumericParamValue() wrapped a read-then-write
counter increment in a plain $connection->transactional(), relying
on RowLock::suffix()'s `FOR UPDATE` - a no-op on SQLite. PDO's
sqlite driver only ever issues a deferred BEGIN, so two concurrent
transactions could both pass the read under a shared lock before
either took a write lock, letting the second writer overwrite the
first's increment with a stale value (silent duplicate invoice
numbers), or surface as an uncaught LockWaitTimeoutException.
Fixed by driving the transaction with a raw BEGIN IMMEDIATE on
SQLite instead, taking the write lock upfront like FOR UPDATE does
elsewhere; reserveNextNumericParamValue()'s existing single retry
now also catches LockWaitTimeoutException.
Verified live: a real multi-process stress test (6 child PHP
processes, real on-disk SQLite file) hands out exactly the expected
contiguous range with no duplicates against the fix, and reproduces
the pre-fix crash/race when run against the old code.
🤖 Generated with Claude Code
Co-Authored-By: Claude <noreply@anthropic.com>
* Add SQLite-locking terms to cspell dictionary
namedlock (from the fb-namedlock-test-lock fixture string literal) and
entrancy (from re-entrancy, split at the hyphen) - both new in the
flock()-based SQLite locking follow-up.
* Address second CodeRabbit review pass: 3 real fixes, 1 false positive
- CacheFactory::create(metadataCacheNamespace())->clear() was called
unguarded at both call sites (Config::setConfig(), System::Service::
clearCache()), added in the previous review-response commit -
create() already falls back safely for a misconfigured/unreachable
backend, but clear() itself can still fail after that (e.g. a Redis
ACL that permits get/set but not the flush command), which would
make config persistence/cache clearing look like it failed even
though the actual work (writing config.php, wiping the filesystem
cache) had already succeeded. Extracted CacheFactory::clearNamespace()
- the same per-namespace try/catch clearAll() already uses internally
- and call it from both sites instead of duplicating the guard.
- ReserveNumericParamValueConcurrencyTest.php spawned worker processes
via a bare 'php' name resolved through PATH, which can point at a
different interpreter (or none at all) than the one actually running
the test - switched to PHP_BINARY.
- RequirementsTest.php's driver-check test also asserted the aggregate
can_install flag, which folds in unrelated requirements (folder
permissions, other extensions, PHP version) - narrowed to just the
driver-loaded assertion the test is actually named for.
One finding was a false positive: the review claimed
Doctrine\DBAL\Exception\LockWaitTimeoutException is a final class,
making it unmockable with Mockery - checked the actual installed
source (doctrine/dbal 4.4.4, per composer.lock): neither
LockWaitTimeoutException nor its ServerException parent is declared
final, and the existing test mocking it already passes.
Full Pest suite (2973 tests), Rector, PHPStan, php-cs-fixer, and
cspell all pass.
* Address third CodeRabbit review pass: 1 real test fix, 1 documented gap
- RequirementsTest.php's driver-check test only proved extension
detection, not that checkCompat() actually leaves can_install
successful when a driver is loaded - a regression in that decision
could still pass. Requirements::$writable/$php_reqs are public,
mutable properties, so isolating the driver rule from every other
requirement (folder/file writability, other extensions) is real
isolation, not a mock standing in for the real check - emptied both
before calling checkCompat() and asserted can_install again.
- System::Service.php's reserveNumericParamValue(): confirmed real but
not currently reachable - if the connection already has an open
transaction (nesting level > 0), the SQLite BEGIN IMMEDIATE branch is
skipped in favor of a plain transactional() savepoint, reopening the
exact race this method exists to close. Traced every current caller
(Invoice::Service.php's 3 call sites, all via getNextInvoiceNumber())
and confirmed none reach this with an open transaction - each runs
after its own flush() has already committed and closed one. Given
this reserves invoice numbers and a correct fix needs either forcing
an early real write (SQLite escalates its lock on the first write at
any nesting depth) or a different atomic mechanism entirely,
documented the gap in place with a comment rather than rushing a fix
without dedicated review - spawned as a follow-up task.
Full Pest suite (2973 tests), Rector, PHPStan, php-cs-fixer, and
cspell all pass.
* Fix reserveNumericParamValue's SQLite nested-transaction lock gap
reserveNumericParamValue() only drove a raw BEGIN IMMEDIATE to take
SQLite's write lock upfront when the connection had no transaction
already open. A caller wrapping the call in its own outer transaction
fell through to transactional()'s plain SAVEPOINT-based nesting, which
takes no lock until the first write - reopening the exact race
(two concurrent readers passing under a shared lock before either
takes a write lock) BEGIN IMMEDIATE existed to close. Not reachable by
any current caller, but flagged by CodeRabbit on PR #4207 as a real
gap in a correctness-critical, money-adjacent path (invoice numbers).
SQLite escalates a transaction's lock to RESERVED on its first write
statement regardless of nesting depth - a SAVEPOINT shares the one
connection-wide transaction's lock state rather than having its own.
So doReserveNumericParamValue() now issues a lock-escalating no-op
UPDATE (0 rows affected whenever the counter doesn't exist yet)
before the read, uniformly, and reserveNumericParamValue() always
goes through transactional() - no more nesting-level branching or
manual BEGIN IMMEDIATE/COMMIT/ROLLBACK.
Verified the RESERVED-escalation mechanism against real SQLite (both
top-level and SAVEPOINT-nested, plus a negative control) before
committing to this approach. Added a second multi-process concurrency
test alongside the existing top-level one, exercising the nested case
by having each worker open its own outer transaction around every
reservation; confirmed it fails against the old nesting-gated logic
before restoring the fix. Updated the mocked reserveNextNumericParamValue
unit tests in ServiceTest.php to match the new unconditional flow.
* Bound reserveNextNumericParamValue's SQLite retry, extract concurrency test harness
CodeRabbit flagged that reserveNextNumericParamValue() only retried a
failed reservation once. On MySQL/PostgreSQL that single retry is always
enough - two callers racing to seed a new counter both pass the locking
read (InnoDB gap locks are shared) and collide on the insert instead, so
the loser's one retry finds the winner's row already there and there's no
further contention. SQLite is different: DriverManagerFactory's pdo_sqlite
connection sets no busy timeout, so the lock-escalating write fails
outright (non-blocking) rather than waiting. With only two or three
concurrent writers a single retry is still typically enough, but with
more than that, the retry itself can land in the same instant as another
failed attempt and collide again.
Replace the single try/catch retry with a bounded loop (up to 5 attempts,
with a short random backoff between them) so contention beyond a single
collision keeps retrying instead of surfacing to the caller.
Added two regression tests to ServiceTest.php covering the new behavior:
one where three attempts fail before the fourth succeeds, and one where
every attempt is exhausted and the underlying exception is rethrown.
Also addressed a nitpick on ReserveNumericParamValueConcurrencyTest.php:
the two concurrency tests had near-identical worker-spawning/teardown
code duplicated between them. Extracted a shared
reserveNumericParamValueConcurrently() helper that takes each test's
worker body as a string spliced into the generated child-process script,
keeping the process-spawning/collection logic in one place.
* Document reserveNextNumericParamValue's outer-transaction SQLite limitation
CodeRabbit flagged a real gap in the bounded-retry fix: when a caller has
already read something on an outer SQLite transaction before calling
reserveNextNumericParamValue(), that outer transaction is holding a SHARED
lock the entire time this method runs. If a competing writer holds
RESERVED when the lock-escalating write tries to upgrade that SHARED
lock, the outer transaction can't release just its read - only rolling
back the whole thing would - so sustained contention can exhaust every
retry with the outer transaction still open.
This doesn't affect any current caller: getNextInvoiceNumber(), the only
caller today, invokes this before its own flush() opens any transaction,
so it never nests inside a caller-owned read. CodeRabbit's suggested fix
(reserve before the outer read, or use a separate short transaction) is a
call-site-ordering contract this method can't enforce on its own, and
there's no live bug to reproduce a regression test against. Documenting
the limitation on the method itself so a future caller that does nest a
reservation inside an already-open, already-read transaction knows what
to expect.
---------
Co-authored-by: Claude <noreply@anthropic.com>
|
||
|
Adam DaleyandClaude Sonnet 5
|
019f7d4231 |
Fix cron cleanup leaving order/invoice data inconsistent (#4180)
🐛 Fix cron cleanup leaving order/invoice data inconsistent onAfterAdminCronRun() removed overdue unpaid invoices with a raw SQL DELETE, bypassing rmInvoice(). This left client_order.unpaid_ invoice_id pointing at a deleted invoice, orphaned invoice_item rows, and skipped releasing reserved stock/promo redemptions. Add InvoiceRepository::findUnpaidOlderThan() and route the cron cleanup through rmInvoice() for each matching invoice so related orders, invoice items, and reserved resources stay consistent. Fixes #4178 Claude-Session: https://claude.ai/code/session_01KdphsdrmmWYBi7mFZK94QQ Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> |
||
|
Adam Daley
|
b364588f30 | Harden release cleanup and updater integrity (#4166) | ||
|
Adam Daley
|
66c82f5220 |
Use settings repository in system service (#4146)
* Use setting repository in system service * Remove redundant SettingRepository wrappers * Recover setting writes after EM closes * Harden setting writes on insert conflicts * Batch flush settings writes in a single flush() |
||
|
Adam Daley
|
c2d5459751 |
Migrate Custompages service to Doctrine ORM (#4140)
* Migrate Custompages service to Doctrine ORM * Enforce unique custom page slugs safely * Update cspell.json * Harden custom page slug uniqueness handling |
||
|
Adam Daley
|
7f892a10c7 |
Replace Box_Database with Doctrine DBAL (#4139)
* Replace Box_Database usage with Doctrine DBAL * Harden invoice lookups and DB session setup * Clean up invoice renewal test mock * Make DB session timeouts configurable |
||
|
Adam Daley
|
60882047b9 |
Add locking to concurrent billing operations (#4132)
* Add locking to concurrent billing operations Several billing paths read a value, check it, and then act on it without holding a lock, so concurrent requests can each pass the check and each act on the same state. - Credit payment: tryPayWithCredits() now reads the balance under an exclusive lock and re-checks the invoice status under that lock, with the deduction and the paid-marking in one transaction. Task execution moved after the commit so provisioning does not run while the balance is locked. - Stock: reduceStock() now decrements in a single atomic statement rather than reading the quantity and writing back the difference, matching PromoRepository::incrementUsageIfAvailable(). - Invoice numbering: the invoice number counter is claimed and advanced under a row lock, so two concurrent approvals cannot take the same number and issue invoices sharing an invoice number. - Stripe: handleInvoicePaymentSucceeded() is now held under the existing named lock. Stripe delivers invoice.payment_succeeded, invoice.paid and invoice_payment.paid for the same payment, and its deduplication check was unguarded. The lock helper is renamed withStripeObjectLock() since it is no longer PaymentIntent specific. Also fixes the lock timeout log call, which passed printf-style varargs to a PSR logger expecting an array context and threw a TypeError instead of logging the timeout. * Address review feedback on billing locking - Guard the FOR UPDATE helpers against being called outside a transaction. The lock is released immediately in autocommit mode, so the caller would treat an unprotected value as locked with no visible failure. - Fire onAfterAdminInvoicePaymentReceived after the credit payment transaction commits. Listeners render PDFs and send email, which would otherwise hold the client and invoice row locks for the duration of an SMTP send. markAsPaid() takes a $deferEvents flag for this. - Drop the RedBeanPHP client load from tryPayWithCredits(); only the ID is needed. getClientBalanceForUpdate() now accepts an ID directly. - Ignore non-positive quantities in reduceStock(). A negative quantity subtracted a negative and inflated stock. - Reject non-integer counter values in reserveNextNumericParamValue(). is_numeric() accepts '5.5', which would be truncated to 5, written back as 6, and silently skip a number. - Use globally()->ordered() in the balance lock ordering test. Mockery orders per-mock by default, so the cross-mock ordering it claimed to assert was not being enforced. * Make the invoice number fallback allocation atomic If the invoice_starting_number setting is missing or invalid, the fallback derived the next number from the latest invoice and then wrote it back with setParamValue(). Two concurrent callers could derive the same number, and the write did not stop both from returning it. reserveNextNumericParamValue() now takes an optional seed, applied only if the setting is still missing or invalid once the row lock is held. A caller that seeds after another has already done so reserves from the repaired counter instead of overwriting it. * Handle collisions when seeding the invoice number counter Two callers seeding a counter row that does not exist yet both pass the locking read: InnoDB gap locks are shared, so neither blocks the other and they collide on the insert instead. The loser's transaction is rolled back and the caller saw the failure. reserveNextNumericParamValue() now catches the unique constraint violation and the deadlock InnoDB may raise instead, and retries once. The retry finds the winner's committed row and reserves from it, so both callers get distinct sequential numbers. |
||
|
Adam Daley
|
2709e552a4 |
Migrate Invoice model from RedBeanPHP to Doctrine (#4122)
* Migrate Invoice model from RedBeanPHP to Doctrine * Apply review fixes to the Invoice Doctrine migration * Copy buyer phone country code to refund invoices * Reject subscription payments when the renewal invoice cannot be generated |
||
|
Adam Daley
|
4e38560e7e |
Migrate Transaction and Subscription to Doctrine (#4105)
* Migrate Transaction and Subscription to Doctrine * Remove redundant inline documentation * Harden invoice tx flow and subscription list * Add invoice_id filter to subscription search |
||
|
Adam Daley
|
0d357d234a |
Migrate Invoice PayGateway to Doctrine (#4099)
* Migrate Invoice PayGateway to Doctrine * Remove stale PayGateway comments * Fix payment gateway handling and boolean normalization * Add 'falsey' to cspell dictionary * Handle empty gateway names in custom payments |
||
|
|
3acbfc3aa1 |
Apply PHP Code Styling & Quality Changes (#4071)
Co-authored-by: FOSSBilling Bot <fossbilling-bot> Co-authored-by: Adam Daley <admdly@users.noreply.github.com> |
||
|
Adam Daley
|
94c56cf5f9 |
Migrate Client and Profile to Doctrine (#4049)
* refactor: migrate client and profile to Doctrine * fix: address client migration review findings * test: strengthen client migration coverage |
||
|
Adam Daley
|
fe7d293df4 |
Refactor to use Request object for locale and cookie handling (#3934)
* Use Request object instead of superglobals * Use Request in locale resolution * Queue cookies and remove direct header writes * Fix flaky i18nTest: de_DE locale isn't installed in CI CI builds with INSTALL_TRANSLATIONS=false, so only en_US exists on disk and getLocales() never considers de_DE "enabled". Use en_US as the cookie value (always installed) against a different config default to still prove the cookie takes precedence. |
||
|
Adam Daley
|
9ea2095ce1 |
Allow admins to edit ticket replies, with edit history (#3906)
* Allow admins to edit ticket replies, with edit history (#2317) Admins previously had no way to fix a typo or correct a reply without editing the database directly. Adds an Edit action to admin-authored messages in the admin ticket view; client-authored messages remain uneditable so an admin can never alter a client's own words. Every edit snapshots the message's prior content into a new support_ticket_message_history table before overwriting it, so the original wording sent to a client is never silently lost. Admins can view a message's revision history via a lazy-loaded modal. Also fixes ticket_message_update, which existed but was never wired into any UI and skipped the markdown sanitization that ticket_reply already applies. * Bump fresh-install last_patch to 83 Fresh installs seed last_patch from content.sql so the patcher doesn't try to reapply migrations already baked into structure.sql. Missed bumping this when patch83 (support_ticket_message_history) was added. * Fix message history modal getting stuck on Loading... after a failed fetch dataset.loaded was set before the API call resolved, so a failed request left the modal permanently stuck on "Loading..." with no way to retry short of a page reload. Move the flag to the success callback and add an error handler that shows a retryable message. * Make support_ticket_message_history.support_ticket_message_id required A history row is meaningless without the message it snapshots; allowing NULL let orphaned rows exist. Tightened the column in patch84, structure.sql, and the entity's JoinColumn to NOT NULL. * Delete message edit history when a ticket is removed Service::rm() explicitly removes a ticket's notes and messages rather than relying on the entity mapping's onDelete: CASCADE, because this schema has no real FK constraints anywhere (referential integrity is handled at the application layer throughout this codebase). History rows were missed by that pattern and were left orphaned after a ticket delete. Now removed alongside their message, following the same explicit-cleanup convention as notes/messages. Also cache SupportTicketMessageHistoryRepository via setDi(), matching every other repository on this service, and add direct coverage for getMessageHistory(). |
||
|
Adam Daley
|
85fa3dc1b2 |
Refactor HTTP helpers and inject Filesystem via DI (#3900)
* Remove redundant HTTP helper wrappers * Inject Filesystem via DI across services * Fix constructor and Theme DI wiring |
||
|
Adam Daley
|
c3d91257d4 |
Restrict staff notification emails by staff group (#3899)
Previously an email template's "enabled" flag was the only way to control whether a staff notification (new order, ticket, signup, etc.) went out, and when enabled it always went to every active staff member. This adds a per-template staff-group allow-list so notifications can be scoped to the teams that should actually receive them. - New email_template_group join table/entity linking templates to admin groups, with admin API endpoints and a UI card on the template page mirroring the existing staff-member group management UI. - Email\Service resolves to_staff recipients from the template's assigned groups; a template with no groups assigned sends to nobody. - Existing and newly-created built-in staff templates are auto-linked to every current staff group (via an UpdatePatcher migration and createBuiltinTemplateRecord) so upgrades and fresh installs keep today's "notify everyone" behavior by default. - Staff\Service::deleteGroup() now blocks removing a group that's still restricting an email template. Closes #1247 |
||
|
Yağızhan Burak YakarandAdam Daley
|
03a6079418 |
Rework admin groups and permissions (#3821)
* Redo and migrate the Admin Group structure * Update permisison checks to use groups * Migrate admin group CRUD to Doctrine * Remove legacy permission and group membership logic * Replace legacy role logic * Drop the admin.permissions column * Only allow super admins to create/edit/delete groups, better guards * Remove the redundant `admin.protected` column * Clean group memberships when an admin is deleted * Make the "Exclude cron user" filter work * Improve labels * Addressed code review comments * Require an initial group * Improved logger messages * Replace role subtitle with email * Bypass permission checks during update finalization to make sure it's accessible * More sensible default groups * Update seed content timestamps * Fixed tests * Cron login guard * Addressed code review comments * Change permission column type to JSON * Addressed code review comments * Migrate regular staff to a temporary group and disallow groupless staff management * Look for Super Administrator or legacy `admin` role * Fixed tests * Changed test name to make cspell happy * Grant required permissions to the groups --------- Co-authored-by: Adam Daley <admdly@users.noreply.github.com> |
||
|
Adam Daley
|
5ee72a234a |
Migrate Email, Extension and Support modules to Doctrine (#3863)
* Migrate Email, Extension, Support modules to Doctrine ORM * Add 'initialises' to cspell.json custom words * Add patch76 and fix extension deactivation event * Bump database patch version to 76 * Refactor Support APIs to mapped Doctrine flows * Handle legacy email queue and ticket filters * Migrate Support module to Doctrine ORM * Refactor services to use typed repositories * Route order ticket count and ticket messages through repositories Drops the last two thin Support\Service wrappers, calling the SupportTicketRepository and SupportTicketMessageRepository directly from the cross-module callers. * Rename email queue table and add patch * Preserve unlimited email queue processing * Preserve unlimited email queue processing |
||
|
Yağızhan Burak Yakar
|
d189ee468e |
Knowledge base, canned responses and helpdesk enhancements (#3808)
* Migrate the knowledge base to Doctrine * Addressed code review comments * Move incrementViews to the repo * Knowledge base suggestions in support forms * Improve the UI and make search actually search * Make the knowledge base enabled on new installs by default * Redid the kb index to list articles alongside categories * Display articles in admin manage category * Redirect authenticated users from guest ticket page to client ticket page * Address code review comments * Addressed code review comments * Improved search accuracy by tokenizing queries * Updated article layout and added CTAs * Added a setting to toggle view count's visibility * Removed contact button from the sidebar * Migrate canned responses to Doctrine and improve admin UI * Improved support settings canned dropdowns * Added a flag to hide view count when serializing * Improved search query matching * Migrate SupportHelpdesk to Doctrine * Fixed tests * Update copyright headers * Implemented suggestions (credit @admdly) * Remove dead test |
||
|
|
83949ce8f0 |
Apply PHP Code Styling & Quality Changes (#3705)
Co-authored-by: FOSSBilling Bot <fossbilling-bot> Co-authored-by: Adam Daley <admdly@users.noreply.github.com> |
||
|
Adam Daley
|
dd3dcbfd52 |
Enhance type safety, authentication checks, and testing framework (#3701)
* Improve type safety, auth checks and PHPStan fixes (#3250) * Improve type safety, auth checks and PHPStan fixes * Remove @phpstan-ignore annotations * Improve type safety, auth checks and PHPStan fixes * Remove @phpstan-ignore annotations * Remove redundant @phpstan-ignore comment * Improve code robustness and reliability (#3248) * Harden code against null/undefined values * Update dependency lockfiles * Sanitize ticket input and use IDs in email list * Restructure tests and add test utilities * Move E2E tests to `Library\E2E` namespace * Update `php-build` action ref in workflows * Switch tests to Pest and add test scaffolding * Refactor E2E tests to Pest-style tests m Skip E2E tests when env not configured * Refactor Unit tests and update test config Add test datasets and refine test helpers * Add DEBUG guard, fix API JSON decode, update tests * Refactor tests: add types and Mockery expectations * Adjust tests, phpstan config, and composer script * Instantiate test APIs locally, remove beforeEach * Refactor runtime checks, types & logging * Apply PHP Code Styling Changes m * Add PHPStan ignores, test renames & small fixes * Enable Pest tests in CI workflow * Remove legacy tests; add new unit tests/config * Avoid undefined index in fingerprint comparison * Ignore properties absent in both fingerprints * Remove public_ticket_create API method * Add Fingerprint unit tests * Remove em dash from fingerprint test name * Switch to Pest; assorted bug fixes & refactors * Prefer getDi() and guard DEBUG define * Refactor tests: DBAL, Antispam rename, helpers * Refactor email tests to use Doctrine entities * Update tests, use DBAL mocks & improve Formbuilder admin * Validate plan_id and update unit tests * Replace error_log with DI logger and update tests * Expect db->store in Invoice Service test * Use DI logger instead of error_log; fix tests * Fix typo in Hook Service test * Fix Staff test spelling fixture * Complete Pest test migration * Fix Product test spelling fixtures * Fix Product test spelling fixtures * Run live tests with Pest and load helpers * Migrate tests to ApiClient; remove legacy APIHelper * Fix Email test spelling * Remove unused PHPUnit bridge * Organize Cypress end-to-end tests * Document Pest test directory root * Remove noisy Pest comments |