Improve type hinting in various classes #23811
Conversation
sgiehl
added
not-in-changelog
sgiehl
force-pushed
the
dev-19713
branch
2 times, most recently
from
1d6db1b to
5e516bd
Compare
| * @return mixed | ||
| * @template T of object | ||
| * @param class-string<T>|string $name Container entry name. | ||
| * @return ($name is class-string<T> ? T : mixed) |
There was a problem hiding this comment.
This should ensure that for something like StaticContainer('Piwik/Auth') an object of Piwik/Auth will be expected as returned value.
| { | ||
| $tasks = array(); | ||
|
|
||
| /** @var Tasks[] $pluginTasks */ |
There was a problem hiding this comment.
Incorrect type hint, as findComponents actually returns class names not objects.
| public static function findAvailableSmsProviders() | ||
| { | ||
| /** @var SMSProvider[] $smsProviders */ | ||
| $smsProviders = Plugin\Manager::getInstance()->findMultipleComponents('SMSProvider', 'Piwik\Plugins\MobileMessaging\SMSProvider'); |
There was a problem hiding this comment.
findMultipleComponents returns an array of class names, not objects.
| } | ||
|
|
||
| $auth = StaticContainer::get('Piwik\Auth'); | ||
| if ($auth && !$auth->getLogin() && method_exists($auth, 'getTokenAuth') && $auth->getTokenAuth()) { |
There was a problem hiding this comment.
If Piwik\Auth object can't be created an exception should be thrown, so checking for its existence should be unneeded.
| $string = $env->getRuntime(\Twig\Runtime\EscaperRuntime::class) | ||
| ->escape($string, $strategy, $charset, $autoescape); |
There was a problem hiding this comment.
´twig_escape_filter´ is deprecated since a while already, so using an alternative way to achieve the same result.
| $this->twig->addFilter($percentage); | ||
| } | ||
|
|
||
| private function getProfessionalServicesAdvertising() |
| * @param int $key (don't care) | ||
| * @param string $path Piwik root | ||
| */ | ||
| public static function addPiwikPath(&$value, $key, $path) |
|
|
||
| /** | ||
| * @template T of object | ||
| * @param class-string<T>|string $name Container entry name. |
There was a problem hiding this comment.
Should we add this too?
| * @param class-string<T>|string $name Container entry name. | |
| * @param class-string<T>|string $name Container entry name. | |
| * @throws NotFoundException |
There was a problem hiding this comment.
That should be "auto-detectable"
There was a problem hiding this comment.
I’m not sure to understand what you mean here. There is no need to precise it since it’s auto-detectable?
There was a problem hiding this comment.
I guess that's something we need to decide in general. I'm currently only trying to add type hints for those variables, where the code doesn't or can't define the explicitly. We can for sure consider to add doc blocks that contain everything, but that will also cause more maintaining work to keep everything up to date
Description
Checklist
Review