-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Form] Added an alternative signature Form::add($name, $type, $options) #6355
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
Conversation
@@ -122,9 +115,9 @@ public function preBind(FormEvent $event) | ||
if ($this->allowAdd) { | ||
foreach ($data as $name => $value) { | ||
if (!$form->has($name)) { | ||
$form->add($this->factory->createNamed($name, $this->type, null, array_replace(array( | ||
$form->add((string) $name, $this->type, array_replace(array( |
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.
The form should probably also accept an integer as name, which makes it more fault-tolerant (instead of throwing an exception).
The Form::has does also accept an integer as there is no difference between $array[1]
or $array['1']
. Furthermore you would not need to cast to string 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.
casting is necessary because of the check in Form::add
: an integer would trigger the exception line 862
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.
Yes thats what I'd like to see improved...
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.
Well, this condition is the same than when adding a child through the builder: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/FormBuilder.php#L96
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.
I changed Form::add()
and FormBuilder::add()
to accept numerics.
ping @fabpot |
self::validateName($name); | ||
|
||
$name = (string) $name; |
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.
You can simply do $this->name = (string) $name;
below instead.
@@ -5,6 +5,8 @@ CHANGELOG | ||
----- | ||
|
||
* TrimListener now removes unicode whitespaces | ||
* deprecated getParent(), setParent() and hasParent() in FormBuilderInterface |
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 this change. We need this feature in the form type extensions.
This PR was merged into the master branch. Commits ------- 19d8510 [Form] Improved Form::add() and FormBuilder::add() to accept integers as field names fb71964 [Form] Added an alternative signature Form::add($name, $type, $options) Discussion ---------- [Form] Added an alternative signature Form::add($name, $type, $options) Bug fix: no Feature addition: yes Backwards compatibility break: no Symfony2 tests pass: yes Fixes the following tickets: #5806 Todo: - License of the code: MIT Documentation PR: symfony/symfony-docs#2024 --------------------------------------------------------------------------- by bschussek at 2012-12-18T10:42:55Z ping @fabpot
This PR was merged into the 4.4 branch. Discussion ---------- Fine tune constructor types | 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 | symfony/symfony-docs#... <!-- required for new features --> Fine tunes some constructor types that have been added in #24722 Form names as integer was only a workaround as forms names are used as array keys which get transformed to int. So it was added as a workaround in #6355 (comment) With typehints added in #24722 those were mostly auto-cast anyway, e.g. in FormBuilder. There are only a few integer form names remaining documented, in the main entry points of the Form component (`\Symfony\Component\Form\FormInterface::add`). Internally it's always a string now. So I could remove some int docs which also fixes #30032 what @xabbuh tried to do. Some of these changes we're just not done before because of broken tests. It's mainly a missing explicit mock for `TranslationInterface::trans` which returned null. If we had return types hints in interfaces, this wouldn't happen. Commits ------- 507794a Fine tune constructor types
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #5806
Todo: -
License of the code: MIT
Documentation PR: symfony/symfony-docs#2024