From 73b11bb7c0c7cb2690b5e29bd0a4fb84a88d5858 Mon Sep 17 00:00:00 2001 From: Artur Kyryliuk Date: Fri, 6 Nov 2026 03:32:49 +0100 Subject: [PATCH 1/3] fix(packages): restore a branch at the commit the site ran 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#), 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 --- AGENTS.md | 2 + core/src/Services/CustomComposerPackages.php | 46 +++++++++--- .../Install/CliInstallCustomPackagesTest.php | 71 +++++++++++++++++-- 3 files changed, 103 insertions(+), 16 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index eb2d06257c..37f9371680 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -164,6 +164,8 @@ under one policy. Do not add a restore path that skips it: - Versions: what the site ran (`previousVersions()`: the lock while it still lists them, else `core/custom/composer.installed.json`, written by every restore and by `package:installrequire`), pinned by `pins()`; a package the release ships or the core requires keeps the release's version. + A branch from git or hg is pinned at its commit (`dev-main#`); a branch whose commit is not + known on both sides counts as a change, so it needs consent. - Arguments: `updateArguments()`, `--with-dependencies` (`--minimal-changes` from Composer 2.7), never `--with-all-dependencies`, which moves the core's own packages too. - `plan()` dry run first (`planInstall()`, a `composer install --dry-run`, when nothing needs restoring: diff --git a/core/src/Services/CustomComposerPackages.php b/core/src/Services/CustomComposerPackages.php index 9ecf84e1db..1a3e94ac4e 100644 --- a/core/src/Services/CustomComposerPackages.php +++ b/core/src/Services/CustomComposerPackages.php @@ -687,14 +687,16 @@ public static function unexpectedChanges(array $operations, array $pins, array $ { $custom = array_fill_keys(array_map('strtolower', $packages), true); $sameVersion = static function (string $a, string $b): bool { - // A branch is "dev-main 18f1a03" in one source and "dev-main" in another: a reference - // on one side only says nothing, two different ones mean the branch moved. - [$versionA, $referenceA] = array_pad(explode(' ', strtolower(ltrim(trim($a), 'vV')), 2), 2, ''); - [$versionB, $referenceB] = array_pad(explode(' ', strtolower(ltrim(trim($b), 'vV')), 2), 2, ''); - - return $versionA === $versionB - && ($referenceA === '' || $referenceB === '' - || str_starts_with($referenceA, $referenceB) || str_starts_with($referenceB, $referenceA)); + // A branch is "dev-main#18f1a03..." as a pin and "dev-main 18f1a03" as Composer prints it. + // A commit on one side only cannot show the branch stayed where it was: that needs consent. + $parts = static fn (string $version): array => array_pad(preg_split('/[ #]/', strtolower(ltrim(trim($version), 'vV')), 2), 2, ''); + [$versionA, $referenceA] = $parts($a); + [$versionB, $referenceB] = $parts($b); + if ($versionA !== $versionB || ($referenceA === '') !== ($referenceB === '')) { + return false; + } + + return $referenceA === '' || str_starts_with($referenceA, $referenceB) || str_starts_with($referenceB, $referenceA); }; $changes = []; @@ -738,7 +740,7 @@ private static function satisfies(string $version, string $constraint): bool /** * Packages of core/composer.lock with their versions and requirements. * - * @return array}> + * @return array}> */ private function lockPackages(): array { @@ -755,6 +757,7 @@ private function lockPackages(): array if ($name !== '') { $packages[$name] = [ 'version' => (string) ($package['version'] ?? ''), + 'pin' => self::pinnedVersion($package), 'require' => array_map('strtolower', array_keys((array) ($package['require'] ?? []))), ]; } @@ -764,6 +767,27 @@ private function lockPackages(): array return $packages; } + /** + * The version to bring a locked package back at. "dev-main" alone is wherever the branch + * stands by then, so a branch keeps its commit: "dev-main#18f1a03...". + * + * Only for git and hg sources, the ones Composer prints a commit for: no other can be + * compared with what a dry run reports. + * + * @param array $package Entry of composer.lock. + */ + private static function pinnedVersion(array $package): string + { + $version = (string) ($package['version'] ?? ''); + $source = (array) ($package['source'] ?? []); + $reference = (string) ($source['reference'] ?? ''); + $isBranch = str_starts_with($version, 'dev-') || str_ends_with($version, '-dev'); + + return $isBranch && $reference !== '' && in_array($source['type'] ?? '', ['git', 'hg'], true) + ? $version . '#' . $reference + : $version; + } + /** * Packages core/composer.json requires itself; they keep the core's version. * @@ -785,7 +809,7 @@ private function coreRequirements(): array /** * A package and what it requires in the lock, without the core's own requirements. * - * @param array}> $lock + * @param array}> $lock * @param string $root Package name. * @param array $core * @return array Package name => version. @@ -799,7 +823,7 @@ private function dependencyClosure(array $lock, string $root, array $core): arra if (isset($versions[$name]) || !isset($lock[$name]) || ($name !== $root && isset($core[$name]))) { continue; } - $versions[$name] = $lock[$name]['version']; + $versions[$name] = $lock[$name]['pin']; foreach ($lock[$name]['require'] as $dependency) { if (self::isPackageName($dependency)) { $queue[] = $dependency; diff --git a/core/tests/Unit/Install/CliInstallCustomPackagesTest.php b/core/tests/Unit/Install/CliInstallCustomPackagesTest.php index e959b16ff8..6206a53311 100644 --- a/core/tests/Unit/Install/CliInstallCustomPackagesTest.php +++ b/core/tests/Unit/Install/CliInstallCustomPackagesTest.php @@ -503,19 +503,80 @@ function removeCustomPackagesCore(string $core): void ]); }); -test('a branch counts as changed only when its reference moved', function (string $pinned, string $to, array $changes) { +test('a branch counts as unchanged only when both sides name one commit', function (string $pinned, string $to, array $changes) { $operations = ['acme/widget' => ['op' => 'install', 'from' => '', 'to' => $to]]; expect(CustomComposerPackages::unexpectedChanges($operations, ['acme/widget' => $pinned], ['acme/widget']))->toBe($changes); })->with([ - 'the lock knows the reference, Composer prints none' => ['dev-main 18f1a03', 'dev-main', []], - 'Composer prints it, the lock does not' => ['dev-main', 'dev-main 18f1a03', []], + 'the pin and what Composer prints' => ['dev-main#18f1a03c9d2e', 'dev-main 18f1a03', []], 'short and long spelling of one commit' => ['dev-main 18f1a03', 'dev-main 18f1a03c9d2e', []], - 'the branch moved' => ['dev-main 18f1a03', 'dev-main 77aa012', ['acme/widget dev-main 18f1a03 => dev-main 77aa012']], - 'another branch' => ['dev-main 18f1a03', 'dev-next 18f1a03', ['acme/widget dev-main 18f1a03 => dev-next 18f1a03']], + 'no commit on either side, as a path repository' => ['dev-main', 'dev-main', []], + 'the branch moved' => ['dev-main#18f1a03', 'dev-main 77aa012', ['acme/widget dev-main#18f1a03 => dev-main 77aa012']], + 'the commit the site ran is not known' => ['dev-main', 'dev-main 18f1a03', ['acme/widget dev-main => dev-main 18f1a03']], + 'Composer names no commit' => ['dev-main#18f1a03', 'dev-main', ['acme/widget dev-main#18f1a03 => dev-main']], + 'another branch' => ['dev-main#18f1a03', 'dev-next 18f1a03', ['acme/widget dev-main#18f1a03 => dev-next 18f1a03']], 'another release' => ['1.2.0', '1.3.0', ['acme/widget 1.2.0 => 1.3.0']], ]); +test('a package on a branch comes back at the commit the site ran, or not without consent', function (bool $fromRecord) { + $commit = '18f1a03c9d2e4b7a6f0e1d2c3b4a59687f6e5d4c'; + $moved = '77aa012b3c4d5e6f708192a3b4c5d6e7f8091a2b'; + $locked = ['name' => 'acme/widget', 'version' => 'dev-main', 'source' => ['type' => 'git', 'url' => 'https://example.test/widget.git', 'reference' => $commit], 'dist' => ['type' => 'zip', 'reference' => $commit]]; + $core = customPackagesCore(['acme/widget' => 'dev-main'], ['packages' => [$locked]]); + $packages = new CustomComposerPackages($core); + $packages->recordInstalled(['acme/widget']); + $previous = $fromRecord ? [] : $packages->previousVersions(['acme/widget']); + + // The update lays the release's lock over the site, which knows nothing of the package. + file_put_contents($core . 'composer.lock', json_encode(['packages' => []])); + $previous = $fromRecord ? $packages->previousVersions(['acme/widget']) : $previous; + $pins = $packages->pins(['acme/widget'], $previous); + $resolves = fn (string $at) => fn (string $command): array => [0, "Lock file operations: 1 install, 0 updates, 0 removals\n - Locking acme/widget (dev-main " . substr($at, 0, 7) . ")\n"]; + + $same = $packages->plan($resolves($commit), 'composer', $pins, ['acme/widget'], true, false, false); + $branchMoved = $packages->plan($resolves($moved), 'composer', $pins, ['acme/widget'], true, false, false); + $withConsent = $packages->plan($resolves($moved), 'composer', $pins, ['acme/widget'], true, true, false); + removeCustomPackagesCore($core); + + expect($pins)->toBe(['acme/widget' => 'dev-main#' . $commit]) + ->and($same['arguments'])->toContain('acme/widget:dev-main#' . $commit) + ->and($same['ok'])->toBeTrue() + ->and($branchMoved['ok'])->toBeFalse() + ->and($branchMoved['reason'])->toBe('changes') + ->and($withConsent['ok'])->toBeTrue(); +})->with(['read from the lock' => false, 'read from the record of the last restore' => true]); + +test('a record that does not know the commit of a branch asks for consent', function () { + $core = customPackagesCore(['acme/widget' => 'dev-main'], ['packages' => []], null, ['acme/widget' => ['version' => 'dev-main', 'dependencies' => []]]); + $packages = new CustomComposerPackages($core); + $pins = $packages->pins(['acme/widget'], $packages->previousVersions(['acme/widget'])); + $plan = $packages->plan(fn (string $command): array => [0, "Lock file operations: 1 install, 0 updates, 0 removals\n - Locking acme/widget (dev-main 18f1a03)\n"], 'composer', $pins, ['acme/widget'], true, false, false); + removeCustomPackagesCore($core); + + expect($plan['ok'])->toBeFalse()->and($plan['reason'])->toBe('changes'); +}); + +test('only a branch Composer names by a commit is pinned to one', function () { + $commit = '18f1a03c9d2e4b7a6f0e1d2c3b4a59687f6e5d4c'; + $core = customPackagesCore(['acme/widget' => '*'], ['packages' => [ + ['name' => 'acme/widget', 'version' => '1.2.0', 'source' => ['type' => 'git', 'reference' => $commit], 'require' => ['acme/branch' => '*', 'acme/numbered' => '*', 'acme/local' => '*', 'acme/mirror' => '*']], + ['name' => 'acme/branch', 'version' => 'dev-main', 'source' => ['type' => 'git', 'reference' => $commit]], + ['name' => 'acme/numbered', 'version' => '2.x-dev', 'source' => ['type' => 'hg', 'reference' => $commit]], + ['name' => 'acme/local', 'version' => 'dev-main', 'dist' => ['type' => 'path', 'reference' => $commit]], + ['name' => 'acme/mirror', 'version' => 'dev-main', 'source' => ['type' => 'svn', 'reference' => 'trunk@1']], + ]]); + $previous = (new CustomComposerPackages($core))->previousVersions(['acme/widget']); + removeCustomPackagesCore($core); + + expect($previous)->toBe([ + 'acme/widget' => '1.2.0', + 'acme/branch' => 'dev-main#' . $commit, + 'acme/numbered' => '2.x-dev#' . $commit, + 'acme/local' => 'dev-main', + 'acme/mirror' => 'dev-main', + ]); +}); + test('changes are judged on the lock, not on the vendor drift an update package ships', function () { // Composer 2.10 against the unpacked 3.5.8 archive, whose vendor is newer than its own lock. $output = "Loading composer repositories with package information\n" From 2a69122f582fcbcca6af0cc352bcec4a765a1ef0 Mon Sep 17 00:00:00 2001 From: Artur Kyryliuk Date: Fri, 6 Nov 2026 03:32:49 +0100 Subject: [PATCH 2/3] fix(manager): keep paths out of manager_language 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 --- assets/modules/store/installer/index.php | 2 +- .../extrascheck/CheckOutdated.class.php | 2 +- assets/plugins/updater/plugin.updater.php | 5 +- core/src/Core.php | 19 ++++ core/src/Legacy/ErrorHandler.php | 2 +- .../Services/Store/StoreContextService.php | 3 +- .../tests/Unit/ManagerLanguageSettingTest.php | 91 +++++++++++++++++++ core/tests/Unit/Store/StoreLanguageTest.php | 15 +++ manager/actions/mutate_web_user.dynamic.php | 2 +- 9 files changed, 134 insertions(+), 7 deletions(-) create mode 100644 core/tests/Unit/ManagerLanguageSettingTest.php diff --git a/assets/modules/store/installer/index.php b/assets/modules/store/installer/index.php index a8b2f856ce..179754c66f 100644 --- a/assets/modules/store/installer/index.php +++ b/assets/modules/store/installer/index.php @@ -16,7 +16,7 @@ $_lang = array(); $_params = array(); $lang = evo()->config['manager_language']; -if (file_exists($modulePath . '/lang/' . $lang . '.inc.php')){ +if (\EvolutionCMS\Support\ManagerLanguage::isName($lang) && file_exists($modulePath . '/lang/' . $lang . '.inc.php')){ include_once($modulePath . '/lang/' . $lang . '.inc.php'); } else { include_once($modulePath . '/lang/en.inc.php'); diff --git a/assets/plugins/extrascheck/CheckOutdated.class.php b/assets/plugins/extrascheck/CheckOutdated.class.php index e7166e23b3..c61fa5eae5 100644 --- a/assets/plugins/extrascheck/CheckOutdated.class.php +++ b/assets/plugins/extrascheck/CheckOutdated.class.php @@ -398,7 +398,7 @@ public function makeConfigButton($title) public function setLang($lexicon = []) { $lang = $this->modx->getConfig('manager_language'); - if (file_exists( __DIR__ . '/lang/'.$lang.'/core.inc.php')){ + if (\EvolutionCMS\Support\ManagerLanguage::isName($lang) && file_exists( __DIR__ . '/lang/'.$lang.'/core.inc.php')){ include_once(__DIR__ . '/lang/'.$lang.'/core.inc.php'); } else { include_once(__DIR__ . '/lang/english/core.inc.php'); diff --git a/assets/plugins/updater/plugin.updater.php b/assets/plugins/updater/plugin.updater.php index 8c878e4ef3..b9b0bf993b 100644 --- a/assets/plugins/updater/plugin.updater.php +++ b/assets/plugins/updater/plugin.updater.php @@ -852,8 +852,9 @@ function poll() { $_lang = []; $plugin_path = EVO_BASE_PATH . "assets/plugins/updater/"; include($plugin_path . 'lang/en.php'); - if (file_exists($plugin_path . 'lang/' . $modx->config['manager_language'] . '.php')) { - include($plugin_path . 'lang/' . $modx->config['manager_language'] . '.php'); + $language = $modx->config['manager_language']; + if (\EvolutionCMS\Support\ManagerLanguage::isName($language) && file_exists($plugin_path . 'lang/' . $language . '.php')) { + include($plugin_path . 'lang/' . $language . '.php'); } $e = &$modx->Event; diff --git a/core/src/Core.php b/core/src/Core.php index 65e479b09e..14aede3852 100644 --- a/core/src/Core.php +++ b/core/src/Core.php @@ -6822,6 +6822,7 @@ public function getSettings() $this->config = $this->configCompatibility(); $this->loadConfig(); + $this->dropManagerLanguagePath(); // Apply the system timezone before user settings and OnLoadSettings hooks. $siteTimezone = Support\SiteTimezone::apply( @@ -6841,6 +6842,19 @@ public function getSettings() $this->syncManagerTheme(); } + /** + * manager_language names a file to include, here and in packages that read it as it + * stands, and a stored value may be anything: only a locale name gets into the + * configuration. A deleted locale passes, it reads as English where it is used. + */ + private function dropManagerLanguagePath(): void + { + $language = $this->config['manager_language'] ?? ''; + if ($language !== '' && !Support\ManagerLanguage::isName($language)) { + $this->config['manager_language'] = Support\ManagerLanguage::DEFAULT; + } + } + /** * Realign the already resolved ManagerTheme with the current manager_theme and * manager_language. A different theme needs a new instance (the theme name drives the @@ -6934,6 +6948,11 @@ public function getUserSettings() } } + // The same rule as dropManagerLanguagePath(); the system language stays. + if (isset($usrSettings['manager_language']) && !Support\ManagerLanguage::isName($usrSettings['manager_language'])) { + unset($usrSettings['manager_language']); + } + if (empty($usrSettings)) { return []; } diff --git a/core/src/Legacy/ErrorHandler.php b/core/src/Legacy/ErrorHandler.php index 62ca3a4ac5..ca2602d3cb 100644 --- a/core/src/Legacy/ErrorHandler.php +++ b/core/src/Legacy/ErrorHandler.php @@ -78,7 +78,7 @@ public function includeLang($context = 'common') // @phpstan-ignore-next-line includeOnce.fileNotFound include_once($lang_path . 'en/global.php'); $manager_language = $modx->getConfig('manager_language'); - if (is_file($lang_path.$manager_language."/global.php")) { + if (\EvolutionCMS\Support\ManagerLanguage::isName($manager_language) && is_file($lang_path.$manager_language."/global.php")) { include_once($lang_path.$manager_language."/global.php"); } diff --git a/core/src/Services/Store/StoreContextService.php b/core/src/Services/Store/StoreContextService.php index 5225886ba9..ca7ea04d75 100644 --- a/core/src/Services/Store/StoreContextService.php +++ b/core/src/Services/Store/StoreContextService.php @@ -2,6 +2,7 @@ use EvolutionCMS\Models\RolePermissions; use EvolutionCMS\Models\UserRole; +use EvolutionCMS\Support\ManagerLanguage; class StoreContextService { @@ -16,7 +17,7 @@ public function loadLanguage(string $modulePath, string $managerLanguage): array if (file_exists($fallbackFile)) { include $fallbackFile; } - if ($languageFile !== $fallbackFile && file_exists($languageFile)) { + if ($languageFile !== $fallbackFile && ManagerLanguage::isName($managerLanguage) && file_exists($languageFile)) { include $languageFile; } diff --git a/core/tests/Unit/ManagerLanguageSettingTest.php b/core/tests/Unit/ManagerLanguageSettingTest.php new file mode 100644 index 0000000000..848a1dd66e --- /dev/null +++ b/core/tests/Unit/ManagerLanguageSettingTest.php @@ -0,0 +1,91 @@ +addConnection(['driver' => 'sqlite', 'database' => ':memory:']); + $database->bootEloquent(); + $connection = $database->getConnection(); + $connection->getSchemaBuilder()->create('user_settings', function ($table) { + $table->integer('user'); $table->string('setting_name'); $table->string('setting_value'); + }); + $connection->table('user_settings')->insert(['user' => 1, 'setting_name' => 'manager_language', 'setting_value' => $userValue]); + $core = Mockery::mock(Core::class)->makePartial(); + $core->instance('config', new \Illuminate\Config\Repository([])); + $core->config = $config + ['filemanager_path' => '[(base_path)]assets/', 'rb_base_dir' => '[(base_path)]assets/']; + $core->shouldReceive('getLoginUserID')->withNoArgs()->andReturn(1); + $core->shouldReceive('getLoginUserID')->with('mgr')->andReturn(1); + $core->shouldReceive('getLoginUserType')->andReturn('manager'); + $core->shouldReceive('isFrontend')->andReturn(true); + $core->shouldReceive('isBackend')->andReturn(false); + try { + $check($core); + } finally { + if ($previous === null) UserSetting::unsetConnectionResolver(); + else UserSetting::setConnectionResolver($previous); + $_SESSION = $session; + Mockery::close(); + } +} + +test('a per-user manager_language that is a path leaves the system language in place', function (string $value) { + managerLanguageCore(['manager_language' => 'de'], $value, function ($core) { + $settings = $core->getUserSettings(); + + expect($settings)->not->toHaveKey('manager_language') + ->and($core->config['manager_language'])->toBe('de'); + }); +})->with(['../../../assets/files/x', '..\\..\\x', 'de/../uk', '..']); + +test('a per-user manager_language that names a locale still applies, installed or not', function () { + managerLanguageCore(['manager_language' => 'de'], 'uk', function ($core) { + expect($core->getUserSettings()['manager_language'])->toBe('uk') + ->and($core->config['manager_language'])->toBe('uk'); + }); +}); + +test('a system manager_language that is a path reads as English', function (string $value, string $expected) { + $core = Mockery::mock(Core::class)->makePartial(); + $core->config = ['manager_language' => $value]; + + (new ReflectionMethod(Core::class, 'dropManagerLanguagePath'))->invoke($core); + expect($core->config['manager_language'])->toBe($expected); + Mockery::close(); +})->with([ + 'a path' => ['../../x', 'en'], + 'a deleted locale, kept for when it returns' => ['xx', 'xx'], + 'not set yet' => ['', ''], +]); + +test('the system language is checked before anything reads the settings', function () { + $source = (string) file_get_contents(dirname(__DIR__, 2) . '/src/Core.php'); + $load = strpos($source, '$this->loadConfig();', (int) strpos($source, 'public function getSettings()')); + $check = strpos($source, '$this->dropManagerLanguagePath();', $load); + + expect($check)->toBeGreaterThan($load) + ->and($check)->toBeLessThan((int) strpos($source, '$this->getUserSettings();', $load)); +}); + +test('every include named after manager_language asks for a locale name first', function (string $file, string $variable) { + $source = (string) file_get_contents(dirname(__DIR__, 3) . '/' . $file); + + expect($source)->toContain('ManagerLanguage::isName(' . $variable . ')'); +})->with([ + ['core/src/Legacy/ErrorHandler.php', '$manager_language'], + ['core/src/Services/Store/StoreContextService.php', '$managerLanguage'], + ['assets/plugins/updater/plugin.updater.php', '$language'], + ['assets/plugins/extrascheck/CheckOutdated.class.php', '$lang'], + ['assets/modules/store/installer/index.php', '$lang'], + ['manager/actions/mutate_web_user.dynamic.php', '$manager_language'], +]); diff --git a/core/tests/Unit/Store/StoreLanguageTest.php b/core/tests/Unit/Store/StoreLanguageTest.php index e87ba703c0..852d86de01 100644 --- a/core/tests/Unit/Store/StoreLanguageTest.php +++ b/core/tests/Unit/Store/StoreLanguageTest.php @@ -61,6 +61,21 @@ function storeLanguageStrings(string $file): array ->toBe((new StoreContextService())->loadLanguage($storeModule, 'en')); }); +test('a manager language that is no locale name includes nothing', function () { + $module = sys_get_temp_dir() . '/evo-store-language-' . getmypid() . '-' . uniqid(); + mkdir($module . '/lang', 0777, true); + file_put_contents($module . '/lang/en.php', "loadLanguage($module, '../outside'); + unlink($module . '/outside.php'); + unlink($module . '/lang/en.php'); + rmdir($module . '/lang'); + rmdir($module); + + expect($strings)->toBe(['from' => 'english']); +}); + test('no store translation carries keys that english does not have', function () use ($storeModule) { $english = storeLanguageStrings($storeModule . '/lang/en.php'); diff --git a/manager/actions/mutate_web_user.dynamic.php b/manager/actions/mutate_web_user.dynamic.php index da19c90e51..74a575b5a5 100755 --- a/manager/actions/mutate_web_user.dynamic.php +++ b/manager/actions/mutate_web_user.dynamic.php @@ -128,7 +128,7 @@ // include the country list language file $_country_lang = []; -if($manager_language != "english" && file_exists(EVO_MANAGER_PATH . "includes/lang/country/" . $manager_language . "_country.inc.php")) { +if(\EvolutionCMS\Support\ManagerLanguage::isName($manager_language) && $manager_language != "english" && file_exists(EVO_MANAGER_PATH . "includes/lang/country/" . $manager_language . "_country.inc.php")) { include_once EVO_MANAGER_PATH . "includes/lang/country/" . $manager_language . "_country.inc.php"; } else { include_once EVO_MANAGER_PATH . "includes/lang/country/en_country.inc.php"; From 38794e36b6560d6e1c421577876c8e07c5aa03bd Mon Sep 17 00:00:00 2001 From: Artur Kyryliuk Date: Sat, 7 Nov 2026 01:51:36 +0100 Subject: [PATCH 3/3] fix(packages): compare the reference of a branch without a source Composer prints the dist reference of a dev package that has no source, so an unchanged path package read as a change and needed consent on every update. Keep that reference in the pin and the record to compare, and drop it before anything reaches Composer. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 6 +- core/src/Services/CustomComposerPackages.php | 38 ++++-- .../Install/CliInstallCustomPackagesTest.php | 114 +++++++++++++++++- .../Install/WebInstallCustomPackagesTest.php | 14 +++ install/src/functions.php | 1 + 5 files changed, 156 insertions(+), 17 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 37f9371680..70f61fd4c8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -164,8 +164,10 @@ under one policy. Do not add a restore path that skips it: - Versions: what the site ran (`previousVersions()`: the lock while it still lists them, else `core/custom/composer.installed.json`, written by every restore and by `package:installrequire`), pinned by `pins()`; a package the release ships or the core requires keeps the release's version. - A branch from git or hg is pinned at its commit (`dev-main#`); a branch whose commit is not - known on both sides counts as a change, so it needs consent. + A branch from git or hg is pinned at its commit (`dev-main#`). A branch without a source keeps + the dist reference Composer prints for it (`dev-main `, a path repository's hash or HEAD), to + compare only: `composerVersion()` drops it before anything reaches Composer. A branch whose reference is + not known on both sides counts as a change, so it needs consent. - Arguments: `updateArguments()`, `--with-dependencies` (`--minimal-changes` from Composer 2.7), never `--with-all-dependencies`, which moves the core's own packages too. - `plan()` dry run first (`planInstall()`, a `composer install --dry-run`, when nothing needs restoring: diff --git a/core/src/Services/CustomComposerPackages.php b/core/src/Services/CustomComposerPackages.php index 1a3e94ac4e..7d8a2dbc21 100644 --- a/core/src/Services/CustomComposerPackages.php +++ b/core/src/Services/CustomComposerPackages.php @@ -216,7 +216,7 @@ public function installed(array $packages): bool /** * Arguments of the "composer update" that restores the packages. * - * @param array $pins Package name => version ('' leaves the version to Composer). + * @param array $pins Package name => pin ('' leaves the version to Composer). * @param bool $minimalChanges Whether the Composer that runs them knows --minimal-changes (2.7.0+). * @return string Quoted package list followed by the update options. */ @@ -229,7 +229,7 @@ public function updateArguments(array $pins, bool $minimalChanges): string } $package = strtolower((string) $package); - $version = trim((string) $version); + $version = self::composerVersion((string) $version); $packages[$package] = $version !== '' ? $package . ':' . $version : $package; } @@ -687,8 +687,8 @@ public static function unexpectedChanges(array $operations, array $pins, array $ { $custom = array_fill_keys(array_map('strtolower', $packages), true); $sameVersion = static function (string $a, string $b): bool { - // A branch is "dev-main#18f1a03..." as a pin and "dev-main 18f1a03" as Composer prints it. - // A commit on one side only cannot show the branch stayed where it was: that needs consent. + // A branch is "dev-main#18f1a03..." or "dev-main 18f1a03..." as a pin and "dev-main 18f1a03" as Composer prints it. + // A reference on one side only cannot show the branch stayed where it was: that needs consent. $parts = static fn (string $version): array => array_pad(preg_split('/[ #]/', strtolower(ltrim(trim($version), 'vV')), 2), 2, ''); [$versionA, $referenceA] = $parts($a); [$versionB, $referenceB] = $parts($b); @@ -716,6 +716,14 @@ public static function unexpectedChanges(array $operations, array $pins, array $ return $changes; } + /** + * The part of a pin Composer is given: what follows a space is only compared (pinnedVersion()). + */ + public static function composerVersion(string $pin): string + { + return explode(' ', trim($pin), 2)[0]; + } + public static function isPackageName(string $package): bool { return preg_match('~^[a-z0-9][a-z0-9_.-]*/[a-z0-9][a-z0-9_.-]*$~i', $package) === 1; @@ -769,23 +777,31 @@ private function lockPackages(): array /** * The version to bring a locked package back at. "dev-main" alone is wherever the branch - * stands by then, so a branch keeps its commit: "dev-main#18f1a03...". + * stands by then, so a branch from git or hg keeps its commit: "dev-main#18f1a03...". * - * Only for git and hg sources, the ones Composer prints a commit for: no other can be - * compared with what a dry run reports. + * A branch without a source (a path repository) cannot be asked for a commit, but Composer + * prints its dist reference: "dev-main 18f1a03...", kept the same way to compare with what + * a dry run reports and never handed to Composer (composerVersion()). Any other source + * prints no reference, so there is nothing to compare. * * @param array $package Entry of composer.lock. */ private static function pinnedVersion(array $package): string { $version = (string) ($package['version'] ?? ''); + if (!str_starts_with($version, 'dev-') && !str_ends_with($version, '-dev')) { + return $version; + } + $source = (array) ($package['source'] ?? []); $reference = (string) ($source['reference'] ?? ''); - $isBranch = str_starts_with($version, 'dev-') || str_ends_with($version, '-dev'); + if (in_array($source['type'] ?? '', ['git', 'hg'], true) && $reference !== '') { + return $version . '#' . $reference; + } + + $distReference = (string) (((array) ($package['dist'] ?? []))['reference'] ?? ''); - return $isBranch && $reference !== '' && in_array($source['type'] ?? '', ['git', 'hg'], true) - ? $version . '#' . $reference - : $version; + return (string) ($source['type'] ?? '') === '' && $distReference !== '' ? $version . ' ' . $distReference : $version; } /** diff --git a/core/tests/Unit/Install/CliInstallCustomPackagesTest.php b/core/tests/Unit/Install/CliInstallCustomPackagesTest.php index 6206a53311..c9808bae33 100644 --- a/core/tests/Unit/Install/CliInstallCustomPackagesTest.php +++ b/core/tests/Unit/Install/CliInstallCustomPackagesTest.php @@ -21,6 +21,31 @@ $root = dirname(__DIR__, 4); +/** + * A composer.lock entry of a path repository, as Composer writes it. + * + * @param array $require + * @return array + */ +function lockedPathPackage(string $name, ?string $reference, array $require = []): array +{ + return array_filter(['name' => $name, 'version' => 'dev-main', 'dist' => ['type' => 'path', 'url' => 'custom/packages/' . basename($name), 'reference' => $reference], 'require' => $require]); +} + +/** + * A dry run that locks these entries, in Composer's own words: its formatter decides what a version looks like. + * + * @param list> $entries + * @return callable(string): array{0: int, 1: string} + */ +function composerLocks(array $entries): callable +{ + $loader = new Composer\Package\Loader\ArrayLoader(); + $lines = array_map(fn (array $entry): string => ' - ' . strip_tags(Composer\DependencyResolver\Operation\InstallOperation::format($loader->load($entry), true)), $entries); + + return fn (string $command): array => [0, 'Lock file operations: ' . count($lines) . " installs, 0 updates, 0 removals\n" . implode("\n", $lines) . "\n"]; +} + function customPackagesCore(array $require, ?array $lock = null, ?array $installed = null, ?array $record = null, array $coreRequire = []): string { $core = sys_get_temp_dir() . '/evo-custom-packages-' . getmypid() . '-' . uniqid() . '/'; @@ -510,7 +535,9 @@ function removeCustomPackagesCore(string $core): void })->with([ 'the pin and what Composer prints' => ['dev-main#18f1a03c9d2e', 'dev-main 18f1a03', []], 'short and long spelling of one commit' => ['dev-main 18f1a03', 'dev-main 18f1a03c9d2e', []], - 'no commit on either side, as a path repository' => ['dev-main', 'dev-main', []], + 'no reference on either side, as a path repository with reference: none' => ['dev-main', 'dev-main', []], + 'one reference of a package without a source' => ['dev-main 18f1a03c9d2e', 'dev-main 18f1a03', []], + 'another reference of a package without a source' => ['dev-main 18f1a03c9d2e', 'dev-main 77aa012', ['acme/widget dev-main 18f1a03c9d2e => dev-main 77aa012']], 'the branch moved' => ['dev-main#18f1a03', 'dev-main 77aa012', ['acme/widget dev-main#18f1a03 => dev-main 77aa012']], 'the commit the site ran is not known' => ['dev-main', 'dev-main 18f1a03', ['acme/widget dev-main => dev-main 18f1a03']], 'Composer names no commit' => ['dev-main#18f1a03', 'dev-main', ['acme/widget dev-main#18f1a03 => dev-main']], @@ -556,13 +583,14 @@ function removeCustomPackagesCore(string $core): void expect($plan['ok'])->toBeFalse()->and($plan['reason'])->toBe('changes'); }); -test('only a branch Composer names by a commit is pinned to one', function () { +test('only a branch Composer checks out by a commit is pinned to one; a package without a source keeps its reference to compare', function () { $commit = '18f1a03c9d2e4b7a6f0e1d2c3b4a59687f6e5d4c'; $core = customPackagesCore(['acme/widget' => '*'], ['packages' => [ - ['name' => 'acme/widget', 'version' => '1.2.0', 'source' => ['type' => 'git', 'reference' => $commit], 'require' => ['acme/branch' => '*', 'acme/numbered' => '*', 'acme/local' => '*', 'acme/mirror' => '*']], + ['name' => 'acme/widget', 'version' => '1.2.0', 'source' => ['type' => 'git', 'reference' => $commit], 'require' => ['acme/branch' => '*', 'acme/numbered' => '*', 'acme/local' => '*', 'acme/unreferenced' => '*', 'acme/mirror' => '*']], ['name' => 'acme/branch', 'version' => 'dev-main', 'source' => ['type' => 'git', 'reference' => $commit]], ['name' => 'acme/numbered', 'version' => '2.x-dev', 'source' => ['type' => 'hg', 'reference' => $commit]], ['name' => 'acme/local', 'version' => 'dev-main', 'dist' => ['type' => 'path', 'reference' => $commit]], + ['name' => 'acme/unreferenced', 'version' => 'dev-main', 'dist' => ['type' => 'path', 'reference' => null]], ['name' => 'acme/mirror', 'version' => 'dev-main', 'source' => ['type' => 'svn', 'reference' => 'trunk@1']], ]]); $previous = (new CustomComposerPackages($core))->previousVersions(['acme/widget']); @@ -572,11 +600,89 @@ function removeCustomPackagesCore(string $core): void 'acme/widget' => '1.2.0', 'acme/branch' => 'dev-main#' . $commit, 'acme/numbered' => '2.x-dev#' . $commit, - 'acme/local' => 'dev-main', + 'acme/local' => 'dev-main ' . $commit, + 'acme/unreferenced' => 'dev-main', 'acme/mirror' => 'dev-main', ]); }); +test('an unchanged path package and its private dependency come back without consent, update after update', function (bool $fromRecord) { + $locked = [lockedPathPackage('acme/local', 'a1b2c3d4e5f60718293a4b5c6d7e8f9012345678', ['acme/private' => '*']), lockedPathPackage('acme/private', 'b2c3d4e5f60718293a4b5c6d7e8f901234567890')]; + $core = customPackagesCore(['acme/local' => '*'], ['packages' => $locked]); + $packages = new CustomComposerPackages($core); + $packages->recordInstalled(['acme/local']); + + $plans = []; + foreach ([1, 2] as $update) { + $previous = $fromRecord ? [] : $packages->previousVersions(['acme/local']); + file_put_contents($core . 'composer.lock', json_encode(['packages' => []])); + $previous = $fromRecord ? $packages->previousVersions(['acme/local']) : $previous; + $pins = $packages->pins(['acme/local'], $previous); + $plans[] = $packages->plan(composerLocks($locked), 'composer', $pins, ['acme/local'], true, false, false); + + // The restore succeeds: the lock holds the packages again and the record is rewritten. + file_put_contents($core . 'composer.lock', json_encode(['packages' => $locked])); + $packages->recordInstalled(['acme/local']); + } + removeCustomPackagesCore($core); + + foreach ($plans as $plan) { + expect($plan['changes'])->toBe([]) + ->and($plan['ok'])->toBeTrue() + ->and($plan['arguments'])->toStartWith(escapeshellarg('acme/local:dev-main') . ' ' . escapeshellarg('acme/private:dev-main') . ' --with-dependencies'); + } +})->with(['read from the lock' => false, 'read from the record of the last restore' => true]); + +test('a path package whose reference changed needs consent, also when only a dependency moved', function (string $package, bool $fromRecord) { + $locked = [lockedPathPackage('acme/local', 'a1b2c3d4e5f60718293a4b5c6d7e8f9012345678', ['acme/private' => '*']), lockedPathPackage('acme/private', 'b2c3d4e5f60718293a4b5c6d7e8f901234567890')]; + $core = customPackagesCore(['acme/local' => '*'], ['packages' => $locked]); + $packages = new CustomComposerPackages($core); + $packages->recordInstalled(['acme/local']); + $previous = $fromRecord ? [] : $packages->previousVersions(['acme/local']); + file_put_contents($core . 'composer.lock', json_encode(['packages' => []])); + $previous = $fromRecord ? $packages->previousVersions(['acme/local']) : $previous; + $pins = $packages->pins(['acme/local'], $previous); + + $moved = array_map(fn (array $entry): array => $entry['name'] === $package ? lockedPathPackage($package, 'c3d4e5f60718293a4b5c6d7e8f90123456789012', $entry['require'] ?? []) : $entry, $locked); + $refused = $packages->plan(composerLocks($moved), 'composer', $pins, ['acme/local'], true, false, false); + $withConsent = $packages->plan(composerLocks($moved), 'composer', $pins, ['acme/local'], true, true, false); + removeCustomPackagesCore($core); + + expect($refused['ok'])->toBeFalse() + ->and($refused['reason'])->toBe('changes') + ->and($refused['changes'])->toHaveCount(1) + ->and($refused['changes'][0])->toStartWith($package . ' dev-main ') + ->and($withConsent['ok'])->toBeTrue(); +})->with([ + 'the custom package, from the lock' => ['acme/local', false], + 'the custom package, from the record' => ['acme/local', true], + 'its dependency, from the lock' => ['acme/private', false], + 'its dependency, from the record' => ['acme/private', true], +]); + +test('a path package with reference: none passes with no reference on either side', function () { + $locked = [lockedPathPackage('acme/local', null)]; + $core = customPackagesCore(['acme/local' => '*'], ['packages' => $locked]); + $packages = new CustomComposerPackages($core); + $previous = $packages->previousVersions(['acme/local']); + file_put_contents($core . 'composer.lock', json_encode(['packages' => []])); + $pins = $packages->pins(['acme/local'], $previous); + $plan = $packages->plan(composerLocks($locked), 'composer', $pins, ['acme/local'], true, false, false); + removeCustomPackagesCore($core); + + expect($pins)->toBe(['acme/local' => 'dev-main'])->and($plan['ok'])->toBeTrue(); +}); + +test('a reference that is only compared never reaches Composer, a commit does', function () { + $pins = ['acme/local' => 'dev-main 18f1a03', 'acme/branch' => 'dev-main#18f1a03', 'acme/release' => '1.2.0', 'acme/new' => '']; + $arguments = (new CustomComposerPackages(sys_get_temp_dir()))->updateArguments($pins, true); + + expect($arguments)->toStartWith(implode(' ', array_map('escapeshellarg', ['acme/local:dev-main', 'acme/branch:dev-main#18f1a03', 'acme/release:1.2.0', 'acme/new'])) . ' --with-dependencies') + ->and(CustomComposerPackages::composerVersion('dev-main 18f1a03'))->toBe('dev-main') + ->and(CustomComposerPackages::composerVersion('dev-main#18f1a03'))->toBe('dev-main#18f1a03') + ->and(CustomComposerPackages::composerVersion(''))->toBe(''); +}); + test('changes are judged on the lock, not on the vendor drift an update package ships', function () { // Composer 2.10 against the unpacked 3.5.8 archive, whose vendor is newer than its own lock. $output = "Loading composer repositories with package information\n" diff --git a/core/tests/Unit/Install/WebInstallCustomPackagesTest.php b/core/tests/Unit/Install/WebInstallCustomPackagesTest.php index fd5115090d..88cea6ee72 100644 --- a/core/tests/Unit/Install/WebInstallCustomPackagesTest.php +++ b/core/tests/Unit/Install/WebInstallCustomPackagesTest.php @@ -70,6 +70,20 @@ function webRestoreCore(array $require): string } }); +test('the command left to run names a path package without the reference it is compared by', function () use ($root) { + $core = webRestoreCore(['acme/local' => '*']); + file_put_contents($core . 'custom/composer.installed.json', json_encode(['packages' => ['acme/local' => ['version' => 'dev-main 18f1a03c9d2e', 'dependencies' => []]]])); + $composer = $core . (PHP_OS_FAMILY === 'Windows' ? 'composer.bat' : 'composer.sh'); + file_put_contents($composer, PHP_OS_FAMILY === 'Windows' ? "@exit /b 2\r\n" : "#!/bin/sh\nexit 2\n"); + chmod($composer, 0755); + + try { + expect(webRestoreRun($root, $core, $composer)['command'])->toBe('cd core && composer update acme/local:dev-main --with-dependencies --no-dev'); + } finally { + (new Illuminate\Filesystem\Filesystem())->deleteDirectory($core); + } +}); + test('the web update restores the packages last and shows the outcome', function () use ($root) { $controller = (string) file_get_contents($root . '/install/src/controllers/install.php'); $view = (string) file_get_contents($root . '/install/src/template/actions/install.php'); diff --git a/install/src/functions.php b/install/src/functions.php index dd62fc167e..3580b16a76 100644 --- a/install/src/functions.php +++ b/install/src/functions.php @@ -1458,6 +1458,7 @@ function installerRestoreCommand(array $pins): string $packages = []; foreach ($pins as $package => $version) { + $version = \EvolutionCMS\Services\CustomComposerPackages::composerVersion((string) $version); $packages[] = $version !== '' ? $package . ':' . $version : $package; }