From 3d2d6ee31330aad50a72e1756af27a4cf51fc766 Mon Sep 17 00:00:00 2001 From: NickSdot Date: Tue, 21 Jul 2026 05:55:27 +0700 Subject: [PATCH 01/15] refactor: reworked command flags, scopes, and diff detection --- README.md | 20 ++ bin/docbook-cs | 2 +- phpunit.xml.dist | 3 + src/Application.php | 153 ++++++-------- src/Diff/DiffParser.php | 16 +- src/Diff/DiffProviderInterface.php | 10 + src/Diff/FileChange.php | 6 +- src/Diff/GitDiffProvider.php | 92 +++++++++ src/Path/DiffPathLoader.php | 101 ++++++++++ src/Path/EntityResolver.php | 59 +++++- src/Process/NativeProcessRunner.php | 38 ++++ src/Process/ProcessResult.php | 15 ++ src/Process/ProcessRunnerInterface.php | 14 ++ src/Runner/RunPlan.php | 23 +++ src/Runner/RunPlanner.php | 79 ++++++++ src/Runner/RunScopeResolver.php | 186 ++++++++++++++++++ src/Runner/SniffRunner.php | 41 +--- tests/Feature/ApplicationInputTest.php | 99 ++++++++++ .../Integration/Diff/GitDiffProviderTest.php | 112 +++++++++++ tests/Integration/Path/DiffPathLoaderTest.php | 80 ++++++++ .../Process/NativeProcessRunnerTest.php | 33 ++++ .../Runner/RunScopeResolverTest.php | 125 ++++++++++++ tests/Unit/ApplicationTest.php | 130 ++---------- tests/Unit/Diff/DiffParserTest.php | 40 ++++ tests/Unit/Path/EntityResolverPathsTest.php | 38 ++++ tests/Unit/Runner/RunPlannerTest.php | 67 +++++++ tests/Unit/Runner/SniffRunnerTest.php | 51 +++-- 27 files changed, 1375 insertions(+), 258 deletions(-) create mode 100644 src/Diff/DiffProviderInterface.php create mode 100644 src/Diff/GitDiffProvider.php create mode 100644 src/Path/DiffPathLoader.php create mode 100644 src/Process/NativeProcessRunner.php create mode 100644 src/Process/ProcessResult.php create mode 100644 src/Process/ProcessRunnerInterface.php create mode 100644 src/Runner/RunPlan.php create mode 100644 src/Runner/RunPlanner.php create mode 100644 src/Runner/RunScopeResolver.php create mode 100644 tests/Feature/ApplicationInputTest.php create mode 100644 tests/Integration/Diff/GitDiffProviderTest.php create mode 100644 tests/Integration/Path/DiffPathLoaderTest.php create mode 100644 tests/Integration/Process/NativeProcessRunnerTest.php create mode 100644 tests/Integration/Runner/RunScopeResolverTest.php create mode 100644 tests/Unit/Path/EntityResolverPathsTest.php create mode 100644 tests/Unit/Runner/RunPlannerTest.php diff --git a/README.md b/README.md index 5ee464c..4be5799 100644 --- a/README.md +++ b/README.md @@ -63,6 +63,26 @@ Register it in your config: ``` +## CLI Scope + +By default, DocbookCS checks the current Git diff from its upstream branch point +through the working tree. Alternatively, a unified diff can be piped or file and +directory paths passed. The inspection scope is limited to the given diff or the +full contents of the given file paths. + +XML references are expanded by default, but reported violations remain limited +to the given scope. With `--wide`, every file inferred from paths or a diff is +checked as a whole, and referenced `SYSTEM` XML files are recursively included. + +| Input | `--wide` | Full File(s) | References | +|------------|---------:|-------------:|-----------:| +| none | no | no | no | +| none | yes | yes | yes | +| path | no | yes | no | +| path | yes | yes | yes | +| piped diff | no | no | no | +| piped diff | yes | yes | yes | + ## License Apache 2.0 diff --git a/bin/docbook-cs b/bin/docbook-cs index 45e5abf..4b72e67 100644 --- a/bin/docbook-cs +++ b/bin/docbook-cs @@ -25,4 +25,4 @@ use DocbookCS\Application; exit(2); })(); -new Application($argv ?? [])->run(); +exit(Application::fromGlobals($argv ?? [])->run()); diff --git a/phpunit.xml.dist b/phpunit.xml.dist index eaa454f..540a1a7 100644 --- a/phpunit.xml.dist +++ b/phpunit.xml.dist @@ -21,6 +21,9 @@ tests/Integration + + tests/Feature + diff --git a/src/Application.php b/src/Application.php index 9827b56..bb36f76 100644 --- a/src/Application.php +++ b/src/Application.php @@ -7,7 +7,6 @@ use DocbookCS\Config\ConfigData; use DocbookCS\Config\ConfigParser; use DocbookCS\Config\ConfigParserException; -use DocbookCS\Diff\DiffParser; use DocbookCS\Progress\ConsoleProgress; use DocbookCS\Progress\NullProgress; use DocbookCS\Progress\ProgressInterface; @@ -15,6 +14,7 @@ use DocbookCS\Report\Reporter\ConsoleReporter; use DocbookCS\Report\Reporter\JsonReporter; use DocbookCS\Report\Reporter\ReporterInterface; +use DocbookCS\Runner\RunPlanner; use DocbookCS\Runner\SniffRunner; final class Application @@ -32,21 +32,50 @@ final class Application /** @var resource */ private $stderr; - /** @var resource */ - private $stdin; + private ?string $stdin; + + /** + * @param list $argv + * @throws \RuntimeException if redirected stdin cannot be read. + * @api + */ + public static function fromGlobals(array $argv): self + { + $stdin = null; + $stat = fstat(STDIN); + + if ($stat === false) { + return new self($argv, stdin: $stdin); + } + + $type = $stat['mode'] & 0170000; + + if ($type === 0010000 || $type === 0100000) { + $stdin = stream_get_contents(STDIN); + + if ($stdin === false) { + throw new \RuntimeException('Could not read diff from stdin.'); + } + } + + return new self($argv, stdin: $stdin); + } /** * @param list $argv * @param ?resource $stdout * @param ?resource $stderr - * @param ?resource $stdin */ - public function __construct(array $argv, mixed $stdout = null, mixed $stderr = null, mixed $stdin = null) - { + public function __construct( + array $argv, + mixed $stdout = null, + mixed $stderr = null, + ?string $stdin = null, + ) { $this->argv = $argv; $this->stdout = $stdout ?? STDOUT; $this->stderr = $stderr ?? STDERR; - $this->stdin = $stdin ?? STDIN; + $this->stdin = $stdin; } /** @@ -54,7 +83,13 @@ public function __construct(array $argv, mixed $stdout = null, mixed $stderr = n */ public function run(): int { - $options = $this->parseArgv(); + try { + $options = $this->parseArgv(); + } catch (\InvalidArgumentException $e) { + $this->writeError('Error: ' . $e->getMessage() . PHP_EOL); + + return 2; + } if ($options['help']) { $this->printHelp(); @@ -76,31 +111,19 @@ public function run(): int return 2; } - $overridePaths = $options['paths'] !== [] ? $options['paths'] : null; - - // If override paths are relative, resolve them against cwd. - if ($overridePaths !== null) { - $overridePaths = $this->resolveOverridePaths($overridePaths); - } - - $diff = null; - - if ($options['diff'] !== null) { - try { - $diffContent = $this->readDiff($options['diff']); - $diff = (new DiffParser())->parse($diffContent); - } catch (\Throwable $e) { - $this->writeError('Error reading diff: ' . $e->getMessage() . PHP_EOL); + try { + $runPlan = new RunPlanner($config, $options['wide'])->plan($options['paths'], $this->stdin); + } catch (\Throwable $e) { + $this->writeError('Error resolving input: ' . $e->getMessage() . PHP_EOL); - return 2; - } + return 2; } $progress = $this->createProgress($options); try { $runner = new SniffRunner($progress); - $report = $runner->run($config, $overridePaths, $diff); + $report = $runner->run($runPlan); } catch (\Throwable $e) { $this->writeError('Runtime error: ' . $e->getMessage() . PHP_EOL); @@ -118,27 +141,6 @@ public function run(): int return (int) $report->hasViolations(); } - /** - * @param list $paths - * @return list - */ - private function resolveOverridePaths(array $paths): array - { - $cwd = getcwd() ?: '.'; - $resolved = []; - - foreach ($paths as $path) { - if (str_starts_with($path, '/') || preg_match('#^[a-zA-Z]:[/\\\\]#', $path)) { - $resolved[] = $path; - continue; - } - - $resolved[] = $cwd . '/' . $path; - } - - return $resolved; - } - /** * @return array{ * help: bool, @@ -148,9 +150,10 @@ private function resolveOverridePaths(array $paths): array * colors: bool, * quiet: bool, * paths: list, - * diff: string|null, + * wide: bool, * perf: bool, * } + * @throws \InvalidArgumentException for unsupported options. */ private function parseArgv(): array { @@ -162,7 +165,7 @@ private function parseArgv(): array 'colors' => $this->detectColorSupport(), 'quiet' => false, 'paths' => [], - 'diff' => null, + 'wide' => false, 'perf' => false, ]; @@ -227,23 +230,14 @@ private function parseArgv(): array continue; } - // --diff = read from stdin - // --diff=FILE = read from file - // --diff=- = read from stdin (explicit) - if ($arg === '--diff') { - $result['diff'] = ''; - $i++; - continue; - } - - if (str_starts_with($arg, '--diff=')) { - $result['diff'] = substr($arg, 7); + if ($arg === '--perf') { + $result['perf'] = true; $i++; continue; } - if ($arg === '--perf') { - $result['perf'] = true; + if ($arg === '--wide') { + $result['wide'] = true; $i++; continue; } @@ -251,33 +245,16 @@ private function parseArgv(): array // Anything else is a path to scan. if (!str_starts_with($arg, '-')) { $result['paths'][] = $arg; + $i++; + continue; } - $i++; + throw new \InvalidArgumentException(sprintf('Unknown option: %s', $arg)); } return $result; } - /** @throws \RuntimeException if the source cannot be read. */ - private function readDiff(string $source): string - { - if ($source === '' || $source === '-') { - $content = stream_get_contents($this->stdin); - if ($content === false) { - throw new \RuntimeException('Could not read diff from stdin.'); // @codeCoverageIgnore - } - return $content; - } - - $content = @file_get_contents($source); - if ($content === false) { - throw new \RuntimeException(sprintf('Could not read diff file: %s', $source)); - } - - return $content; - } - /** @param array{report: string, quiet: bool, colors: bool} $options */ private function createProgress(array $options): ProgressInterface { @@ -382,22 +359,20 @@ private function printHelp(): void --report= Output format: console (default), checkstyle, json. --colors Force ANSI color output. --no-colors Disable ANSI color output. - --diff[=] Restrict analysis to files changed in a unified diff. - Omit the value or pass "-" to read the diff from stdin. - Violations are only reported when the violating element - is on or contains a changed line (parent-context aware). + --wide Check whole selected files and recursively include + referenced XML files. Arguments: One or more files or directories to scan. - If omitted, the paths from the config file are used. + Paths cannot be combined with diff input. Examples: docbook-cs docbook-cs --config=myconfig.xml reference/ docbook-cs --report=checkstyle --no-colors > report.xml docbook-cs reference/strings/functions/strlen.xml - git diff HEAD | docbook-cs --diff --report=checkstyle - docbook-cs --diff=changes.patch --report=json + git diff HEAD | docbook-cs + git diff HEAD | docbook-cs --wide --report=checkstyle HELP; diff --git a/src/Diff/DiffParser.php b/src/Diff/DiffParser.php index 9dbd2fe..0c62f3a 100644 --- a/src/Diff/DiffParser.php +++ b/src/Diff/DiffParser.php @@ -12,12 +12,15 @@ public function parse(string $diff): Diff { /** @var array> $changedLinesByFile */ $changedLinesByFile = []; + /** @var array> $deletionAnchorsByFile */ + $deletionAnchorsByFile = []; $currentFile = null; $deleted = false; $newLineNumber = 0; $oldLinesRemaining = 0; $newLinesRemaining = 0; $inHunk = false; + $previousLineWasDeletion = false; foreach (explode("\n", $diff) as $line) { if (str_starts_with($line, 'diff --git ')) { @@ -25,6 +28,7 @@ public function parse(string $diff): Diff $deleted = false; $newLineNumber = 0; $inHunk = false; + $previousLineWasDeletion = false; continue; } @@ -43,6 +47,7 @@ public function parse(string $diff): Diff $inHunk = false; if ($currentFile !== null && !isset($changedLinesByFile[$currentFile])) { $changedLinesByFile[$currentFile] = []; + $deletionAnchorsByFile[$currentFile] = []; } continue; } @@ -58,6 +63,7 @@ public function parse(string $diff): Diff $newLineNumber = (int) $m[2]; $newLinesRemaining = isset($m[3]) ? (int) $m[3] : 1; $inHunk = true; + $previousLineWasDeletion = false; } continue; } @@ -70,24 +76,32 @@ public function parse(string $diff): Diff $changedLinesByFile[$currentFile][] = $newLineNumber; $newLineNumber++; $newLinesRemaining--; + $previousLineWasDeletion = false; } elseif (str_starts_with($line, '-')) { + if (!$previousLineWasDeletion) { + $deletionAnchorsByFile[$currentFile][] = max(1, $newLineNumber); + } + $oldLinesRemaining--; + $previousLineWasDeletion = true; } elseif (str_starts_with($line, ' ')) { // Context line — present in both old and new file. $newLineNumber++; $oldLinesRemaining--; $newLinesRemaining--; + $previousLineWasDeletion = false; } if ($oldLinesRemaining === 0 && $newLinesRemaining === 0) { $inHunk = false; + $previousLineWasDeletion = false; } } $fileChanges = []; foreach ($changedLinesByFile as $filePath => $lineNumbers) { - $fileChanges[] = new FileChange($filePath, $lineNumbers); + $fileChanges[] = new FileChange($filePath, $lineNumbers, $deletionAnchorsByFile[$filePath]); } return new Diff($fileChanges); diff --git a/src/Diff/DiffProviderInterface.php b/src/Diff/DiffProviderInterface.php new file mode 100644 index 0000000..418a7b4 --- /dev/null +++ b/src/Diff/DiffProviderInterface.php @@ -0,0 +1,10 @@ + $addedLineNumbers */ + /** + * @param list $addedLineNumbers + * @param list $deletionAnchors + */ public function __construct( public string $filePath, public array $addedLineNumbers, + public array $deletionAnchors = [], ) { } } diff --git a/src/Diff/GitDiffProvider.php b/src/Diff/GitDiffProvider.php new file mode 100644 index 0000000..3e7c146 --- /dev/null +++ b/src/Diff/GitDiffProvider.php @@ -0,0 +1,92 @@ +runOrThrow( + ['git', 'rev-parse', '--show-toplevel'], + $workingDirectory, + 'Could not find Git repository.', + )); + + $baseReference = $this->resolveBaseReference($repositoryRoot); + $mergeBase = $this->runOrThrow( + ['git', 'merge-base', 'HEAD', $baseReference], + $repositoryRoot, + sprintf('Unclear where HEAD branched from %s.', $baseReference), + ); + + return $this->runOrThrow( + ['git', 'diff', '--no-ext-diff', '--no-color', trim($mergeBase), '--'], + $repositoryRoot, + 'Could not read diff.', + ); + } + + /** @throws \RuntimeException if no default branch reference exists. */ + private function resolveBaseReference(string $repositoryRoot): string + { + $candidates = []; + + foreach (['upstream', 'origin'] as $remote) { + $result = $this->processRunner->run( + ['git', 'symbolic-ref', '--quiet', sprintf('refs/remotes/%s/HEAD', $remote)], + $repositoryRoot, + ); + + if ($result->exitCode === 0) { + $candidates[] = trim($result->stdout); + } + + $candidates[] = sprintf('refs/remotes/%s/main', $remote); + $candidates[] = sprintf('refs/remotes/%s/master', $remote); + } + + $candidates[] = 'refs/heads/main'; + $candidates[] = 'refs/heads/master'; + + foreach (array_unique($candidates) as $candidate) { + $result = $this->processRunner->run( + ['git', 'rev-parse', '--verify', '--quiet', $candidate . '^{commit}'], + $repositoryRoot, + ); + + if ($result->exitCode === 0) { + return $candidate; + } + } + + throw new \RuntimeException('Could not determine the upstream default branch for the contribution diff.'); + } + + /** + * @param list $command + * @throws \RuntimeException if the command fails. + */ + private function runOrThrow(array $command, string $workingDirectory, string $error): string + { + $result = $this->processRunner->run($command, $workingDirectory); + + if ($result->exitCode === 0) { + return $result->stdout; + } + + $detail = trim($result->stderr); + + throw new \RuntimeException($detail !== '' ? "$error $detail" : $error); + } +} diff --git a/src/Path/DiffPathLoader.php b/src/Path/DiffPathLoader.php new file mode 100644 index 0000000..22706e0 --- /dev/null +++ b/src/Path/DiffPathLoader.php @@ -0,0 +1,101 @@ + $projectRoots */ + public function __construct( + private Diff $diff, + private string $workingDirectory, + private string $basePath, + private array $projectRoots, + private PathMatcher $matcher, + ) { + } + + public function load(): Diff + { + $changes = []; + + foreach ($this->diff->fileChanges as $fileChange) { + foreach ($this->candidates($fileChange->filePath) as $candidate) { + if ( + is_file($candidate) + && str_ends_with(strtolower($candidate), '.xml') + && $this->matcher->isIncluded($candidate) + ) { + $changes[$candidate] = new FileChange( + $candidate, + $fileChange->addedLineNumbers, + $fileChange->deletionAnchors, + ); + break; + } + } + } + + ksort($changes); + + return new Diff(array_values($changes)); + } + + /** @return list */ + private function candidates(string $path): array + { + $path = str_replace('\\', '/', $path); + + if ($this->isAbsolute($path)) { + return [$path]; + } + + $candidates = [ + $this->workingDirectory . '/' . $path, + $this->basePath . '/' . $path, + ]; + + foreach ($this->projectRoots as $root => $directory) { + $candidates[] = $root . '/' . $path; + + $prefix = trim(str_replace('\\', '/', $directory), '/') . '/'; + if ($prefix !== '/' && str_starts_with($path, $prefix)) { + $candidates[] = $root . '/' . substr($path, strlen($prefix)); + } + } + + return array_map($this->normalize(...), $candidates) + |> array_unique(...) + |> array_values(...); + } + + private function isAbsolute(string $path): bool + { + return str_starts_with($path, '/') || preg_match('#^[a-zA-Z]:/#', $path) === 1; + } + + private function normalize(string $path): string + { + $prefix = str_starts_with($path, '/') ? '/' : ''; + $segments = []; + + foreach (explode('/', str_replace('\\', '/', $path)) as $segment) { + if ($segment === '' || $segment === '.') { + continue; + } + + if ($segment === '..' && $segments !== [] && end($segments) !== '..') { + array_pop($segments); + continue; + } + + $segments[] = $segment; + } + + return $prefix . implode('/', $segments); + } +} diff --git a/src/Path/EntityResolver.php b/src/Path/EntityResolver.php index 64d2200..8dc3f29 100644 --- a/src/Path/EntityResolver.php +++ b/src/Path/EntityResolver.php @@ -8,6 +8,12 @@ final class EntityResolver { private string $extension; + /** @var array|null */ + private ?array $resolvedEntities = null; + + /** @var array|null */ + private ?array $resolvedPaths = null; + /** * @param array $projectRoots * @param list $entityPaths @@ -26,15 +32,41 @@ public function __construct( */ public function resolve(): array { + $this->resolveAll(); + + return $this->resolvedEntities ?? []; + } + + /** + * @return array + * @throws \UnexpectedValueException if the directory cannot be read. + */ + public function paths(): array + { + $this->resolveAll(); + + return $this->resolvedPaths ?? []; + } + + /** @throws \UnexpectedValueException if the directory cannot be read. */ + private function resolveAll(): void + { + if ($this->resolvedEntities !== null && $this->resolvedPaths !== null) { + return; + } + $entities = []; + $paths = []; foreach ($this->entityPaths as $path) { foreach ($this->getEntityFiles($path) as $file) { - $entities += $this->resolveFile($file); + $visited = []; + $entities += $this->resolveFile($file, $visited, $paths); } } - return $entities; + $this->resolvedEntities = $entities; + $this->resolvedPaths = $paths; } /** @@ -86,11 +118,13 @@ private function scanDirectory(string $directory): array /** * @param array $visited + * @param array $paths * @return array */ private function resolveFile( string $filePath, - array &$visited = [], + array &$visited, + array &$paths, ?string $originEntity = null ): array { if (isset($visited[$filePath]) || !is_readable($filePath)) { @@ -105,7 +139,7 @@ private function resolveFile( return []; // @codeCoverageIgnore } - $entities = $this->extractEntities($content, $filePath, $visited); + $entities = $this->extractEntities($content, $filePath, $visited, $paths); if ($originEntity !== null) { $entities[$originEntity] = $this->normalize($content); @@ -116,26 +150,30 @@ private function resolveFile( /** * @param array $visited + * @param array $paths * @return array */ private function extractEntities( string $content, string $filePath, - array &$visited + array &$visited, + array &$paths, ): array { return - $this->extractDtdEntities($content, $filePath, $visited) + $this->extractDtdEntities($content, $filePath, $visited, $paths) + $this->extractXmlEntities($content); } /** * @param array $visited + * @param array $paths * @return array */ private function extractDtdEntities( string $content, string $filePath, - array &$visited + array &$visited, + array &$paths, ): array { $result = []; @@ -161,7 +199,12 @@ private function extractDtdEntities( if ($type === 'SYSTEM') { $resolvedPath = $this->resolvePath($filePath, $value); - $result += $this->resolveFile($resolvedPath, $visited, $name); + + if (is_readable($resolvedPath)) { + $paths[$name] ??= $resolvedPath; + } + + $result += $this->resolveFile($resolvedPath, $visited, $paths, $name); continue; } diff --git a/src/Process/NativeProcessRunner.php b/src/Process/NativeProcessRunner.php new file mode 100644 index 0000000..dce353c --- /dev/null +++ b/src/Process/NativeProcessRunner.php @@ -0,0 +1,38 @@ + $command + * @throws \RuntimeException if the process cannot be started. + */ + public function run(array $command, string $workingDirectory): ProcessResult; +} diff --git a/src/Runner/RunPlan.php b/src/Runner/RunPlan.php new file mode 100644 index 0000000..71917e1 --- /dev/null +++ b/src/Runner/RunPlan.php @@ -0,0 +1,23 @@ + $sniffs + * @param array $targets + * @param array $entities + */ + public function __construct( + public array $sniffs, + public array $targets, + public array $entities, + ) { + } +} diff --git a/src/Runner/RunPlanner.php b/src/Runner/RunPlanner.php new file mode 100644 index 0000000..b17c3cf --- /dev/null +++ b/src/Runner/RunPlanner.php @@ -0,0 +1,79 @@ +diffProvider = $diffProvider ?? new GitDiffProvider(); + $this->entityResolver = new EntityResolver( + $config->getProjectRoots(), + $config->getEntityPaths(), + ); + } + + /** + * @param list $paths + * @throws \InvalidArgumentException if paths and a piped diff are both provided. + * @throws \RuntimeException if the contribution diff cannot be determined. + * @throws \UnexpectedValueException if an entity or selected directory cannot be read. + */ + public function plan(array $paths, ?string $pipedDiff): RunPlan + { + if ($paths === []) { + return $this->planDiff(new DiffParser()->parse($pipedDiff ?? $this->diffProvider->for(getcwd() ?: '.'))); + } + + if ($pipedDiff !== null) { + throw new \InvalidArgumentException('Paths cannot be combined with diff input.'); + } + + return $this->planPaths($paths); + } + + /** + * @param list $paths + * @throws \UnexpectedValueException if an entity or selected directory cannot be read. + */ + public function planPaths(array $paths): RunPlan + { + return new RunPlan( + sniffs: $this->config->getSniffs(), + targets: $this->scopeResolver()->resolvePaths($paths), + entities: $this->entityResolver->resolve(), + ); + } + + /** @throws \UnexpectedValueException if an entity directory cannot be read. */ + public function planDiff(Diff $diff): RunPlan + { + return new RunPlan( + sniffs: $this->config->getSniffs(), + targets: $this->scopeResolver()->resolveDiff($diff), + entities: $this->entityResolver->resolve(), + ); + } + + /** @throws \UnexpectedValueException if an entity directory cannot be read. */ + private function scopeResolver(): RunScopeResolver + { + return new RunScopeResolver($this->config, $this->entityResolver->paths(), $this->wide); + } +} diff --git a/src/Runner/RunScopeResolver.php b/src/Runner/RunScopeResolver.php new file mode 100644 index 0000000..cd47f5d --- /dev/null +++ b/src/Runner/RunScopeResolver.php @@ -0,0 +1,186 @@ + $entityPaths */ + public function __construct( + private ConfigData $config, + private array $entityPaths, + private bool $wide = false, + ) { + $this->pathMatcher = new PathMatcher( + $config->getBasePath(), + $config->getExcludePatterns(), + ); + } + + /** + * @param list $paths + * @return array + * @throws \UnexpectedValueException if a selected directory cannot be read. + */ + public function resolvePaths(array $paths): array + { + $targets = []; + + foreach (new PathLoader($this->absolutePaths($paths), $this->pathMatcher)->loadPaths() as $file) { + $targets[$file] = null; + } + + return $this->finalize($targets); + } + + /** @return array */ + public function resolveDiff(Diff $diff): array + { + $resolvedDiff = new DiffPathLoader( + $diff, + getcwd() ?: '.', + $this->config->getBasePath(), + $this->config->getProjectRoots(), + $this->pathMatcher, + )->load(); + + $targets = []; + + foreach ($resolvedDiff->fileChanges as $fileChange) { + $targets[$fileChange->filePath] = $fileChange; + } + + return $this->finalize($targets); + } + + /** + * @param array $targets + * @return array + */ + private function finalize(array $targets): array + { + if ($this->wide) { + $targets = array_fill_keys(array_keys($targets), null); + $this->expandReferencedTargets($targets); + } + + ksort($targets); + + return $targets; + } + + /** + * @param list $paths + * @return list + */ + private function absolutePaths(array $paths): array + { + $workingDirectory = getcwd() ?: '.'; + $absolutePaths = []; + + foreach ($paths as $path) { + if (str_starts_with($path, '/') || preg_match('#^[a-zA-Z]:[/\\\\]#', $path)) { + $absolutePaths[] = $path; + continue; + } + + $absolutePaths[] = $workingDirectory . '/' . $path; + } + + return $absolutePaths; + } + + /** @param array $targets */ + private function expandReferencedTargets(array &$targets): void + { + $pending = array_keys($targets); + $visitedFiles = []; + $visitedEntityPaths = []; + + for ($i = 0; isset($pending[$i]); $i++) { + $file = $pending[$i]; + + if (isset($visitedFiles[$file])) { + continue; + } + + $visitedFiles[$file] = true; + $content = @file_get_contents($file); + + if ($content === false) { + continue; + } + + foreach ($this->targetFilesFromContent($content, $visitedEntityPaths) as $targetFile) { + if (array_key_exists($targetFile, $targets)) { + continue; + } + + $targets[$targetFile] = null; + $pending[] = $targetFile; + } + } + } + + /** + * @param array $visitedEntityPaths + * @return list + */ + private function targetFilesFromContent(string $content, array &$visitedEntityPaths): array + { + if (!preg_match_all(self::ENTITY_PATTERN, $content, $matches)) { + return []; + } + + $files = []; + + foreach ($matches[1] as $name) { + if (!isset($this->entityPaths[$name])) { + continue; + } + + foreach ($this->expandEntityPath($this->entityPaths[$name], $visitedEntityPaths) as $file) { + $files[$file] = true; + } + } + + return array_keys($files); + } + + /** + * @param array $visited + * @return list + */ + private function expandEntityPath(string $path, array &$visited): array + { + $path = str_replace('\\', '/', $path); + + if (isset($visited[$path]) || !is_file($path)) { + return []; + } + + $visited[$path] = true; + + if (str_ends_with($path, '.xml')) { + return $this->pathMatcher->isIncluded($path) ? [$path] : []; + } + + $content = @file_get_contents($path); + + return $content !== false + ? $this->targetFilesFromContent($content, $visited) + : []; + } +} diff --git a/src/Runner/SniffRunner.php b/src/Runner/SniffRunner.php index 2884155..923919b 100644 --- a/src/Runner/SniffRunner.php +++ b/src/Runner/SniffRunner.php @@ -4,12 +4,7 @@ namespace DocbookCS\Runner; -use DocbookCS\Config\ConfigData; use DocbookCS\Config\SniffEntry; -use DocbookCS\Diff\Diff; -use DocbookCS\Path\EntityResolver; -use DocbookCS\Path\PathLoader; -use DocbookCS\Path\PathMatcher; use DocbookCS\Progress\NullProgress; use DocbookCS\Progress\ProgressInterface; use DocbookCS\Report\Report; @@ -25,45 +20,29 @@ public function __construct(?ProgressInterface $progress = null) } /** - * @param list|null $overridePaths * @throws \RuntimeException if a sniff class cannot be found or does not implement SniffInterface. - * @throws \UnexpectedValueException if no files are found to scan. */ - public function run(ConfigData $config, ?array $overridePaths = null, ?Diff $diff = null): Report + public function run(RunPlan $plan): Report { $startTime = microtime(true); - $sniffs = $this->instantiateSniffs($config->getSniffs()); - - $matcher = new PathMatcher($config->getBasePath(), $config->getExcludePatterns()); - - $includePaths = $overridePaths ?? $config->getIncludePaths(); - - $entityResolver = new EntityResolver($config->getProjectRoots(), $config->getEntityPaths()); - $entities = $entityResolver->resolve(); - - $pathLoader = new PathLoader($includePaths, $matcher); - $files = $pathLoader->loadPaths(); - - if ($diff !== null) { - $files = array_values(array_filter( - $files, - static fn(string $file): bool => $diff->changeFor($file) !== null, - )); - } + $sniffs = $this->instantiateSniffs($plan->sniffs); $report = new Report(); - $preprocessor = new EntityPreprocessor($entities); + $preprocessor = new EntityPreprocessor($plan->entities); $processor = new XmlFileProcessor($sniffs, $preprocessor, $report); - $total = count($files); + $total = count($plan->targets); $this->progress->start($total); - foreach ($files as $index => $file) { + $index = 0; + foreach ($plan->targets as $file => $fileChange) { $report->incrementFilesScanned(); - $changedLines = $diff?->changeFor($file)?->addedLineNumbers; + $changedLines = $fileChange !== null + ? array_values(array_unique([...$fileChange->addedLineNumbers, ...$fileChange->deletionAnchors])) + : null; $fileReport = $processor->processFile( $file, @@ -77,7 +56,7 @@ public function run(ConfigData $config, ?array $overridePaths = null, ?Diff $dif $report->addFileReport($fileReport); } - $this->progress->advance($index + 1, $file, $violationCount); + $this->progress->advance(++$index, $file, $violationCount); } $this->progress->finish(); diff --git a/tests/Feature/ApplicationInputTest.php b/tests/Feature/ApplicationInputTest.php new file mode 100644 index 0000000..a68ac9a --- /dev/null +++ b/tests/Feature/ApplicationInputTest.php @@ -0,0 +1,99 @@ +stdout = $stdout; + $this->stderr = $stderr; + } + + #[Test] + public function itRejectsTheRemovedDiffOption(): void + { + $app = new Application( + ['docbook-cs', '--config=' . self::VALID_CONFIG, '--diff'], + $this->stdout, + $this->stderr, + ); + + self::assertSame(2, $app->run()); + self::assertStringContainsString('Unknown option: --diff', $this->readStream($this->stderr)); + } + + #[Test] + public function itDetectsAPipedDiffWithoutAFlag(): void + { + $app = new Application( + ['docbook-cs', '--config=' . self::VALID_CONFIG], + $this->stdout, + $this->stderr, + stdin: '', + ); + + self::assertSame(0, $app->run()); + self::assertSame('', $this->readStream($this->stderr)); + } + + #[Test] + public function itRejectsPathsCombinedWithAPipedDiff(): void + { + $app = new Application( + ['docbook-cs', '--config=' . self::VALID_CONFIG, self::SCAN_FILE], + $this->stdout, + $this->stderr, + stdin: '', + ); + + self::assertSame(2, $app->run()); + self::assertStringContainsString('Paths cannot be combined with diff input', $this->readStream($this->stderr)); + } + + #[Test] + public function itIncludesTheWideOptionInHelp(): void + { + $app = new Application(['docbook-cs', '--help'], $this->stdout, $this->stderr); + + $app->run(); + + $output = $this->readStream($this->stdout); + + self::assertStringContainsString('--wide', $output); + self::assertStringNotContainsString('--diff', $output); + } + + /** @param resource $stream */ + private function readStream(mixed $stream): string + { + rewind($stream); + + return stream_get_contents($stream) ?: ''; + } +} diff --git a/tests/Integration/Diff/GitDiffProviderTest.php b/tests/Integration/Diff/GitDiffProviderTest.php new file mode 100644 index 0000000..3cf7dc9 --- /dev/null +++ b/tests/Integration/Diff/GitDiffProviderTest.php @@ -0,0 +1,112 @@ +processRunner = new NativeProcessRunner(); + + $tmpDir = sys_get_temp_dir() . '/docbook-cs-git-diff-' . bin2hex(random_bytes(6)); + mkdir($tmpDir); + $this->repository = $tmpDir; + } + + protected function tearDown(): void + { + $files = new \RecursiveIteratorIterator( + new \RecursiveDirectoryIterator($this->repository, \FilesystemIterator::SKIP_DOTS), + \RecursiveIteratorIterator::CHILD_FIRST, + ); + + foreach ($files as $file) { + if (!$file instanceof \SplFileInfo) { + throw new \UnexpectedValueException('Unexpected directory entry.'); + } + + $file->isDir() ? rmdir($file->getPathname()) : unlink($file->getPathname()); + } + + rmdir($this->repository); + } + + #[Test] + public function itDiffsTheWorkingTreeFromTheUpstreamBranchPoint(): void + { + $this->git('init', '--quiet', '--initial-branch=main'); + $this->configureAuthor(); + + file_put_contents($this->repository . '/base.xml', "base\n"); + $this->git('add', 'base.xml'); + $this->git('commit', '--quiet', '-m', 'Base'); + + $base = $this->git('rev-parse', 'HEAD'); + $this->git('update-ref', 'refs/remotes/upstream/main', $base); + $this->git('symbolic-ref', 'refs/remotes/upstream/HEAD', 'refs/remotes/upstream/main'); + $this->git('switch', '--quiet', '-c', 'contribution'); + $this->git('branch', '--delete', '--force', 'main'); + + file_put_contents($this->repository . '/committed.xml', "committed\n"); + $this->git('add', 'committed.xml'); + $this->git('commit', '--quiet', '-m', 'Contribution'); + file_put_contents($this->repository . '/base.xml', "working tree\n"); + + $diff = new GitDiffProvider($this->processRunner)->for($this->repository); + + self::assertStringContainsString('diff --git a/base.xml b/base.xml', $diff); + self::assertStringContainsString('+working tree', $diff); + self::assertStringContainsString('diff --git a/committed.xml b/committed.xml', $diff); + self::assertStringContainsString('+committed', $diff); + } + + #[Test] + public function itFailsClearlyWhenNoUpstreamDefaultBranchCanBeFound(): void + { + $this->git('init', '--quiet', '--initial-branch=contribution'); + $this->configureAuthor(); + + file_put_contents($this->repository . '/base.xml', "base\n"); + $this->git('add', 'base.xml'); + $this->git('commit', '--quiet', '-m', 'Contribution'); + + $this->expectException(\RuntimeException::class); + $this->expectExceptionMessageIsOrContains('Could not determine the upstream default branch'); + + new GitDiffProvider($this->processRunner)->for($this->repository); + } + + private function configureAuthor(): void + { + $this->git('config', 'user.name', 'DocbookCS Tests'); + $this->git('config', 'user.email', 'docbook-cs@example.invalid'); + } + + private function git(string ...$arguments): string + { + $result = $this->processRunner->run(array_values(['git', ...$arguments]), $this->repository); + + self::assertSame(0, $result->exitCode, $result->stderr); + + return trim($result->stdout); + } +} diff --git a/tests/Integration/Path/DiffPathLoaderTest.php b/tests/Integration/Path/DiffPathLoaderTest.php new file mode 100644 index 0000000..47ba2e4 --- /dev/null +++ b/tests/Integration/Path/DiffPathLoaderTest.php @@ -0,0 +1,80 @@ +directory = sys_get_temp_dir() . '/docbook-cs-diff-path-' . bin2hex(random_bytes(6)); + mkdir($this->directory); + } + + protected function tearDown(): void + { + @unlink($this->directory . '/chapter.xml'); + @unlink($this->directory . '/excluded.xml'); + @rmdir($this->directory); + } + + #[Test] + public function itLoadsChangedXmlFilesWithoutScanningConfiguredPaths(): void + { + $file = $this->directory . '/chapter.xml'; + file_put_contents($file, ''); + + $loader = new DiffPathLoader( + new Diff([new FileChange('docs/chapter.xml', [1])]), + workingDirectory: dirname($this->directory), + basePath: $this->directory, + projectRoots: [$this->directory => 'docs'], + matcher: new PathMatcher($this->directory, []), + ); + + $change = $loader->load()->changeFor($file); + + self::assertNotNull($change); + self::assertSame([1], $change->addedLineNumbers); + } + + #[Test] + public function itIgnoresMissingNonXmlAndExcludedFiles(): void + { + $excluded = $this->directory . '/excluded.xml'; + file_put_contents($excluded, ''); + + $loader = new DiffPathLoader( + new Diff([ + new FileChange('excluded.xml', [1]), + new FileChange('notes.txt', [1]), + new FileChange('missing.xml', [1]), + ]), + workingDirectory: $this->directory, + basePath: $this->directory, + projectRoots: [], + matcher: new PathMatcher($this->directory, ['excluded.xml']), + ); + + self::assertSame([], $loader->load()->fileChanges); + } +} diff --git a/tests/Integration/Process/NativeProcessRunnerTest.php b/tests/Integration/Process/NativeProcessRunnerTest.php new file mode 100644 index 0000000..9b930fb --- /dev/null +++ b/tests/Integration/Process/NativeProcessRunnerTest.php @@ -0,0 +1,33 @@ +run( + [PHP_BINARY, '-r', 'fwrite(STDOUT, "out"); fwrite(STDERR, "err"); exit(7);'], + getcwd() ?: '.', + ); + + self::assertSame(7, $result->exitCode); + self::assertSame('out', $result->stdout); + self::assertSame('err', $result->stderr); + } +} diff --git a/tests/Integration/Runner/RunScopeResolverTest.php b/tests/Integration/Runner/RunScopeResolverTest.php new file mode 100644 index 0000000..9bde123 --- /dev/null +++ b/tests/Integration/Runner/RunScopeResolverTest.php @@ -0,0 +1,125 @@ +directory = sys_get_temp_dir() . '/docbook-cs-scope-' . bin2hex(random_bytes(6)); + mkdir($this->directory); + + $this->sourceFile = $this->directory . '/source.xml'; + $this->targetFile = $this->directory . '/target.xml'; + $this->entityFile = $this->directory . '/bridge.ent'; + + file_put_contents($this->sourceFile, '&bridge;'); + file_put_contents($this->targetFile, ''); + file_put_contents($this->entityFile, '⌖'); + } + + protected function tearDown(): void + { + @unlink($this->sourceFile); + @unlink($this->targetFile); + @unlink($this->entityFile); + @rmdir($this->directory); + } + + #[Test] + public function narrowScopeKeepsOnlySelectedFilesAndDiffLines(): void + { + $targets = $this->resolver()->resolveDiff(new Diff([new FileChange('source.xml', [2, 3])])); + + self::assertSame([2, 3], $targets[$this->sourceFile]?->addedLineNumbers); + self::assertCount(1, $targets); + } + + #[Test] + public function wideScopeWidensSelectedFilesAndFollowsReferencedTargets(): void + { + $targets = $this->resolver(wide: true)->resolveDiff(new Diff([new FileChange('source.xml', [2, 3])])); + + self::assertNull($targets[$this->sourceFile]); + self::assertNull($targets[$this->targetFile]); + self::assertCount(2, $targets); + } + + #[Test] + public function expandedScopeHonorsTargetExclusions(): void + { + $resolver = new RunScopeResolver( + $this->config(['target.xml']), + [ + 'bridge' => $this->entityFile, + 'target' => $this->targetFile, + ], + wide: true, + ); + + self::assertSame([$this->sourceFile => null], $resolver->resolvePaths([$this->sourceFile])); + } + + #[Test] + public function pathScopeResolvesRelativePathsAgainstTheWorkingDirectory(): void + { + $path = 'tests/fixtures/sniff_runner/default/file_a.xml'; + $absolutePath = (getcwd() ?: '.') . '/' . $path; + + self::assertSame([$absolutePath => null], $this->resolver()->resolvePaths([$path])); + } + + private function resolver(bool $wide = false): RunScopeResolver + { + return new RunScopeResolver( + $this->config(), + [ + 'bridge' => $this->entityFile, + 'target' => $this->targetFile, + ], + $wide, + ); + } + + /** @param list $excludePatterns */ + private function config(array $excludePatterns = []): ConfigData + { + return new ConfigData( + projectRoots: [], + sniffs: [], + includePaths: [$this->sourceFile], + excludePatterns: $excludePatterns, + entityPaths: [], + basePath: $this->directory, + ); + } +} diff --git a/tests/Unit/ApplicationTest.php b/tests/Unit/ApplicationTest.php index 275288a..50f1f09 100644 --- a/tests/Unit/ApplicationTest.php +++ b/tests/Unit/ApplicationTest.php @@ -6,15 +6,19 @@ use DocbookCS\Application; use DocbookCS\Config\ConfigData; -use DocbookCS\Diff\Diff; -use DocbookCS\Diff\DiffParser; -use DocbookCS\Diff\FileChange; use DocbookCS\Config\ConfigParser; use DocbookCS\Config\ConfigParserException; use DocbookCS\Config\SniffEntry; +use DocbookCS\Diff\Diff; +use DocbookCS\Diff\DiffParser; +use DocbookCS\Diff\FileChange; +use DocbookCS\Diff\GitDiffProvider; +use DocbookCS\Path\DiffPathLoader; use DocbookCS\Path\EntityResolver; use DocbookCS\Path\PathLoader; use DocbookCS\Path\PathMatcher; +use DocbookCS\Process\NativeProcessRunner; +use DocbookCS\Process\ProcessResult; use DocbookCS\Progress\NullProgress; use DocbookCS\Report\FileReport; use DocbookCS\Report\Report; @@ -22,6 +26,9 @@ use DocbookCS\Report\Reporter\ConsoleReporter; use DocbookCS\Report\Reporter\JsonReporter; use DocbookCS\Runner\EntityPreprocessor; +use DocbookCS\Runner\RunPlan; +use DocbookCS\Runner\RunPlanner; +use DocbookCS\Runner\RunScopeResolver; use DocbookCS\Runner\SniffRunner; use DocbookCS\Runner\XmlFileProcessor; use DocbookCS\Sniff\ExceptionNameSniff; @@ -47,11 +54,18 @@ CoversClass(PathLoader::class), CoversClass(PathMatcher::class), CoversClass(Report::class), + CoversClass(RunPlan::class), + CoversClass(RunPlanner::class), CoversClass(SniffEntry::class), CoversClass(SniffRunner::class), CoversClass(XmlFileProcessor::class), UsesClass(Diff::class), + UsesClass(DiffPathLoader::class), UsesClass(FileChange::class), + UsesClass(GitDiffProvider::class), + UsesClass(NativeProcessRunner::class), + UsesClass(ProcessResult::class), + UsesClass(RunScopeResolver::class), ] final class ApplicationTest extends TestCase { @@ -249,7 +263,7 @@ public function itResolvesRelativeOverridePathsAgainstCwd(): void public function itCatchesRuntimeErrorFromRunner(): void { $app = new Application( - ['docbook-cs', '--config=' . self::INVALID_SNIFF_CONFIG], + ['docbook-cs', '--config=' . self::INVALID_SNIFF_CONFIG, self::SCAN_FILE], $this->stdout, $this->stderr, ); @@ -288,114 +302,6 @@ public function itPassesThroughAbsoluteOverridePaths(): void self::assertNotSame(2, $exitCode); } - #[Test] - public function itSupportsDiffFromFile(): void - { - $diffFile = tempnam(sys_get_temp_dir(), 'docbookcs_test_'); - self::assertIsString($diffFile); - - // Diff that references no XML files the config would normally scan. - file_put_contents($diffFile, <<<'DIFF' -diff --git a/nonexistent.xml b/nonexistent.xml ---- a/nonexistent.xml -+++ b/nonexistent.xml -@@ -1,1 +1,2 @@ - line1 -+line2 -DIFF); - - try { - $app = new Application( - ['docbook-cs', '--config=' . self::VALID_CONFIG, "--diff={$diffFile}"], - $this->stdout, - $this->stderr, - ); - - $exitCode = $app->run(); - - // No matching files → no violations → exit 0. - self::assertSame(0, $exitCode); - self::assertSame('', $this->readStream($this->stderr)); - } finally { - unlink($diffFile); - } - } - - #[Test] - public function itSupportsDiffFromStdin(): void - { - $stdin = fopen('php://memory', 'rb+'); - self::assertIsResource($stdin); - - fwrite($stdin, <<<'DIFF' -diff --git a/nonexistent.xml b/nonexistent.xml ---- a/nonexistent.xml -+++ b/nonexistent.xml -@@ -1,1 +1,2 @@ - line1 -+line2 -DIFF); - rewind($stdin); - - $app = new Application( - ['docbook-cs', '--config=' . self::VALID_CONFIG, '--diff'], - $this->stdout, - $this->stderr, - $stdin, - ); - - $exitCode = $app->run(); - - self::assertSame(0, $exitCode); - self::assertSame('', $this->readStream($this->stderr)); - } - - #[Test] - public function itSupportsDiffFromStdinWithExplicitDash(): void - { - $stdin = fopen('php://memory', 'rb+'); - self::assertIsResource($stdin); - - fwrite($stdin, ''); - rewind($stdin); - - $app = new Application( - ['docbook-cs', '--config=' . self::VALID_CONFIG, '--diff=-'], - $this->stdout, - $this->stderr, - $stdin, - ); - - $exitCode = $app->run(); - - self::assertSame(0, $exitCode); - } - - #[Test] - public function itReturnsErrorWhenDiffFileCannotBeRead(): void - { - $app = new Application( - ['docbook-cs', '--config=' . self::VALID_CONFIG, '--diff=/nonexistent/path.patch'], - $this->stdout, - $this->stderr, - ); - - $exitCode = $app->run(); - - self::assertSame(2, $exitCode); - self::assertStringContainsString('Error reading diff', $this->readStream($this->stderr)); - } - - #[Test] - public function itIncludesDiffOptionInHelp(): void - { - $app = new Application(['docbook-cs', '--help'], $this->stdout, $this->stderr); - - $app->run(); - - self::assertStringContainsString('--diff', $this->readStream($this->stdout)); - } - #[Test] public function itSuppressesProgressWhenQuietFlagIsSet(): void { diff --git a/tests/Unit/Diff/DiffParserTest.php b/tests/Unit/Diff/DiffParserTest.php index ad3e5d2..1e49b92 100644 --- a/tests/Unit/Diff/DiffParserTest.php +++ b/tests/Unit/Diff/DiffParserTest.php @@ -176,6 +176,26 @@ public function itIgnoresRemovedLines(): void self::assertSame([], $result['file.xml']); } + #[Test] + public function itAnchorsRemovedLinesInTheResultingFile(): void + { + $diff = <<<'DIFF' +diff --git a/file.xml b/file.xml +--- a/file.xml ++++ b/file.xml +@@ -1,4 +1,3 @@ + line1 +-removed line + line2 + line3 +DIFF; + + $change = $this->parser->parse($diff)->changeFor('file.xml'); + self::assertNotNull($change); + + self::assertSame([2], $change->deletionAnchors); + } + #[Test] public function itIgnoresTheMissingFinalNewlineMarker(): void { @@ -195,6 +215,26 @@ public function itIgnoresTheMissingFinalNewlineMarker(): void self::assertSame([1, 2], $result['file.xml']); } + #[Test] + public function itAnchorsReplacedLinesWhenTheMissingFinalNewlineMarkerIsPresent(): void + { + $diff = <<<'DIFF' +diff --git a/file.xml b/file.xml +--- a/file.xml ++++ b/file.xml +@@ -1 +1,2 @@ +-old +\ No newline at end of file ++new ++second +DIFF; + + $change = $this->parser->parse($diff)->changeFor('file.xml'); + self::assertNotNull($change); + + self::assertSame([1], $change->deletionAnchors); + } + #[Test] public function itTracksLineNumbersAcrossMultipleHunks(): void { diff --git a/tests/Unit/Path/EntityResolverPathsTest.php b/tests/Unit/Path/EntityResolverPathsTest.php new file mode 100644 index 0000000..d09d1ff --- /dev/null +++ b/tests/Unit/Path/EntityResolverPathsTest.php @@ -0,0 +1,38 @@ +paths(); + $entities = $resolver->resolve(); + + self::assertSame($fixtureRoot . '/included.ent', $paths['inc']); + self::assertSame('child-value', $entities['child']); + } + + #[Test] + public function itDoesNotExposeUnreadableEntityTargets(): void + { + $fixture = __DIR__ . '/../../fixtures/entity_tree/system/missing_target.ent'; + $resolver = new EntityResolver([], [$fixture]); + + self::assertArrayNotHasKey('missing', $resolver->paths()); + } +} diff --git a/tests/Unit/Runner/RunPlannerTest.php b/tests/Unit/Runner/RunPlannerTest.php new file mode 100644 index 0000000..fa0a912 --- /dev/null +++ b/tests/Unit/Runner/RunPlannerTest.php @@ -0,0 +1,67 @@ +createMock(DiffProviderInterface::class); + $diffProvider + ->expects(self::once()) + ->method('for') + ->willReturn(<<<'DIFF' +diff --git a/nonexistent.xml b/nonexistent.xml +--- a/nonexistent.xml ++++ b/nonexistent.xml +@@ -1 +1 @@ +-old ++new +DIFF); + + $planner = new RunPlanner($config, diffProvider: $diffProvider); + + self::assertSame([], $planner->plan([], null)->targets); + } +} diff --git a/tests/Unit/Runner/SniffRunnerTest.php b/tests/Unit/Runner/SniffRunnerTest.php index 47b6d25..04fa634 100644 --- a/tests/Unit/Runner/SniffRunnerTest.php +++ b/tests/Unit/Runner/SniffRunnerTest.php @@ -8,6 +8,8 @@ use DocbookCS\Config\SniffEntry; use DocbookCS\Diff\Diff; use DocbookCS\Diff\FileChange; +use DocbookCS\Diff\GitDiffProvider; +use DocbookCS\Path\DiffPathLoader; use DocbookCS\Path\EntityResolver; use DocbookCS\Path\PathLoader; use DocbookCS\Path\PathMatcher; @@ -18,6 +20,9 @@ use DocbookCS\Report\Severity; use DocbookCS\Report\Violation; use DocbookCS\Runner\EntityPreprocessor; +use DocbookCS\Runner\RunPlan; +use DocbookCS\Runner\RunPlanner; +use DocbookCS\Runner\RunScopeResolver; use DocbookCS\Runner\SniffRunner; use DocbookCS\Runner\XmlFileProcessor; use DocbookCS\Sniff\SniffInterface; @@ -35,12 +40,17 @@ CoversClass(PathLoader::class), CoversClass(PathMatcher::class), CoversClass(Report::class), + CoversClass(RunPlan::class), + CoversClass(RunPlanner::class), CoversClass(SniffEntry::class), CoversClass(SniffRunner::class), CoversClass(Violation::class), CoversClass(XmlFileProcessor::class), UsesClass(Diff::class), + UsesClass(DiffPathLoader::class), UsesClass(FileChange::class), + UsesClass(GitDiffProvider::class), + UsesClass(RunScopeResolver::class), ] final class SniffRunnerTest extends TestCase { @@ -65,7 +75,7 @@ public function itProcessesFilesWithoutViolations(): void $config = $this->createConfig(); $runner = new SniffRunner(); - $report = $runner->run($config); + $report = $runner->run($this->planPaths($config)); self::assertSame(2, $report->getFilesScanned()); self::assertFalse($report->hasViolations()); @@ -78,7 +88,7 @@ public function itUsesOverridePathsWhenProvided(): void $config = $this->createConfig(); $runner = new SniffRunner(); - $report = $runner->run($config, [self::FIXTURE_DIR . '/../override']); + $report = $runner->run($this->planPaths($config, [self::FIXTURE_DIR . '/../override'])); self::assertSame(1, $report->getFilesScanned()); } @@ -101,7 +111,7 @@ public function itCallsProgressMethods(): void $config = $this->createConfig(); $runner = new SniffRunner($progress); - $runner->run($config); + $runner->run($this->planPaths($config)); } #[Test] @@ -134,7 +144,7 @@ public function setProperty(string $name, string $value): void $config = $this->createConfig(sniffs: [new SniffEntry($sniff::class)]); $runner = new SniffRunner(); - $report = $runner->run($config); + $report = $runner->run($this->planPaths($config)); self::assertSame(2, $report->getFilesScanned()); self::assertCount(2, $report->getFileReports()); @@ -171,7 +181,7 @@ public function setProperty(string $name, string $value): void $config = $this->createConfig(sniffs: [new SniffEntry($sniff::class)]); $runner = new SniffRunner(); - $report = $runner->run($config); + $report = $runner->run($this->planPaths($config)); foreach ($report->getFileReports() as $fileReport) { self::assertFalse( @@ -206,7 +216,7 @@ public function process(\DOMDocument $document, string $content, string $filePat $config = $this->createConfig(sniffs: [new SniffEntry($sniffClass::class, ['someProp' => 'someValue'])]); $runner = new SniffRunner(); - $runner->run($config); + $runner->run($this->planPaths($config)); self::assertSame('someValue', $sniffClass::$captured); } @@ -221,7 +231,7 @@ public function itThrowsWhenSniffClassDoesNotExist(): void $this->expectException(\RuntimeException::class); $this->expectExceptionMessage('does not exist'); - $runner->run($config); + $runner->run($this->planPaths($config)); } #[Test] @@ -234,7 +244,7 @@ public function itThrowsWhenClassDoesNotImplementSniffInterface(): void $this->expectException(\RuntimeException::class); $this->expectExceptionMessage('does not implement'); - $runner->run($config); + $runner->run($this->planPaths($config)); } #[Test] @@ -243,8 +253,8 @@ public function itFiltersFilesToOnlyThoseInTheDiff(): void $config = $this->createConfig(); $runner = new SniffRunner(); - $diff = new Diff([new FileChange('sniff_runner/default/file_a.xml', [1])]); - $report = $runner->run($config, null, $diff); + $diff = new Diff([new FileChange(self::FIXTURE_DIR . '/file_a.xml', [1])]); + $report = $runner->run($this->planDiff($config, $diff)); self::assertSame(1, $report->getFilesScanned()); } @@ -256,7 +266,7 @@ public function itScansNoFilesWhenDiffContainsNoMatchingPaths(): void $runner = new SniffRunner(); $diff = new Diff([new FileChange('completely/different/file.xml', [1, 2, 3])]); - $report = $runner->run($config, null, $diff); + $report = $runner->run($this->planDiff($config, $diff)); self::assertSame(0, $report->getFilesScanned()); } @@ -270,7 +280,7 @@ public function itMatchesWhenDiffPathEqualsDiscoveredPath(): void $discoveredPath = self::FIXTURE_DIR . '/file_a.xml'; $diff = new Diff([new FileChange($discoveredPath, [1])]); - $report = $runner->run($config, null, $diff); + $report = $runner->run($this->planDiff($config, $diff)); self::assertSame(1, $report->getFilesScanned()); } @@ -281,7 +291,7 @@ public function itScansAllFilesWhenNoDiffIsGiven(): void $config = $this->createConfig(); $runner = new SniffRunner(); - $report = $runner->run($config); + $report = $runner->run($this->planPaths($config)); self::assertSame(2, $report->getFilesScanned()); } @@ -316,10 +326,21 @@ public function setProperty(string $name, string $value): void $config = $this->createConfig(sniffs: [new SniffEntry($sniff::class)]); $runner = new SniffRunner(); - $diff = new Diff([new FileChange('sniff_runner/default/file_a.xml', [])]); - $report = $runner->run($config, null, $diff); + $diff = new Diff([new FileChange(self::FIXTURE_DIR . '/file_a.xml', [])]); + $report = $runner->run($this->planDiff($config, $diff)); self::assertSame(1, $report->getFilesScanned()); self::assertFalse($report->hasViolations()); } + + /** @param list|null $paths */ + private function planPaths(ConfigData $config, ?array $paths = null): RunPlan + { + return new RunPlanner($config)->planPaths($paths ?? $config->getIncludePaths()); + } + + private function planDiff(ConfigData $config, Diff $diff): RunPlan + { + return new RunPlanner($config)->planDiff($diff); + } } From 78d850b582b9b2d5f349fccb8fb6e204571e2739 Mon Sep 17 00:00:00 2001 From: NickSdot Date: Tue, 21 Jul 2026 15:30:59 +0700 Subject: [PATCH 02/15] fix: prevented duplicate scans for equivalent paths --- src/Runner/RunScopeResolver.php | 38 ++++++++++++--- .../Runner/RunScopeResolverTest.php | 21 +++++++++ tests/Unit/Runner/SniffRunnerTest.php | 46 +++++++++++++++++++ 3 files changed, 99 insertions(+), 6 deletions(-) diff --git a/src/Runner/RunScopeResolver.php b/src/Runner/RunScopeResolver.php index cd47f5d..931da12 100644 --- a/src/Runner/RunScopeResolver.php +++ b/src/Runner/RunScopeResolver.php @@ -91,12 +91,11 @@ private function absolutePaths(array $paths): array $absolutePaths = []; foreach ($paths as $path) { - if (str_starts_with($path, '/') || preg_match('#^[a-zA-Z]:[/\\\\]#', $path)) { - $absolutePaths[] = $path; - continue; - } + $absolutePath = str_starts_with($path, '/') || preg_match('#^[a-zA-Z]:[/\\\\]#', $path) + ? $path + : $workingDirectory . '/' . $path; - $absolutePaths[] = $workingDirectory . '/' . $path; + $absolutePaths[] = $this->normalizePath($absolutePath); } return $absolutePaths; @@ -165,7 +164,7 @@ private function targetFilesFromContent(string $content, array &$visitedEntityPa */ private function expandEntityPath(string $path, array &$visited): array { - $path = str_replace('\\', '/', $path); + $path = $this->normalizePath($path); if (isset($visited[$path]) || !is_file($path)) { return []; @@ -183,4 +182,31 @@ private function expandEntityPath(string $path, array &$visited): array ? $this->targetFilesFromContent($content, $visited) : []; } + + private function normalizePath(string $path): string + { + $path = str_replace('\\', '/', $path); + $prefix = ''; + + if (preg_match('#^([a-zA-Z]:/|/)#', $path, $matches)) { + $prefix = $matches[1]; + $path = substr($path, strlen($prefix)); + } + + $segments = []; + foreach (explode('/', $path) as $segment) { + if ($segment === '' || $segment === '.') { + continue; + } + + if ($segment === '..' && $segments !== [] && end($segments) !== '..') { + array_pop($segments); + continue; + } + + $segments[] = $segment; + } + + return $prefix . implode('/', $segments); + } } diff --git a/tests/Integration/Runner/RunScopeResolverTest.php b/tests/Integration/Runner/RunScopeResolverTest.php index 9bde123..f4368bf 100644 --- a/tests/Integration/Runner/RunScopeResolverTest.php +++ b/tests/Integration/Runner/RunScopeResolverTest.php @@ -98,6 +98,27 @@ public function pathScopeResolvesRelativePathsAgainstTheWorkingDirectory(): void self::assertSame([$absolutePath => null], $this->resolver()->resolvePaths([$path])); } + #[Test] + public function widePathScopeDoesNotDuplicateLexicallyEquivalentTargets(): void + { + $resolver = new RunScopeResolver( + $this->config(), + [ + 'bridge' => $this->entityFile, + 'target' => $this->directory . '/./target.xml', + ], + wide: true, + ); + + self::assertSame( + [ + $this->sourceFile => null, + $this->targetFile => null, + ], + $resolver->resolvePaths([$this->directory . '/.']), + ); + } + private function resolver(bool $wide = false): RunScopeResolver { return new RunScopeResolver( diff --git a/tests/Unit/Runner/SniffRunnerTest.php b/tests/Unit/Runner/SniffRunnerTest.php index 04fa634..c98c4be 100644 --- a/tests/Unit/Runner/SniffRunnerTest.php +++ b/tests/Unit/Runner/SniffRunnerTest.php @@ -19,6 +19,7 @@ use DocbookCS\Report\Report; use DocbookCS\Report\Severity; use DocbookCS\Report\Violation; +use DocbookCS\Runner\EntityExpansionMarker; use DocbookCS\Runner\EntityPreprocessor; use DocbookCS\Runner\RunPlan; use DocbookCS\Runner\RunPlanner; @@ -48,6 +49,7 @@ CoversClass(XmlFileProcessor::class), UsesClass(Diff::class), UsesClass(DiffPathLoader::class), + UsesClass(EntityExpansionMarker::class), UsesClass(FileChange::class), UsesClass(GitDiffProvider::class), UsesClass(RunScopeResolver::class), @@ -296,6 +298,50 @@ public function itScansAllFilesWhenNoDiffIsGiven(): void self::assertSame(2, $report->getFilesScanned()); } + #[Test] + public function itScansLexicallyEquivalentWideTargetsOnlyOnce(): void + { + $directory = sys_get_temp_dir() . '/docbook-cs-scan-' . bin2hex(random_bytes(6)); + mkdir($directory); + + $sourceFile = $directory . '/source.xml'; + $targetFile = $directory . '/target.xml'; + $entityFile = $directory . '/bridge.ent'; + + file_put_contents($sourceFile, '&bridge;'); + file_put_contents($targetFile, ''); + file_put_contents($entityFile, '⌖'); + + try { + $config = new ConfigData([], [], [], [], [], $directory); + $resolver = new RunScopeResolver( + $config, + [ + 'bridge' => $entityFile, + 'target' => $directory . '/./target.xml', + ], + wide: true, + ); + $plan = new RunPlan( + sniffs: [], + targets: $resolver->resolvePaths([$directory . '/.']), + entities: [ + 'bridge' => '⌖', + 'target' => '', + ], + ); + + $report = new SniffRunner()->run($plan); + + self::assertSame(2, $report->getFilesScanned()); + } finally { + @unlink($sourceFile); + @unlink($targetFile); + @unlink($entityFile); + @rmdir($directory); + } + } + #[Test] public function itReportsNoViolationsForFilesInDiffWithoutAddedLines(): void { From 79b4c8cfa26d8fefc6d72161ecd2313529f0c6de Mon Sep 17 00:00:00 2001 From: NickSdot Date: Fri, 24 Jul 2026 18:05:28 +0700 Subject: [PATCH 03/15] review: renamed fromGlobals method --- bin/docbook-cs | 2 +- src/Application.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/bin/docbook-cs b/bin/docbook-cs index 4b72e67..68d109a 100644 --- a/bin/docbook-cs +++ b/bin/docbook-cs @@ -25,4 +25,4 @@ use DocbookCS\Application; exit(2); })(); -exit(Application::fromGlobals($argv ?? [])->run()); +exit(Application::withArguments($argv ?? [])->run()); diff --git a/src/Application.php b/src/Application.php index bb36f76..4ce2594 100644 --- a/src/Application.php +++ b/src/Application.php @@ -39,7 +39,7 @@ final class Application * @throws \RuntimeException if redirected stdin cannot be read. * @api */ - public static function fromGlobals(array $argv): self + public static function withArguments(array $argv): self { $stdin = null; $stat = fstat(STDIN); From e4a7f4389c9a953b15477946eb032a5b5eeaacf7 Mon Sep 17 00:00:00 2001 From: NickSdot Date: Fri, 24 Jul 2026 18:14:03 +0700 Subject: [PATCH 04/15] review: renamed stdin constructor argument --- src/Application.php | 8 ++++---- tests/Feature/ApplicationInputTest.php | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Application.php b/src/Application.php index 4ce2594..c0781ec 100644 --- a/src/Application.php +++ b/src/Application.php @@ -45,7 +45,7 @@ public static function withArguments(array $argv): self $stat = fstat(STDIN); if ($stat === false) { - return new self($argv, stdin: $stdin); + return new self($argv, unifiedDiff: $stdin); } $type = $stat['mode'] & 0170000; @@ -58,7 +58,7 @@ public static function withArguments(array $argv): self } } - return new self($argv, stdin: $stdin); + return new self($argv, unifiedDiff: $stdin); } /** @@ -70,12 +70,12 @@ public function __construct( array $argv, mixed $stdout = null, mixed $stderr = null, - ?string $stdin = null, + ?string $unifiedDiff = null, ) { $this->argv = $argv; $this->stdout = $stdout ?? STDOUT; $this->stderr = $stderr ?? STDERR; - $this->stdin = $stdin; + $this->stdin = $unifiedDiff; } /** diff --git a/tests/Feature/ApplicationInputTest.php b/tests/Feature/ApplicationInputTest.php index a68ac9a..e38b3f0 100644 --- a/tests/Feature/ApplicationInputTest.php +++ b/tests/Feature/ApplicationInputTest.php @@ -55,7 +55,7 @@ public function itDetectsAPipedDiffWithoutAFlag(): void ['docbook-cs', '--config=' . self::VALID_CONFIG], $this->stdout, $this->stderr, - stdin: '', + unifiedDiff: '', ); self::assertSame(0, $app->run()); @@ -69,7 +69,7 @@ public function itRejectsPathsCombinedWithAPipedDiff(): void ['docbook-cs', '--config=' . self::VALID_CONFIG, self::SCAN_FILE], $this->stdout, $this->stderr, - stdin: '', + unifiedDiff: '', ); self::assertSame(2, $app->run()); From 8caa7876d9517912f0d4263586dbb292af60331e Mon Sep 17 00:00:00 2001 From: NickSdot Date: Fri, 24 Jul 2026 18:32:35 +0700 Subject: [PATCH 05/15] review: moved all tests back to Unit --- phpunit.xml.dist | 6 ------ tests/{Feature => Unit}/ApplicationInputTest.php | 2 +- tests/{Integration => Unit}/Diff/GitDiffProviderTest.php | 2 +- tests/{Integration => Unit}/Path/DiffPathLoaderTest.php | 2 +- .../Process/NativeProcessRunnerTest.php | 2 +- .../Runner/EntityExpansionMarkerTest.php | 2 +- tests/{Integration => Unit}/Runner/RunScopeResolverTest.php | 2 +- .../{Integration => Unit}/Sniff/EntityExpandedSniffTest.php | 2 +- 8 files changed, 7 insertions(+), 13 deletions(-) rename tests/{Feature => Unit}/ApplicationInputTest.php (98%) rename tests/{Integration => Unit}/Diff/GitDiffProviderTest.php (98%) rename tests/{Integration => Unit}/Path/DiffPathLoaderTest.php (98%) rename tests/{Integration => Unit}/Process/NativeProcessRunnerTest.php (94%) rename tests/{Integration => Unit}/Runner/EntityExpansionMarkerTest.php (98%) rename tests/{Integration => Unit}/Runner/RunScopeResolverTest.php (98%) rename tests/{Integration => Unit}/Sniff/EntityExpandedSniffTest.php (97%) diff --git a/phpunit.xml.dist b/phpunit.xml.dist index 540a1a7..f173b58 100644 --- a/phpunit.xml.dist +++ b/phpunit.xml.dist @@ -18,12 +18,6 @@ tests/Unit - - tests/Integration - - - tests/Feature - diff --git a/tests/Feature/ApplicationInputTest.php b/tests/Unit/ApplicationInputTest.php similarity index 98% rename from tests/Feature/ApplicationInputTest.php rename to tests/Unit/ApplicationInputTest.php index e38b3f0..e92ea41 100644 --- a/tests/Feature/ApplicationInputTest.php +++ b/tests/Unit/ApplicationInputTest.php @@ -2,7 +2,7 @@ declare(strict_types=1); -namespace DocbookCS\Tests\Feature; +namespace DocbookCS\Tests\Unit; use DocbookCS\Application; use PHPUnit\Framework\Attributes\CoversNothing; diff --git a/tests/Integration/Diff/GitDiffProviderTest.php b/tests/Unit/Diff/GitDiffProviderTest.php similarity index 98% rename from tests/Integration/Diff/GitDiffProviderTest.php rename to tests/Unit/Diff/GitDiffProviderTest.php index 3cf7dc9..7ee6be7 100644 --- a/tests/Integration/Diff/GitDiffProviderTest.php +++ b/tests/Unit/Diff/GitDiffProviderTest.php @@ -2,7 +2,7 @@ declare(strict_types=1); -namespace DocbookCS\Tests\Integration\Diff; +namespace DocbookCS\Tests\Unit\Diff; use DocbookCS\Diff\GitDiffProvider; use DocbookCS\Process\NativeProcessRunner; diff --git a/tests/Integration/Path/DiffPathLoaderTest.php b/tests/Unit/Path/DiffPathLoaderTest.php similarity index 98% rename from tests/Integration/Path/DiffPathLoaderTest.php rename to tests/Unit/Path/DiffPathLoaderTest.php index 47ba2e4..620b711 100644 --- a/tests/Integration/Path/DiffPathLoaderTest.php +++ b/tests/Unit/Path/DiffPathLoaderTest.php @@ -2,7 +2,7 @@ declare(strict_types=1); -namespace DocbookCS\Tests\Integration\Path; +namespace DocbookCS\Tests\Unit\Path; use DocbookCS\Diff\Diff; use DocbookCS\Diff\FileChange; diff --git a/tests/Integration/Process/NativeProcessRunnerTest.php b/tests/Unit/Process/NativeProcessRunnerTest.php similarity index 94% rename from tests/Integration/Process/NativeProcessRunnerTest.php rename to tests/Unit/Process/NativeProcessRunnerTest.php index 9b930fb..b985080 100644 --- a/tests/Integration/Process/NativeProcessRunnerTest.php +++ b/tests/Unit/Process/NativeProcessRunnerTest.php @@ -2,7 +2,7 @@ declare(strict_types=1); -namespace DocbookCS\Tests\Integration\Process; +namespace DocbookCS\Tests\Unit\Process; use DocbookCS\Process\NativeProcessRunner; use DocbookCS\Process\ProcessResult; diff --git a/tests/Integration/Runner/EntityExpansionMarkerTest.php b/tests/Unit/Runner/EntityExpansionMarkerTest.php similarity index 98% rename from tests/Integration/Runner/EntityExpansionMarkerTest.php rename to tests/Unit/Runner/EntityExpansionMarkerTest.php index e2fa549..427ddab 100644 --- a/tests/Integration/Runner/EntityExpansionMarkerTest.php +++ b/tests/Unit/Runner/EntityExpansionMarkerTest.php @@ -2,7 +2,7 @@ declare(strict_types=1); -namespace DocbookCS\Tests\Integration\Runner; +namespace DocbookCS\Tests\Unit\Runner; use DocbookCS\Runner\EntityExpansionMarker; use DocbookCS\Runner\EntityPreprocessor; diff --git a/tests/Integration/Runner/RunScopeResolverTest.php b/tests/Unit/Runner/RunScopeResolverTest.php similarity index 98% rename from tests/Integration/Runner/RunScopeResolverTest.php rename to tests/Unit/Runner/RunScopeResolverTest.php index f4368bf..48ecb83 100644 --- a/tests/Integration/Runner/RunScopeResolverTest.php +++ b/tests/Unit/Runner/RunScopeResolverTest.php @@ -2,7 +2,7 @@ declare(strict_types=1); -namespace DocbookCS\Tests\Integration\Runner; +namespace DocbookCS\Tests\Unit\Runner; use DocbookCS\Config\ConfigData; use DocbookCS\Diff\Diff; diff --git a/tests/Integration/Sniff/EntityExpandedSniffTest.php b/tests/Unit/Sniff/EntityExpandedSniffTest.php similarity index 97% rename from tests/Integration/Sniff/EntityExpandedSniffTest.php rename to tests/Unit/Sniff/EntityExpandedSniffTest.php index a6b1925..a56c40c 100644 --- a/tests/Integration/Sniff/EntityExpandedSniffTest.php +++ b/tests/Unit/Sniff/EntityExpandedSniffTest.php @@ -2,7 +2,7 @@ declare(strict_types=1); -namespace DocbookCS\Tests\Integration\Sniff; +namespace DocbookCS\Tests\Unit\Sniff; use DocbookCS\Report\Violation; use DocbookCS\Runner\EntityExpansionMarker; From 21b694cef6a0f5bdfe5fb8bb7aff58a1616090d4 Mon Sep 17 00:00:00 2001 From: NickSdot Date: Fri, 24 Jul 2026 18:59:17 +0700 Subject: [PATCH 06/15] test: increased test coverage --- tests/Unit/ApplicationInputTest.php | 58 ++++++++++++++++++- tests/Unit/Diff/DiffParserTest.php | 3 +- tests/Unit/Diff/GitDiffProviderTest.php | 9 +++ tests/Unit/Path/DiffPathLoaderTest.php | 34 +++++++++++ .../Unit/Process/NativeProcessRunnerTest.php | 4 +- tests/Unit/Runner/RunPlannerTest.php | 20 +++++++ tests/Unit/Runner/RunScopeResolverTest.php | 51 ++++++++++++++++ 7 files changed, 173 insertions(+), 6 deletions(-) diff --git a/tests/Unit/ApplicationInputTest.php b/tests/Unit/ApplicationInputTest.php index e92ea41..1fbf8f9 100644 --- a/tests/Unit/ApplicationInputTest.php +++ b/tests/Unit/ApplicationInputTest.php @@ -5,11 +5,51 @@ namespace DocbookCS\Tests\Unit; use DocbookCS\Application; -use PHPUnit\Framework\Attributes\CoversNothing; +use DocbookCS\Config\ConfigData; +use DocbookCS\Config\ConfigParser; +use DocbookCS\Config\SniffEntry; +use DocbookCS\Diff\Diff; +use DocbookCS\Diff\DiffParser; +use DocbookCS\Diff\GitDiffProvider; +use DocbookCS\Path\DiffPathLoader; +use DocbookCS\Path\EntityResolver; +use DocbookCS\Path\PathMatcher; +use DocbookCS\Progress\NullProgress; +use DocbookCS\Report\Report; +use DocbookCS\Report\Reporter\ConsoleReporter; +use DocbookCS\Runner\EntityPreprocessor; +use DocbookCS\Runner\RunPlan; +use DocbookCS\Runner\RunPlanner; +use DocbookCS\Runner\RunScopeResolver; +use DocbookCS\Runner\SniffRunner; +use DocbookCS\Runner\XmlFileProcessor; +use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\Attributes\Test; +use PHPUnit\Framework\Attributes\UsesClass; use PHPUnit\Framework\TestCase; -#[CoversNothing] +#[ + CoversClass(Application::class), + // + UsesClass(ConfigData::class), + UsesClass(ConfigParser::class), + UsesClass(ConsoleReporter::class), + UsesClass(Diff::class), + UsesClass(DiffParser::class), + UsesClass(DiffPathLoader::class), + UsesClass(EntityPreprocessor::class), + UsesClass(EntityResolver::class), + UsesClass(GitDiffProvider::class), + UsesClass(NullProgress::class), + UsesClass(PathMatcher::class), + UsesClass(Report::class), + UsesClass(RunPlan::class), + UsesClass(RunPlanner::class), + UsesClass(RunScopeResolver::class), + UsesClass(SniffEntry::class), + UsesClass(SniffRunner::class), + UsesClass(XmlFileProcessor::class), +] final class ApplicationInputTest extends TestCase { private const string FIXTURE_DIR = __DIR__ . '/../fixtures/application'; @@ -89,6 +129,20 @@ public function itIncludesTheWideOptionInHelp(): void self::assertStringNotContainsString('--diff', $output); } + #[Test] + public function itAcceptsTheWideOption(): void + { + $app = new Application( + ['docbook-cs', '--config=' . self::VALID_CONFIG, '--wide'], + $this->stdout, + $this->stderr, + unifiedDiff: '', + ); + + self::assertSame(0, $app->run()); + self::assertSame('', $this->readStream($this->stderr)); + } + /** @param resource $stream */ private function readStream(mixed $stream): string { diff --git a/tests/Unit/Diff/DiffParserTest.php b/tests/Unit/Diff/DiffParserTest.php index 1e49b92..08dcdbf 100644 --- a/tests/Unit/Diff/DiffParserTest.php +++ b/tests/Unit/Diff/DiffParserTest.php @@ -14,8 +14,9 @@ #[ CoversClass(DiffParser::class), + CoversClass(FileChange::class), + // UsesClass(Diff::class), - UsesClass(FileChange::class), ] final class DiffParserTest extends TestCase { diff --git a/tests/Unit/Diff/GitDiffProviderTest.php b/tests/Unit/Diff/GitDiffProviderTest.php index 7ee6be7..db7e632 100644 --- a/tests/Unit/Diff/GitDiffProviderTest.php +++ b/tests/Unit/Diff/GitDiffProviderTest.php @@ -95,6 +95,15 @@ public function itFailsClearlyWhenNoUpstreamDefaultBranchCanBeFound(): void new GitDiffProvider($this->processRunner)->for($this->repository); } + #[Test] + public function itIncludesGitErrorsWhenACommandFails(): void + { + $this->expectException(\RuntimeException::class); + $this->expectExceptionMessageIsOrContains('Could not find Git repository. fatal: not a git repository'); + + new GitDiffProvider($this->processRunner)->for($this->repository); + } + private function configureAuthor(): void { $this->git('config', 'user.name', 'DocbookCS Tests'); diff --git a/tests/Unit/Path/DiffPathLoaderTest.php b/tests/Unit/Path/DiffPathLoaderTest.php index 620b711..7606e72 100644 --- a/tests/Unit/Path/DiffPathLoaderTest.php +++ b/tests/Unit/Path/DiffPathLoaderTest.php @@ -77,4 +77,38 @@ public function itIgnoresMissingNonXmlAndExcludedFiles(): void self::assertSame([], $loader->load()->fileChanges); } + + #[Test] + public function itLoadsAbsolutePaths(): void + { + $file = $this->directory . '/chapter.xml'; + file_put_contents($file, ''); + + $loader = new DiffPathLoader( + new Diff([new FileChange($file, [1])]), + workingDirectory: $this->directory, + basePath: $this->directory, + projectRoots: [], + matcher: new PathMatcher($this->directory, []), + ); + + self::assertNotNull($loader->load()->changeFor($file)); + } + + #[Test] + public function itNormalisesParentDirectorySegments(): void + { + $file = $this->directory . '/chapter.xml'; + file_put_contents($file, ''); + + $loader = new DiffPathLoader( + new Diff([new FileChange('nested/../chapter.xml', [1])]), + workingDirectory: $this->directory, + basePath: $this->directory, + projectRoots: [], + matcher: new PathMatcher($this->directory, []), + ); + + self::assertNotNull($loader->load()->changeFor($file)); + } } diff --git a/tests/Unit/Process/NativeProcessRunnerTest.php b/tests/Unit/Process/NativeProcessRunnerTest.php index b985080..9970358 100644 --- a/tests/Unit/Process/NativeProcessRunnerTest.php +++ b/tests/Unit/Process/NativeProcessRunnerTest.php @@ -8,13 +8,11 @@ use DocbookCS\Process\ProcessResult; use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\Attributes\Test; -use PHPUnit\Framework\Attributes\UsesClass; use PHPUnit\Framework\TestCase; #[ CoversClass(NativeProcessRunner::class), - // - UsesClass(ProcessResult::class), + CoversClass(ProcessResult::class), ] final class NativeProcessRunnerTest extends TestCase { diff --git a/tests/Unit/Runner/RunPlannerTest.php b/tests/Unit/Runner/RunPlannerTest.php index fa0a912..3de1821 100644 --- a/tests/Unit/Runner/RunPlannerTest.php +++ b/tests/Unit/Runner/RunPlannerTest.php @@ -64,4 +64,24 @@ public function itUsesTheContributionDiffWhenNoInputIsProvided(): void self::assertSame([], $planner->plan([], null)->targets); } + + #[Test] + public function itRejectsPathsCombinedWithAPipedDiff(): void + { + $config = new ConfigData( + projectRoots: [], + sniffs: [], + includePaths: [], + excludePatterns: [], + entityPaths: [], + basePath: getcwd() ?: '.', + ); + + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessageIs('Paths cannot be combined with diff input.'); + + $planner = new RunPlanner($config, diffProvider: $this->createStub(DiffProviderInterface::class)); + + $planner->plan(['file.xml'], ''); + } } diff --git a/tests/Unit/Runner/RunScopeResolverTest.php b/tests/Unit/Runner/RunScopeResolverTest.php index 48ecb83..cc36cbc 100644 --- a/tests/Unit/Runner/RunScopeResolverTest.php +++ b/tests/Unit/Runner/RunScopeResolverTest.php @@ -119,6 +119,57 @@ public function widePathScopeDoesNotDuplicateLexicallyEquivalentTargets(): void ); } + #[Test] + public function wideScopeIgnoresUnknownEntityReferences(): void + { + file_put_contents($this->sourceFile, '&unknown;&bridge;'); + + $targets = $this->resolver(wide: true)->resolvePaths([$this->sourceFile]); + + self::assertSame( + [ + $this->sourceFile => null, + $this->targetFile => null, + ], + $targets, + ); + } + + #[Test] + public function wideScopeHandlesCyclicEntityReferences(): void + { + file_put_contents($this->entityFile, '&bridge;⌖'); + + self::assertSame( + [ + $this->sourceFile => null, + $this->targetFile => null, + ], + $this->resolver(wide: true)->resolvePaths([$this->sourceFile]), + ); + } + + #[Test] + public function wideScopeNormalisesParentDirectorySegmentsInEntityPaths(): void + { + $resolver = new RunScopeResolver( + $this->config(), + [ + 'bridge' => $this->directory . '/nested/../bridge.ent', + 'target' => $this->targetFile, + ], + wide: true, + ); + + self::assertSame( + [ + $this->sourceFile => null, + $this->targetFile => null, + ], + $resolver->resolvePaths([$this->sourceFile]), + ); + } + private function resolver(bool $wide = false): RunScopeResolver { return new RunScopeResolver( From 626222d47713454416168a2fc971ab867539fec2 Mon Sep 17 00:00:00 2001 From: NickSdot Date: Fri, 24 Jul 2026 20:49:54 +0700 Subject: [PATCH 07/15] review: revisited git diff handling --- src/Diff/DiffBaseResolver.php | 95 ++++++ src/Diff/GitDiffProvider.php | 89 ++---- src/Diff/UpstreamResolver.php | 131 ++++++++ src/Git/GitClient.php | 227 ++++++++++++++ src/Git/GitException.php | 30 ++ src/Process/NativeProcessRunner.php | 22 +- src/Process/ProcessException.php | 13 + src/Process/ProcessRunnerInterface.php | 5 +- tests/Unit/ApplicationInputTest.php | 8 + tests/Unit/ApplicationTest.php | 8 + tests/Unit/Diff/GitDiffProviderTest.php | 283 ++++++++++++++++-- tests/Unit/Git/GitClientTest.php | 44 +++ .../Unit/Process/NativeProcessRunnerTest.php | 13 + tests/Unit/Runner/SniffRunnerTest.php | 6 + 14 files changed, 879 insertions(+), 95 deletions(-) create mode 100644 src/Diff/DiffBaseResolver.php create mode 100644 src/Diff/UpstreamResolver.php create mode 100644 src/Git/GitClient.php create mode 100644 src/Git/GitException.php create mode 100644 src/Process/ProcessException.php create mode 100644 tests/Unit/Git/GitClientTest.php diff --git a/src/Diff/DiffBaseResolver.php b/src/Diff/DiffBaseResolver.php new file mode 100644 index 0000000..4962398 --- /dev/null +++ b/src/Diff/DiffBaseResolver.php @@ -0,0 +1,95 @@ +repositoryName($repoRoot); + + if ($repoName === null) { + return $this->localMergeBase($repoRoot); + } + + return $this->officialUpstream->resolve($repoRoot, $repoName) + ?? $this->localMergeBase($repoRoot); + } + + /** @throws GitException */ + private function localMergeBase(string $repoRoot): string + { + $baseReference = $this->localBaseReference($repoRoot); + + if (null === $mergeBase = $this->git->findMergeBase($repoRoot, 'HEAD', $baseReference)) { + throw GitException::mergeBaseNotFound($baseReference); + } + + return $mergeBase; + } + + /** @throws GitException */ + private function localBaseReference(string $repoRoot): string + { + if ($this->git->currentBranchName($repoRoot) === self::DEFAULT_BRANCH) { + return $this->git->configuredUpstreamReference($repoRoot, self::DEFAULT_BRANCH) ?? 'HEAD'; + } + + if ($this->git->resolveCommitHash($repoRoot, self::DEFAULT_BRANCH_REFERENCE) !== null) { + return self::DEFAULT_BRANCH_REFERENCE; + } + + throw GitException::localMasterNotFound(); + } + + /** @throws GitException */ + private function repositoryName(string $repoRoot): ?string + { + $repoNames = []; + + foreach ($this->git->configuredRemoteUrls($repoRoot) as $url) { + $repoName = $this->repositoryNameFrom($url); + + if ($repoName !== null) { + $repoNames[$repoName] = true; + } + } + + if (count($repoNames) === 1) { + return array_key_first($repoNames); + } + + return $repoNames === [] + ? $this->repositoryNameFrom($repoRoot) + : null; + } + + private function repositoryNameFrom(string $path): ?string + { + $path = rtrim(str_replace(['\\', ':'], '/', $path), '/'); + $name = preg_replace('/\\.git$/i', '', basename($path)); + + $isPhpDocsRepo = is_string($name) + && preg_match(self::PHP_DOCS_REPO_PATTERN, $name) === 1; + + return $isPhpDocsRepo ? strtolower($name) : null; + } +} diff --git a/src/Diff/GitDiffProvider.php b/src/Diff/GitDiffProvider.php index 3e7c146..d827b00 100644 --- a/src/Diff/GitDiffProvider.php +++ b/src/Diff/GitDiffProvider.php @@ -4,89 +4,40 @@ namespace DocbookCS\Diff; +use DocbookCS\Git\GitClient; +use DocbookCS\Git\GitException; use DocbookCS\Process\NativeProcessRunner; use DocbookCS\Process\ProcessRunnerInterface; final readonly class GitDiffProvider implements DiffProviderInterface { + private GitClient $git; + + private DiffBaseResolver $baseResolver; + public function __construct( - private ProcessRunnerInterface $processRunner = new NativeProcessRunner(), + ProcessRunnerInterface $processRunner = new NativeProcessRunner(), + ?string $cacheDirectory = null, ) { - } - - /** @throws \RuntimeException if the repository, branch point, or diff cannot be determined. */ - public function for(string $workingDirectory): string - { - $repositoryRoot = trim($this->runOrThrow( - ['git', 'rev-parse', '--show-toplevel'], - $workingDirectory, - 'Could not find Git repository.', - )); + $gitClient = new GitClient($processRunner); - $baseReference = $this->resolveBaseReference($repositoryRoot); - $mergeBase = $this->runOrThrow( - ['git', 'merge-base', 'HEAD', $baseReference], - $repositoryRoot, - sprintf('Unclear where HEAD branched from %s.', $baseReference), - ); + $cacheDirectory ??= dirname(__DIR__, 2) . '/var/upstream'; - return $this->runOrThrow( - ['git', 'diff', '--no-ext-diff', '--no-color', trim($mergeBase), '--'], - $repositoryRoot, - 'Could not read diff.', + $this->baseResolver = new DiffBaseResolver( + $gitClient, + new UpstreamResolver($gitClient, $cacheDirectory), ); - } - - /** @throws \RuntimeException if no default branch reference exists. */ - private function resolveBaseReference(string $repositoryRoot): string - { - $candidates = []; - foreach (['upstream', 'origin'] as $remote) { - $result = $this->processRunner->run( - ['git', 'symbolic-ref', '--quiet', sprintf('refs/remotes/%s/HEAD', $remote)], - $repositoryRoot, - ); - - if ($result->exitCode === 0) { - $candidates[] = trim($result->stdout); - } - - $candidates[] = sprintf('refs/remotes/%s/main', $remote); - $candidates[] = sprintf('refs/remotes/%s/master', $remote); - } - - $candidates[] = 'refs/heads/main'; - $candidates[] = 'refs/heads/master'; - - foreach (array_unique($candidates) as $candidate) { - $result = $this->processRunner->run( - ['git', 'rev-parse', '--verify', '--quiet', $candidate . '^{commit}'], - $repositoryRoot, - ); - - if ($result->exitCode === 0) { - return $candidate; - } - } - - throw new \RuntimeException('Could not determine the upstream default branch for the contribution diff.'); + $this->git = $gitClient; } - /** - * @param list $command - * @throws \RuntimeException if the command fails. - */ - private function runOrThrow(array $command, string $workingDirectory, string $error): string + /** @throws GitException */ + public function for(string $workingDirectory): string { - $result = $this->processRunner->run($command, $workingDirectory); - - if ($result->exitCode === 0) { - return $result->stdout; - } - - $detail = trim($result->stderr); + $mergeBase = $this->baseResolver->resolve( + $repoRoot = $this->git->repoRoot($workingDirectory) + ); - throw new \RuntimeException($detail !== '' ? "$error $detail" : $error); + return $this->git->diffFromMergeBase($repoRoot, $mergeBase); } } diff --git a/src/Diff/UpstreamResolver.php b/src/Diff/UpstreamResolver.php new file mode 100644 index 0000000..96ce4c0 --- /dev/null +++ b/src/Diff/UpstreamResolver.php @@ -0,0 +1,131 @@ +cacheDirectory = rtrim($cacheDirectory, '/\\'); + } + + /** Serialises cache updates across parallel runs. */ + public function resolve(string $repoRoot, string $repoName): ?string + { + if (!$this->prepareCacheDirectory()) { + return null; + } + + $lock = @fopen($this->cacheLockPath($repoName), 'c'); + + if ($lock === false) { + return null; + } + + try { + if (!flock($lock, LOCK_EX)) { + return null; + } + + return $this->refreshAndResolve($repoRoot, $repoName); + } catch (GitException) { + return null; + } finally { + flock($lock, LOCK_UN); + fclose($lock); + } + } + + /** @throws GitException */ + private function refreshAndResolve(string $repoRoot, string $repoName): ?string + { + $cacheRepository = $this->cacheRepositoryPath($repoName); + + if (!$this->prepareCacheRepository($cacheRepository)) { + return null; + } + + $this->git->fetchBranchIntoRepo( + $cacheRepository, + sprintf(self::OFFICIAL_REPOSITORY_URL, $repoName), + self::OFFICIAL_BRANCH, + self::CACHED_UPSTREAM_REFERENCE, + ); + + $upstreamCommit = $this->git->resolveCommitHash( + $cacheRepository, + self::CACHED_UPSTREAM_REFERENCE, + ); + + if ($upstreamCommit === null) { + return null; + } + + return $this->git->findMergeBase( + $repoRoot, + 'HEAD', + $upstreamCommit, + sprintf(self::CACHE_OBJECTS_PATH, $cacheRepository), + ); + } + + private function prepareCacheDirectory(): bool + { + return is_dir($this->cacheDirectory) + || @mkdir($this->cacheDirectory, 0777, recursive: true) + || is_dir($this->cacheDirectory); + } + + /** @throws GitException */ + private function prepareCacheRepository(string $cacheRepository): bool + { + if (!is_dir($cacheRepository)) { + return $this->git->initialiseBareRepo($cacheRepository); + } + + if ($this->git->isBareRepo($cacheRepository)) { + return true; + } + + // Keep unexpected directories instead of deleting user data. + $invalidRepository = sprintf(self::INVALID_CACHE_REPOSITORY_PATH, $cacheRepository, date('YmdHis')); + + return @rename($cacheRepository, $invalidRepository) + && $this->git->initialiseBareRepo($cacheRepository); + } + + private function cacheRepositoryPath(string $repoName): string + { + return sprintf(self::CACHE_REPOSITORY_PATH, $this->cacheDirectory, $repoName); + } + + private function cacheLockPath(string $repoName): string + { + return sprintf(self::CACHE_LOCK_PATH, $this->cacheDirectory, $repoName); + } +} diff --git a/src/Git/GitClient.php b/src/Git/GitClient.php new file mode 100644 index 0000000..8008b1f --- /dev/null +++ b/src/Git/GitClient.php @@ -0,0 +1,227 @@ +runAndRequireSuccess( + ['git', 'rev-parse', '--show-toplevel'], + $workingDirectory, + 'Could not find Git repository.', + )); + } + + /** + * Reads configured remote URLs without contacting any remote. + * + * @return list + * @throws GitException + */ + public function configuredRemoteUrls(string $repoRoot): array + { + $result = $this->execute( + ['git', 'config', '--get-regexp', '^remote\\..*\\.url$'], + $repoRoot, + ); + + if ($result->exitCode !== 0) { + return []; + } + + $urls = []; + + foreach (preg_split('/\\R/', trim($result->stdout)) ?: [] as $line) { + $parts = preg_split('/\\s+/', $line, 2); + + if (isset($parts[1])) { + $urls[] = $parts[1]; + } + } + + return $urls; + } + + /** + * Returns the branch name, or null for a detached HEAD. + * + * @throws GitException + */ + public function currentBranchName(string $repoRoot): ?string + { + $result = $this->execute( + ['git', 'symbolic-ref', '--quiet', '--short', 'HEAD'], + $repoRoot, + ); + + return $result->exitCode === 0 ? trim($result->stdout) : null; + } + + /** + * Returns the tracking reference selected by Git configuration. + * + * @throws GitException + */ + public function configuredUpstreamReference(string $repoRoot, string $branch): ?string + { + $result = $this->execute( + ['git', 'rev-parse', '--abbrev-ref', '--symbolic-full-name', $branch . '@{upstream}'], + $repoRoot, + ); + + return $result->exitCode === 0 ? trim($result->stdout) : null; + } + + /** + * Resolves a reference to its commit hash. + * + * @throws GitException + */ + public function resolveCommitHash(string $repoRoot, string $reference): ?string + { + $result = $this->execute( + ['git', 'rev-parse', '--verify', '--quiet', $reference . '^{commit}'], + $repoRoot, + ); + + return $result->exitCode === 0 ? trim($result->stdout) : null; + } + + /** + * Finds a merge base using an optional external object directory. + * + * @throws GitException + */ + public function findMergeBase( + string $repoRoot, + string $firstReference, + string $secondReference, + ?string $alternateObjectDirectory = null, + ): ?string { + $environment = $alternateObjectDirectory !== null + ? ['GIT_ALTERNATE_OBJECT_DIRECTORIES' => $alternateObjectDirectory] + : []; + + $result = $this->execute( + ['git', 'merge-base', $firstReference, $secondReference], + $repoRoot, + $environment, + ); + + return $result->exitCode === 0 ? trim($result->stdout) : null; + } + + /** @throws GitException */ + public function diffFromMergeBase(string $repoRoot, string $mergeBase): string + { + return $this->runAndRequireSuccess( + ['git', 'diff', '--no-ext-diff', '--no-color', $mergeBase, '--'], + $repoRoot, + 'Could not read diff.', + ); + } + + /** + * Checks whether a path contains a bare Git repository. + * + * @throws GitException + */ + public function isBareRepo(string $repoPath): bool + { + $result = $this->execute( + ['git', '-C', $repoPath, 'rev-parse', '--is-bare-repository'], + dirname($repoPath), + ); + + return $result->exitCode === 0 && trim($result->stdout) === 'true'; + } + + /** + * Creates a bare repository for cached upstream history. + * + * @throws GitException + */ + public function initialiseBareRepo(string $repoPath): bool + { + return $this->execute( + ['git', 'init', '--bare', '--quiet', $repoPath], + dirname($repoPath), + )->exitCode === 0; + } + + /** + * Fetches a branch into one private reference. + * Leaves the actual repository unchanged. + * + * @throws GitException + */ + public function fetchBranchIntoRepo(string $repoPath, string $url, string $branch, string $reference): ProcessResult + { + return $this->execute( + [ + 'git', + '-c', + 'credential.interactive=false', + '-c', + 'http.lowSpeedLimit=1', + '-c', + 'http.lowSpeedTime=10', + '-C', + $repoPath, + 'fetch', + '--quiet', + '--no-tags', + '--filter=tree:0', + $url, + sprintf(self::FORCED_BRANCH_REF_SPEC, $branch, $reference), + ], + dirname($repoPath), + ['GIT_TERMINAL_PROMPT' => '0'], + ); + } + + /** + * @param list $command + * @throws GitException + */ + private function runAndRequireSuccess(array $command, string $workingDirectory, string $error): string + { + $result = $this->execute($command, $workingDirectory); + + if ($result->exitCode === 0) { + return $result->stdout; + } + + $detail = trim($result->stderr); + + throw GitException::commandFailed($error, $detail); + } + + /** + * @param list $command + * @param array $environment + * @throws GitException + */ + private function execute(array $command, string $workingDirectory, array $environment = []): ProcessResult + { + try { + return $this->processRunner->run($command, $workingDirectory, $environment); + } catch (ProcessException $exception) { + throw GitException::processFailed($exception); + } + } +} diff --git a/src/Git/GitException.php b/src/Git/GitException.php new file mode 100644 index 0000000..7c9711d --- /dev/null +++ b/src/Git/GitException.php @@ -0,0 +1,30 @@ +getMessage(), 0, $exception); + } + + public static function commandFailed(string $message, string $detail): self + { + return new self($detail !== '' ? "$message $detail" : $message); + } + + public static function localMasterNotFound(): self + { + return new self('Could not find local master branch for the contribution diff.'); + } + + public static function mergeBaseNotFound(string $reference): self + { + return new self(sprintf('Unclear where HEAD branched from %s.', $reference)); + } +} diff --git a/src/Process/NativeProcessRunner.php b/src/Process/NativeProcessRunner.php index dce353c..8e8907c 100644 --- a/src/Process/NativeProcessRunner.php +++ b/src/Process/NativeProcessRunner.php @@ -6,7 +6,7 @@ final class NativeProcessRunner implements ProcessRunnerInterface { - public function run(array $command, string $workingDirectory): ProcessResult + public function run(array $command, string $workingDirectory, array $environment = []): ProcessResult { $process = proc_open( $command, @@ -17,10 +17,11 @@ public function run(array $command, string $workingDirectory): ProcessResult ], $pipes, $workingDirectory, + $this->environmentWithOverrides($environment), ); if (!is_resource($process)) { - throw new \RuntimeException('Could not start process.'); + throw ProcessException::couldNotStart(); } fclose($pipes[0]); @@ -35,4 +36,21 @@ public function run(array $command, string $workingDirectory): ProcessResult stderr: $stderr !== false ? $stderr : '', ); } + + /** + * @param array $overrides + * @return array|null + */ + private function environmentWithOverrides(array $overrides): ?array + { + if ($overrides === []) { + return null; + } + + // proc_open replaces inherited variables with overrides. + // Keep them, then apply only the requested overrides. + $inherited = getenv(); + + return array_replace(is_array($inherited) ? $inherited : [], $overrides); + } } diff --git a/src/Process/ProcessException.php b/src/Process/ProcessException.php new file mode 100644 index 0000000..5dcff63 --- /dev/null +++ b/src/Process/ProcessException.php @@ -0,0 +1,13 @@ + $command - * @throws \RuntimeException if the process cannot be started. + * @param array $environment + * @throws ProcessException if the process cannot be started. */ - public function run(array $command, string $workingDirectory): ProcessResult; + public function run(array $command, string $workingDirectory, array $environment = []): ProcessResult; } diff --git a/tests/Unit/ApplicationInputTest.php b/tests/Unit/ApplicationInputTest.php index 1fbf8f9..b6a8258 100644 --- a/tests/Unit/ApplicationInputTest.php +++ b/tests/Unit/ApplicationInputTest.php @@ -9,8 +9,12 @@ use DocbookCS\Config\ConfigParser; use DocbookCS\Config\SniffEntry; use DocbookCS\Diff\Diff; +use DocbookCS\Diff\DiffBaseResolver; use DocbookCS\Diff\DiffParser; use DocbookCS\Diff\GitDiffProvider; +use DocbookCS\Diff\UpstreamResolver; +use DocbookCS\Git\GitClient; +use DocbookCS\Git\GitException; use DocbookCS\Path\DiffPathLoader; use DocbookCS\Path\EntityResolver; use DocbookCS\Path\PathMatcher; @@ -35,11 +39,14 @@ UsesClass(ConfigParser::class), UsesClass(ConsoleReporter::class), UsesClass(Diff::class), + UsesClass(DiffBaseResolver::class), UsesClass(DiffParser::class), UsesClass(DiffPathLoader::class), UsesClass(EntityPreprocessor::class), UsesClass(EntityResolver::class), + UsesClass(GitClient::class), UsesClass(GitDiffProvider::class), + UsesClass(GitException::class), UsesClass(NullProgress::class), UsesClass(PathMatcher::class), UsesClass(Report::class), @@ -48,6 +55,7 @@ UsesClass(RunScopeResolver::class), UsesClass(SniffEntry::class), UsesClass(SniffRunner::class), + UsesClass(UpstreamResolver::class), UsesClass(XmlFileProcessor::class), ] final class ApplicationInputTest extends TestCase diff --git a/tests/Unit/ApplicationTest.php b/tests/Unit/ApplicationTest.php index 50f1f09..d2b0c61 100644 --- a/tests/Unit/ApplicationTest.php +++ b/tests/Unit/ApplicationTest.php @@ -10,9 +10,13 @@ use DocbookCS\Config\ConfigParserException; use DocbookCS\Config\SniffEntry; use DocbookCS\Diff\Diff; +use DocbookCS\Diff\DiffBaseResolver; use DocbookCS\Diff\DiffParser; use DocbookCS\Diff\FileChange; use DocbookCS\Diff\GitDiffProvider; +use DocbookCS\Diff\UpstreamResolver; +use DocbookCS\Git\GitClient; +use DocbookCS\Git\GitException; use DocbookCS\Path\DiffPathLoader; use DocbookCS\Path\EntityResolver; use DocbookCS\Path\PathLoader; @@ -60,12 +64,16 @@ CoversClass(SniffRunner::class), CoversClass(XmlFileProcessor::class), UsesClass(Diff::class), + UsesClass(DiffBaseResolver::class), UsesClass(DiffPathLoader::class), UsesClass(FileChange::class), + UsesClass(GitClient::class), UsesClass(GitDiffProvider::class), + UsesClass(GitException::class), UsesClass(NativeProcessRunner::class), UsesClass(ProcessResult::class), UsesClass(RunScopeResolver::class), + UsesClass(UpstreamResolver::class), ] final class ApplicationTest extends TestCase { diff --git a/tests/Unit/Diff/GitDiffProviderTest.php b/tests/Unit/Diff/GitDiffProviderTest.php index db7e632..67ddb4b 100644 --- a/tests/Unit/Diff/GitDiffProviderTest.php +++ b/tests/Unit/Diff/GitDiffProviderTest.php @@ -4,7 +4,11 @@ namespace DocbookCS\Tests\Unit\Diff; +use DocbookCS\Diff\DiffBaseResolver; use DocbookCS\Diff\GitDiffProvider; +use DocbookCS\Diff\UpstreamResolver; +use DocbookCS\Git\GitClient; +use DocbookCS\Git\GitException; use DocbookCS\Process\NativeProcessRunner; use DocbookCS\Process\ProcessResult; use PHPUnit\Framework\Attributes\CoversClass; @@ -13,29 +17,44 @@ use PHPUnit\Framework\TestCase; #[ + CoversClass(DiffBaseResolver::class), + CoversClass(GitClient::class), CoversClass(GitDiffProvider::class), + CoversClass(GitException::class), + CoversClass(UpstreamResolver::class), // UsesClass(NativeProcessRunner::class), UsesClass(ProcessResult::class), ] final class GitDiffProviderTest extends TestCase { + private string $workspace; private string $repository; + private string $cacheDirectory; private NativeProcessRunner $processRunner; + private string|false $gitConfigGlobal; protected function setUp(): void { $this->processRunner = new NativeProcessRunner(); + $this->workspace = sys_get_temp_dir() . '/docbook-cs-git-diff-' . bin2hex(random_bytes(6)); + $this->repository = $this->workspace . '/en'; + $this->cacheDirectory = $this->workspace . '/cache'; + $this->gitConfigGlobal = getenv('GIT_CONFIG_GLOBAL'); - $tmpDir = sys_get_temp_dir() . '/docbook-cs-git-diff-' . bin2hex(random_bytes(6)); - mkdir($tmpDir); - $this->repository = $tmpDir; + mkdir($this->workspace); + mkdir($this->repository); + putenv('GIT_CONFIG_GLOBAL=' . $this->workspace . '/gitconfig'); } protected function tearDown(): void { + putenv($this->gitConfigGlobal === false + ? 'GIT_CONFIG_GLOBAL' + : 'GIT_CONFIG_GLOBAL=' . $this->gitConfigGlobal); + $files = new \RecursiveIteratorIterator( - new \RecursiveDirectoryIterator($this->repository, \FilesystemIterator::SKIP_DOTS), + new \RecursiveDirectoryIterator($this->workspace, \FilesystemIterator::SKIP_DOTS), \RecursiveIteratorIterator::CHILD_FIRST, ); @@ -47,72 +66,292 @@ protected function tearDown(): void $file->isDir() ? rmdir($file->getPathname()) : unlink($file->getPathname()); } - rmdir($this->repository); + rmdir($this->workspace); } #[Test] - public function itDiffsTheWorkingTreeFromTheUpstreamBranchPoint(): void + public function itDiffsTheWorkingTreeFromTheCanonicalUpstreamBranchPoint(): void { - $this->git('init', '--quiet', '--initial-branch=main'); - $this->configureAuthor(); - + $this->initializeRepository(); file_put_contents($this->repository . '/base.xml', "base\n"); $this->git('add', 'base.xml'); $this->git('commit', '--quiet', '-m', 'Base'); - $base = $this->git('rev-parse', 'HEAD'); - $this->git('update-ref', 'refs/remotes/upstream/main', $base); - $this->git('symbolic-ref', 'refs/remotes/upstream/HEAD', 'refs/remotes/upstream/main'); + $officialRepository = $this->createOfficialRepository(); + $this->redirectCanonicalUrl($officialRepository); + $this->git('switch', '--quiet', '-c', 'contribution'); - $this->git('branch', '--delete', '--force', 'main'); + $this->git('branch', '--delete', '--force', 'master'); file_put_contents($this->repository . '/committed.xml', "committed\n"); $this->git('add', 'committed.xml'); $this->git('commit', '--quiet', '-m', 'Contribution'); file_put_contents($this->repository . '/base.xml', "working tree\n"); - $diff = new GitDiffProvider($this->processRunner)->for($this->repository); + $diff = $this->provider()->for($this->repository); self::assertStringContainsString('diff --git a/base.xml b/base.xml', $diff); self::assertStringContainsString('+working tree', $diff); self::assertStringContainsString('diff --git a/committed.xml b/committed.xml', $diff); self::assertStringContainsString('+committed', $diff); + self::assertDirectoryExists($this->cacheDirectory . '/doc-en.git'); + } + + #[Test] + public function itIgnoresStaleLocalRemoteTrackingReferences(): void + { + $this->initializeRepository(); + file_put_contents($this->repository . '/base.xml', "base\n"); + $this->git('add', 'base.xml'); + $this->git('commit', '--quiet', '-m', 'Base'); + $base = $this->git('rev-parse', 'HEAD'); + + file_put_contents($this->repository . '/upstream.xml', "upstream\n"); + $this->git('add', 'upstream.xml'); + $this->git('commit', '--quiet', '-m', 'Upstream'); + $upstream = $this->git('rev-parse', 'HEAD'); + + $officialRepository = $this->createOfficialRepository(); + $this->redirectCanonicalUrl($officialRepository); + + $this->git('update-ref', 'refs/remotes/upstream/master', $base); + $this->git('symbolic-ref', 'refs/remotes/upstream/HEAD', 'refs/remotes/upstream/master'); + $this->git('update-ref', 'refs/remotes/origin/master', $upstream); + $this->git('switch', '--quiet', '-c', 'contribution'); + + file_put_contents($this->repository . '/contribution.xml', "contribution\n"); + $this->git('add', 'contribution.xml'); + $this->git('commit', '--quiet', '-m', 'Contribution'); + + $diff = $this->provider()->for($this->repository); + + self::assertStringContainsString('diff --git a/contribution.xml b/contribution.xml', $diff); + self::assertStringNotContainsString('diff --git a/upstream.xml b/upstream.xml', $diff); + } + + #[Test] + public function itUsesTheLastCanonicalCacheWhenRefreshingFails(): void + { + $this->initializeRepository(); + file_put_contents($this->repository . '/base.xml', "base\n"); + $this->git('add', 'base.xml'); + $this->git('commit', '--quiet', '-m', 'Base'); + + $officialRepository = $this->createOfficialRepository(); + $this->redirectCanonicalUrl($officialRepository); + + $this->git('switch', '--quiet', '-c', 'contribution'); + file_put_contents($this->repository . '/contribution.xml', "contribution\n"); + $this->git('add', 'contribution.xml'); + + $provider = $this->provider(); + $provider->for($this->repository); + + rename($officialRepository, $officialRepository . '.offline'); + + $diff = $provider->for($this->repository); + + self::assertStringContainsString('diff --git a/contribution.xml b/contribution.xml', $diff); + } + + #[Test] + public function itRebuildsAnInvalidCacheRepository(): void + { + $this->initializeRepository(); + file_put_contents($this->repository . '/base.xml', "base\n"); + $this->git('add', 'base.xml'); + $this->git('commit', '--quiet', '-m', 'Base'); + + $officialRepository = $this->createOfficialRepository(); + $this->redirectCanonicalUrl($officialRepository); + mkdir($this->cacheDirectory); + mkdir($this->cacheDirectory . '/doc-en.git'); + file_put_contents($this->cacheDirectory . '/doc-en.git/unexpected', ''); + + $this->git('switch', '--quiet', '-c', 'contribution'); + file_put_contents($this->repository . '/contribution.xml', "contribution\n"); + $this->git('add', 'contribution.xml'); + + $diff = $this->provider()->for($this->repository); + + self::assertStringContainsString( + 'diff --git a/contribution.xml b/contribution.xml', + $diff, + ); + self::assertCount( + 1, + glob($this->cacheDirectory . '/doc-en.git.invalid-*') ?: [], + ); + } + + #[Test] + public function itFallsBackToLocalMasterWhenCanonicalRepositoryCannotBeIdentified(): void + { + $this->repository = $this->workspace . '/project'; + mkdir($this->repository); + $this->initializeRepository(); + + file_put_contents($this->repository . '/base.xml', "base\n"); + $this->git('add', 'base.xml'); + $this->git('commit', '--quiet', '-m', 'Base'); + $this->git('switch', '--quiet', '-c', 'contribution'); + + file_put_contents($this->repository . '/contribution.xml', "contribution\n"); + $this->git('add', 'contribution.xml'); + + $diff = $this->provider()->for($this->repository); + + self::assertStringContainsString('diff --git a/contribution.xml b/contribution.xml', $diff); + } + + #[Test] + public function itFindsUnpushedCommitsOnLocalMasterFromItsConfiguredUpstream(): void + { + $this->repository = $this->workspace . '/project'; + mkdir($this->repository); + $this->initializeRepository(); + + file_put_contents($this->repository . '/base.xml', "base\n"); + $this->git('add', 'base.xml'); + $this->git('commit', '--quiet', '-m', 'Base'); + + $upstreamRepository = $this->workspace . '/upstream.git'; + $this->runCommand( + ['git', 'clone', '--bare', '--quiet', $this->repository, $upstreamRepository], + $this->workspace, + ); + $this->git('remote', 'add', 'origin', $upstreamRepository); + $this->git('fetch', '--quiet', 'origin'); + $this->git('branch', '--set-upstream-to=origin/master', 'master'); + + file_put_contents($this->repository . '/unpushed.xml', "unpushed\n"); + $this->git('add', 'unpushed.xml'); + $this->git('commit', '--quiet', '-m', 'Unpushed'); + + $diff = $this->provider()->for($this->repository); + + self::assertStringContainsString('diff --git a/unpushed.xml b/unpushed.xml', $diff); + } + + #[Test] + public function itFallsBackToLocalMasterWhenNoCanonicalCacheCanBeFetched(): void + { + $this->initializeRepository(); + file_put_contents($this->repository . '/base.xml', "base\n"); + $this->git('add', 'base.xml'); + $this->git('commit', '--quiet', '-m', 'Base'); + $this->git('remote', 'add', 'origin', $this->workspace . '/missing/doc-en.git'); + $this->redirectCanonicalUrl($this->workspace . '/missing/doc-en.git'); + + $this->git('switch', '--quiet', '-c', 'contribution'); + file_put_contents($this->repository . '/contribution.xml', "contribution\n"); + $this->git('add', 'contribution.xml'); + + $diff = $this->provider()->for($this->repository); + + self::assertStringContainsString('diff --git a/contribution.xml b/contribution.xml', $diff); } #[Test] - public function itFailsClearlyWhenNoUpstreamDefaultBranchCanBeFound(): void + public function itFailsClearlyWhenNoCanonicalOrLocalMasterCanBeFound(): void { - $this->git('init', '--quiet', '--initial-branch=contribution'); - $this->configureAuthor(); + $this->repository = $this->workspace . '/project'; + mkdir($this->repository); + $this->initializeRepository('contribution'); file_put_contents($this->repository . '/base.xml', "base\n"); $this->git('add', 'base.xml'); $this->git('commit', '--quiet', '-m', 'Contribution'); $this->expectException(\RuntimeException::class); - $this->expectExceptionMessageIsOrContains('Could not determine the upstream default branch'); + $this->expectExceptionMessageIs('Could not find local master branch for the contribution diff.'); - new GitDiffProvider($this->processRunner)->for($this->repository); + $this->provider()->for($this->repository); + } + + #[Test] + public function itFailsClearlyWhenLocalHistoriesAreUnrelated(): void + { + $this->repository = $this->workspace . '/project'; + mkdir($this->repository); + $this->initializeRepository(); + + file_put_contents($this->repository . '/base.xml', "base\n"); + $this->git('add', 'base.xml'); + $this->git('commit', '--quiet', '-m', 'Base'); + $this->git('switch', '--orphan', 'contribution'); + file_put_contents($this->repository . '/contribution.xml', "contribution\n"); + $this->git('add', '--all'); + $this->git('commit', '--quiet', '-m', 'Contribution'); + + $this->expectException(GitException::class); + $this->expectExceptionMessageIs( + 'Unclear where HEAD branched from refs/heads/master.' + ); + + $this->provider()->for($this->repository); } #[Test] public function itIncludesGitErrorsWhenACommandFails(): void { + $this->repository = $this->workspace . '/not-a-repository'; + mkdir($this->repository); + $this->expectException(\RuntimeException::class); $this->expectExceptionMessageIsOrContains('Could not find Git repository. fatal: not a git repository'); - new GitDiffProvider($this->processRunner)->for($this->repository); + $this->provider()->for($this->repository); } - private function configureAuthor(): void + private function initializeRepository(string $branch = 'master'): void { + $this->git('init', '--quiet', '--initial-branch=' . $branch); $this->git('config', 'user.name', 'DocbookCS Tests'); $this->git('config', 'user.email', 'docbook-cs@example.invalid'); } + private function createOfficialRepository(): string + { + $officialRepository = $this->workspace . '/doc-en.git'; + $this->runCommand( + ['git', 'clone', '--bare', '--quiet', $this->repository, $officialRepository], + $this->workspace, + ); + $this->git('remote', 'add', 'origin', $officialRepository); + + return $officialRepository; + } + + private function redirectCanonicalUrl(string $officialRepository): void + { + $this->runCommand( + [ + 'git', + 'config', + '--file', + $this->workspace . '/gitconfig', + 'url.' . $officialRepository . '.insteadOf', + 'https://github.com/php/doc-en.git', + ], + $this->workspace, + ); + } + + private function provider(): GitDiffProvider + { + return new GitDiffProvider($this->processRunner, $this->cacheDirectory); + } + private function git(string ...$arguments): string { - $result = $this->processRunner->run(array_values(['git', ...$arguments]), $this->repository); + return $this->runCommand(array_values(['git', ...$arguments]), $this->repository); + } + + /** @param list $command */ + private function runCommand(array $command, string $workingDirectory): string + { + $result = $this->processRunner->run($command, $workingDirectory); self::assertSame(0, $result->exitCode, $result->stderr); diff --git a/tests/Unit/Git/GitClientTest.php b/tests/Unit/Git/GitClientTest.php new file mode 100644 index 0000000..3407f3d --- /dev/null +++ b/tests/Unit/Git/GitClientTest.php @@ -0,0 +1,44 @@ +repoRoot('.'); + self::fail('Expected GitException was not thrown.'); + } catch (GitException $exception) { + self::assertSame('Could not start process.', $exception->getMessage()); + self::assertInstanceOf( + ProcessException::class, + $exception->getPrevious(), + ); + } + } +} diff --git a/tests/Unit/Process/NativeProcessRunnerTest.php b/tests/Unit/Process/NativeProcessRunnerTest.php index 9970358..3716181 100644 --- a/tests/Unit/Process/NativeProcessRunnerTest.php +++ b/tests/Unit/Process/NativeProcessRunnerTest.php @@ -28,4 +28,17 @@ public function itReturnsTheExitCodeAndOutputStreams(): void self::assertSame('out', $result->stdout); self::assertSame('err', $result->stderr); } + + #[Test] + public function itPassesEnvironmentVariablesToTheProcess(): void + { + $result = new NativeProcessRunner()->run( + [PHP_BINARY, '-r', 'fwrite(STDOUT, getenv("DOCBOOK_CS_TEST") ?: "");'], + getcwd() ?: '.', + ['DOCBOOK_CS_TEST' => 'value'], + ); + + self::assertSame(0, $result->exitCode); + self::assertSame('value', $result->stdout); + } } diff --git a/tests/Unit/Runner/SniffRunnerTest.php b/tests/Unit/Runner/SniffRunnerTest.php index c98c4be..a65da62 100644 --- a/tests/Unit/Runner/SniffRunnerTest.php +++ b/tests/Unit/Runner/SniffRunnerTest.php @@ -7,8 +7,11 @@ use DocbookCS\Config\ConfigData; use DocbookCS\Config\SniffEntry; use DocbookCS\Diff\Diff; +use DocbookCS\Diff\DiffBaseResolver; use DocbookCS\Diff\FileChange; use DocbookCS\Diff\GitDiffProvider; +use DocbookCS\Diff\UpstreamResolver; +use DocbookCS\Git\GitClient; use DocbookCS\Path\DiffPathLoader; use DocbookCS\Path\EntityResolver; use DocbookCS\Path\PathLoader; @@ -48,11 +51,14 @@ CoversClass(Violation::class), CoversClass(XmlFileProcessor::class), UsesClass(Diff::class), + UsesClass(DiffBaseResolver::class), UsesClass(DiffPathLoader::class), UsesClass(EntityExpansionMarker::class), UsesClass(FileChange::class), + UsesClass(GitClient::class), UsesClass(GitDiffProvider::class), UsesClass(RunScopeResolver::class), + UsesClass(UpstreamResolver::class), ] final class SniffRunnerTest extends TestCase { From 504ad8034781463f501318e760fe1b8422f2f04f Mon Sep 17 00:00:00 2001 From: NickSdot Date: Fri, 24 Jul 2026 22:02:01 +0700 Subject: [PATCH 08/15] review: allow --diff as noop for CI compatibility --- src/Application.php | 6 ++++++ tests/Unit/ApplicationInputTest.php | 21 +++++++++++++++++++-- 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/src/Application.php b/src/Application.php index c0781ec..038a841 100644 --- a/src/Application.php +++ b/src/Application.php @@ -242,6 +242,12 @@ private function parseArgv(): array continue; } + // todo: remove; noop - for now kept for CI compatibility + if ($arg === '--diff') { + $i++; + continue; + } + // Anything else is a path to scan. if (!str_starts_with($arg, '-')) { $result['paths'][] = $arg; diff --git a/tests/Unit/ApplicationInputTest.php b/tests/Unit/ApplicationInputTest.php index b6a8258..bc3c7cb 100644 --- a/tests/Unit/ApplicationInputTest.php +++ b/tests/Unit/ApplicationInputTest.php @@ -84,16 +84,33 @@ protected function setUp(): void } #[Test] - public function itRejectsTheRemovedDiffOption(): void + public function itIgnoresTheLegacyDiffOption(): void { $app = new Application( ['docbook-cs', '--config=' . self::VALID_CONFIG, '--diff'], $this->stdout, $this->stderr, + unifiedDiff: '', + ); + + self::assertSame(0, $app->run()); + self::assertSame('', $this->readStream($this->stderr)); + } + + #[Test] + public function itRejectsUnknownOptions(): void + { + $app = new Application( + ['docbook-cs', '--config=' . self::VALID_CONFIG, '--widde'], + $this->stdout, + $this->stderr, ); self::assertSame(2, $app->run()); - self::assertStringContainsString('Unknown option: --diff', $this->readStream($this->stderr)); + self::assertStringContainsString( + 'Unknown option: --widde', + $this->readStream($this->stderr), + ); } #[Test] From 481e96eb463923e996f3e82b447782020b67e89b Mon Sep 17 00:00:00 2001 From: NickSdot Date: Fri, 24 Jul 2026 22:42:52 +0700 Subject: [PATCH 09/15] refactor: moved reporting messages to constants --- src/Sniff/AttributeOrderSniff.php | 7 +++---- src/Sniff/ExceptionNameSniff.php | 7 +++---- src/Sniff/SimparaSniff.php | 4 +++- src/Sniff/WhitespaceSniff.php | 12 +++++++++--- 4 files changed, 18 insertions(+), 12 deletions(-) diff --git a/src/Sniff/AttributeOrderSniff.php b/src/Sniff/AttributeOrderSniff.php index b4d03eb..3332af7 100644 --- a/src/Sniff/AttributeOrderSniff.php +++ b/src/Sniff/AttributeOrderSniff.php @@ -13,6 +13,8 @@ */ final class AttributeOrderSniff extends AbstractSniff { + private const string REPORTING_MESSAGE = 'Element <%s>: xml:id should appear before xmlns attributes.'; + public function getCode(): string { return 'DocbookCS.AttributeOrder'; @@ -84,10 +86,7 @@ private function checkAttributes( $violations[] = $this->createViolation( $filePath, $line, - sprintf( - 'Element <%s>: xml:id should appear before xmlns attributes.', - $tagName, - ), + sprintf(self::REPORTING_MESSAGE, $tagName), ); } } diff --git a/src/Sniff/ExceptionNameSniff.php b/src/Sniff/ExceptionNameSniff.php index acbfbda..95dd91c 100644 --- a/src/Sniff/ExceptionNameSniff.php +++ b/src/Sniff/ExceptionNameSniff.php @@ -14,6 +14,8 @@ */ final class ExceptionNameSniff extends AbstractSniff { + private const string REPORTING_MESSAGE = '"%s" is wrapped in but should use .'; + /** * Default suffixes that indicate the class is an exception or error. */ @@ -56,10 +58,7 @@ public function process(\DOMDocument $document, string $content, string $filePat $violations[] = $this->createViolation( $filePath, $node->getLineNo(), - sprintf( - '"%s" is wrapped in but should use .', - $text, - ), + sprintf(self::REPORTING_MESSAGE, $text), ); } } diff --git a/src/Sniff/SimparaSniff.php b/src/Sniff/SimparaSniff.php index 667da93..3343484 100644 --- a/src/Sniff/SimparaSniff.php +++ b/src/Sniff/SimparaSniff.php @@ -6,6 +6,8 @@ final class SimparaSniff extends AbstractSniff { + private const string REPORTING_MESSAGE = ' contains only inline content and should be .'; + private const array SIMPARA_ALLOWED = [ 'abbrev', 'acronym', @@ -127,7 +129,7 @@ public function process(\DOMDocument $document, string $content, string $filePat $violations[] = $this->createViolation( $filePath, $para->getLineNo(), - ' contains only inline content and should be .', + self::REPORTING_MESSAGE, ); } } diff --git a/src/Sniff/WhitespaceSniff.php b/src/Sniff/WhitespaceSniff.php index 6f72852..5ffa26c 100644 --- a/src/Sniff/WhitespaceSniff.php +++ b/src/Sniff/WhitespaceSniff.php @@ -14,6 +14,12 @@ */ final class WhitespaceSniff extends AbstractSniff { + private const string TRAILING_WHITESPACE_MESSAGE = 'Trailing whitespace detected.'; + + private const string MIXED_INDENTATION_MESSAGE = 'Mixed tabs and spaces in indentation.'; + + private const string INCONSISTENT_INDENTATION_MESSAGE = 'Inconsistent indentation.'; + public function getCode(): string { return 'DocbookCS.Whitespace'; @@ -31,9 +37,9 @@ public function process(\DOMDocument $document, string $content, string $filePat if (preg_match($pattern, $line, $matches)) { $message = match (true) { - !empty($matches[1]) => 'Trailing whitespace detected.', - !empty($matches[2]) || !empty($matches[3]) => 'Mixed tabs and spaces in indentation.', - default => 'Inconsistent indentation.', // @codeCoverageIgnore + !empty($matches[1]) => self::TRAILING_WHITESPACE_MESSAGE, + !empty($matches[2]) || !empty($matches[3]) => self::MIXED_INDENTATION_MESSAGE, + default => self::INCONSISTENT_INDENTATION_MESSAGE, // @codeCoverageIgnore }; $violations[] = $this->createViolation($filePath, $lineNo, $message); From 121a8f5e2248edef724df878b240643d75fa897c Mon Sep 17 00:00:00 2001 From: NickSdot Date: Fri, 24 Jul 2026 22:46:15 +0700 Subject: [PATCH 10/15] refactor: simplified entity preprocessing --- src/Runner/EntityPreprocessor.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Runner/EntityPreprocessor.php b/src/Runner/EntityPreprocessor.php index f7d80b8..9f597e2 100644 --- a/src/Runner/EntityPreprocessor.php +++ b/src/Runner/EntityPreprocessor.php @@ -4,17 +4,17 @@ namespace DocbookCS\Runner; -final class EntityPreprocessor +final readonly class EntityPreprocessor { private const array PREDEFINED = ['amp', 'lt', 'gt', 'quot', 'apos']; - private const string ENTITY_PATTERN = '&([a-zA-Z_][\w.\-]*);'; + private const string EXPANSION_PATTERN = '/||<\?.*?\?>|&([a-zA-Z_][\w.-]*);/s'; private const string XML_DECLARATION_PATTERN = '/<\?xml[^?]*\?>/i'; /** * @param array $entities */ public function __construct( - private array $entities, + private array $entities = [], ) { } @@ -40,7 +40,7 @@ private function expandEntities(string $content, bool $markXmlExpansions = false $changed = false; $content = preg_replace_callback( - '/||<\?[\s\S]*?\?>|' . self::ENTITY_PATTERN . '/', + self::EXPANSION_PATTERN, function (array $matches) use (&$changed, $markXmlExpansions): string { if ( str_starts_with($matches[0], '