[Form] RFC: Adapt Form API to avoid exponential growth of the violation mapping #3903

webmozart · 2012-04-12T12:36:24Z

NOTE: I'm aware that 2.1 introduces quite a few BC breaks in the Form component. Unfortunately these are necessary in order to fix a number of bugs present in 2.0. Doing these changes now will hopefully help to stabilize the API after 2.1.

I suggest to make property paths first class citizens of Forms. I will start with explaining the API change and continue with the reasoning why I think this should be done.

Old API:

<?php
$builder
    // mapped to $data->getCity() or $data['city']
    ->add('city')
    // mapped to $data->getAddress()->getStreet()
    // or $data->getAddress()['street']
    // or $data['address']->getStreet()
    // or $data['address']['street']
    ->add('street', array(
        'property_path' => 'address.street'
    ))
    // not mapped at all
    ->add('terms', array(
        'property_path' => false,
    ));

// Form backed by an array
$builder
    ->add('city')
    ->add('street', array(
        // can be used, but makes no difference
        'property_path' => '[street]',
    ));

New API:

<?php
$builder
    // mapped to $data->getCity()
    ->add('city')
    // mapped to $data['city']
    ->add('[city]')
    // mapped to $data->getAddress()->getStreet()
    ->add('address.street')
    // mapped to $data['address']->getStreet()
    ->add('[address].street')
    // mapped to $data->getAddress()['street']
    ->add('address[street]')
    // mapped to $data['address']['street']
    ->add('[address][street]')
    // not mapped at all
    ->add('terms', array(
        'mapped' => false,
    ))
    // custom, beautified HTML/POST parameter name (important for REST APIs)
    ->add('address.street', array(
        'name' => 'street',
    ));

// Form backed by an array
$builder
    ->add('[city]')
    ->add('[street]');

Negative consequences:

people who used the "property_path" option must adapt their code
forms backed by arrays must be adapted: field names need to be changed from "fieldname" to "[fieldname]"

Positive consequences:

