mirror of
https://github.com/FOSSBilling/FOSSBilling.git
synced 2026-11-04 02:57:11 +09:00
main
2
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Adam Daley
|
0eb833e026 |
Reorganise FOSSBilling core library (#4210)
* refactor(library): reorganize FOSSBilling core library
Group Update* prefix into FOSSBilling\Update\{Updater,Patcher,Finalization,ReadinessCheck}
Group Pagination into FOSSBilling\Pagination\{Service,Options,ViewPaginator}
Group System (Config,Environment,Version,Requirements) into FOSSBilling\System
Consolidate Security (Crypt,PasswordManager,Fingerprint,Session,CheckResult/Enum)
Group Exception into FOSSBilling\Exception\{BaseException,InformationException}
Fix I18n case (i18n.php -> I18n/I18n.php) and move Translate -> I18n/Translator
Group Logging (Logger, Monolog -> Factory)
Group Remote (CentralAlerts,ExtensionManager), move ErrorPage -> Http, InjectionAwareInterface -> Interfaces
Update all call sites (di.php, load.php, modules, tests) with no BC shims
Fix import aliasing and phpstan docblocks, remove unused imports
Tests: 2856 passed, phpstan OK
* refactor(validation): migrate Validate into Validation namespace
Split monolithic FOSSBilling\Validate (232 lines, 7 mixed responsibilities)
into focused validators:
- Validation\DomainValidator (isSldValid, isTldValid with DI cache/http)
- Validation\PasswordValidator (isPasswordStrong, passwordsMatch)
- Validation\EmailValidator (isEmailValid)
- Validation\BirthdayValidator (isBirthdayValid)
- Validation\RequiredValidator (checkRequiredParamsForArray)
- Validation\Validator facade retaining same API for $di['validator'] BC-free
Move FOSSBilling\Validate -> FOSSBilling\Validation\Validator,
add FOSSBilling\Validation\DomainValidator via $di['domainValidator'],
keep $di['validator'] as facade. Remove old Validate.php.
Update all call sites and tests (FOSSBilling\Validate -> FOSSBilling\Validation\Validator),
make Validator/DomainValidator non-final for Mockery.
Tests: 2856 passed, phpstan OK
* refactor(tools): split Tools into Utils/Security/Sanitizer/Validation (WIP)
Extract Tools god class (534 lines) into focused helpers:
- Utils\Str (slug, toCamelCase, fromCamelCase)
- Utils\Arr (safeCount, sortByOneKey, getPairsForTableByIds via arr service)
- Utils\Normalizer (normalizeBoolean, normalizePort)
- Utils\Network (isValidHttpInterface, getDefaultInterface, listHttpInterfaces, getExternalIP via network service)
- Utils\Format (humanReadableBytes)
- Security\Credential (generatePassword, session restore tokens)
- Sanitizer\Sanitizer (sanitizeContent, sanitizePlainText, sanitizeMarkdownContent)
- Validation\PhoneValidator + EmailValidator::validateAndSanitizeEmail
Update di.php: replace tools service with arr/network services,
migrate 23 src files to new helpers (FOSSBilling\Utils etc.),
keep Tools.php for remaining tests (to be removed next).
Tests: partial, 11 errors remain for slug mocks (to be fixed).
* refactor(api): introduce Role enum, tighten Api layer (deslopped)
- Add FOSSBilling\Api\Role enum (guest/client/admin) replacing stringly-typed role handling
- Api\Identity now stores Role, exposes getRole():Role + getType():string (BC), typeFromObject():Role
- Api\AbstractApi: make abstract, typed properties (string $ip, ?Module $module, Client|Admin|Guest|null $identity), typed setters/getters, fix BaseException usage
- Api\Proxy: compose Identity VO instead of duplicating type logic, keep lazy dispatcher from container, remove redundant helpers
- Api\Dispatcher: use Role enum and Identity VO, normalize identity, keep direct $di['update_finalization'] lookup, deslop finalization injection and apiClassSuffix helper
- Update tests/Pest IdentityTest to expect Role enum
- Minor: fix Cron/Staff Api call sites for typed identity
Deslop: removed hasModule/hasIdentity/hasService helpers, duplicate forwarding comment, unnecessary setFinalization/constructor injection and apiClassSuffix helper.
* refactor(cache): introduce Driver enum, use typed driver handling
- Add FOSSBilling\Cache\Driver enum (filesystem/redis/memcached)
- CacheFactory: parse config via Driver::tryFrom, match on enum, use ->value for messages, remove unused Tools import
- System\Api\Admin: validate driver via Driver::tryFrom instead of in_array(SUPPORTED_DRIVERS)
* refactor(doctrine+api): address review findings
- Doctrine: remove unused Tools import in DriverManagerFactory
- Doctrine: add Driver/Charset enums (pdo_mysql, utf8/utf8mb4/latin1) and use tryFrom for validation
- Api: make AbstractApi::setModule(object) lenient for test doubles (anonymous class in DispatcherTest)
- Tests: apiEndpoint() now ensures identity via getIdentity() catch and sets Admin/Client/Guest based on Api class
* Type AbstractApi module as Module
* Update Doctrine factory copyright years
* refactor(enums): relocate Client/Order status enums to domain modules
Move FOSSBilling\Enums\ClientStatusEnum -> Box\Mod\Client\Enum\Status
and FOSSBilling\Enums\ClientOrderStatusEnum -> Box\Mod\Order\Enum\Status,
colocating statuses with their owning entities (Client.php:29,
Order.php:24) and removing the generic FOSSBilling\Enums catch-all.
Drops Enum suffix, fixes ClientOrder vs Order naming mismatch, updates
sole consumer Massmailer\Service and deletes src/library/FOSSBilling/Enums/.
* refactor(exception): clean up BaseException and InformationException
- BaseException: type stackTrace(int $length,bool $protected), fix
camelCase ($Length->$length, $my_entry->$arg, $entry_file/file),
debug_backtrace typing, copyright 2025->2026
- InformationException: remove redundant passthrough __construct, now
minimal marker extends BaseException (retains 100+ user-facing
throw sites and Sentry ignore distinction)
* refactor(http): harden Http factories, fix escaping and copyrights
- CsvResponseFactory: validate table name preg_match /^[a-zA-Z0-9_]+$/ and throw
InformationException, add import, copyright 2025->2026
- ErrorPage: escape message via htmlspecialchars in renderPage, use
escapedMessage for fallback and original toggle, copyright 2025->2026
- ExceptionResponseFactory: split rawMessage, htmlspecialchars only for
API_MODE Json, pass raw to ErrorPage (single escape source)
- RequestFactory: copyright 2025->2026
* Clean up locale detection error handling
* refactor(interfaces): split generic Interfaces into domain colocated contracts
- Delete FOSSBilling\Interfaces catch-all (5 files)
- FOSSBilling\Interfaces\ApiArrayInterface -> FOSSBilling\Api\ArrayInterface
(ArrayInterface, removes Api stutter, 60+ entities)
- FOSSBilling\Interfaces\InjectionAwareInterface -> FOSSBilling\Container\InjectionAwareInterface
(Container, 100+ services)
- FOSSBilling\Interfaces\SecurityCheckInterface -> FOSSBilling\Security\CheckInterface
(Security, removes Security stutter, 4 checks)
- FOSSBilling\Interfaces\TimestampInterface -> FOSSBilling\Doctrine\TimestampInterface
(Doctrine, with TimestampTrait)
- FOSSBilling\Interfaces\WidgetProviderInterface -> Box\Mod\Widgets\ProviderInterface
(Widgets module, provider contract for Branding etc)
Update 172 usages, keep InjectionAware/Timestamp short names, shorten
ApiArray/SecurityCheck/WidgetProvider stutter.
* Type logger cache as array in factory
* Update pagination copyright headers
* Update CentralAlerts copyright year
* Clean up sanitizer class imports
* Refine security helpers and enum namespace
* Update system class copyright headers
* Move Twig AppArea enum to root namespace
* Update copyright year in update classes
* Drop legacy Arr/Str wrappers and DI binding
* refactor(utils): complete Tools split and delete god class
- Delete FOSSBilling\Tools (534 lines) god class, remove load.php require/use
- Migrate 16 call sites: di['tools']->validateAndSanitizeEmail -> EmailValidator::validateAndSanitizeEmail (8 Api), News slug -> Str::slug, generatePassword already via Credential, humanReadableBytes via Utils\Format, etc.
- Remove 20 unused use FOSSBilling\Tools imports (Order/Extension/Currency etc.)
- Harden Arr::getPairsForTableByIds with preg_match table validation and backticks (CsvResponseFactory style)
- Remove Str legacy wrappers (slugLegacy/slugInstance/to_camel_case)
- Update 11 test files to remove Tools mocks (Extension/Product/Profile/Custompages/Staff/Client/Email/Invoice/Support) and Container helper
- phpstan 443 OK (one less file), tests 60/6 pre-existing failures (AbstractApi identity/pagination) remain, Tools InvalidCount 54->0 for new code
* Update RequiredParams copyright year
* Update copyright headers to 2026
* Remove dead validation code
* fix(ci): address Rector, CS-Fixer and test failures after library reorg
- Run Rector fixes: RemoveUselessReturnTag, RemoveReadonlyOnReadonlyClass, ReadOnlyProperty, RecastingRemoval
- Run cs:fix (132 files) for import ordering and formatting
- Fix Pest helper to handle Mockery mocks for Admin/Client identities
- Fix DispatcherTest to use Module mocks for extension
- Update Client tests to remove Tools mocks and use new EmailValidator/Url
- Fix Custompages ServiceTest to use real Str::slug and long titles
- Fix Pagination mocks to use Service class
- Fix i18n, InformationException, Requirements, Config namespaces
- Add url to test container with SYSTEM_URL base
- Fix a few remaining service tests (Profile, Extension, Product, Support)
phpstan 451/451 OK, pest now 11 failures (down from 107)
* fix(tests): make suite green after reorg
- Fix Extension ServiceTest InformationException namespace and Str mock
- Fix Product Api Admin paginated products mock
- Fix Client/Service i18n, Profile i18n, and url container
- Fix Custompages long-title slug handling
- All 2998 tests pass (2 skipped), phpstan 451/451
* style: fix cs
* fix(docker): update Version.php path after move to System namespace
* fix(tests): remove remaining Tools references
- Delete obsolete ToolsTest (Tools class removed)
- Remove di['tools'] mocks from Profile ServiceTest
* feat(update): add patch115 to clean up library reorg orphans
Removes stale PSR-4 paths left on disk after refactor/library-reorganization
(Tools split, moved namespaces, low-case i18n) which persist on updated
installs because the updater extracts on top. Uses executeFileActions +
removeEmptyDirectories with exists guards, idempotent.
Bumps fresh-install last_patch to 115 to match latestPatchLevel().
* fix(cache): align TLS + instance_id merge with reorg namespaces
- Use Normalizer::normalizeBoolean instead of removed Tools
- Use BaseException + driver->value for instance_id/TLS errors
- Update tests to expect BaseException
* fix(tests): use US spelling to satisfy spellcheck
Normalise -> Normalize, normalised -> normalized
* refactor(box): move Box legacy globals to semantic namespaces
- Box_App/AppAdmin/AppClient -> FOSSBilling\Http\App/AppAdmin/AppClient
- Box_Authorization -> FOSSBilling\Security\Authorization
- Box_Event/EventDispatcher/EventManager -> FOSSBilling\Event\Event/Dispatcher/Manager
- Codemod 499 references across src/modules, src/di.php, src/index.php, tests
- Fix namespace imports (ArrayObject, Pimple\Container, ReflectionMethod, \RuntimeException, \InvalidArgumentException, \Box\Mod\*, \Override)
- Add Patcher patch116 to clean up orphaned src/library/Box/* + bump last_patch 115->116
* refactor(update): extract Patcher patches to Patch/* orchestrator
- Create FOSSBilling\Update\Patch\PatchInterface + PatchRegistry (explicit MAP, not Finder)
- Move 89 private patch25..patch116 methods to dedicated classes Patch25..Patch116
- Each Patch::apply(Patcher $patcher) delegates via public helpers ($patcher->executeSql etc.)
- Patcher now shrinks from 3624 to 1049 LOC: getPatches() uses PatchRegistry::MAP, applyCorePatches() loops [new PatchClass(), 'apply'] with call_user_func($patch, $this)
- Make helpers public (executeSql, fetchAll, tableHasColumn etc.) and properties public for patch access
- Update UpdatePatcherTest to invoke Patch*::apply and expect 'apply' method, keep ordering/skip checks
- No behavior change: MySQL guard, SchemaSynchronizer, last_patch 116 intact
* fix(spellcheck): add checkperm to dictionary after Box->Http move
* style: apply Rector StringClassNameToClassConstant + typed const
* style: apply Rector IfToNullCoalescing for changed files
* style: apply remaining Rector IfToNullCoalescing
* refactor(update): drop redundant Patch101
ADD INDEX client_order_unpaid_invoice_id_idx is already declared in
Order.php:19 and covered by SchemaSynchronizer's additive sync (safeAddedIndexes).
On PG/SQLite Patch101 never ran (isMysqlDriver guard), MySQL now gets it
via syncPortableSchema after the patch loop. Keeps last_patch at 116,
adds gap 101 alongside 92/95/98.
* refactor(update): make Patch116 superset of 115+116, keep 115 as no-op gap
* fix(spellcheck): add superset/orphans after Patch115/116 merge
* refactor(update): remove Patch115 file, keep 116 as superset
Patch115 orphan cleanup is now handled by Patch116 superset (39 paths).
115 remains a gap (like 92/95/98/101) — installs at 115 still get cleanup via
116's idempotent deletes.
* refactor(update): make Patch39 superset of 38+39, drop Patch38 file
38 (Ispconfig3/Virtualmin) + 39 (srp) are idempotent file deletions.
39 now deletes all 3 orphans; 38 remains gap like 101/115.
Installs at 37 -> 39 and 38 -> 39 both get cleanup via 39 superset.
* fix(spellcheck): add Ispconfig3 for Patch39 superset
* refactor(update): make Patch33 superset of 30-33, drop Patch30-32 files
30 (guzzlehttp) + 31 (htaccess/config.old) + 32 (phpmailer/Box/*) + 33 (FileCache)
are idempotent file deletions. 33 now deletes all 19 orphans; 30-32 remain gaps
like 38/101/115. Installs at 29 -> 33 and 32 -> 33 both get cleanup via 33 superset.
* fix(spellcheck): add idempotently/gaps/FileCache for Patch33 superset
* refactor(update): move gateway/branding helpers into Patch64
- migrateDefaultBrandingAssetsToPublicDirectory and
migrateGatewayAssetsToPublicDirectory were only used by Patch64.
Move them as private helpers inside Patch64 (param Patcher $patcher)
and remove public versions from Patcher.
- Patcher shrinks by ~66 LOC, stays public only for shared helpers.
* Drop getVersion from update patches
* refactor(update): move downloadable helpers into Patch65
- generateDownloadableStoredFilename, copyLegacyDownloadableFile,
migrateDownloadableProduct/Service/OrderStorageKeys were only used
by Patch65 (and internally). Move them as private helpers inside
Patch65 (private array $downloadableStorageMigrationMap + 5 methods
taking Patcher $patcher) and remove public versions + property from
Patcher. Keeps Patcher public API for shared helpers only.
* refactor(update): move email template helpers into Patch52
- getDefaultEmailTemplateData and getDefaultEmailTemplatePath were only
used by Patch52 directly (and via Patcher::restoreLegacy... for 57/90).
Move them as private helpers inside Patch52 (taking Patcher $patcher)
so Patch52 is self-contained. Keep public versions in Patcher for
restoreLegacyDefaultEmailTemplates used by Patch57/90 (duplicate for now,
could also be moved if desired).
* refactor(update): move email template helpers into Patch52/57/90
- getDefaultEmailTemplateData and getDefaultEmailTemplatePath were only
used by Patch52 direct and via Patcher::restoreLegacyDefaultEmailTemplates
for Patch57/90. Move them as private helpers into Patch52 (self-contained)
and duplicate into Patch57/90 alongside private
restoreLegacyDefaultEmailTemplates (which also moved from Patcher).
- Remove public versions from Patcher — keeps Patcher public API for shared
helpers only (executeSql, tableHasColumn etc.). Fixes unrelated
Patcher.php diff (\s vs \s) by removing the methods entirely.
* refactor(update): move custom page slug helpers into Patch102
- allocateUniqueCustomPageSlug and fitCustomPageSlug were only used
by Patch102 (via $patcher->). Move them as private helpers inside
Patch102 (taking Patcher $patcher, using $patcher->fetchOne and
$this->fitCustomPageSlug) and remove public versions from Patcher.
Keeps Patcher public API for shared helpers only.
* refactor(update): make Patcher helpers and patch bookkeeping driver-agnostic
- applyConfigPatches: normalize driver aliases (mysql/pgsql/sqlite -> pdo_*) and use per-driver port defaults (5432 for pgsql, none for sqlite) instead of hardcoded 3306; aligns with DriverManagerFactory::DEFAULT_PORTS.
- setPatchLevel: replace MySQL-only ON DUPLICATE KEY UPDATE with portable SELECT then UPDATE/INSERT so last_patch works on pgsql/sqlite.
- Rename isMysqlDriver -> isLegacyPatchDriver (keep isMysqlDriver as deprecated alias); availablePatches/applyCorePatches now use isLegacyPatchDriver; update SchemeSynchronizer and test docblocks.
- Make tableExists/getTableColumns/getColumnLength/getColumnType/tableHasIndex portable via AbstractSchemaManager + driver-specific fallbacks (information_schema.tables WHERE table_schema='public' for pgsql, sqlite_master for sqlite); MySQL path keeps SHOW COLUMNS/INDEX for BC.
- updateTable: use driver-aware quoting (backticks for mysql, double quotes for pgsql/sqlite) instead of hardcoded backticks.
Verified: phpstan 536/536, cs:fix clean, pest 2990 passed.
* chore: add pg catalog words to spellcheck dictionary
* Remove deprecated compatibility shims
* refactor(doctrine): expand Driver enum and use it throughout
- Driver.php: add PdoPgsql/PdoSqlite cases, tryFromAlias(), defaultPort(), isMysql/isSqlite helpers; was single-case.
- DriverManagerFactory.php: use Driver::tryFromAlias() in normalizeDriver(), Driver::tryFrom()->defaultPort()/isMysql/isSqlite in getDatabaseConfig/buildConnectionParams/applySessionSettings instead of string compares.
- Patcher.php: use Driver enum in applyConfigPatches (alias + defaultPort), isLegacyPatchDriver (isMysql), tableExists/getTableColumns/getColumnLength/getColumnType/tableHasIndex/updateTable (portable via AbstractSchemaManager + enum driver checks).
Ensures single source of truth for supported drivers; phpstan 536/536, pest 2990.
* Unify HTML sanitization across Twig and APIs
Replace the old sanitizer helpers with a shared HtmlSanitizerFactory and reuse context-specific sanitizers for content, markdown, adapter output, and theme settings. This also exposes sanitize_html and sanitize_markdown Twig filters, sanitizes markdown render output and email/signature HTML consistently, and simplifies support/email input cleanup by trimming/null-byte stripping instead of HTML-encoding raw input.
* fix: align PayGateway entities with Doctrine TimestampInterface after merge
* Type HTML sanitizer scheme constants
* style: fix import order and FQCN usage
* refactor(sanitizer): centralize HtmlSanitizerFactory sanitization and remove redundant config
* fix: add commonmark to cspell dictionary
* fix: apply Rector autofixes for library reorganization
Addresses Code Quality failures after merging main:
- remove unnecessary (string)/(bool) recasts (RecastingRemovalRector)
- add strict string casts for null-able args (NullToStrictStringFuncCallArgRector)
Files are limited to the PR's changed PHP set (BASE_SHA...HEAD) so
pre-existing drift elsewhere doesn't block this branch.
* fix: stabilize DomainValidator arrow function type for Rector
Add explicit string type to array_filter callback to prevent
oscillation between NullToStrictString (add cast) and
RecastingRemoval (remove cast) on the tld parameter.
* fix: apply Rector autofixes after merge from main
BinaryOpNullableToInstanceof / WhileNullableToInstanceof
* style: fix CS imports after Rector (Invoice / ServiceLicense FQCN -> imported)
* fix: use FOSSBilling\Event\Event in Antispam isSpam test (Box_Event -> Event reorg)
* Move src/library/FOSSBilling to src/core
First-party namespaced classes now live in src/core, leaving src/library
as the legacy PSR-0 adapter layer (Payment_, Registrar_, Server_).
- composer.json: PSR-4 FOSSBilling\ -> src/core/ (classmap unchanged)
- load.php/install.php: new PATH_CORE constant; pre-autoload requires repointed
- Update/ReadinessCheck: core/ added to the writable-folder checks
- Patch117: removes the obsolete library/FOSSBilling tree on upgraded installs
- Dockerfile: copies src/core in the vendor stages; release rewrites
(Version.php, SentryHelper.php, archive composer remap) repointed
- labeler/CODEOWNERS/cspell/AGENTS.md/test path references updated
- install/sql/content.sql: last_patch seed 116 -> 117
* Rename the FOSSBilling\ namespace to FOSSBilling\Core\
All first-party namespaced classes now live under FOSSBilling\Core\,
matching their src/core location. Clean cut, consistent with the Box\
removal on this branch: released extensions referencing the old names
must update their imports once in the same release.
- namespace/use/FQCN/docblock rewrite across src/ and tests/ (626 files)
- composer.json PSR-4: FOSSBilling\Core\ -> src/core/
- Dockerfile release composer remap updated to match
- SentryHelper: core.class tag for core paths, library.class retained
for the PSR-0 adapter layer
- Doctrine proxy namespaces (prod + test) follow the new prefix
- tests/Unit/FOSSBilling -> tests/Unit/FOSSBilling/Core; template
harness dirname depths adjusted
- ignore Strict/ test-harness findings artifacts
* Refresh the Composer autoloader after restoring the cached vendor tree
The php-build vendor cache is keyed on composer.lock, whose content-hash
does not change for autoload-only composer.json edits. On a cache hit the
install step is skipped entirely, leaving vendor/autoload.php generated
from the old PSR-4 mapping - so the library-to-core reorganization made
Rector fatal with "Trait FOSSBilling\Core\Doctrine\TimestampTrait not
found" while compiling analyzed files. Re-dump the autoloader from the
current composer.json after the cache restore.
* Trim comments on the two hand-written pieces to house style
Patch116 uses two dense lines with a short SHA rather than a commit
title; mirror that for Patch117. Shorten the autoloader-refresh comment
in php-quality-setup to the essentials.
* Fix two stale references from the library-to-core move
- GeoIP\Reader still built its bundled MMDB paths from PATH_LIBRARY
. FOSSBilling - point them at PATH_CORE so the default reader works
now that Patch117 removes the old tree; add existence tests so a
future move can't silently break this again.
- Dispatcher passed Request::getClientIp() (nullable on CLI, e.g. cron)
to the string-typed setIp(), TypeErroring before the first cron task;
coalesce to '' to match AbstractApi's own empty-string default.
* Dockerfile: namespace-wrapped cache sanity check belongs under Core
* Add dasharray to cspell words (SVG stroke-dasharray in branding assets)
Fixes Spellcheck failure on the push CI run.
* Deslop merge resolutions: trim comments, drop dead test stubs, import Patch classes
* Fix stale old-namespace references found in completeness audit
- CONTRIBUTING.md documented src/library/FOSSBilling paths/namespaces
- EntityMigrationCoverageTest read the deleted
library/FOSSBilling/UpdatePatcher.php; scan Patcher.php + Patch
classes instead (proven to detect ported migrations)
* Satisfy Rector 2.8: drop redundant cast, add nullable return type
* Address code review: restore Patch30-32, trim Patch33, drop retired hook call in Patch55
- Patch55 called mod_service('hook')->batchConnect(), removed on main
when the hook runtime was retired; the throw skipped the remaining
spamchecker cleanup. Match main (no hook lines).
- Patch33 had absorbed patches 30-32 cleanups, newly deleting the live
vendor/guzzlehttp (now required via sentry/sentry) for installs past
patch 30. Restore Patch30/31/32 as separate classes matching main and
trim Patch33 to the FileCache.php removal.
* Fix uninitialized Arr helper for group-restricted promos
enrichPromoApiArray built Arr without a container, so any promotion
with client-group restrictions fatally dereferenced null di in
getPairsForTableByIds. Initialize the helper with the service
container (with the file's usual missing-container guard) and add a
regression test.
* Keep client-group lookup portable: drop MySQL backticks
The Tools split introduced backtick quoting around the validated
table name; PostgreSQL rejects it. Main used the validated unquoted
name - restore that and assert the exact SQL in the regression test.
|
||
|
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>
|