CodeIgniter / phpstan-codeigniter

CodeIgniter extensions and rules for PHPStan
https://codeigniter.com/
MIT License
15 stars 0 forks source link

bug: phpstan-codeigniter forces a model class extends `CodeIgniter\Model` #8

Closed kenjis closed 1 year ago

kenjis commented 1 year ago

PHP Version

8.1

PHPStan CodeIgniter Version

v1.4.2.70400

PHPStan Version

1.10.39

What happened?

 ------ ---------------------------------------------------------------------------------------------------- 
  Line   app/Controllers/ActionsController.php                                                               
 ------ ---------------------------------------------------------------------------------------------------- 
  46     Argument #1 $name ('App\\Models\\NotificationMutedModel') passed to function model does not extend  
         CodeIgniter\\Model.                                                                                 
 ------ ----------------------------------------------------------------------------------------------------

https://github.com/kenjis/forum-example/blob/37a339f050ee2c40ed452e6570cef188dc0f90a0/app/Controllers/ActionsController.php#L46 https://github.com/kenjis/forum-example/blob/37a339f050ee2c40ed452e6570cef188dc0f90a0/app/Models/NotificationMutedModel.php#L11

Minimum Reproduction Script

git clone https://github.com/kenjis/forum-example --branch=add-phpstan-codeigniter
cd forum-example/
composer install
vendor/bin/phpstan

Expected Output

No error on ActionsController

paulbalandan commented 1 year ago

You can turn this rule off:

Checks if the string argument passed to config() or model() function is a valid class string extending CodeIgniter\Config\BaseConfig or CodeIgniter\Model, respectively. This can be turned off by setting codeigniter.checkArgumentTypeOfFactories: false in your phpstan.neon.

kenjis commented 1 year ago

The config also disables the config() class checks like the following:

 ------ --------------------------------------------------------------------------------- 
  Line   app/Controllers/Home.php                                                         
 ------ --------------------------------------------------------------------------------- 
  11     Argument #1 $name ('Config\\Modules') passed to function config does not extend  
         CodeIgniter\\Config\\BaseConfig.                                                 
 ------ --------------------------------------------------------------------------------- 
paulbalandan commented 1 year ago

This is conditionally activated here: https://github.com/CodeIgniter/phpstan-codeigniter/blob/d4f77abcd44672075342e2b8647f583b734671c4/extension.neon#L84-L85

If I understand correctly, you want to enable check for config() but not for model(). The non-BC way to do it that I can think of is too add 2 new options (added to extension.neon) that will be passed to the constructor of CodeIgniter\PHPStan\Rules\Functions\FactoriesFunctionArgumentTypeRule. Then from there check if the resolved function name is either config or model then check if the user wants to bypass the check using the 2 new constructor parameters.

kenjis commented 1 year ago

you want to enable check for config() but not for model().

Yes! config() checking is needed if we use Config caching, and using config() to non-BaseConfig class is probably meaningless and using new is faster. But model() checking is not needed. Because CodeIgniter does not force us to extend the model. https://codeigniter4.github.io/CodeIgniter4/models/model.html#manual-model-creation

By the way, what was the type checking for model() implemented for in the first place?

paulbalandan commented 1 year ago

Because factories has a verifyInstanceof option. The opt-out feature strictly checks the instance in case that option is used. Think of it like "strict-rules" for this extension.