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] Add parameter type declarations #32237

Merged
merged 1 commit into from Aug 13, 2019

Conversation

@vudaltsov
Copy link
Contributor

commented Jun 27, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #32179
License MIT
Doc PR n/a

Some hints added, will finish within 2 days.

@derrabus derrabus referenced this pull request Jun 27, 2019
57 of 57 tasks complete
src/Symfony/Component/Form/Button.php Outdated Show resolved Hide resolved
src/Symfony/Component/Form/ButtonBuilder.php Outdated Show resolved Hide resolved
src/Symfony/Component/Form/Button.php Outdated Show resolved Hide resolved

@nicolas-grekas nicolas-grekas added this to the 5.0 milestone Jun 28, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2019

should we typehint \Symfony\Component\Form\FormTypeGuesserInterface also?
(asking as i looked in typehinting doctrine bridge)

@vudaltsov

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

I will continue to work on the issue soon. Symfony\Component\Form\FormTypeGuesserInterface should be typehinted in this PR, I guess.

@derrabus

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

I will continue to work on the issue soon. Symfony\Component\Form\FormTypeGuesserInterface should be typehinted in this PR, I guess.

Yes. If you also update the corresponding implementation in the doctrine bridge, remember to update the composer.json file of the bridge accordingly.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

Good for rebase.

@vudaltsov vudaltsov force-pushed the vudaltsov:32179-form-type-hints branch from 5cbb4a6 to 9d4eb15 Aug 4, 2019

@vudaltsov vudaltsov marked this pull request as ready for review Aug 4, 2019

@vudaltsov vudaltsov changed the title [Form] Add parameter type-hints [Form] Add parameter type declarations Aug 4, 2019

fabpot added a commit that referenced this pull request Aug 5, 2019

minor #32934 [Form] type cannot be a FormTypeInterface anymore (Tobion)
This PR was merged into the 4.4 branch.

Discussion
----------

[Form] type cannot be a FormTypeInterface anymore

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets |
| License       | MIT
| Doc PR        |

Found in #32237. This style of adding FormTypes has been removed in sf 3. Could also be merged in 3.4 if preferred. But there was an outdated, broken test. So 4.4 seems enough as well.

Commits
-------

6ee0d53 [Form] type cannot be a FormTypeInterface anymore
@xabbuh

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

@vudaltsov Can you please rebase here? :)

@vudaltsov vudaltsov force-pushed the vudaltsov:32179-form-type-hints branch from 16d3969 to 4ce52e4 Aug 5, 2019

@Tobion
Copy link
Member

left a comment

@Tobion

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

Tests are failing. The $label of createView can also be false, see \Symfony\Component\Form\ChoiceList\Factory\DefaultChoiceListFactory::addChoiceView

@vudaltsov

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

@Tobion , I know that, that's exactly why I fixed it in #32935, see #32237 (comment). I propose waiting for the merge and then I will rebase and fix and squash everything, ok?

nicolas-grekas added a commit that referenced this pull request Aug 5, 2019

