Skip to content

fix(packages): restore a branch at its commit, and keep paths out of manager_language (3.5) - #2558

Open
elcreator wants to merge 2 commits into
evolution-cms:3.5.xfrom
elcreator:fix-custom-package-commit-and-language-3.5
Open

elcreator wants to merge 2 commits into
evolution-cms:3.5.xfrom
elcreator:fix-custom-package-commit-and-language-3.5

Conversation

@elcreator

@elcreator elcreator commented Nov 6, 2026 •

Copy link
Copy Markdown

Problem

1. A package on a branch could move without consent. This is the regression reported on #2551 and #2552. lockPackages() keeps only version, so a site on dev-main at commit A was pinned as plain dev-main. After a release replaced the lock, Composer resolved the branch wherever it stood by then, and unexpectedChanges() accepted that because a commit on one side only counted as equal. plan() returned ok with consent off.

Reproduced on both branches with real Composer 2.10.2, a real lock and a real record, before changing anything.

2. manager_language could hold a path. The user editor stores any value, and getUserSettings() merged it as it was. ManagerTheme normalises the setting in the manager only, so a manager on the front end kept the stored value. Several includes read it without asking ManagerLanguage::isName(): the error handler, the updater and extras plugins, the store and its installer, and the old web user form. Packages that build a path from the setting read it the same way. Writing the value takes a permission to edit users, and each include has a fixed suffix.

Change

Packages

  • lockPackages() keeps the commit of a git or hg branch, the sources Composer prints a commit for. The pin becomes dev-main#<commit>, through previousVersions() and the record file.
  • updateArguments() needs no change: Composer accepts that form, so the restore comes back at the commit the site ran. An older Composer that ignores the commit resolves the branch and is refused like any other change.
  • A branch whose commit is not known on both sides counts as a change, so it needs consent. A path repository names no commit on either side and passes as before.
  • AGENTS.md records the rule.

Language

  • Core::getSettings() and getUserSettings() let only a locale name into the configuration, which also covers packages. A bad per-user value leaves the system language in place; a bad system value reads as English. A deleted locale still passes, as ManagerLanguage documents.
  • The six includes ask ManagerLanguage::isName() themselves, because the error handler can run before the settings are loaded.

Behaviour to expect

  • A record written before this change has no commit, so a site with a package on a branch is asked for consent once, on its next update.
  • If the commit is gone from the repository (a force push), the dry run cannot tell, because Composer does not check the reference there. The real update then fails with exit 1 and the update stays incomplete. The site is never moved silently.

Tests

  • New regression test from the review: a real lock at A, replaced by a release, restored at B without consent is refused, read from the lock and from the record. The same commit passes.

  • ManagerLanguageSettingTest covers the settings merge and each include; StoreLanguageTest covers a path as the store language.

  • All 21 new cases failed on unfixed 3.6.x. On 3.5.x the unfixed behaviour was shown by the Composer reproduction above, not by running these tests.

  • Real Composer with the fix, on both branches, branch moved to B: the update exits 0, the lock holds A, B's file is not in vendor, and the record keeps A.

  • Full suite on Linux: 3.5.x 2335 passed, 3.6.x 2521 passed, 9 skipped on each. PHPStan reports no errors.

  • On a real site, 3.5.x and 3.6.x, each branch installed in phramark's evo-latte stack: guest and admin pass. With a path stored as manager_language for the site's user, the manager's login, edit, save and logout pass with no fatals, and the front end and manager answer 200. The stored value stays in the table and is ignored.

The manager's language page itself was not driven in a browser.

🤖 Generated with Claude Code

elcreator and others added 2 commits November 6, 2026 03:32
The lock kept only "dev-main", so a restore resolved the branch wherever it
stood by then, and treating a commit on one side as equal let that pass
without consent. The commit now travels with the pin (dev-main#<commit>),
through the lock and the record, and a branch whose commit is not known on
both sides counts as a change.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The user editor stores any value and the settings merge took it as it was.
The manager theme normalises it, but a manager on the front end kept it, and
it reached include in the error handler, the updater and extras plugins and
the store. Only a locale name now gets into the configuration, which also
covers packages that read the setting, and those includes ask for one too.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Seiger commented Nov 6, 2026

Copy link
Copy Markdown

@elcreator Please fix the path-repository regression before merging this and #2559.

Unchanged dev path packages are classified as changed. This is a static finding against 24eb88a, checked against Composer 2.10.2's formatting and path-repository code.

Conditions: a custom package (or its private dependency) has a dev version, no source, and dist.type = path with a non-empty dist.reference. This is the normal default reference: auto case, not just an unusual configuration: Composer generates a config hash, or uses Git HEAD when available. reference: config also supplies a hash.

pinnedVersion() keeps only dev-main for that package, because it only reads git/hg source.reference. However, Composer's getFullPrettyVersion() falls back to dist.reference for a dev package without a source, so its lock operation is, for example:

Lock file operations: 1 install, 0 updates, 0 removals
  - Locking acme/local (dev-main 18f1a03)

After a release replaces the lock and the package needs restoring, unexpectedChanges() compares dev-main with dev-main 18f1a03. The new one-sided-reference check rejects it, so plan(..., allowChanges: false) returns ok=false, reason=changes even when the path reference is unchanged. recordInstalled() also persists only the plain version via dependencyClosure(), so accepting once does not cure this: the same false consent requirement recurs on later restores after the release lock replaces the site's lock.

Requested correction: preserve the reference used for comparison through both the live-lock and saved-record paths, including private dependencies, separately from the constraint passed to Composer. A path dist.reference is comparison metadata, not a VCS commit override to append blindly as #ref. Keep the new protection for unknown/moved git/hg commits; do not restore the old blanket rule that a missing reference equals any reference.

Please add regressions on both branches using Composer-formatted output (ideally generated by InstallOperation::format() or a real Composer dry run):

  • An unchanged dev path package with a non-empty default/config reference restores without consent, from both the original lock and the saved record.
  • After recording the successful restore and replacing the lock again, the unchanged package still restores without consent.
  • A changed path reference still needs consent; reference: none and the existing git/hg unknown/moved-commit cases remain covered.

The current dev-main vs dev-main path test only covers a no-reference configuration, such as reference: none; it does not exercise Composer's default path reference generation. Please also correct the PR description's claim that path repositories have no reference on either side.

SokilAI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants