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] Add parameter type declarations#32237
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
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Form/ChoiceList/Factory/ChoiceListFactoryInterface.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Form/ChoiceList/Loader/CallbackChoiceLoader.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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
kaznovac commentedJul 15, 2019
should we typehint |
Uh oh!
There was an error while loading.Please reload this page.
vudaltsov commentedJul 18, 2019
I will continue to work on the issue soon. |
derrabus commentedJul 18, 2019
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 commentedJul 31, 2019
Good for rebase. |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
5cbb4a6 to9d4eb15CompareThis 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 <!-- seehttps://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 commentedAug 5, 2019
@vudaltsov Can you please rebase here? :) |
16d3969 to4ce52e4Comparesrc/Symfony/Component/Form/ChoiceList/Factory/CachingFactoryDecorator.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Tobion commentedAug 5, 2019
Tests are failing. The $label of createView can also be |
vudaltsov commentedAug 5, 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.
@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? |
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-grekasCommits-------360711c [Form] Fix inconsistencies
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 insymfony/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-grekasCommits-------360711ce4e [Form] Fix inconsistencies
Tobion commentedAug 5, 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.
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 |
nicolas-grekas commentedAug 5, 2019
(merged up) |
vudaltsov commentedAug 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 :) |
Tobion commentedAug 5, 2019
#32955 should solve this problem. |
nicolas-grekas commentedAug 8, 2019
up for a rebase? |
vudaltsov commentedAug 8, 2019
@nicolas-grekas , sure, tonight I will take over#32955 and then we can safely add |
c8716fe toe6f1868Comparevudaltsov commentedAug 9, 2019
@derrabus , should I revert changes to |
Uh oh!
There was an error while loading.Please reload this page.
| * | ||
| * @param FormView $view The view to render | ||
| * @param mixed $resource The renderer resource | ||
| * @param string $blockName The name of the block to render |
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.
Why did you remove this one but not the others?
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.
Think it happened when rebasing. I addedstring 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 targeting3.4? Or just revert my change in this class?
Uh oh!
There was an error while loading.Please reload this page.
| * @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 |
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.
Woulditerable<FormInterface> be a correct notation here?
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.
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 |
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.
Woulditerable<FormInterface> be a correct notation here?
xabbuh commentedAug 9, 2019
We need to update the minimum required version for the |
vudaltsov commentedAug 9, 2019
@xabbuh , for |
src/Symfony/Component/Form/ChoiceList/Factory/DefaultChoiceListFactory.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
1fb8555 to99a1d4cComparenicolas-grekas commentedAug 13, 2019
Thank you@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/aSome hints added, will finish within 2 days.Commits-------99a1d4c [Form] Add parameter type declarations
Some hints added, will finish within 2 days.