Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Form] Name related PHPDoc fixes #32400

Merged
merged 1 commit into from Jul 13, 2019

Conversation

Projects
None yet
5 participants
@vudaltsov
Copy link
Contributor

commented Jul 6, 2019

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.
  2. 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.
  3. 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.
  4. 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

@vudaltsov vudaltsov requested a review from xabbuh as a code owner Jul 6, 2019

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Jul 8, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

I'd prefer not listing int explicitly. Yes, they are accepted. But so would they if we add the string type, because of automatic PHP casts.

@vudaltsov

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

@nicolas-grekas , are we going to have strict types in the future?

@xabbuh

This comment has been minimized.

Copy link
Member

commented Jul 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
Copy link
Member

left a comment

Please revert all docblock changes.

Show resolved Hide resolved src/Symfony/Component/Form/Form.php Outdated
@vudaltsov

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

@nicolas-grekas , if we use string only, then it means that the Symfony code passing int (like in CollectionType) is working against the docblock as well as all the tests checking that passing int is ok. And if we set string type explicitly in 5.0 then user will be unable to do $form->add(int $name) in a class with strict types enabled which feels kind of wrong.

@vudaltsov vudaltsov force-pushed the vudaltsov:form-phpdoc-fixes branch from 7f0f679 to eae95c4 Jul 12, 2019

@vudaltsov

This comment has been minimized.

Copy link
Contributor Author

commented Jul 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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2019

status: needs review

@xuanquynh
Copy link
Contributor

left a comment

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

xabbuh approved these changes Jul 13, 2019

@xabbuh

This comment has been minimized.

Copy link
Member

commented Jul 13, 2019

Thank you @vudaltsov.

@xabbuh xabbuh merged commit eae95c4 into symfony:3.4 Jul 13, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

xabbuh added a commit that referenced this pull request Jul 13, 2019

minor #32400 [Form] Name related PHPDoc fixes (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

@vudaltsov vudaltsov deleted the vudaltsov:form-phpdoc-fixes branch Jul 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.