fix: keep the configuration a save replaces - #905
Merged
Conversation
ConfigBackupService::backup() has existed since this rewrite was imported and is called from nowhere in src/, so config_backup was never written and the 'Download config backup' link the Information page renders — beside a date, unconditionally — answered 'Unable to retrieve the configuration' every time, on every installation. Every integration test around that download seeds the parameter by hand with #[InjectConfigParam], which is the shape of a feature nothing produces. On paper the call belongs in ConfigFile::save(). It cannot go there: ConfigBackup needs ConfigService, which needs Application, which needs ConfigFileService — a cycle only a lazy proxy breaks, on the object the container builds while booting. ConfigTrait::saveConfig() is the shared path all nine configuration screens go through, runs per-request long after boot, and a controller taking ConfigBackupService is already proven by DownloadConfigBackupController. Required rather than optional, because an optional collaborator a caller forgets is silently no backup — the failure being fixed. getConfigData() answers a clone and save() has not run when the backup is taken, so what is stored is the previous configuration. backup() logs and swallows its own failures, so a database that cannot take it does not stop an administrator saving.
blaipr
deleted the
fix/the-config-backup-download-has-a-backup-to-offer
branch
September 3, 2026 01:40
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ConfigBackupService::backup()has existed since this rewrite was imported and is called fromnowhere in
src/. Soconfig_backupwas never written, and the "Download config backup" link theInformation page renders — beside a date, unconditionally — answered "Unable to retrieve the
configuration" every time, on every installation.
The tell was already in the test suite: every integration test around that download seeds the
parameter by hand with
#[InjectConfigParam(['config_backup' => …])]. That is the shape of afeature nothing produces.
It also matters more than it did last week:
config.xmlis now replaced atomically rather thantruncated in place, and that PR noted there was nothing to fall back on if it were lost. This is the
fallback.
Where the call goes, and why not where it belongs
On paper this belongs in
ConfigFile::save()— the one place the file is written. It cannot gothere.
ConfigBackupneedsConfigService, which extendsServiceand needsApplication, whichneeds
ConfigFileService:Only a lazy proxy breaks that, on the object the container builds while booting and every request
depends on — the exact place this repo has had a total outage from a DI mistake before. Not worth it
for this.
ConfigTrait::saveConfig()is the shared path all nine configuration screens go through, it runsper-request long after boot, and a controller taking
ConfigBackupServiceis already proven —DownloadConfigBackupControllerdoes it. So the backup is taken there, and the nineSaveControllerspass the service in.
Making it a required parameter of
saveConfig()rather than an optional one is deliberate: anoptional collaborator that a caller forgets is silently no backup, which is the failure this PR is
fixing. The signature change means every call site had to be updated to compile.
It stores the previous configuration, not the new one
ConfigFileService::getConfigData()answers a clone, andsave()has not run when the backup istaken — so what reaches
backup()is still the stored configuration. Ordering is the only thingthat makes that true, so it is asserted directly rather than assumed.
backup()logs and swallows its own failures, so a database that cannot take it does not stop anadministrator saving configuration.
Verified
ConfigBackupService, and all three constructor parameters of apatched controller are required — so php-di wires them rather than silently leaving a default
null, which is this codebase's recorded trap.refuses the save, stores nothing. Mutation-verified — removing the call fails the first two and
leaves the demo one passing, which is right, since it asserts an absence.