diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9a2dd7f..8adfab3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -7,11 +7,22 @@ on: jobs: run: + name: php ${{ matrix.php-versions }} / grid-bundle ${{ matrix.grid-bundle }} runs-on: ${{ matrix.operating-system }} strategy: + fail-fast: false matrix: operating-system: ['ubuntu-latest'] php-versions: ['8.2'] + # The extension supports grid-bundle >= 1.15. Sylius 1.16 moved the + # builder interfaces into Sylius\Component\Grid and deprecated the + # Sylius\Bundle\GridBundle equivalents, so both sides of that boundary + # need explicit coverage. Constraints are pinned per lane instead of + # reusing the committed lockfile, otherwise bumping the lock to 1.16 + # would silently turn the low lane into a second high lane. + grid-bundle: + - '^1.15 <1.16' + - '^1.16' coveralls: [ false ] steps: - name: Checkout repo @@ -26,7 +37,15 @@ jobs: extensions: bcmath, gd - name: Install Composer dependencies - run: composer install + run: | + composer require --dev "sylius/grid-bundle:${{ matrix.grid-bundle }}" --no-update --no-interaction + composer update --no-interaction --no-progress + + - name: Show resolved dependency versions + run: | + composer show sylius/grid-bundle + composer show phpstan/phpstan + composer show phpunit/phpunit - name: Composer license check run: composer check-license diff --git a/.php-cs-fixer.dist.php b/.php-cs-fixer.dist.php index 1fee96a..bbf3138 100644 --- a/.php-cs-fixer.dist.php +++ b/.php-cs-fixer.dist.php @@ -4,7 +4,16 @@ $finder = (new PhpCsFixer\Finder()) ->in(__DIR__) - ->exclude(['vendor']); + ->exclude([ + 'vendor', + // Analysis fixtures are loaded through composer autoload-dev.files, not + // by class name, so the psr_autoloading rule that @Symfony enables does + // not apply to them: their file name is snake_case while their classes + // are PSR-1. Without this the fixer would rewrite every fixture class to + // its file's basename, which breaks the references between fixtures. + 'tests/bitExpert/PHPStan/Sylius/Rule/Grid/data', + 'tests/bitExpert/PHPStan/Sylius/Rule/Resource/data', + ]); return (new PhpCsFixer\Config()) ->setRiskyAllowed(true) diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000..65dd1dc --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,624 @@ +# AGENTS.md – PHPStan Sylius Extension + +This file provides structured context for AI agents (e.g., copilot, Claude, etc.) working on the `bitexpert/phpstan-sylius` codebase. It documents architecture, conventions, and key implementation details to enable rapid, accurate contributions. + +--- + +## Overview + +- **Package**: `bitexpert/phpstan-sylius` +- **Type**: PHPStan extension (type: `phpstan-extension` in `composer.json`) +- **Purpose**: Additional static analysis rules for Sylius projects, validating grid configurations and resource metadata. +- **Requirements**: PHP ^8.2, PHPStan ^2.1. Dev: resource-bundle ^1.12, grid-bundle **^1.15**. +- **grid-bundle support**: `>= 1.15`. CI exercises both `^1.15 <1.16` and `^1.16`; behaviour differs between them (see [Grid Bundle 1.16](#grid-bundle-116)). +- **License**: MIT + +### Installation & Usage + +Install via Composer as a dev dependency: +```bash +composer require --dev bitexpert/phpstan-sylius +``` + +The extension is auto-discovered via `composer.json` `extra.phpstan.includes` (`extension.neon`). + +--- + +## Project Structure + +``` +src/ +└─ bitExpert/PHPStan/ + ├─ Util/PropertyName.php # Utility for snake_case → camelCase conversion + ├─ Sylius/ + │ ├─ Collector/Grid/ + │ │ ├─ AbstractGridClassCollector.php # Base: grid detection logic + │ │ ├─ CollectRessourceClassForGridClass.php # Collector: grid ↔ resource mapping + │ │ ├─ CollectFieldsForGridClass.php # Collector: grid field definitions + │ │ └─ CollectFilterForGridClass.php # Collector: grid filter definitions + │ │ + │ ├─ Collector/Grid/Field/ + │ │ ├─ FieldNode.php # Interface for field type descriptors + │ │ ├─ FieldRegistry.php # Interface + │ │ ├─ FieldRegistryFactory.php # DI factory for field registry + │ │ ├─ DefaultFieldRegistry.php # Concrete registry + │ │ └─ *FieldNode.php # StringFieldNode, DateTimeFieldNode, TwigFieldNode, EnumFieldNode, CallableFieldNode, GenericFieldNode + │ │ + │ ├─ Collector/Grid/Filter/ + │ │ ├─ FilterNode.php # Interface for filter type descriptors + │ │ ├─ FilterRegistry.php # Interface + │ │ ├─ FilterRegistryFactory.php # DI factory for filter registry + │ │ ├─ DefaultFilterRegistry.php # Concrete registry + │ │ └─ *.php # EntityFilter, EnumFilter, ExistsFilter, SelectFilter, StringFilter, BooleanFilter, DateFilter, MoneyFilter, Filter (catch-all) + │ │ + │ └─ Rule/ + │ ├─ Grid/ + │ │ ├─ ResourceAwareGridNeedsResourceClass.php # Rule: check grid resource class exists + │ │ ├─ GridBuilderFieldIsPartOfResourceClass.php # Rule: grid fields must match resource + │ │ └─ GridBuilderFilterIsPartOfResourceClass.php# Rule: grid filters must match resource + │ │ + │ └─ Resource/ + │ ├─ IndexOperationNeedsGridClassRule.php # Rule: Index metadata grid exists + │ └─ ResourceAttributeNeedsFormTypeRule.php # Rule: AsResource form type exists +``` + +**Note**: `CollectRessourceClassForGridClass.php` contains a typo: `Ressource` (double `s`), which is preserved for historical compatibility. + +**Note**: field nodes carry a `Node` suffix in their class name; filter nodes do **not**. `Field/StringFieldNode.php` vs `Filter/StringFilter.php`. + +--- + +## Key Conventions + +### 1. Coding Standards +- PSR-2 (with PHP-CS-Fixer config): + - Short array syntax `[]` + - Strict types declared per file (`declare(strict_types=1);`) + - `final` classes where appropriate + - `readonly` properties (PHP 8.1+) + - Constructor property promotion + - No trailing whitespace, single space around concatenation +- **Strict mode**: All files are strict-typed. +- **PHP Version**: Minimum 8.2 (uses readonly properties, union types, attributes, etc.) + +### 2. File Naming +- Classes are `PascalCase`. +- Test files end with `UnitTest.php`. +- Data fixtures (for analysis) use descriptive names like `grid.php`, `entity.php`, often matching test method context. + +### 3. Namespace +- Source: `bitExpert\PHPStan\` +- Tests: `bitExpert\PHPStan\` (autoload-dev maps tests to same namespace) + +--- + +## Architecture & Workflow + +### Two-Layer Pattern: Collectors → Rules + +PHPStan extensions in this project follow a two-phase approach: + +1. **Collectors** scan AST and return lightweight tuples (grid class, property/filter names, line numbers). +2. **Rules** consume collected data (via `CollectedDataNode`) and perform validation, reporting errors. + +This separation improves performance (collectors can run in parallel) and keeps rules focused on analysis rather than parsing. + +> **Note**: `getNodeType()` is authoritative and must agree with the `@implements` generic. Both grid +> rules return `CollectedDataNode::class`. The `processNode()` parameter stays typed as `Node` because +> that is what `Rule::processNode()` declares, and narrowing a parameter is a contravariance +> violation. Do **not** add a defensive `instanceof` guard to compensate — see +> [Known Issue 2](#2-a-redundant-instanceof-guard-can-hide-a-wrong-implements). + +### Rule Execution Flow + +| Rule | Node Type | Key Logic | +|------|-----------|-----------| +| `ResourceAwareGridNeedsResourceClass` | `InClassNode` | Checks `#AsGrid(resourceClass:)` first, then `getResourceClass()` / `ResourceAwareGridInterface`, for an existing class. | +| `GridBuilderFieldIsPartOfResourceClass` | `CollectedDataNode` | Validates every grid field exists as a property/getter on the resource class. Supports recursive fields (`address.city`). | +| `GridBuilderFilterIsPartOfResourceClass` | `CollectedDataNode` | Validates every grid filter field exists on resource. Does **not** support recursive checks (dots are skipped). | +| `IndexOperationNeedsGridClassRule` | `InClassNode` | Validates `#[Index(grid: '...')]` refers to an existing grid class. | +| `ResourceAttributeNeedsFormTypeRule` | `InClassNode` | Validates `#[AsResource(formType: '...')]` refers to an existing form type. | + +--- + +## Rules Reference + +### `ResourceAwareGridNeedsResourceClass` + +- **Namespace**: `bitExpert\PHPStan\Sylius\Rule\Grid` +- **Node type**: `InClassNode` +- **Checks, in order**: + 1. `#[Sylius\Component\Grid\Attribute\AsGrid(resourceClass: 'App\Entity\Supplier')]` — the attribute wins over any method, because 1.16 grids may declare no methods at all. + 2. Legacy `public function getResourceClass(): string { return Supplier::class; }` (also the `ResourceAwareGridInterface` contract). + 3. Class/subclass of `Sylius\Bundle\GridBundle\Grid\AbstractGrid`. +- **Error**: `sylius.grid.resourceClassRequired` → `Resource class "%s" not found!` +- **Note**: reads the attribute argument from the AST, not the PHPDoc-resolved value, so it also fires under 1.16 where the class exposes no `getResourceClass()`. + +--- + +### `GridBuilderFieldIsPartOfResourceClass` + +- **Namespace**: `bitExpert\PHPStan\Sylius\Rule\Grid` +- **Node type**: `CollectedDataNode` +- **Collectors used**: + - `CollectRessourceClassForGridClass` → `gridClass → resourceClass` + - `CollectFieldsForGridClass` → `gridClass → [fieldName, line]` +- **Validation**: + - Non-dotted field names (e.g., `name`) → check property or `get{Name}` method. + - Dotted field names (e.g., `address.city`) → recursive property traversal: + 1. Check first segment (`address`) exists. + 2. Determine next type via getter return or property type (native → PHPDoc). + 3. Repeat until all segments validated. +- **Special case**: Field name `.` means "whole object passed"; ignored. +- **Errors**: + - `sylius.grid.resourceClassMissingProperty`: Field missing as property/getter. + - `sylius.grid.resourceClassPropertyMissingType`: Unable to identify type for recursive path. +- **Identifier note**: Grammar intentionally uses "needs to exists" (not "need to exist") in error messages. + +#### Invariant: re-resolve the resource class per field + +`$resourceClass` is re-resolved from `$resourceClassName` at the **top of every field iteration**, and +the recursive walk keeps it a `ClassReflection` (`resolveNextClass()` / `toClassReflection()`). Both are load-bearing: + +- The recursive branch reassigns `$resourceClass` to the last segment's type. Without the re-resolve, a grid + with `address.city` followed by any other field validates the rest of the grid against `App\Entity\Address`. +- `Type::hasProperty()` is declared on `PHPStan\Type\Type` and returns **`TrinaryLogic`**. Casting that object + to bool is *always true*, so a `Type` in that position makes `!$type->hasProperty($x)` always `false` and + every remaining segment silently passes. This is a silent-pass bug, not a crash — it only shows up as + missing expected errors. + +--- + +### `GridBuilderFilterIsPartOfResourceClass` + +- **Namespace**: `bitExpert\PHPStan\Sylius\Rule\Grid` +- **Node type**: `CollectedDataNode` +- **Collectors used**: + - `CollectRessourceClassForGridClass` → `gridClass → resourceClass` + - `CollectFilterForGridClass` → `gridClass → [filterFieldList, line]` +- **Validation**: + - Each filter field (from array) checked against resource class. + - **No recursion**: Fields containing `.` (e.g., `address.city`) are **skipped** entirely. +- **Error**: `sylius.grid.resourceClassMissingFilter` → `The filter field "%s" needs to exists as property in resource class "%s".` + +--- + +### `IndexOperationNeedsGridClassRule` + +- **Namespace**: `bitExpert\PHPStan\Sylius\Rule\Resource` +- **Node type**: `InClassNode` +- **Scope**: Classes implementing `Sylius\Resource\Model\ResourceInterface`. +- **Checks**: `#[Sylius\Resource\Metadata\Index(grid: 'App\Grid\MyGrid')]` +- **Error**: `sylius.resource.gridClassNotFound` → `Grid class "%s" not found!` + +--- + +### `ResourceAttributeNeedsFormTypeRule` + +- **Namespace**: `bitExpert\PHPStan\Sylius\Rule\Resource` +- **Node type**: `InClassNode` +- **Scope**: Classes implementing `Sylius\Resource\Model\ResourceInterface`. +- **Checks**: `#[Sylius\Resource\Metadata\AsResource(formType: 'App\Form\MyType')]` +- **Error**: `sylius.resource.formTypeNotFound` → `Form Type "%s" not found!` + +--- + +## Collectors Reference + +### `AbstractGridClassCollector` + +- **Purpose**: Base helper for detecting grid classes. +- **Key methods**: + - `scopeIsGrid(Scope $scope): bool` + - Checks for `#AsGrid` attribute OR subclass of `Sylius\Bundle\GridBundle\Grid\AbstractGrid`. + - Catches any `Throwable` to avoid crashes. + - `isSubtypeOf(Type $type, string $superType): bool` + - Returns `true` if `$type` is a subtype of `$superType`. +- **Usage**: All grid collectors extend this. +- **Note**: `scopeIsGrid()` does not match traits analysed on their own, because the scope class is the + trait itself. Only fields declared in a class/trait that a real grid uses are collected. + +--- + +### `CollectRessourceClassForGridClass` + +- **Namespace**: `bitExpert\PHPStan\Sylius\Collector\Grid` +- **Type**: `Collector` +- **Returns**: `[gridClass, resourceClass, line]` +- **Logic**: + 1. Check `#AsGrid(resourceClass: ...)` attribute for constant string. + 2. Fallback to `getResourceClass()` method returning `String_` or `ClassConstFetch`. +- **Note**: Typo in class name (`Ressource` with double `s`). + +--- + +### `CollectFieldsForGridClass` + +- **Type**: `Collector` +- **Validates node**: + - Must be a static **`create()` or `createForService()`** call (`FACTORY_METHODS`). + `CallableField::createForService()` is the only alternative factory in grid-bundle. + - Must be inside grid scope (`scopeIsGrid`). + - Return type must be subtype of `Sylius\Component\Grid\Builder\Field\FieldInterface` (new) **or** `Sylius\Bundle\GridBundle\Builder\Field\FieldInterface` (old). +- **Field resolution**: + - Iterates `FieldRegistry` entries (ordered by service registration). + - First `supports()` match wins. + - Returns only the **first field name** from `getFieldNames()` (even if multiple returned). +- **Special handling**: If field name is `.` (object passed to field), returns `null` (ignored). + +--- + +### `CollectFilterForGridClass` + +- **Type**: `Collector, -1|int<1, max>}>` (doc declares `-1|positive int` but actual line numbers are positive) +- **Validates node**: + - Static `create()` call. + - Grid scope. + - Return type subtype of `Sylius\Component\Grid\Builder\Filter\FilterInterface` (new) **or** `Sylius\Bundle\GridBundle\Builder\Filter\FilterInterface` (old). +- **Filter resolution**: + - Iterates `FilterRegistry` entries. + - First `supports()` match wins. + - Returns `[gridClass, array of field names, line]`. +- **Key**: Supports array of field names (e.g., `StringFilter::create('name', ['field1', 'field2'])`). + +--- + +## Field Registry & Field Nodes + +### Field Registry System + +- **Factory**: `FieldRegistryFactory::createRegistry()` uses PHPStan Container to collect all services tagged `phpstan.sylius.grid.field`. +- **Implementation**: `DefaultFieldRegistry` stores an immutable array of `FieldNode[]`. + +### Field Nodes + +All field nodes implement `FieldNode`: + +```php +interface FieldNode { + public function supports(FullyQualified $nodeClass): bool; + public function getFieldNames(StaticCall $node): array; // snake_case → camelCase +} +``` + +#### Built-in Field Nodes + +| Class | Sylius Class Name | Argument(s) Processed | Conversion | +|-------|-------------------|-----------------------|------------| +| `StringFieldNode` | `Sylius\Bundle\GridBundle\Builder\Field\StringField` | args[0] string | snake → camel | +| `DateTimeFieldNode` | `Sylius\Bundle\GridBundle\Builder\Field\DateTimeField` | args[0] string | snake → camel | +| `TwigFieldNode` | `Sylius\Bundle\GridBundle\Builder\Field\TwigField` | args[0] string | snake → camel | +| `EnumFieldNode` | `Sylius\Bundle\GridBundle\Builder\Field\EnumField` | args[0] string | snake → camel | +| `CallableFieldNode` | `Sylius\Bundle\GridBundle\Builder\Field\CallableField` | args[0] string | snake → camel | +| `GenericFieldNode` | `Sylius\Bundle\GridBundle\Builder\Field\Field` | args[0] string | snake → camel | + +#### Important Notes + +- Matching the **bundle** namespace is correct on *both* grid-bundle 1.15 and 1.16 — see + [Grid Bundle 1.16](#grid-bundle-116). Do not "fix" these to the `Component` namespace; there is + nothing there to match. +- The conversion helper `PropertyName::convertSnakeToCamelCase()` lowercases the entire string before shifting, so `My_Field` → `myField`. + +--- + +## Filter Registry & Filter Nodes + +### Filter Registry System + +- **Factory**: `FilterRegistryFactory::createRegistry()` collects services tagged `phpstan.sylius.grid.filter`. +- **Implementation**: `DefaultFilterRegistry` stores `FilterNode[]`. + +### Filter Nodes + +All filter nodes implement `FilterNode`: + +```php +interface FilterNode { + public function supports(FullyQualified $nodeClass): bool; + public function getFilterFields(StaticCall $node): array; // snake_case → camelCase +} +``` + +#### Built-in Filter Nodes + +Signatures below are the real grid-bundle ones; the `create()` first argument is always the filter +name, and every node falls back to it. + +| Class | Sylius Class Name | `create()` signature | Field Argument | +|-------|-------------------|----------------------|----------------| +| `EntityFilter` | `...Builder\Filter\EntityFilter` | `create(string $name, string $resourceClass, ?bool $multiple = null, ?array $fields = null)` | args[3] if array of strings; fallback args[0] | +| `EnumFilter` | `...Builder\Filter\EnumFilter` | `create(string $name, string $enumClass, ?bool $multiple = null, ?string $field = null)` | args[3] if string; fallback args[0] | +| `BooleanFilter` | `...Builder\Filter\BooleanFilter` | `create(string $name)` | args[0] | +| `DateFilter` | `...Builder\Filter\DateFilter` | `create(string $name)` | args[0] | +| `MoneyFilter` | `...Builder\Filter\MoneyFilter` | `create(string $name, string $currencyCode, ?int $scale = null)` | args[0] (currency + scale ignored) | +| `ExistsFilter` | `...Builder\Filter\ExistsFilter` | `create(string $name, ?string $field = null)` | args[1] if string; fallback args[0] | +| `SelectFilter` | `...Builder\Filter\SelectFilter` | `create(string $name, array $choices, ?bool $multiple = null, ?string $field = null)` | args[3] if string; fallback args[0] | +| `StringFilter` | `...Builder\Filter\StringFilter` | `create(string $name, ?array $fields = null, $type = null)` | args[1] if array of strings; fallback args[0] | +| `Filter` (catch-all) | matches anything that **implements** `FilterInterface` (old *or* new) | `create(string $name, string $type = null)` | args[0] only | + +#### Important Notes + +- **Registry order matters: keep the catch-all `Filter` node last.** First `supports()` match wins, so an + earlier catch-all shadows any node registered after it — in particular a user's custom node. Only the + catch-all is interface-based; the others match one concrete class name each, so the built-in set of + outcomes is order-independent. `GridBuilderFilterIsPartOfResourceClassUnitTest` drives both orders + against `grid_custom_filter.php` to show which node wins; keeping the shipped order in + `extension.neon` correct is a manual job. +- The catch-all is the *only* interface-based node, and only `Filter` itself implements `FilterInterface`. + `StringFilter`, `BooleanFilter` etc. are plain `final class`es with a static `create()` returning the + interface — they are matched by name, never by the catch-all. Anything without a dedicated node is + silently unvalidated, which is exactly how `BooleanFilter`/`DateFilter`/`MoneyFilter` went unnoticed. +- Recursive filter fields (e.g., `address.city`) are **skipped** by the rule, not by the collector. + +--- + +## Grid Bundle 1.16 + +grid-bundle 1.16 moved the *interfaces* into `Sylius\Component\Grid`, but **every concrete +`create()` factory class still lives in `Sylius\Bundle\GridBundle\Builder\...`** and returns the new +interface. In `Sylius\Component\Grid\Builder\Field\` and `...\Filter\` there are only the +`*Interface` files — no concrete field or filter classes exist there at all. + +Consequences: +- Name-based nodes must keep matching the **bundle** namespace on both lanes. +- The catch-all `Filter` node must check **both** interfaces, because a legacy `Filter::create()` call + on 1.16 returns the new `Sylius\Component\Grid\Builder\Filter\FilterInterface`. With only the legacy + interface it matches nothing on 1.16. +- Fixtures using new-namespace concrete classes would not even parse, so + `grid_needs_resource_model_native_interface.php` is excluded from PHPStan in `phpstan.dist.neon` + (it only parses on the `>= 1.16` lane). + +--- + +## Utility + +### `PropertyName::convertSnakeToCamelCase` + +- **Signature**: `static function convertSnakeToCamelCase(string $string): string` +- **Behavior**: + - If no `_`, returns input unchanged. + - Splits on `_`, lowercases each part, makes first part lowercase, rest `ucfirst`. + - Example: `user_profile_image` → `userProfileImage`. +- **Usage**: All field/filter nodes call this on the first string argument. + +--- + +## Configuration & Extension Points + +### `extension.neon` + +- **Rules** (registered with PHPStan): + ```neon + rules: + - bitExpert\PHPStan\Sylius\Rule\Grid\ResourceAwareGridNeedsResourceClass + - bitExpert\PHPStan\Sylius\Rule\Grid\GridBuilderFieldIsPartOfResourceClass + - bitExpert\PHPStan\Sylius\Rule\Grid\GridBuilderFilterIsPartOfResourceClass + - bitExpert\PHPStan\Sylius\Rule\Resource\IndexOperationNeedsGridClassRule + - bitExpert\PHPStan\Sylius\Rule\Resource\ResourceAttributeNeedsFormTypeRule + ``` +- **Services**: + - `FieldRegistryFactory` → `syliusFieldTypeRegistry` via `createRegistry()`. + - `FilterRegistryFactory` → `syliusFilterTypeRegistry`. + - Collectors (`CollectRessourceClassForGridClass`, `CollectFieldsForGridClass`, `CollectFilterForGridClass`) tagged `phpstan.collector`. + - All field/filter nodes tagged with `phpstan.sylius.grid.field` / `phpstan.sylius.grid.filter`. + - Filter nodes are registered specific-first; the catch-all `Filter` node is deliberately last. + +### Custom Field/Filter Types + +To add custom types: + +1. Implement `FieldNode` or `FilterNode`. +2. Register service in `phpstan.neon` with the appropriate tag. +3. Register it **before** the catch-all `Filter` node if you are also adding an interface-based node. + +Example (custom filter node): +```neon +services: + - class: App\PHPStan\CustomFilterNode + tags: + - phpstan.sylius.grid.filter +``` + +`supports()` should match on the concrete class name (`$nodeClass->name`) like the built-in nodes do. +Prefer that over an interface check: interface-based nodes compete with the catch-all. + +--- + +## Testing + +### Test Infrastructure + +- **Framework**: PHPUnit 11. +- **Configuration**: `phpunit.xml.dist` (suffix `UnitTest.php`, bootstrap `tests/bootstrap.php`). +- **Rules** use `PHPStan\Testing\RuleTestCase`. +- **Utility/Registry** use standard `PHPUnit\Framework\TestCase`. +- 17 tests total across both lanes. + +#### Gotchas that make tests lie + +- **`RuleTestCase` asserts only `line: message`, never the file.** A wrong `->file()` in a rule cannot be + caught by these tests. Do not add a test that appears to cover file attribution. +- **`phpunit.xml.dist` sets `stopOnFailure="true"`.** A failing run reports only the tests up to the first + failure, so a low test count is not a discovery problem — confirm with `--list-tests`. +- **Test registries are hand-built.** Each grid rule test overrides `getCollectors()` and assembles its own + node list. Registering a node in `extension.neon` alone changes nothing in the tests, so a green suite can + pass while the new node is never exercised. Update both, and prove the node is load-bearing by pointing + one `FILTER_TYPE`/`FIELD_TYPE` at a wrong class name and confirming the expectation disappears. Keep the + hand-built list in the same order as `extension.neon`, since that is the order the collector really uses. +- **Do not bulk-shift fixture line numbers with sequential `str.replace`.** Replacing `46→49` and then + `49→52` re-edits the value just written and shifts the wrong entries. Re-derive line numbers from the file. + +### Test Files + +| Test File | Purpose | Fixtures | +|-----------|---------|----------| +| `PropertyNameUnitTest` | Unit tests for snake → camel conversion | None | +| `ResourceAttributeNeedsFormTypeUnitTest` | Validates `AsResource` form type existence | `Rule/Resource/data/entity.php`, `entity_non_constant_attribute.php` | +| `IndexOperationNeedsGridClassUnitTest` | Validates `Index` grid reference | `Rule/Resource/data/entity_index.php` | +| `ResourceAwareGridNeedsResourceClassUnitTest` | Missing resource class (old, attribute, 1.16-native, method-less) | `grid_needs_resource_model.php`, `grid_needs_resource_model_attr.php`, `grid_needs_resource_model_native_interface.php`, `grid_needs_resource_model_no_methods.php` | +| `ResourceAwareGridNeedsResourceClassValidUnitTest` | Validates correct configurations | `grid_valid.php`, `grid_valid_attr.php` | +| `GridBuilderFieldIsPartOfResourceClassUnitTest` | Invalid field detection (incl. `createForService`) | `grid.php` | +| `GridBuilderFieldIsPartOfResourceClassValidUnitTest` | Validates correct grids | `grid_valid.php` | +| `GridBuilderFilterIsPartOfResourceClassUnitTest` | Invalid filter detection, plus registry-order (catch-all vs. dedicated node) | `grid.php`, `grid_custom_filter.php`, `custom_filter.php` | +| `GridBuilderFilterIsPartOfResourceClassValidUnitTest` | Validates correct grids | `grid_valid.php` | +| `DefaultFilterRegistryUnitTest` | Registry functionality | None | + +### Fixtures + +All fixtures (for analysis) reside under `tests/bitExpert/PHPStan/Sylius/Rule/*/data/`. + +`Rule/Grid/data/` (namespace `App\Entity` / `App\Grid`): +- `entity.php`: `App\Entity\Status` (enum), `Country`, `Address`, `Supplier` (implements `ResourceInterface`, missing `name`). `Country` and `Address::$country`/`getCountry()` back the three-segment field case. +- `grid.php`: `AdminSupplierGrid` with invalid field/filter definitions, plus `SomeOtherClass`. Shared by the field *and* filter tests, so any edit shifts expectations in both. +- `grid_valid.php` / `grid_valid_attr.php`: `GridValid` / `GridValidAttr`, correct grid configurations. +- `grid_needs_resource_model.php` / `_attr.php` / `_no_methods.php` / `_native_interface.php`: resource-class detection cases. +- `custom_filter.php`: namespace `App\Filter`, declaring `class CustomFilter`; a user-style filter that implements `FilterInterface`, so the catch-all matches it by interface. Implements every method for real so PHPStan verifies the signatures on both lanes. +- `grid_custom_filter.php`: `GridCustomFilter`, holding the one `CustomFilter::create()` call whose extracted field differs depending on which node wins. + +`Rule/Resource/data/` (namespace `App\Entity`): +- `entity.php`: `Entity`, implementing `ResourceInterface`. +- `entity_index.php`: `EntityIndex`, missing `Index(grid:)`. +- `entity_non_constant_attribute.php`: `EntityWithNonConstantFormType`, `EntityWithArrayFormType`, `EntityWithIntFormType`, `EntityWithNonConstantGrid`, `EntityWithArrayGrid`, `EntityWithConstantFormType` — non-constant attribute arguments, which the rules must skip silently. + +These files are loaded via `composer.json` `autoload-dev.files` so classes exist during analysis, and +`Rule/Grid/data/grid.php` + `entity.php` + `grid_custom_filter.php` are excluded from PHPStan analysis +in `phpstan.dist.neon`. `custom_filter.php` is deliberately **not** excluded: it declares no invalid +references, so analysing it is what proves the interface implementation still holds on grid-bundle 1.16. + +--- + +## Development Commands + +From `composer.json`: + +| Command | Description | +|---------|-------------| +| `composer cs` | Check coding standards (dry-run) | +| `composer cs-fix` | Fix coding standards | +| `composer check-license` | Verify license compliance | +| `composer static-analysis` | Run PHPStan (uses `phpstan.dist.neon`) | +| `composer test` | Run PHPUnit tests | + +### CI Pipeline (`.github/workflows/ci.yml`) + +Matrix over PHP version × OS × grid-bundle constraint. Each lane runs: + +1. Checkout repo +2. Configure PHP (`shivammathur/setup-php`) +3. `composer require --dev "sylius/grid-bundle:" --no-update` + `composer update` +4. Show resolved versions — **one `composer show ` call per package**; `composer show` takes a single + package plus an optional version, so passing several names exits `1` +5. `composer check-license` +6. `composer cs` +7. `composer static-analysis` +8. `composer test` + +The two grid-bundle lanes are `^1.15 <1.16` and `^1.16`. Keep both green; a change that only passes the +second lane is not done. + +### Coding Standards + +- **Tool**: PHP-CS-Fixer +- **Config**: `.php-cs-fixer.dist.php` +- **Rules**: `@Symfony`, `@Symfony:risky`, strict comparison/param, declare strict, ordered imports, multi-line extends, trailing commas, etc. +- **Excludes**: `vendor/` + +--- + +## Known Issues & Gotchas + +### 1. Typo in Collector Name +- **File**: `CollectRessourceClassForGridClass.php` (`Ressource` double `s`). +- **Impact**: Must be preserved for BC; do not rename. + +### 2. A redundant `instanceof` guard can hide a wrong `@implements` +- `processNode()` must declare `Node $node` because `Rule::processNode()` does, so the generic in + `@implements` is the only thing that narrows `$node` inside the method body. +- Both grid rules used to open with `if (!$node instanceof CollectedDataNode) { return []; }`. That + re-narrowing is exactly what **suppressed** their wrong `@implements Rule` docblocks from + ever reaching PHPStan. Deleting the guard without fixing the docblock turns + `$node->get()` into `Call to an undefined method PhpParser\Node::get()`. +- **Action**: when adding a rule, keep `@implements` and `getNodeType()` in agreement, and skip the + defensive `instanceof` guard — it can mask a real annotation mistake instead of catching one. Every + rule's `getNodeType()` now carries `@return class-string`, so PHPStan rejects a + mismatch between the two; do not widen that back to a bare `class-string`, or the safety net goes away. + +### 3. Node/collector coverage is opt-in +- A field or filter class with **no** matching node is silently never validated — no error, no warning. + The catch-all only covers classes that implement `FilterInterface`, and only `Filter` itself does. +- **Impact**: `BooleanFilter`, `DateFilter` and `MoneyFilter` were unvalidated until nodes were added for + them. When a new grid-bundle factory appears, add a node in the same change, and check the grid-bundle + changelog when bumping the floor. + +### 4. Recursive Filter Fields Skipped +- The rule `GridBuilderFilterIsPartOfResourceClass` skips any filter field containing `.` (e.g., `address.city`). +- **Rationale**: Filters typically do not need recursive access, but this may be overly restrictive. +- **Consequence**: Users cannot validate recursive filter fields. + +### 5. Field errors are attributed to the grid's file, not the node's +- Both grid rules use `$gridFilesMap[$gridClass]`, which is **last-write-wins per grid class** and shared + by all fields of that grid. +- **Impact**: A grid split across multiple files reports every field error against whichever file was seen + last. Fields inside a trait are always attributed to the using class, because the node's real file is not + recoverable: PHP-Parser 5 exposes only an integer `startFilePos` (no path), and `Scope::getFile()`, + `getFunction()->getFileName()` and `->getDeclaringClass()->getFileName()` all return the using class file. +- **Action**: not fixable without a broader file-identity design; `RuleTestCase` cannot even assert it. + +### 6. `data/` fixtures are not PSR-4 and are excluded from PHP-CS-Fixer +- Fixtures under `tests/**/data/` carry **no license header** (only `src/` is header-checked) and are + registered in `composer.json` `autoload-dev.files`, so they are loaded by inclusion, not by class name. + PSR-4 file-name/class-name pairing therefore does not apply, and the file name deliberately stays + snake_case (`grid_valid.php`) while the class is PSR-1 (`GridValid`). +- That combination is exactly what `@Symfony`'s `psr_autoloading` rule "fixes": its matcher compares + class and file name **case-insensitively**, so `class GridValid` in `grid_valid.php` gets rewritten + to `class grid_valid`. `.php-cs-fixer.dist.php` therefore excludes both `Rule/Grid/data` and + `Rule/Resource/data` from the finder. Do not drop those exclusions: `composer cs` is a CI step, and a + local `cs-fix` would rewrite fixture classes, breaking the references between fixtures. +- The failure mode is a *missing* error, not a wrong one. A renamed fixture class makes the call site + name a non-existent class, `CollectFilterForGridClass` can no longer resolve a `FilterInterface` + return type, the call is silently skipped, and the test fails only on the missing expected error. So a + green-looking `composer cs-fix` followed by a red test usually means the fixer edited a fixture. +- **Action**: fixture file names stay snake_case and lowercase, fixture classes are always PSR-1. When + renaming either one, update all of them together — the class declaration, every call site and `use` + statement in sibling fixtures, and any class name a test asserts or matches on + (e.g. `'App\Filter\CustomFilter'` in the filter ordering tests). + +--- + +## Extending the Extension + +### Adding a New Rule + +1. Create rule class in `src/bitExpert/PHPStan/Sylius/Rule/...`. +2. Implement `PHPStan\Rules\Rule` — make `@implements` and `getNodeType()` agree. +3. Register in `extension.neon` under `rules`. +4. Add test in `tests/.../Rule/.../...UnitTest.php`. +5. Add fixture if needed. + +### Adding a New Collector + +1. Implement `PHPStan\Collectors\Collector`. +2. Extend `AbstractGridClassCollector` for grid-related logic. +3. Register in `extension.neon` with `phpstan.collector` tag. +4. Use in rule(s) by requesting collected data via `CollectedDataNode`. + +### Adding a New Field/Filter Node + +1. Implement `FieldNode` or `FilterNode`. +2. Tag service in `extension.neon` with `phpstan.sylius.grid.field` or `phpstan.sylius.grid.filter`. +3. Ensure `supports()` correctly checks the **concrete** class name. +4. Register before the catch-all `Filter` node. +5. Add the node to the affected tests' `getCollectors()` too, then prove it is load-bearing with a + deliberately wrong class name. + +--- + +## References + +- **PHPStan Docs**: [https://phpstan.org](https://phpstan.org) +- **Sylius Resource Bundle**: [https://github.com/Sylius/ResourceBundle](https://github.com/Sylius/ResourceBundle) +- **Sylius Grid Bundle**: [https://github.com/Sylius/GridBundle](https://github.com/Sylius/GridBundle) + +--- + +*Last updated: 2026-09-26* diff --git a/README.md b/README.md index 0b366ac..5ac22e3 100644 --- a/README.md +++ b/README.md @@ -37,14 +37,12 @@ This PHPStan extension works for both Sylius plugins and Sylius application proj The following rules have been implemented: - Rule to check if resource classes defined either via AbstractGrid::getResourceClass() or the #AsGrid attribute exist - Rule to check that configured grid fields belong to the configured resource class +- - custom field types are supported - Rule to check that configured filter fields belong to the configured resource class - custom filter types are supported - Rule to check that grid class configured via the `Index` attribute exists - Rule to check that form type configured via the `AsResource` attribute exists -Current assumptions: -- Grids are configured by extending the `Sylius\Bundle\GridBundle\Grid\AbstractGrid` class - ### Custom filter types To include your custom filter type in the checks, implement the `bitExpert\PHPStan\Sylius\Collector\Grid\Filter\FilterNode` interface and add a service to your `phpstan.neon` file and tag it with the `phpstan.sylius.grid.filter` tag. diff --git a/composer.json b/composer.json index 755622b..a34d55d 100644 --- a/composer.json +++ b/composer.json @@ -16,7 +16,7 @@ }, "require-dev": { "sylius/resource-bundle": "^1.12", - "sylius/grid-bundle": "^1.13", + "sylius/grid-bundle": "^1.15", "nikic/php-parser": "^5.4", "phpunit/phpunit": "^11.5", "friendsofphp/php-cs-fixer": "^3.69", @@ -34,9 +34,15 @@ "files": [ "tests/bitExpert/PHPStan/Sylius/Rule/Grid/data/entity.php", "tests/bitExpert/PHPStan/Sylius/Rule/Grid/data/grid.php", + "tests/bitExpert/PHPStan/Sylius/Rule/Grid/data/custom_filter.php", + "tests/bitExpert/PHPStan/Sylius/Rule/Grid/data/grid_custom_filter.php", "tests/bitExpert/PHPStan/Sylius/Rule/Grid/data/grid_needs_resource_model.php", "tests/bitExpert/PHPStan/Sylius/Rule/Grid/data/grid_needs_resource_model_attr.php", - "tests/bitExpert/PHPStan/Sylius/Rule/Resource/data/entity.php" + "tests/bitExpert/PHPStan/Sylius/Rule/Grid/data/grid_needs_resource_model_no_methods.php", + "tests/bitExpert/PHPStan/Sylius/Rule/Grid/data/grid_needs_resource_model_native_interface.php", + "tests/bitExpert/PHPStan/Sylius/Rule/Resource/data/entity.php", + "tests/bitExpert/PHPStan/Sylius/Rule/Resource/data/entity_index.php", + "tests/bitExpert/PHPStan/Sylius/Rule/Resource/data/entity_non_constant_attribute.php" ] }, "extra": { diff --git a/composer.lock b/composer.lock index ce58728..7a5b22b 100644 --- a/composer.lock +++ b/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", "This file is @generated automatically" ], - "content-hash": "67e6e8576547503d886971e1594d1c53", + "content-hash": "8d1c1b52ac01881ae4b0d930d5158e1f", "packages": [ { "name": "phpstan/phpstan", diff --git a/extension.neon b/extension.neon index fc85a66..81e1761 100644 --- a/extension.neon +++ b/extension.neon @@ -66,11 +66,19 @@ services: tags: - phpstan.sylius.grid.filter - - class: bitExpert\PHPStan\Sylius\Collector\Grid\Filter\ExistsFilter + class: bitExpert\PHPStan\Sylius\Collector\Grid\Filter\BooleanFilter tags: - phpstan.sylius.grid.filter - - class: bitExpert\PHPStan\Sylius\Collector\Grid\Filter\Filter + class: bitExpert\PHPStan\Sylius\Collector\Grid\Filter\DateFilter + tags: + - phpstan.sylius.grid.filter + - + class: bitExpert\PHPStan\Sylius\Collector\Grid\Filter\MoneyFilter + tags: + - phpstan.sylius.grid.filter + - + class: bitExpert\PHPStan\Sylius\Collector\Grid\Filter\ExistsFilter tags: - phpstan.sylius.grid.filter - @@ -81,3 +89,10 @@ services: class: bitExpert\PHPStan\Sylius\Collector\Grid\Filter\StringFilter tags: - phpstan.sylius.grid.filter + + # Registered last on purpose: this node matches every filter that + # implements FilterInterface, so all concrete nodes have to be asked first. + - + class: bitExpert\PHPStan\Sylius\Collector\Grid\Filter\Filter + tags: + - phpstan.sylius.grid.filter diff --git a/phpstan.dist.neon b/phpstan.dist.neon index c998124..f311302 100644 --- a/phpstan.dist.neon +++ b/phpstan.dist.neon @@ -1,6 +1,5 @@ parameters: level: max - reportUnmatchedIgnoredErrors: false treatPhpDocTypesAsCertain: false paths: - src @@ -8,18 +7,15 @@ parameters: excludePaths: - tests/bitExpert/PHPStan/Sylius/Rule/Grid/data/entity.php - tests/bitExpert/PHPStan/Sylius/Rule/Grid/data/grid.php + - tests/bitExpert/PHPStan/Sylius/Rule/Grid/data/grid_custom_filter.php - tests/bitExpert/PHPStan/Sylius/Rule/Grid//data/grid_needs_resource_model.php - tests/bitExpert/PHPStan/Sylius/Rule/Grid//data/grid_needs_resource_model_attr.php + - tests/bitExpert/PHPStan/Sylius/Rule/Grid//data/grid_needs_resource_model_no_methods.php + # Only parses on grid-bundle >= 1.16, so it must not be statically analysed + # against the >= 1.15 lane. + - tests/bitExpert/PHPStan/Sylius/Rule/Grid//data/grid_needs_resource_model_native_interface.php - tests/bitExpert/PHPStan/Sylius/Rule/Resource/data/entity.php + - tests/bitExpert/PHPStan/Sylius/Rule/Resource/data/entity_index.php + - tests/bitExpert/PHPStan/Sylius/Rule/Resource/data/entity_non_constant_attribute.php fileExtensions: - php - ignoreErrors: - - - message: '~Cannot cast mixed to string.~' - path: src/bitExpert/PHPStan/Sylius/Rule/Grid/AbstractGridBuilderRule.php - - - message: '~Cannot call~' - path: src/bitExpert/PHPStan/Sylius/Rule/Grid/GridBuilderFieldIsPartOfResourceClass.php - - - message: '~Cannot call~' - path: src/bitExpert/PHPStan/Sylius/Rule/Grid/GridBuilderFilterIsPartOfResourceClass.php diff --git a/src/bitExpert/PHPStan/Sylius/Collector/Grid/CollectFieldsForGridClass.php b/src/bitExpert/PHPStan/Sylius/Collector/Grid/CollectFieldsForGridClass.php index c4e5296..002aacf 100644 --- a/src/bitExpert/PHPStan/Sylius/Collector/Grid/CollectFieldsForGridClass.php +++ b/src/bitExpert/PHPStan/Sylius/Collector/Grid/CollectFieldsForGridClass.php @@ -27,6 +27,13 @@ */ final class CollectFieldsForGridClass extends AbstractGridClassCollector implements Collector { + /** + * Grid fields are built through a static factory. Every field class uses create(), + * and CallableField additionally offers createForService() for a service-backed + * callable. Both take the field name as their first argument. + */ + private const FACTORY_METHODS = ['create', 'createForService']; + public function __construct(private readonly FieldRegistry $fieldRegistry) { } @@ -45,7 +52,7 @@ public function processNode(Node $node, Scope $scope): ?array return null; } - if ((!$node->name instanceof Identifier) || ('create' !== $node->name->toString())) { + if (!$node->name instanceof Identifier || !\in_array($node->name->toString(), self::FACTORY_METHODS, true)) { return null; } diff --git a/src/bitExpert/PHPStan/Sylius/Collector/Grid/Filter/BooleanFilter.php b/src/bitExpert/PHPStan/Sylius/Collector/Grid/Filter/BooleanFilter.php new file mode 100644 index 0000000..2ccbaa8 --- /dev/null +++ b/src/bitExpert/PHPStan/Sylius/Collector/Grid/Filter/BooleanFilter.php @@ -0,0 +1,43 @@ +name; + } + + public function getFilterFields(StaticCall $node): array + { + $filterFields = []; + + if (isset($node->args[0])) { + $arg = $node->args[0]; + if ($arg instanceof Arg && $arg->value instanceof String_) { + $filterFields[] = PropertyName::convertSnakeToCamelCase($arg->value->value); + } + } + + return $filterFields; + } +} diff --git a/src/bitExpert/PHPStan/Sylius/Collector/Grid/Filter/DateFilter.php b/src/bitExpert/PHPStan/Sylius/Collector/Grid/Filter/DateFilter.php new file mode 100644 index 0000000..e30f54a --- /dev/null +++ b/src/bitExpert/PHPStan/Sylius/Collector/Grid/Filter/DateFilter.php @@ -0,0 +1,43 @@ +name; + } + + public function getFilterFields(StaticCall $node): array + { + $filterFields = []; + + if (isset($node->args[0])) { + $arg = $node->args[0]; + if ($arg instanceof Arg && $arg->value instanceof String_) { + $filterFields[] = PropertyName::convertSnakeToCamelCase($arg->value->value); + } + } + + return $filterFields; + } +} diff --git a/src/bitExpert/PHPStan/Sylius/Collector/Grid/Filter/Filter.php b/src/bitExpert/PHPStan/Sylius/Collector/Grid/Filter/Filter.php index bc4c0e3..4dcd59e 100644 --- a/src/bitExpert/PHPStan/Sylius/Collector/Grid/Filter/Filter.php +++ b/src/bitExpert/PHPStan/Sylius/Collector/Grid/Filter/Filter.php @@ -21,15 +21,26 @@ final readonly class Filter implements FilterNode { - private const FILTER_TYPE = 'Sylius\\Bundle\\GridBundle\\Builder\\Filter\\FilterInterface'; + /** + * Both the old bundle interface and the one introduced in 1.16. The concrete + * grid-bundle filter factories return the new interface from 1.16 on, so + * checking only the old one would make this node silently stop matching. + */ + private const FILTER_TYPES = [ + 'Sylius\\Bundle\\GridBundle\\Builder\\Filter\\FilterInterface', + 'Sylius\\Component\\Grid\\Builder\\Filter\\FilterInterface', + ]; public function supports(FullyQualified $nodeClass): bool { try { - $filterType = new ObjectType(self::FILTER_TYPE); $nodeClassType = new ObjectType($nodeClass->toString()); - return $nodeClassType->isSuperTypeOf($filterType)->yes(); + foreach (self::FILTER_TYPES as $filterType) { + if ((new ObjectType($filterType))->isSuperTypeOf($nodeClassType)->yes()) { + return true; + } + } } catch (\Throwable $e) { } diff --git a/src/bitExpert/PHPStan/Sylius/Collector/Grid/Filter/MoneyFilter.php b/src/bitExpert/PHPStan/Sylius/Collector/Grid/Filter/MoneyFilter.php new file mode 100644 index 0000000..57e4e4a --- /dev/null +++ b/src/bitExpert/PHPStan/Sylius/Collector/Grid/Filter/MoneyFilter.php @@ -0,0 +1,45 @@ +name; + } + + public function getFilterFields(StaticCall $node): array + { + $filterFields = []; + + // create(string $name, string $currencyCode, ?int $scale = null): + // the currency code is the second argument, the field is the first one. + if (isset($node->args[0])) { + $arg = $node->args[0]; + if ($arg instanceof Arg && $arg->value instanceof String_) { + $filterFields[] = PropertyName::convertSnakeToCamelCase($arg->value->value); + } + } + + return $filterFields; + } +} diff --git a/src/bitExpert/PHPStan/Sylius/Rule/Grid/GridBuilderFieldIsPartOfResourceClass.php b/src/bitExpert/PHPStan/Sylius/Rule/Grid/GridBuilderFieldIsPartOfResourceClass.php index 0b2b4f8..05d5159 100644 --- a/src/bitExpert/PHPStan/Sylius/Rule/Grid/GridBuilderFieldIsPartOfResourceClass.php +++ b/src/bitExpert/PHPStan/Sylius/Rule/Grid/GridBuilderFieldIsPartOfResourceClass.php @@ -15,7 +15,6 @@ use bitExpert\PHPStan\Sylius\Collector\Grid\CollectFieldsForGridClass; use bitExpert\PHPStan\Sylius\Collector\Grid\CollectRessourceClassForGridClass; use PhpParser\Node; -use PhpParser\Node\Expr\StaticCall; use PHPStan\Analyser\Scope; use PHPStan\Node\CollectedDataNode; use PHPStan\Reflection\ClassReflection; @@ -23,9 +22,10 @@ use PHPStan\Reflection\ReflectionProvider; use PHPStan\Rules\Rule; use PHPStan\Rules\RuleErrorBuilder; +use PHPStan\Type\Type; /** - * @implements Rule + * @implements Rule */ readonly class GridBuilderFieldIsPartOfResourceClass implements Rule { @@ -34,7 +34,7 @@ public function __construct(protected ReflectionProvider $broker) } /** - * @return class-string + * @return class-string */ public function getNodeType(): string { @@ -43,10 +43,6 @@ public function getNodeType(): string public function processNode(Node $node, Scope $scope): array { - if (!$node instanceof CollectedDataNode) { - return []; - } - $gridResourceMap = []; $gridFilesMap = []; $gridFieldsMap = []; @@ -83,12 +79,16 @@ public function processNode(Node $node, Scope $scope): array /** @var array $gridResourceMap */ foreach ($gridResourceMap as $gridClassName => $resourceClassName) { if (isset($gridFieldsMap[$gridClassName])) { - $resourceClass = $this->broker->getClass($resourceClassName); - foreach ($gridFieldsMap[$gridClassName] as $field) { $fieldName = $field[0]; $lineNo = $field[1]; + // The resource class has to be re-resolved for every field. The + // recursive branch below walks down the field path and replaces + // this with the type of the last resolved segment, which would + // otherwise leak into the next field of the same grid. + $resourceClass = $this->broker->getClass($resourceClassName); + if (!\str_contains($fieldName, '.')) { // single property check $getterMethod = 'get' . \ucfirst($fieldName); @@ -105,62 +105,54 @@ public function processNode(Node $node, Scope $scope): array ->line($lineNo) ->build(); } - } else { - // recursive property check - $fieldNames = \explode('.', $fieldName); - while (\count($fieldNames) > 0) { - $fieldName = \array_shift($fieldNames); - - $getterMethod = 'get' . \ucfirst($fieldName); - if (!$resourceClass->hasProperty($fieldName) && !$resourceClass->hasMethod($getterMethod)) { + + continue; + } + + // recursive property check + $fieldNames = \explode('.', $fieldName); + while (\count($fieldNames) > 0) { + $segment = \array_shift($fieldNames); + $getterMethod = 'get' . \ucfirst($segment); + + if (!$resourceClass->hasProperty($segment) && !$resourceClass->hasMethod($getterMethod)) { + $message = \sprintf( + 'The field "%s" needs to exists as property in class "%s".', + $segment, + $resourceClassName, + ); + + $errors[] = RuleErrorBuilder::message($message) + ->identifier('sylius.grid.resourceClassMissingProperty') + ->file($gridFilesMap[$gridClassName]) + ->line($lineNo) + ->build(); + } + + // Resolve the type of this segment to keep walking. It has to + // stay a ClassReflection: Type::hasProperty() answers with a + // TrinaryLogic, and casting that object to bool is always + // true, so every later segment would silently pass. + $nextClass = $this->resolveNextClass($resourceClass, $segment, $getterMethod, $scope); + if (null === $nextClass) { + if (\count($fieldNames) > 0) { $message = \sprintf( - 'The field "%s" needs to exists as property in class "%s".', - $fieldName, - $resourceClassName, + 'Unable to identify the type of the field "%s" in class "%s".', + $segment, + $resourceClass->getName(), ); $errors[] = RuleErrorBuilder::message($message) - ->identifier('sylius.grid.resourceClassMissingProperty') + ->identifier('sylius.grid.resourceClassPropertyMissingType') ->file($gridFilesMap[$gridClassName]) ->line($lineNo) ->build(); } - try { - $resourceClass = $resourceClass->getMethod($getterMethod, $scope)->getOnlyVariant()->getReturnType(); - } catch (\Exception $e) { - try { - $property = $resourceClass->getProperty($fieldName, $scope); - if ($property->hasNativeType()) { - $resourceClass = $property->getNativeType(); - } elseif ($property->hasPHPDocType()) { - $resourceClass = $property->getPhpDocType(); - } elseif (\count($fieldNames) > 0) { - /** @var ClassReflection $resourceClass */ - $message = \sprintf( - 'Unable to identify the type of the field "%s" in class "%s".', - $fieldName, - $resourceClass->getName(), - ); - - $errors[] = RuleErrorBuilder::message($message) - ->identifier('sylius.grid.resourceClassPropertyMissingType') - ->file($gridFilesMap[$gridClassName]) - ->line($lineNo) - ->build(); - } - } catch (\Exception $e) { - /* @phpstan-ignore phpstanApi.class */ - if ($e instanceof MissingPropertyFromReflectionException) { - $errors[] = RuleErrorBuilder::message($e->getMessage()) - ->identifier('sylius.grid.resourceClassPropertyMissingType') - ->file($gridFilesMap[$gridClassName]) - ->line($lineNo) - ->build(); - } - } - } + break; } + + $resourceClass = $nextClass; } } } @@ -168,4 +160,51 @@ public function processNode(Node $node, Scope $scope): array return $errors; } + + /** + * Resolves the type a single field path segment points at, as a ClassReflection + * so the next segment can be looked up on it. Returns null when the type cannot + * be narrowed down to exactly one class. + */ + private function resolveNextClass( + ClassReflection $class, + string $segment, + string $getterMethod, + Scope $scope, + ): ?ClassReflection { + try { + return $this->toClassReflection($class->getMethod($getterMethod, $scope)->getOnlyVariant()->getReturnType()); + } catch (\Exception) { + // The getter is absent or is not resolvable, so fall back to the property. + } + + try { + $property = $class->getProperty($segment, $scope); + if ($property->hasNativeType()) { + return $this->toClassReflection($property->getNativeType()); + } + + if ($property->hasPHPDocType()) { + return $this->toClassReflection($property->getPhpDocType()); + } + } catch (MissingPropertyFromReflectionException) { + // Reported by the caller as a missing property on the parent segment. + } + + return null; + } + + private function toClassReflection(Type $type): ?ClassReflection + { + $classNames = $type->getObjectClassNames(); + if (1 !== \count($classNames)) { + return null; + } + + try { + return $this->broker->getClass($classNames[0]); + } catch (\Throwable) { + return null; + } + } } diff --git a/src/bitExpert/PHPStan/Sylius/Rule/Grid/GridBuilderFilterIsPartOfResourceClass.php b/src/bitExpert/PHPStan/Sylius/Rule/Grid/GridBuilderFilterIsPartOfResourceClass.php index 4284ee7..3fe5016 100644 --- a/src/bitExpert/PHPStan/Sylius/Rule/Grid/GridBuilderFilterIsPartOfResourceClass.php +++ b/src/bitExpert/PHPStan/Sylius/Rule/Grid/GridBuilderFilterIsPartOfResourceClass.php @@ -15,7 +15,6 @@ use bitExpert\PHPStan\Sylius\Collector\Grid\CollectFilterForGridClass; use bitExpert\PHPStan\Sylius\Collector\Grid\CollectRessourceClassForGridClass; use PhpParser\Node; -use PhpParser\Node\Expr\StaticCall; use PHPStan\Analyser\Scope; use PHPStan\Node\CollectedDataNode; use PHPStan\Reflection\ReflectionProvider; @@ -23,7 +22,7 @@ use PHPStan\Rules\RuleErrorBuilder; /** - * @implements Rule + * @implements Rule */ readonly class GridBuilderFilterIsPartOfResourceClass implements Rule { @@ -32,7 +31,7 @@ public function __construct(protected ReflectionProvider $broker) } /** - * @return class-string + * @return class-string */ public function getNodeType(): string { @@ -41,10 +40,6 @@ public function getNodeType(): string public function processNode(Node $node, Scope $scope): array { - if (!$node instanceof CollectedDataNode) { - return []; - } - $gridResourceMap = []; $gridFilesMap = []; $gridFilterFieldsMap = []; diff --git a/src/bitExpert/PHPStan/Sylius/Rule/Grid/ResourceAwareGridNeedsResourceClass.php b/src/bitExpert/PHPStan/Sylius/Rule/Grid/ResourceAwareGridNeedsResourceClass.php index 77e988e..1e55954 100644 --- a/src/bitExpert/PHPStan/Sylius/Rule/Grid/ResourceAwareGridNeedsResourceClass.php +++ b/src/bitExpert/PHPStan/Sylius/Rule/Grid/ResourceAwareGridNeedsResourceClass.php @@ -14,103 +14,191 @@ use PhpParser\Node; use PhpParser\Node\Expr\ClassConstFetch; -use PhpParser\Node\Name\FullyQualified; +use PhpParser\Node\Name; use PhpParser\Node\Scalar\String_; +use PhpParser\Node\Stmt\ClassLike; use PhpParser\Node\Stmt\Return_; use PHPStan\Analyser\Scope; use PHPStan\Broker\ClassNotFoundException; -use PHPStan\Node\MethodReturnStatementsNode; +use PHPStan\Node\InClassNode; +use PHPStan\Reflection\ClassReflection; use PHPStan\Reflection\ReflectionProvider; +use PHPStan\Rules\IdentifierRuleError; use PHPStan\Rules\Rule; use PHPStan\Rules\RuleErrorBuilder; -use PHPStan\Type\ObjectType; +use PHPStan\Type\Type; /** - * @implements Rule + * @implements Rule */ readonly class ResourceAwareGridNeedsResourceClass implements Rule { + private const AS_GRID_ATTRIBUTE = 'Sylius\Component\Grid\Attribute\AsGrid'; + + private const LEGACY_GRID_CLASS = 'Sylius\Bundle\GridBundle\Grid\AbstractGrid'; + + private const LEGACY_RESOURCE_AWARE_INTERFACE = 'Sylius\Bundle\GridBundle\Grid\ResourceAwareGridInterface'; + public function __construct(private ReflectionProvider $broker) { } /** - * @return class-string + * @return class-string */ public function getNodeType(): string { - return MethodReturnStatementsNode::class; + return InClassNode::class; } public function processNode(Node $node, Scope $scope): array { - if (!$node instanceof MethodReturnStatementsNode) { + if (!$node instanceof InClassNode) { return []; } - // run the checks only for subclasses of \Sylius\Bundle\GridBundle\Grid\AbstractGrid - $classReflection = $scope->getClassReflection(); - if (null === $classReflection) { - return []; + $classReflection = $node->getClassReflection(); + $class = $node->getOriginalNode(); + + // The attribute is the explicit opt-in, so it takes precedence and the + // legacy method is not inspected a second time. + $resourceClass = $this->resolveResourceClassFromAttribute($classReflection->getAttributes()); + if (null !== $resourceClass) { + $error = $this->validateResourceClass($resourceClass, self::findAttributeLine($class)); + + return null === $error ? [] : [$error]; } - $parentType = new ObjectType('\Sylius\Bundle\GridBundle\Grid\AbstractGrid'); - $classType = new ObjectType($classReflection->getName()); - if (!$parentType->isSuperTypeOf($classType)->yes()) { - return []; + $resourceClass = $this->resolveResourceClassFromMethod($classReflection, $class, $scope); + if (null !== $resourceClass) { + $error = $this->validateResourceClass($resourceClass, self::findMethodLine($class, 'getResourceClass')); + + return null === $error ? [] : [$error]; } - $resourceClassName = ''; + return []; + } - // new Resource Bundle logic: check the #AsGrid attribute of the class - $attributes = $classReflection->getAttributes(); + /** + * The resource class declared by #[AsGrid(resourceClass: ...)]. This is + * deliberately not gated on the grid class hierarchy: since grid-bundle 1.16 + * a grid may implement Sylius\Component\Grid\GridInterface without extending + * Sylius\Bundle\GridBundle\Grid\AbstractGrid, and the attribute is what makes + * the class a grid in the first place. + * + * @param list<\PHPStan\Reflection\AttributeReflection> $attributes + */ + private function resolveResourceClassFromAttribute(array $attributes): ?string + { foreach ($attributes as $attribute) { - if ('Sylius\Component\Grid\Attribute\AsGrid' === $attribute->getName()) { - $argumentTypes = $attribute->getArgumentTypes(); - if (isset($argumentTypes['resourceClass'])) { - $resourceClassName = $argumentTypes['resourceClass']->getConstantStrings()[0]->getValue(); - break; - } + if (self::AS_GRID_ATTRIBUTE !== $attribute->getName()) { + continue; } - } - // old Resource Bundle logic: find the getResourceClass() method to get the resource class - if (empty($resourceClassName)) { - $methodReflection = $node->getMethodReflection(); - if ('getResourceClass' === $methodReflection->getName()) { - /** @var Return_[] $statements */ - $statements = $node->getStatements(); - if ($statements[0]->expr instanceof String_) { - $resourceClassName = $statements[0]->expr->value; - } elseif ($statements[0]->expr instanceof ClassConstFetch) { - /** @var FullyQualified $class */ - $class = $statements[0]->expr->class; - $resourceClassName = $class->name; - } else { - return []; - } + $argumentTypes = $attribute->getArgumentTypes(); + $resourceClass = self::resolveConstantString($argumentTypes['resourceClass'] ?? null); + if (null !== $resourceClass) { + return $resourceClass; } } - if (empty($resourceClassName)) { - return []; + return null; + } + + /** + * The resource class declared by the pre-1.14 + * ResourceAwareGridInterface::getResourceClass() method. + * + * The returned statement is read from the AST instead of the resolved method + * return type, because a method declared 'getResourceClass(): string' that + * returns Supplier::class has the plain type 'string' whenever PHPDoc types + * are not treated as certain, which would lose the class name. + */ + private function resolveResourceClassFromMethod(ClassReflection $classReflection, ClassLike $class, Scope $scope): ?string + { + $isSyliusGrid = $classReflection->isSubclassOf(self::LEGACY_GRID_CLASS) + || $classReflection->implementsInterface(self::LEGACY_RESOURCE_AWARE_INTERFACE); + if (!$isSyliusGrid) { + return null; + } + + $method = $class->getMethod('getResourceClass'); + if (null === $method) { + return null; + } + + foreach ($method->stmts ?? [] as $statement) { + if (!$statement instanceof Return_ || null === $statement->expr) { + continue; + } + + if ($statement->expr instanceof String_) { + return $statement->expr->value; + } + + if ($statement->expr instanceof ClassConstFetch && $statement->expr->class instanceof Name) { + return $scope->resolveName($statement->expr->class); + } + + return null; } + return null; + } + + private function validateResourceClass(string $resourceClass, int $line): ?IdentifierRuleError + { try { - $resourceClass = $this->broker->getClass($resourceClassName); - } catch (ClassNotFoundException $e) { - $message = \sprintf( - 'Resource class "%s" not found!', - $resourceClassName, - ); - - return [ - RuleErrorBuilder::message($message) - ->identifier('sylius.grid.resourceClassRequired') - ->build(), - ]; + $this->broker->getClass($resourceClass); + } catch (ClassNotFoundException) { + return RuleErrorBuilder::message(\sprintf('Resource class "%s" not found!', $resourceClass)) + ->identifier('sylius.grid.resourceClassRequired') + ->line($line) + ->build(); } - return []; + return null; + } + + /** + * Reports the error on the attribute itself rather than on the class or on + * whichever method happened to be visited first. + */ + private static function findAttributeLine(ClassLike $class): int + { + foreach ($class->attrGroups as $attrGroup) { + foreach ($attrGroup->attrs as $attribute) { + if (self::AS_GRID_ATTRIBUTE === $attribute->name->toString()) { + return $attribute->getStartLine(); + } + } + } + + return $class->getStartLine(); + } + + /** + * Reports the error on the getResourceClass() declaration rather than on the + * class. + */ + private static function findMethodLine(ClassLike $class, string $methodName): int + { + return $class->getMethod($methodName)?->getStartLine() ?? $class->getStartLine(); + } + + /** + * An attribute argument is not guaranteed to be a single constant string. + * Calling Type::getValue() on anything else raises an internal error and + * aborts the analysis, so only single constant strings are accepted. + */ + private static function resolveConstantString(?Type $argumentType): ?string + { + if (null === $argumentType) { + return null; + } + + $constantStrings = $argumentType->getConstantStrings(); + + return 1 === \count($constantStrings) ? $constantStrings[0]->getValue() : null; } } diff --git a/src/bitExpert/PHPStan/Sylius/Rule/Resource/IndexOperationNeedsGridClassRule.php b/src/bitExpert/PHPStan/Sylius/Rule/Resource/IndexOperationNeedsGridClassRule.php index d56816b..fdaa9b7 100644 --- a/src/bitExpert/PHPStan/Sylius/Rule/Resource/IndexOperationNeedsGridClassRule.php +++ b/src/bitExpert/PHPStan/Sylius/Rule/Resource/IndexOperationNeedsGridClassRule.php @@ -18,7 +18,7 @@ use PHPStan\Reflection\ReflectionProvider; use PHPStan\Rules\Rule; use PHPStan\Rules\RuleErrorBuilder; -use PHPStan\Type\Constant\ConstantStringType; +use PHPStan\Type\Type; /** * @implements Rule @@ -29,6 +29,9 @@ public function __construct(private ReflectionProvider $broker) { } + /** + * @return class-string + */ public function getNodeType(): string { return InClassNode::class; @@ -48,26 +51,43 @@ public function processNode(Node $node, Scope $scope): array $resourceClassAttributes = $classReflection->getAttributes(); foreach ($resourceClassAttributes as $attribute) { if ('Sylius\Resource\Metadata\Index' === $attribute->getName()) { - /** @var array $argumentTypes */ $argumentTypes = $attribute->getArgumentTypes(); - if (isset($argumentTypes['grid'])) { - $gridClass = $argumentTypes['grid']->getValue(); + $gridClass = self::resolveConstantString($argumentTypes['grid'] ?? null); + if (null === $gridClass) { + continue; + } - try { - $this->broker->getClass($gridClass); - } catch (\Throwable $e) { - $message = \sprintf('Grid class "%s" not found!', $gridClass); + try { + $this->broker->getClass($gridClass); + } catch (\Throwable) { + $message = \sprintf('Grid class "%s" not found!', $gridClass); - return [ - RuleErrorBuilder::message($message) - ->identifier('sylius.resource.gridClassNotFound') - ->build(), - ]; - } + return [ + RuleErrorBuilder::message($message) + ->identifier('sylius.resource.gridClassNotFound') + ->build(), + ]; } } } return []; } + + /** + * Attribute arguments are not guaranteed to be a single constant string, for + * example #[Index(grid: new SomeType())] yields an ObjectType. Calling + * Type::getValue() on such an argument raises an internal error and aborts the + * whole analysis, so only single constant strings are accepted. + */ + private static function resolveConstantString(?Type $argumentType): ?string + { + if (null === $argumentType) { + return null; + } + + $constantStrings = $argumentType->getConstantStrings(); + + return 1 === \count($constantStrings) ? $constantStrings[0]->getValue() : null; + } } diff --git a/src/bitExpert/PHPStan/Sylius/Rule/Resource/ResourceAttributeNeedsFormTypeRule.php b/src/bitExpert/PHPStan/Sylius/Rule/Resource/ResourceAttributeNeedsFormTypeRule.php index c4b2a65..c9ba180 100644 --- a/src/bitExpert/PHPStan/Sylius/Rule/Resource/ResourceAttributeNeedsFormTypeRule.php +++ b/src/bitExpert/PHPStan/Sylius/Rule/Resource/ResourceAttributeNeedsFormTypeRule.php @@ -18,7 +18,7 @@ use PHPStan\Reflection\ReflectionProvider; use PHPStan\Rules\Rule; use PHPStan\Rules\RuleErrorBuilder; -use PHPStan\Type\Constant\ConstantStringType; +use PHPStan\Type\Type; /** * @implements Rule @@ -29,6 +29,9 @@ public function __construct(private ReflectionProvider $broker) { } + /** + * @return class-string + */ public function getNodeType(): string { return InClassNode::class; @@ -48,26 +51,43 @@ public function processNode(Node $node, Scope $scope): array $resourceClassAttributes = $classReflection->getAttributes(); foreach ($resourceClassAttributes as $attribute) { if ('Sylius\Resource\Metadata\AsResource' === $attribute->getName()) { - /** @var array $argumentTypes */ $argumentTypes = $attribute->getArgumentTypes(); - if (isset($argumentTypes['formType'])) { - $formType = $argumentTypes['formType']->getValue(); + $formType = self::resolveConstantString($argumentTypes['formType'] ?? null); + if (null === $formType) { + continue; + } - try { - $this->broker->getClass($formType); - } catch (\Throwable $e) { - $message = \sprintf('Form Type "%s" not found!', $formType); + try { + $this->broker->getClass($formType); + } catch (\Throwable) { + $message = \sprintf('Form Type "%s" not found!', $formType); - return [ - RuleErrorBuilder::message($message) - ->identifier('sylius.resource.formTypeNotFound') - ->build(), - ]; - } + return [ + RuleErrorBuilder::message($message) + ->identifier('sylius.resource.formTypeNotFound') + ->build(), + ]; } } } return []; } + + /** + * Attribute arguments are not guaranteed to be a single constant string, for + * example #[AsResource(formType: new SomeType())] yields an ObjectType. + * Calling Type::getValue() on such an argument raises an internal error and + * aborts the whole analysis, so only single constant strings are accepted. + */ + private static function resolveConstantString(?Type $argumentType): ?string + { + if (null === $argumentType) { + return null; + } + + $constantStrings = $argumentType->getConstantStrings(); + + return 1 === \count($constantStrings) ? $constantStrings[0]->getValue() : null; + } } diff --git a/tests/bitExpert/PHPStan/Sylius/Rule/Grid/GridBuilderFieldIsPartOfResourceClassUnitTest.php b/tests/bitExpert/PHPStan/Sylius/Rule/Grid/GridBuilderFieldIsPartOfResourceClassUnitTest.php index ecd80e2..9a4afb9 100644 --- a/tests/bitExpert/PHPStan/Sylius/Rule/Grid/GridBuilderFieldIsPartOfResourceClassUnitTest.php +++ b/tests/bitExpert/PHPStan/Sylius/Rule/Grid/GridBuilderFieldIsPartOfResourceClassUnitTest.php @@ -57,7 +57,30 @@ public function testRule(): void [ [ 'The field "name" needs to exists as property in class "App\Entity\Supplier".', - 29, + 34, + ], + [ + // This field is declared after the recursive "address.city" field. + // Walking the recursive path replaced the resource class with the + // type of the last resolved segment, so this field was looked up on + // App\Entity\Address instead of App\Entity\Supplier and no error was + // ever reported for it. + 'The field "plainFieldAfterDottedField" needs to exists as property in class "App\Entity\Supplier".', + 43, + ], + [ + // Third segment of a recursive field. Once the walk leaves the + // ClassReflection it queried a Type, and Type::hasProperty() + // answers with a TrinaryLogic whose cast to bool is always true, + // so every segment past the first silently passed. + 'The field "missingThirdSegment" needs to exists as property in class "App\Entity\Supplier".', + 61, + ], + [ + // CallableField::createForService() was skipped by the collector, + // which only accepted a static call named create(). + 'The field "missingCallableServiceField" needs to exists as property in class "App\Entity\Supplier".', + 67, ], ], ); diff --git a/tests/bitExpert/PHPStan/Sylius/Rule/Grid/GridBuilderFilterIsPartOfResourceClassUnitTest.php b/tests/bitExpert/PHPStan/Sylius/Rule/Grid/GridBuilderFilterIsPartOfResourceClassUnitTest.php index 496f461..2f0a3da 100644 --- a/tests/bitExpert/PHPStan/Sylius/Rule/Grid/GridBuilderFilterIsPartOfResourceClassUnitTest.php +++ b/tests/bitExpert/PHPStan/Sylius/Rule/Grid/GridBuilderFilterIsPartOfResourceClassUnitTest.php @@ -14,13 +14,21 @@ use bitExpert\PHPStan\Sylius\Collector\Grid\CollectFilterForGridClass; use bitExpert\PHPStan\Sylius\Collector\Grid\CollectRessourceClassForGridClass; +use bitExpert\PHPStan\Sylius\Collector\Grid\Filter\BooleanFilter; +use bitExpert\PHPStan\Sylius\Collector\Grid\Filter\DateFilter; use bitExpert\PHPStan\Sylius\Collector\Grid\Filter\DefaultFilterRegistry; use bitExpert\PHPStan\Sylius\Collector\Grid\Filter\EntityFilter; use bitExpert\PHPStan\Sylius\Collector\Grid\Filter\EnumFilter; use bitExpert\PHPStan\Sylius\Collector\Grid\Filter\ExistsFilter; use bitExpert\PHPStan\Sylius\Collector\Grid\Filter\Filter; +use bitExpert\PHPStan\Sylius\Collector\Grid\Filter\FilterNode; +use bitExpert\PHPStan\Sylius\Collector\Grid\Filter\MoneyFilter; use bitExpert\PHPStan\Sylius\Collector\Grid\Filter\SelectFilter; use bitExpert\PHPStan\Sylius\Collector\Grid\Filter\StringFilter; +use PhpParser\Node; +use PhpParser\Node\Expr\StaticCall; +use PhpParser\Node\Name\FullyQualified; +use PHPStan\Collectors\Collector; use PHPStan\Rules\Rule; use PHPStan\Testing\RuleTestCase; @@ -34,22 +42,68 @@ protected function getRule(): Rule return new GridBuilderFilterIsPartOfResourceClass($this->createReflectionProvider()); } + /** + * Overrides the registry for the test currently running. Left null the + * production order from extension.neon is used. + * + * @var list|null + */ + private ?array $filterNodes = null; + protected function getCollectors(): array { - $filters = []; - $filters[] = new EntityFilter(); - $filters[] = new EnumFilter(); - $filters[] = new ExistsFilter(); - $filters[] = new Filter(); - $filters[] = new SelectFilter(); - $filters[] = new StringFilter(); + if (null !== $this->filterNodes) { + return $this->collectorsFor($this->filterNodes); + } + + // Mirrors the registration order in extension.neon: the catch-all has to + // come last so it cannot shadow a node for a more specific class. + return $this->collectorsFor([ + new EntityFilter(), + new EnumFilter(), + new BooleanFilter(), + new DateFilter(), + new MoneyFilter(), + new ExistsFilter(), + new SelectFilter(), + new StringFilter(), + new Filter(), + ]); + } + /** + * @param list $filters + * + * @return array> + */ + private function collectorsFor(array $filters): array + { return [ new CollectRessourceClassForGridClass(), new CollectFilterForGridClass(new DefaultFilterRegistry($filters)), ]; } + /** + * A node dedicated to App\Filter\CustomFilter, i.e. what a user would + * register for their own filter class. It reports a different field than + * the catch-all would, so the two are distinguishable in the output. + */ + private function dedicatedCustomFilterNode(): FilterNode + { + return new class implements FilterNode { + public function supports(FullyQualified $nodeClass): bool + { + return 'App\\Filter\\CustomFilter' === $nodeClass->name; + } + + public function getFilterFields(StaticCall $node): array + { + return ['customNodeField']; + } + }; + } + public function testRule(): void { $this->analyse( @@ -57,15 +111,119 @@ public function testRule(): void [ [ 'The filter field "name" needs to exists as property in resource class "App\Entity\Supplier".', - 41, + 55, + ], + [ + 'The filter field "name" needs to exists as property in resource class "App\Entity\Supplier".', + 49, ], [ 'The filter field "status123" needs to exists as property in resource class "App\Entity\Supplier".', - 44, + 52, ], [ - 'The filter field "name" needs to exists as property in resource class "App\Entity\Supplier".', - 47, + // Handled by the generic Filter node, whose supports() used to + // compare the subtypes the wrong way round and therefore never + // matched anything. + 'The filter field "missingGenericFilterField" needs to exists as property in resource class "App\Entity\Supplier".', + 64, + ], + [ + // Had no node at all, and they do not implement FilterInterface + // either, so the catch-all node never matched them. + 'The filter field "missingBooleanFilterField" needs to exists as property in resource class "App\Entity\Supplier".', + 70, + ], + [ + // snake_case is converted, like every other filter field. + 'The filter field "missingDateFilterField" needs to exists as property in resource class "App\Entity\Supplier".', + 73, + ], + [ + 'The filter field "missingMoneyFilterField" needs to exists as property in resource class "App\Entity\Supplier".', + 76, + ], + ], + ); + } + + /** + * Baseline: with no dedicated node registered, the catch-all resolves the + * user-defined filter class purely because it implements FilterInterface. + * This also proves the fixture is actually wired into grid scope. + */ + public function testCatchAllResolvesAUserFilterByInterface(): void + { + $this->analyse( + [__DIR__ . '/data/grid_custom_filter.php'], + [ + [ + 'The filter field "catchAllOnlyField" needs to exists as property in resource class "App\Entity\Supplier".', + 41, + ], + [ + 'The filter field "catchAllField" needs to exists as property in resource class "App\Entity\Supplier".', + 45, + ], + ], + ); + } + + /** + * The documented registration order: a dedicated node comes before the + * catch-all, so it wins for the class it targets. + */ + public function testDedicatedNodeWinsWhenRegisteredBeforeTheCatchAll(): void + { + $this->filterNodes = [ + new StringFilter(), + $this->dedicatedCustomFilterNode(), + new Filter(), + ]; + + $this->analyse( + [__DIR__ . '/data/grid_custom_filter.php'], + [ + [ + 'The filter field "catchAllOnlyField" needs to exists as property in resource class "App\Entity\Supplier".', + 41, + ], + [ + // customNodeField comes from the dedicated node, not from + // the create() argument, so the node clearly won. + 'The filter field "customNodeField" needs to exists as property in resource class "App\Entity\Supplier".', + 45, + ], + ], + ); + } + + /** + * The inverse order, which is the bug the registration order guards + * against: the catch-all is interface-based, so it matches the user's + * filter class and the dedicated node is never consulted. The dedicated + * field never appears in the output. + */ + public function testCatchAllShadowsADedicatedNodeRegisteredAfterIt(): void + { + $this->filterNodes = [ + new StringFilter(), + new Filter(), + $this->dedicatedCustomFilterNode(), + ]; + + $this->analyse( + [__DIR__ . '/data/grid_custom_filter.php'], + [ + [ + 'The filter field "catchAllOnlyField" needs to exists as property in resource class "App\Entity\Supplier".', + 41, + ], + [ + // catchAllField is the create() argument, which is what the + // catch-all extracts. The dedicated node was shadowed. + 'The filter field "catchAllField" needs to exists as property in resource class "App\Entity\Supplier".', + 45, ], ], ); diff --git a/tests/bitExpert/PHPStan/Sylius/Rule/Grid/ResourceAwareGridNeedsResourceClassUnitTest.php b/tests/bitExpert/PHPStan/Sylius/Rule/Grid/ResourceAwareGridNeedsResourceClassUnitTest.php index 7c7216f..9674c94 100644 --- a/tests/bitExpert/PHPStan/Sylius/Rule/Grid/ResourceAwareGridNeedsResourceClassUnitTest.php +++ b/tests/bitExpert/PHPStan/Sylius/Rule/Grid/ResourceAwareGridNeedsResourceClassUnitTest.php @@ -47,8 +47,52 @@ public function ruleForAttr(): void [__DIR__ . '/data/grid_needs_resource_model_attr.php'], [ [ + // Reported on the #[AsGrid(resourceClass: ...)] attribute at line + // 21, not on whichever method happened to be visited first. 'Resource class "App\Entity\SupplierNotFound" not found!', - 24, + 21, + ], + ], + ); + } + + /** + * A grid is not required to declare any method. Because the rule listened on + * MethodReturnStatementsNode before, this class was never visited at all. + */ + #[Test] + public function ruleChecksGridWithoutAnyMethod(): void + { + $this->analyse( + [__DIR__ . '/data/grid_needs_resource_model_no_methods.php'], + [ + [ + 'Resource class "App\Entity\SupplierNotFound" not found!', + 17, + ], + ], + ); + } + + /** + * Since grid-bundle 1.16 a grid may implement + * Sylius\Component\Grid\GridInterface without extending the legacy + * AbstractGrid. The resource class of such a grid used to go unvalidated, + * because AbstractGrid was required before the attribute was even read. + */ + #[Test] + public function ruleChecksGridUsingTheNewGridInterface(): void + { + if (!\interface_exists('Sylius\Component\Grid\GridInterface')) { + self::markTestSkipped('Sylius\Component\Grid\GridInterface requires grid-bundle >= 1.16.'); + } + + $this->analyse( + [__DIR__ . '/data/grid_needs_resource_model_native_interface.php'], + [ + [ + 'Resource class "App\Entity\SupplierNotFound" not found!', + 35, ], ], ); diff --git a/tests/bitExpert/PHPStan/Sylius/Rule/Grid/data/custom_filter.php b/tests/bitExpert/PHPStan/Sylius/Rule/Grid/data/custom_filter.php new file mode 100644 index 0000000..81806d4 --- /dev/null +++ b/tests/bitExpert/PHPStan/Sylius/Rule/Grid/data/custom_filter.php @@ -0,0 +1,153 @@ + */ + private array $options = []; + + /** @var array */ + private array $formOptions = []; + + public static function create(string $name, string $type): self + { + return new self(); + } + + public function getName(): string + { + return ''; + } + + public function getLabel(): string|bool|null + { + return null; + } + + public function setLabel(string|bool|null $label): self + { + return $this; + } + + public function isEnabled(): bool + { + return true; + } + + public function setEnabled(bool $enabled): self + { + return $this; + } + + public function getTemplate(): ?string + { + return null; + } + + public function setTemplate(?string $template): self + { + return $this; + } + + /** + * @return array + */ + public function getOptions(): array + { + return $this->options; + } + + /** + * @param array $options + */ + public function setOptions(array $options): self + { + $this->options = $options; + + return $this; + } + + /** + * @param mixed $value + */ + public function addOption(string $option, $value): self + { + $this->options[$option] = $value; + + return $this; + } + + public function removeOption(string $option): self + { + unset($this->options[$option]); + + return $this; + } + + /** + * @return array + */ + public function getFormOptions(): array + { + return $this->formOptions; + } + + /** + * @param array $formOptions + */ + public function setFormOptions(array $formOptions): self + { + $this->formOptions = $formOptions; + + return $this; + } + + /** + * @param mixed $value + */ + public function addFormOption(string $option, $value): self + { + $this->formOptions[$option] = $value; + + return $this; + } + + public function removeFormOption(string $option): self + { + unset($this->formOptions[$option]); + + return $this; + } + + /** + * @param array $criteria + */ + public function setCriteria(array $criteria): self + { + return $this; + } + + /** + * @return array + */ + public function toArray(): array + { + return []; + } +} diff --git a/tests/bitExpert/PHPStan/Sylius/Rule/Grid/data/entity.php b/tests/bitExpert/PHPStan/Sylius/Rule/Grid/data/entity.php index bd7f85e..bc482f6 100644 --- a/tests/bitExpert/PHPStan/Sylius/Rule/Grid/data/entity.php +++ b/tests/bitExpert/PHPStan/Sylius/Rule/Grid/data/entity.php @@ -12,14 +12,31 @@ enum Status: string case INACTIVE = 'inactive'; } +class Country +{ + private string $isoCode; + + public function getIsoCode(): string + { + return $this->isoCode; + } +} + class Address { private $city; + private Country $country; + public function getCity(): string { return $this->city; } + + public function getCountry(): Country + { + return $this->country; + } } class Supplier implements ResourceInterface diff --git a/tests/bitExpert/PHPStan/Sylius/Rule/Grid/data/grid.php b/tests/bitExpert/PHPStan/Sylius/Rule/Grid/data/grid.php index e32ba32..3d69175 100644 --- a/tests/bitExpert/PHPStan/Sylius/Rule/Grid/data/grid.php +++ b/tests/bitExpert/PHPStan/Sylius/Rule/Grid/data/grid.php @@ -6,8 +6,13 @@ use App\Entity\Status; use App\Entity\Supplier; +use Sylius\Bundle\GridBundle\Builder\Field\CallableField; use Sylius\Bundle\GridBundle\Builder\Field\StringField; +use Sylius\Bundle\GridBundle\Builder\Filter\BooleanFilter; +use Sylius\Bundle\GridBundle\Builder\Filter\DateFilter; use Sylius\Bundle\GridBundle\Builder\Filter\EnumFilter; +use Sylius\Bundle\GridBundle\Builder\Filter\Filter; +use Sylius\Bundle\GridBundle\Builder\Filter\MoneyFilter; use Sylius\Bundle\GridBundle\Builder\Filter\StringFilter; use Sylius\Bundle\GridBundle\Builder\GridBuilderInterface; use Sylius\Bundle\GridBundle\Grid\AbstractGrid; @@ -34,6 +39,9 @@ public function buildGrid(GridBuilderInterface $gridBuilder): void $gridBuilder->addField( StringField::create('address.city')->setLabel('app.ui.address.city'), ); + $gridBuilder->addField( + StringField::create('plainFieldAfterDottedField')->setLabel('app.ui.plain'), + ); $gridBuilder->addField( StringField::create('.')->setLabel('app.ui.some_calculated_field'), ); @@ -46,6 +54,27 @@ public function buildGrid(GridBuilderInterface $gridBuilder): void $gridBuilder->addFilter( StringFilter::create('virtual-field', ['name', 'address.city']), ); + $gridBuilder->addField( + StringField::create('address.country.isoCode')->setLabel('app.ui.isoCode'), + ); + $gridBuilder->addField( + StringField::create('address.country.missingThirdSegment')->setLabel('app.ui.missing'), + ); + $gridBuilder->addFilter( + Filter::create('missingGenericFilterField', 'string'), + ); + $gridBuilder->addField( + CallableField::createForService('missingCallableServiceField', 'app.some_service'), + ); + $gridBuilder->addFilter( + BooleanFilter::create('missingBooleanFilterField'), + ); + $gridBuilder->addFilter( + DateFilter::create('missing_date_filter_field'), + ); + $gridBuilder->addFilter( + MoneyFilter::create('missingMoneyFilterField', 'USD'), + ); } public function getResourceClass(): string diff --git a/tests/bitExpert/PHPStan/Sylius/Rule/Grid/data/grid_custom_filter.php b/tests/bitExpert/PHPStan/Sylius/Rule/Grid/data/grid_custom_filter.php new file mode 100644 index 0000000..5ebd089 --- /dev/null +++ b/tests/bitExpert/PHPStan/Sylius/Rule/Grid/data/grid_custom_filter.php @@ -0,0 +1,48 @@ +addFilter( + Filter::create('catchAllOnlyField', 'string'), + ); + + $gridBuilder->addFilter( + CustomFilter::create('catchAllField', 'string'), + ); + } +} diff --git a/tests/bitExpert/PHPStan/Sylius/Rule/Grid/data/grid_needs_resource_model_native_interface.php b/tests/bitExpert/PHPStan/Sylius/Rule/Grid/data/grid_needs_resource_model_native_interface.php new file mode 100644 index 0000000..53a9e31 --- /dev/null +++ b/tests/bitExpert/PHPStan/Sylius/Rule/Grid/data/grid_needs_resource_model_native_interface.php @@ -0,0 +1,43 @@ += 1.16, and + * this file is registered in composer's autoload-dev files, so the declarations + * are guarded to keep the older lane from fataling on an unknown interface. + */ +if (\interface_exists(GridInterface::class)) { + #[AsGrid(resourceClass: Supplier::class)] + final class NativeGridWithExistingResourceClass implements GridInterface + { + public function getName(): string + { + return 'app_admin_supplier'; + } + } + + #[AsGrid(resourceClass: 'App\Entity\SupplierNotFound')] + final class NativeGridWithMissingResourceClass implements GridInterface + { + public function getName(): string + { + return 'app_admin_supplier_no_class'; + } + } +} diff --git a/tests/bitExpert/PHPStan/Sylius/Rule/Grid/data/grid_needs_resource_model_no_methods.php b/tests/bitExpert/PHPStan/Sylius/Rule/Grid/data/grid_needs_resource_model_no_methods.php new file mode 100644 index 0000000..536f7fc --- /dev/null +++ b/tests/bitExpert/PHPStan/Sylius/Rule/Grid/data/grid_needs_resource_model_no_methods.php @@ -0,0 +1,20 @@ + + */ +class IndexOperationNeedsGridClassUnitTest extends RuleTestCase +{ + protected function getRule(): Rule + { + return new IndexOperationNeedsGridClassRule($this->createReflectionProvider()); + } + + #[Test] + public function ruleReportsMissingGridClass(): void + { + $this->analyse( + [__DIR__ . '/data/entity_index.php'], + [ + [ + 'Grid class "App\Grid\GridClassNotExists" not found!', + 10, + ], + ], + ); + } + + /** + * The fixture mixes AsResource and Index attributes, including constant-array + * and object arguments. None of those are a single constant string, so this + * rule must skip all of them. Before the fix, Index(grid: [self::class]) + * raised an internal error and aborted the analysis. + */ + #[Test] + public function ruleSkipsNonConstantGridArgument(): void + { + $this->analyse( + [__DIR__ . '/data/entity_non_constant_attribute.php'], + [], + ); + } +} diff --git a/tests/bitExpert/PHPStan/Sylius/Rule/Resource/ResourceAttributeNeedsFormTypeUnitTest.php b/tests/bitExpert/PHPStan/Sylius/Rule/Resource/ResourceAttributeNeedsFormTypeUnitTest.php index b7e5da5..4d10025 100644 --- a/tests/bitExpert/PHPStan/Sylius/Rule/Resource/ResourceAttributeNeedsFormTypeUnitTest.php +++ b/tests/bitExpert/PHPStan/Sylius/Rule/Resource/ResourceAttributeNeedsFormTypeUnitTest.php @@ -39,4 +39,24 @@ public function ruleForClassmethods(): void ], ); } + + /** + * Only the single constant string is reported. The object, constant-array + * and integer arguments must be skipped, which they were not before: the + * rule called Type::getValue() on them, which raised an internal error and + * aborted the analysis. + */ + #[Test] + public function ruleSkipsNonConstantFormTypeArgument(): void + { + $this->analyse( + [__DIR__ . '/data/entity_non_constant_attribute.php'], + [ + [ + 'Form Type "FormClassNotExists" not found!', + 75, + ], + ], + ); + } } diff --git a/tests/bitExpert/PHPStan/Sylius/Rule/Resource/data/entity.php b/tests/bitExpert/PHPStan/Sylius/Rule/Resource/data/entity.php index d85a6bd..b575b63 100644 --- a/tests/bitExpert/PHPStan/Sylius/Rule/Resource/data/entity.php +++ b/tests/bitExpert/PHPStan/Sylius/Rule/Resource/data/entity.php @@ -10,7 +10,7 @@ #[AsResource( formType: 'FormClassNotExists', )] -class entity implements ResourceInterface +class Entity implements ResourceInterface { private int $id; diff --git a/tests/bitExpert/PHPStan/Sylius/Rule/Resource/data/entity_index.php b/tests/bitExpert/PHPStan/Sylius/Rule/Resource/data/entity_index.php new file mode 100644 index 0000000..89f9a01 --- /dev/null +++ b/tests/bitExpert/PHPStan/Sylius/Rule/Resource/data/entity_index.php @@ -0,0 +1,19 @@ +id; + } +} diff --git a/tests/bitExpert/PHPStan/Sylius/Rule/Resource/data/entity_non_constant_attribute.php b/tests/bitExpert/PHPStan/Sylius/Rule/Resource/data/entity_non_constant_attribute.php new file mode 100644 index 0000000..4fcb0e8 --- /dev/null +++ b/tests/bitExpert/PHPStan/Sylius/Rule/Resource/data/entity_non_constant_attribute.php @@ -0,0 +1,84 @@ +id; + } +} + +#[AsResource(formType: [self::class])] +class EntityWithArrayFormType implements ResourceInterface +{ + private int $id; + + public function getId(): int + { + return $this->id; + } +} + +#[AsResource(formType: 123)] +class EntityWithIntFormType implements ResourceInterface +{ + private int $id; + + public function getId(): int + { + return $this->id; + } +} + +#[Index(grid: new \stdClass())] +class EntityWithNonConstantGrid implements ResourceInterface +{ + private int $id; + + public function getId(): int + { + return $this->id; + } +} + +#[Index(grid: [self::class])] +class EntityWithArrayGrid implements ResourceInterface +{ + private int $id; + + public function getId(): int + { + return $this->id; + } +} + +/** + * A single constant string is still resolved, so a missing class is reported. + */ +#[AsResource(formType: 'FormClassNotExists')] +class EntityWithConstantFormType implements ResourceInterface +{ + private int $id; + + public function getId(): int + { + return $this->id; + } +}