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 7b6d92d8cc89f986b70ec54329be989a7f69fbcc Mon Sep 17 00:00:00 2001 From: NickSdot Date: Tue, 21 Jul 2026 05:58:38 +0700 Subject: [PATCH 03/15] refactor: preserve absolute paths until report rendering --- src/RelativePath.php | 23 +++++++++++++++++++ src/Report/Reporter/CheckstyleReporter.php | 3 ++- src/Report/Reporter/ConsoleReporter.php | 9 +++++--- src/Report/Reporter/JsonReporter.php | 3 ++- src/Runner/SniffRunner.php | 19 +-------------- tests/Unit/Report/ReportTest.php | 12 ++++++++++ .../Reporter/CheckstyleReporterTest.php | 21 +++++++++++++++++ .../Report/Reporter/ConsoleReporterTest.php | 18 +++++++++++++++ .../Unit/Report/Reporter/JsonReporterTest.php | 18 +++++++++++++++ tests/Unit/Runner/SniffRunnerTest.php | 6 ++--- 10 files changed, 106 insertions(+), 26 deletions(-) create mode 100644 src/RelativePath.php diff --git a/src/RelativePath.php b/src/RelativePath.php new file mode 100644 index 0000000..1f0689b --- /dev/null +++ b/src/RelativePath.php @@ -0,0 +1,23 @@ +createElement('file'); - $fileNode->setAttribute('name', $fileReport->filePath); + $fileNode->setAttribute('name', RelativePath::fromWorkingDirectory($fileReport->filePath)); foreach ($fileReport->getViolations() as $violation) { $errorNode = $dom->createElement('error'); diff --git a/src/Report/Reporter/ConsoleReporter.php b/src/Report/Reporter/ConsoleReporter.php index 2599bce..3e2a1bb 100644 --- a/src/Report/Reporter/ConsoleReporter.php +++ b/src/Report/Reporter/ConsoleReporter.php @@ -4,6 +4,7 @@ namespace DocbookCS\Report\Reporter; +use DocbookCS\RelativePath; use DocbookCS\Report\Report; use DocbookCS\Report\Severity; @@ -27,9 +28,11 @@ public function generate(Report $report): string continue; } + $filePath = RelativePath::fromWorkingDirectory($fileReport->filePath); + $output .= PHP_EOL; - $output .= $this->bold('FILE: ' . $fileReport->filePath) . PHP_EOL; - $output .= str_repeat('-', min(80, 6 + strlen($fileReport->filePath))) . PHP_EOL; + $output .= $this->bold('FILE: ' . $filePath) . PHP_EOL; + $output .= str_repeat('-', min(80, 6 + strlen($filePath))) . PHP_EOL; foreach ($fileReport->getViolations() as $violation) { $output .= sprintf( @@ -41,7 +44,7 @@ public function generate(Report $report): string ) . PHP_EOL; } - $output .= str_repeat('-', min(80, 6 + strlen($fileReport->filePath))) . PHP_EOL; + $output .= str_repeat('-', min(80, 6 + strlen($filePath))) . PHP_EOL; } $output .= PHP_EOL; diff --git a/src/Report/Reporter/JsonReporter.php b/src/Report/Reporter/JsonReporter.php index 0e52b0f..0eb3e0f 100644 --- a/src/Report/Reporter/JsonReporter.php +++ b/src/Report/Reporter/JsonReporter.php @@ -4,6 +4,7 @@ namespace DocbookCS\Report\Reporter; +use DocbookCS\RelativePath; use DocbookCS\Report\Report; final class JsonReporter implements ReporterInterface @@ -38,7 +39,7 @@ public function generate(Report $report): string ]; } - $data['files'][$fileReport->filePath] = [ + $data['files'][RelativePath::fromWorkingDirectory($fileReport->filePath)] = [ 'violations' => count($violations), 'messages' => $violations, ]; diff --git a/src/Runner/SniffRunner.php b/src/Runner/SniffRunner.php index 923919b..eee358d 100644 --- a/src/Runner/SniffRunner.php +++ b/src/Runner/SniffRunner.php @@ -47,7 +47,7 @@ public function run(RunPlan $plan): Report $fileReport = $processor->processFile( $file, $changedLines, - $this->makeRelative($file), + $file, ); $violationCount = $fileReport->getViolationCount(); @@ -104,21 +104,4 @@ private function instantiateSniffs(array $entries): array return $sniffs; } - - private function makeRelative(string $absolutePath): string - { - $cwd = getcwd(); - if ($cwd === false) { - return $absolutePath; // @codeCoverageIgnore - } - - $prefix = rtrim(str_replace('\\', '/', $cwd), '/') . '/'; - $normalized = str_replace('\\', '/', $absolutePath); - - if (str_starts_with($normalized, $prefix)) { - return substr($normalized, strlen($prefix)); - } - - return $absolutePath; // @codeCoverageIgnore - } } diff --git a/tests/Unit/Report/ReportTest.php b/tests/Unit/Report/ReportTest.php index 86eca0c..c15648f 100644 --- a/tests/Unit/Report/ReportTest.php +++ b/tests/Unit/Report/ReportTest.php @@ -4,6 +4,7 @@ namespace DocbookCS\Tests\Unit\Report; +use DocbookCS\RelativePath; use DocbookCS\Report\FileReport; use DocbookCS\Report\Report; use DocbookCS\Report\Severity; @@ -14,6 +15,7 @@ #[ CoversClass(FileReport::class), + CoversClass(RelativePath::class), CoversClass(Report::class), CoversClass(Violation::class), ] @@ -94,6 +96,16 @@ public function itOverwritesFileReportWithSamePath(): void self::assertSame($second, $report->getFileReports()['file.xml']); } + #[Test] + public function itKeepsTheFileReportPathWhileRenderingItRelativeToWorkingDirectory(): void + { + $filePath = (getcwd() ?: '') . '/src/chapter.xml'; + $fileReport = new FileReport($filePath); + + self::assertSame($filePath, $fileReport->filePath); + self::assertSame('src/chapter.xml', RelativePath::fromWorkingDirectory($fileReport->filePath)); + } + #[Test] public function itReturnsTotalViolationsAcrossAllFiles(): void { diff --git a/tests/Unit/Report/Reporter/CheckstyleReporterTest.php b/tests/Unit/Report/Reporter/CheckstyleReporterTest.php index 87cbfb1..ec30288 100644 --- a/tests/Unit/Report/Reporter/CheckstyleReporterTest.php +++ b/tests/Unit/Report/Reporter/CheckstyleReporterTest.php @@ -4,6 +4,7 @@ namespace DocbookCS\Tests\Unit\Report\Reporter; +use DocbookCS\RelativePath; use DocbookCS\Report\FileReport; use DocbookCS\Report\Report; use DocbookCS\Report\Reporter\CheckstyleReporter; @@ -11,6 +12,7 @@ use DocbookCS\Report\Violation; use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\Attributes\Test; +use PHPUnit\Framework\Attributes\UsesClass; use PHPUnit\Framework\TestCase; #[ @@ -18,6 +20,8 @@ CoversClass(FileReport::class), CoversClass(Report::class), CoversClass(Violation::class), + // + UsesClass(RelativePath::class), ] final class CheckstyleReporterTest extends TestCase { @@ -113,6 +117,23 @@ public function itIncludesFileNodeWithNameAttribute(): void self::assertSame('src/broken.xml', $fileNodes->item(0)?->getAttribute('name')); } + #[Test] + public function itRendersAbsoluteFilePathRelativeToWorkingDirectory(): void + { + $fileReport = new FileReport((getcwd() ?: '') . '/src/broken.xml'); + $fileReport->addViolation($this->createViolation()); + + $report = new Report(); + $report->addFileReport($fileReport); + + $dom = $this->parseOutput($this->reporter->generate($report)); + + self::assertSame( + 'src/broken.xml', + $dom->getElementsByTagName('file')->item(0)?->getAttribute('name'), + ); + } + #[Test] public function itSetsLineAttribute(): void { diff --git a/tests/Unit/Report/Reporter/ConsoleReporterTest.php b/tests/Unit/Report/Reporter/ConsoleReporterTest.php index ad2ba58..34f0bac 100644 --- a/tests/Unit/Report/Reporter/ConsoleReporterTest.php +++ b/tests/Unit/Report/Reporter/ConsoleReporterTest.php @@ -4,6 +4,7 @@ namespace DocbookCS\Tests\Unit\Report\Reporter; +use DocbookCS\RelativePath; use DocbookCS\Report\FileReport; use DocbookCS\Report\Report; use DocbookCS\Report\Reporter\ConsoleReporter; @@ -11,6 +12,7 @@ use DocbookCS\Report\Violation; use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\Attributes\Test; +use PHPUnit\Framework\Attributes\UsesClass; use PHPUnit\Framework\TestCase; #[ @@ -18,6 +20,8 @@ CoversClass(FileReport::class), CoversClass(Report::class), CoversClass(Violation::class), + // + UsesClass(RelativePath::class), ] final class ConsoleReporterTest extends TestCase { @@ -90,6 +94,20 @@ public function itShowsFilePathInHeader(): void self::assertStringContainsString('FILE: src/broken.xml', $output); } + #[Test] + public function itRendersAbsoluteFilePathRelativeToWorkingDirectory(): void + { + $fileReport = new FileReport((getcwd() ?: '') . '/src/broken.xml'); + $fileReport->addViolation($this->createViolation()); + + $report = new Report(); + $report->addFileReport($fileReport); + + $output = $this->reporter->generate($report); + + self::assertStringContainsString('FILE: src/broken.xml', $output); + } + #[Test] public function itShowsDashSeparatorAfterFileHeader(): void { diff --git a/tests/Unit/Report/Reporter/JsonReporterTest.php b/tests/Unit/Report/Reporter/JsonReporterTest.php index 3cbbe72..0a7915c 100644 --- a/tests/Unit/Report/Reporter/JsonReporterTest.php +++ b/tests/Unit/Report/Reporter/JsonReporterTest.php @@ -4,6 +4,7 @@ namespace DocbookCS\Tests\Unit\Report\Reporter; +use DocbookCS\RelativePath; use DocbookCS\Report\FileReport; use DocbookCS\Report\Report; use DocbookCS\Report\Reporter\JsonReporter; @@ -11,6 +12,7 @@ use DocbookCS\Report\Violation; use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\Attributes\Test; +use PHPUnit\Framework\Attributes\UsesClass; use PHPUnit\Framework\TestCase; #[ @@ -18,6 +20,8 @@ CoversClass(JsonReporter::class), CoversClass(Report::class), CoversClass(Violation::class), + // + UsesClass(RelativePath::class), ] final class JsonReporterTest extends TestCase { @@ -312,6 +316,20 @@ public function itDoesNotEscapeSlashesInOutput(): void self::assertStringNotContainsString('path\/to\/file.xml', $output); } + #[Test] + public function itRendersAbsoluteFilePathRelativeToWorkingDirectory(): void + { + $fileReport = new FileReport((getcwd() ?: '') . '/path/to/file.xml'); + $fileReport->addViolation($this->createViolation()); + + $report = new Report(); + $report->addFileReport($fileReport); + + $data = $this->parseOutput($this->reporter->generate($report)); + + self::assertArrayHasKey('path/to/file.xml', $data['files']); + } + #[Test] public function itUsesPrettyPrintedJson(): void { diff --git a/tests/Unit/Runner/SniffRunnerTest.php b/tests/Unit/Runner/SniffRunnerTest.php index c98c4be..6118344 100644 --- a/tests/Unit/Runner/SniffRunnerTest.php +++ b/tests/Unit/Runner/SniffRunnerTest.php @@ -154,7 +154,7 @@ public function setProperty(string $name, string $value): void } #[Test] - public function itStoresRelativePathsInFileReports(): void + public function itStoresAbsolutePathsInFileReports(): void { $sniff = new class implements SniffInterface { public function getCode(): string @@ -186,9 +186,9 @@ public function setProperty(string $name, string $value): void $report = $runner->run($this->planPaths($config)); foreach ($report->getFileReports() as $fileReport) { - self::assertFalse( + self::assertTrue( str_starts_with($fileReport->filePath, '/'), - 'Expected relative path, got: ' . $fileReport->filePath, + 'Expected absolute path, got: ' . $fileReport->filePath, ); } } From 62ecbc0d7b61633e3b7acd7c87adef0c4cd287bb Mon Sep 17 00:00:00 2001 From: NickSdot Date: Tue, 21 Jul 2026 06:01:13 +0700 Subject: [PATCH 04/15] feat: add automatic fixers --- README.md | 12 +- bin/docbook-cs | 11 +- phpcs.xml | 3 + src/Application.php | 35 +- src/Diff/DiffParser.php | 6 +- src/Diff/GitDiffProvider.php | 13 +- src/Fix/Fix.php | 18 + src/Fix/FixApplier.php | 205 ++++++++++ src/Fix/FixPlan.php | 34 ++ src/Fix/FixResult.php | 19 + src/Fix/Fixer/AttributeOrderFixer.php | 80 ++++ src/Fix/Fixer/ExceptionNameFixer.php | 54 +++ src/Fix/Fixer/Fixer.php | 16 + src/Fix/Fixer/MixedIndentationFixer.php | 39 ++ src/Fix/Fixer/SimparaFixer.php | 47 +++ src/Fix/Fixer/TrailingWhitespaceFixer.php | 35 ++ src/Fix/Fixer/WhitespaceFixer.php | 43 ++ src/Fix/FixerException.php | 49 +++ src/Path/DiffPathLoader.php | 8 +- src/Path/EntityResolver.php | 27 +- src/Report/FileReport.php | 11 + src/Report/Report.php | 2 + src/Report/Reporter/ConsoleReporter.php | 2 +- src/Report/Violation.php | 17 - src/Runner/EntityPreprocessor.php | 17 +- .../{SniffRunner.php => RunCoordinator.php} | 64 +-- src/Runner/RunMode.php | 21 + src/Runner/RunPlan.php | 3 +- src/Runner/RunPlanner.php | 10 +- src/Runner/SourceScope.php | 219 ++++++++++ src/Runner/ViolationScopeFilter.php | 129 ++++++ src/Runner/XmlFileProcessor.php | 270 +++++-------- src/Runner/XmlProcessingResult.php | 34 ++ src/Sniff/AbstractSniff.php | 26 +- src/Sniff/AttributeOrderSniff.php | 68 ++-- src/Sniff/ExceptionNameSniff.php | 110 +++-- src/Sniff/Fixable.php | 13 + src/Sniff/MixedIndentationSniff.php | 52 +++ src/Sniff/SimparaSniff.php | 148 ++++++- src/Sniff/SniffInterface.php | 17 +- src/Sniff/TrailingWhitespaceSniff.php | 50 +++ src/Sniff/WhitespaceSniff.php | 56 ++- src/Source/File.php | 116 ++++++ src/Source/Line.php | 26 ++ src/{Report => Violation}/Severity.php | 2 +- src/Violation/SourceRange.php | 15 + src/Violation/Violation.php | 30 ++ tests/Feature/ApplicationInputTest.php | 6 +- .../Integration/Diff/GitDiffProviderTest.php | 3 +- .../Fix/AttributeOrderFixerTest.php | 72 ++++ .../Fix/ExceptionNameFixerTest.php | 128 ++++++ tests/Integration/Fix/SimparaFixerTest.php | 111 ++++++ .../Fix/WhitespaceConcernFixersTest.php | 73 ++++ tests/Integration/Fix/WhitespaceFixerTest.php | 75 ++++ .../Process/NativeProcessRunnerTest.php | 19 +- .../Integration/Runner/FixConvergenceTest.php | 375 ++++++++++++++++++ .../Runner/RunCoordinatorFileFailureTest.php | 107 +++++ .../Runner/RunScopeResolverTest.php | 20 +- tests/Integration/Runner/RunScopeTest.php | 227 +++++++++++ .../Runner/SourceRangeScopeTest.php | 126 ++++++ .../Runner/XmlFileProcessorPipelineTest.php | 101 +++++ .../Sniff/EntityExpandedSniffTest.php | 29 +- tests/Support/Fix/LineBreakFixer.php | 29 ++ tests/Support/Fix/ToggleElementFixer.php | 35 ++ tests/Unit/ApplicationTest.php | 56 +-- tests/Unit/Fix/FixPlanTest.php | 110 +++++ tests/Unit/Report/ReportTest.php | 30 +- .../Reporter/CheckstyleReporterTest.php | 8 +- .../Report/Reporter/ConsoleReporterTest.php | 8 +- .../Unit/Report/Reporter/JsonReporterTest.php | 8 +- tests/Unit/Runner/RunPlannerTest.php | 2 +- tests/Unit/Runner/SniffRunnerTest.php | 153 ++++--- tests/Unit/Runner/SourceScopeTest.php | 140 +++++++ .../Unit/Runner/ViolationScopeFilterTest.php | 51 +++ tests/Unit/Runner/XmlFileProcessorTest.php | 244 ++++++++++-- tests/Unit/Runner/XmlProcessingResultTest.php | 73 ++++ tests/Unit/Sniff/AbstractSniffTest.php | 5 +- tests/Unit/Sniff/AffectedRangesTest.php | 71 ++++ tests/Unit/Sniff/AttributeOrderSniffTest.php | 41 +- tests/Unit/Sniff/ExceptionNameSniffTest.php | 65 ++- tests/Unit/Sniff/SimparaSniffTest.php | 59 +-- .../Sniff/WhitespaceConcernSniffsTest.php | 88 ++++ tests/Unit/Sniff/WhitespaceSniffTest.php | 28 +- tests/Unit/Source/FileTest.php | 96 +++++ tests/Unit/Violation/AffectedRangesTest.php | 50 +++ 85 files changed, 4621 insertions(+), 583 deletions(-) create mode 100644 src/Fix/Fix.php create mode 100644 src/Fix/FixApplier.php create mode 100644 src/Fix/FixPlan.php create mode 100644 src/Fix/FixResult.php create mode 100644 src/Fix/Fixer/AttributeOrderFixer.php create mode 100644 src/Fix/Fixer/ExceptionNameFixer.php create mode 100644 src/Fix/Fixer/Fixer.php create mode 100644 src/Fix/Fixer/MixedIndentationFixer.php create mode 100644 src/Fix/Fixer/SimparaFixer.php create mode 100644 src/Fix/Fixer/TrailingWhitespaceFixer.php create mode 100644 src/Fix/Fixer/WhitespaceFixer.php create mode 100644 src/Fix/FixerException.php delete mode 100644 src/Report/Violation.php rename src/Runner/{SniffRunner.php => RunCoordinator.php} (52%) create mode 100644 src/Runner/RunMode.php create mode 100644 src/Runner/SourceScope.php create mode 100644 src/Runner/ViolationScopeFilter.php create mode 100644 src/Runner/XmlProcessingResult.php create mode 100644 src/Sniff/Fixable.php create mode 100644 src/Sniff/MixedIndentationSniff.php create mode 100644 src/Sniff/TrailingWhitespaceSniff.php create mode 100644 src/Source/File.php create mode 100644 src/Source/Line.php rename src/{Report => Violation}/Severity.php (81%) create mode 100644 src/Violation/SourceRange.php create mode 100644 src/Violation/Violation.php create mode 100644 tests/Integration/Fix/AttributeOrderFixerTest.php create mode 100644 tests/Integration/Fix/ExceptionNameFixerTest.php create mode 100644 tests/Integration/Fix/SimparaFixerTest.php create mode 100644 tests/Integration/Fix/WhitespaceConcernFixersTest.php create mode 100644 tests/Integration/Fix/WhitespaceFixerTest.php create mode 100644 tests/Integration/Runner/FixConvergenceTest.php create mode 100644 tests/Integration/Runner/RunCoordinatorFileFailureTest.php create mode 100644 tests/Integration/Runner/RunScopeTest.php create mode 100644 tests/Integration/Runner/SourceRangeScopeTest.php create mode 100644 tests/Integration/Runner/XmlFileProcessorPipelineTest.php create mode 100644 tests/Support/Fix/LineBreakFixer.php create mode 100644 tests/Support/Fix/ToggleElementFixer.php create mode 100644 tests/Unit/Fix/FixPlanTest.php create mode 100644 tests/Unit/Runner/SourceScopeTest.php create mode 100644 tests/Unit/Runner/ViolationScopeFilterTest.php create mode 100644 tests/Unit/Runner/XmlProcessingResultTest.php create mode 100644 tests/Unit/Sniff/AffectedRangesTest.php create mode 100644 tests/Unit/Sniff/WhitespaceConcernSniffsTest.php create mode 100644 tests/Unit/Source/FileTest.php create mode 100644 tests/Unit/Violation/AffectedRangesTest.php diff --git a/README.md b/README.md index 4be5799..179d77d 100644 --- a/README.md +++ b/README.md @@ -40,15 +40,16 @@ Implement `DocbookCS\Sniff\SniffInterface` (or extend `AbstractSniff`): namespace Acme\DocbookSniffs; use DocbookCS\Sniff\AbstractSniff; +use DocbookCS\Source\File; final class MySniff extends AbstractSniff { - public function getCode(): string + public static function getCode(): string { return 'Acme.MySniff'; } - public function process(\DOMDocument $document, string $content, string $filePath): array + public function process(\DOMDocument $document, File $file): array { $violations = []; // ... inspect $document, add violations via $this->createViolation(...) @@ -70,9 +71,10 @@ 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. +XML references are expanded by default, but violations and fixes remain limited +to the given scope. With `--wide`, every file, inferred from paths or diff, as +a whole will be checked and referenced `SYSTEM` XML files recursively included +in violation reports and fixing. | Input | `--wide` | Full File(s) | References | |------------|---------:|-------------:|-----------:| diff --git a/bin/docbook-cs b/bin/docbook-cs index 4b72e67..5db6d29 100644 --- a/bin/docbook-cs +++ b/bin/docbook-cs @@ -14,7 +14,7 @@ use DocbookCS\Application; foreach ($candidates as $file) { if (!is_file($file)) { - continue; + continue; } require $file; @@ -25,4 +25,11 @@ use DocbookCS\Application; exit(2); })(); -exit(Application::fromGlobals($argv ?? [])->run()); +try { + $application = Application::fromGlobals($argv ?? []); +} catch (\RuntimeException $e) { + fwrite(STDERR, $e->getMessage() . PHP_EOL); + exit(2); +} + +exit($application->run()); diff --git a/phpcs.xml b/phpcs.xml index 93f3b2f..3495fd9 100644 --- a/phpcs.xml +++ b/phpcs.xml @@ -2,6 +2,9 @@ DocbookCS Coding Standard + + tests/* + diff --git a/src/Application.php b/src/Application.php index bb36f76..59e7e3e 100644 --- a/src/Application.php +++ b/src/Application.php @@ -14,8 +14,9 @@ use DocbookCS\Report\Reporter\ConsoleReporter; use DocbookCS\Report\Reporter\JsonReporter; use DocbookCS\Report\Reporter\ReporterInterface; +use DocbookCS\Runner\RunCoordinator; +use DocbookCS\Runner\RunMode; use DocbookCS\Runner\RunPlanner; -use DocbookCS\Runner\SniffRunner; final class Application { @@ -112,7 +113,11 @@ public function run(): int } try { - $runPlan = new RunPlanner($config, $options['wide'])->plan($options['paths'], $this->stdin); + $runPlan = new RunPlanner( + config: $config, + mode: RunMode::fromFixFlag($options['fix']), + wide: $options['wide'], + )->plan($options['paths'], $this->stdin); } catch (\Throwable $e) { $this->writeError('Error resolving input: ' . $e->getMessage() . PHP_EOL); @@ -122,8 +127,7 @@ public function run(): int $progress = $this->createProgress($options); try { - $runner = new SniffRunner($progress); - $report = $runner->run($runPlan); + $report = new RunCoordinator($progress)->run($runPlan); } catch (\Throwable $e) { $this->writeError('Runtime error: ' . $e->getMessage() . PHP_EOL); @@ -148,12 +152,13 @@ public function run(): int * config: string, * report: string, * colors: bool, + * fix: bool, + * wide: bool, * quiet: bool, * paths: list, - * wide: bool, * perf: bool, * } - * @throws \InvalidArgumentException for unsupported options. + * @throws \InvalidArgumentException for unsupported or removed options. */ private function parseArgv(): array { @@ -163,9 +168,10 @@ private function parseArgv(): array 'config' => self::DEFAULT_CONFIG, 'report' => 'console', 'colors' => $this->detectColorSupport(), + 'fix' => false, + 'wide' => false, 'quiet' => false, 'paths' => [], - 'wide' => false, 'perf' => false, ]; @@ -236,6 +242,12 @@ private function parseArgv(): array continue; } + if ($arg === '--fix') { + $result['fix'] = true; + $i++; + continue; + } + if ($arg === '--wide') { $result['wide'] = true; $i++; @@ -359,6 +371,8 @@ private function printHelp(): void --report= Output format: console (default), checkstyle, json. --colors Force ANSI color output. --no-colors Disable ANSI color output. + --fix Automatically fix violations when fixers exist + (experimental). --wide Check whole selected files and recursively include referenced XML files. @@ -370,9 +384,14 @@ private function printHelp(): void docbook-cs docbook-cs --config=myconfig.xml reference/ docbook-cs --report=checkstyle --no-colors > report.xml + docbook-cs . --fix + docbook-cs reference/ docbook-cs reference/strings/functions/strlen.xml + docbook-cs reference/strings/functions/strlen.xml --wide + docbook-cs reference/strings/functions/strlen.xml --wide --fix git diff HEAD | docbook-cs - git diff HEAD | docbook-cs --wide --report=checkstyle + git diff HEAD | docbook-cs --wide + git diff HEAD | docbook-cs --wide --fix --report=checkstyle HELP; diff --git a/src/Diff/DiffParser.php b/src/Diff/DiffParser.php index 0c62f3a..95e274f 100644 --- a/src/Diff/DiffParser.php +++ b/src/Diff/DiffParser.php @@ -101,7 +101,11 @@ public function parse(string $diff): Diff $fileChanges = []; foreach ($changedLinesByFile as $filePath => $lineNumbers) { - $fileChanges[] = new FileChange($filePath, $lineNumbers, $deletionAnchorsByFile[$filePath]); + $fileChanges[] = new FileChange( + $filePath, + $lineNumbers, + $deletionAnchorsByFile[$filePath], + ); } return new Diff($fileChanges); diff --git a/src/Diff/GitDiffProvider.php b/src/Diff/GitDiffProvider.php index 3e7c146..fe4e6b3 100644 --- a/src/Diff/GitDiffProvider.php +++ b/src/Diff/GitDiffProvider.php @@ -24,10 +24,13 @@ public function for(string $workingDirectory): string )); $baseReference = $this->resolveBaseReference($repositoryRoot); + + $error = sprintf('Unclear where HEAD branched from %s.', $baseReference); + $mergeBase = $this->runOrThrow( ['git', 'merge-base', 'HEAD', $baseReference], $repositoryRoot, - sprintf('Unclear where HEAD branched from %s.', $baseReference), + $error, ); return $this->runOrThrow( @@ -70,7 +73,9 @@ private function resolveBaseReference(string $repositoryRoot): string } } - throw new \RuntimeException('Could not determine the upstream default branch for the contribution diff.'); + throw new \RuntimeException( + 'Could not determine the upstream default branch for the contribution diff.', + ); } /** @@ -87,6 +92,8 @@ private function runOrThrow(array $command, string $workingDirectory, string $er $detail = trim($result->stderr); - throw new \RuntimeException($detail !== '' ? "$error $detail" : $error); + throw new \RuntimeException( + $detail !== '' ? "$error $detail" : $error, + ); } } diff --git a/src/Fix/Fix.php b/src/Fix/Fix.php new file mode 100644 index 0000000..031b262 --- /dev/null +++ b/src/Fix/Fix.php @@ -0,0 +1,18 @@ + $fixes + */ + public function apply(File $file, array $fixes): FixResult + { + if ($fixes === []) { + return new FixResult($file); + } + + $plans = []; + foreach ($fixes as $index => $fix) { + $plan = $fix instanceof FixPlan ? $fix : new FixPlan($fix); + $plans[] = [ + 'index' => $index, + 'plan' => $plan, + ]; + } + + usort($plans, static function (array $a, array $b): int { + $offsetComparison = $a['plan']->firstOffset() <=> $b['plan']->firstOffset(); + + return $offsetComparison !== 0 + ? $offsetComparison + : $a['index'] <=> $b['index']; + }); + + /** @var list $acceptedFixes */ + $acceptedFixes = []; + $acceptedPlans = 0; + $skipped = 0; + + $content = $file->content; + $length = strlen($content); + + foreach ($plans as ['plan' => $plan]) { + if (!$this->canApply($file, $length, $plan, $acceptedFixes)) { + $skipped++; + continue; + } + + if (!$this->changesContent($content, $plan)) { + $skipped++; + continue; + } + + foreach ($plan->fixes as $fix) { + $this->insertFix($acceptedFixes, $fix); + } + + $acceptedPlans++; + } + + $fixedContent = ''; + $sourceOffset = 0; + + foreach ($acceptedFixes as $fix) { + $fixedContent .= substr($content, $sourceOffset, $fix->beginOffset - $sourceOffset); + $fixedContent .= $fix->replacement; + $sourceOffset = $fix->untilOffset; + } + + $content = $fixedContent . substr($content, $sourceOffset); + + return new FixResult( + file: $file->withContent($content), + applied: $acceptedPlans, + skipped: $skipped, + appliedFixes: $acceptedFixes, + ); + } + + /** + * @param list $acceptedFixes + */ + private function canApply(File $file, int $contentLength, FixPlan $plan, array $acceptedFixes): bool + { + $content = $file->content; + $first = $plan->fixes[0]; + $previous = null; + + foreach ($plan->fixes as $fix) { + if ( + $fix->filePath !== $file->path + || $fix->filePath !== $first->filePath + || $fix->sniffCode !== $first->sniffCode + || $fix->beginOffset < 0 + || $fix->untilOffset < $fix->beginOffset + || $fix->untilOffset > $contentLength + || ($previous !== null && self::overlaps($previous, $fix)) + || $this->conflictsWithAcceptedFix($fix, $acceptedFixes) + ) { + return false; + } + + $currentContent = substr($content, $fix->beginOffset, $fix->untilOffset - $fix->beginOffset); + + if ($fix->expectedContent !== null && $currentContent !== $fix->expectedContent) { + return false; + } + + $previous = $fix; + } + + return true; + } + + private function changesContent(string $content, FixPlan $plan): bool + { + foreach ($plan->fixes as $fix) { + $currentContent = substr($content, $fix->beginOffset, $fix->untilOffset - $fix->beginOffset); + + if ($currentContent !== $fix->replacement) { + return true; + } + } + + return false; + } + + /** + * @param list $acceptedFixes + */ + private function conflictsWithAcceptedFix(Fix $fix, array $acceptedFixes): bool + { + $index = $this->insertionIndex($acceptedFixes, $fix); + + return ($index > 0 && self::overlaps($acceptedFixes[$index - 1], $fix)) + || ($index < count($acceptedFixes) && self::overlaps($acceptedFixes[$index], $fix)); + } + + /** + * @param list $fixes + */ + private function insertFix(array &$fixes, Fix $fix): void + { + $lastIndex = count($fixes) - 1; + if ($lastIndex < 0 || self::compare($fixes[$lastIndex], $fix) < 0) { + $fixes[] = $fix; + return; + } + + array_splice($fixes, $this->insertionIndex($fixes, $fix), 0, [$fix]); + } + + /** + * @param list $fixes + */ + private function insertionIndex(array $fixes, Fix $fix): int + { + $low = 0; + $high = count($fixes); + + while ($low < $high) { + $middle = intdiv($low + $high, 2); + if (self::compare($fixes[$middle], $fix) < 0) { + $low = $middle + 1; + } else { + $high = $middle; + } + } + + return $low; + } + + private static function compare(Fix $a, Fix $b): int + { + return [ + $a->beginOffset, + $a->untilOffset, + ] <=> [ + $b->beginOffset, + $b->untilOffset, + ]; + } + + private static function overlaps(Fix $a, Fix $b): bool + { + $aIsInsertion = $a->beginOffset === $a->untilOffset; + $bIsInsertion = $b->beginOffset === $b->untilOffset; + + if ($aIsInsertion && $bIsInsertion) { + return $a->beginOffset === $b->beginOffset; + } + + if ($aIsInsertion) { + return $a->beginOffset > $b->beginOffset && $a->beginOffset < $b->untilOffset; + } + + if ($bIsInsertion) { + return $b->beginOffset > $a->beginOffset && $b->beginOffset < $a->untilOffset; + } + + return $a->beginOffset < $b->untilOffset && $b->beginOffset < $a->untilOffset; + } +} diff --git a/src/Fix/FixPlan.php b/src/Fix/FixPlan.php new file mode 100644 index 0000000..d3d2f63 --- /dev/null +++ b/src/Fix/FixPlan.php @@ -0,0 +1,34 @@ + */ + public array $fixes; + + public function __construct(Fix $first, Fix ...$additionalFixes) + { + $fixes = [$first, ...$additionalFixes]; + + usort( + $fixes, + static fn(Fix $a, Fix $b): int => [ + $a->beginOffset, + $a->untilOffset, + ] <=> [ + $b->beginOffset, + $b->untilOffset, + ], + ); + + $this->fixes = $fixes; + } + + public function firstOffset(): int + { + return $this->fixes[0]->beginOffset; + } +} diff --git a/src/Fix/FixResult.php b/src/Fix/FixResult.php new file mode 100644 index 0000000..e4d4b00 --- /dev/null +++ b/src/Fix/FixResult.php @@ -0,0 +1,19 @@ + $appliedFixes */ + public function __construct( + public File $file, + public int $applied = 0, + public int $skipped = 0, + public array $appliedFixes = [], + ) { + } +} diff --git a/src/Fix/Fixer/AttributeOrderFixer.php b/src/Fix/Fixer/AttributeOrderFixer.php new file mode 100644 index 0000000..c49ad3e --- /dev/null +++ b/src/Fix/Fixer/AttributeOrderFixer.php @@ -0,0 +1,80 @@ +'; + private const string OPENING_TAG_PATTERN = '/^<([a-zA-Z0-9:_-]+)\b([^<>]*?)>$/'; + private const string ATTRIBUTE_TOKEN_PATTERN = '/\s+([a-zA-Z0-9:_-]+)\s*=\s*(?:"[^"]*"|\'[^\']*\')/'; + + /** @throws FixerException */ + public function process(Violation $violation): Fix + { + if ($violation->content === null) { + throw FixerException::cannotFixMissingContent(); + } + + if (!preg_match(self::OPENING_TAG_PATTERN, $violation->content, $matches)) { + throw FixerException::cannotFixInvalidContent($violation); + } + + $fixedAttributeString = $this->fixedAttributeString($matches[2]); + + if ($fixedAttributeString === null) { + throw FixerException::cannotFixInvalidContent($violation); + } + + return new Fix( + $violation->filePath, + $violation->beginOffset, + $violation->untilOffset, + sprintf(self::OPENING_TAG_FORMAT, $matches[1], $fixedAttributeString), + $violation->sniffCode, + ); + } + + private function fixedAttributeString(string $attrString): ?string + { + if (!str_contains($attrString, 'xml:id') || !str_contains($attrString, 'xmlns')) { + return null; + } + + preg_match_all(self::ATTRIBUTE_TOKEN_PATTERN, $attrString, $matches, PREG_OFFSET_CAPTURE); + + $xmlIdToken = null; + $firstXmlnsStart = null; + + foreach ($matches[0] as $i => [$token, $start]) { + $start = (int) $start; + $name = $matches[1][$i][0]; + + if ($name === 'xml:id') { + $xmlIdToken = [ + 'text' => $token, + 'start' => $start, + 'end' => $start + strlen($token), + ]; + } + + if (($name === 'xmlns' || str_starts_with($name, 'xmlns:')) && $firstXmlnsStart === null) { + $firstXmlnsStart = $start; + } + } + + if ($xmlIdToken === null || $firstXmlnsStart === null || $xmlIdToken['start'] < $firstXmlnsStart) { + return null; + } + + return substr($attrString, 0, $firstXmlnsStart) + . $xmlIdToken['text'] + . substr($attrString, $firstXmlnsStart, $xmlIdToken['start'] - $firstXmlnsStart) + . substr($attrString, $xmlIdToken['end']); + } +} diff --git a/src/Fix/Fixer/ExceptionNameFixer.php b/src/Fix/Fixer/ExceptionNameFixer.php new file mode 100644 index 0000000..0949246 --- /dev/null +++ b/src/Fix/Fixer/ExceptionNameFixer.php @@ -0,0 +1,54 @@ +]*)>([^<]*)<\/classname>$/'; + + /** @throws FixerException */ + public function process(Violation $violation): FixPlan + { + if ($violation->content === null) { + throw FixerException::cannotFixMissingContent(); + } + + if (!preg_match(self::CLASSNAME_PATTERN, $violation->content, $matches)) { + throw FixerException::cannotFixInvalidContent($violation); + } + + $text = trim($matches[2]); + + if ($text === '' || !ExceptionNameSniff::looksLikeException($text)) { + throw FixerException::cannotFixInvalidContent($violation); + } + + if (count($violation->affectedRanges) !== 2) { + throw FixerException::cannotFixInvalidContent($violation); + } + + $fixes = []; + foreach ($violation->affectedRanges as $range) { + $fixes[] = new Fix( + $violation->filePath, + $range->beginOffset, + $range->untilOffset, + self::TARGET_ELEMENT, + $violation->sniffCode, + self::SOURCE_ELEMENT, + ); + } + + return new FixPlan(...$fixes); + } +} diff --git a/src/Fix/Fixer/Fixer.php b/src/Fix/Fixer/Fixer.php new file mode 100644 index 0000000..a488c7f --- /dev/null +++ b/src/Fix/Fixer/Fixer.php @@ -0,0 +1,16 @@ +content === null) { + throw FixerException::cannotFixMissingContent(); + } + + if ( + !preg_match(self::INDENTATION_PATTERN, $violation->content) + || !str_contains($violation->content, ' ') + || !str_contains($violation->content, "\t") + ) { + throw FixerException::cannotFixInvalidContent($violation); + } + + return new Fix( + filePath: $violation->filePath, + beginOffset: $violation->beginOffset, + untilOffset: $violation->untilOffset, + replacement: str_replace("\t", ' ', $violation->content), + sniffCode: $violation->sniffCode, + expectedContent: $violation->content, + ); + } +} diff --git a/src/Fix/Fixer/SimparaFixer.php b/src/Fix/Fixer/SimparaFixer.php new file mode 100644 index 0000000..b815a26 --- /dev/null +++ b/src/Fix/Fixer/SimparaFixer.php @@ -0,0 +1,47 @@ +]*)>(.*)<\/para>$/s'; + + /** @throws FixerException */ + public function process(Violation $violation): FixPlan + { + if ($violation->content === null) { + throw FixerException::cannotFixMissingContent(); + } + + if (!preg_match(self::PARA_PATTERN, $violation->content, $matches)) { + throw FixerException::cannotFixInvalidContent($violation); + } + + if (count($violation->affectedRanges) !== 2) { + throw FixerException::cannotFixInvalidContent($violation); + } + + $fixes = []; + foreach ($violation->affectedRanges as $range) { + $fixes[] = new Fix( + $violation->filePath, + $range->beginOffset, + $range->untilOffset, + self::TARGET_ELEMENT, + $violation->sniffCode, + self::SOURCE_ELEMENT, + ); + } + + return new FixPlan(...$fixes); + } +} diff --git a/src/Fix/Fixer/TrailingWhitespaceFixer.php b/src/Fix/Fixer/TrailingWhitespaceFixer.php new file mode 100644 index 0000000..092e534 --- /dev/null +++ b/src/Fix/Fixer/TrailingWhitespaceFixer.php @@ -0,0 +1,35 @@ +content === null) { + throw FixerException::cannotFixMissingContent(); + } + + if (!preg_match(self::WHITESPACE_PATTERN, $violation->content)) { + throw FixerException::cannotFixInvalidContent($violation); + } + + return new Fix( + filePath: $violation->filePath, + beginOffset: $violation->beginOffset, + untilOffset: $violation->untilOffset, + replacement: '', + sniffCode: $violation->sniffCode, + expectedContent: $violation->content, + ); + } +} diff --git a/src/Fix/Fixer/WhitespaceFixer.php b/src/Fix/Fixer/WhitespaceFixer.php new file mode 100644 index 0000000..f630150 --- /dev/null +++ b/src/Fix/Fixer/WhitespaceFixer.php @@ -0,0 +1,43 @@ +content === null) { + throw FixerException::cannotFixMissingContent(); + } + + $fixed = rtrim($violation->content, " \t"); + + if (preg_match('/^[ \t]+/', $fixed, $matches)) { + $fixedIndent = str_replace("\t", ' ', $matches[0]); + $fixed = $fixedIndent . substr($fixed, strlen($matches[0])); + } + + if ($fixed === $violation->content) { + throw FixerException::cannotFixInvalidContent($violation); + } + + return new Fix( + $violation->filePath, + $violation->beginOffset, + $violation->untilOffset, + $fixed, + $violation->sniffCode, + ); + } +} diff --git a/src/Fix/FixerException.php b/src/Fix/FixerException.php new file mode 100644 index 0000000..afad17b --- /dev/null +++ b/src/Fix/FixerException.php @@ -0,0 +1,49 @@ +sniffCode, + $violation->filePath, + $violation->line, + )); + } + + public static function cannotReadFixedContent(): self + { + return new self('Cannot read fixed content when no fix application was attempted.'); + } + + public static function invalidFixedXml(string $filePath): self + { + return new self( + sprintf('Fixers produced invalid XML for %s; no changes were written.', $filePath), + ); + } + + public static function didNotConverge(string $filePath): self + { + return new self( + sprintf('Fixers did not converge for %s; no changes were written.', $filePath), + ); + } +} diff --git a/src/Path/DiffPathLoader.php b/src/Path/DiffPathLoader.php index 22706e0..5e48116 100644 --- a/src/Path/DiffPathLoader.php +++ b/src/Path/DiffPathLoader.php @@ -9,7 +9,9 @@ final readonly class DiffPathLoader { - /** @param array $projectRoots */ + /** + * @param array $projectRoots + */ public function __construct( private Diff $diff, private string $workingDirectory, @@ -69,8 +71,8 @@ private function candidates(string $path): array } return array_map($this->normalize(...), $candidates) - |> array_unique(...) - |> array_values(...); + |> array_unique(...) + |> array_values(...); } private function isAbsolute(string $path): bool diff --git a/src/Path/EntityResolver.php b/src/Path/EntityResolver.php index 8dc3f29..aafb27b 100644 --- a/src/Path/EntityResolver.php +++ b/src/Path/EntityResolver.php @@ -121,12 +121,8 @@ private function scanDirectory(string $directory): array * @param array $paths * @return array */ - private function resolveFile( - string $filePath, - array &$visited, - array &$paths, - ?string $originEntity = null - ): array { + private function resolveFile(string $filePath, array &$visited, array &$paths, ?string $originEntity = null): array + { if (isset($visited[$filePath]) || !is_readable($filePath)) { return []; } @@ -153,14 +149,9 @@ private function resolveFile( * @param array $paths * @return array */ - private function extractEntities( - string $content, - string $filePath, - array &$visited, - array &$paths, - ): array { - return - $this->extractDtdEntities($content, $filePath, $visited, $paths) + private function extractEntities(string $content, string $filePath, array &$visited, array &$paths): array + { + return $this->extractDtdEntities($content, $filePath, $visited, $paths) + $this->extractXmlEntities($content); } @@ -169,12 +160,8 @@ private function extractEntities( * @param array $paths * @return array */ - private function extractDtdEntities( - string $content, - string $filePath, - array &$visited, - array &$paths, - ): array { + private function extractDtdEntities(string $content, string $filePath, array &$visited, array &$paths): array + { $result = []; if (!str_contains($content, ' */ @@ -19,6 +22,14 @@ public function addViolation(Violation $violation): void $this->violations[] = $violation; } + /** @param list $violations */ + public function addViolations(array $violations): void + { + foreach ($violations as $violation) { + $this->addViolation($violation); + } + } + /** @return list */ public function getViolations(): array { diff --git a/src/Report/Report.php b/src/Report/Report.php index 37ccbe5..77e7335 100644 --- a/src/Report/Report.php +++ b/src/Report/Report.php @@ -4,6 +4,8 @@ namespace DocbookCS\Report; +use DocbookCS\Violation\Violation; + final class Report { /** @var array */ diff --git a/src/Report/Reporter/ConsoleReporter.php b/src/Report/Reporter/ConsoleReporter.php index 3e2a1bb..b6967c6 100644 --- a/src/Report/Reporter/ConsoleReporter.php +++ b/src/Report/Reporter/ConsoleReporter.php @@ -6,7 +6,7 @@ use DocbookCS\RelativePath; use DocbookCS\Report\Report; -use DocbookCS\Report\Severity; +use DocbookCS\Violation\Severity; final class ConsoleReporter implements ReporterInterface { diff --git a/src/Report/Violation.php b/src/Report/Violation.php deleted file mode 100644 index 24df707..0000000 --- a/src/Report/Violation.php +++ /dev/null @@ -1,17 +0,0 @@ -||<\?[\s\S]*?\?>|' . self::ENTITY_PATTERN . '/', function (array $matches) use (&$changed, $markXmlExpansions): string { - if ( - str_starts_with($matches[0], '', + ' ']]>', + ' '?>', + ]; + /** @var array */ protected array $properties = []; @@ -62,4 +68,89 @@ protected function createViolation( affectedRanges: $affectedRanges, ); } + + protected function maskNonElementMarkup(string $source): string + { + $masked = $source; + $offset = 0; + + while (false !== $start = strpos($source, '<', $offset)) { + $endOffset = $this->nonElementMarkupEndOffset($source, $start); + + if ($endOffset === null) { + $offset = $start + 1; + continue; + } + + for ($i = $start; $i < $endOffset; $i++) { + $masked[$i] = ' '; + } + + $offset = $endOffset; + } + + return $masked; + } + + private function nonElementMarkupEndOffset(string $source, int $start): ?int + { + foreach (self::NON_ELEMENT_DELIMITERS as $opening => $closing) { + if (substr_compare($source, $opening, $start, strlen($opening)) === 0) { + return $this->offsetAfterDelimiter($source, $closing, $start); + } + } + + if (substr_compare($source, 'declarationEndOffset($source, $start); + } + + return null; + } + + private function offsetAfterDelimiter(string $source, string $delimiter, int $offset): int + { + $end = strpos($source, $delimiter, $offset); + + return $end === false ? strlen($source) : $end + strlen($delimiter); + } + + private function declarationEndOffset(string $source, int $offset): int + { + $length = strlen($source); + $quote = null; + $bracketDepth = 0; + + for ($i = $offset; $i < $length; $i++) { + $character = $source[$i]; + + if ($quote !== null) { + if ($character === $quote) { + $quote = null; + } + + continue; + } + + if ($character === '"' || $character === "'") { + $quote = $character; + continue; + } + + if ($character === '[') { + $bracketDepth++; + continue; + } + + if ($character === ']') { + $bracketDepth--; + continue; + } + + if ($character === '>' && $bracketDepth === 0) { + return $i + 1; + } + } + + return $length; + } } diff --git a/src/Sniff/AttributeOrderSniff.php b/src/Sniff/AttributeOrderSniff.php index cc0fa22..119824c 100644 --- a/src/Sniff/AttributeOrderSniff.php +++ b/src/Sniff/AttributeOrderSniff.php @@ -38,7 +38,12 @@ public function process(\DOMDocument $document, File $file): array $violations = []; // Match ONLY opening tags (skip closing, comments, xml decl) - preg_match_all(self::OPENING_TAG_PATTERN, $file->content, $matches, PREG_OFFSET_CAPTURE); + preg_match_all( + self::OPENING_TAG_PATTERN, + $this->maskNonElementMarkup($file->content), + $matches, + PREG_OFFSET_CAPTURE, + ); foreach ($matches[0] as $i => [$fullMatch, $offset]) { $tagName = $matches[1][$i][0]; diff --git a/src/Sniff/ExceptionNameSniff.php b/src/Sniff/ExceptionNameSniff.php index 1a4c553..49c3cc4 100644 --- a/src/Sniff/ExceptionNameSniff.php +++ b/src/Sniff/ExceptionNameSniff.php @@ -118,7 +118,12 @@ public static function looksLikeException(string $text): bool */ private function sourceMatches(File $file): array { - preg_match_all(self::CLASSNAME_PATTERN, $file->content, $matches, PREG_OFFSET_CAPTURE); + preg_match_all( + self::CLASSNAME_PATTERN, + $this->maskNonElementMarkup($file->content), + $matches, + PREG_OFFSET_CAPTURE, + ); $sourceMatches = []; foreach ($matches[0] as $i => [$fullMatch, $offset]) { diff --git a/src/Sniff/SimparaSniff.php b/src/Sniff/SimparaSniff.php index fe687c9..f41d164 100644 --- a/src/Sniff/SimparaSniff.php +++ b/src/Sniff/SimparaSniff.php @@ -221,7 +221,12 @@ private function getAllowedElements(): array */ private function sourceMatches(File $file): array { - preg_match_all(self::PARA_TAG_PATTERN, $file->content, $matches, PREG_OFFSET_CAPTURE); + preg_match_all( + self::PARA_TAG_PATTERN, + $this->maskNonElementMarkup($file->content), + $matches, + PREG_OFFSET_CAPTURE, + ); /** @var list $stack */ $stack = []; diff --git a/tests/Integration/Fix/AttributeOrderFixerTest.php b/tests/Integration/Fix/AttributeOrderFixerTest.php index 2a40473..19d22c4 100644 --- a/tests/Integration/Fix/AttributeOrderFixerTest.php +++ b/tests/Integration/Fix/AttributeOrderFixerTest.php @@ -62,6 +62,29 @@ public function itMovesXmlIdBeforeXmlns(): void self::assertSame(1, $result->applied); } + #[Test] + public function itPreservesTagShapedTextInsideComments(): void + { + $content = '' + . ''; + $document = $this->createDocument($content); + $source = new File('file.xml', $content); + + $violations = new AttributeOrderSniff(RunMode::Fix)->process($document, $source); + + self::assertCount(1, $violations); + + $fix = new AttributeOrderFixer()->process($violations[0]); + $result = new FixApplier()->apply($source, [$fix]); + + self::assertSame( + '' + . '', + $result->file->content, + ); + self::assertSame(1, $result->applied); + } + private function createDocument(string $xml): \DOMDocument { $document = new \DOMDocument(); diff --git a/tests/Integration/Fix/ExceptionNameFixerTest.php b/tests/Integration/Fix/ExceptionNameFixerTest.php index 846f6d1..9744a35 100644 --- a/tests/Integration/Fix/ExceptionNameFixerTest.php +++ b/tests/Integration/Fix/ExceptionNameFixerTest.php @@ -118,6 +118,29 @@ public function itKeepsSourceContentAlignedAfterRegularClassnames(): void self::assertSame(1, $result->applied); } + #[Test] + public function itPreservesTagShapedTextInsideComments(): void + { + $content = '' + . 'RuntimeException'; + $document = $this->createDocument($content); + $source = new File('file.xml', $content); + + $violations = new ExceptionNameSniff(RunMode::Fix)->process($document, $source); + + self::assertCount(1, $violations); + + $fix = new ExceptionNameFixer()->process($violations[0]); + $result = new FixApplier()->apply($source, [$fix]); + + self::assertSame( + '' + . 'RuntimeException', + $result->file->content, + ); + self::assertSame(1, $result->applied); + } + private function createDocument(string $xml): \DOMDocument { $document = new \DOMDocument(); diff --git a/tests/Integration/Fix/SimparaFixerTest.php b/tests/Integration/Fix/SimparaFixerTest.php index 99aba05..23de962 100644 --- a/tests/Integration/Fix/SimparaFixerTest.php +++ b/tests/Integration/Fix/SimparaFixerTest.php @@ -101,6 +101,27 @@ public function itCanFixNestedInnerParas(): void self::assertSame(1, $result->applied); } + #[Test] + public function itPreservesTagShapedTextInsideComments(): void + { + $content = 'Source'; + $document = $this->createDocument($content); + $source = new File('file.xml', $content); + + $violations = new SimparaSniff(RunMode::Fix)->process($document, $source); + + self::assertCount(1, $violations); + + $fix = new SimparaFixer()->process($violations[0]); + $result = new FixApplier()->apply($source, [$fix]); + + self::assertSame( + 'Source', + $result->file->content, + ); + self::assertSame(1, $result->applied); + } + private function createDocument(string $xml): \DOMDocument { $document = new \DOMDocument(); diff --git a/tests/Unit/Sniff/AttributeOrderSniffTest.php b/tests/Unit/Sniff/AttributeOrderSniffTest.php index 4a89fbe..173fe44 100644 --- a/tests/Unit/Sniff/AttributeOrderSniffTest.php +++ b/tests/Unit/Sniff/AttributeOrderSniffTest.php @@ -156,4 +156,17 @@ public function itAddsSourceContent(): void self::assertSame(38, $violations[0]->untilOffset); self::assertSame(1, $violations[0]->line); } + + #[Test] + public function itIgnoresElementsInsideComments(): void + { + $content = '' + . ''; + $doc = $this->createDocument($content); + + $violations = new AttributeOrderSniff()->process($doc, new File('file.xml', $content)); + + self::assertCount(1, $violations); + self::assertSame('', $violations[0]->content); + } } diff --git a/tests/Unit/Sniff/ExceptionNameSniffTest.php b/tests/Unit/Sniff/ExceptionNameSniffTest.php index b4c8355..ab3abf4 100644 --- a/tests/Unit/Sniff/ExceptionNameSniffTest.php +++ b/tests/Unit/Sniff/ExceptionNameSniffTest.php @@ -201,4 +201,17 @@ public function itKeepsSourceContentAlignedAfterRegularClassnames(): void self::assertSame($beginOffset, $violations[0]->beginOffset); self::assertSame($beginOffset + strlen($sourceContent), $violations[0]->untilOffset); } + + #[Test] + public function itIgnoresClassnamesInsideCommentsWhenMappingSource(): void + { + $content = '' + . 'RuntimeException'; + $doc = $this->createDocument($content); + + $violations = new ExceptionNameSniff()->process($doc, new File('file.xml', $content)); + + self::assertCount(1, $violations); + self::assertSame('RuntimeException', $violations[0]->content); + } } diff --git a/tests/Unit/Sniff/SimparaSniffTest.php b/tests/Unit/Sniff/SimparaSniffTest.php index b0004f0..5542d00 100644 --- a/tests/Unit/Sniff/SimparaSniffTest.php +++ b/tests/Unit/Sniff/SimparaSniffTest.php @@ -124,6 +124,27 @@ public function itReportsCorrectLineNumber(): void self::assertSame(2, $violations[0]->line); } + #[Test] + public function itIgnoresTagShapedTextOutsideElementsWhenMappingSource(): void + { + $content = <<<'XML' + Declared">]> + + + CDATA]]> + Instruction?> + Source + + XML; + $doc = $this->createDocument($content); + + $violations = new SimparaSniff()->process($doc, new File('file.xml', $content)); + + self::assertCount(1, $violations); + self::assertSame('Source', $violations[0]->content); + self::assertSame(6, $violations[0]->line); + } + #[Test] public function itDoesNotFlagParaInsideFormalpara(): void { From 156cc0dc15b7a9f3bb519f9e1534f05702e531d6 Mon Sep 17 00:00:00 2001 From: NickSdot Date: Tue, 21 Jul 2026 06:55:53 +0700 Subject: [PATCH 06/15] refactor: renamed diff class --- src/Diff/{Diff.php => DiffChangeset.php} | 2 +- src/Diff/DiffParser.php | 4 ++-- src/Path/DiffPathLoader.php | 8 ++++---- src/Runner/RunPlanner.php | 4 ++-- src/Runner/RunScopeResolver.php | 4 ++-- tests/Integration/Path/DiffPathLoaderTest.php | 8 ++++---- .../Runner/RunCoordinatorFileFailureTest.php | 6 +++--- tests/Integration/Runner/RunScopeResolverTest.php | 8 ++++---- tests/Integration/Runner/RunScopeTest.php | 10 +++++----- tests/Unit/ApplicationTest.php | 5 +++-- tests/Unit/Diff/DiffParserTest.php | 7 ++++--- tests/Unit/Runner/RunPlannerTest.php | 4 ++-- tests/Unit/Runner/SniffRunnerTest.php | 15 ++++++++------- 13 files changed, 44 insertions(+), 41 deletions(-) rename src/Diff/{Diff.php => DiffChangeset.php} (95%) diff --git a/src/Diff/Diff.php b/src/Diff/DiffChangeset.php similarity index 95% rename from src/Diff/Diff.php rename to src/Diff/DiffChangeset.php index 83b3bd9..c6f5319 100644 --- a/src/Diff/Diff.php +++ b/src/Diff/DiffChangeset.php @@ -4,7 +4,7 @@ namespace DocbookCS\Diff; -final readonly class Diff +final readonly class DiffChangeset { /** @param list $fileChanges */ public function __construct(public array $fileChanges) diff --git a/src/Diff/DiffParser.php b/src/Diff/DiffParser.php index 95e274f..b43c2c3 100644 --- a/src/Diff/DiffParser.php +++ b/src/Diff/DiffParser.php @@ -8,7 +8,7 @@ final class DiffParser { private const string NO_FINAL_LINE_MARKER = '\ No newline at end of file'; - public function parse(string $diff): Diff + public function parse(string $diff): DiffChangeset { /** @var array> $changedLinesByFile */ $changedLinesByFile = []; @@ -108,6 +108,6 @@ public function parse(string $diff): Diff ); } - return new Diff($fileChanges); + return new DiffChangeset($fileChanges); } } diff --git a/src/Path/DiffPathLoader.php b/src/Path/DiffPathLoader.php index 5e48116..82aa1d9 100644 --- a/src/Path/DiffPathLoader.php +++ b/src/Path/DiffPathLoader.php @@ -4,7 +4,7 @@ namespace DocbookCS\Path; -use DocbookCS\Diff\Diff; +use DocbookCS\Diff\DiffChangeset; use DocbookCS\Diff\FileChange; final readonly class DiffPathLoader @@ -13,7 +13,7 @@ * @param array $projectRoots */ public function __construct( - private Diff $diff, + private DiffChangeset $diff, private string $workingDirectory, private string $basePath, private array $projectRoots, @@ -21,7 +21,7 @@ public function __construct( ) { } - public function load(): Diff + public function load(): DiffChangeset { $changes = []; @@ -44,7 +44,7 @@ public function load(): Diff ksort($changes); - return new Diff(array_values($changes)); + return new DiffChangeset(array_values($changes)); } /** @return list */ diff --git a/src/Runner/RunPlanner.php b/src/Runner/RunPlanner.php index 1e41086..09ea004 100644 --- a/src/Runner/RunPlanner.php +++ b/src/Runner/RunPlanner.php @@ -5,7 +5,7 @@ namespace DocbookCS\Runner; use DocbookCS\Config\ConfigData; -use DocbookCS\Diff\Diff; +use DocbookCS\Diff\DiffChangeset; use DocbookCS\Diff\DiffParser; use DocbookCS\Diff\DiffProviderInterface; use DocbookCS\Diff\GitDiffProvider; @@ -65,7 +65,7 @@ public function planPaths(array $paths): RunPlan } /** @throws \UnexpectedValueException if an entity directory cannot be read. */ - public function planDiff(Diff $diff): RunPlan + public function planDiff(DiffChangeset $diff): RunPlan { return new RunPlan( mode: $this->mode, diff --git a/src/Runner/RunScopeResolver.php b/src/Runner/RunScopeResolver.php index 931da12..89efb53 100644 --- a/src/Runner/RunScopeResolver.php +++ b/src/Runner/RunScopeResolver.php @@ -5,7 +5,7 @@ namespace DocbookCS\Runner; use DocbookCS\Config\ConfigData; -use DocbookCS\Diff\Diff; +use DocbookCS\Diff\DiffChangeset; use DocbookCS\Diff\FileChange; use DocbookCS\Path\DiffPathLoader; use DocbookCS\Path\PathLoader; @@ -46,7 +46,7 @@ public function resolvePaths(array $paths): array } /** @return array */ - public function resolveDiff(Diff $diff): array + public function resolveDiff(DiffChangeset $diff): array { $resolvedDiff = new DiffPathLoader( $diff, diff --git a/tests/Integration/Path/DiffPathLoaderTest.php b/tests/Integration/Path/DiffPathLoaderTest.php index 47ba2e4..0bf530d 100644 --- a/tests/Integration/Path/DiffPathLoaderTest.php +++ b/tests/Integration/Path/DiffPathLoaderTest.php @@ -4,7 +4,7 @@ namespace DocbookCS\Tests\Integration\Path; -use DocbookCS\Diff\Diff; +use DocbookCS\Diff\DiffChangeset; use DocbookCS\Diff\FileChange; use DocbookCS\Path\DiffPathLoader; use DocbookCS\Path\PathMatcher; @@ -14,7 +14,7 @@ use PHPUnit\Framework\TestCase; #[ - CoversClass(Diff::class), + CoversClass(DiffChangeset::class), CoversClass(DiffPathLoader::class), CoversClass(PathMatcher::class), // @@ -44,7 +44,7 @@ public function itLoadsChangedXmlFilesWithoutScanningConfiguredPaths(): void file_put_contents($file, ''); $loader = new DiffPathLoader( - new Diff([new FileChange('docs/chapter.xml', [1])]), + new DiffChangeset([new FileChange('docs/chapter.xml', [1])]), workingDirectory: dirname($this->directory), basePath: $this->directory, projectRoots: [$this->directory => 'docs'], @@ -64,7 +64,7 @@ public function itIgnoresMissingNonXmlAndExcludedFiles(): void file_put_contents($excluded, ''); $loader = new DiffPathLoader( - new Diff([ + new DiffChangeset([ new FileChange('excluded.xml', [1]), new FileChange('notes.txt', [1]), new FileChange('missing.xml', [1]), diff --git a/tests/Integration/Runner/RunCoordinatorFileFailureTest.php b/tests/Integration/Runner/RunCoordinatorFileFailureTest.php index bcc21d8..a3578b1 100644 --- a/tests/Integration/Runner/RunCoordinatorFileFailureTest.php +++ b/tests/Integration/Runner/RunCoordinatorFileFailureTest.php @@ -5,7 +5,7 @@ namespace DocbookCS\Tests\Integration\Runner; use DocbookCS\Config\ConfigData; -use DocbookCS\Diff\Diff; +use DocbookCS\Diff\DiffChangeset; use DocbookCS\Diff\FileChange; use DocbookCS\Progress\ProgressInterface; use DocbookCS\Runner\RunCoordinator; @@ -87,7 +87,7 @@ static function () use ($xmlFilePath): void { entityPaths: [], basePath: dirname($xmlFilePath), ); - $diff = new Diff([new FileChange($xmlFilePath, [42])]); + $diff = new DiffChangeset([new FileChange($xmlFilePath, [42])]); $report = new RunCoordinator($progress)->run($this->planDiff($config, $diff)); @@ -100,7 +100,7 @@ private function planPaths(ConfigData $config): RunPlan return new RunPlanner($config)->planPaths($config->getIncludePaths()); } - private function planDiff(ConfigData $config, Diff $diff): RunPlan + private function planDiff(ConfigData $config, DiffChangeset $diff): RunPlan { return new RunPlanner($config)->planDiff($diff); } diff --git a/tests/Integration/Runner/RunScopeResolverTest.php b/tests/Integration/Runner/RunScopeResolverTest.php index 2315c5b..dbf2f28 100644 --- a/tests/Integration/Runner/RunScopeResolverTest.php +++ b/tests/Integration/Runner/RunScopeResolverTest.php @@ -5,7 +5,7 @@ namespace DocbookCS\Tests\Integration\Runner; use DocbookCS\Config\ConfigData; -use DocbookCS\Diff\Diff; +use DocbookCS\Diff\DiffChangeset; use DocbookCS\Diff\FileChange; use DocbookCS\Path\DiffPathLoader; use DocbookCS\Path\PathLoader; @@ -21,7 +21,7 @@ CoversClass(RunScopeResolver::class), // UsesClass(ConfigData::class), - UsesClass(Diff::class), + UsesClass(DiffChangeset::class), UsesClass(DiffPathLoader::class), UsesClass(FileChange::class), UsesClass(PathLoader::class), @@ -61,7 +61,7 @@ public function narrowScopeKeepsOnlySelectedFilesAndDiffLines(): void $resolver = $this->resolver(); $targets = $resolver->resolveDiff( - new Diff([new FileChange('source.xml', [2, 3])]), + new DiffChangeset([new FileChange('source.xml', [2, 3])]), ); self::assertSame([2, 3], $targets[$this->sourceFile]?->addedLineNumbers); @@ -74,7 +74,7 @@ public function wideScopeWidensSelectedFilesAndFollowsReferencedTargets(): void $resolver = $this->resolver(wide: true); $targets = $resolver->resolveDiff( - new Diff([new FileChange('source.xml', [2, 3])]), + new DiffChangeset([new FileChange('source.xml', [2, 3])]), ); self::assertNull($targets[$this->sourceFile]); diff --git a/tests/Integration/Runner/RunScopeTest.php b/tests/Integration/Runner/RunScopeTest.php index 0841a65..a34f4e5 100644 --- a/tests/Integration/Runner/RunScopeTest.php +++ b/tests/Integration/Runner/RunScopeTest.php @@ -6,7 +6,7 @@ use DocbookCS\Config\ConfigData; use DocbookCS\Config\SniffEntry; -use DocbookCS\Diff\Diff; +use DocbookCS\Diff\DiffChangeset; use DocbookCS\Diff\FileChange; use DocbookCS\Diff\GitDiffProvider; use DocbookCS\Fix\Fix; @@ -54,7 +54,7 @@ // UsesClass(AbstractSniff::class), UsesClass(ConfigData::class), - UsesClass(Diff::class), + UsesClass(DiffChangeset::class), UsesClass(EntityExpansionMarker::class), UsesClass(EntityPreprocessor::class), UsesClass(File::class), @@ -159,7 +159,7 @@ public function aDiffProvidesItsOwnFilesWithoutConfiguredIncludePaths(): void $report = $this->executeDiff( $config, - new Diff([ + new DiffChangeset([ new FileChange($this->sourceFile, [1]), ]), ); @@ -181,7 +181,7 @@ public function aDiffPathUsingAProjectDirectoryKeepsItsSourceRanges(): void $report = $this->executeDiff( $config, - new Diff([ + new DiffChangeset([ new FileChange('docs/source.xml', [1]), ]), ); @@ -216,7 +216,7 @@ private function executePaths( private function executeDiff( ConfigData $config, - Diff $diff, + DiffChangeset $diff, RunMode $mode = RunMode::Sniff, bool $wide = false, ): Report { diff --git a/tests/Unit/ApplicationTest.php b/tests/Unit/ApplicationTest.php index 8ecf80c..654bd48 100644 --- a/tests/Unit/ApplicationTest.php +++ b/tests/Unit/ApplicationTest.php @@ -6,7 +6,7 @@ use DocbookCS\Application; use DocbookCS\Config\ConfigData; -use DocbookCS\Diff\Diff; +use DocbookCS\Diff\DiffChangeset; use DocbookCS\Diff\DiffParser; use DocbookCS\Diff\FileChange; use DocbookCS\Diff\GitDiffProvider; @@ -65,7 +65,8 @@ CoversClass(RunPlanner::class), CoversClass(SniffEntry::class), CoversClass(XmlFileProcessor::class), - UsesClass(Diff::class), + // + UsesClass(DiffChangeset::class), UsesClass(DiffPathLoader::class), UsesClass(File::class), UsesClass(FileChange::class), diff --git a/tests/Unit/Diff/DiffParserTest.php b/tests/Unit/Diff/DiffParserTest.php index 1e49b92..b1bd988 100644 --- a/tests/Unit/Diff/DiffParserTest.php +++ b/tests/Unit/Diff/DiffParserTest.php @@ -4,7 +4,7 @@ namespace DocbookCS\Tests\Unit\Diff; -use DocbookCS\Diff\Diff; +use DocbookCS\Diff\DiffChangeset; use DocbookCS\Diff\DiffParser; use DocbookCS\Diff\FileChange; use PHPUnit\Framework\Attributes\CoversClass; @@ -14,7 +14,8 @@ #[ CoversClass(DiffParser::class), - UsesClass(Diff::class), + // + UsesClass(DiffChangeset::class), UsesClass(FileChange::class), ] final class DiffParserTest extends TestCase @@ -277,7 +278,7 @@ public function itHandlesHunkWithNoContext(): void // TODO: avoids test diff churn; remove when fixers merged /** @return array> */ - private function lineNumbersByFile(Diff $diff): array + private function lineNumbersByFile(DiffChangeset $diff): array { $lineNumbersByFile = []; diff --git a/tests/Unit/Runner/RunPlannerTest.php b/tests/Unit/Runner/RunPlannerTest.php index 511586b..8515f25 100644 --- a/tests/Unit/Runner/RunPlannerTest.php +++ b/tests/Unit/Runner/RunPlannerTest.php @@ -5,7 +5,7 @@ namespace DocbookCS\Tests\Unit\Runner; use DocbookCS\Config\ConfigData; -use DocbookCS\Diff\Diff; +use DocbookCS\Diff\DiffChangeset; use DocbookCS\Diff\FileChange; use DocbookCS\Diff\DiffParser; use DocbookCS\Diff\DiffProviderInterface; @@ -25,7 +25,7 @@ CoversClass(RunPlanner::class), // UsesClass(ConfigData::class), - UsesClass(Diff::class), + UsesClass(DiffChangeset::class), UsesClass(DiffParser::class), UsesClass(DiffPathLoader::class), UsesClass(EntityResolver::class), diff --git a/tests/Unit/Runner/SniffRunnerTest.php b/tests/Unit/Runner/SniffRunnerTest.php index bc1e8c3..3e37cda 100644 --- a/tests/Unit/Runner/SniffRunnerTest.php +++ b/tests/Unit/Runner/SniffRunnerTest.php @@ -6,7 +6,7 @@ use DocbookCS\Config\ConfigData; use DocbookCS\Config\SniffEntry; -use DocbookCS\Diff\Diff; +use DocbookCS\Diff\DiffChangeset; use DocbookCS\Diff\FileChange; use DocbookCS\Diff\GitDiffProvider; use DocbookCS\Path\DiffPathLoader; @@ -55,7 +55,8 @@ CoversClass(SniffEntry::class), CoversClass(Violation::class), CoversClass(XmlFileProcessor::class), - UsesClass(Diff::class), + // + UsesClass(DiffChangeset::class), UsesClass(DiffPathLoader::class), UsesClass(EntityExpansionMarker::class), UsesClass(File::class), @@ -291,7 +292,7 @@ public function itFiltersFilesToOnlyThoseInTheDiff(): void $config = $this->createConfig(); $runner = new RunCoordinator(); - $diff = new Diff([new FileChange(self::FIXTURE_DIR . '/file_a.xml', [1])]); + $diff = new DiffChangeset([new FileChange(self::FIXTURE_DIR . '/file_a.xml', [1])]); $report = $runner->run($this->planDiff($config, $diff)); self::assertSame(1, $report->getFilesScanned()); @@ -303,7 +304,7 @@ public function itScansNoFilesWhenDiffContainsNoMatchingPaths(): void $config = $this->createConfig(); $runner = new RunCoordinator(); - $diff = new Diff([new FileChange('completely/different/file.xml', [1, 2, 3])]); + $diff = new DiffChangeset([new FileChange('completely/different/file.xml', [1, 2, 3])]); $report = $runner->run($this->planDiff($config, $diff)); self::assertSame(0, $report->getFilesScanned()); @@ -317,7 +318,7 @@ public function itMatchesWhenDiffPathEqualsDiscoveredPath(): void $discoveredPath = self::FIXTURE_DIR . '/file_a.xml'; - $diff = new Diff([new FileChange($discoveredPath, [1])]); + $diff = new DiffChangeset([new FileChange($discoveredPath, [1])]); $report = $runner->run($this->planDiff($config, $diff)); self::assertSame(1, $report->getFilesScanned()); @@ -415,7 +416,7 @@ public function setProperty(string $name, string $value): void $config = $this->createConfig(sniffs: [new SniffEntry($sniff::class)]); $runner = new RunCoordinator(); - $diff = new Diff([new FileChange(self::FIXTURE_DIR . '/file_a.xml', [])]); + $diff = new DiffChangeset([new FileChange(self::FIXTURE_DIR . '/file_a.xml', [])]); $report = $runner->run($this->planDiff($config, $diff)); self::assertSame(1, $report->getFilesScanned()); @@ -428,7 +429,7 @@ private function planPaths(ConfigData $config, ?array $paths = null, RunMode $mo return new RunPlanner($config, $mode)->planPaths($paths ?? $config->getIncludePaths()); } - private function planDiff(ConfigData $config, Diff $diff, RunMode $mode = RunMode::Sniff): RunPlan + private function planDiff(ConfigData $config, DiffChangeset $diff, RunMode $mode = RunMode::Sniff): RunPlan { return new RunPlanner($config, $mode)->planDiff($diff); } From 614bfe3fd9973e02b7060668bdc570a40ce22cbf Mon Sep 17 00:00:00 2001 From: NickSdot Date: Tue, 21 Jul 2026 07:22:29 +0700 Subject: [PATCH 07/15] test: remove diff parser test helper --- tests/Unit/Diff/DiffParserTest.php | 69 +++++++++++++----------------- 1 file changed, 29 insertions(+), 40 deletions(-) diff --git a/tests/Unit/Diff/DiffParserTest.php b/tests/Unit/Diff/DiffParserTest.php index b1bd988..c1f3040 100644 --- a/tests/Unit/Diff/DiffParserTest.php +++ b/tests/Unit/Diff/DiffParserTest.php @@ -30,7 +30,7 @@ protected function setUp(): void #[Test] public function itReturnsEmptyArrayForEmptyDiff(): void { - self::assertSame([], $this->lineNumbersByFile($this->parser->parse(''))); + self::assertSame([], $this->parser->parse('')->fileChanges); } #[Test] @@ -47,10 +47,10 @@ public function itParsesAddedLineNumbers(): void line3 DIFF; - $result = $this->lineNumbersByFile($this->parser->parse($diff)); + $change = $this->parser->parse($diff)->changeFor('reference/file.xml'); - self::assertArrayHasKey('reference/file.xml', $result); - self::assertSame([2], $result['reference/file.xml']); + self::assertNotNull($change); + self::assertSame([2], $change->addedLineNumbers); } #[Test] @@ -68,9 +68,9 @@ public function itParsesMultipleAddedLines(): void last line DIFF; - $result = $this->lineNumbersByFile($this->parser->parse($diff)); + $result = $this->parser->parse($diff); - self::assertSame([6, 7], $result['doc/chapter.xml']); + self::assertSame([6, 7], $result->changeFor('doc/chapter.xml')?->addedLineNumbers); } #[Test] @@ -85,10 +85,10 @@ public function itStripsTheBPrefix(): void +added DIFF; - $result = $this->lineNumbersByFile($this->parser->parse($diff)); + $result = $this->parser->parse($diff); - self::assertArrayHasKey('src/file.xml', $result); - self::assertArrayNotHasKey('b/src/file.xml', $result); + self::assertCount(1, $result->fileChanges); + self::assertSame('src/file.xml', $result->fileChanges[0]->filePath); } #[Test] @@ -105,7 +105,7 @@ public function itExcludesDeletedFiles(): void -line3 DIFF; - self::assertSame([], $this->lineNumbersByFile($this->parser->parse($diff))); + self::assertSame([], $this->parser->parse($diff)->fileChanges); } #[Test] @@ -122,10 +122,10 @@ public function itHandlesNewlyCreatedFiles(): void +line3 DIFF; - $result = $this->lineNumbersByFile($this->parser->parse($diff)); + $change = $this->parser->parse($diff)->changeFor('new.xml'); - self::assertArrayHasKey('new.xml', $result); - self::assertSame([1, 2, 3], $result['new.xml']); + self::assertNotNull($change); + self::assertSame([1, 2, 3], $change->addedLineNumbers); } #[Test] @@ -148,12 +148,14 @@ public function itHandlesMultipleFilesInOneDiff(): void unchanged DIFF; - $result = $this->lineNumbersByFile($this->parser->parse($diff)); + $result = $this->parser->parse($diff); + $firstChange = $result->changeFor('first.xml'); + $secondChange = $result->changeFor('second.xml'); - self::assertArrayHasKey('first.xml', $result); - self::assertArrayHasKey('second.xml', $result); - self::assertSame([2], $result['first.xml']); - self::assertSame([2], $result['second.xml']); + self::assertNotNull($firstChange); + self::assertNotNull($secondChange); + self::assertSame([2], $firstChange->addedLineNumbers); + self::assertSame([2], $secondChange->addedLineNumbers); } #[Test] @@ -170,11 +172,11 @@ public function itIgnoresRemovedLines(): void line3 DIFF; - $result = $this->lineNumbersByFile($this->parser->parse($diff)); + $change = $this->parser->parse($diff)->changeFor('file.xml'); // No lines added, so the changed set is empty (not absent — the file is tracked). - self::assertArrayHasKey('file.xml', $result); - self::assertSame([], $result['file.xml']); + self::assertNotNull($change); + self::assertSame([], $change->addedLineNumbers); } #[Test] @@ -211,9 +213,9 @@ public function itIgnoresTheMissingFinalNewlineMarker(): void +second DIFF; - $result = $this->lineNumbersByFile($this->parser->parse($diff)); + $result = $this->parser->parse($diff); - self::assertSame([1, 2], $result['file.xml']); + self::assertSame([1, 2], $result->changeFor('file.xml')?->addedLineNumbers); } #[Test] @@ -255,9 +257,9 @@ public function itTracksLineNumbersAcrossMultipleHunks(): void line12 DIFF; - $result = $this->lineNumbersByFile($this->parser->parse($diff)); + $result = $this->parser->parse($diff); - self::assertSame([2, 12], $result['file.xml']); + self::assertSame([2, 12], $result->changeFor('file.xml')?->addedLineNumbers); } #[Test] @@ -271,21 +273,8 @@ public function itHandlesHunkWithNoContext(): void +only line DIFF; - $result = $this->lineNumbersByFile($this->parser->parse($diff)); - - self::assertSame([1], $result['file.xml']); - } - - // TODO: avoids test diff churn; remove when fixers merged - /** @return array> */ - private function lineNumbersByFile(DiffChangeset $diff): array - { - $lineNumbersByFile = []; - - foreach ($diff->fileChanges as $fileChange) { - $lineNumbersByFile[$fileChange->filePath] = $fileChange->addedLineNumbers; - } + $result = $this->parser->parse($diff); - return $lineNumbersByFile; + self::assertSame([1], $result->changeFor('file.xml')?->addedLineNumbers); } } From d74fb1343d5ff716a2180069ff7dcbe57b145119 Mon Sep 17 00:00:00 2001 From: NickSdot Date: Tue, 21 Jul 2026 07:29:54 +0700 Subject: [PATCH 08/15] test: move tests to appropriate suites --- tests/{Unit => Feature}/ApplicationTest.php | 38 ++++++------- .../Runner/SniffRunnerTest.php | 30 +++++------ .../Runner/XmlFileProcessorPipelineTest.php | 54 +++++++++++++++++++ tests/Unit/Runner/XmlFileProcessorTest.php | 46 ---------------- 4 files changed, 88 insertions(+), 80 deletions(-) rename tests/{Unit => Feature}/ApplicationTest.php (93%) rename tests/{Unit => Integration}/Runner/SniffRunnerTest.php (95%) diff --git a/tests/Unit/ApplicationTest.php b/tests/Feature/ApplicationTest.php similarity index 93% rename from tests/Unit/ApplicationTest.php rename to tests/Feature/ApplicationTest.php index 654bd48..d60ff78 100644 --- a/tests/Unit/ApplicationTest.php +++ b/tests/Feature/ApplicationTest.php @@ -2,7 +2,7 @@ declare(strict_types=1); -namespace DocbookCS\Tests\Unit; +namespace DocbookCS\Tests\Feature; use DocbookCS\Application; use DocbookCS\Config\ConfigData; @@ -112,7 +112,7 @@ private function readStream(mixed $stream): string return stream_get_contents($stream) ?: ''; } - #[Test] // TODO: should be feature + #[Test] public function itPrintsHelpAndExitsWithZero(): void { $app = new Application(['docbook-cs', '--help'], $this->stdout, $this->stderr); @@ -124,7 +124,7 @@ public function itPrintsHelpAndExitsWithZero(): void self::assertSame('', $this->readStream($this->stderr)); } - #[Test] // TODO: should be feature + #[Test] public function itPrintsVersionAndExitsWithZero(): void { $app = new Application(['docbook-cs', '--version'], $this->stdout, $this->stderr); @@ -136,7 +136,7 @@ public function itPrintsVersionAndExitsWithZero(): void self::assertSame('', $this->readStream($this->stderr)); } - #[Test] // TODO: should be feature + #[Test] public function itReturnsErrorWhenConfigCannotBeLoaded(): void { $app = new Application( @@ -151,7 +151,7 @@ public function itReturnsErrorWhenConfigCannotBeLoaded(): void self::assertStringContainsString('Error:', $this->readStream($this->stderr)); } - #[Test] // TODO: should be feature + #[Test] public function itHandlesSeparateConfigArgument(): void { $app = new Application( @@ -166,7 +166,7 @@ public function itHandlesSeparateConfigArgument(): void self::assertStringContainsString('Error:', $this->readStream($this->stderr)); } - #[Test] // TODO: should be feature + #[Test] public function itAcceptsPathsWithoutCrashing(): void { $app = new Application( @@ -180,7 +180,7 @@ public function itAcceptsPathsWithoutCrashing(): void self::assertContains($exitCode, [0, 1, 2]); } - #[Test] // TODO: should be feature + #[Test] public function itSupportsQuietFlag(): void { $app = new Application(['docbook-cs', '--quiet'], $this->stdout, $this->stderr); @@ -190,7 +190,7 @@ public function itSupportsQuietFlag(): void self::assertContains($exitCode, [0, 1, 2]); } - #[Test] // TODO: should be feature + #[Test] public function itSupportsReportFormats(): void { foreach (['console', 'json', 'checkstyle'] as $format) { @@ -206,7 +206,7 @@ public function itSupportsReportFormats(): void } } - #[Test] // TODO: should be feature + #[Test] public function itSupportsColorFlags(): void { foreach (['--colors', '--no-colors'] as $flag) { @@ -222,7 +222,7 @@ public function itSupportsColorFlags(): void } } - #[Test] // TODO: should be feature + #[Test] public function helpShortCircuitsExecution(): void { $app = new Application( @@ -238,7 +238,7 @@ public function helpShortCircuitsExecution(): void self::assertSame('', $this->readStream($this->stderr)); } - #[Test] // TODO: should be feature + #[Test] public function versionShortCircuitsExecution(): void { $app = new Application( @@ -254,7 +254,7 @@ public function versionShortCircuitsExecution(): void self::assertSame('', $this->readStream($this->stderr)); } - #[Test] // TODO: should be feature + #[Test] public function itResolvesRelativeOverridePathsAgainstCwd(): void { $app = new Application( @@ -270,7 +270,7 @@ public function itResolvesRelativeOverridePathsAgainstCwd(): void self::assertNotSame(2, $exitCode); } - #[Test] // TODO: should be feature + #[Test] public function itCatchesRuntimeErrorFromRunner(): void { $app = new Application( @@ -285,7 +285,7 @@ public function itCatchesRuntimeErrorFromRunner(): void self::assertStringContainsString('Runtime error:', $this->readStream($this->stderr)); } - #[Test] // TODO: should be feature + #[Test] public function itSupportsSeparateReportArgument(): void { $app = new Application( @@ -299,7 +299,7 @@ public function itSupportsSeparateReportArgument(): void self::assertContains($exitCode, [0, 1, 2]); } - #[Test] // TODO: should be feature + #[Test] public function itPassesThroughAbsoluteOverridePaths(): void { $app = new Application( @@ -313,7 +313,7 @@ public function itPassesThroughAbsoluteOverridePaths(): void self::assertNotSame(2, $exitCode); } - #[Test] // TODO: should be feature + #[Test] public function itSuppressesProgressWhenQuietFlagIsSet(): void { $app = new Application( @@ -328,7 +328,7 @@ public function itSuppressesProgressWhenQuietFlagIsSet(): void self::assertSame('', $this->readStream($this->stderr)); } - #[Test] // TODO: should be feature + #[Test] public function itSuppressesProgressForStructuredReportFormats(): void { foreach (['json', 'checkstyle'] as $format) { @@ -351,7 +351,7 @@ public function itSuppressesProgressForStructuredReportFormats(): void } } - #[Test] // TODO: should be feature + #[Test] public function itShowsPerformanceWhenPerfFlagIsEnabled(): void { $app = new Application( @@ -374,7 +374,7 @@ public function itShowsPerformanceWhenPerfFlagIsEnabled(): void self::assertStringContainsString('PERFORMANCE', $output); } - #[Test] // TODO: should be feature + #[Test] public function itDoesNotShowPerformanceByDefault(): void { $app = new Application( diff --git a/tests/Unit/Runner/SniffRunnerTest.php b/tests/Integration/Runner/SniffRunnerTest.php similarity index 95% rename from tests/Unit/Runner/SniffRunnerTest.php rename to tests/Integration/Runner/SniffRunnerTest.php index 3e37cda..852691d 100644 --- a/tests/Unit/Runner/SniffRunnerTest.php +++ b/tests/Integration/Runner/SniffRunnerTest.php @@ -2,7 +2,7 @@ declare(strict_types=1); -namespace DocbookCS\Tests\Unit\Runner; +namespace DocbookCS\Tests\Integration\Runner; use DocbookCS\Config\ConfigData; use DocbookCS\Config\SniffEntry; @@ -86,7 +86,7 @@ private function createConfig(array $sniffs = []): ConfigData ); } - #[Test] // TODO: should be integration + #[Test] public function itProcessesFilesWithoutViolations(): void { $config = $this->createConfig(); @@ -99,7 +99,7 @@ public function itProcessesFilesWithoutViolations(): void self::assertCount(0, $report->getFileReports()); } - #[Test] // TODO: should be integration + #[Test] public function itUsesOverridePathsWhenProvided(): void { $config = $this->createConfig(); @@ -113,7 +113,7 @@ public function itUsesOverridePathsWhenProvided(): void self::assertSame(1, $report->getFilesScanned()); } - #[Test] // TODO: should be integration + #[Test] public function itCallsProgressMethods(): void { $progress = $this->createMock(ProgressInterface::class); @@ -134,7 +134,7 @@ public function itCallsProgressMethods(): void $runner->run($this->planPaths($config)); } - #[Test] // TODO: should be integration + #[Test] public function itAddsFileReportsForFilesWithViolations(): void { $sniff = new class (RunMode::Sniff) implements SniffInterface { @@ -177,7 +177,7 @@ public function setProperty(string $name, string $value): void self::assertTrue($report->hasViolations()); } - #[Test] // TODO: should be integration + #[Test] public function itStoresAbsolutePathsInFileReports(): void { $sniff = new class (RunMode::Sniff) implements SniffInterface { @@ -223,7 +223,7 @@ public function setProperty(string $name, string $value): void } } - #[Test] // TODO: should be integration + #[Test] public function itPassesPropertiesToSniffs(): void { $sniffClass = new class (RunMode::Sniff) implements SniffInterface { @@ -260,7 +260,7 @@ public function process(\DOMDocument $document, File $file): array self::assertSame(RunMode::Fix, $sniffClass::$capturedMode); } - #[Test] // TODO: should be integration + #[Test] public function itThrowsWhenSniffClassDoesNotExist(): void { $config = $this->createConfig(sniffs: [new SniffEntry('NonExistent\\FakeSniff')]); @@ -273,7 +273,7 @@ public function itThrowsWhenSniffClassDoesNotExist(): void $runner->run($this->planPaths($config)); } - #[Test] // TODO: should be integration + #[Test] public function itThrowsWhenClassDoesNotImplementSniffInterface(): void { $config = $this->createConfig(sniffs: [new SniffEntry(\stdClass::class)]); @@ -286,7 +286,7 @@ public function itThrowsWhenClassDoesNotImplementSniffInterface(): void $runner->run($this->planPaths($config)); } - #[Test] // TODO: should be integration + #[Test] public function itFiltersFilesToOnlyThoseInTheDiff(): void { $config = $this->createConfig(); @@ -298,7 +298,7 @@ public function itFiltersFilesToOnlyThoseInTheDiff(): void self::assertSame(1, $report->getFilesScanned()); } - #[Test] // TODO: should be integration + #[Test] public function itScansNoFilesWhenDiffContainsNoMatchingPaths(): void { $config = $this->createConfig(); @@ -310,7 +310,7 @@ public function itScansNoFilesWhenDiffContainsNoMatchingPaths(): void self::assertSame(0, $report->getFilesScanned()); } - #[Test] // TODO: should be integration + #[Test] public function itMatchesWhenDiffPathEqualsDiscoveredPath(): void { $config = $this->createConfig(); @@ -324,7 +324,7 @@ public function itMatchesWhenDiffPathEqualsDiscoveredPath(): void self::assertSame(1, $report->getFilesScanned()); } - #[Test] // TODO: should be integration + #[Test] public function itScansAllFilesWhenNoDiffIsGiven(): void { $config = $this->createConfig(); @@ -335,7 +335,7 @@ public function itScansAllFilesWhenNoDiffIsGiven(): void self::assertSame(2, $report->getFilesScanned()); } - #[Test] // TODO: should be integration + #[Test] public function itScansLexicallyEquivalentWideTargetsOnlyOnce(): void { $directory = sys_get_temp_dir() . '/docbook-cs-scan-' . bin2hex(random_bytes(6)); @@ -380,7 +380,7 @@ public function itScansLexicallyEquivalentWideTargetsOnlyOnce(): void } } - #[Test] // TODO: should be integration + #[Test] public function itReportsNoViolationsForFilesInDiffWithoutAddedLines(): void { $sniff = new class (RunMode::Sniff) implements SniffInterface { diff --git a/tests/Integration/Runner/XmlFileProcessorPipelineTest.php b/tests/Integration/Runner/XmlFileProcessorPipelineTest.php index e34a775..5a8b9f6 100644 --- a/tests/Integration/Runner/XmlFileProcessorPipelineTest.php +++ b/tests/Integration/Runner/XmlFileProcessorPipelineTest.php @@ -72,6 +72,52 @@ public function itAppliesFixesToTheOriginalSourceWhenEntitiesExpandBeforeTheViol } } + #[Test] + public function itHandlesEntitiesWithoutParseErrors(): void + { + $xml = $this->xml( + ' + + &link.superglobals; &php.ini; & + ' + ); + + $processor = $this->processor([], new EntityPreprocessor([ + 'link.superglobals' => '', + 'php.ini' => '', + ])); + + $report = $this->process($processor, $xml); + + self::assertCount( + 0, + array_filter( + $report->getViolations(), + fn($v) => $v->sniffCode === 'DocbookCS.Internal' + ) + ); + } + + #[Test] + public function itUsesCustomPreprocessor(): void + { + $processor = $this->processor([], new EntityPreprocessor([ + 'custom.entity' => '[X]', + ])); + + $xml = $this->xml('&custom.entity;'); + + $report = $this->process($processor, $xml); + + self::assertCount( + 0, + array_filter( + $report->getViolations(), + fn($v) => $v->sniffCode === 'DocbookCS.Internal' + ) + ); + } + private function process(XmlFileProcessor $processor, string $content, string $path = 'input.xml'): FileReport { return $processor->process(new File($path, $content))->fileReport; @@ -90,6 +136,14 @@ private function processFile(XmlFileProcessor $processor, string $path): FileRep return $result->fileReport; } + private function xml(string $body): string + { + return << +$body +XML; + } + /** @param list $sniffs */ private function processor(array $sniffs = [], ?EntityPreprocessor $pre = null): XmlFileProcessor { diff --git a/tests/Unit/Runner/XmlFileProcessorTest.php b/tests/Unit/Runner/XmlFileProcessorTest.php index 0eb1d7a..d609060 100644 --- a/tests/Unit/Runner/XmlFileProcessorTest.php +++ b/tests/Unit/Runner/XmlFileProcessorTest.php @@ -87,52 +87,6 @@ public function itAcceptsValidXmlWithoutViolations(): void self::assertFalse($report->hasViolations()); } - #[Test] // TODO: should be integration - public function itHandlesEntitiesWithoutParseErrors(): void - { - $xml = $this->xml( - ' - - &link.superglobals; &php.ini; & - ' - ); - - $processor = $this->processor([], new EntityPreprocessor([ - 'link.superglobals' => '', - 'php.ini' => '', - ])); - - $report = $this->process($processor, $xml); - - self::assertCount( - 0, - array_filter( - $report->getViolations(), - fn($v) => $v->sniffCode === 'DocbookCS.Internal' - ) - ); - } - - #[Test] // TODO: should be integration - public function itUsesCustomPreprocessor(): void - { - $processor = $this->processor([], new EntityPreprocessor([ - 'custom.entity' => '[X]', - ])); - - $xml = $this->xml('&custom.entity;'); - - $report = $this->process($processor, $xml); - - self::assertCount( - 0, - array_filter( - $report->getViolations(), - fn($v) => $v->sniffCode === 'DocbookCS.Internal' - ) - ); - } - #[Test] public function itReturnsZeroViolationsWithoutSniffs(): void { From 5b5815a5b04275dcd45dc7f66b9f7859200cef22 Mon Sep 17 00:00:00 2001 From: NickSdot Date: Tue, 21 Jul 2026 14:17:43 +0700 Subject: [PATCH 09/15] deps: updated composer dependencies --- composer.json | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/composer.json b/composer.json index ba5461a..db89730 100644 --- a/composer.json +++ b/composer.json @@ -11,13 +11,13 @@ "ext-simplexml": "*" }, "require-dev": { - "phpunit/phpunit": "^13.1", - "phpstan/phpstan": "^2.1", - "phpunit/php-code-coverage": "^14.1", - "squizlabs/php_codesniffer": "^4.0", - "phpstan/phpstan-strict-rules": "^2.0", - "phpstan/phpstan-phpunit": "^2.0", - "shipmonk/dead-code-detector": "^1.3" + "phpunit/phpunit": "^13.2.4", + "phpstan/phpstan": "^2.2.5", + "phpunit/php-code-coverage": "^14.2.3", + "squizlabs/php_codesniffer": "^4.0.1", + "phpstan/phpstan-strict-rules": "^2.0.12", + "phpstan/phpstan-phpunit": "^2.0.18", + "shipmonk/dead-code-detector": "^1.3.2" }, "autoload": { "psr-4": { From 04ebb8bc745229556055cf6071c8adc3911ad2d1 Mon Sep 17 00:00:00 2001 From: NickSdot Date: Tue, 21 Jul 2026 14:30:33 +0700 Subject: [PATCH 10/15] refactor: dropped getter methods --- src/Config/SniffEntry.php | 22 +++------------------- src/Runner/RunCoordinator.php | 4 ++-- tests/Unit/Config/ConfigParserTest.php | 6 +++--- 3 files changed, 8 insertions(+), 24 deletions(-) diff --git a/src/Config/SniffEntry.php b/src/Config/SniffEntry.php index 3dae442..2264e06 100644 --- a/src/Config/SniffEntry.php +++ b/src/Config/SniffEntry.php @@ -4,39 +4,23 @@ namespace DocbookCS\Config; -final class SniffEntry +final readonly class SniffEntry { - /** @var array */ - private array $properties; - /** * @param array $properties * @throws \InvalidArgumentException if $className is empty or only whitespace. */ public function __construct( - private readonly string $className, - array $properties = [], + public string $className, + public array $properties = [], ) { if (trim($className) === '') { throw new \InvalidArgumentException('Sniff class name must not be empty.'); } - - $this->properties = $properties; - } - - public function getClassName(): string - { - return $this->className; } public function getProperty(string $name, ?string $default = null): ?string { return $this->properties[$name] ?? $default; } - - /** @return array */ - public function getProperties(): array - { - return $this->properties; - } } diff --git a/src/Runner/RunCoordinator.php b/src/Runner/RunCoordinator.php index 699805d..a8fc437 100644 --- a/src/Runner/RunCoordinator.php +++ b/src/Runner/RunCoordinator.php @@ -95,7 +95,7 @@ private function instantiateSniffs(array $entries, RunMode $mode): array $sniffs = []; foreach ($entries as $entry) { - $className = $entry->getClassName(); + $className = $entry->className; if (!class_exists($className)) { throw new \RuntimeException( @@ -111,7 +111,7 @@ private function instantiateSniffs(array $entries, RunMode $mode): array ); } - foreach ($entry->getProperties() as $name => $value) { + foreach ($entry->properties as $name => $value) { $instance->setProperty($name, $value); } diff --git a/tests/Unit/Config/ConfigParserTest.php b/tests/Unit/Config/ConfigParserTest.php index ca490d2..1201e20 100644 --- a/tests/Unit/Config/ConfigParserTest.php +++ b/tests/Unit/Config/ConfigParserTest.php @@ -67,10 +67,10 @@ public function itParsesAFullConfigFromString(): void // Sniffs self::assertCount(2, $config->getSniffs()); - self::assertSame('App\Sniffs\ParaSniff', $config->getSniffs()[0]->getClassName()); - self::assertSame([], $config->getSniffs()[0]->getProperties()); + self::assertSame('App\Sniffs\ParaSniff', $config->getSniffs()[0]->className); + self::assertSame([], $config->getSniffs()[0]->properties); - self::assertSame('App\Sniffs\IdSniff', $config->getSniffs()[1]->getClassName()); + self::assertSame('App\Sniffs\IdSniff', $config->getSniffs()[1]->className); self::assertSame('^[a-z]+$', $config->getSniffs()[1]->getProperty('pattern')); // Include paths (resolved relative to basePath) From 5bdb8d254ed070a4ac6b136d80be85eebd7cc3a6 Mon Sep 17 00:00:00 2001 From: NickSdot Date: Tue, 21 Jul 2026 14:30:55 +0700 Subject: [PATCH 11/15] chore: exception formatting --- src/Config/ConfigParserException.php | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/Config/ConfigParserException.php b/src/Config/ConfigParserException.php index 46e92f7..75f087c 100644 --- a/src/Config/ConfigParserException.php +++ b/src/Config/ConfigParserException.php @@ -23,12 +23,6 @@ public static function missingElement(string $element): self public static function missingAttribute(string $element, string $attribute): self { - return new self( - sprintf( - 'Element <%s> is missing required attribute "%s".', - $element, - $attribute, - ) - ); + return new self(sprintf('Element <%s> is missing required attribute "%s".', $element, $attribute)); } } From 155b240c5fadb7baaf30ac2bb63cdb7dedf01293 Mon Sep 17 00:00:00 2001 From: NickSdot Date: Tue, 21 Jul 2026 14:31:14 +0700 Subject: [PATCH 12/15] chore: control flow formatting --- src/Diff/DiffChangeset.php | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/Diff/DiffChangeset.php b/src/Diff/DiffChangeset.php index c6f5319..fe04d62 100644 --- a/src/Diff/DiffChangeset.php +++ b/src/Diff/DiffChangeset.php @@ -18,10 +18,8 @@ public function changeFor(string $filePath): ?FileChange foreach ($this->fileChanges as $fileChange) { $normalisedDiffPath = str_replace('\\', '/', $fileChange->filePath); - if ( - $normalisedPath === $normalisedDiffPath - || str_ends_with($normalisedPath, '/' . ltrim($normalisedDiffPath, '/')) - ) { + $needle = '/' . ltrim($normalisedDiffPath, '/'); + if ($normalisedPath === $normalisedDiffPath || str_ends_with($normalisedPath, $needle)) { return $fileChange; } } From 35bf8a2bb9d296bf07635f20c2529a082bca634d Mon Sep 17 00:00:00 2001 From: NickSdot Date: Tue, 21 Jul 2026 14:33:40 +0700 Subject: [PATCH 13/15] fix: added types to make SA recognise usage --- src/Fix/FixApplier.php | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/src/Fix/FixApplier.php b/src/Fix/FixApplier.php index ebbf1d1..869a81c 100644 --- a/src/Fix/FixApplier.php +++ b/src/Fix/FixApplier.php @@ -19,20 +19,27 @@ public function apply(File $file, array $fixes): FixResult $plans = []; foreach ($fixes as $index => $fix) { - $plan = $fix instanceof FixPlan ? $fix : new FixPlan($fix); $plans[] = [ 'index' => $index, - 'plan' => $plan, + 'plan' => $fix instanceof FixPlan ? $fix : new FixPlan($fix), ]; } - usort($plans, static function (array $a, array $b): int { - $offsetComparison = $a['plan']->firstOffset() <=> $b['plan']->firstOffset(); - - return $offsetComparison !== 0 - ? $offsetComparison - : $a['index'] <=> $b['index']; - }); + usort( + /** @var list $plans */ + $plans, + /** + * @param array{index: int, plan: FixPlan} $a + * @param array{index: int, plan: FixPlan} $b + */ + static function (array $a, array $b): int { + $offsetComparison = $a['plan']->firstOffset() <=> $b['plan']->firstOffset(); + + return $offsetComparison !== 0 + ? $offsetComparison + : $a['index'] <=> $b['index']; + } + ); /** @var list $acceptedFixes */ $acceptedFixes = []; From 644ed0426465a07dc584f8b13fbbebfb91a5b529 Mon Sep 17 00:00:00 2001 From: NickSdot Date: Tue, 21 Jul 2026 14:34:38 +0700 Subject: [PATCH 14/15] style: nested functions to pipes --- src/Sniff/SimparaSniff.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Sniff/SimparaSniff.php b/src/Sniff/SimparaSniff.php index f41d164..73665b5 100644 --- a/src/Sniff/SimparaSniff.php +++ b/src/Sniff/SimparaSniff.php @@ -206,7 +206,9 @@ private function getAllowedElements(): array $additional = array_map('trim', explode(',', $extra)); $additional = array_filter($additional, static fn(string $s): bool => $s !== ''); - return array_values(array_unique(array_merge(self::SIMPARA_ALLOWED, $additional))); + return array_merge(self::SIMPARA_ALLOWED, $additional) + |> array_unique(...) + |> array_values(...); } /** From 20ffd416d0757b06185c40fa90f9a8f2aa827062 Mon Sep 17 00:00:00 2001 From: NickSdot Date: Tue, 21 Jul 2026 16:52:58 +0700 Subject: [PATCH 15/15] perf: optimised source range resolving --- src/Sniff/AbstractSniff.php | 31 +++++++++++++ src/Sniff/AttributeOrderSniff.php | 2 +- src/Sniff/ExceptionNameSniff.php | 28 +++++------- src/Sniff/SimparaSniff.php | 51 +++++++++------------ src/Source/File.php | 73 +++++++++++++++++++------------ tests/Unit/Source/FileTest.php | 20 ++++----- 6 files changed, 120 insertions(+), 85 deletions(-) diff --git a/src/Sniff/AbstractSniff.php b/src/Sniff/AbstractSniff.php index b1ccf90..8c10b6e 100644 --- a/src/Sniff/AbstractSniff.php +++ b/src/Sniff/AbstractSniff.php @@ -6,6 +6,7 @@ use DocbookCS\Runner\EntityExpansionMarker; use DocbookCS\Runner\RunMode; +use DocbookCS\Source\File; use DocbookCS\Violation\Severity; use DocbookCS\Violation\SourceRange; use DocbookCS\Violation\Violation; @@ -41,6 +42,36 @@ protected function isSourceBacked(\DOMNode $node): bool return !EntityExpansionMarker::contains($node); } + /** + * The offsets point at the opening "<" and closing "<" in the source. + * + * @return array{SourceRange, SourceRange} + * @throws \OutOfBoundsException if a tag offset lies outside the source + */ + protected function elementNameRanges( + File $file, + int $openingTagOffset, + int $closingTagOffset, + string $elementName, + ): array { + $openingNameOffset = $openingTagOffset + 1; + $closingNameOffset = $closingTagOffset + 2; + $elementNameLength = strlen($elementName); + + return [ + new SourceRange( + $file->lineNumberAtOffset($openingNameOffset), + $openingNameOffset, + $openingNameOffset + $elementNameLength, + ), + new SourceRange( + $file->lineNumberAtOffset($closingNameOffset), + $closingNameOffset, + $closingNameOffset + $elementNameLength, + ), + ]; + } + /** * @param list $affectedRanges * @throws \LogicException if an invalid severity level is configured diff --git a/src/Sniff/AttributeOrderSniff.php b/src/Sniff/AttributeOrderSniff.php index 119824c..80050bf 100644 --- a/src/Sniff/AttributeOrderSniff.php +++ b/src/Sniff/AttributeOrderSniff.php @@ -63,7 +63,7 @@ public function process(\DOMDocument $document, File $file): array $tagName, $attrString, $file->path, - $file->lineAtOffset($beginOffset)->number, + $file->lineNumberAtOffset($beginOffset), $beginOffset, $beginOffset + strlen($fullMatch), $violations, diff --git a/src/Sniff/ExceptionNameSniff.php b/src/Sniff/ExceptionNameSniff.php index 49c3cc4..f6c9fef 100644 --- a/src/Sniff/ExceptionNameSniff.php +++ b/src/Sniff/ExceptionNameSniff.php @@ -6,7 +6,6 @@ use DocbookCS\Fix\Fixer\ExceptionNameFixer; use DocbookCS\Source\File; -use DocbookCS\Violation\SourceRange; /** * Detects exception/error class names wrapped in that @@ -84,14 +83,21 @@ public function process(\DOMDocument $document, File $file): array throw new \LogicException('Could not map classname violation to source content.'); } + $affectedRanges = $this->elementNameRanges( + $file, + $match['beginOffset'], + $match['closingOffset'], + self::ELEMENT_NAME, + ); + $violations[] = $this->createViolation( $file->path, - $match['affectedRanges'][0]->line, + $affectedRanges[0]->line, $match['beginOffset'], $match['untilOffset'], sprintf('"%s" is wrapped in but should use .', $text), $match['content'], - affectedRanges: $match['affectedRanges'], + affectedRanges: $affectedRanges, ); } @@ -112,9 +118,8 @@ public static function looksLikeException(string $text): bool * untilOffset: int, * content: string, * text: string, - * affectedRanges: non-empty-list + * closingOffset: int * }> - * @throws \OutOfBoundsException if a matched tag offset lies outside the source */ private function sourceMatches(File $file): array { @@ -134,18 +139,7 @@ private function sourceMatches(File $file): array 'untilOffset' => $offset + strlen($fullMatch), 'content' => $fullMatch, 'text' => trim($matches[1][$i][0]), - 'affectedRanges' => [ - new SourceRange( - $file->lineAtOffset($offset)->number, - $offset + 1, - $offset + 1 + strlen(self::ELEMENT_NAME), - ), - new SourceRange( - $file->lineAtOffset($closingOffset)->number, - $closingOffset + 2, - $closingOffset + 2 + strlen(self::ELEMENT_NAME), - ), - ], + 'closingOffset' => $closingOffset, ]; } diff --git a/src/Sniff/SimparaSniff.php b/src/Sniff/SimparaSniff.php index 73665b5..978ea32 100644 --- a/src/Sniff/SimparaSniff.php +++ b/src/Sniff/SimparaSniff.php @@ -6,7 +6,6 @@ use DocbookCS\Fix\Fixer\SimparaFixer; use DocbookCS\Source\File; -use DocbookCS\Violation\SourceRange; final class SimparaSniff extends AbstractSniff implements Fixable { @@ -160,14 +159,26 @@ public function process(\DOMDocument $document, File $file): array continue; } + $closingOffset = $match['closingOffset']; + if ($closingOffset === null) { + throw new \LogicException('Could not map simpara violation to source content.'); + } + + $affectedRanges = $this->elementNameRanges( + $file, + $match['beginOffset'], + $closingOffset, + self::ELEMENT_NAME, + ); + $violations[] = $this->createViolation( $file->path, - $match['affectedRanges'][0]->line, + $affectedRanges[0]->line, $match['beginOffset'], $match['untilOffset'], self::MESSAGE, $match['content'], - affectedRanges: $match['affectedRanges'], + affectedRanges: $affectedRanges, ); } @@ -217,9 +228,8 @@ private function getAllowedElements(): array * untilOffset: int, * content: string, * selfClosing: bool, - * affectedRanges: non-empty-list + * closingOffset: int|null * }> - * @throws \OutOfBoundsException if a matched tag offset lies outside the source */ private function sourceMatches(File $file): array { @@ -230,7 +240,7 @@ private function sourceMatches(File $file): array PREG_OFFSET_CAPTURE, ); - /** @var list $stack */ + /** @var list $stack */ $stack = []; $sourceMatches = []; @@ -243,24 +253,13 @@ private function sourceMatches(File $file): array 'untilOffset' => $offset + strlen($tag), 'content' => $tag, 'selfClosing' => true, - 'affectedRanges' => [new SourceRange( - $file->lineAtOffset($offset)->number, - $offset + 1, - $offset + 1 + strlen(self::ELEMENT_NAME), - )], + 'closingOffset' => null, ]; continue; } if (!str_starts_with($tag, ' $offset, - 'range' => new SourceRange( - $file->lineAtOffset($offset)->number, - $offset + 1, - $offset + 1 + strlen(self::ELEMENT_NAME), - ), - ]; + $stack[] = $offset; continue; } @@ -268,21 +267,13 @@ private function sourceMatches(File $file): array continue; } - $start = $opening['offset']; $untilOffset = $offset + strlen($tag); $sourceMatches[] = [ - 'beginOffset' => $start, + 'beginOffset' => $opening, 'untilOffset' => $untilOffset, - 'content' => substr($file->content, (int)$start, $untilOffset - $start), + 'content' => substr($file->content, $opening, $untilOffset - $opening), 'selfClosing' => false, - 'affectedRanges' => [ - $opening['range'], - new SourceRange( - $file->lineAtOffset($offset)->number, - $offset + 2, - $offset + 2 + strlen(self::ELEMENT_NAME), - ), - ], + 'closingOffset' => $offset, ]; } diff --git a/src/Source/File.php b/src/Source/File.php index 9f66a32..02f8be5 100644 --- a/src/Source/File.php +++ b/src/Source/File.php @@ -36,30 +36,9 @@ public function lines(): \Generator } /** @throws \OutOfBoundsException if the offset lies outside the source */ - public function lineAtOffset(int $offset): Line + public function lineNumberAtOffset(int $offset): int { - $sourceLength = strlen($this->content); - if ($offset < 0 || $offset > $sourceLength) { - throw new \OutOfBoundsException( - sprintf('Source offset %d is outside the valid range 0..%d.', $offset, $sourceLength), - ); - } - - $lineBeginOffsets = $this->lineBeginOffsets(); - $low = 0; - $high = count($lineBeginOffsets) - 1; - - while ($low < $high) { - $middle = intdiv($low + $high + 1, 2); - - if ($lineBeginOffsets[$middle] <= $offset) { - $low = $middle; - } else { - $high = $middle - 1; - } - } - - return $this->createLineAtOffset($low + 1, $lineBeginOffsets[$low]); + return $this->lineIndexAtOffset($offset, $this->lineBeginOffsets()) + 1; } public function withContent(string $content): self @@ -77,18 +56,58 @@ private function lineBeginOffsets(): array } $lineBeginOffsets = [0]; + $sourceLength = strlen($this->content); + $lineBeginOffset = 0; + + while ($lineBeginOffset < $sourceLength) { + $lineLength = strcspn($this->content, "\r\n", $lineBeginOffset); + $lineEndingOffset = $lineBeginOffset + $lineLength; - foreach ($this->lines() as $line) { - if ($line->number === 1) { - continue; + if ($lineEndingOffset === $sourceLength) { + break; } - $lineBeginOffsets[] = $line->beginOffset; + $lineBeginOffset = $lineEndingOffset + ( + $this->content[$lineEndingOffset] === "\r" + && ($this->content[$lineEndingOffset + 1] ?? null) === "\n" + ? 2 + : 1 + ); + $lineBeginOffsets[] = $lineBeginOffset; } return $this->lineBeginOffsets = $lineBeginOffsets; } + /** + * @param non-empty-list $lineBeginOffsets + * @throws \OutOfBoundsException if the offset lies outside the source + */ + private function lineIndexAtOffset(int $offset, array $lineBeginOffsets): int + { + $sourceLength = strlen($this->content); + if ($offset < 0 || $offset > $sourceLength) { + throw new \OutOfBoundsException( + sprintf('Source offset %d is outside the valid range 0..%d.', $offset, $sourceLength), + ); + } + + $low = 0; + $high = count($lineBeginOffsets) - 1; + + while ($low < $high) { + $middle = intdiv($low + $high + 1, 2); + + if ($lineBeginOffsets[$middle] <= $offset) { + $low = $middle; + } else { + $high = $middle - 1; + } + } + + return $low; + } + private function createLineAtOffset(int $lineNumber, int $lineBeginOffset): Line { $sourceLength = strlen($this->content); diff --git a/tests/Unit/Source/FileTest.php b/tests/Unit/Source/FileTest.php index ad7a524..7cca8e3 100644 --- a/tests/Unit/Source/FileTest.php +++ b/tests/Unit/Source/FileTest.php @@ -39,17 +39,17 @@ public function itRepresentsLinesAndTheirEndings(): void } #[Test] - public function itResolvesOffsetsToLines(): void + public function itResolvesOffsetsToLineNumbers(): void { $file = new File('file.xml', "one\r\ntwo\nthree"); - self::assertSame(1, $file->lineAtOffset(0)->number); - self::assertSame(1, $file->lineAtOffset(3)->number); - self::assertSame(1, $file->lineAtOffset(4)->number); - self::assertSame(2, $file->lineAtOffset(5)->number); - self::assertSame(2, $file->lineAtOffset(8)->number); - self::assertSame(3, $file->lineAtOffset(9)->number); - self::assertSame(3, $file->lineAtOffset(14)->number); + self::assertSame(1, $file->lineNumberAtOffset(0)); + self::assertSame(1, $file->lineNumberAtOffset(3)); + self::assertSame(1, $file->lineNumberAtOffset(4)); + self::assertSame(2, $file->lineNumberAtOffset(5)); + self::assertSame(2, $file->lineNumberAtOffset(8)); + self::assertSame(3, $file->lineNumberAtOffset(9)); + self::assertSame(3, $file->lineNumberAtOffset(14)); } #[Test] @@ -62,7 +62,7 @@ public function itIncludesAnEmptyLineAfterTheFinalLineEnding(): void new Line(2, '', '', 4), ], iterator_to_array($file->lines())); - self::assertSame(2, $file->lineAtOffset(4)->number); + self::assertSame(2, $file->lineNumberAtOffset(4)); } #[Test] @@ -91,6 +91,6 @@ public function itRejectsOffsetsOutsideTheSource(): void $file = new File('file.xml', 'one'); $this->expectException(\OutOfBoundsException::class); - $file->lineAtOffset(4); + $file->lineNumberAtOffset(4); } }