psalm-plugin-symfony icon indicating copy to clipboard operation
psalm-plugin-symfony copied to clipboard

Add default FormView `vars` in the stub class

Open mpdude opened this issue 3 years ago • 9 comments

These keys are added to FormView::$vars here:

https://github.com/symfony/symfony/blob/4a22bcbda96f347cafd943236bd3029a9fef9467/src/Symfony/Component/Form/Extension/Core/Type/FormType.php#L107

That's the basic FormType at the very top of the Form Type hierarchy, so about any FormView instance should have these keys in their vars.

mpdude avatar Aug 06 '22 13:08 mpdude

@mpdude thank you for your contribution! I am not familiar with these new array attributes. In Symfony source code, I did not see anything related. Could you please share reference for it?

seferov avatar Aug 11 '22 09:08 seferov

Thats important information :+1: , so I've updated the initial comment on top ☝🏻

mpdude avatar Aug 11 '22 12:08 mpdude

@mpdude thanks for the explanation 👍 . I got it but I have some concerns:

  • adding 'valid' => false to $vars assumes it is not valid by default which by default should be true. Even if it is changed to true by default, it means psalm will always deduct as true thus will return in false positives
  • the other variables (for example, required, label_attr, etc.) should be also added to the stub as well
  • the most important point is normally psalm itself should evaluate this code block and add all these missing variables: $view->vars = array_replace($view->vars, [

seferov avatar Aug 17 '22 16:08 seferov

I think I don’t get the first and last item… could you please try to rephrase or show me with an example what needs to be done?

thanks!

mpdude avatar Aug 17 '22 17:08 mpdude

adding 'valid' => false to $vars assumes it is not valid by default which by default should be true. Even if it is changed to true by default, it means psalm will always deduct as true thus will return in false positives

I think that's fine, the inferred type should be overridden by the @psalm-var tag, which has valid: bool. It should maybe be changed to true in the array if that matches Symfony's declaration, but it doesn't really matter.

AndrolGenhald avatar Aug 17 '22 17:08 AndrolGenhald

I have added the remaining vars as well

mpdude avatar Sep 08 '22 13:09 mpdude

@mpdude thank you. I will try to fix test failures on master and after that a rebase might be needed

seferov avatar Sep 09 '22 09:09 seferov

@seferov ok, ping me when I shall rebase this PR

mpdude avatar Sep 09 '22 14:09 mpdude

@mpdude after merging master, the remaining failing tests are related to this changes. Could you please take a look at them?

seferov avatar Sep 11 '22 10:09 seferov