a clearer API for newcomers, no hidden "property_path" option anymore
a deterministic mechanism for violation mapping, allowing more sophisticated techniques to assign errors to specific fields (to fix errors like [Form][DelegatingValidator] RepeatedType error keeps on bubbling to root form #1971)

Reasoning:

The validator generates violation errors with property paths that must be mapped to the corresponding form fields. Right now, this mapping grows exponentially in the length of the property path:

PP of constraint violation	Guessed PP of mapped form field
city	city [city]
[city]	[city]
address.city	address.city address[city] [address].city address[city]
address[city]	address[city] [address][city]
[address].city	[address].city [address][city]
[address][city]	[address][city]
person.address.city	person.address.city person.address[city] person[address].city person[address][city] [person].address.city [person].address[city] [person][address].city [person][address][city]

Changing the API would make property paths deterministic and greatly simplify the mapping:

PP of constraint violation	Guessed PP of mapped form field
city	city
[city]	[city]
address.city	address.city
address[city]	address[city]
[address].city	[address].city
[address][city]	[address][city]

A simplified mapping mechanism allows more elaborate mapping options, like specifying that

errors mapped to a form should be mapped to a child "foo" instead
errors mapped to a child "foo" should be mapped to a child "bar" instead

Feedback is appreciated.

stof · 2012-04-12T12:45:22Z

@bschussek During the sfPot 2 days ago, @rande told me that the PropertyPath::readProperty method and the corresponding writeProperty were the most expensive places of the Form component regarding perf. Would this change regarding property paths allow to simplify them or would it need to be a separate refactoring ?

webmozart · 2012-04-12T12:48:10Z

@stof The performance of PropertyPath would not change.

attilabukor · 2012-04-12T13:00:47Z

@bschussek this looks fine, but it's a pretty big BC break (other BC breaks are usually only configuration stuff and all)

vicb · 2012-04-12T13:03:03Z

@stof it might be possible to implement some kind of cache there ?

@bschussek could the name option takes an arbitrary name (thinking of including '.'s) - this could then be a great addition.

stof · 2012-04-12T13:06:51Z

@vicb yeah, this is what we talked about. These method could probably delegate the work to another object, which could be replaced by a cached implementation (or eventually an implementation used by generating some code during the cache-warming)

rande · 2012-04-12T13:07:29Z

@vicb the cache solution can be a generated set of classes which "know" how to set or get values. This will avoid the ReflectionClass initialization on each read/write and also checks done in the PropertyPath class. This can be also backed by the ORM metadata layer

vicb · 2012-04-12T13:12:35Z

@stof / @rande could you open an issue for the cache topic (to avoid polluting this RFC more and to keep track). Thanks.

webmozart · 2012-04-12T13:17:40Z

@r1pp3rj4ck Yes, this is unfortunate.

@vicb Periods would then be allowed in names, the other restrictions would remain (in order to generate valid IDs). Also be aware that the name would still be wrapped into the parent's name, i.e. the real HTML value is "foo[ba.ar]" if you name your field "ba.ar" and its parent is called "foo".

@stof Sounds good. I guess it's time to finally pull PropertyPath into a separate component.

marijn · 2012-04-12T13:40:34Z

Although it's quite a BC break, I think it's all quite reasonable. 👍

datiecher · 2012-04-12T14:27:04Z

I was never found on property_path to begin with and #1971 is an issue that really pisses one of once you hit it (and you will). So, thumbs up for me!

We will definitively need to have a big warning for this BC, though.

webmozart · 2012-04-13T08:01:11Z

See also #3272

fabpot · 2012-04-13T14:47:22Z

This BC break is unfortunately definitely needed (and anyone who has tried to debug property paths in DelegatingValidator knows what I'm talking about).

I'm not a big fan of the [foo] notation for forms backed by an array as it looks quite arbitrary. I have not any better idea except perhaps being able to switch from the array notation to the method notation (and the other way around) with some method call?

webmozart · 2012-04-14T07:53:12Z

@fabpot I agree that [foo] is not a beautiful notation, but it makes sense if you look at cases like #3924.

vicb · 2012-04-14T08:04:08Z

could foo[] be an option, it could make nesting arrays looking better foo[bar[]] vs [foo[bar]]?

webmozart · 2012-04-14T08:54:58Z

@vicb For me this intuitively has a different meaning. foo[] for me refers to an arbitrary index of the array named "foo", while [foo] refers to the array key "foo" of the "current" array (like $data['foo']). Nested array keys are expressed this way: [foo][bar] (like $data['foo']['bar']).

vicb · 2012-04-14T09:10:01Z

right, your notation makes more sense.

@juliendidier

Commits ------- 6e4ed9e [Form] Fixed regression: bind(null) was not converted to an empty string anymore fcb2227 [Form] Deprecated FieldType, which has been merged into FormType bfa7ef2 [Form] Removed obsolete exceptions 2a49449 [Form] Simplified CSRF mechanism and removed "csrf" type Discussion ---------- [Form] Merged FieldType into FormType Bug fix: no Feature addition: no Backwards compatibility break: yes Symfony2 tests pass: yes Fixes the following tickets: #3878 Todo: update the documentation on theming ![Travis Build Status](https://secure.travis-ci.org/bschussek/symfony.png?branch=issue3878) This PR is a preparatory PR for #3879. See also #3878. --------------------------------------------------------------------------- by juliendidier at 2012-04-13T14:25:19Z What's the benefit ? --------------------------------------------------------------------------- by henrikbjorn at 2012-04-13T14:26:40Z why `input_widget` ? and not just `widget` --------------------------------------------------------------------------- by Burgov at 2012-04-13T14:27:49Z @juliendidier dynamic inheritance is now obsolete which fixes some other issues --------------------------------------------------------------------------- by stloyd at 2012-04-13T14:37:26Z What about __not__ breaking API so *badly* and leaving `FieldType` which will be simple like (with marking as deprecated): ```php <?php class FieldType extends AbstractType { public function getParent(array $options) { return 'form'; } public function getName() { return 'field'; } } --------------------------------------------------------------------------- by bschussek at 2012-04-13T14:43:41Z @stloyd That's a very good idea. --------------------------------------------------------------------------- by mvrhov at 2012-04-13T17:41:21Z IMHO what @stloyd proposed sounds like a good idea, but removing FieldType class, if #3903 will come into life might ensure that more forms will broke and people will check them thoroughly. --------------------------------------------------------------------------- by r1pp3rj4ck at 2012-04-13T18:46:08Z @bschussek looks great, but I'm concerned about how quickly will the third-party bundles adapt to this BC break. I hope really quick, because if they don't the whole stuff will be useless :S of course it's not your problem to solve. --------------------------------------------------------------------------- by stof at 2012-04-13T18:50:32Z @r1pp3rj4ck there is already another BC break requiring to update custom types for Symfony master. So third party bundles already have to do some work. --------------------------------------------------------------------------- by r1pp3rj4ck at 2012-04-13T18:59:37Z @stof which one? I've looked into @bschussek 's RFC about these [foo].bar stuff, but it's not yet implemented. Are you refering to this or another one I've missed? --------------------------------------------------------------------------- by stof at 2012-04-13T19:04:06Z @r1pp3rj4ck the change regarding default options --------------------------------------------------------------------------- by r1pp3rj4ck at 2012-04-13T19:06:10Z @stof oh, I forgot that one. Weird thing is that I've already changed my default options today and still forgetting these stuff :D --------------------------------------------------------------------------- by bschussek at 2012-04-14T08:58:29Z I restored and deprecated FieldType now. I'd appreciate further reviews. --------------------------------------------------------------------------- by stloyd at 2012-04-14T09:02:32Z Maybe we should try to avoid this BC in templates ? What do you think about similar move like with `FieldType` ? (hold old, but inside just render new) --------------------------------------------------------------------------- by bschussek at 2012-04-14T09:07:22Z @stloyd You mean for those cases where people explicitely render the block "field_*"? We can do that. --------------------------------------------------------------------------- by stloyd at 2012-04-14T09:09:45Z @bschussek Yes I mean this case =) Sorry for not being explicit, I need some coffee I think =) --------------------------------------------------------------------------- by bschussek at 2012-04-17T14:45:35Z I added the field_* block 8000 s again for BC. Could someone please review again? Otherwise this can be merged. --------------------------------------------------------------------------- by Burgov at 2012-04-17T15:11:16Z @bschussek I'm not sure what has changed to cause this, but if I try out your branch on our forms, if I leave the value of an input empty, eventually the reverseTransform method receives a null value, rather than a '' (empty string) value, as on the current symfony master. DateTimeToLocalizedStringTransformer, for example, will throw an Exception if the value is not a string ```php if (!is_string($value)) { throw new UnexpectedTypeException($value, 'string'); } ``` Other than that, all forms render just the same as they do on symfony master --------------------------------------------------------------------------- by bschussek at 2012-04-17T15:30:29Z @Burgov Fixed.

mvrhov · 2012-04-19T10:29:43Z

Would probably also fix #3995

webmozart · 2012-05-16T19:33:10Z

As a side note: This change will move the recommended Twig syntax from

{{ form_widget(form.child) }}

to

{{ form_widget(form['child']) }}
{{ form_widget(form['[child]']) }}
{{ form_widget(form['[address][street]']) }}

Alternatively (if that's too many a bracket for you), this could be:

{{ form_widget(form.getChild('child')) }}
{{ form_widget(form.getChild('[child]')) }}
{{ form_widget(form.getChild('[address][street]')) }}

What do you think?

fabpot · 2012-05-16T19:41:10Z

That's much less intuitive than the current version, and this is indeed a very big BC break.

webmozart · 2012-05-16T19:43:20Z

Yes. Trying to find something better is difficult though.

craue · 2012-05-16T19:44:22Z

... and it's absolutely ugly.

marijn · 2012-05-16T19:50:36Z

I'm not sure if it's possible -- but what about mimicking the PHP api of the form objects?

{{ form_widget(form.child) }}
{{ form_widget(form['child']) }}
{{ form_widget(form['address']['street']) }}

webmozart · 2012-05-16T19:54:29Z

A completely alternative approach to this problem could be to let a form whose "data_class" is set to null only be backed by an array. I.e., if the form should be backed by an object, I always need to specify this class in "data_class".

Then we could keep the current approach and do the following:

<?php
// Form backed by a My\Class object
// The construction is done by the framework
$builder = new FormBuilder($factory, $dispatcher, 'My\Class');
// Done by the user:
$builder
    // mapped to $data->getCity()
    ->add('city')
    // mapped to $data->getAddress()->getStreet()
    ->add('street', null, array(
        'property_path' => 'address.street',
    ))
    // not mapped at all
    ->add('terms', array(
        'property_path' => false,
    ));

// Form backed by an array
$builder = new FormBuilder($factory, $dispatcher, null);
// Done by the user
$builder
    // mapped to $data['city']
    ->add('city')
    // mapped to $data->getAddress()->getStreet()
    ->add('street', null, array(
        'property_path' => 'address.street',
    ))
    // not mapped at all
    ->add('terms', array(
        'property_path' => false,
    ));

As you can see, the only difference is when the "property_path" option is not explicitely set. Nevertheless, we can deterministically guess the default property path from the field name ("city" vs. "[city]").

Thoughts?

stof · 2012-05-16T19:54:50Z

@marijn when adding a field with ->add('[address][street]'), it would not created something accessible as $form['address']['street'] in PHP but as $form['[address][street]']

stof · 2012-05-16T19:56:34Z

@bschussek what if I need to map something to $data->getAddress()['street'] ?

webmozart · 2012-05-16T19:57:03Z

@stof address[street]

stof · 2012-05-16T20:04:37Z

oh, so a form backed by an object would still be able to use array access by forcing it with your previous suggestion ? Seems good

marijn · 2012-05-16T20:04:39Z

So it's not only ugly in twig but also in PHP? (Excuse me if that sounds harsh, it just seems unintuitive and... well, ugly)

stof · 2012-05-16T20:06:54Z

btw, are you also dropping the suggestion about replacing the property_path option by a name option, thus allowing dots in HTML names ?

Tobion · 2012-05-16T20:14:13Z

@bschussek Is this new syntax also necessarily needed for forms based on object data?

webmozart · 2012-05-16T20:14:28Z

My last suggestion was about leaving everything exactly as is, except for interpreting the case with "data_class"==null differently. About dots in HTML names, they are probably possible then, but I need to check.

rande · 2012-05-16T20:35:13Z

Why not using something different, I think twig can help here : {% widget form 'address.street' %}

stof · 2012-05-16T20:51:04Z

@rande I don't understand you proposal. There is no issue about the form_widget function at all (replacing it with by a tag would be a BC break and would be semantically wrong). the issue is about the way to access a element in the FormView tree.

rande · 2012-05-16T20:54:20Z

@stof the proposed syntax is ugly, I just try to find another syntax ...

stof · 2012-05-16T20:56:20Z

@rande But you are not considering the appropriate stuff. The syntax to navigate in the tree comes from the way the object works (and would be as ugly in PHP with the proposal).
And btw, the latest proposal done by @bschussek does not look like this anymore.

webmozart · 2012-05-19T21:36:13Z

Solution with minimal BC breaks available now at the linked PR.

@Valid

Commits ------- 1422506 [Form] Clarified the usage of "constraints" in the UPGRADE file af41a1a [Form] Fixed typos ac69394 [Form] Allowed native framework errors to be mapped as well 59d6b55 [Form] Fixed: error mapping aborts if reaching an unsynchronized form 9eda5f5 [Form] Fixed: RepeatedType now maps all errors to the first field 215b687 [Form] Added capability to process "." rules in "error_mapping" c9c4900 [Form] Fixed: errors are not mapped to unsynchronized forms anymore c8b61d5 [Form] Renamed FormMapping to MappingRule and moved some logic there to make rules more extendable d0d1fe6 [Form] Added more information to UPGRADE and CHANGELOG 0c09a0e [Form] Made $name parameters optional in PropertyPathBuilder:replaceBy(Index|Property) 081c643 [Form] Updated UPGRADE and CHANGELOG bbffd1b [Form] Fixed index checks in PropertyPath classes ea5ff77 [Form] Fixed issues mentioned in the PR comments 7a4ba52 [EventDispatcher] Added class UnmodifiableEventDispatcher 306324e [Form] Greatly improved the error mapping done in DelegatingValidationListener 8f7e2f6 [Validator] Fixed: @Valid does not recurse the traversal of collections anymore by default 5e87dd8 [Form] Added tests for the case when "property_path" is null or false. Instead of setting "property_path" to false, you should set "mapped" to false instead. 2301b15 [Form] Tightened PropertyPath validation to reject any empty value (such as false) 7ff2a9b Revert "[Form] removed a constraint in PropertyPath as the path can definitely be an empty string for errors attached on the main form (when using a constraint defined with the 'validation_constraint' option)" 860dd1f [Form] Adapted Form to create a deterministic property path by default 03f5058 [Form] Fixed property name in PropertyPathMapperTest c2a243f [Form] Made PropertyPath deterministic: "[prop]" always refers to indices (array or ArrayAccess), "prop" always refers to properties 2996340 [Form] Extracted FormConfig class to simplify the Form's constructor Discussion ---------- [Form] Improved the error mapping and made property paths deterministic Bug fix: yes Feature addition: no Backwards compatibility break: **yes** Symfony2 tests pass: yes Fixes the following tickets: #1971, #2945, #3272, #3308, #3903, #4329 Probably fixes: #2729 Todo: - This PR is ready for review. The algorithm for assigning errors to forms in the form tree was improved a lot. Also the error mapping works better now. There are still a few features to be added (e.g. wildcards "*"), but these can be implemented now pretty easily. This PR breaks PR in that a form explicitely needs to set the "data_class" option if it wants to map to an object and needs to leave that option empty if it wants to map to an array. Furthermore, property paths must be deterministic now: `foo` now only maps to `(g|s)etFoo()`, but not the index `["foo"]` (array or ArrayAccess), while `[foo]` only maps to the latter but not the former. See #3903 for more information. --------------------------------------------------------------------------- by travisbot at 2012-05-19T21:35:24Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1377086) (merged 9e346990 into 2229461). --------------------------------------------------------------------------- by Tobion at 2012-05-20T01:47:48Z Good stuff in general :) --------------------------------------------------------------------------- by bschussek at 2012-05-20T09:19:18Z Fixed everything mentioned here so far. --------------------------------------------------------------------------- by travisbot at 2012-05-20T09:22:22Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1379548) (merged 49918bef into 2229461). --------------------------------------------------------------------------- by Tobion at 2012-05-20T14:29:14Z many occurences of two spaces after `@param` (should be only one space). --------------------------------------------------------------------------- by Koc at 2012-05-20T14:40:18Z Sorry, I'm cannot observe all changes for form component in 2.1, so I have a question: ```php <?php protected $isPrivate; public function isPrivate() {} public function setPrivate() {} ``` Is it possible validate this property with accessors/mutators from code above in 2.1 now? --------------------------------------------------------------------------- by bschussek at 2012-05-20T14:41:09Z The type after `@param` used to be aligned with the type of the `@return` tag. Let's get the PHPDoc-guidelines straight before nitpicking more on these trivialities. --------------------------------------------------------------------------- by bschussek at 2012-05-20T14:42:34Z @Koc Please move your question to the user mailing list, let's keep this PR on topic. --------------------------------------------------------------------------- by bschussek at 2012-05-20T14:45:42Z Fixed everything mentioned until now. --------------------------------------------------------------------------- by travisbot at 2012-05-20T14:47:48Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1380903) (merged 03d60a03 into f433f6b). --------------------------------------------------------------------------- by bschussek at 2012-05-20T15:18:12Z CHANGELOG/UPGRADE is now updated. --------------------------------------------------------------------------- by travisbot at 2012-05-20T15:19:39Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1381047) (merged 48cc3eca into f433f6b). --------------------------------------------------------------------------- by Tobion at 2012-05-20T16:16:51Z All the deprecated methods and changed constructor arguments should probably be mentioned in the changelog/upgrade. --------------------------------------------------------------------------- by travisbot at 2012-05-21T07:31:47Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1386621) (merged c0ef69a1 into 1407f11). --------------------------------------------------------------------------- by travisbot at 2012-05-21T08:01:46Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1386826) (merged 4f3fc1fe into 1407f11). --------------------------------------------------------------------------- by travisbot at 2012-05-21T09:22:30Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1387263) (merged e3675050 into 1407f11). --------------------------------------------------------------------------- by bschussek at 2012-05-21T09:43:08Z This PR now fixes #1971. --------------------------------------------------------------------------- by travisbot at 2012-05-21T09:45:51Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1387370) (merged de33f9ef into 1407f11). --------------------------------------------------------------------------- by travisbot at 2012-05-21T11:06:53Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1387838) (merged da3b562e into 1407f11). --------------------------------------------------------------------------- by bschussek at 2012-05-21T11:07:45Z This PR now fixes #2945. --------------------------------------------------------------------------- by bschussek at 2012-05-21T15:33:33Z Native errors (such as "invalid", "extra_fields" etc.) are now respected by the "error_mapping" option as well. The option "validation_constraint" was deprecated, "constraints" is its replacement and a lot handier, because it allows you to work easily with arrays. ```php <?php $builder ->add('name', 'text', array( 'constraints' => new NotBlank(), )) ->add('phoneNumber', 'text', array( 'constraints' => array( new NotBlank(), new MinLength(7), new Type('numeric') ) )); ``` Ready for review again. --------------------------------------------------------------------------- by travisbot at 2012-05-21T15:33:45Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1390239) (merged e162f56d into ea33d4d). --------------------------------------------------------------------------- by travisbot at 2012-05-21T15:40:02Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1390367) (merged e8729a7f into ea33d4d). --------------------------------------------------------------------------- by travisbot at 2012-05-21T16:06:03Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1390663) (merged ef39aba4 into ea33d4d). --------------------------------------------------------------------------- by travisbot at 2012-05-22T08:54:36Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1398153) (merged af41a1a into e4e3ce6). --------------------------------------------------------------------------- by travisbot at 2012-05-22T09:26:12Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1398415) (merged 1422506 into e4e3ce6).

mvrhov mentioned this issue Apr 13, 2012

[Form] Merged FieldType into FormType #3923

Merged

webmozart mentioned this issue Apr 14, 2012

[Form] InvalidPropertyException using an ArrayObject as data #3924

Closed

webmozart mentioned this issue Apr 18, 2012

[Form] [Validator] validator messages assigned to the parent form if using repeated form element type #3969

Closed

This was referenced Apr 20, 2012

[Form] property path for violations is incorrect #3995

Closed

[Form] Add "choice_labels", "choice_values", "choice_attr" and "group_by" options to ChoiceType #4067

Closed

webmozart mentioned this issue May 19, 2012

[Form] Improved the error mapping and made property paths deterministic #4341

Merged

fabpot closed this as completed May 22, 2012

Uh oh!

[Form] RFC: Adapt Form API to avoid exponential growth of the violation mapping #3903

[Form] RFC: Adapt Form API to avoid exponential growth of the violation mapping #3903

Comments

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!