Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NEW Add method annotation checks #1

Merged

Conversation

GuySartorelli
Copy link
Member

@GuySartorelli GuySartorelli commented Jan 24, 2024

Since this doesn't have CI yet, you'll have to check the CI in creative-commoners instead of on this PR directly.
See https://github.com/creative-commoners/silverstripe-standards/actions/workflows/ci.yml

Note that the PDO failure will be fixed by silverstripe/gha-ci#97

How to test this manually

  1. Add this repo and phpstan/extension-installer as dev dependencies
  2. Add a phpstan.neon or phpstan.neon.dist file to your project root with this content:
    parameters:
      paths:
        - vendor/silverstripe/framework/src/Security
  3. Run vendor/bin/phpstan
    This will run it against the path set above, and due to the rules set in this repo it will only run our custom rule.
  4. It should be pretty quick, and the only failures you should see should relate to @method annotations.

Issue

@GuySartorelli GuySartorelli marked this pull request as draft January 24, 2024 01:38
@GuySartorelli GuySartorelli force-pushed the pulls/1/method-annotation branch 15 times, most recently from 56acbee to 3cbf892 Compare January 28, 2024 22:51
@GuySartorelli GuySartorelli marked this pull request as ready for review January 28, 2024 22:53
Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should also validate the that classes like HasManyList are imported when they're used for method annotations

src/PHPStan/MethodAnnotationsRule.php Show resolved Hide resolved
src/PHPStan/MethodAnnotationsRule.php Show resolved Hide resolved
@GuySartorelli
Copy link
Member Author

Should also validate the that classes like HasManyList are imported when they're used for method annotations

Do you mean checking for missing use statements? That's done already, see the code under the "Complain about missing use statements" comment.

Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally with steps in description, though I tried running on all of framework rather than just the Security dir. I got this:

Internal error: Internal error: Too few arguments to function                                  
SilverStripe\ORM\Hierarchy\Hierarchy::get_extra_config(), 0 passed in                          
/path/to/somesite/vendor/silverstripe/standards/src/PHPStan/MethodAnnotationsRule.php  
on line 320 and exactly 3 expected while analysing file                                        
/path/to/somesite/vendor/silverstripe/framework/src/ORM/Hierarchy/Hierarchy.php        
Run PHPStan with -v option and post the stack trace to:                                        
https://github.com/phpstan/phpstan/issues/new?template=Bug_report.yaml                         
Child process error (exit code 1):                                                             
Internal error: Internal error: Internal error. while analysing file                           
/path/to/somesite/vendor/silverstripe/framework/src/ORM/PaginatedList.php              
Run PHPStan with -v option and post the stack trace to:                                        
https://github.com/phpstan/phpstan/issues/new?template=Bug_report.yaml                         
Child process error (exit code 1):                                                             
Internal error: Internal error: Internal error. while analysing file                           
/path/to/somesite/vendor/silverstripe/framework/src/ORM/ListDecorator.php              
Run PHPStan with -v option and post the stack trace to:                                        
https://github.com/phpstan/phpstan/issues/new?template=Bug_report.yaml                         
Child process error (exit code 1):                                                             
Internal error: Internal error: Internal error. while analysing file                           
/path/to/somesite/vendor/silverstripe/framework/src/ORM/Connect/MySQLDatabase.php      
Run PHPStan with -v option and post the stack trace to:                                        
https://github.com/phpstan/phpstan/issues/new?template=Bug_report.yaml                         
Internal error: Internal error: Too few arguments to function                                  
SilverStripe\ORM\Search\FulltextSearchable::get_extra_config(), 0 passed                       
in                                                                                             
/path/to/somesite/vendor/silverstripe/standards/src/PHPStan/MethodAnnotationsRule.php  
on line 320 and exactly 3 expected while analysing file                                        
/path/to/somesite/vendor/silverstripe/framework/src/ORM/Search/FulltextSearchable.php  
Run PHPStan with -v option and post the stack trace to:                                        
https://github.com/phpstan/phpstan/issues/new?template=Bug_report.yaml                         
Internal error: Internal error: Internal error. while analysing file                           
/path/to/somesite/vendor/silverstripe/framework/src/ORM/GroupedList.php                
Run PHPStan with -v option and post the stack trace to:                                        
https://github.com/phpstan/phpstan/issues/new?template=Bug_report.yaml                         
Child process error (exit code 1):      

@GuySartorelli
Copy link
Member Author

Ahh, yes, looks like in my haste I didn't notice that the method signature for get_extra_config() actually takes arguments which can change the returned value. This will be interesting.

@GuySartorelli GuySartorelli force-pushed the pulls/1/method-annotation branch 2 times, most recently from 73a7b0e to de41123 Compare January 29, 2024 23:01
@GuySartorelli
Copy link
Member Author

Turns out the method is ignored if it's declared on a DataObject directly.
When it's declared on an Extension subclass, it has this signature:

/**
 * @param string $class The FQCN of the class being extended (will always be DataObject from PHPStan's pov)
 * @param string $extension The FQCN of this class
 * @param array $args Extra arguments as part of the extension configuration (will always be an empty array from PHPStan's pov)
 */
public static function get_extra_config($class, $extension, $args): array;

That's accounted for now.

@GuySartorelli
Copy link
Member Author

The remaining errors don't seem to be related to the custom rule. They occur before any of the code in this repository is executed. Those errors are:

Internal error: Internal error: Internal error. while analysing file                                
     /home/gsartorelli/projects/phpstan/vendor/silverstripe/framework/src/ORM/Connect/MySQLDatabase.php  
     Run PHPStan with -v option and post the stack trace to:                                             
     https://github.com/phpstan/phpstan/issues/new?template=Bug_report.yaml                              
     Child process error (exit code 1):                                                                  
     Internal error: Internal error: Internal error. while analysing file                                
     /home/gsartorelli/projects/phpstan/vendor/silverstripe/framework/src/ORM/GroupedList.php            
     Run PHPStan with -v option and post the stack trace to:                                             
     https://github.com/phpstan/phpstan/issues/new?template=Bug_report.yaml                              
     Internal error: Internal error: Internal error. while analysing file                                
     /home/gsartorelli/projects/phpstan/vendor/silverstripe/framework/src/ORM/PaginatedList.php          
     Run PHPStan with -v option and post the stack trace to:                                             
     https://github.com/phpstan/phpstan/issues/new?template=Bug_report.yaml                              
     Internal error: Internal error: Internal error. while analysing file                                
     /home/gsartorelli/projects/phpstan/vendor/silverstripe/framework/src/ORM/ListDecorator.php          
     Run PHPStan with -v option and post the stack trace to:                                             
     https://github.com/phpstan/phpstan/issues/new?template=Bug_report.yaml                              
     Child process error (exit code 1):

I've raised phpstan/phpstan#10509 about that, but IMO that shouldn't be a blocker for merging this PR (though it will be a blocker for adding the PHPstan config to the framework repo).

@GuySartorelli GuySartorelli force-pushed the pulls/1/method-annotation branch from de41123 to 47862a9 Compare January 29, 2024 23:36
@emteknetnz emteknetnz merged commit 2065608 into silverstripe:1 Jan 30, 2024
@emteknetnz emteknetnz deleted the pulls/1/method-annotation branch January 30, 2024 01:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants