Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Form] Name related PHPDoc fixes#32400
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
nicolas-grekas commentedJul 8, 2019
I'd prefer not listing int explicitly. Yes, they are accepted. But so would they if we add the |
vudaltsov commentedJul 8, 2019
@nicolas-grekas , are we going to have strict types in the future? |
xabbuh commentedJul 8, 2019
@vudaltsov Whether or not Symfony will use strict types internally is not really important as the initial call will be coming from userland where developers need to decide if they want to use strict types and thus pass real strings or if they want to rely on PHP's implicit type casting. |
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Please revert all docblock changes.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
vudaltsov commentedJul 8, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@nicolas-grekas , if we use |
vudaltsov commentedJul 12, 2019
After discussing the issue with@nicolas-grekas and@xabbuh , finally got convinced that we don't need ints here. Still not sure that we should not explicitly deprecate passing ints, but that's not the point of this PR anyway. |
vudaltsov commentedJul 12, 2019
status: needs review |
seriquynh left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Also int is available for $name but only using string is more suitable, I think.
The given "$form->add(1)" has no meaning with $name = 1
xabbuh commentedJul 13, 2019
Thank you@vudaltsov. |
This PR was merged into the 3.4 branch.Discussion----------[Form] Name related PHPDoc fixes| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | relates to#32179| License | MIT| Doc PR | n/aAs I started working on#32179 in#32237, I noticed some PHPDoc inconsistencies around the form's name. I have researched the issue thoroughly and here's my proposal:1. All "factory" methods (`FormFactory::create*`, `ResolvedFormType::createBuilder`, `FormBuilder::create|add`, `Form::add`) currently accept string and integer names. This should not be deprecated, because it's very convenient and natural to pass ints when adding children to types like `ChoiceType` or `CollectionType`.1. None of the "factory" methods explicitly accepts `null` as a name, although passing `null` works the same as passing `''`. I consider passing `null` in this case to be ugly, so I corrected the builder's constructors mentioning `null` as a possible argument. One should pass an empty string `''` when creating such a form. We could also deprecate passing `null` in a PR targeting 4.4.1. Currently the name becomes a string in the builder's (or config builder's) constructor. Which means that `FormConfigInterface::getName` always returns a string and thus the form's `$name` property is always a string. All related checks and PHPDocs should be corrected.1. The "children accessors" (`Form::has|get|remove`, `FormBuilder::has|get|remove`) should accept both strings and ints because they are array-accessible by design (so it does not really matter, if the key is a string or an int). If it's valid to have `$collectionForm->add(1, ItemType::class)`, then it should be valid to do `$collectionForm->get(1)`. And it works in code, but is not mentioned in the PHPDoc.ping@nicolas-grekas ,@xabbuh ,@HeahDudeCommits-------eae95c4 PHPDoc fixes
As I started working on#32179 in#32237, I noticed some PHPDoc inconsistencies around the form's name. I have researched the issue thoroughly and here's my proposal:
FormFactory::create*,ResolvedFormType::createBuilder,FormBuilder::create|add,Form::add) currently accept string and integer names. This should not be deprecated, because it's very convenient and natural to pass ints when adding children to types likeChoiceTypeorCollectionType.nullas a name, although passingnullworks the same as passing''. I consider passingnullin this case to be ugly, so I corrected the builder's constructors mentioningnullas a possible argument. One should pass an empty string''when creating such a form. We could also deprecate passingnullin a PR targeting 4.4.FormConfigInterface::getNamealways returns a string and thus the form's$nameproperty is always a string. All related checks and PHPDocs should be corrected.Form::has|get|remove,FormBuilder::has|get|remove) should accept both strings and ints because they are array-accessible by design (so it does not really matter, if the key is a string or an int). If it's valid to have$collectionForm->add(1, ItemType::class), then it should be valid to do$collectionForm->get(1). And it works in code, but is not mentioned in the PHPDoc.ping@nicolas-grekas ,@xabbuh ,@HeahDude