[Form] deprecate integers as form names #30032
Conversation
xabbuh
commented
Jan 30, 2019
| Q | A |
|---|---|
| Branch? | master |
| Bug fix? | no |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | yes |
| Tests pass? | yes |
| Fixed tickets | |
| License | MIT |
| Doc PR |
|
The |
| @@ -179,6 +179,10 @@ public function __construct($name, ?string $dataClass, EventDispatcherInterface | |||
| { | |||
| self::validateName($name); | |||
|
|
|||
| if (\is_int($name)) { | |||
There was a problem hiding this comment.
Is there a specific reason why strings with numerical values are still allowed?
There was a problem hiding this comment.
The CollectionType for example handles forms that have numerical names. And from an HTML point of view it does not really matter that the name does only contain digits.
| @@ -23,6 +23,7 @@ Config | |||
| Form | |||
| ---- | |||
|
|
|||
| * Support for using integers as form names is deprecated and will lead to a type error in 5.0, use strings instead. | |||
There was a problem hiding this comment.
will it? automatic casting rules would turn the int to a string and all will be silent, no?
|
What's the technical benefit of this change? |
|
From my perspective it makes Code more clear and a variable called $name should be expected as strong in most situations. @nicolas-grekas what do you mean by automatic casting rules? I am 👍 for this change |
|
I mean that I don't understand what this will change allow from a technical pov: the name is explicitly cast to string a few lines below. |
|
There won't be any TypeError in Symfony 5. There will be a cast (as done today explicitly in the constructor) because its only caller is FormBuilder (for the parent call), which is not running using strict types (as Symfony does not have them). And even if we had typehints on that name everywhere in Symfony (where we already document |
|
Let me clarify my intention here: This change would not have any effect for our internal code as all the callers of this constructor inside the Form component already pass a string. This would only affect consumers of the component who build But reiterating over my reasoning I think it's probably not worth the effort here. Let's close it. |
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