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

Fix generics signature of config() and model() #7660

Conversation

paulbalandan
Copy link
Member

Description
Since the current undocumented behavior of config() and model() is to allow any class string as name input and return their respective instances, then I believe we should change the generics signature to account this fact.

How to test

  1. Create a test file: test.php
<?php

use function PHPStan\dumpType;

dumpType(config(stdClass::class));
dumpType(config('Config\App'));
dumpType(config('App'));
  1. Run vendor/bin/phpstan analyse -v test.php
  2. The dumped types would be:
------ ------------------------- 
  Line   test.php
 ------ -------------------------
  :5     Dumped type: stdClass
  :6     Dumped type: Config\App
  :7     Dumped type: App|null
 ------ -------------------------

In line 7, PHPStan gives the inferred type as App|null since it does not know if App is a valid class string or not. In PHPStan parlance, this is a "maybe" class string, thus it can be App or null. More precise type inference will come once our own phpstan extension is out, which is currently in development.

This PR also fixes the resulting error in code in Toolbar.

 ------ ----------------------------------------------------------------------------------------------------------------------------------------------- 
  Line   system\Debug\Toolbar.php
 ------ -----------------------------------------------------------------------------------------------------------------------------------------------   
  :368   Parameter #1 $config of static method CodeIgniter\Config\BaseService::toolbar() expects Config\Toolbar|null, CodeIgniter\Debug\Toolbar given.
 ------ -----------------------------------------------------------------------------------------------------------------------------------------------   

Related: #7224 #7254

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@paulbalandan paulbalandan added the refactor Pull requests that refactor code label Jul 6, 2023
kenjis
kenjis previously approved these changes Jul 6, 2023
Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

I would prefer to see the stricter definitions. Our User Guide sets the expectation:

... which returns a new instance of a Config class...

... returns a new instance of a Model class...

Just because they can be used to return another class in the same directory doesn't mean we want that to happen. We could tighten Factories by adding instanceOf directives to the calls if there is a problem with this not being explicit.

@paulbalandan
Copy link
Member Author

@MGatner thank you for your opinion. If the behavior is unintended, then we should fix those two functions.
You can read more on the discussion in slack.

@paulbalandan
Copy link
Member Author

If we are to enforce instanceOf checks, in config() we cannot call the following as they do not extend BaseConfig:

  • Config\Autoload
  • Config\DocTypes
  • Config\ForeignCharacters
  • Config\Mimes
  • Config\Modules
  • Config\Paths
  • Config\Services

So, tightening the two functions can be breaking changes.

@MGatner
Copy link
Member

MGatner commented Jul 7, 2023

I responded on Slack. I don't think all of those Config files need to be loadable via config(), and I would actually argue that many of them aren't Config files at all but something else. Mimes isn't really even a class, just an organizing unit - it could maybe even become an Enum. DocTypes ought to extend BaseConfig, as another example. I think we could make config() work with some caveats in the User Guide.

@kenjis
Copy link
Member

kenjis commented Jul 13, 2023

These classes are required before the Autoloader instantiation:

  • Config\Autoload
  • Config\Modules
  • Config\Paths

@kenjis
Copy link
Member

kenjis commented Jul 25, 2023

I sent #7735 to fix the bug in this PR.

@kenjis kenjis dismissed their stale review July 27, 2023 05:00

return ?object does not help.

@paulbalandan paulbalandan deleted the config-model-phpstan-signature branch August 25, 2023 05:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Pull requests that refactor code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants