codeigniter4 / CodeIgniter4

Open Source PHP Framework (originally from EllisLab)
https://codeigniter.com/
MIT License
5.3k stars 1.89k forks source link

Bug: Model::_call() static analysis #4509

Closed MGatner closed 2 years ago

MGatner commented 3 years ago

Describe the bug With the removal of @mixin Model from BaseBuilder, magic calls to Model that match a BaseBuilder method will now "return" that method's type, causing subsequent calls too create static analysis issues. For example:

$users = model(UserModel::class)->where('name', 'Bob');

$users is now a Model with its $builder property primed for the criterium, but because the @mixin BaseBuilder matched BaseBuild::where(): self PHPStan thinks $users is BaseBuilder. So calling a model method after this:

$user = $users->first();

... will cause static analysis to complain:

Call to an undefined method CodeIgniter\Database\BaseBuilder::first()

I have tried a few things, like adding @method to Model, but thus far I don't see a good solution.

CodeIgniter 4 version develop latest

Affected module(s) Model, BaseBuilder

Context

WinterSilence commented 3 years ago

@MGatner https://phpstan.org/developing-extensions/dynamic-return-type-extensions

MGatner commented 3 years ago

Thanks @WinterSilence I will take a look

WinterSilence commented 3 years ago

@MGatner for example

paulbalandan commented 3 years ago

Did you try adding an inline phpdoc?


/** @var \CodeIgniter\Model $users */
$users = (new UserModel)->where('name', 'foo');
$users->first();

PHPStan respects inline types regardless of the treatPhpDocTypesAsCertain setting.

WinterSilence commented 3 years ago

@paulbalandan it's less usefull than add @method tags to Model/BaseModel, but it's not clear solution. For example: now Model::__call() have @return $this|null, but in real Model::__call() return anything(@return mixed) and need write extension for phpstan

MGatner commented 3 years ago

@paulbalandan yes that was just an example. We've created this issue for ever Model in all projects, so we need to come up with a solution. @WinterSilence 's suggestion of a template I think will be the way to go, but I haven't had time to dig into it yet.

WinterSilence commented 3 years ago

@MGatner don't need to dig, just replace Nette\Utils\Html/Html in phpstan/phpstan-nette mem

jozefrebjak commented 3 years ago

@MGatner is there some solution to this issue ?

MGatner commented 3 years ago

@jozefrebjak I have not researched a templates solution at all.

kenjis commented 2 years ago

@WinterSilence

don't need to dig, just replace Nette\Utils\Html/Html in phpstan/phpstan-nette

If you could write PHPStan extensions easily, could you write and send PR for this issue? It is too difficult to understand.

WinterSilence commented 2 years ago

@kenjis you no need writes something new, just replace Nette\Utils\Html https://github.com/phpstan/phpstan-nette/blob/f4654b27b107241e052755ec187a0b1964541ba6/src/Reflection/Nette/HtmlClassReflectionExtension.php#L16 to your class name

kenjis commented 2 years ago

@WinterSilence Please, could you create a PR for us?

kenjis commented 2 years ago

It seems "Dynamic Return Type Extensions" does not solve the problem.

If the return type of a method is not always the same, but depends on an argument passed to the method, you can specify the return type by writing and registering an extension. https://phpstan.org/developing-extensions/dynamic-return-type-extensions

I propose that removing @mixin BaseBuilder and adding @method for methods which can be used in the Model.

--- a/system/Model.php
+++ b/system/Model.php
@@ -38,9 +38,10 @@ use ReflectionProperty;
  *      - allow intermingling calls to the builder
  *      - removes the need to use Result object directly in most cases
  *
- * @mixin BaseBuilder
- *
  * @property BaseConnection $db
+ *
+ * @method $this where($key, $value = null, ?bool $escape = null)
+ * @method $this select($select = '*', ?bool $escape = null)
  */
 class Model extends BaseModel
 {
kenjis commented 2 years ago

I sent a PR #5970