Closed MartinMystikJonas closed 1 year ago
Is there anything I can do to help this PR to be merged? It blocks some work on our phpstan-latte extension.
Ok I will add test.
I just checked and it seems tehre are not tests at all for components dynamic return types.
There is FormContainerValuesDynamicReturnTypeExtensionTest but I am not sure about usefulness of this type of tests. It looks like it result depends basically only on creatign right mocks that matches internal implementation.
Should I create similar test? I think it might be better to create RuleTestCase based test. But it would not test any rule just right types returned by dynamic return types extenions. So I am not sure which way to go now.
I'd expect your change to be tested via TypeInferenceTestCase.
OK. Tests added.
Thank you.
Thank you.
Two more changes after testing on a real-world project:
What is reason to change type back to mixed?
Is there any case where getComponent
or createComponent
could return something that is not IComponent
so it has to be widened to mixed
? I think it is not possible.
Fact that this extension forces getComponent
return type to mixed
if createComponent*
does not exists is reason why I implemented this in the first place because it breaks analysis of latte templates. In some cases (most notable example is Multiplier
) there are no createComponent*
methods to define type because creation of components is handled by overriden createComponent
. And in these cases it is impossible to specify type of created components because it is forced as mixed
and return type of createComponent
is ignored.
What is reason to change type back to mixed?
Because it's annoying in practice. Take for example code like this:
protected function setupForm(): void
{
$this->addSubmit('saveAndContinue', _('Uložit a pokračovat'));
$this->addSubmit('save', _('Uložit'));
}
/**
* @return list<SubmitButton>
*/
protected function getDraftButtons(): array
{
return [
$this->getComponent('saveAndContinue'),
$this->getComponent('save'),
];
}
PHPStan complained that IComponent was returned but SubmitButton was expected. This code is inside a Form
. PHPStan doesn't know the type of these components so mixed
is the best we can do in order to avoid false positives.
But it creates different kind of false positives on higher levels with no way to avoid them:
$this->getComponent('save')->redrawControl(); // Cannot call redrawControl() on mixed.
Also it breaks Multipler
and other similar components that redefine createComponent
.
Maybe made it level-dependent? Or at least keep type from createComponent
if it is overriden in component?
Type from createComponent
is kept, check the tests: https://github.com/phpstan/phpstan-nette/blob/0e3a6805917811d685e59bb83c2286315f2f6d78/tests/Type/Nette/data/componentModel.php#L53-L58
Sorry you are right. Our tests failed in different palce where it was caused by calling render()
on mixed
returned from getComponent
. I guess it is caused by use of checkImplicitMixed
maybe? Call of method in implicit mixed causes this kind error.
So maybe make behavior of this dependent on checkImplicitMixed
? If it is disabled use mixed nad when not use default type from createComponent
?
When used with combination with
Container
that overwritesgetComponent
(likeMultiplier
) return type was forced to mixed instead of keeping return type ofgetComponent
in casecreateComponent*
method was not present