[Form] Name related PHPDoc fixes #32400
Conversation
|
I'd prefer not listing int explicitly. Yes, they are accepted. But so would they if we add the |
|
@nicolas-grekas , are we going to have strict types in the future? |
|
@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 , if we use |
7f0f679 to
eae95c4
Compare
|
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. |
|
status: needs review |
|
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/a 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: 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 , @HeahDude Commits ------- 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