diff --git a/docs/available-rules.md b/docs/available-rules.md index 91dfcd9f..7d547c38 100644 --- a/docs/available-rules.md +++ b/docs/available-rules.md @@ -76,6 +76,8 @@ Namespace: `Boundwize\StructArmed\Rule\Rules\Class_`. | Rule | Constructor | Checks | |---|---|---| | `AnonymousClassMayNotHaveEmptyParenthesesRule` | `new AnonymousClassMayNotHaveEmptyParenthesesRule(layer: 'Source')` | Anonymous classes that pass no constructor argument omit the parentheses after `class` (`new class {}`, not `new class () {}`), per [PER Coding Style](https://www.php-fig.org/per/coding-style/#8-anonymous-classes). Supports `--fix` by removing the empty parentheses. | +| `ApiMayNotBeInternalRule` | `new ApiMayNotBeInternalRule(layer: 'Source')` | Class-likes in a layer are not tagged both `@api` and `@internal`, which contradict each other. | +| `ApiMayNotExtendInternalRule` | `new ApiMayNotExtendInternalRule(layer: 'Source')` | Classes and interfaces tagged `@api` in a layer do not extend a scanned class or interface tagged `@internal`, directly or through any parent, which would expose its members through the public API. Unscanned parents are skipped, as their docblock is unknown. | | `ClassConstantNameMustBeUpperCaseRule` | `new ClassConstantNameMustBeUpperCaseRule(layer: 'Domain')` | Class, interface, and trait constants use upper case with underscore separators. Enums are skipped (PER Coding Style recommends PascalCase enum constants). | | `ClassImplementingInterfaceMustHaveSuffixRule` | `new ClassImplementingInterfaceMustHaveSuffixRule(layer: 'HTTP', interface: MiddlewareInterface::class, suffix: 'Middleware')` | Classes implementing a specific interface use the required suffix. | | `ClassNameMustBeStudlyCapsRule` | `new ClassNameMustBeStudlyCapsRule(layer: 'Source')` | Class names use StudlyCaps. | diff --git a/docs/cache.md b/docs/cache.md index cb988480..b0c64b53 100644 --- a/docs/cache.md +++ b/docs/cache.md @@ -53,3 +53,16 @@ You can also clear cache directly: ```bash vendor/bin/structarmed --clear-cache ``` + +## Disabling Cache + +Use `--no-cache` to run analysis without caching. StructArmed skips file hashing and neither reads nor writes cache entries. Skipping this work can make analysis faster on very huge codebases. + +```bash +vendor/bin/structarmed analyse --no-cache +``` + +{: .important } +Keep in mind that every run with `--no-cache` repeats the full analysis, so similar runs will take roughly the same amount of time. With caching enabled, later runs can reuse cached results and finish faster. + +The existing cache is left untouched. `--no-cache` cannot be combined with `--clear-cache`; clear the cache separately with `vendor/bin/structarmed --clear-cache`. diff --git a/docs/cli.md b/docs/cli.md index e32fed27..e0561b93 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -27,6 +27,7 @@ vendor/bin/structarmed init --preset=mvc vendor/bin/structarmed init --preset=ddd vendor/bin/structarmed init --preset=yagni vendor/bin/structarmed init --preset=codequality +vendor/bin/structarmed init --preset=stability vendor/bin/structarmed init --preset=all ``` diff --git a/docs/configuration.md b/docs/configuration.md index 5d95f6c3..eaa3af50 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -79,6 +79,35 @@ Use `replaceRule()` to swap a preset rule's configuration: `replaceRule()` throws `RuleNotFoundException` if the target key does not exist, so typos are caught immediately. +## Importing Config Files + +Use `import()` to split a large config into per-module files: + +```php +import(__DIR__ . '/module/Foo/structarmed.php') + ->import(__DIR__ . '/module/Bar/structarmed.php'); +``` + +Each imported file returns a callable that receives the root `Architecture`: + +```php +layer('Foo', 'module/Foo/src/'); +}; +``` + +Paths in imported files are still resolved from the project root. Imported files can import other files. Changing any imported file invalidates the analysis cache, just like changing the main config. + ## Custom Extensions Use [Custom Rules And Presets](../custom-rules-and-presets/) when you want to add project-specific rules or package reusable rule sets. diff --git a/docs/custom-rules-and-presets.md b/docs/custom-rules-and-presets.md index 13a1ff44..0c6ffac7 100644 --- a/docs/custom-rules-and-presets.md +++ b/docs/custom-rules-and-presets.md @@ -121,6 +121,54 @@ return Architecture::define() ); ``` +## Reading @api And @internal Tags In A Custom Rule + +Each `ClassNode` records whether the docblock of its class, interface, trait, or enum declaration carries an `@api` or `@internal` tag: + +| Flag | Meaning | +| --- | --- | +| `$classNode->isApi` | The class-like docblock has an `@api` tag | +| `$classNode->isInternal` | The class-like docblock has an `@internal` tag | + +A tag counts when it starts a docblock line, also when a description follows, such as `@internal only used by the analyser`. A tag name mentioned within the description does not count, and neither do longer tag names such as `@apiVersion` and `@api-version` or the phpDocumentor inline `{@internal ...}}` form, which marks part of the description as internal rather than the class-like itself. Member docblocks are not read. + +The built-in `ApiMayNotBeInternalRule` and `ApiMayNotExtendInternalRule` read both flags. The example below enforces that every class-like in a layer declares one of the two tags: + +```php +isInLayer('Core'); + } + + public function evaluate(ClassNode $classNode): ?RuleViolation + { + if ($classNode->isApi || $classNode->isInternal) { + return null; + } + + return new RuleViolation( + message: sprintf('%s [%s] must be annotated @api or @internal', $classNode->getType(), $classNode->className), + file: $classNode->file, + line: $classNode->line, + className: $classNode->className, + layer: $classNode->layer, + ); + } +} +``` + ## Reading Usage Flags In A Custom Rule Usage flags are opt-in because collecting them costs extra analysis time. Before reading a usage flag, a custom rule must declare that it needs the data by implementing the corresponding marker interface instead of the plain `Boundwize\StructArmed\Rule\RuleInterface`. Each marker extends `RuleInterface`, so no other change is needed: @@ -205,7 +253,7 @@ Trade-off: only usage within the scanned paths is known. A class-like used solel ## Reading Other Classes' Layers In A Custom Rule -A rule only receives the node it is evaluating. When a check depends on the layer of *another* class — the layer a dependency belongs to, for instance — extend `Boundwize\StructArmed\Rule\AbstractLayerAwareRule`. Before any class is evaluated, the analyser injects the map of every scanned class into its protected `$classNodeMap` property, keyed by fully qualified class name, and `getDependencyNode()` looks a class up in it: +A rule only receives the node it is evaluating. When a check depends on the layer of *another* class — the layer a dependency belongs to, for instance — extend `Boundwize\StructArmed\Rule\AbstractLayerAwareRule`. Before any class is evaluated, the analyser injects the map of every scanned class into its protected `$classNodeMap` property, keyed by fully qualified class name, and `getDependencyNode()` looks a class up in it, ignoring case as PHP does for class names: ```php scalarType instan : null, nonClassDependencies: $analysis['nonClassDependencies'], constantFetches: $analysis['constantFetches'], + isApi: $this->hasDocBlockTag($classLike, 'api'), + isInternal: $this->hasDocBlockTag($classLike, 'internal'), ); } @@ -1829,6 +1832,20 @@ private function resolveClassName(ClassLike $classLike): string : (string) $classLike->name; } + /** + * Whether the docblock carries the $tag tag, e.g. `@api`. A tag starts a + * docblock line, so the tag name mentioned in the description, a longer + * tag such as `@api-version`, or an inline `{@internal ...}` comment does + * not count. + */ + private function hasDocBlockTag(ClassLike $classLike, string $tag): bool + { + $docComment = $classLike->getDocComment(); + + return $docComment instanceof Doc + && preg_match('#^[/*\s]*@' . $tag . '(?![\w-])#m', $docComment->getText()) === 1; + } + /** * @return array{ * dependencies: list, diff --git a/src/Analyser/ClassNode.php b/src/Analyser/ClassNode.php index 2a1ca7b7..67a81c97 100644 --- a/src/Analyser/ClassNode.php +++ b/src/Analyser/ClassNode.php @@ -41,6 +41,8 @@ final class ClassNode * @param string|null $enumBackingType Backing type for a backed enum, null otherwise * @param list $nonClassDependencies Dependencies only ever used as a function or constant name * @param list $constantFetches Global and namespaced constants fetched within this class + * @param bool $isApi Whether the docblock marks it as @api + * @param bool $isInternal Whether the docblock marks it as @internal */ public function __construct( public readonly string $className, @@ -75,6 +77,8 @@ public function __construct( public readonly ?string $enumBackingType = null, public readonly array $nonClassDependencies = [], public readonly array $constantFetches = [], + public readonly bool $isApi = false, + public readonly bool $isInternal = false, ) { $this->layers = $layers ?: array_filter([$this->layer]); } diff --git a/src/Analyser/Parallel/ParallelAnalysisNodeExtractor.php b/src/Analyser/Parallel/ParallelAnalysisNodeExtractor.php index 9301967d..2b0e9432 100644 --- a/src/Analyser/Parallel/ParallelAnalysisNodeExtractor.php +++ b/src/Analyser/Parallel/ParallelAnalysisNodeExtractor.php @@ -39,6 +39,7 @@ use function min; use function mkdir; use function proc_close; +use function rmdir; use function serialize; use function sprintf; use function stream_set_blocking; @@ -334,6 +335,13 @@ public function extract( } } + // when --no-cache is used, the directory only held worker temporary files: + // remove it best-effort, rmdir() refuses a non-empty directory atomically, + // so a concurrent run's files or an existing cache keep it in place + if (! $this->analysisResultCache instanceof AnalysisResultCache) { + @rmdir($cacheDirectory); + } + if ($failures !== []) { throw new RuntimeException(implode("\n", $failures)); } diff --git a/src/Architecture.php b/src/Architecture.php index 14d3b1fd..267bec8b 100644 --- a/src/Architecture.php +++ b/src/Architecture.php @@ -19,7 +19,9 @@ use function array_merge; use function array_unique; use function array_values; +use function is_callable; use function is_int; +use function realpath; use function sprintf; /** @@ -112,6 +114,12 @@ final class Architecture private ?string $baseline = null; + /** @var list config files applied through import(), nested imports included */ + private array $importedFiles = []; + + /** @var array config files whose import() is still running */ + private array $importing = []; + private function __construct() { } @@ -121,6 +129,53 @@ public static function define(): self return new self(); } + /** + * Apply another config file, which returns a callable that receives this + * Architecture: + * + * return Architecture::define() + * ->import(__DIR__ . '/module/Foo/structarmed.php') + * ->import(__DIR__ . '/module/Bar/structarmed.php'); + * + * // module/Foo/structarmed.php + * return static function (Architecture $architecture): void { + * $architecture->layer('Foo', 'module/Foo/src/'); + * }; + * + * Imported files are part of the config cache key, so changing one + * invalidates the analysis cache. + */ + public function import(string $file): self + { + $importedFile = realpath($file) ?: $file; + + // only imports still in progress are tracked, so a file imported twice + // through separate branches is applied again rather than rejected + if (isset($this->importing[$importedFile])) { + throw new InvalidArgumentException(sprintf('Circular import of configuration [%s].', $file)); + } + + $configure = require $file; + + if (! is_callable($configure)) { + throw new InvalidArgumentException( + sprintf('Imported configuration [%s] must return a callable.', $file) + ); + } + + $this->importing[$importedFile] = true; + + try { + $configure($this); + } finally { + unset($this->importing[$importedFile]); + } + + $this->importedFiles[] = $importedFile; + + return $this; + } + // ------------------------------------------------------------------------- // Layer definition // ------------------------------------------------------------------------- @@ -492,6 +547,12 @@ public function getBaseline(): ?string return $this->baseline; } + /** @return list */ + public function getImportedFiles(): array + { + return $this->importedFiles; + } + private function registerPendingSkip(string $skip): void { if (isset($this->rules[$skip])) { diff --git a/src/Cache/AnalysisCacheMetadataFactory.php b/src/Cache/AnalysisCacheMetadataFactory.php index a76833c8..5f7a04a0 100644 --- a/src/Cache/AnalysisCacheMetadataFactory.php +++ b/src/Cache/AnalysisCacheMetadataFactory.php @@ -68,6 +68,17 @@ public function fileHash(string $path): string return $this->fileHashProvider->hash($path); } + /** + * Files merged through Architecture::import() shape the config as much as + * the config file itself, so a change to any of them must change the hash. + * + * @param list $importedFiles + */ + public function configHash(string $configPath, array $importedFiles): string + { + return $this->filesHash([$configPath, ...$importedFiles]); + } + /** * Cached analysis nodes store resolved layer assignments, which depend on the * composer.json PSR-4 mappings as well as the config, so both hashes must diff --git a/src/Cache/AnalysisResultCache.php b/src/Cache/AnalysisResultCache.php index 3b1637b0..6578bd94 100644 --- a/src/Cache/AnalysisResultCache.php +++ b/src/Cache/AnalysisResultCache.php @@ -17,7 +17,6 @@ use Boundwize\StructArmed\Composer\ComposerJsonProvider; use Boundwize\StructArmed\Rule\RuleViolation; use Boundwize\StructArmed\Rule\RuleViolationCollection; -use FilesystemIterator; use function array_fill_keys; use function array_is_list; @@ -69,7 +68,7 @@ final class AnalysisResultCache * their shape, naming, or reference semantics change. It is recorded in * the metadata marker, so older caches are cleared on their next use. */ - public const FORMAT_VERSION = 17; + public const FORMAT_VERSION = 18; private readonly string $cacheDirectory; @@ -177,11 +176,9 @@ public function clear(): void } } - // only remove the cache directory if it is empty - $fileSystemIterator = new FilesystemIterator($this->cacheDirectory); - if (! $fileSystemIterator->valid()) { - rmdir($this->cacheDirectory); - } + // only remove the cache directory if it is empty: rmdir() refuses a + // non-empty directory atomically, so a concurrent run's files keep it + @rmdir($this->cacheDirectory); } public function getCacheDirectory(): string @@ -976,8 +973,8 @@ private function functionLikeBodyFromArray(array $node, string $file): ?array /** * The file is not stored: the payload belongs to one file, known when - * loading. Empty lists — most of a typical class's — are left out and - * default on load. + * loading. Empty lists — most of a typical class's — and the rarely set + * isApi and isInternal flags are left out and default on load. * * @return array */ @@ -997,6 +994,14 @@ private function classNodeToArray(ClassNode $classNode): array 'enumBackingType' => $classNode->enumBackingType, ]; + if ($classNode->isApi) { + $node['isApi'] = true; + } + + if ($classNode->isInternal) { + $node['isInternal'] = true; + } + $lists = [ 'dependencies' => $classNode->dependencies, 'nonClassDependencies' => $classNode->nonClassDependencies, @@ -1040,6 +1045,8 @@ private function classNodeFromArray(array $node, string $file): ?ClassNode $isTrait = $node['isTrait'] ?? null; $isEnum = $node['isEnum'] ?? null; $isReadonly = $node['isReadonly'] ?? null; + $isApi = $node['isApi'] ?? false; + $isInternal = $node['isInternal'] ?? false; $dependencies = $node['dependencies'] ?? []; $nonClassDependencies = $node['nonClassDependencies'] ?? []; $constantFetches = $node['constantFetches'] ?? []; @@ -1065,6 +1072,8 @@ private function classNodeFromArray(array $node, string $file): ?ClassNode || ! is_bool($isTrait) || ! is_bool($isEnum) || ! is_bool($isReadonly) + || ! is_bool($isApi) + || ! is_bool($isInternal) || ! $this->isStringArray($dependencies) || ! $this->isStringArray($nonClassDependencies) || ! $this->isStringArray($constantFetches) @@ -1125,6 +1134,8 @@ enumCases: $enumCases, enumBackingType: $enumBackingType, nonClassDependencies: array_values($nonClassDependencies), constantFetches: array_values($constantFetches), + isApi: $isApi, + isInternal: $isInternal, ); } diff --git a/src/Cli/AnalyseCommand.php b/src/Cli/AnalyseCommand.php index 385118a4..c2290c00 100644 --- a/src/Cli/AnalyseCommand.php +++ b/src/Cli/AnalyseCommand.php @@ -45,6 +45,7 @@ * generate-baseline?: string, * no-progress?: true, * clear-cache?: true, + * no-cache?: true, * disable-parallel?: true, * fix?: true * } @@ -62,6 +63,7 @@ private const FLAG_OPTIONS = [ '--no-progress' => 'no-progress', '--clear-cache' => 'clear-cache', + '--no-cache' => 'no-cache', '--disable-parallel' => 'disable-parallel', '--fix' => 'fix', ]; @@ -100,6 +102,12 @@ public function run(array $arguments, string $basePath): int return 1; } + if (isset($options['no-cache'], $options['clear-cache'])) { + echo "Error: --no-cache and --clear-cache cannot be used together.\n"; + + return 1; + } + $workingDirectory = $basePath; if (isset($options['basepath'])) { @@ -146,34 +154,54 @@ public function run(array $arguments, string $basePath): int $start = microtime(true); $fileHashProvider = new FileHashProvider(); $analysisCacheMetadataFactory = new AnalysisCacheMetadataFactory($fileHashProvider); - $configHash = $analysisCacheMetadataFactory->fileHash($configFile); - $composerGeneratedVersionHash = $analysisCacheMetadataFactory->composerGeneratedVersionHash(); - $analysisResultCache = new AnalysisResultCache( - $basePath, - $fileHashProvider, - $architecture->getCacheDirectory(), - $configHash, - $composerGeneratedVersionHash - ); - $analysisNodeCacheNamespace = $analysisCacheMetadataFactory->analysisNodeCacheNamespace( - $basePath, - $configHash - ); - $analyser = new Analyser($basePath, $analysisResultCache, $analysisNodeCacheNamespace); - - if (isset($options['clear-cache']) || $analysisResultCache->shouldInvalidate()) { - $analysisResultCache->clear(); + $analysisResultCache = null; + $analysisNodeCacheNamespace = ''; + + // Without the cache, no file is hashed and no cache entry is read or + // written, which saves that work on a one-off run over a huge codebase. + if (! isset($options['no-cache'])) { + $configHash = $analysisCacheMetadataFactory->configHash( + $configFile, + $architecture->getImportedFiles() + ); + $analysisResultCache = new AnalysisResultCache( + $basePath, + $fileHashProvider, + $architecture->getCacheDirectory(), + $configHash, + $analysisCacheMetadataFactory->composerGeneratedVersionHash() + ); + $analysisNodeCacheNamespace = $analysisCacheMetadataFactory->analysisNodeCacheNamespace( + $basePath, + $configHash + ); + + if (isset($options['clear-cache']) || $analysisResultCache->shouldInvalidate()) { + $analysisResultCache->clear(); + } } + $analyser = new Analyser($basePath, $analysisResultCache, $analysisNodeCacheNamespace); $files = $analyser->filesForAnalysis($architecture, $scanPaths); - $metadata = $analysisCacheMetadataFactory->metadata($basePath, $configFile, $scanPaths, $files); - $cacheKey = $analysisCacheMetadataFactory->key($metadata); $progress = $reportType === 'console' && ! isset($options['no-progress']) ? $this->progressHandler ?? new ConsoleProgressBar() : null; $analyserOptions = isset($options['disable-parallel']) ? AnalyserOptions::sequential() : null; + $metadata = []; + $cacheKey = ''; - $ruleViolationCollection = $analysisResultCache->load($cacheKey, $metadata); + $ruleViolationCollection = null; + + if ($analysisResultCache instanceof AnalysisResultCache) { + $metadata = $analysisCacheMetadataFactory->metadata( + $basePath, + $configFile, + $scanPaths, + $files + ); + $cacheKey = $analysisCacheMetadataFactory->key($metadata); + $ruleViolationCollection = $analysisResultCache->load($cacheKey, $metadata); + } if (! $ruleViolationCollection instanceof RuleViolationCollection) { $ruleViolationCollection = $analyser->analyse( @@ -183,7 +211,7 @@ public function run(array $arguments, string $basePath): int $analyserOptions, $files ); - $analysisResultCache->store($cacheKey, $metadata, $ruleViolationCollection); + $analysisResultCache?->store($cacheKey, $metadata, $ruleViolationCollection); } $elapsed = microtime(true) - $start; @@ -215,16 +243,9 @@ public function run(array $arguments, string $basePath): int } $violationCountBeforePass = $ruleViolationCollection->count(); - $analysisResultCache->clear(); + $analysisResultCache?->clear(); $files = $analyser->filesForAnalysis($architecture, $scanPaths); - $metadata = $analysisCacheMetadataFactory->metadata( - $basePath, - $configFile, - $scanPaths, - $files - ); - $cacheKey = $analysisCacheMetadataFactory->key($metadata); $unfilteredRuleViolationCollection = $analyser->analyse( $architecture, $scanPaths, @@ -232,7 +253,20 @@ public function run(array $arguments, string $basePath): int $analyserOptions, $files ); - $analysisResultCache->store($cacheKey, $metadata, $unfilteredRuleViolationCollection); + + if ($analysisResultCache instanceof AnalysisResultCache) { + $metadata = $analysisCacheMetadataFactory->metadata( + $basePath, + $configFile, + $scanPaths, + $files + ); + $analysisResultCache->store( + $analysisCacheMetadataFactory->key($metadata), + $metadata, + $unfilteredRuleViolationCollection + ); + } try { $ruleViolationCollection = $this->resolveRuleViolationCollection( diff --git a/src/Cli/InitCommand.php b/src/Cli/InitCommand.php index ea3b27da..8853a1ca 100644 --- a/src/Cli/InitCommand.php +++ b/src/Cli/InitCommand.php @@ -90,6 +90,7 @@ private function presetConfig(string $preset): ?string 'psr15' => ' ->withPreset(Preset::PSR15());', 'yagni' => ' ->withPreset(Preset::YAGNI());', 'codequality' => ' ->withPreset(Preset::CODEQUALITY());', + 'stability' => ' ->withPreset(Preset::STABILITY());', 'all' => " ->withPresets(\n" . " Preset::PSR4(),\n" . " Preset::PSR1(),\n" @@ -99,7 +100,8 @@ private function presetConfig(string $preset): ?string . " Preset::DDD(),\n" . " Preset::MVC(),\n" . " Preset::YAGNI(),\n" - . " Preset::CODEQUALITY()\n" + . " Preset::CODEQUALITY(),\n" + . " Preset::STABILITY()\n" . " );", default => null, }; diff --git a/src/Cli/Usage.php b/src/Cli/Usage.php index 8e80bdef..8ec0ffa9 100644 --- a/src/Cli/Usage.php +++ b/src/Cli/Usage.php @@ -11,10 +11,10 @@ public static function render(): string return <<<'TXT' Usage: structarmed --version - structarmed init [--preset=ddd|mvc|psr4|psr1|psr12|per|psr15|yagni|codequality|all] + structarmed init [--preset=ddd|mvc|psr4|psr1|psr12|per|psr15|yagni|codequality|stability|all] structarmed analyse|analyze [path ...] [--config=path/to/structarmed.php] [-d|--basepath=path/to/project] [--report=console|json|github] [--no-progress] - [--clear-cache] [--disable-parallel] [--fix] [--generate-baseline=structarmed-baseline.php] + [--clear-cache] [--no-cache] [--disable-parallel] [--fix] [--generate-baseline=structarmed-baseline.php] structarmed --clear-cache [--config=path/to/structarmed.php] [-d|--basepath=path/to/project] TXT; diff --git a/src/PHPUnit/StructArmedExtension.php b/src/PHPUnit/StructArmedExtension.php index 6c22c258..3f3f8316 100644 --- a/src/PHPUnit/StructArmedExtension.php +++ b/src/PHPUnit/StructArmedExtension.php @@ -50,7 +50,10 @@ public function bootstrap( $start = microtime(true); $fileHashProvider = new FileHashProvider(); $analysisCacheMetadataFactory = new AnalysisCacheMetadataFactory($fileHashProvider); - $configHash = $analysisCacheMetadataFactory->fileHash($configFile); + $configHash = $analysisCacheMetadataFactory->configHash( + $configFile, + $architecture->getImportedFiles() + ); $composerGeneratedVersionHash = $analysisCacheMetadataFactory->composerGeneratedVersionHash(); $analysisResultCache = new AnalysisResultCache( $basePath, diff --git a/src/Preset/Preset.php b/src/Preset/Preset.php index e810a360..10621f0d 100644 --- a/src/Preset/Preset.php +++ b/src/Preset/Preset.php @@ -12,6 +12,7 @@ use Boundwize\StructArmed\Preset\Presets\Psr15Preset; use Boundwize\StructArmed\Preset\Presets\Psr1Preset; use Boundwize\StructArmed\Preset\Presets\Psr4Preset; +use Boundwize\StructArmed\Preset\Presets\StabilityPreset; use Boundwize\StructArmed\Preset\Presets\YagniPreset; /** @@ -27,6 +28,7 @@ * ->withPreset(Preset::PSR15()) * ->withPreset(Preset::YAGNI()) * ->withPreset(Preset::CODEQUALITY()) + * ->withPreset(Preset::STABILITY()) * ->withPresets(Preset::DDD(), Preset::MVC()) */ final class Preset @@ -124,6 +126,17 @@ public static function CODEQUALITY( ); } + /** + * @param list|null $sourcePaths + */ + public static function STABILITY( + ?array $sourcePaths = null, + ): StabilityPreset { + return new StabilityPreset( + sourcePaths: $sourcePaths, + ); + } + public static function MVC( int $controllerMaxComplexity = 5, int $controllerMaxMethodLength = 20, diff --git a/src/Preset/Presets/StabilityPreset.php b/src/Preset/Presets/StabilityPreset.php new file mode 100644 index 00000000..9ba10573 --- /dev/null +++ b/src/Preset/Presets/StabilityPreset.php @@ -0,0 +1,50 @@ +|null $sourcePaths + */ + public function __construct( + private ?array $sourcePaths = null, + ) { + } + + public function apply(Architecture $architecture): void + { + $layerName = $this->resolveLayerName($architecture); + $architecture->layer($layerName, $this->sourcePaths ?? []); + + $architecture->rule( + self::API_MAY_NOT_BE_INTERNAL, + new ApiMayNotBeInternalRule($layerName) + ); + $architecture->rule( + self::API_MAY_NOT_EXTEND_INTERNAL, + new ApiMayNotExtendInternalRule($layerName) + ); + } +} diff --git a/src/Rule/AbstractLayerAwareRule.php b/src/Rule/AbstractLayerAwareRule.php index 4faf2b40..65c7164e 100644 --- a/src/Rule/AbstractLayerAwareRule.php +++ b/src/Rule/AbstractLayerAwareRule.php @@ -6,6 +6,9 @@ use Boundwize\StructArmed\Analyser\ClassNode; +use function array_change_key_case; +use function strtolower; + /** * A rule that needs the layer of a class other than the one under evaluation, * such as the layer a dependency belongs to. The analyser injects the scanned @@ -16,15 +19,25 @@ abstract class AbstractLayerAwareRule /** @var array class name → class node */ protected array $classNodeMap = []; + /** @var array lowercased class name → class node */ + private array $lowercaseClassNodeMap = []; + /** @param array $classNodeMap */ public function injectClassNodeMap(array $classNodeMap): void { - $this->classNodeMap = $classNodeMap; + $this->classNodeMap = $classNodeMap; + $this->lowercaseClassNodeMap = array_change_key_case($classNodeMap); } - /** The scanned node of a dependency, or null when it lies outside the scanned paths */ + /** + * The scanned node of a dependency, or null when it lies outside the + * scanned paths. Class names are case-insensitive, so `extends internalbase` + * still finds the node declared as InternalBase. + */ protected function getDependencyNode(string $dependency): ?ClassNode { - return $this->classNodeMap[$dependency] ?? null; + return $this->classNodeMap[$dependency] + ?? $this->lowercaseClassNodeMap[strtolower($dependency)] + ?? null; } } diff --git a/src/Rule/Rules/Class_/ApiMayNotBeInternalRule.php b/src/Rule/Rules/Class_/ApiMayNotBeInternalRule.php new file mode 100644 index 00000000..278b1a42 --- /dev/null +++ b/src/Rule/Rules/Class_/ApiMayNotBeInternalRule.php @@ -0,0 +1,43 @@ +isApi && $classNode->isInLayer($this->layer); + } + + public function evaluate(ClassNode $classNode): ?RuleViolation + { + if (! $classNode->isInternal) { + return null; + } + + return new RuleViolation( + message: sprintf( + '%s [%s] must not be marked both @api and @internal', + $classNode->getType(), + $classNode->className + ), + file: $classNode->file, + line: $classNode->line, + className: $classNode->className, + layer: $classNode->layer, + ); + } +} diff --git a/src/Rule/Rules/Class_/ApiMayNotExtendInternalRule.php b/src/Rule/Rules/Class_/ApiMayNotExtendInternalRule.php new file mode 100644 index 00000000..640d7336 --- /dev/null +++ b/src/Rule/Rules/Class_/ApiMayNotExtendInternalRule.php @@ -0,0 +1,62 @@ +isClass() && ! $classNode->isInterface) { + return false; + } + + return $classNode->isApi && $classNode->isInLayer($this->layer); + } + + public function evaluate(ClassNode $classNode): ?RuleViolation + { + // Direct and transitive parents; a class's parentInterfaces would add + // the interfaces it implements, which it does not extend + $parents = $classNode->isInterface ? $classNode->parentInterfaces : $classNode->parentClasses; + + foreach ($parents as $parent) { + // Only a scanned parent has a known docblock + $parentNode = $this->getDependencyNode($parent); + + if (! $parentNode instanceof ClassNode || ! $parentNode->isInternal) { + continue; + } + + return new RuleViolation( + message: sprintf( + '@api %s [%s] must not extend @internal %s [%s]', + $classNode->getType(), + $classNode->className, + $parentNode->getType(), + $parentNode->className + ), + file: $classNode->file, + line: $classNode->line, + className: $classNode->className, + layer: $classNode->layer, + ); + } + + return null; + } +} diff --git a/src/Rule/Rules/Composer/Psr4NamespaceRule.php b/src/Rule/Rules/Composer/Psr4NamespaceRule.php index aa8fa274..1010c2ff 100644 --- a/src/Rule/Rules/Composer/Psr4NamespaceRule.php +++ b/src/Rule/Rules/Composer/Psr4NamespaceRule.php @@ -20,7 +20,7 @@ use function file_exists; use function ltrim; use function max; -use function preg_replace; +use function preg_match; use function sprintf; use function str_ends_with; use function str_replace; @@ -30,6 +30,10 @@ final class Psr4NamespaceRule implements RuleInterface, ProjectRuleInterface { + private const IDENTIFIER = '[a-zA-Z_\x80-\xff][a-zA-Z0-9_\x80-\xff]*'; + + private const CLASS_NAME_PATTERN = '/^' . self::IDENTIFIER . '(?:\\\\' . self::IDENTIFIER . ')*$/D'; + private ?string $projectBasePath = null; /** @var array>> */ @@ -68,13 +72,13 @@ public function evaluate(ClassNode $classNode): ?RuleViolation return null; } + $expectedClassName = array_key_first($expectedClassNames); + $format = preg_match(self::CLASS_NAME_PATTERN, $expectedClassName) === 1 + ? '%s [%s] must match PSR-4 class [%s]' + : '%s [%s] is in a file that maps to invalid PSR-4 class name [%s]'; + return new RuleViolation( - message: sprintf( - '%s [%s] must match PSR-4 class [%s]', - $classNode->getType(), - $classNode->className, - array_key_first($expectedClassNames) - ), + message: sprintf($format, $classNode->getType(), $classNode->className, $expectedClassName), file: $classNode->file, line: $classNode->line, className: $classNode->className, @@ -113,7 +117,6 @@ private function expectedClassNames(string $file): array } $relativeClass = substr($relativeClass, 0, -4); - $relativeClass = (string) preg_replace('/\.class$/i', '', $relativeClass); $relativeClass = str_replace('/', '\\', $relativeClass); $className = $namespace . ltrim($relativeClass, '\\'); diff --git a/structarmed.php b/structarmed.php index 8479b684..cd96e607 100644 --- a/structarmed.php +++ b/structarmed.php @@ -57,7 +57,7 @@ __DIR__ . '/tests', ], ]) - ->withPresets(Preset::PER(), Preset::YAGNI(), Preset::CODEQUALITY()) + ->withPresets(Preset::PER(), Preset::YAGNI(), Preset::CODEQUALITY(), Preset::STABILITY()) ->rule( 'source.must_be_final', new MustBeFinalRule(layer: 'Source') diff --git a/tests/Analyser/AnalyserTest.php b/tests/Analyser/AnalyserTest.php index ad5afbf9..84d7b4e1 100644 --- a/tests/Analyser/AnalyserTest.php +++ b/tests/Analyser/AnalyserTest.php @@ -33,6 +33,7 @@ use Boundwize\StructArmed\Rule\FunctionRuleInterface; use Boundwize\StructArmed\Rule\RuleInterface; use Boundwize\StructArmed\Rule\Rules\Class_\AnonymousClassMayNotHaveEmptyParenthesesRule; +use Boundwize\StructArmed\Rule\Rules\Class_\ApiMayNotExtendInternalRule; use Boundwize\StructArmed\Rule\Rules\Class_\MayNotExtendClassRule; use Boundwize\StructArmed\Rule\Rules\Class_\MayNotImplementInterfaceRule; use Boundwize\StructArmed\Rule\Rules\Class_\MustBeFinalRule; @@ -7061,6 +7062,50 @@ final class AuthTokenStore {} $this->assertStringContainsString('Auth', $violations[0]->message); } + public function testApiMayNotExtendInternalRuleReadsAncestorDocBlockViaAnalyser(): void + { + $basePath = $this->makeTempProject([ + 'src/Facade.php' => <<<'PHP' + <<<'PHP' + <<<'PHP' + layer('Core', 'src/') + ->rule('api.not_extend_internal', new ApiMayNotExtendInternalRule(layer: 'Core')); + + $ruleViolationCollection = (new Analyser($basePath)) + ->analyse($architecture, ['src/'], null, AnalyserOptions::sequential()); + + $violations = $ruleViolationCollection->forRule('api.not_extend_internal'); + $this->assertCount(1, $violations); + $this->assertSame( + '@api Class [App\\Facade] must not extend @internal Class [App\\InternalFacade]', + $violations[0]->message + ); + } + public function testAnalyserRulesetAllowsUnscannedDependencyWhenFirstMatchedLayerIsAllowed(): void { $basePath = $this->makeTempProject([ diff --git a/tests/Analyser/AnalysisNodeCollectorTest.php b/tests/Analyser/AnalysisNodeCollectorTest.php index bfc2fe8f..371317d4 100644 --- a/tests/Analyser/AnalysisNodeCollectorTest.php +++ b/tests/Analyser/AnalysisNodeCollectorTest.php @@ -29,6 +29,8 @@ use PHPUnit\Framework\TestCase; use function array_column; +use function array_combine; +use function array_map; #[CoversClass(AnonymousClassNode::class)] #[CoversClass(AnalysisNodeCollector::class)] @@ -910,6 +912,51 @@ public function testCollectsAbstractClass(): void $this->assertFalse($classNode->isFinal); } + public function testCollectsApiAndInternalDocBlockTags(): void + { + $classNodes = $this->collectNodes(<<<'CODE_SAMPLE' + assertSame( + [ + 'PublicFacade' => [true, false], + 'InternalContract' => [false, true], + 'Untagged' => [false, false], + 'WithoutDocBlock' => [false, false], + ], + array_combine( + array_column($classNodes, 'className'), + array_map( + static fn (ClassNode $classNode): array => [$classNode->isApi, $classNode->isInternal], + $classNodes + ) + ) + ); + } + public function testCollectsInterface(): void { $classNode = $this->collect('assertInstanceOf(Architecture::class, Architecture::define()); @@ -450,4 +455,87 @@ public function testSkipPathsForRulesetAcceptsStringAndArray(): void $this->assertSame(['tests/', 'fixtures/', 'stubs/'], $architecture->getRulesetSkipPaths()); } + + public function testImportAppliesReturnedCallableAndTracksNestedFiles(): void + { + $directory = $this->makeTemporaryDirectory('structarmed-import'); + file_put_contents($directory . '/bar.php', <<<'PHP' + layer('Bar', 'module/Bar/src/'); + }; + + PHP); + file_put_contents($directory . '/foo.php', <<<'PHP' + layer('Foo', 'module/Foo/src/') + ->import(__DIR__ . '/bar.php'); + }; + + PHP); + + // bar.php again after foo.php imported it: a repeated import is not a cycle + $architecture = Architecture::define() + ->layer('App', 'src/') + ->import($directory . '/foo.php') + ->import($directory . '/bar.php'); + + $this->assertSame( + ['App' => 'src/', 'Foo' => 'module/Foo/src/', 'Bar' => 'module/Bar/src/'], + $architecture->getLayers() + ); + $this->assertSame( + [realpath($directory . '/bar.php'), realpath($directory . '/foo.php'), realpath($directory . '/bar.php')], + $architecture->getImportedFiles() + ); + } + + public function testImportThrowsWhenFileDoesNotReturnCallable(): void + { + $path = $this->makeTemporaryFile('structarmed-import'); + file_put_contents($path, "expectException(InvalidArgumentException::class); + $this->expectExceptionMessage(sprintf('Imported configuration [%s] must return a callable.', $path)); + + Architecture::define()->import($path); + } + + public function testImportThrowsOnCircularImport(): void + { + $directory = $this->makeTemporaryDirectory('structarmed-import'); + file_put_contents($directory . '/foo.php', <<<'PHP' + import(__DIR__ . '/bar.php'); + }; + + PHP); + file_put_contents($directory . '/bar.php', <<<'PHP' + import(__DIR__ . '/foo.php'); + }; + + PHP); + + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessageMatches('#^Circular import of configuration \[.+/foo\.php\]\.$#'); + + Architecture::define()->import($directory . '/foo.php'); + } } diff --git a/tests/Cache/AnalysisResultCacheTest.php b/tests/Cache/AnalysisResultCacheTest.php index cba0d7df..bb37f497 100644 --- a/tests/Cache/AnalysisResultCacheTest.php +++ b/tests/Cache/AnalysisResultCacheTest.php @@ -2043,6 +2043,25 @@ public static function malformedClassNodePayloadProvider(): iterable ], ], ]; + yield 'node has invalid isApi field' => [ + [ + 'nodes' => [ + [ + 'className' => Foo::class, + 'line' => 1, + 'layer' => null, + 'extends' => null, + 'isAbstract' => false, + 'isFinal' => true, + 'isInterface' => false, + 'isTrait' => false, + 'isEnum' => false, + 'isReadonly' => false, + 'isApi' => 'yes', + ], + ], + ], + ]; yield 'node has invalid string array key' => [ [ 'nodes' => [ @@ -2864,6 +2883,40 @@ public function testMetadataFileHashDoesNotChangeWhenOnlyFileMtimeChanges(): voi } } + public function testConfigHashChangesWhenImportedFileChanges(): void + { + $directory = $this->createTempDirectory(); + $config = $directory . '/structarmed.php'; + $imported = $directory . '/module.php'; + + file_put_contents($config, 'configHash($config, [$imported]); + + $this->assertNotSame( + (new AnalysisCacheMetadataFactory(new FileHashProvider()))->configHash($config, []), + $configHash + ); + + file_put_contents($imported, 'assertNotSame( + (new AnalysisCacheMetadataFactory(new FileHashProvider()))->configHash($config, [$imported]), + $configHash + ); + } finally { + foreach ([$config, $imported] as $file) { + if (file_exists($file)) { + unlink($file); + } + } + + $this->removeTempDirectory($directory); + } + } + public function testCacheMissesWhenComposerGeneratedVersionChanges(): void { $directory = $this->createTempDirectory(); @@ -2958,6 +3011,8 @@ className: Foo::class, functionCalls: ['sprintf'], superglobals: ['_SERVER'], constantFetches: ['PHP_EOL'], + isApi: true, + isInternal: true, ); } diff --git a/tests/Cli/InitCommandTest.php b/tests/Cli/InitCommandTest.php index a9f5b365..1b9a9d45 100644 --- a/tests/Cli/InitCommandTest.php +++ b/tests/Cli/InitCommandTest.php @@ -84,6 +84,11 @@ public static function presetProvider(): iterable ' ->withPreset(Preset::CODEQUALITY());', ]; + yield 'stability' => [ + ['--preset=stability'], + ' ->withPreset(Preset::STABILITY());', + ]; + yield 'all' => [ ['--preset=all'], " ->withPresets(\n" @@ -95,7 +100,8 @@ public static function presetProvider(): iterable . " Preset::DDD(),\n" . " Preset::MVC(),\n" . " Preset::YAGNI(),\n" - . " Preset::CODEQUALITY()\n" + . " Preset::CODEQUALITY(),\n" + . " Preset::STABILITY()\n" . " );", ]; } diff --git a/tests/Cli/StructArmedApplicationCommandRoutingTest.php b/tests/Cli/StructArmedApplicationCommandRoutingTest.php index 8b3ca7c8..f1adfd1b 100644 --- a/tests/Cli/StructArmedApplicationCommandRoutingTest.php +++ b/tests/Cli/StructArmedApplicationCommandRoutingTest.php @@ -35,7 +35,7 @@ public function testApplicationPrintsUsageWithoutCommand(): void $this->assertSame(0, $exitCode); $this->assertStringContainsString('structarmed --version', $output); $this->assertStringContainsString( - 'structarmed init [--preset=ddd|mvc|psr4|psr1|psr12|per|psr15|yagni|codequality|all]', + 'structarmed init [--preset=ddd|mvc|psr4|psr1|psr12|per|psr15|yagni|codequality|stability|all]', $output ); $this->assertStringContainsString('structarmed analyse|analyze', $output); diff --git a/tests/Cli/StructArmedApplicationTest.php b/tests/Cli/StructArmedApplicationTest.php index 296ef73c..bfeba192 100644 --- a/tests/Cli/StructArmedApplicationTest.php +++ b/tests/Cli/StructArmedApplicationTest.php @@ -223,6 +223,11 @@ public static function presetProvider(): iterable ' ->withPreset(Preset::CODEQUALITY());', ]; + yield 'stability' => [ + ['--preset=stability'], + ' ->withPreset(Preset::STABILITY());', + ]; + yield 'all' => [ ['--preset=all'], " ->withPresets(\n" @@ -234,7 +239,8 @@ public static function presetProvider(): iterable . " Preset::DDD(),\n" . " Preset::MVC(),\n" . " Preset::YAGNI(),\n" - . " Preset::CODEQUALITY()\n" + . " Preset::CODEQUALITY(),\n" + . " Preset::STABILITY()\n" . " );", ]; } @@ -462,6 +468,141 @@ class Foo } } + public function testAnalyseCommandDetectsNestedImportedConfigChangesWithoutClearCache(): void + { + $basePath = $this->createTempDirectory(); + $progress = new class implements ProgressHandlerInterface { + public bool $started = false; + + public function start(int $total): void + { + $this->started = true; + } + + public function advance(string $file): void + { + } + + public function finish(): void + { + } + }; + mkdir($basePath . '/src'); + + file_put_contents($basePath . '/src/Foo.php', <<<'PHP' +layer('Source', []) + ->import(__DIR__ . '/foo.php'); +PHP); + + file_put_contents($basePath . '/foo.php', <<<'PHP' +import(__DIR__ . '/bar.php'); +}; +PHP); + + file_put_contents($basePath . '/bar.php', <<<'PHP' +runApplication( + [ + 'structarmed', + 'analyse', + 'src', + '--config=' . $basePath . '/structarmed.php', + '--clear-cache', + '--no-progress', + ], + $basePath + ); + + $this->assertSame(0, $firstExitCode, $firstOutput); + $this->assertStringContainsString('No violations found', $firstOutput); + + [$warmExitCode, $warmOutput] = $this->runAnalyseCommand( + ['src', '--config=' . $basePath . '/structarmed.php'], + $basePath, + $progress + ); + + $this->assertSame(0, $warmExitCode, $warmOutput); + $this->assertStringContainsString('No violations found', $warmOutput); + $this->assertFalse($progress->started); + + file_put_contents($basePath . '/bar.php', <<<'PHP' +rule('source.must_be_final', new MustBeFinalRule('Source')); +}; +PHP); + + [$secondExitCode, $secondOutput] = $this->runApplication( + [ + 'structarmed', + 'analyse', + 'src', + '--config=' . $basePath . '/structarmed.php', + '--no-progress', + ], + $basePath + ); + + $this->assertSame(1, $secondExitCode, $secondOutput); + $this->assertStringContainsString('Class [App\Foo] must be declared final', $secondOutput); + } finally { + unlink($basePath . '/foo.php'); + unlink($basePath . '/bar.php'); + $this->removeTempDirectory($basePath); + } + } + public function testAnalyseCommandResolvesProjectPathsAgainstBasePathOption(): void { $basePath = (string) realpath($this->createTempDirectory()); @@ -658,6 +799,137 @@ public function testAnalyseCommandStoresResultInConfiguredCacheDirectory(): void } } + public function testAnalyseCommandWithNoCacheOptionDoesNotWriteCache(): void + { + $basePath = $this->createProjectDirectoryWithViolation(); + $cacheDirectory = $basePath . '/var/cache/structarmed'; + + file_put_contents($basePath . '/structarmed.php', <<<'PHP' +cacheDirectory('var/cache/structarmed') + ->layer('Source', 'src/') + ->rule('source.must_be_final', new MustBeFinalRule('Source')); +PHP); + + try { + [$exitCode, $output] = $this->runApplication( + [ + 'structarmed', + 'analyse', + '--config=' . $basePath . '/structarmed.php', + '--no-cache', + '--no-progress', + ], + $basePath + ); + + $this->assertSame(1, $exitCode, $output); + $this->assertStringContainsString('1 violation(s) found', $output); + $this->assertDirectoryDoesNotExist($cacheDirectory); + } finally { + $this->removeTempDirectory($basePath); + } + } + + public function testAnalyseCommandWithNoCacheOptionKeepsExistingCache(): void + { + $basePath = $this->createProjectDirectoryWithViolation(); + $cacheDirectory = $basePath . '/var/cache/structarmed'; + + file_put_contents($basePath . '/structarmed.php', <<<'PHP' +cacheDirectory('var/cache/structarmed') + ->layer('Source', 'src/') + ->rule('source.must_be_final', new MustBeFinalRule('Source')); +PHP); + + try { + [$cachedExitCode, $cachedOutput] = $this->runApplication( + ['structarmed', 'analyse', '--config=' . $basePath . '/structarmed.php', '--no-progress'], + $basePath + ); + $cacheContents = $this->directoryContents($cacheDirectory); + + // A cached run would now rewrite the node cache of the changed file. + file_put_contents($basePath . '/src/Foo.php', 'runApplication( + [ + 'structarmed', + 'analyse', + '--config=' . $basePath . '/structarmed.php', + '--no-cache', + '--no-progress', + ], + $basePath + ); + + $this->assertSame(1, $cachedExitCode, $cachedOutput); + $this->assertNotSame([], $cacheContents); + $this->assertSame(0, $exitCode, $output); + $this->assertStringContainsString('No violations found', $output); + $this->assertSame($cacheContents, $this->directoryContents($cacheDirectory)); + } finally { + $this->removeTempDirectory($basePath); + } + } + + public function testAnalyseCommandRejectsNoCacheWithClearCacheOption(): void + { + $basePath = $this->createProjectDirectory(); + $cacheDirectory = $basePath . '/var/cache/structarmed'; + + try { + mkdir($cacheDirectory, 0777, true); + file_put_contents($cacheDirectory . '/_metadata.json', '{}'); + + [$exitCode, $output] = $this->runApplication( + [ + 'structarmed', + 'analyse', + '--config=' . $basePath . '/structarmed.php', + '--no-cache', + '--clear-cache', + ], + $basePath + ); + + $this->assertSame(1, $exitCode, $output); + $this->assertSame("Error: --no-cache and --clear-cache cannot be used together.\n", $output); + $this->assertFileExists($cacheDirectory . '/_metadata.json'); + } finally { + $this->removeTempDirectory($basePath); + } + } + + /** + * @return array + */ + private function directoryContents(string $directory): array + { + $contents = []; + + foreach (glob($directory . '/*.json') ?: [] as $file) { + $contents[$file] = (string) file_get_contents($file); + } + + return $contents; + } + public function testAnalyseCommandRendersJsonReport(): void { $basePath = $this->createProjectDirectory(); @@ -731,6 +1003,35 @@ public function testAnalyseCommandFixesFixableViolations(): void } } + public function testAnalyseCommandFixesFixableViolationsWithNoCacheOption(): void + { + $basePath = $this->createProjectDirectoryWithImplicitMethodVisibilityViolation(); + + try { + [$exitCode, $output] = $this->runApplication( + [ + 'structarmed', + 'analyze', + '--config=' . $basePath . '/structarmed.php', + '--fix', + '--no-cache', + '--no-progress', + ], + $basePath + ); + + $this->assertSame(0, $exitCode, $output); + $this->assertStringContainsString('1 violation has been fixed.', $this->withoutAnsi($output)); + $this->assertStringContainsString('No violations found', $output); + $this->assertStringContainsString( + ' public function handle(): void', + (string) file_get_contents($basePath . '/src/Foo.php') + ); + } finally { + $this->removeTempDirectory($basePath); + } + } + public function testAnalyseCommandCountsEveryViolationResolvedByOneFix(): void { $basePath = $this->createProjectDirectory(); diff --git a/tests/Preset/PresetTest.php b/tests/Preset/PresetTest.php index 0040c224..5d0d61a5 100644 --- a/tests/Preset/PresetTest.php +++ b/tests/Preset/PresetTest.php @@ -15,7 +15,10 @@ use Boundwize\StructArmed\Preset\Presets\Psr1Preset; use Boundwize\StructArmed\Preset\Presets\Psr4Preset; use Boundwize\StructArmed\Preset\Presets\ResolvesSourceLayerNameTrait; +use Boundwize\StructArmed\Preset\Presets\StabilityPreset; use Boundwize\StructArmed\Preset\Presets\YagniPreset; +use Boundwize\StructArmed\Rule\Rules\Class_\ApiMayNotBeInternalRule; +use Boundwize\StructArmed\Rule\Rules\Class_\ApiMayNotExtendInternalRule; use Boundwize\StructArmed\Rule\Rules\Class_\ExtendedClassMustBeAbstractOrInstantiatedRule; use Boundwize\StructArmed\Rule\Rules\Class_\MayNotExtendClassRule; use Boundwize\StructArmed\Rule\Rules\Class_\MustBeUsedAbstractClassRule; @@ -37,6 +40,7 @@ #[CoversClass(Psr15Preset::class)] #[CoversClass(Psr4Preset::class)] #[CoversClass(ResolvesSourceLayerNameTrait::class)] +#[CoversClass(StabilityPreset::class)] #[CoversClass(YagniPreset::class)] final class PresetTest extends TestCase { @@ -105,6 +109,28 @@ public function testCodeQualityPresetRegistersSourceLayerAndRules(): void ); } + public function testStabilityPresetRegistersSourceLayerAndRules(): void + { + $architecture = Architecture::define(); + + Preset::STABILITY( + sourcePaths: ['src/'], + )->apply($architecture); + + $this->assertSame(['Source' => ['src/']], $architecture->getLayers()); + + $rules = $architecture->getRules(); + $this->assertCount(2, $rules); + $this->assertInstanceOf( + ApiMayNotBeInternalRule::class, + $rules[StabilityPreset::API_MAY_NOT_BE_INTERNAL] ?? null + ); + $this->assertInstanceOf( + ApiMayNotExtendInternalRule::class, + $rules[StabilityPreset::API_MAY_NOT_EXTEND_INTERNAL] ?? null + ); + } + public function testCodeQualityPresetUsesComposerSourcePathsByDefault(): void { $architecture = Architecture::define(); diff --git a/tests/Rule/Class_/ApiMayNotBeInternalRuleTest.php b/tests/Rule/Class_/ApiMayNotBeInternalRuleTest.php new file mode 100644 index 00000000..11e03782 --- /dev/null +++ b/tests/Rule/Class_/ApiMayNotBeInternalRuleTest.php @@ -0,0 +1,70 @@ +assertTrue($apiMayNotBeInternalRule->appliesTo($this->makeNode(isApi: true))); + $this->assertFalse($apiMayNotBeInternalRule->appliesTo($this->makeNode(isInternal: true))); + $this->assertFalse($apiMayNotBeInternalRule->appliesTo($this->makeNode(layer: 'Other', isApi: true))); + } + + public function testViolatesWhenMarkedBothApiAndInternal(): void + { + $apiMayNotBeInternalRule = new ApiMayNotBeInternalRule(layer: 'Core'); + + $violation = $apiMayNotBeInternalRule->evaluate( + $this->makeNode(isInterface: true, isApi: true, isInternal: true) + ); + + $this->assertInstanceOf(RuleViolation::class, $violation); + $this->assertSame( + 'Interface [App\Facade] must not be marked both @api and @internal', + $violation->message + ); + } + + public function testPassesWhenMarkedOnlyApi(): void + { + $apiMayNotBeInternalRule = new ApiMayNotBeInternalRule(layer: 'Core'); + + $this->assertNotInstanceOf( + RuleViolation::class, + $apiMayNotBeInternalRule->evaluate($this->makeNode(isApi: true)) + ); + } + + private function makeNode( + string $layer = 'Core', + bool $isInterface = false, + bool $isApi = false, + bool $isInternal = false, + ): ClassNode { + return new ClassNode( + className: 'App\Facade', + file: '/fake.php', + line: 1, + layer: $layer, + extends: null, + isAbstract: false, + isFinal: false, + isInterface: $isInterface, + isReadonly: false, + isApi: $isApi, + isInternal: $isInternal, + ); + } +} diff --git a/tests/Rule/Class_/ApiMayNotExtendInternalRuleTest.php b/tests/Rule/Class_/ApiMayNotExtendInternalRuleTest.php new file mode 100644 index 00000000..d6da31a5 --- /dev/null +++ b/tests/Rule/Class_/ApiMayNotExtendInternalRuleTest.php @@ -0,0 +1,149 @@ +assertTrue($apiMayNotExtendInternalRule->appliesTo($this->makeNode('App\Facade', isApi: true))); + $this->assertTrue( + $apiMayNotExtendInternalRule->appliesTo($this->makeNode('App\Facade', isInterface: true, isApi: true)) + ); + $this->assertFalse( + $apiMayNotExtendInternalRule->appliesTo($this->makeNode('App\Facade', isTrait: true, isApi: true)) + ); + $this->assertFalse( + $apiMayNotExtendInternalRule->appliesTo($this->makeNode('App\Facade', isEnum: true, isApi: true)) + ); + $this->assertFalse($apiMayNotExtendInternalRule->appliesTo($this->makeNode('App\Facade'))); + $this->assertFalse( + $apiMayNotExtendInternalRule->appliesTo($this->makeNode('App\Facade', layer: 'Other', isApi: true)) + ); + } + + public function testViolatesWhenClassExtendsInternalAncestor(): void + { + $apiMayNotExtendInternalRule = new ApiMayNotExtendInternalRule(layer: 'Core'); + $apiMayNotExtendInternalRule->injectClassNodeMap([ + 'App\BaseFacade' => $this->makeNode('App\BaseFacade'), + 'App\InternalFacade' => $this->makeNode('App\InternalFacade', isInternal: true), + ]); + + $violation = $apiMayNotExtendInternalRule->evaluate( + $this->makeNode('App\Facade', parentClasses: ['App\BaseFacade', 'App\InternalFacade'], isApi: true) + ); + + $this->assertInstanceOf(RuleViolation::class, $violation); + $this->assertSame( + '@api Class [App\Facade] must not extend @internal Class [App\InternalFacade]', + $violation->message + ); + } + + public function testFindsInternalAncestorWrittenInAnotherCase(): void + { + $apiMayNotExtendInternalRule = new ApiMayNotExtendInternalRule(layer: 'Core'); + $apiMayNotExtendInternalRule->injectClassNodeMap([ + 'App\InternalFacade' => $this->makeNode('App\InternalFacade', isInternal: true), + ]); + + // Class names are case-insensitive: `extends internalfacade` is valid PHP + $violation = $apiMayNotExtendInternalRule->evaluate( + $this->makeNode('App\Facade', parentClasses: ['App\BaseFacade', 'app\internalfacade'], isApi: true) + ); + + $this->assertInstanceOf(RuleViolation::class, $violation); + $this->assertSame( + '@api Class [App\Facade] must not extend @internal Class [App\InternalFacade]', + $violation->message + ); + } + + public function testViolatesWhenInterfaceExtendsInternalInterface(): void + { + $apiMayNotExtendInternalRule = new ApiMayNotExtendInternalRule(layer: 'Core'); + $apiMayNotExtendInternalRule->injectClassNodeMap([ + 'App\Public' => $this->makeNode('App\Public', isInterface: true), + 'App\Internal' => $this->makeNode('App\Internal', isInterface: true, isInternal: true), + ]); + + $violation = $apiMayNotExtendInternalRule->evaluate($this->makeNode( + 'App\Contract', + parentInterfaces: ['App\Public', 'App\Unscanned', 'App\Internal'], + isInterface: true, + isApi: true, + )); + + $this->assertInstanceOf(RuleViolation::class, $violation); + $this->assertSame( + '@api Interface [App\Contract] must not extend @internal Interface [App\Internal]', + $violation->message + ); + } + + public function testPassesWhenNoParentClassIsInternalOrScanned(): void + { + $apiMayNotExtendInternalRule = new ApiMayNotExtendInternalRule(layer: 'Core'); + $apiMayNotExtendInternalRule->injectClassNodeMap([ + 'App\BaseFacade' => $this->makeNode('App\BaseFacade', isApi: true), + 'App\InternalContract' => $this->makeNode('App\InternalContract', isInterface: true, isInternal: true), + ]); + + // A class implements interfaces rather than extending them + $this->assertNotInstanceOf( + RuleViolation::class, + $apiMayNotExtendInternalRule->evaluate($this->makeNode( + 'App\Facade', + parentClasses: ['App\BaseFacade', 'Vendor\Base'], + parentInterfaces: ['App\InternalContract'], + isApi: true, + )) + ); + } + + /** + * @param list $parentClasses + * @param list $parentInterfaces + */ + private function makeNode( + string $className, + string $layer = 'Core', + array $parentClasses = [], + array $parentInterfaces = [], + bool $isInterface = false, + bool $isTrait = false, + bool $isEnum = false, + bool $isApi = false, + bool $isInternal = false, + ): ClassNode { + return new ClassNode( + className: $className, + file: '/fake.php', + line: 1, + layer: $layer, + extends: $parentClasses[0] ?? null, + isAbstract: false, + isFinal: false, + isInterface: $isInterface, + isReadonly: false, + isTrait: $isTrait, + isEnum: $isEnum, + parentClasses: $parentClasses, + parentInterfaces: $parentInterfaces, + isApi: $isApi, + isInternal: $isInternal, + ); + } +} diff --git a/tests/Rule/Composer/Psr4NamespaceRuleTest.php b/tests/Rule/Composer/Psr4NamespaceRuleTest.php index 73893a52..43ee72b5 100644 --- a/tests/Rule/Composer/Psr4NamespaceRuleTest.php +++ b/tests/Rule/Composer/Psr4NamespaceRuleTest.php @@ -16,6 +16,7 @@ use function file_put_contents; use function json_encode; use function mkdir; +use function sprintf; #[CoversClass(Psr4NamespaceRule::class)] final class Psr4NamespaceRuleTest extends TestCase @@ -205,19 +206,45 @@ public function testPassesWhenRelativeFileIsNotPhpFile(): void $this->assertNotInstanceOf(RuleViolation::class, $psr4NamespaceRule->evaluate($this->makeNode('Foo', $file))); } - public function testStripsClassSuffixFromExpectedClassName(): void + public function testDoesNotTreatClassPhpSuffixAsPhpFile(): void { $basePath = $this->makeTempProject(); - $file = $basePath . '/tests/Foo.class.php'; + mkdir($basePath . '/tests/Service'); - file_put_contents($file, 'assertNotInstanceOf( RuleViolation::class, - $psr4NamespaceRule->evaluate($this->makeNode('App\\Tests\\Foo', $file)) + $psr4NamespaceRule->evaluate($this->makeNode('App\\Tests\\Foo', $basePath . '/tests/Foo.php')) ); + + foreach ($this->classPhpSuffixFiles() as $relativeFile => [$className, $expectedClassName]) { + $file = $basePath . '/tests/' . $relativeFile; + file_put_contents($file, 'evaluate($this->makeNode($className, $file)); + + $this->assertInstanceOf(RuleViolation::class, $violation, $relativeFile); + $this->assertSame( + sprintf( + 'Class [%s] is in a file that maps to invalid PSR-4 class name [%s]', + $className, + $expectedClassName + ), + $violation->message, + $relativeFile + ); + } + } + + /** @return iterable */ + private function classPhpSuffixFiles(): iterable + { + yield 'Foo.class.php' => ['App\\Tests\\Foo', 'App\\Tests\\Foo.class']; + yield 'Bar.class.php' => ['App\\Tests\\BarClass', 'App\\Tests\\Bar.class']; + yield 'Service/Foo.class.php' => ['App\\Tests\\Service\\Foo', 'App\\Tests\\Service\\Foo.class']; } public function testFailsWhenTraitNameDoesNotMatchFilename(): void diff --git a/tests/Rule/Layer/MayNotDependOnRuleTest.php b/tests/Rule/Layer/MayNotDependOnRuleTest.php index 12d1380b..85b698dd 100644 --- a/tests/Rule/Layer/MayNotDependOnRuleTest.php +++ b/tests/Rule/Layer/MayNotDependOnRuleTest.php @@ -202,6 +202,17 @@ public function testPassesUsingClassNodeMapWhenDependencyNotInForbiddenLayer(): $this->assertNotInstanceOf(RuleViolation::class, $mayNotDependOnRule->evaluate($classNode)); } + public function testFindsDependencyNodeWrittenInAnotherCase(): void + { + // Path matching would miss the lowercased name; the node's layer catches it. + $mayNotDependOnRule = new MayNotDependOnRule(from: 'Domain', to: 'Infrastructure'); + $dependencyNode = $this->makeNode(layer: 'Infrastructure', className: 'App\Persistence\OrderRepository'); + $mayNotDependOnRule->injectClassNodeMap([$dependencyNode->className => $dependencyNode]); + $classNode = $this->makeNode('Domain', ['app\persistence\orderrepository']); + + $this->assertInstanceOf(RuleViolation::class, $mayNotDependOnRule->evaluate($classNode)); + } + public function testClassNodeMapTakesPrecedenceOverToPath(): void { // Path would match 'App\Infrastructure\A', but its node is in Domain — no violation.