-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
refactor: add Factories::models() to suppress PHPStan error #5358
Conversation
e7d322c
to
d382c70
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think this would be better handled by a stub file. I don't see the explicit method causing a problem right now but it will make maintenance harder.
I think this same docblock could be placed in a stub and work fine even though the method doesn't actually exist (not 100% sure on that but I believe so).
<?php
namespace CodeIgniter\Config;
use CodeIgniter\Database\ConnectionInterface;
use CodeIgniter\Model;
use Config\Services;
class Factories
{
/**
* @template T of Model
*
* @param class-string<T> $name
*
* @return T
*/
public static function models(string $name, array $options = [], ?ConnectionInterface &$conn = null);
} --- a/phpstan.neon.dist
+++ b/phpstan.neon.dist
@@ -61,3 +61,5 @@ parameters:
- CI_DEBUG
- ENVIRONMENT
- SODIUM_LIBRARY_VERSION
+ stubFiles:
+ - utils/PHPStan/stubs/Factories.stub But errors ocurres. Why invalid return type CodeIgniter\Model?
|
I will have to look on desktop. |
Any progress on this? The PHPStan errors are really annoying for both packages and projects... |
It seems that if any type is used in a stub file, there needs to be a stub file for this type, too. See the bottom notice here: |
Is there anyone a littlle more familiar with PHPStan? (Not me unfortunately...) I really think this should be handled somehow. In the worst case, my personal opinion would be that improving auto-complete for a single IDE (phpstorm, #5186) does not really justify having to suppress phpstan errors all across projects/packages (as these now occur in projects wherever methods of models derived from the |
It is not so. PhpStorm does not need the But removing it causes PHPStan error. I recommend to merge this PR. I don't know why everybody loves magic method. |
My ideal would still be to handle this with stub files but it seems like we lack the expertise currently to pull it off. I'm okay with proceeding since this is not a likely candidate for changes in the near future. |
How about merging for now and then create a new |
I wonder if adding |
Nevermind, that is what used to be there 🤦♂️ They don't support generics. |
I have tried this with stub files before, way before @kenjis tried in his other PR, and it doesn't work. A better solution would be I think is a PHPStan rule for this, but merging this for now is our best compromise. |
I don't like to make special rules/stubs for CI4. Essentially it should not be needed. Apart from my own preferences, this PR is a small hack and easily can be reverted anytime. |
Does anyone have the capacity to see what impact it would have on PHPStan and PHPStorm if we added |
I don't have phpstorm, but it should not be affected, as the fix for the autocompletion was the other changes that led to the PHPStan errors (that are fixed with this PR). I can try if it affects PHPStan when I'm home. |
Passes locally with |
d382c70
to
9096197
Compare
I guess not - those strikes will get us questions and issues. Maybe just add a note on the method stating something like "This method is supplied only to assist with static analysis and should not be relied on as a non-magic method." |
And that it is not covered by the BC promise, probably. |
@MGatner I added a commen already.
What do you mean? I don't understand what users should not do. |
Basically people should not use this method in their apps/modules/whatever, as it is intended for internal SA use only. |
@sfadschm What's SA ? It is not real |
Sorry, SA is static analysis. |
Yes, sorry to be unclear! I want this method to be considered "internal", but we can't flag it like that because IDEs will complain even though the "magic version" is acceptable. Basically, it is fine for people to do this: Factories::model('Foo'); But I don't want people's code to rely on this being there, so none of this: if (method_exists(Factories::class, 'model')) {
echo 'Modeling!';
} @kenjis i forgot you already had a note on there, I think what you said is fine. Let's merge, unless there are more thoughts? |
Description
Follow-up: #5186
Checklist: