Skip to content

Conversation

@mpdude
Copy link

@mpdude mpdude commented Aug 6, 2022

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.

@seferov
Copy link
Member

seferov commented Aug 11, 2022

@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?

@mpdude
Copy link
Author

mpdude commented Aug 11, 2022

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

@seferov
Copy link
Member

seferov commented Aug 17, 2022

@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, [

@mpdude
Copy link
Author

mpdude commented Aug 17, 2022

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!

@AndrolGenhald
Copy link

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.

@mpdude mpdude changed the title Add errors and valid to FormView stub Add default FormView vars in the stub class Sep 8, 2022
@mpdude
Copy link
Author

mpdude commented Sep 8, 2022

I have added the remaining vars as well

@seferov
Copy link
Member

seferov commented Sep 9, 2022

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

fabpot added a commit to symfony/symfony that referenced this pull request Sep 9, 2022
This PR was merged into the 6.2 branch.

Discussion
----------

Remove `size` FormView variable

| Q             | A
| ------------- | ---
| Branch?       | 6.2
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       |
| License       | MIT
| Doc PR        |

While [adding `FormView::$vars` support in the Psalm Plugin for Symfony](psalm/psalm-plugin-symfony#271), I came across the `size` key.

I tried to figure out the meaning of the setting, but it is not documented, seems to be used nowhere, and has been around since the early days of the Form Component a decade ago.

So, my assumption is that is has no meaning/relevance and thus my suggestion is to remove it.

Commits
-------

c6d455a Remove `size` FormView variable
symfony-splitter pushed a commit to symfony/form that referenced this pull request Sep 9, 2022
This PR was merged into the 6.2 branch.

Discussion
----------

Remove `size` FormView variable

| Q             | A
| ------------- | ---
| Branch?       | 6.2
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       |
| License       | MIT
| Doc PR        |

While [adding `FormView::$vars` support in the Psalm Plugin for Symfony](psalm/psalm-plugin-symfony#271), I came across the `size` key.

I tried to figure out the meaning of the setting, but it is not documented, seems to be used nowhere, and has been around since the early days of the Form Component a decade ago.

So, my assumption is that is has no meaning/relevance and thus my suggestion is to remove it.

Commits
-------

c6d455a610 Remove `size` FormView variable
@mpdude
Copy link
Author

mpdude commented Sep 9, 2022

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

@seferov
Copy link
Member

seferov commented Sep 11, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants