From 20656082268bd85c37ff8ed325f1730430095113 Mon Sep 17 00:00:00 2001 From: Guy Sartorelli <36352093+GuySartorelli@users.noreply.github.com> Date: Tue, 30 Jan 2024 14:57:43 +1300 Subject: [PATCH] NEW Add method annotation checks (#1) --- .editorconfig | 21 + .gitattributes | 7 + .github/ISSUE_TEMPLATE.md | 1 + .github/ISSUE_TEMPLATE/1_bug_report.yml | 72 ++++ .github/ISSUE_TEMPLATE/2_feature_request.yml | 35 ++ .github/ISSUE_TEMPLATE/config.yml | 8 + .github/PULL_REQUEST_TEMPLATE.md | 39 ++ .github/workflows/ci.yml | 11 + .github/workflows/dispatch-ci.yml | 16 + .github/workflows/keepalive.yml | 17 + .github/workflows/merge-up.yml | 17 + .gitignore | 3 + LICENSE | 29 ++ README.md | 24 ++ composer.json | 32 ++ phpcs.xml.dist | 10 + phpunit.xml.dist | 8 + rules.neon | 9 + src/PHPStan/MethodAnnotationsRule.php | 376 ++++++++++++++++++ tests/PHPStan/MethodAnnotationsRuleTest.php | 133 +++++++ .../DataObjectAllConfigFromMethodIgnored.php | 56 +++ .../DataObjectAllPropertiesAllErrors.php | 77 ++++ .../DataObjectAllPropertiesNoErrors.php | 85 ++++ .../DataObjectNoAnnotations.php | 23 ++ .../ExtensionMixedAllErrors.php | 91 +++++ .../ExtensionMixedNoErrors.php | 100 +++++ .../ManyManyThroughModel.php | 15 + .../NotAnExtensionOrDataObject.php | 20 + 28 files changed, 1335 insertions(+) create mode 100644 .editorconfig create mode 100644 .gitattributes create mode 100644 .github/ISSUE_TEMPLATE.md create mode 100644 .github/ISSUE_TEMPLATE/1_bug_report.yml create mode 100644 .github/ISSUE_TEMPLATE/2_feature_request.yml create mode 100644 .github/ISSUE_TEMPLATE/config.yml create mode 100644 .github/PULL_REQUEST_TEMPLATE.md create mode 100644 .github/workflows/ci.yml create mode 100644 .github/workflows/dispatch-ci.yml create mode 100644 .github/workflows/keepalive.yml create mode 100644 .github/workflows/merge-up.yml create mode 100644 .gitignore create mode 100644 LICENSE create mode 100644 composer.json create mode 100644 phpcs.xml.dist create mode 100644 phpunit.xml.dist create mode 100644 rules.neon create mode 100644 src/PHPStan/MethodAnnotationsRule.php create mode 100644 tests/PHPStan/MethodAnnotationsRuleTest.php create mode 100644 tests/PHPStan/MethodAnnotationsRuleTest/DataObjectAllConfigFromMethodIgnored.php create mode 100644 tests/PHPStan/MethodAnnotationsRuleTest/DataObjectAllPropertiesAllErrors.php create mode 100644 tests/PHPStan/MethodAnnotationsRuleTest/DataObjectAllPropertiesNoErrors.php create mode 100644 tests/PHPStan/MethodAnnotationsRuleTest/DataObjectNoAnnotations.php create mode 100644 tests/PHPStan/MethodAnnotationsRuleTest/ExtensionMixedAllErrors.php create mode 100644 tests/PHPStan/MethodAnnotationsRuleTest/ExtensionMixedNoErrors.php create mode 100644 tests/PHPStan/MethodAnnotationsRuleTest/ManyManyThroughModel.php create mode 100644 tests/PHPStan/MethodAnnotationsRuleTest/NotAnExtensionOrDataObject.php diff --git a/.editorconfig b/.editorconfig new file mode 100644 index 0000000..9dd906a --- /dev/null +++ b/.editorconfig @@ -0,0 +1,21 @@ +# For more information about the properties used in +# this file, please see the EditorConfig documentation: +# http://editorconfig.org/ + +[*] +charset = utf-8 +end_of_line = lf +indent_size = 4 +indent_style = space +insert_final_newline = true +trim_trailing_whitespace = true + +[*.md] +trim_trailing_whitespace = false + +[*.eslintrc] +indent_size = 2 +indent_style = space + +[composer.json] +indent_size = 4 diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 0000000..c4242b4 --- /dev/null +++ b/.gitattributes @@ -0,0 +1,7 @@ +/.github export-ignore +/.gitattributes export-ignore +/.gitignore export-ignore +/phpunit.xml.dist export-ignore +/phpcs.xml.dist export-ignore +/tests export-ignore +/.editorconfig export-ignore diff --git a/.github/ISSUE_TEMPLATE.md b/.github/ISSUE_TEMPLATE.md new file mode 100644 index 0000000..cb5f058 --- /dev/null +++ b/.github/ISSUE_TEMPLATE.md @@ -0,0 +1 @@ + diff --git a/.github/ISSUE_TEMPLATE/1_bug_report.yml b/.github/ISSUE_TEMPLATE/1_bug_report.yml new file mode 100644 index 0000000..c956653 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/1_bug_report.yml @@ -0,0 +1,72 @@ +name: 🪳 Bug Report +description: Tell us if something isn't working the way it's supposed to + +body: + - type: markdown + attributes: + value: | + We strongly encourage you to [submit a pull request](https://docs.silverstripe.org/en/contributing/code/) which fixes the issue. + Bug reports which are accompanied with a pull request are a lot more likely to be resolved quickly. + - type: input + id: affected-versions + attributes: + label: Module version(s) affected + description: | + What version of _this module_ have you reproduced this bug on? + Run `composer info` to see the specific version of each module installed in your project. + If you don't have access to that, check inside the help menu in the bottom left of the CMS. + placeholder: x.y.z + validations: + required: true + - type: textarea + id: description + attributes: + label: Description + description: A clear and concise description of the problem + validations: + required: true + - type: textarea + id: how-to-reproduce + attributes: + label: How to reproduce + description: | + ⚠️ This is the most important part of the report ⚠️ + Without a way to easily reproduce your issue, there is little chance we will be able to help you and work on a fix. + - Please, take the time to show us some code and/or configuration that is needed for others to reproduce the problem easily. + - If the bug is too complex to reproduce with some short code samples, please reproduce it in a public repository and provide a link to the repository along with steps for setting up and reproducing the bug using that repository. + - If part of the bug includes an error or exception, please provide a full stack trace. + - If any user interaction is required to reproduce the bug, please add an ordered list of steps that are required to reproduce it. + - Be as clear as you can, but don't miss any steps out. Simply saying "create a page" is less useful than guiding us through the steps you're taking to create a page, for example. + placeholder: | + + #### Code sample + ```php + + ``` + + #### Reproduction steps + 1. + validations: + required: true + - type: textarea + id: possible-solution + attributes: + label: Possible Solution + description: | + *Optional: only if you have suggestions on a fix/reason for the bug* + Please consider [submitting a pull request](https://docs.silverstripe.org/en/contributing/code/) with your solution! It helps get faster feedback and greatly increases the chance of the bug being fixed. + - type: textarea + id: additional-context + attributes: + label: Additional Context + description: "*Optional: any other context about the problem: log messages, screenshots, etc.*" + - type: checkboxes + id: validations + attributes: + label: Validations + description: "Before submitting the issue, please make sure you do the following:" + options: + - label: Check that there isn't already an issue that reports the same bug + required: true + - label: Double check that your reproduction steps work in a fresh installation of [`silverstripe/installer`](https://github.com/silverstripe/silverstripe-installer) (with any code examples you've provided) + required: true diff --git a/.github/ISSUE_TEMPLATE/2_feature_request.yml b/.github/ISSUE_TEMPLATE/2_feature_request.yml new file mode 100644 index 0000000..fe808a5 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/2_feature_request.yml @@ -0,0 +1,35 @@ +name: 🚀 Feature Request +description: Submit a feature request (but only if you're planning on implementing it) +body: + - type: markdown + attributes: + value: | + Please only submit feature requests if you plan on implementing the feature yourself. + See the [contributing code documentation](https://docs.silverstripe.org/en/contributing/code/#make-or-find-a-github-issue) for more guidelines about submitting feature requests. + - type: textarea + id: description + attributes: + label: Description + description: A clear and concise description of the new feature, and why it belongs in core + validations: + required: true + - type: textarea + id: more-info + attributes: + label: Additional context or points of discussion + description: | + *Optional: Any additional context, points of discussion, etc that might help validate and refine your idea* + - type: checkboxes + id: validations + attributes: + label: Validations + description: "Before submitting the issue, please confirm the following:" + options: + - label: You intend to implement the feature yourself + required: true + - label: You have read the [contributing guide](https://docs.silverstripe.org/en/contributing/code/) + required: true + - label: You strongly believe this feature should be in core, rather than being its own community module + required: true + - label: You have checked for existing issues or pull requests related to this feature (and didn't find any) + required: true diff --git a/.github/ISSUE_TEMPLATE/config.yml b/.github/ISSUE_TEMPLATE/config.yml new file mode 100644 index 0000000..f765200 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/config.yml @@ -0,0 +1,8 @@ +blank_issues_enabled: true +contact_links: + - name: Security Vulnerability + url: https://docs.silverstripe.org/en/contributing/issues_and_bugs/#reporting-security-issues + about: ⚠️ We do not use GitHub issues to track security vulnerability reports. Click "open" on the right to see how to report security vulnerabilities. + - name: Support Question + url: https://www.silverstripe.org/community/ + about: We use GitHub issues only to discuss bugs and new features. For support questions, please use one of the support options available in our community channels. diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md new file mode 100644 index 0000000..0e44db1 --- /dev/null +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -0,0 +1,39 @@ + +## Description + + +## Manual testing steps + + +## Issues + +- # + +## Pull request checklist + +- [ ] The target branch is correct + - See [picking the right version](https://docs.silverstripe.org/en/contributing/code/#picking-the-right-version) +- [ ] All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting) + - Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR. +- [ ] The commit messages follow our [commit message guidelines](https://docs.silverstripe.org/en/contributing/code/#commit-messages) +- [ ] The PR follows our [contribution guidelines](https://docs.silverstripe.org/en/contributing/code/) +- [ ] Code changes follow our [coding conventions](https://docs.silverstripe.org/en/contributing/coding_conventions/) +- [ ] This change is covered with tests (or tests aren't necessary for this change) +- [ ] Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release +- [ ] CI is green diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..bf02210 --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,11 @@ +name: CI + +on: + push: + pull_request: + workflow_dispatch: + +jobs: + ci: + name: CI + uses: silverstripe/gha-ci/.github/workflows/ci.yml@v1 diff --git a/.github/workflows/dispatch-ci.yml b/.github/workflows/dispatch-ci.yml new file mode 100644 index 0000000..c266b0d --- /dev/null +++ b/.github/workflows/dispatch-ci.yml @@ -0,0 +1,16 @@ +name: Dispatch CI + +on: + # At 3:15 AM UTC, only on Monday and Tuesday + schedule: + - cron: '15 3 * * 1,2' + +jobs: + dispatch-ci: + name: Dispatch CI + # Only run cron on the silverstripe account + if: (github.event_name == 'schedule' && github.repository_owner == 'silverstripe') || (github.event_name != 'schedule') + runs-on: ubuntu-latest + steps: + - name: Dispatch CI + uses: silverstripe/gha-dispatch-ci@v1 diff --git a/.github/workflows/keepalive.yml b/.github/workflows/keepalive.yml new file mode 100644 index 0000000..48d8dff --- /dev/null +++ b/.github/workflows/keepalive.yml @@ -0,0 +1,17 @@ +name: Keepalive + +on: + # At 3:15 AM UTC, on day 16 of the month + schedule: + - cron: '15 3 16 * *' + workflow_dispatch: + +jobs: + keepalive: + name: Keepalive + # Only run cron on the silverstripe account + if: (github.event_name == 'schedule' && github.repository_owner == 'silverstripe') || (github.event_name != 'schedule') + runs-on: ubuntu-latest + steps: + - name: Keepalive + uses: silverstripe/gha-keepalive@v1 diff --git a/.github/workflows/merge-up.yml b/.github/workflows/merge-up.yml new file mode 100644 index 0000000..bfd2940 --- /dev/null +++ b/.github/workflows/merge-up.yml @@ -0,0 +1,17 @@ +name: Merge-up + +on: + # At 3:15 AM UTC, only on Monday + schedule: + - cron: '15 3 * * 1' + workflow_dispatch: + +jobs: + merge-up: + name: Merge-up + # Only run cron on the silverstripe account + if: (github.event_name == 'schedule' && github.repository_owner == 'silverstripe') || (github.event_name != 'schedule') + runs-on: ubuntu-latest + steps: + - name: Merge-up + uses: silverstripe/gha-merge-up@v1 diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..525642d --- /dev/null +++ b/.gitignore @@ -0,0 +1,3 @@ +/vendor/ +/composer.lock +.phpunit.result.cache diff --git a/LICENSE b/LICENSE new file mode 100644 index 0000000..848c4ea --- /dev/null +++ b/LICENSE @@ -0,0 +1,29 @@ +BSD 3-Clause License + +Copyright (c) 2024, Silverstripe Limited - www.silverstripe.com +All rights reserved. + +Redistribution and use in source and binary forms, with or without +modification, are permitted provided that the following conditions are met: + +* Redistributions of source code must retain the above copyright notice, this + list of conditions and the following disclaimer. + +* Redistributions in binary form must reproduce the above copyright notice, + this list of conditions and the following disclaimer in the documentation + and/or other materials provided with the distribution. + +* Neither the name of the copyright holder nor the names of its + contributors may be used to endorse or promote products derived from + this software without specific prior written permission. + +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" +AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE +IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE +DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE +FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL +DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR +SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER +CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, +OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. diff --git a/README.md b/README.md index e69de29..2ba3d02 100644 --- a/README.md +++ b/README.md @@ -0,0 +1,24 @@ +# Silverstripe standards + +This repository contains some dependencies, custom rules, and predefined rulesets which are used to help maintain coding standards for Silverstripe CMS commercially supported modules. + +## Installation + +To use this extension, require it in [Composer](https://getcomposer.org/): + +```bash +composer require --dev silverstripe/standards +``` + +If you also install [phpstan/extension-installer](https://github.com/phpstan/extension-installer) then you're all set! + +
+Manual installation + +If you don't want to use `phpstan/extension-installer`, include rules.neon in your project's PHPStan config: + +```neon +includes: + - vendor/silverstripe/standards/rules.neon +``` +
diff --git a/composer.json b/composer.json new file mode 100644 index 0000000..4a335a8 --- /dev/null +++ b/composer.json @@ -0,0 +1,32 @@ +{ + "name": "silverstripe/standards", + "description": "A set of coding standards which are applied to all commercially supported Silverstripe CMS modules", + "license": "BSD-3-Clause", + "require": { + "php": "^8.1", + "phpstan/phpstan": "^1.10" + }, + "require-dev": { + "phpunit/phpunit": "^9.6", + "squizlabs/php_codesniffer": "^3.7", + "silverstripe/framework": "^5.2" + }, + "suggest": { + "phpstan/extension-installer": "Automatically includes rules from this repo in your phpstan config" + }, + "autoload": { + "psr-4": { + "SilverStripe\\Standards\\": "src/", + "SilverStripe\\Standards\\Tests\\": "tests/" + } + }, + "extra": { + "phpstan": { + "includes": [ + "rules.neon" + ] + } + }, + "minimum-stability": "dev", + "prefer-stable": true +} diff --git a/phpcs.xml.dist b/phpcs.xml.dist new file mode 100644 index 0000000..c3b486b --- /dev/null +++ b/phpcs.xml.dist @@ -0,0 +1,10 @@ + + + + + + */tests/* + + ./src + ./tests + diff --git a/phpunit.xml.dist b/phpunit.xml.dist new file mode 100644 index 0000000..bae62f4 --- /dev/null +++ b/phpunit.xml.dist @@ -0,0 +1,8 @@ + + + + + tests + + + diff --git a/rules.neon b/rules.neon new file mode 100644 index 0000000..5588bfd --- /dev/null +++ b/rules.neon @@ -0,0 +1,9 @@ +rules: + - SilverStripe\Standards\PHPStan\MethodAnnotationsRule + +parameters: + # Setting customRulestUsed to true allows us to avoid using the built-in rules + # until we're ready for them. For now, we only want to apply the specific rules + # we've implemented ourselves. + # In the future we can add a "level" config to re-enable built-in rules. + customRulesetUsed: true diff --git a/src/PHPStan/MethodAnnotationsRule.php b/src/PHPStan/MethodAnnotationsRule.php new file mode 100644 index 0000000..8619184 --- /dev/null +++ b/src/PHPStan/MethodAnnotationsRule.php @@ -0,0 +1,376 @@ + + */ +class MethodAnnotationsRule implements Rule +{ + private ReflectionProvider $reflectionProvider; + + public function __construct(ReflectionProvider $reflectionProvider) + { + $this->reflectionProvider = $reflectionProvider; + } + + public function getNodeType(): string + { + return Class_::class; + } + + public function processNode(Node $node, Scope $scope): array + { + /** @var Class_ $node */ + $className = $node->namespacedName->toString(); + if (!$this->reflectionProvider->hasClass($className)) { + return []; + } + + $classReflection = $this->reflectionProvider->getClass($className); + if (!$this->isDataObjectOrExtension($classReflection)) { + return []; + } + + $existingAnnotations = $this->getMethodAnnotations($classReflection); + $expectedAnnotations = $this->getExpectedAnnotations($classReflection); + + $errors = []; + + // Check that existing annotations are all valid + foreach ($existingAnnotations as $methodName => $annotationData) { + $annotationString = $annotationData['annotationString']; + $returnString = $annotationData['returnString']; + + // Complain about any @method annotations we don't expect + if (!array_key_exists($methodName, $expectedAnnotations)) { + $errors[] = RuleErrorBuilder::message( + "@method annotation '$annotationString' isn't expected and should be removed." + )->build(); + continue; + } + + // Complain about @method annotations if a real method exists with that name + if ($classReflection->hasNativeMethod($methodName)) { + $errors[] = RuleErrorBuilder::message( + "@method annotation '$annotationString' should be removed," + . " because a method named '$methodName' already exists." + )->build(); + continue; + } + + // Complain about duplicated @method annotations + if ($annotationData['count'] > 1) { + $count = $annotationData['count']; + $errors[] = RuleErrorBuilder::message( + "@method annotation '$annotationString' appears $count times. Remove the duplicates." + )->build(); + continue; + } + + $expectedAnnotationString = $expectedAnnotations[$methodName]['annotationString']; + $expectedReturnString = $expectedAnnotations[$methodName]['returnString']; + + if ($returnString !== $expectedReturnString) { + // Complain about missing use statements + $fullReturnType = $annotationData['returnType']; + if ($fullReturnType instanceof ObjectType) { + $returnClassName = $fullReturnType->getClassName(); + if (!$this->reflectionProvider->hasClass($returnClassName)) { + $shortClassName = $this->getShortClassName($returnClassName); + $errors[] = RuleErrorBuilder::message( + "@method annotation '$annotationString' needs a use statement for class '$shortClassName'." + )->build(); + continue; + } + } + + // Complain about type mismatches + $errors[] = RuleErrorBuilder::message( + "@method annotation '$annotationString' should be '$expectedAnnotationString'." + )->build(); + continue; + } + + // Complain about annotation strings looking different than what we expect. + // Can be caused e.g. by adding parameters to the method annotation, which should not be included. + if ($annotationString !== $expectedAnnotationString) { + $errors[] = RuleErrorBuilder::message( + "@method annotation '$annotationString' should be '$expectedAnnotationString'." + )->build(); + continue; + } + } + + // Complain about any missing annotations. + // Note that if an @method annotation is malformed, it will also be reported as missing. + $missingAnnotations = array_diff_key($expectedAnnotations, $existingAnnotations); + foreach ($missingAnnotations as $methodName => $missingData) { + // Skip if theres a method with that name + if ($classReflection->hasNativeMethod($methodName)) { + continue; + } + $expectedAnnotationString = $missingData['annotationString']; + $errors[] = RuleErrorBuilder::message( + "@method annotation '$expectedAnnotationString' is missing or has an invalid syntax." + )->build(); + } + + return $errors; + } + + /** + * Check if the class represented by the ClassReflection object is DataObject, + * Extension, or subclass of either of those. + */ + private function isDataObjectOrExtension(ClassReflection $reflection): bool + { + return $this->isDataObject($reflection) || $this->isExtension($reflection); + } + + /** + * Check if the class represented by the ClassReflection object is DataObject, + * Extension, or subclass of either of those. + */ + private function isDataObject(ClassReflection $reflection): bool + { + return $reflection->getName() === DataObject::class + || $reflection->isSubclassOf(DataObject::class); + } + + /** + * Check if the class represented by the ClassReflection object is Extension (or subclass). + */ + private function isExtension(ClassReflection $reflection): bool + { + return $reflection->getName() === Extension::class + || $reflection->isSubclassOf(Extension::class); + } + + /** + * Get all `@method` annotations for the class represented by the ClassReflection object. + */ + private function getMethodAnnotations(ClassReflection $reflection): array + { + $phpDoc = $reflection->getResolvedPhpDoc(); + if (!$phpDoc) { + return []; + } + + $annotations = []; + foreach ($phpDoc->getPhpDocNodes() as $phpDocNode) { + $methodNodes = $phpDocNode->getMethodTagValues(); + $resolvedMethodNodes = $phpDoc->getMethodTags(); + + foreach ($methodNodes as $node) { + $methodName = $node->methodName; + if (isset($annotations[$methodName])) { + $annotations[$methodName]['count']++; + continue; + } + $returnType = $resolvedMethodNodes[$methodName]->getReturnType(); + $annotations[$methodName] = [ + 'returnString' => $returnType->describe(VerbosityLevel::typeOnly()), + 'returnType' => $returnType, + // Ignore the description - it's fine if the developer adds a description for the method, + // but we won't validate it + 'annotationString' => trim(str_replace($node->description, '', $node->__toString())), + 'count' => 1, + ]; + } + } + + return $annotations; + } + + /** + * Get all expected `@method` annotations for the class represented by the ClassReflection object. + * + * This is based on the relations defined in config on that class directly. + * Config defined on extensions will only count towards the expected annotations for that extension class. + * Config defined in yaml doesn't count towards any expected annotations. + * If the class has an actual defined method with the same name, it WILL be returned from this method, and needs + * to be checked against separately. + */ + private function getExpectedAnnotations(ClassReflection $reflection): array + { + $expected = []; + + // Get any has_one and belongs_to relation methods + $hasOne = array_merge( + $this->getDefaultConfigValue($reflection, 'has_one'), + $this->getDefaultConfigValue($reflection, 'belongs_to') + ); + foreach ($hasOne as $name => $spec) { + // has_one can be defined with a special associative array + if (is_array($spec)) { + // If there's no 'class' key, the has_one is malformed and we can't infer what is expected. + // This rule isn't for validating config so we won't give an error. + // In that case just assume it's polymorphic and move on. + $className = $spec['class'] ?? DataObject::class; + } else { + $className = $spec; + } + // Remove any dot notation (for belongs_to) to get the true class name + $className = strtok($className, '.'); + $shortClassName = $this->getShortClassName($className); + $expected[$name] = [ + 'returnString' => $className, + 'annotationString' => "{$shortClassName} {$name}()", + ]; + } + + // Get any has_many relation methods + $hasMany = $this->getDefaultConfigValue($reflection, 'has_many'); + foreach ($hasMany as $name => $spec) { + // Remove any dot notation to get the class name + $className = strtok($spec, '.'); + $shortClassName = $this->getShortClassName($className); + // We don't need to be as specific as PolyMorphicHasManyList - there's too many edge cases + // and that's a subclass of HasManyList anyway. + $expected[$name] = [ + 'returnString' => HasManyList::class . "<$className>", + 'annotationString' => "HasManyList<{$shortClassName}> {$name}()", + ]; + } + + // Get any many_many relation methods + $manyMany = array_merge( + $this->getDefaultConfigValue($reflection, 'many_many'), + $this->getDefaultConfigValue($reflection, 'belongs_many_many') + ); + foreach ($manyMany as $name => $spec) { + if (is_array($spec)) { + $className = $this->getClassFromManyManyThrough($spec); + $listClass = ManyManyThroughList::class; + } else { + // Remove any dot notation to get the class name + $className = strtok($spec, '.'); + $listClass = ManyManyList::class; + } + $shortClassName = $this->getShortClassName($className); + $shortListClass = $this->getShortClassName($listClass); + $expected[$name] = [ + 'returnString' => "{$listClass}<{$className}>", + 'annotationString' => "{$shortListClass}<{$shortClassName}> {$name}()", + ]; + } + + return $expected; + } + + /** + * Get the default value for the given configuration property. + * Ignores YAML, config inherited from extensions, and config inherited from superclasses. + */ + private function getDefaultConfigValue(ClassReflection $class, string $propertyName): array + { + return array_merge( + $this->getDefaultConfigFromProperty($class, $propertyName), + $this->getDefaultConfigFromMethod($class, $propertyName) + ); + } + + /** + * Get default configuration defined using a private static property. + */ + private function getDefaultConfigFromProperty(ClassReflection $class, string $propertyName): array + { + if (!$class->hasNativeProperty($propertyName)) { + return []; + } + + $property = $class->getNativeProperty($propertyName); + + if (!$property->isStatic() || !$property->isPrivate()) { + return []; + } + + /** @var ReflectionProperty $nativeProperty */ + $nativeProperty = $property->getNativeReflection(); + $value = $nativeProperty->getDefaultValue(); + + return is_array($value) ? $value : []; + } + + /** + * Get configuration defined using the public static get_extra_config() method. + */ + private function getDefaultConfigFromMethod(ClassReflection $class, string $propertyName): array + { + if (!$this->isExtension($class) || !$class->hasNativeMethod('get_extra_config')) { + return []; + } + + $method = $class->getNativeMethod('get_extra_config'); + $className = $class->getName(); + + if ( + !$method->isStatic() + || !$method->isPublic() + || $method->getDeclaringClass()->getName() !== $className + ) { + return []; + } + + $allExtraConfig = $className::get_extra_config(DataObject::class, $className, []); + $value = $allExtraConfig[$propertyName] ?? []; + return is_array($value) ? $value : []; + } + + /** + * Get the name of the class that will be contained in a ManyManyThroughList + */ + private function getClassFromManyManyThrough(array $spec): string + { + if (!isset($spec['through']) || !isset($spec['to'])) { + // Malformed many_many - we can't infer what is expected here. + // This rule isn't for validating config so we won't give an error. + // Just assume it's polymorphic and move on. + return DataObject::class; + } + + $throughClass = $spec['through']; + if (!$this->reflectionProvider->hasClass($throughClass)) { + // Probably malformed many_many. Again, just assume it's polymorphic. + return DataObject::class; + } + + $classReflection = $this->reflectionProvider->getClass($throughClass); + $hasOne = $this->getDefaultConfigValue($classReflection, 'has_one'); + + // If that relation isn't there, it's a malformed through class. + // Again, just assume it's polymorphic in that scenario. + return $hasOne[$spec['to']] ?? DataObject::class; + } + + /** + * Get the class name portion of a FQCN + */ + private function getShortClassName(string $className): string + { + $parts = explode('\\', $className); + return end($parts); + } +} diff --git a/tests/PHPStan/MethodAnnotationsRuleTest.php b/tests/PHPStan/MethodAnnotationsRuleTest.php new file mode 100644 index 0000000..2f77f8a --- /dev/null +++ b/tests/PHPStan/MethodAnnotationsRuleTest.php @@ -0,0 +1,133 @@ + + */ +class MethodAnnotationsRuleTest extends RuleTestCase +{ + protected function getRule(): Rule + { + $reflectionProvider = $this->createReflectionProvider(); + return new MethodAnnotationsRule($reflectionProvider); + } + + public function provideRule() + { + // phpcs:disable Generic.Files.LineLength.TooLong + $lineForErrors = 37; + $errors = [ + 'duplicated annotation' => [ + '@method annotation \'SiteTree NormalHasOne()\' appears 2 times. Remove the duplicates.', + $lineForErrors, + ], + 'wrong type 2' => [ + '@method annotation \'ManyManyList NormalManyMany()\' should be \'ManyManyList NormalManyMany()\'.', + $lineForErrors, + ], + 'method already exists' => [ + '@method annotation \'ManyManyList TheresAMethodWithThisName()\' should be removed, because a method named \'TheresAMethodWithThisName\' already exists.', + $lineForErrors, + ], + 'unnecessary annotation 1' => [ + '@method annotation \'ManyManyList NoManyManyForThis()\' isn\'t expected and should be removed.', + $lineForErrors, + ], + 'unnecessary annotation 2' => [ + '@method annotation \'Member NoHasOneForThis()\' isn\'t expected and should be removed.', + $lineForErrors, + ], + 'unnecessary annotation 3' => [ + '@method annotation \'string completely_left_field()\' isn\'t expected and should be removed.', + $lineForErrors, + ], + 'shouldnt have params 1' => [ + '@method annotation \'Group DotNotationBelongsTo(string $paramShouldntBeHere)\' should be \'Group DotNotationBelongsTo()\'.', + $lineForErrors, + ], + 'shouldnt have params 2' => [ + '@method annotation \'ManyManyThroughList ManyManyThrough(int $idShouldntBeHere)\' should be \'ManyManyThroughList ManyManyThrough()\'.', + $lineForErrors, + ], + 'wrong type 1' => [ + '@method annotation \'SS_List PolyMorphicManyMany()\' should be \'ManyManyList PolyMorphicManyMany()\'.', + $lineForErrors, + ], + 'missing use statement 1' => [ + '@method annotation \'HasManyList NormalHasMany()\' needs a use statement for class \'HasManyList\'.', + $lineForErrors, + ], + 'missing use statement 2' => [ + '@method annotation \'HasManyList DotNotationHasMany()\' needs a use statement for class \'HasManyList\'.', + $lineForErrors, + ], + 'outright missing annotation' => [ + '@method annotation \'DataObject PolymorphicHasOne()\' is missing or has an invalid syntax.', + $lineForErrors, + ], + 'poorly formed annotation 1' => [ + '@method annotation \'Member NormalBelongsTo()\' is missing or has an invalid syntax.', + $lineForErrors, + ], + 'poorly formed annotation 2' => [ + '@method annotation \'ManyManyList DotNotationManyMany()\' is missing or has an invalid syntax.', + $lineForErrors, + ], + ]; + return [ + 'DataObjectAllPropertiesNoErrors.php' => [ + 'filePath' => __DIR__ . '/MethodAnnotationsRuleTest/DataObjectAllPropertiesNoErrors.php', + 'expectedErrors' => [], + ], + 'ExtensionMixedNoErrors.php' => [ + 'filePath' => __DIR__ . '/MethodAnnotationsRuleTest/ExtensionMixedNoErrors.php', + 'expectedErrors' => [], + ], + 'DataObjectNoAnnotations.php' => [ + 'filePath' => __DIR__ . '/MethodAnnotationsRuleTest/DataObjectNoAnnotations.php', + 'expectedErrors' => [], + ], + 'NotAnExtensionOrDataObject.php' => [ + 'filePath' => __DIR__ . '/MethodAnnotationsRuleTest/NotAnExtensionOrDataObject.php', + 'expectedErrors' => [], + ], + 'DataObjectAllPropertiesAllErrors.php' => [ + 'filePath' => __DIR__ . '/MethodAnnotationsRuleTest/DataObjectAllPropertiesAllErrors.php', + 'expectedErrors' => $errors, + ], + 'DataObjectAllConfigFromMethodIgnored.php' => [ + 'filePath' => __DIR__ . '/MethodAnnotationsRuleTest/DataObjectAllConfigFromMethodIgnored.php', + 'expectedErrors' => [ + [ + '@method annotation \'DataObject PolymorphicHasOne()\' is missing or has an invalid syntax.', + 16 + ], + [ + '@method annotation \'HasManyList DotNotationHasMany()\' is missing or has an invalid syntax.', + 16 + ], + ], + ], + 'ExtensionMixedAllErrors.php' => [ + 'filePath' => __DIR__ . '/MethodAnnotationsRuleTest/ExtensionMixedAllErrors.php', + 'expectedErrors' => $errors, + ], + ]; + // phpcs:enable Generic.Files.LineLength.TooLong + } + + /** + * @dataProvider provideRule + */ + public function testRule(string $filePath, array $expectedErrors): void + { + $this->analyse([$filePath], $expectedErrors); + } +} diff --git a/tests/PHPStan/MethodAnnotationsRuleTest/DataObjectAllConfigFromMethodIgnored.php b/tests/PHPStan/MethodAnnotationsRuleTest/DataObjectAllConfigFromMethodIgnored.php new file mode 100644 index 0000000..d1870fc --- /dev/null +++ b/tests/PHPStan/MethodAnnotationsRuleTest/DataObjectAllConfigFromMethodIgnored.php @@ -0,0 +1,56 @@ + 'Varchar', + ]; + + private static $has_one = [ + 'PolymorphicHasOne' => DataObject::class, + ]; + + private static $has_many = [ + 'DotNotationHasMany' => Group::class . '.Parent', + ]; + + public static function get_extra_config($class, $extension, $args): array + { + return [ + 'has_one' => [ + 'NormalHasOne' => SiteTree::class, + ], + 'has_many' => [ + 'DotNotationHasMany' => Group::class . '.Parent', + ], + 'many_many' => [ + 'ManyManyThrough' => [ + 'through' => ManyManyThroughModel::class, + 'to' => 'To', + 'from' => 'From', + ], + 'PolyMorphicManyMany' => DataObject::class, + ], + 'belongs_to' => [ + 'NormalBelongsTo' => Member::class, + 'DotNotationBelongsTo' => Group::class . '.Parent', + ], + 'belongs_many_many' => [ + 'DotNotationManyMany' => Member::class . '.Groups', + ], + ]; + } +} diff --git a/tests/PHPStan/MethodAnnotationsRuleTest/DataObjectAllPropertiesAllErrors.php b/tests/PHPStan/MethodAnnotationsRuleTest/DataObjectAllPropertiesAllErrors.php new file mode 100644 index 0000000..58bba93 --- /dev/null +++ b/tests/PHPStan/MethodAnnotationsRuleTest/DataObjectAllPropertiesAllErrors.php @@ -0,0 +1,77 @@ + NormalManyMany() + * @method ManyManyList TheresAMethodWithThisName() + * @method ManyManyList NoManyManyForThis() + * @method Member NoHasOneForThis() + * @method string completely_left_field() + * @method Group DotNotationBelongsTo(string $paramShouldntBeHere) + * @method ManyManyThroughList ManyManyThrough(int $idShouldntBeHere) + * @method Member NormalBelongsTo + * @method DotNotationManyMany: ManyManyList + * @method SS_List PolyMorphicManyMany() + * @method HasManyList NormalHasMany() Descriptions can be on any method annotation, not just has_one + * @method HasManyList DotNotationHasMany() + * @method SiteTree NormalHasOne() duplicated + */ +class DataObjectAllPropertiesAllErrors extends DataObject implements TestOnly +{ + private static $db = [ + 'ThisGetsIgnored' => 'Varchar', + ]; + + private static $has_one = [ + 'NormalHasOne' => SiteTree::class, + 'PolymorphicHasOne' => DataObject::class, + ]; + + private static $has_many = [ + 'NormalHasMany' => Member::class, + 'DotNotationHasMany' => Group::class . '.Parent', + ]; + + private static $many_many = [ + 'NormalManyMany' => Permission::class, + 'ManyManyThrough' => [ + 'through' => ManyManyThroughModel::class, + 'to' => 'To', + 'from' => 'From', + ], + 'PolyMorphicManyMany' => DataObject::class, + 'TheresAMethodWithThisName' => Permission::class, + ]; + + private static $belongs_to = [ + 'NormalBelongsTo' => Member::class, + 'DotNotationBelongsTo' => Group::class . '.Parent', + ]; + + private static $belongs_many_many = [ + 'DotNotationManyMany' => Member::class . '.Groups', + ]; + + public function TheresAMethodWithThisName() + { + //no-op + } +} diff --git a/tests/PHPStan/MethodAnnotationsRuleTest/DataObjectAllPropertiesNoErrors.php b/tests/PHPStan/MethodAnnotationsRuleTest/DataObjectAllPropertiesNoErrors.php new file mode 100644 index 0000000..c1c02a5 --- /dev/null +++ b/tests/PHPStan/MethodAnnotationsRuleTest/DataObjectAllPropertiesNoErrors.php @@ -0,0 +1,85 @@ + NormalHasMany() Descriptions can be on any method annotation, not just has_one + * @method HasManyList DotNotationHasMany() + * @method ManyManyList NormalManyMany() + * @method ManyManyThroughList ManyManyThrough() + * @method ManyManyList PolyMorphicManyMany() + * @method ManyManyList NormalBelongsMany() + * @method ManyManyList DotNotationManyMany() + * @method Member NormalBelongsTo() + * @method Group DotNotationBelongsTo() + */ +class DataObjectAllPropertiesNoErrors extends DataObject implements TestOnly +{ + private static $db = [ + 'ThisGetsIgnored' => 'Varchar', + ]; + + private static $has_one = [ + 'NormalHasOne' => SiteTree::class, + 'PolymorphicHasOne' => DataObject::class, + 'MultiRelationalHasOne' => [ + 'class' => DataObject::class, + DataObjectSchema::HAS_ONE_MULTI_RELATIONAL => true, + ], + 'WeirdButStilValidHasOne' => [ + 'class' => Permission::class, + ], + ]; + + private static $has_many = [ + 'NormalHasMany' => Member::class, + 'DotNotationHasMany' => Group::class . '.Parent', + ]; + + private static $many_many = [ + 'NormalManyMany' => Permission::class, + 'ManyManyThrough' => [ + 'through' => ManyManyThroughModel::class, + 'to' => 'To', + 'from' => 'From', + ], + 'PolyMorphicManyMany' => DataObject::class, + 'TheresAMethodWithThisName' => Permission::class, + ]; + + private static $belongs_to = [ + 'NormalBelongsTo' => Member::class, + 'DotNotationBelongsTo' => Group::class . '.Parent', + ]; + + private static $belongs_many_many = [ + 'NormalBelongsMany' => Group::class, + 'DotNotationManyMany' => Member::class . '.Groups', + ]; + + public function TheresAMethodWithThisName() + { + //no-op + } +} diff --git a/tests/PHPStan/MethodAnnotationsRuleTest/DataObjectNoAnnotations.php b/tests/PHPStan/MethodAnnotationsRuleTest/DataObjectNoAnnotations.php new file mode 100644 index 0000000..b61659c --- /dev/null +++ b/tests/PHPStan/MethodAnnotationsRuleTest/DataObjectNoAnnotations.php @@ -0,0 +1,23 @@ + 'Varchar', + ]; + + private static $has_one = []; + + private static $has_many = []; + + private static $many_many = []; + + private static $belongs_to = []; + + private static $belongs_many_many = []; +} diff --git a/tests/PHPStan/MethodAnnotationsRuleTest/ExtensionMixedAllErrors.php b/tests/PHPStan/MethodAnnotationsRuleTest/ExtensionMixedAllErrors.php new file mode 100644 index 0000000..00481da --- /dev/null +++ b/tests/PHPStan/MethodAnnotationsRuleTest/ExtensionMixedAllErrors.php @@ -0,0 +1,91 @@ + NormalManyMany() + * @method ManyManyList TheresAMethodWithThisName() + * @method ManyManyList NoManyManyForThis() + * @method Member NoHasOneForThis() + * @method string completely_left_field() + * @method Group DotNotationBelongsTo(string $paramShouldntBeHere) + * @method ManyManyThroughList ManyManyThrough(int $idShouldntBeHere) + * @method Member NormalBelongsTo + * @method DotNotationManyMany: ManyManyList + * @method SS_List PolyMorphicManyMany() + * @method HasManyList NormalHasMany() Descriptions can be on any method annotation, not just has_one + * @method HasManyList DotNotationHasMany() + * @method SiteTree NormalHasOne() duplicated + */ +class ExtensionMixedAllErrors extends Extension implements TestOnly +{ + private static $db = [ + 'ThisGetsIgnored' => 'Varchar', + ]; + + private static $has_many = [ + 'DotNotationHasMany' => Group::class . '.Parent', + ]; + + private static $many_many = [ + 'PolyMorphicManyMany' => DataObject::class, + 'TheresAMethodWithThisName' => Permission::class, + ]; + + private static $belongs_to = [ + 'NormalBelongsTo' => Member::class, + ]; + + private static $belongs_many_many = [ + 'DotNotationManyMany' => Member::class . '.Groups', + ]; + + public static function get_extra_config($class, $extension, $args): array + { + return [ + 'db' => [ + 'ThisGetsIgnored' => 'Varchar', + ], + 'has_one' => [ + 'NormalHasOne' => SiteTree::class, + 'PolymorphicHasOne' => $class, + ], + 'has_many' => [ + 'NormalHasMany' => Member::class, + ], + 'many_many' => [ + 'NormalManyMany' => Permission::class, + 'ManyManyThrough' => [ + 'through' => ManyManyThroughModel::class, + 'to' => 'To', + 'from' => 'From', + ], + ], + 'belongs_to' => [ + 'DotNotationBelongsTo' => Group::class . '.Parent', + ], + ]; + } + + public function TheresAMethodWithThisName() + { + //no-op + } +} diff --git a/tests/PHPStan/MethodAnnotationsRuleTest/ExtensionMixedNoErrors.php b/tests/PHPStan/MethodAnnotationsRuleTest/ExtensionMixedNoErrors.php new file mode 100644 index 0000000..0d536ba --- /dev/null +++ b/tests/PHPStan/MethodAnnotationsRuleTest/ExtensionMixedNoErrors.php @@ -0,0 +1,100 @@ + NormalHasMany() Descriptions can be on any method annotation, not just has_one + * @method HasManyList DotNotationHasMany() + * @method ManyManyList NormalManyMany() + * @method ManyManyThroughList ManyManyThrough() + * @method ManyManyList PolyMorphicManyMany() + * @method ManyManyList NormalBelongsMany() + * @method ManyManyList DotNotationManyMany() + * @method Member NormalBelongsTo() + * @method Group DotNotationBelongsTo() + */ +class ExtensionMixedNoErrors extends Extension implements TestOnly +{ + private static $db = [ + 'ThisGetsIgnored' => 'Varchar', + ]; + + private static $has_many = [ + 'NormalHasMany' => Member::class, + 'DotNotationHasMany' => Group::class . '.Parent', + ]; + + private static $many_many = [ + 'NormalManyMany' => Permission::class, + ]; + + private static $belongs_to = [ + 'DotNotationBelongsTo' => Group::class . '.Parent', + ]; + + private static $belongs_many_many = [ + 'NormalBelongsMany' => Group::class, + 'TheresAMethodWithThisName' => Permission::class, + ]; + + public static function get_extra_config($class, $extension, $args): array + { + return [ + 'db' => [ + 'ThisGetsIgnored' => 'Varchar', + ], + 'has_one' => [ + 'NormalHasOne' => SiteTree::class, + 'PolymorphicHasOne' => $class, + 'MultiRelationalHasOne' => [ + 'class' => DataObject::class, + DataObjectSchema::HAS_ONE_MULTI_RELATIONAL => true, + ], + 'WeirdButStilValidHasOne' => [ + 'class' => Permission::class, + ], + ], + 'many_many' => [ + 'ManyManyThrough' => [ + 'through' => ManyManyThroughModel::class, + 'to' => 'To', + 'from' => 'From', + ], + 'PolyMorphicManyMany' => DataObject::class, + ], + 'belongs_to' => [ + 'NormalBelongsTo' => Member::class, + ], + 'belongs_many_many' => [ + 'DotNotationManyMany' => Member::class . '.Groups', + ], + ]; + } + + public function TheresAMethodWithThisName() + { + //no-op + } +} diff --git a/tests/PHPStan/MethodAnnotationsRuleTest/ManyManyThroughModel.php b/tests/PHPStan/MethodAnnotationsRuleTest/ManyManyThroughModel.php new file mode 100644 index 0000000..a494fd2 --- /dev/null +++ b/tests/PHPStan/MethodAnnotationsRuleTest/ManyManyThroughModel.php @@ -0,0 +1,15 @@ + Member::class, + 'From' => DataObject::class, + ]; +} diff --git a/tests/PHPStan/MethodAnnotationsRuleTest/NotAnExtensionOrDataObject.php b/tests/PHPStan/MethodAnnotationsRuleTest/NotAnExtensionOrDataObject.php new file mode 100644 index 0000000..69f8e76 --- /dev/null +++ b/tests/PHPStan/MethodAnnotationsRuleTest/NotAnExtensionOrDataObject.php @@ -0,0 +1,20 @@ + DataObject::class, + ]; +}