minor #32935 [Form] Fix inconsistencies (vudaltsov)
This PR was squashed before being merged into the 3.4 branch (closes #32935).

Discussion
----------

[Form] Fix inconsistencies

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

- ~~Use `@inheritdoc` in `Button` and `ButtonBuilder` where the method does satisfy the contract.~~
- ~~Add `This method should not be invoked` in all unsupported methods in `Button` and `ButtonBuilder` for consistency.~~
- ~~Fix the misused `idempotent` term in implementations of the `getFormConfig` method. It is wrong in the sense that the method does not always return the same result. You can `setAttribute` for instance and `getFormConfig` will return a different config object.~~
- ~~Add `if ($this->locked)` checks in the supported mutators.~~
- ~~Fix the arguments contract in the `ChoiceListFactoryInterface` — now it supports `PropertyPathInterface` explicitly. The downside of it — breaking LSP in the `DefaultChoiceListFactory`.~~
- Fix the `$label` phpdoc of the `ChoiceView` (arised in #32237).
- Use `PropertyPathInterface` instead of `PropertyPath` in `PropertyAccessDecorator` of the choice factory.
- Fix `ArrayChoiceList::flatten` type hints.

These changes are debatable, so feel free to correct me if I am wrong at some point.

Ping @xabbuh , @HeahDude , @yceruto , @nicolas-grekas

Commits
-------

360711c [Form] Fix inconsistencies

symfony-splitter pushed a commit to symfony/form that referenced this pull request Aug 5, 2019

minor #32935 [Form] Fix inconsistencies (vudaltsov)
This PR was squashed before being merged into the 3.4 branch (closes #32935).

Discussion
----------

[Form] Fix inconsistencies

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

- ~~Use `@inheritdoc` in `Button` and `ButtonBuilder` where the method does satisfy the contract.~~
- ~~Add `This method should not be invoked` in all unsupported methods in `Button` and `ButtonBuilder` for consistency.~~
- ~~Fix the misused `idempotent` term in implementations of the `getFormConfig` method. It is wrong in the sense that the method does not always return the same result. You can `setAttribute` for instance and `getFormConfig` will return a different config object.~~
- ~~Add `if ($this->locked)` checks in the supported mutators.~~
- ~~Fix the arguments contract in the `ChoiceListFactoryInterface` — now it supports `PropertyPathInterface` explicitly. The downside of it — breaking LSP in the `DefaultChoiceListFactory`.~~
- Fix the `$label` phpdoc of the `ChoiceView` (arised in symfony/symfony#32237).
- Use `PropertyPathInterface` instead of `PropertyPath` in `PropertyAccessDecorator` of the choice factory.
- Fix `ArrayChoiceList::flatten` type hints.

These changes are debatable, so feel free to correct me if I am wrong at some point.

Ping @xabbuh , @HeahDude , @yceruto , @nicolas-grekas

Commits
-------

360711ce4e [Form] Fix inconsistencies
@Tobion

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

Even if you know that, #32935 does not fix this completely. That is what I'm saying. https://github.com/vudaltsov/symfony/blob/360711ce4e3b83251e7fe9dac958dc4f4246c0b6/src/Symfony/Component/Form/ChoiceList/Factory/ChoiceListFactoryInterface.php#L81 can also be false. So the phpdocs are still wrong. Or you need to overwrite phpdocs of \Symfony\Component\Form\ChoiceList\Factory\DefaultChoiceListFactory::createView and all the other implementations which should also be done in lower branches.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

(merged up)

@vudaltsov

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

@Tobion , got it, thank you!

I now need some time to distract from Forms before the final fight in this PR and relevant changes in other branches :)
I hope to finish tonight or tomorrow.

@Tobion

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

#32955 should solve this problem.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

up for a rebase?

@vudaltsov

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

@nicolas-grekas , sure, tonight I will take over #32955 and then we can safely add ?callable to $label. But I will rebase this branch today as well.

@vudaltsov vudaltsov force-pushed the vudaltsov:32179-form-type-hints branch from c8716fe to e6f1868 Aug 9, 2019

@vudaltsov

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

@derrabus , should I revert changes to OptionsResolverWrapper extends OptionsResolver and ViolationPath implements PropertyPathInterface to avoid bumping up the versions of the required packages?

@@ -128,7 +128,6 @@ public function getResourceHierarchyLevel(FormView $view, array $blockNameHierar
*
* @param FormView $view The view to render
* @param mixed $resource The renderer resource
* @param string $blockName The name of the block to render

This comment has been minimized.

Copy link
@xabbuh

xabbuh Aug 9, 2019

Member

Why did you remove this one but not the others?

This comment has been minimized.

Copy link
@vudaltsov

vudaltsov Aug 9, 2019

Author Contributor

Think it happened when rebasing. I added string declaration myself and then someone added it too without removing the phpdoc, so only phpdoc removal left here.
Should I remove $view in another PR targeting 3.4? Or just revert my change in this class?

* @param mixed $viewData View data of the compound form being initialized
* @param FormInterface[]|\Traversable $forms A list of {@link FormInterface} instances
* @param mixed $viewData View data of the compound form being initialized
* @param FormInterface[]|iterable $forms A list of {@link FormInterface} instances

This comment has been minimized.

Copy link
@xabbuh

xabbuh Aug 9, 2019

Member

Would iterable<FormInterface> be a correct notation here?

This comment has been minimized.

Copy link
@vudaltsov

vudaltsov Aug 9, 2019

Author Contributor

it's a correct notation, but do we use it in Symfony?

* @param FormInterface[]|\Traversable $forms A list of {@link FormInterface} instances
* @param mixed $viewData The compound form's view data that get mapped
* its children model data
* @param FormInterface[]|iterable $forms A list of {@link FormInterface} instances

This comment has been minimized.

Copy link
@xabbuh

xabbuh Aug 9, 2019

Member

Would iterable<FormInterface> be a correct notation here?

@xabbuh

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

We need to update the minimum required version for the symfony/property-access package to 5.0.

@vudaltsov

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

@xabbuh , for options-resolver as well, right? Since we have OptionsResolverWrapper. Done

@nicolas-grekas nicolas-grekas force-pushed the vudaltsov:32179-form-type-hints branch from 1fb8555 to 99a1d4c Aug 13, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

Thank you @vudaltsov.

@nicolas-grekas nicolas-grekas merged commit 99a1d4c into symfony:master Aug 13, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Aug 13, 2019

minor #32237 [Form] Add parameter type declarations (vudaltsov)
This PR was squashed before being merged into the 5.0-dev branch (closes #32237).

Discussion
----------

[Form] Add parameter type declarations

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #32179
| License       | MIT
| Doc PR        | n/a

Some hints added, will finish within 2 days.

Commits
-------

99a1d4c [Form] Add parameter type declarations
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.