From 4a6da49a8839fd422014701c5b40f4a2bd0224a5 Mon Sep 17 00:00:00 2001 From: Ralf Lang Date: Sat, 27 Jun 2026 12:47:49 +0200 Subject: [PATCH 1/4] feat(ci): surface PHPStan watermark+1 advisory in PR comment and lane table --- src/Ci/Run/PrCommentReporter.php | 75 ++++++++- src/Ci/Run/ResultCollector.php | 9 ++ src/Ci/Run/RunCommand.php | 107 +++++++++++-- src/Qc/Task/Phpstan.php | 83 ++++++++-- test/Unit/Ci/Run/PrCommentReporterTest.php | 172 +++++++++++++++++++++ test/Unit/Ci/Run/RunCommandTest.php | 92 +++++++++++ 6 files changed, 516 insertions(+), 22 deletions(-) diff --git a/src/Ci/Run/PrCommentReporter.php b/src/Ci/Run/PrCommentReporter.php index 6419d958..7721d9fe 100644 --- a/src/Ci/Run/PrCommentReporter.php +++ b/src/Ci/Run/PrCommentReporter.php @@ -356,6 +356,39 @@ private function allPhpStanLanesAdvisory(array $results): bool return $seen; } + /** + * Render the watermark+1 advisory suffix for the PHPStan PR-comment + * line. The maintainer sees "— N advisor{y,ies} at level X" appended + * to the success line when the PHPStan task captured advisory data + * for the first failing level above the post-discovery watermark. + * + * Returns an empty string when: + * - No lane reported advisory data (the field is absent or null + * across the board). + * - The advisory error count is zero (would be misleading - if + * level+1 had zero errors, the discovery loop would have raised + * the watermark to that level and there would be no advisory). + * + * @param array $phpstanStats Aggregated stats from + * {@see self::aggregateToolStats()}. + */ + private function formatAdvisoryNextLevelSuffix(array $phpstanStats): string + { + $level = $phpstanStats['advisory_level'] ?? null; + $errors = $phpstanStats['advisory_errors_max'] ?? 0; + + if ($level === null || $errors <= 0) { + return ''; + } + + return sprintf( + ' — %d %s at level %d', + $errors, + $errors === 1 ? 'advisory' : 'advisories', + $level, + ); + } + /** * True when every lane that produced a PHPUnit result reported * `mode: no_test_suite` (no test/ directory found by Unit::run). @@ -578,8 +611,15 @@ private function generateQualityMetrics(array $results, array $findingsByTool = ); } else { $errors = $phpstanStats['errors'] ?? 0; + // Watermark+1 advisory: the maintainer's "promotion + // budget." Shown only when the lane is passing (no + // hard errors) and the PHPStan task captured advisory + // data for the next level above watermark. When the + // watermark fails, advisory data is meaningless because + // there's no level+1 peek. + $advisorySuffix2 = $this->formatAdvisoryNextLevelSuffix($phpstanStats); if ($errors === 0) { - $md .= "- **PHPStan**{$advisorySuffix}: No errors found ✅\n"; + $md .= "- **PHPStan**{$advisorySuffix}: No errors found ✅{$advisorySuffix2}\n"; } else { $md .= sprintf( "- **PHPStan**%s: %d error%s found ⚠️\n", @@ -907,6 +947,39 @@ private function aggregateToolStats(array $results, string $tool): array $value ); } + + // PHPStan watermark+1 advisory: capture per-lane level and + // error count, then surface the max-across-lanes counts so + // the PR comment can show a single "N advisories at level + // M" suffix. Different lanes might end up with slightly + // different post-discovery watermarks (a fix that's + // version-gated could pass level X on PHP 8.3 but not 8.2), + // so taking the max errors at the highest advisory level + // is the safe upper bound for the maintainer's promotion + // budget. + $advisory = $result['advisory_next_level'] ?? null; + if (is_array($advisory) + && isset($advisory['level'], $advisory['errors']) + && is_int($advisory['level']) + && is_int($advisory['errors']) + ) { + // Track the highest advisory level seen and its + // associated error count. If multiple lanes report + // different levels (e.g. lane A says level 4, lane B + // says level 5 because B's actual watermark is higher), + // pick the level seen and report its max-across-lanes + // error count. + $currentLevel = $aggregated['advisory_level'] ?? null; + if ($currentLevel === null || $advisory['level'] > $currentLevel) { + $aggregated['advisory_level'] = $advisory['level']; + $aggregated['advisory_errors_max'] = $advisory['errors']; + } elseif ($advisory['level'] === $currentLevel) { + $aggregated['advisory_errors_max'] = max( + $aggregated['advisory_errors_max'] ?? 0, + $advisory['errors'], + ); + } + } } return $aggregated; diff --git a/src/Ci/Run/ResultCollector.php b/src/Ci/Run/ResultCollector.php index b234a93f..7aa0aecc 100644 --- a/src/Ci/Run/ResultCollector.php +++ b/src/Ci/Run/ResultCollector.php @@ -99,6 +99,15 @@ public function addResultFromFile(string $laneName, string $tool, string $jsonFi // "which tests failed" view across lanes. 'failures' => $data['failures'] ?? [], 'errors_detail' => $data['errors'] ?? [], + // PHPStan watermark+1 advisory: the smallest failing level + // above watermark, captured by the PHPStan task during its + // auto-raise discovery loop. The PR-comment renderer reads + // this through aggregateToolStats() and appends a "N + // advisories at level X" suffix to the PHPStan success + // line. Null when watermark fails, watermark is already + // at the maximum, or the maintainer reaches the maximum + // via auto-raise (no further level to peek at). + 'advisory_next_level' => $data['advisory_next_level'] ?? null, ]; } diff --git a/src/Ci/Run/RunCommand.php b/src/Ci/Run/RunCommand.php index 39f5f0f9..81699c7e 100644 --- a/src/Ci/Run/RunCommand.php +++ b/src/Ci/Run/RunCommand.php @@ -364,20 +364,46 @@ private function generateSummaryMarkdown(): string // Lane results table $md .= "### Lane Results\n\n"; - $md .= "| Lane | PHPUnit | PHPStan | PHP-CS-Fixer | Status |\n"; - $md .= "|------|---------|---------|--------------|--------|\n"; + + // Watermark+1 advisory: add an optional `PHPStan Advisory` + // column when at least one lane has captured advisory data. + // The column is suppressed entirely when every lane is silent + // (watermark already at max, or watermark failing across the + // board) so non-modernised components don't get a useless + // empty column. + $showAdvisoryColumn = $this->anyLaneHasPhpStanAdvisory($results); + + if ($showAdvisoryColumn) { + $md .= "| Lane | PHPUnit | PHPStan | PHPStan Advisory | PHP-CS-Fixer | Status |\n"; + $md .= "|------|---------|---------|------------------|--------------|--------|\n"; + } else { + $md .= "| Lane | PHPUnit | PHPStan | PHP-CS-Fixer | Status |\n"; + $md .= "|------|---------|---------|--------------|--------|\n"; + } foreach ($results as $laneName => $tools) { $laneStatus = $this->isLanePassed($tools) ? '✅' : '❌'; - $md .= sprintf( - "| %s | %s | %s | %s | %s |\n", - $laneName, - $this->formatToolForTable($tools['phpunit'] ?? null, 'phpunit'), - $this->formatToolForTable($tools['phpstan'] ?? null, 'phpstan'), - $this->formatToolForTable($tools['phpcsfixer'] ?? null, 'phpcsfixer'), - $laneStatus - ); + if ($showAdvisoryColumn) { + $md .= sprintf( + "| %s | %s | %s | %s | %s | %s |\n", + $laneName, + $this->formatToolForTable($tools['phpunit'] ?? null, 'phpunit'), + $this->formatToolForTable($tools['phpstan'] ?? null, 'phpstan'), + $this->formatPhpStanAdvisoryForTable($tools['phpstan'] ?? null), + $this->formatToolForTable($tools['phpcsfixer'] ?? null, 'phpcsfixer'), + $laneStatus + ); + } else { + $md .= sprintf( + "| %s | %s | %s | %s | %s |\n", + $laneName, + $this->formatToolForTable($tools['phpunit'] ?? null, 'phpunit'), + $this->formatToolForTable($tools['phpstan'] ?? null, 'phpstan'), + $this->formatToolForTable($tools['phpcsfixer'] ?? null, 'phpcsfixer'), + $laneStatus + ); + } } // Detailed metrics @@ -541,6 +567,67 @@ private function formatPhpCsFixerCell(array $stats, bool $success): string return sprintf('❌ %d files need formatting', $withIssues); } + /** + * True when at least one lane's PHPStan result carries an + * `advisory_next_level` block with a positive error count. Drives + * whether the lane table renders the `PHPStan Advisory` column. A + * lane reporting `passing: true` at the next level should never + * happen (the discovery loop would have auto-raised the watermark + * instead) but guard against it anyway so we don't emit a useless + * empty column. + * + * @param array>> $results + */ + private function anyLaneHasPhpStanAdvisory(array $results): bool + { + foreach ($results as $tools) { + $phpstan = $tools['phpstan'] ?? null; + if (!is_array($phpstan)) { + continue; + } + $advisory = $phpstan['advisory_next_level'] ?? null; + if (!is_array($advisory)) { + continue; + } + $errors = (int) ($advisory['errors'] ?? 0); + if ($errors > 0) { + return true; + } + } + return false; + } + + /** + * Render the `PHPStan Advisory` table cell for a single lane. + * + * Returns `+N @ L` when the PHPStan task captured `N` advisory + * findings at level `L` above this lane's post-discovery watermark, + * or `-` when the lane has no advisory data (watermark failed, + * watermark is already at the maximum, or auto-raise reached the + * maximum). The cell deliberately omits a ✅ icon: the maintainer + * already sees the green status in the adjacent `PHPStan` cell; + * this column is the climbing budget, not a verdict. + * + * @param array|null $phpstanResult The lane's + * PHPStan tool block. + */ + private function formatPhpStanAdvisoryForTable(?array $phpstanResult): string + { + if (!is_array($phpstanResult)) { + return '-'; + } + $advisory = $phpstanResult['advisory_next_level'] ?? null; + if (!is_array($advisory)) { + return '-'; + } + $level = $advisory['level'] ?? null; + $errors = (int) ($advisory['errors'] ?? 0); + if (!is_int($level) || $errors <= 0) { + return '-'; + } + return sprintf('+%d @ %d', $errors, $level); + } + /** * Generate detailed metrics section. * diff --git a/src/Qc/Task/Phpstan.php b/src/Qc/Task/Phpstan.php index 0e15b58d..b5195140 100644 --- a/src/Qc/Task/Phpstan.php +++ b/src/Qc/Task/Phpstan.php @@ -63,6 +63,31 @@ class Phpstan extends Base */ private bool $advisory = false; + /** + * Findings at watermark+1 (or the lowest failing level above the + * post-raise watermark) when watermark itself passes. Surfaced in + * the PR comment as a "budget" for the next promotion. Null when + * + * - watermark fails (no peek attempted), + * - watermark is already at the maximum supported PHPStan level so + * no level+1 exists, + * - or the discovery loop reaches the maximum level without any + * failure (perfect run). + * + * Shape mirrors the testLevel() return value: ['level', 'errors', + * 'files_with_errors', 'passing']. + * + * @var array{level: int, errors: int, files_with_errors: int, passing: bool}|null + */ + private ?array $advisoryNextLevel = null; + + /** + * Maximum PHPStan level the discovery loop will probe. PHPStan + * itself caps at 9; level 10 does not exist. Exposed as a constant + * so the watermark+1 advisory step can avoid out-of-range probes. + */ + private const MAX_PHPSTAN_LEVEL = 9; + /** * Get the name of this task. * @@ -143,6 +168,11 @@ public function run(array &$options = []): int 'errors' => 0, 'file_errors' => 0, ]; + // Reset advisory state: it's only populated when watermark + // passes AND a higher level still has findings. The two + // null-leaving cases (watermark fails, watermark already at + // max) just leave this null. + $this->advisoryNextLevel = null; // Run at watermark level (MUST PASS) $this->getOutput()->running('Testing watermark level ' . $watermark . '...'); @@ -204,7 +234,7 @@ public function run(array &$options = []): int $highestPassing = $watermark; $testLevel = $watermark + 1; - while ($testLevel <= 9) { + while ($testLevel <= self::MAX_PHPSTAN_LEVEL) { $this->getOutput()->info('Testing level ' . $testLevel . '...'); $result = $this->testLevel($binary, $componentPath, $testLevel, $options); @@ -217,6 +247,20 @@ public function run(array &$options = []): int '✗ Level ' . $testLevel . ' fails with ' . $result['errors'] . ' error' . ($result['errors'] !== 1 ? 's' : '') ); + // Watermark+1 advisory: capture the first failing + // level above watermark. The maintainer sees this + // as "you'd pass level N if you fixed M things" in + // the PR comment, without it failing the lane. + // Whether `testLevel` ends up being watermark+1 or + // (post-raise) highestPassing+1 depends on how far + // the loop walked before failing; either way it's + // the right "next budget" for promotion. + $this->advisoryNextLevel = [ + 'level' => $testLevel, + 'errors' => $result['errors'], + 'files_with_errors' => $result['files_with_errors'] ?? 0, + 'passing' => false, + ]; break; // Stop at first failure } } @@ -240,23 +284,32 @@ public function run(array &$options = []): int $finalResult = $this->testLevel($binary, $componentPath, $highestPassing, $options); $this->nativeResults = $finalResult['results']; $this->level = $highestPassing; - } elseif ($highestPassing === 9) { - // Already at maximum level + } elseif ($highestPassing === self::MAX_PHPSTAN_LEVEL) { + // Already at maximum level - no advisory possible since + // PHPStan tops out here. The discovery loop may have + // walked up to here without ever failing, in which case + // $this->advisoryNextLevel is null (no level+1 to peek + // at). If the loop never ran (watermark started at + // MAX), same outcome. $this->getOutput()->ok('✓ Code passes watermark level ' . $watermark . ' (maximum)'); $this->nativeResults = $watermarkResult['results']; $this->level = $watermark; } else { - // At watermark, cannot raise yet - $nextLevel = $watermark + 1; - $nextResult = $this->testLevel($binary, $componentPath, $nextLevel, $options); - + // At watermark, cannot raise yet. The discovery loop's + // first iteration was watermark+1 and it failed, so + // $this->advisoryNextLevel already holds the right + // failing-level data - no need to re-run testLevel. $this->getOutput()->ok( '✓ Code passes watermark level ' . $watermark ); - $this->getOutput()->info( - 'Next level (' . $nextLevel . ') has ' . $nextResult['errors'] - . ' error' . ($nextResult['errors'] !== 1 ? 's' : '') . ' remaining' - ); + if ($this->advisoryNextLevel !== null) { + $this->getOutput()->info(sprintf( + 'Next level (%d) has %d error%s remaining', + $this->advisoryNextLevel['level'], + $this->advisoryNextLevel['errors'], + $this->advisoryNextLevel['errors'] !== 1 ? 's' : '', + )); + } // Use watermark results for output $this->nativeResults = $watermarkResult['results']; @@ -990,6 +1043,14 @@ private function writeJsonResults(string $componentPath, int $exitCode, int $lev 'success' => $this->advisory ? true : ($exitCode === 0), 'mode' => $this->advisory ? 'advisory' : 'enforced', 'statistics' => $this->stats, + // Watermark+1 advisory: surfaces the smallest failing level + // above the post-discovery watermark, so the PR comment can + // show "you'd pass level N if you fixed M things." Null + // when watermark fails (no peek attempted), when the + // discovery loop walks to MAX without a failure, or when + // the maintainer is already at MAX. The PR comment renderer + // skips the suffix entirely when this is null. + 'advisory_next_level' => $this->advisoryNextLevel, ]; $json = json_encode($summary, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES); diff --git a/test/Unit/Ci/Run/PrCommentReporterTest.php b/test/Unit/Ci/Run/PrCommentReporterTest.php index fcae890e..c5089d83 100644 --- a/test/Unit/Ci/Run/PrCommentReporterTest.php +++ b/test/Unit/Ci/Run/PrCommentReporterTest.php @@ -384,6 +384,178 @@ public function testPhpCsFixerWithIssuesRendersAsWarning(): void $this->assertStringContainsString('⚠️', $line); } + /** + * Watermark+1 advisory: the maintainer's promotion budget. When the + * PHPStan task captures an `advisory_next_level` block, the PR + * comment's success line appends "— N advisories at level X" so the + * maintainer sees how close they are to the next promotion. + */ + public function testPhpStanWatermarkAdvisorySuffixAppendedToSuccessLine(): void + { + $results = [ + 'php8.3-dev' => [ + 'phpstan' => [ + 'success' => true, + 'exit_code' => 0, + 'mode' => 'enforced', + 'statistics' => [ + 'files_scanned' => 12, + 'errors' => 0, + ], + 'advisory_next_level' => [ + 'level' => 4, + 'errors' => 12, + 'files_with_errors' => 5, + 'passing' => false, + ], + ], + ], + ]; + $md = $this->generateQualityMetrics($results); + $line = $this->extractPhpStanLine($md); + $this->assertNotNull($line); + $this->assertStringContainsString('No errors found ✅', $line); + $this->assertStringContainsString('12 advisories at level 4', $line); + } + + public function testPhpStanWatermarkAdvisorySingularGrammar(): void + { + // Single-error advisory should read "1 advisory" not "1 advisories". + $results = [ + 'php8.3-dev' => [ + 'phpstan' => [ + 'success' => true, + 'exit_code' => 0, + 'mode' => 'enforced', + 'statistics' => ['files_scanned' => 12, 'errors' => 0], + 'advisory_next_level' => [ + 'level' => 5, + 'errors' => 1, + 'files_with_errors' => 1, + 'passing' => false, + ], + ], + ], + ]; + $md = $this->generateQualityMetrics($results); + $line = $this->extractPhpStanLine($md); + $this->assertNotNull($line); + $this->assertStringContainsString('1 advisory at level 5', $line); + $this->assertStringNotContainsString('1 advisories', $line); + } + + public function testPhpStanWatermarkAdvisoryAbsentWhenNotCaptured(): void + { + // No `advisory_next_level` field at all - typical of a lane + // already at max level or one where watermark failed. The + // success line stays unchanged ("No errors found ✅"). + $results = [ + 'php8.3-dev' => [ + 'phpstan' => [ + 'success' => true, + 'exit_code' => 0, + 'mode' => 'enforced', + 'statistics' => ['files_scanned' => 12, 'errors' => 0], + // no advisory_next_level + ], + ], + ]; + $md = $this->generateQualityMetrics($results); + $line = $this->extractPhpStanLine($md); + $this->assertNotNull($line); + $this->assertStringContainsString('No errors found ✅', $line); + $this->assertStringNotContainsString('advisor', $line); + $this->assertStringNotContainsString('at level', $line); + } + + public function testPhpStanWatermarkAdvisoryNotShownOnHardFailure(): void + { + // Watermark failed AND a lane reported advisory data (defense + // in depth - shouldn't happen in practice). The hard-error + // line wins; we don't render the advisory suffix on a failing + // line because the maintainer needs to fix the watermark + // first. + $results = [ + 'php8.3-dev' => [ + 'phpstan' => [ + 'success' => false, + 'exit_code' => 1, + 'mode' => 'enforced', + 'statistics' => [ + 'files_scanned' => 12, + 'errors' => 4, + ], + 'advisory_next_level' => [ + 'level' => 4, + 'errors' => 12, + 'passing' => false, + ], + ], + ], + ]; + $md = $this->generateQualityMetrics($results); + $line = $this->extractPhpStanLine($md); + $this->assertNotNull($line); + $this->assertStringContainsString('4 errors found', $line); + $this->assertStringContainsString('⚠️', $line); + $this->assertStringNotContainsString('advisor', $line); + } + + public function testPhpStanWatermarkAdvisoryUsesMaxAcrossLanes(): void + { + // Two lanes report advisory data with different counts at the + // same level; PR comment takes the max as the upper-bound + // promotion budget. + $results = [ + 'php8.3-dev' => [ + 'phpstan' => [ + 'success' => true, + 'exit_code' => 0, + 'mode' => 'enforced', + 'statistics' => ['files_scanned' => 12, 'errors' => 0], + 'advisory_next_level' => [ + 'level' => 4, + 'errors' => 8, + 'passing' => false, + ], + ], + ], + 'php8.4-dev' => [ + 'phpstan' => [ + 'success' => true, + 'exit_code' => 0, + 'mode' => 'enforced', + 'statistics' => ['files_scanned' => 12, 'errors' => 0], + 'advisory_next_level' => [ + 'level' => 4, + 'errors' => 12, + 'passing' => false, + ], + ], + ], + ]; + $md = $this->generateQualityMetrics($results); + $line = $this->extractPhpStanLine($md); + $this->assertNotNull($line); + $this->assertStringContainsString('12 advisories at level 4', $line); + $this->assertStringNotContainsString('8 advisories', $line); + } + + /** + * Helper: pull just the PHPStan metric line out of a quality-metrics + * block so assertions don't get false positives from the PHPUnit or + * PHP-CS-Fixer rows. + */ + private function extractPhpStanLine(string $md): ?string + { + foreach (explode("\n", $md) as $line) { + if (str_starts_with($line, '- **PHPStan**')) { + return $line; + } + } + return null; + } + /** * Helper: invoke the private summariseReason via reflection. */ diff --git a/test/Unit/Ci/Run/RunCommandTest.php b/test/Unit/Ci/Run/RunCommandTest.php index 3aeb9c74..48703855 100644 --- a/test/Unit/Ci/Run/RunCommandTest.php +++ b/test/Unit/Ci/Run/RunCommandTest.php @@ -384,4 +384,96 @@ public function testReportsSkippedWhenResultFileMissing(): void // Execute $this->runCommand->execute($this->tempDir); } + + /** + * PHPStan watermark+1 advisory: the lane table renders a separate + * `PHPStan Advisory` column with `+N @ L` cells when any lane + * captured advisory data. anyLaneHasPhpStanAdvisory gates the + * column on at least one lane having errors > 0. + */ + public function testAnyLaneHasPhpStanAdvisoryDetectsCapturedAdvisory(): void + { + $method = new \ReflectionMethod(RunCommand::class, 'anyLaneHasPhpStanAdvisory'); + $method->setAccessible(true); + + // Lane with positive advisory count → true. + $results = [ + 'php8.3-dev' => [ + 'phpstan' => [ + 'success' => true, + 'advisory_next_level' => [ + 'level' => 4, + 'errors' => 12, + 'passing' => false, + ], + ], + ], + ]; + $this->assertTrue($method->invoke($this->runCommand, $results)); + + // Lane with advisory block but zero errors (would-be impossible + // in practice but guard against it) → false. + $resultsZero = [ + 'php8.3-dev' => [ + 'phpstan' => [ + 'success' => true, + 'advisory_next_level' => [ + 'level' => 4, + 'errors' => 0, + 'passing' => true, + ], + ], + ], + ]; + $this->assertFalse($method->invoke($this->runCommand, $resultsZero)); + + // No advisory block at all → false (column suppressed). + $resultsNone = [ + 'php8.3-dev' => [ + 'phpstan' => [ + 'success' => true, + ], + ], + ]; + $this->assertFalse($method->invoke($this->runCommand, $resultsNone)); + } + + public function testFormatPhpStanAdvisoryForTableProducesCompactCell(): void + { + $method = new \ReflectionMethod(RunCommand::class, 'formatPhpStanAdvisoryForTable'); + $method->setAccessible(true); + + // Positive advisory: "+N @ L" + $this->assertSame( + '+12 @ 4', + $method->invoke($this->runCommand, [ + 'advisory_next_level' => [ + 'level' => 4, + 'errors' => 12, + 'passing' => false, + ], + ]), + ); + + // No advisory block: dash. + $this->assertSame( + '-', + $method->invoke($this->runCommand, ['success' => true]), + ); + + // Null lane result: dash. + $this->assertSame('-', $method->invoke($this->runCommand, null)); + + // Zero errors (defensive case): dash, not "+0 @ N". + $this->assertSame( + '-', + $method->invoke($this->runCommand, [ + 'advisory_next_level' => [ + 'level' => 9, + 'errors' => 0, + 'passing' => true, + ], + ]), + ); + } } From 67258ce321973e7773685e8030e6787e5573227f Mon Sep 17 00:00:00 2001 From: Ralf Lang Date: Sat, 27 Jun 2026 13:10:10 +0200 Subject: [PATCH 2/4] feat(deps): detect installer-plugin need via lock walk, drive composer config.allow-plugins --- src/Helper/PlatformResolver.php | 144 +++++++++++-- src/Runner/Dependencies.php | 20 +- src/Task/Release/RefreshCiBootstrapTask.php | 40 +++- src/Wrapper/HordeYml.php | 24 +++ test/Unit/Components/Wrapper/HordeYmlTest.php | 190 ++++++++++++++++++ test/unit/Helper/PlatformResolverTest.php | 124 ++++++++++++ .../Release/RefreshCiBootstrapTaskTest.php | 25 ++- 7 files changed, 540 insertions(+), 27 deletions(-) create mode 100644 test/Unit/Components/Wrapper/HordeYmlTest.php diff --git a/src/Helper/PlatformResolver.php b/src/Helper/PlatformResolver.php index 9b71f4cb..fbe27213 100644 --- a/src/Helper/PlatformResolver.php +++ b/src/Helper/PlatformResolver.php @@ -111,10 +111,14 @@ public function __construct( * @param HordeYmlFile $hordeYml Parsed .horde.yml of the UUT. * @param string $componentDir Directory containing the UUT's * composer.json (sibling of .horde.yml). - * @return array|string> - * Keyed by minor version. Value is the platform list, or - * {@see self::NOT_RESOLVABLE} when composer could not produce a lock - * for that minor. + * @return array, lock: string}|string> + * Keyed by minor version. Value is either a record carrying the + * parsed platform list and the raw lock JSON (success), or + * {@see self::NOT_RESOLVABLE} when composer could not produce a + * lock for that minor. The lock JSON is preserved so callers + * downstream of {@see self::resolveAndShapeForCiPlatform()} can + * run additional extractors against the same resolution without + * a second composer round-trip. */ public function resolveRangeFromHordeYml( HordeYmlFile $hordeYml, @@ -158,10 +162,22 @@ public function resolveRangeFromHordeYml( * - 'not resolvable' renders as a scalar string under the minor * key when composer could not produce a lock for that PHP version. * + * Alongside the platform structure this method also produces the + * `ci-platform-flags` block: a small map of booleans flagging + * properties of the transitive dep tree that downstream writers + * need to know about. Currently the only flag is + * `needs_installer_plugin` (true when any package in any resolved + * minor is type horde-library, horde-application, or requires + * horde/horde-installer-plugin); the composer.json writer reads + * this to emit the matching `config.allow-plugins` entry. + * * Network access: see {@see self::resolveRangeFromHordeYml()}. This * helper inherits the same Packagist round-trips. * - * @return array>|string> + * @return array{ + * 'ci-platform': array>|string>, + * 'ci-platform-flags': array + * } */ public function resolveAndShapeForCiPlatform( HordeYmlFile $hordeYml, @@ -170,13 +186,21 @@ public function resolveAndShapeForCiPlatform( $resolved = $this->resolveRangeFromHordeYml($hordeYml, $componentDir); $ciPlatform = []; - foreach ($resolved as $minor => $entries) { - if (is_string($entries)) { - $ciPlatform[$minor] = $entries; + $needsInstallerPlugin = false; + + foreach ($resolved as $minor => $record) { + if (is_string($record)) { + // NOT_RESOLVABLE sentinel; pass through under the minor + // key as a string scalar. + $ciPlatform[$minor] = $record; continue; } + + // The new richer record carries both the platform list and + // the raw lock JSON. Shape platform-list for the ci-platform + // block, then walk the same lock for the flags. $list = []; - foreach ($entries as [$name, $constraint]) { + foreach ($record['platform'] as [$name, $constraint]) { if (str_starts_with($name, 'ext-') || str_starts_with($name, 'lib-')) { $list[] = $name; } else { @@ -184,9 +208,21 @@ public function resolveAndShapeForCiPlatform( } } $ciPlatform[$minor] = $list; + + // Union across minors: any single minor's resolution + // surfacing the trigger sets the flag for the component. + $flags = $this->extractFlagsFromLock($record['lock']); + if (!empty($flags['needs_installer_plugin'])) { + $needsInstallerPlugin = true; + } } - return $ciPlatform; + return [ + 'ci-platform' => $ciPlatform, + 'ci-platform-flags' => [ + 'needs_installer_plugin' => $needsInstallerPlugin, + ], + ]; } /** @@ -200,8 +236,13 @@ public function resolveAndShapeForCiPlatform( * COMPOSER_ROOT_VERSION so circular * require-dev deps resolve. Empty * disables the env injection. - * @return list|string Either the parsed - * platform list or {@see self::NOT_RESOLVABLE}. + * @return array{platform: list, lock: string}|string + * Either a record carrying the parsed platform list and + * the raw lock JSON, or {@see self::NOT_RESOLVABLE}. The + * lock JSON is returned so additional extractors (flags, + * per-package metadata) can be run against the same + * resolution without paying for a second composer + * round-trip. */ private function resolveSingleForRoot( string $uutComposerJsonPath, @@ -280,7 +321,12 @@ private function resolveSingleForRoot( return self::NOT_RESOLVABLE; } - $extracted = $this->extractFromLock(file_get_contents($lockPath) ?: ''); + $lockJson = file_get_contents($lockPath); + if ($lockJson === false) { + return self::NOT_RESOLVABLE; + } + + $extracted = $this->extractFromLock($lockJson); if ($extracted === []) { // A UUT with no transitive platform requirements at all // is possible but rare; we cannot distinguish that from @@ -290,7 +336,10 @@ private function resolveSingleForRoot( return self::NOT_RESOLVABLE; } - return $extracted; + return [ + 'platform' => $extracted, + 'lock' => $lockJson, + ]; } finally { $this->rmrf($tmpDir); } @@ -356,6 +405,73 @@ public function extractFromLock(string $lockJson): array return self::sortPlatformEntries($found); } + /** + * Walk a composer.lock document and surface boolean facts about the + * transitive dep tree that downstream writers care about. + * + * Currently produces a single key, `needs_installer_plugin`, which + * is true when ANY package in the lock satisfies any of: + * + * - `type` is `horde-library` + * - `type` is `horde-application` + * - `require` contains `horde/horde-installer-plugin` + * + * Composer 2.2+ refuses to run a plugin unless the consuming project + * lists it under `config.allow-plugins`. A component that pulls + * `horde/horde-installer-plugin` in transitively (via any + * horde-library dependency) still needs the plugin active at install + * time, even when the component's own composer.json does not require + * it directly. The composer.json writer reads this flag and emits + * the allow-plugins entry so the install does not silently skip the + * plugin. + * + * Walks both `packages` and `packages-dev`. The dev section is + * normally empty for synthesized resolver projects but is walked + * defensively so a transitive plugin in require-dev is still picked + * up. + * + * @return array{needs_installer_plugin: bool} + */ + public function extractFlagsFromLock(string $lockJson): array + { + $flags = ['needs_installer_plugin' => false]; + + try { + $lock = json_decode($lockJson, true, flags: JSON_THROW_ON_ERROR); + } catch (JsonException) { + return $flags; + } + + if (!is_array($lock)) { + return $flags; + } + + $packages = array_merge( + is_array($lock['packages'] ?? null) ? $lock['packages'] : [], + is_array($lock['packages-dev'] ?? null) ? $lock['packages-dev'] : [], + ); + + foreach ($packages as $package) { + if (!is_array($package)) { + continue; + } + $type = $package['type'] ?? null; + if ($type === 'horde-library' || $type === 'horde-application') { + $flags['needs_installer_plugin'] = true; + // Early exit: one positive is enough to set the flag. + // Continue walking would not change the outcome. + return $flags; + } + $require = $package['require'] ?? null; + if (is_array($require) && array_key_exists('horde/horde-installer-plugin', $require)) { + $flags['needs_installer_plugin'] = true; + return $flags; + } + } + + return $flags; + } + /** * Decide whether a composer require key is a platform package as * defined by composer itself: php, composer meta, ext-*, lib-*. diff --git a/src/Runner/Dependencies.php b/src/Runner/Dependencies.php index 07671368..ed7dd562 100644 --- a/src/Runner/Dependencies.php +++ b/src/Runner/Dependencies.php @@ -110,16 +110,23 @@ private function runPlatformResolve(): void $hordeYml = new HordeYmlFile($hordeYmlPath); $resolver = new PlatformResolver(new Shell($output), $output); - $ciPlatform = $resolver->resolveAndShapeForCiPlatform($hordeYml, $componentDir); + $shaped = $resolver->resolveAndShapeForCiPlatform($hordeYml, $componentDir); + $ciPlatform = $shaped['ci-platform']; + $ciPlatformFlags = $shaped['ci-platform-flags']; if (!empty($this->options['pretend'])) { // Preview mode: surface the fragment that would be written. + // Both keys are shown so the maintainer sees what `--platform` + // would persist. $output->plain('--- ci-platform (preview, not written) ---'); $output->plain(YamlLoader::dump(['ci-platform' => $ciPlatform])); + $output->plain('--- ci-platform-flags (preview, not written) ---'); + $output->plain(YamlLoader::dump(['ci-platform-flags' => $ciPlatformFlags])); return; } $hordeYml->set('ci-platform', $ciPlatform); + $hordeYml->set('ci-platform-flags', $ciPlatformFlags); $hordeYml->save(); $output->ok(sprintf( @@ -127,6 +134,17 @@ private function runPlatformResolve(): void count($ciPlatform), $hordeYmlPath, )); + if ($ciPlatformFlags['needs_installer_plugin']) { + // Surface the marker so the maintainer notices it landed. + // The composer.json writer reads this on the next regen and + // emits the matching allow-plugins entry; without it, + // composer 2.2+ silently disables horde/horde-installer-plugin. + $output->info( + 'ci-platform-flags.needs_installer_plugin = true ' + . '(transitive horde-library/horde-application or direct ' + . 'horde/horde-installer-plugin require detected)', + ); + } } /** diff --git a/src/Task/Release/RefreshCiBootstrapTask.php b/src/Task/Release/RefreshCiBootstrapTask.php index 4efed868..81168806 100644 --- a/src/Task/Release/RefreshCiBootstrapTask.php +++ b/src/Task/Release/RefreshCiBootstrapTask.php @@ -350,7 +350,7 @@ private function refreshCiPlatformBlock(string $componentPath): bool $resolver = $this->resolver ?? new PlatformResolver(new Shell($this->output), $this->output); try { - $newPlatform = $resolver->resolveAndShapeForCiPlatform($hordeYml, $componentPath); + $shaped = $resolver->resolveAndShapeForCiPlatform($hordeYml, $componentPath); } catch (\Throwable $e) { // Network failures or composer-resolver bugs must not // abort an entire release. A stale ci-platform block is @@ -361,24 +361,50 @@ private function refreshCiPlatformBlock(string $componentPath): bool return false; } + $newPlatform = $shaped['ci-platform']; + $newFlags = $shaped['ci-platform-flags']; + + // Compare both halves separately. Either changing counts as a + // substantive update; both unchanged is the no-op case. $existingPlatform = $hordeYml->get('ci-platform'); - $existingNormalised = $this->normaliseForComparison($existingPlatform); - $newNormalised = $this->normaliseForComparison($newPlatform); - if ($existingNormalised !== null && $this->ciPlatformEquivalent($existingNormalised, $newNormalised)) { + $existingPlatformNorm = $this->normaliseForComparison($existingPlatform); + $newPlatformNorm = $this->normaliseForComparison($newPlatform); + $platformChanged = $existingPlatformNorm === null + || !$this->ciPlatformEquivalent($existingPlatformNorm, $newPlatformNorm); + + $existingFlags = $hordeYml->get('ci-platform-flags'); + $existingFlagsNorm = $this->normaliseForComparison($existingFlags); + $newFlagsNorm = $this->normaliseForComparison($newFlags); + $flagsChanged = $existingFlagsNorm === null + || !$this->ciPlatformEquivalent($existingFlagsNorm, $newFlagsNorm); + + if (!$platformChanged && !$flagsChanged) { // Nothing meaningful changed; do not touch the file. return false; } if ($this->pretend) { $this->output->info( - '[PRETEND] Would update ci-platform block in .horde.yml', + '[PRETEND] Would update ci-platform ' + . ($flagsChanged ? 'and ci-platform-flags ' : '') + . 'block in .horde.yml', ); return true; } - $hordeYml->set('ci-platform', $newPlatform); + if ($platformChanged) { + $hordeYml->set('ci-platform', $newPlatform); + } + if ($flagsChanged) { + $hordeYml->set('ci-platform-flags', $newFlags); + } $hordeYml->save(); - $this->output->info('Updated ci-platform block in .horde.yml'); + $this->output->info(sprintf( + 'Updated %s in .horde.yml', + $platformChanged && $flagsChanged + ? 'ci-platform and ci-platform-flags blocks' + : ($platformChanged ? 'ci-platform block' : 'ci-platform-flags block'), + )); return true; } diff --git a/src/Wrapper/HordeYml.php b/src/Wrapper/HordeYml.php index ae528451..7dc98adf 100644 --- a/src/Wrapper/HordeYml.php +++ b/src/Wrapper/HordeYml.php @@ -216,6 +216,13 @@ public function getName(): string * Get allowed Composer plugins * * Components-specific logic: auto-adds horde-installer-plugin for components/applications + * AND for any package whose `.horde.yml` carries a + * `ci-platform-flags.needs_installer_plugin: true` marker. The + * marker is written by `horde-components deps --platform` when the + * resolved transitive dep tree contains a horde-library, a + * horde-application, or any direct require on + * horde/horde-installer-plugin. Without the auto-add, composer 2.2+ + * silently disables the plugin and the install ends up wired wrong. * * @return array|object Allowed plugins */ @@ -231,6 +238,23 @@ public function getAllowedPlugins(): array|object } } + // ci-platform-flags marker: any library that transitively pulls + // horde/horde-installer-plugin (via a horde-library or + // horde-application dep) needs to opt the plugin in explicitly + // for composer 2.2+. The deps --platform pipeline computes the + // marker; we honour it here regardless of the component's own + // type. + $flags = $this->hordeYmlFile->get('ci-platform-flags'); + $needsPlugin = false; + if (is_object($flags)) { + $needsPlugin = !empty($flags->needs_installer_plugin); + } elseif (is_array($flags)) { + $needsPlugin = !empty($flags['needs_installer_plugin']); + } + if ($needsPlugin && !array_key_exists('horde/horde-installer-plugin', $allowedPlugins)) { + $allowedPlugins['horde/horde-installer-plugin'] = true; + } + return (object) $allowedPlugins; } diff --git a/test/Unit/Components/Wrapper/HordeYmlTest.php b/test/Unit/Components/Wrapper/HordeYmlTest.php new file mode 100644 index 00000000..5329426a --- /dev/null +++ b/test/Unit/Components/Wrapper/HordeYmlTest.php @@ -0,0 +1,190 @@ +tmpDir = sys_get_temp_dir() . '/horde-wrapper-yml-' . uniqid(); + mkdir($this->tmpDir, 0o755, true); + } + + protected function tearDown(): void + { + if (is_dir($this->tmpDir)) { + $files = scandir($this->tmpDir) ?: []; + foreach ($files as $f) { + if ($f === '.' || $f === '..') { + continue; + } + @unlink($this->tmpDir . '/' . $f); + } + @rmdir($this->tmpDir); + } + } + + /** + * A library component with no plugin-related metadata should produce + * an empty allow-plugins object. The auto-allow rules only kick in + * for components/applications or for marker-flagged libraries. + */ + public function testPlainLibraryWithoutMarkerProducesEmptyAllowPlugins(): void + { + $this->writeHordeYml(<<<'YAML' + --- + id: test + name: Test + type: library + version: + release: 1.0.0 + api: 1.0.0 + state: + release: stable + api: stable + YAML); + + $wrapper = new HordeYml($this->tmpDir); + $plugins = (array) $wrapper->getAllowedPlugins(); + $this->assertArrayNotHasKey('horde/horde-installer-plugin', $plugins); + } + + /** + * Components and applications get the plugin auto-allowed + * regardless of any marker - that rule predates the marker + * mechanism and is preserved. + */ + public function testComponentTypeAutoAllowsInstallerPlugin(): void + { + $this->writeHordeYml(<<<'YAML' + --- + id: test + name: Test + type: component + version: + release: 1.0.0 + api: 1.0.0 + state: + release: stable + api: stable + YAML); + + $wrapper = new HordeYml($this->tmpDir); + $plugins = (array) $wrapper->getAllowedPlugins(); + $this->assertArrayHasKey('horde/horde-installer-plugin', $plugins); + $this->assertTrue($plugins['horde/horde-installer-plugin']); + } + + public function testApplicationTypeAutoAllowsInstallerPlugin(): void + { + $this->writeHordeYml(<<<'YAML' + --- + id: test + name: Test + type: application + version: + release: 1.0.0 + api: 1.0.0 + state: + release: stable + api: stable + YAML); + + $wrapper = new HordeYml($this->tmpDir); + $plugins = (array) $wrapper->getAllowedPlugins(); + $this->assertArrayHasKey('horde/horde-installer-plugin', $plugins); + $this->assertTrue($plugins['horde/horde-installer-plugin']); + } + + /** + * The ci-platform-flags marker enables auto-allow for library types + * that transitively pull horde/horde-installer-plugin via a + * horde-library dep. The composer.json writer needs this entry or + * composer 2.2+ silently disables the plugin and the install ends + * up wired wrong. + */ + public function testLibraryWithMarkerTrueAutoAllowsInstallerPlugin(): void + { + $this->writeHordeYml(<<<'YAML' + --- + id: test + name: Test + type: library + version: + release: 1.0.0 + api: 1.0.0 + state: + release: stable + api: stable + ci-platform-flags: + needs_installer_plugin: true + YAML); + + $wrapper = new HordeYml($this->tmpDir); + $plugins = (array) $wrapper->getAllowedPlugins(); + $this->assertArrayHasKey('horde/horde-installer-plugin', $plugins); + $this->assertTrue($plugins['horde/horde-installer-plugin']); + } + + /** + * The marker explicitly set to false leaves the library plugin + * allow-list untouched. The deps --platform pipeline writes + * needs_installer_plugin: false when no transitive trigger is + * detected; this case should NOT cause the plugin to be auto-allowed. + */ + public function testLibraryWithMarkerFalseDoesNotAutoAllow(): void + { + $this->writeHordeYml(<<<'YAML' + --- + id: test + name: Test + type: library + version: + release: 1.0.0 + api: 1.0.0 + state: + release: stable + api: stable + ci-platform-flags: + needs_installer_plugin: false + YAML); + + $wrapper = new HordeYml($this->tmpDir); + $plugins = (array) $wrapper->getAllowedPlugins(); + $this->assertArrayNotHasKey('horde/horde-installer-plugin', $plugins); + } + + private function writeHordeYml(string $yaml): void + { + file_put_contents($this->tmpDir . '/.horde.yml', $yaml); + } +} diff --git a/test/unit/Helper/PlatformResolverTest.php b/test/unit/Helper/PlatformResolverTest.php index ed6c93bf..f5f748e6 100644 --- a/test/unit/Helper/PlatformResolverTest.php +++ b/test/unit/Helper/PlatformResolverTest.php @@ -299,4 +299,128 @@ public function testExtractFromLockLeafWithComposerMetaButNoExtensions(): void $this->resolver->extractFromLock($lock), ); } + + /** + * The installer-plugin flag fires for packages declared with + * type horde-library. Any one such package in the resolved tree + * is enough; the maintainer's component needs the plugin opted + * in via config.allow-plugins or composer 2.2+ silently disables + * it. + */ + public function testExtractFlagsFromLockTrueForHordeLibraryType(): void + { + $lock = json_encode([ + 'packages' => [ + [ + 'name' => 'horde/exception', + 'type' => 'horde-library', + 'require' => ['php' => '^8.2'], + ], + ], + ]); + + $this->assertSame( + ['needs_installer_plugin' => true], + $this->resolver->extractFlagsFromLock((string) $lock), + ); + } + + public function testExtractFlagsFromLockTrueForHordeApplicationType(): void + { + $lock = json_encode([ + 'packages' => [ + [ + 'name' => 'horde/mnemo', + 'type' => 'horde-application', + 'require' => ['php' => '^8.2'], + ], + ], + ]); + + $this->assertTrue( + $this->resolver->extractFlagsFromLock((string) $lock)['needs_installer_plugin'] + ); + } + + public function testExtractFlagsFromLockTrueForDirectInstallerPluginRequire(): void + { + // A package that requires horde/horde-installer-plugin directly + // (even with a non-horde type) still triggers the flag - the + // plugin needs to run for that package to be installed + // correctly. + $lock = json_encode([ + 'packages' => [ + [ + 'name' => 'someone/who-uses-it', + 'type' => 'library', + 'require' => [ + 'php' => '^8.2', + 'horde/horde-installer-plugin' => '^2.0', + ], + ], + ], + ]); + + $this->assertTrue( + $this->resolver->extractFlagsFromLock((string) $lock)['needs_installer_plugin'] + ); + } + + public function testExtractFlagsFromLockFalseForPlainLibraryTree(): void + { + // No horde-library, no horde-application, no plugin require: + // the flag stays false. Components that happen to be standalone + // PHP libraries should not have the plugin opted in + // unnecessarily. + $lock = json_encode([ + 'packages' => [ + [ + 'name' => 'psr/log', + 'type' => 'library', + 'require' => ['php' => '^8.0'], + ], + ], + ]); + + $this->assertSame( + ['needs_installer_plugin' => false], + $this->resolver->extractFlagsFromLock((string) $lock), + ); + } + + public function testExtractFlagsFromLockWalksPackagesDev(): void + { + // A horde-library in packages-dev (rare but possible if a + // transitive plugin pulls dev things in) still triggers the + // flag - the resolver's defensive walk over packages-dev is + // exercised here. + $lock = json_encode([ + 'packages' => [ + ['name' => 'psr/log', 'type' => 'library', 'require' => []], + ], + 'packages-dev' => [ + [ + 'name' => 'horde/test', + 'type' => 'horde-library', + 'require' => [], + ], + ], + ]); + + $this->assertTrue( + $this->resolver->extractFlagsFromLock((string) $lock)['needs_installer_plugin'] + ); + } + + public function testExtractFlagsFromLockHandlesMalformedJson(): void + { + $this->assertSame( + ['needs_installer_plugin' => false], + $this->resolver->extractFlagsFromLock('not even close to JSON'), + ); + $this->assertSame( + ['needs_installer_plugin' => false], + $this->resolver->extractFlagsFromLock(''), + ); + } } diff --git a/test/unit/Task/Release/RefreshCiBootstrapTaskTest.php b/test/unit/Task/Release/RefreshCiBootstrapTaskTest.php index 55a4004c..8cc46fe0 100644 --- a/test/unit/Task/Release/RefreshCiBootstrapTaskTest.php +++ b/test/unit/Task/Release/RefreshCiBootstrapTaskTest.php @@ -470,15 +470,27 @@ public function testCiPlatformBlockNotTouchedWhenIdentical(): void * the baseline ci-platform block (default) or the override. The * stub never touches the network. * - * @param array|null $platformOverride + * @param array|null $platformOverride The ci-platform + * block to return. + * Defaults to the + * test's baseline. + * @param array|null $flagsOverride The ci-platform-flags + * block to return. + * Defaults to all + * flags false. */ private function makeTask( bool $pretend = false, ?array $platformOverride = null, + ?array $flagsOverride = null, ): RefreshCiBootstrapTask { $platform = $platformOverride ?? $this->baselineCiPlatform; + $flags = $flagsOverride ?? ['needs_installer_plugin' => false]; $resolver = $this->createStub(PlatformResolver::class); - $resolver->method('resolveAndShapeForCiPlatform')->willReturn($platform); + $resolver->method('resolveAndShapeForCiPlatform')->willReturn([ + 'ci-platform' => $platform, + 'ci-platform-flags' => $flags, + ]); return new RefreshCiBootstrapTask( $this->makeOutput(), @@ -505,9 +517,10 @@ private function seedCurrentWorkflow(): void } /** - * Seed `.horde.yml` carrying the baseline ci-platform block plus - * the minimal set of fields the release pipeline reads (id, name, - * type, version.release, state.release, dependencies.required.php). + * Seed `.horde.yml` carrying the baseline ci-platform and + * ci-platform-flags blocks plus the minimal set of fields the + * release pipeline reads (id, name, type, version.release, + * state.release, dependencies.required.php). */ private function seedHordeYmlWithBaselineCiPlatform(): void { @@ -532,6 +545,8 @@ private function seedHordeYmlWithBaselineCiPlatform(): void '8.3': - php: ^8.2 - ext-mbstring + ci-platform-flags: + needs_installer_plugin: false YAML; $this->writeFile('.horde.yml', $yaml); } From 7cca2bae2367627ed474f7eca613059d6f2d17d4 Mon Sep 17 00:00:00 2001 From: Ralf Lang Date: Tue, 30 Jun 2026 09:15:41 +0200 Subject: [PATCH 3/4] fix(release): preserve files staged by earlier tasks in release commit --- src/Release/HordeRelease.php | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/src/Release/HordeRelease.php b/src/Release/HordeRelease.php index 82561e97..fdfb7493 100644 --- a/src/Release/HordeRelease.php +++ b/src/Release/HordeRelease.php @@ -235,17 +235,33 @@ private function prepareCommitMessage(Context $context): void $releaseNotes ); - // Files modified by release tasks - $filesToCommit = [ + // Baseline files always touched by the release tasks above. Earlier + // tasks (RefreshCiBootstrapTask, future contributors) may have + // already appended their own paths to `files` via Context — we MUST + // preserve those entries, otherwise the bootstrap refresh, deleted + // sibling workflows, and platform-block updates land on disk but + // never make it into the release commit. The leftover working tree + // after a release run is the symptom of clobbering this list. + $existingFiles = $context->getOption('files'); + $filesToCommit = is_array($existingFiles) ? $existingFiles : []; + + foreach ([ '.gitignore', '.horde.yml', 'doc/changelog.yml', 'composer.json', - ]; + ] as $baseline) { + if (!in_array($baseline, $filesToCommit, true)) { + $filesToCommit[] = $baseline; + } + } // Add application sentinel files if they exist $componentPath = $context->getComponentPath(); - if (file_exists($componentPath . '/lib/Application.php')) { + if ( + file_exists($componentPath . '/lib/Application.php') + && !in_array('lib/Application.php', $filesToCommit, true) + ) { $filesToCommit[] = 'lib/Application.php'; } From 965304ae5b4c67bb0f9c00172fab7edec72f3402 Mon Sep 17 00:00:00 2001 From: Ralf Lang Date: Wed, 1 Jul 2026 13:01:48 +0200 Subject: [PATCH 4/4] fix: Relax version guard regex to accept all valid versions rather than a select subset Also provide more useful output on install error --- data/ci/sudo-helper.sh | 15 ++++++-- src/Ci/Setup/ExtensionInstaller.php | 21 ++++++++++-- src/Ci/Setup/PhpInstaller.php | 37 +++++++++++++++++--- src/Ci/Setup/SudoHelper.php | 53 ++++++++++++++++++++--------- 4 files changed, 99 insertions(+), 27 deletions(-) diff --git a/data/ci/sudo-helper.sh b/data/ci/sudo-helper.sh index 56bd14e8..25808586 100644 --- a/data/ci/sudo-helper.sh +++ b/data/ci/sudo-helper.sh @@ -4,7 +4,7 @@ # This script performs all operations that require root privileges: # - Adding ondrej PPA # - Running apt-get update -# - Installing PHP versions (8.2, 8.3, 8.4, 8.5) +# - Installing PHP versions (whichever versions ondrej/php currently ships) # - Installing PHP extensions # # This script is designed to be called by horde-components CI setup @@ -30,8 +30,17 @@ case "$OPERATION" in install-php) # Install PHP version # Usage: install-php 8.4 + # + # The regex only enforces the shape MAJOR.MINOR so command + # injection via shell metacharacters is impossible. Deciding + # which versions are actually installable is not this script's + # job: PhpInstaller picks the matrix, ondrej/php decides what + # apt can resolve, and apt's own "Unable to locate package ..." + # is the authoritative signal. Hard-coding a version range here + # (previously ^8\.[2-5]$) silently breaks the day PHP 8.6 or 9.0 + # ships. PHP_VERSION="$1" - if [[ ! "$PHP_VERSION" =~ ^8\.[2-5]$ ]]; then + if [[ ! "$PHP_VERSION" =~ ^[0-9]+\.[0-9]+$ ]]; then echo "ERROR: Invalid PHP version: $PHP_VERSION" >&2 exit 1 fi @@ -43,7 +52,7 @@ case "$OPERATION" in # Usage: install-extension 8.4 curl PHP_VERSION="$1" EXTENSION="$2" - if [[ ! "$PHP_VERSION" =~ ^8\.[2-5]$ ]]; then + if [[ ! "$PHP_VERSION" =~ ^[0-9]+\.[0-9]+$ ]]; then echo "ERROR: Invalid PHP version: $PHP_VERSION" >&2 exit 1 fi diff --git a/src/Ci/Setup/ExtensionInstaller.php b/src/Ci/Setup/ExtensionInstaller.php index f0b89209..7926e41f 100644 --- a/src/Ci/Setup/ExtensionInstaller.php +++ b/src/Ci/Setup/ExtensionInstaller.php @@ -482,9 +482,24 @@ private function installExtension(string $extension, string $phpVersion): ?bool return null; // Skip } - // Try to install using sudo helper - if (!SudoHelper::installExtension($phpVersion, $extension)) { - $this->output->warn("Failed to install {$package}"); + // Try to install using sudo helper. + // + // Surface apt's captured output on failure — this is the layer + // where "Unable to locate package php8.4-imagick" turns into a + // useful clue for the reader of the CI log. A bare warn() with + // just the package name used to leave people guessing whether + // the package name was wrong, the PPA was missing, or apt was + // in some transient bad state. + $result = SudoHelper::installExtension($phpVersion, $extension); + if (!$result['success']) { + if ($result['output'] !== '') { + $this->output->plain($result['output']); + } + $this->output->warn(sprintf( + 'Failed to install %s (sudo helper exited %d)', + $package, + $result['exitCode'] + )); return false; } diff --git a/src/Ci/Setup/PhpInstaller.php b/src/Ci/Setup/PhpInstaller.php index 8f0d9a55..b02c3624 100644 --- a/src/Ci/Setup/PhpInstaller.php +++ b/src/Ci/Setup/PhpInstaller.php @@ -150,11 +150,26 @@ private function isPpaAdded(): bool /** * Add ondrej PPA. * + * On failure, prints whatever the sudo helper (and any apt commands + * it invoked) wrote to stdout/stderr before returning false, so + * callers don't have to guess whether add-apt-repository, apt-get + * update, or PPA GPG import was the thing that broke. + * * @return bool */ private function addPpa(): bool { - return SudoHelper::addPpa(); + $result = SudoHelper::addPpa(); + if (!$result['success']) { + if ($result['output'] !== '') { + $this->output->plain($result['output']); + } + $this->output->error(sprintf( + 'sudo helper exited %d while adding ondrej/php PPA', + $result['exitCode'] + )); + } + return $result['success']; } /** @@ -273,9 +288,23 @@ private function aptHasPackage(string $package): bool */ private function installPhpVersion(string $version): bool { - // Use sudo helper to install PHP - if (!SudoHelper::installPhp($version)) { - $this->output->error("Failed to install PHP {$version}"); + // Use sudo helper to install PHP. + // + // On failure, print the helper's captured output (which is + // apt-get's own message plus, if the version was rejected by + // the shape guard in sudo-helper.sh, that rejection line) so + // the CI log shows the real reason — e.g. "E: Unable to locate + // package php9.0-cli" — instead of a bare "Failed to install + // PHP 9.0". + $result = SudoHelper::installPhp($version); + if (!$result['success']) { + if ($result['output'] !== '') { + $this->output->plain($result['output']); + } + $this->output->error(sprintf( + "Failed to install PHP {$version} (sudo helper exited %d)", + $result['exitCode'] + )); return false; } diff --git a/src/Ci/Setup/SudoHelper.php b/src/Ci/Setup/SudoHelper.php index cb3ca813..7690f60c 100644 --- a/src/Ci/Setup/SudoHelper.php +++ b/src/Ci/Setup/SudoHelper.php @@ -86,10 +86,13 @@ public static function canRunPasswordless(): bool /** * Add ondrej PPA. * - * @return bool True if successful + * @return array{success: bool, output: string, exitCode: int} + * Combined stdout/stderr captured from the helper is in + * `output` so callers can surface apt's real complaint + * when the helper exits non-zero. * @throws Exception If helper not available */ - public static function addPpa(): bool + public static function addPpa(): array { if (!self::isAvailable()) { throw new Exception('Sudo helper not found: ' . self::HELPER_SCRIPT); @@ -97,21 +100,21 @@ public static function addPpa(): bool $sudo = self::isRoot() ? '' : 'sudo '; $command = $sudo . escapeshellarg(self::HELPER_SCRIPT) . ' add-ppa 2>&1'; - $output = []; - $exitCode = 0; - exec($command, $output, $exitCode); - - return $exitCode === 0; + return self::runCommand($command); } /** * Install PHP version. * * @param string $version PHP version (e.g., '8.4') - * @return bool True if successful + * @return array{success: bool, output: string, exitCode: int} + * Combined stdout/stderr from apt is in `output` so + * callers can print the real "Unable to locate package" + * (or similar) message on failure instead of a bare + * "Failed to install PHP X". * @throws Exception If helper not available */ - public static function installPhp(string $version): bool + public static function installPhp(string $version): array { if (!self::isAvailable()) { throw new Exception('Sudo helper not found: ' . self::HELPER_SCRIPT); @@ -125,11 +128,7 @@ public static function installPhp(string $version): bool escapeshellarg($version) ); - $output = []; - $exitCode = 0; - exec($command, $output, $exitCode); - - return $exitCode === 0; + return self::runCommand($command); } /** @@ -137,10 +136,11 @@ public static function installPhp(string $version): bool * * @param string $phpVersion PHP version (e.g., '8.4') * @param string $extension Extension name (e.g., 'curl') - * @return bool True if successful + * @return array{success: bool, output: string, exitCode: int} + * Combined stdout/stderr from apt is in `output`. * @throws Exception If helper not available */ - public static function installExtension(string $phpVersion, string $extension): bool + public static function installExtension(string $phpVersion, string $extension): array { if (!self::isAvailable()) { throw new Exception('Sudo helper not found: ' . self::HELPER_SCRIPT); @@ -155,11 +155,30 @@ public static function installExtension(string $phpVersion, string $extension): escapeshellarg($extension) ); + return self::runCommand($command); + } + + /** + * Run a helper command and capture stdout/stderr plus exit code. + * + * Centralised so all privileged wrappers surface diagnostics the + * same way. Callers get the exact bytes apt (or the helper's own + * validation) printed; a bare boolean would strand them. + * + * @param string $command Shell command already redirecting 2>&1 + * @return array{success: bool, output: string, exitCode: int} + */ + private static function runCommand(string $command): array + { $output = []; $exitCode = 0; exec($command, $output, $exitCode); - return $exitCode === 0; + return [ + 'success' => $exitCode === 0, + 'output' => implode("\n", $output), + 'exitCode' => $exitCode, + ]; } /**