Commits
-------
24b764e [Form] Fixed issues mentioned in the PR
9216816 [Form] Turned Twig filters into tests
310f985 [Form] Added a layer of 2.0 BC methods to FormView and updated UPGRADE and CHANGELOG
5984b18 [Form] Precalculated the closure for deciding whether a choice is selected (PHP +30ms, Twig +30ms)
5dc3c39 [Form] Moved the access to templating helpers out of the choice loop for performance reasons (PHP +100ms)
0ef9acb [Form] Moved the method isChoiceSelected() to the ChoiceView class (PHP +150ms)
8b72766 [Form] Tweaked the generation of option tags for performance (PHP +200ms, Twig +50ms)
400c95b [Form] Replace methods in ChoiceView by public properties (PHP +100ms, Twig +400ms)
d072f35 [Form] The properties of FormView are now accessed directly in order to increase performance (PHP +200ms, Twig +150ms)
Discussion
----------
[Form] Made FormView and ChoiceView properties public for performance reasons
Bug fix: no
Feature addition: no
Backwards compatibility break: **yes**
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
This PR changes the access to properties of `FormView` and `ChoiceView` objects from getters to direct property accesses. On [my example form](http://advancedform.gpserver.dk/app_dev.php/taxclasses/1) this improves rendering performance for **300ms** with PHP templates and **550ms** with Twig on my local machine.
Unfortunately, this breaks BC both with 2.0 and with the current master in Form Types and PHP templates. Twig templates are not affected by this change.
2.0:
```
$formView->set('my_var', 'foobar');
$formView->get('my_var');
$formView->getChild('childName');
$formView['childName'];
```
master:
```
$formView->setVar('my_var', 'foobar');
$formView->getVar('my_var');
$formView->get('childName');
$formView['childName'];
```
this PR:
```
$formView->vars['my_var'] = 'foobar';
$formView->vars['my_var'];
$formView->children['childName'];
$formView['childName'];
```
Should we add methods to keep BC with 2.0?
The second part of this PR contains improvements to the rendering of choice fields. These gain another **~500ms** for PHP templates and **80ms** for Twig. These improvements are BC, unless you overwrote the block "choice_widget_options" in your form themes which then needs to be adapted.
**Update:**
The PR now includes a BC layer for 2.0.
---------------------------------------------------------------------------
by stof at 2012-07-21T11:37:41Z
@bschussek couldn't we keep the getters and setters for BC even if the rendering accesses the public properties directly ?
---------------------------------------------------------------------------
by bschussek at 2012-07-21T11:52:33Z
@stof A BC layer for 2.0 is now included. People who upgraded to master already unfortunately need to adapt their code.
---------------------------------------------------------------------------
by sstok at 2012-07-21T12:40:57Z
👍
Commits
-------
1474aa5 [Form] Fixed consideration of Twig's template inheritance and added another performance-improving check
b4ec7f5 Fixed my rubbish English
d11f8b5 [Form] Fixed passing of variables in the FormRenderer
629093e [Form] Extracted common parts of FormHelper and FormExtension into separate classes
216c539 [Form] Implemented a more intelligent caching strategy in FormHelper (PHP +100ms, Twig +100ms)
Discussion
----------
[Form] Merged FormHelper and FormExtension and implemented a better caching strategy
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
This PR extracts common parts of `FormHelper` and `FormExtension` into implementations of the new interfaces `FormRendererInterface` and `FormRendererEngineInterface`. The implemented `AbstractRendererEngine` features a more intelligent caching strategy than the one used before. When this strategy was implemented directly in `FormHelper`, the performance of [this specific, heavy form](http://advancedform.gpserver.dk/app_dev.php/taxclasses/1) could be improved from **2.5** to **2.25 seconds** on my machine for PHP templates.
Due to the abstraction and delegation, the performance gain is not that big anymore, but we still have a performance gain of about **0.1 seconds** for both PHP and Twig in the above example. The second, big improvement of this PR is maintainability - the differences between PHP and Twig templates are now contained in relatively small classes - and extendability (it is very easy now to support different template engines).
---------------------------------------------------------------------------
by stof at 2012-07-14T13:47:19Z
should a similar refactoring be done for the [Twig rendering](https://github.com/symfony/symfony/blob/master/src/Symfony/Bridge/Twig/Extension/FormExtension.php) ?
---------------------------------------------------------------------------
by bschussek at 2012-07-14T13:49:25Z
Yes. I would like to merge the common parts of Twig's FormExtension and PHP's FormHelper into an abstract class. Before that I need to have a [working, heavy Twig Form](https://twitter.com/webmozart/status/224135287377371138) in order to measure whether I don't actually decrease the performance with Twig. Can you help me there?
---------------------------------------------------------------------------
by vicb at 2012-07-16T21:48:24Z
Would it make sense to create a 'renderer' folder in the form component and move related classes there ?
---------------------------------------------------------------------------
by stof at 2012-07-16T22:06:58Z
@vicb It makes sense to keep the Twig renderer in the brisge. This is what the bridge is about. Moving the Twig class to the component would not be consistent. And the PHP renderer is already in the component (but it could make sense to move the helper from FrameworkBundle to the TemplatingExtension of the Form component though)
---------------------------------------------------------------------------
by vicb at 2012-07-16T22:16:50Z
@stof I was only referring to the classes located in the Component/Form folder.
---------------------------------------------------------------------------
by vicb at 2012-07-16T22:27:27Z
Overall I don't really know what to think of this PR. PHP and Twig use a different way to support blocks:
- PHP has one block per file,
- Twig could have many blocks per templates.
I am not sure if this PR is optimal for Twig and improves maintainability ?
---------------------------------------------------------------------------
by stof at 2012-07-16T22:46:11Z
@vicb it avoids duplicating the whole rendering logic for each engine (there is at least a third one in [SmartyBundle](https://github.com/noiselabs/SmartyBundle/blob/master/Extension/FormExtension.php) btw)
---------------------------------------------------------------------------
by bschussek at 2012-07-17T07:16:42Z
@vicb I don't think a renderer subfolder makes sense. The interfaces belong to the main namespace, and then the subfolder would only contain two classes.
Considering maintainability for Twig, I think that this PR in fact increases it. TwigExtension before always had to check the whole type hierarchy, while now the code in AbstractRendererEngine makes sure that this process is speeded up.
Before:
```
load _some_entity_field_label:
- check _some_entity_field_label
- check entity_label
- check choice_label
- check form_label
load _some_other_entity_field_label
- check _some_other_entity_field_label
- check entity_label
- check choice_label
- check form_label
a.s.o.
```
After:
```
load _some_entity_field_label:
- check _some_entity_field_label
- check entity_label (hits the cache if entity_label was checked before)
- check choice_label (hits the cache if choice_label was checked before)
- check form_label
load _some_other_entity_field_label
- check _some_other_entity_field_label
- check entity_label (now definitely hits the cache)
a.s.o.
```
Since many fields share the same ancestors in the inheritance tree, this definitely improves performance.
As can also be deducted here, custom block names such as `_some_entity_field_label` are now a major drawback. There is nothing we can cache for them, so they need to be checked for every individual block that we load. Removing this feature surprisingly gains no performance for Twig (I need to investigate why at some point), but it speeds up rendering for **250ms** using the PHP engine on [this example form](advancedform.gpserver.dk/app_dev.php/taxclasses/1), dropping the rendering time from 1.25 to 1 sec on my local machine. I'm not sure what we should do here.
---------------------------------------------------------------------------
by stof at 2012-07-17T07:21:31Z
@bschussek could it be possible to have an implementation checking the custom block and another one skipping it ? This way, the user could disable this feature when he does not need it.
---------------------------------------------------------------------------
by bschussek at 2012-07-17T07:38:34Z
@stof It would be possible to add a switch to `FormRenderer` that controls whether custom blocks are checked or not.
If this switch is disabled by default, we break BC. If this switch is enabled by default, it will be pretty useless. People will start designing away for custom blocks, and once they want to improve performance, they can't turn off the switch anymore because it would require too many changes.
---------------------------------------------------------------------------
by stof at 2012-07-17T08:08:38Z
@fabpot what do you think about it ?
---------------------------------------------------------------------------
by bschussek at 2012-07-17T08:41:43Z
Another option that just came to mind is to remove inheritance checks for anything but _widget and _row. I.e., if we render `entity_widget`, check
```
_id_widget
entity_widget
choice_widget
form_widget
```
But if we render `entity_label`, only check
```
_id_label
form_label
```
This improves PHP Templating for **170ms** and Twig for **20ms**. We gain another **150ms** for PHP Templating and **~15ms** for Twig if we also restrict custom fields (_id_widget) to the _widget and _row suffixes (it's really hard to tweak the renderer for Twig.. I think a lot of its performance bottlenecks lie in Twig itself).
Do you have any data on how often blocks other than _widget and _row are customized for specific types/IDs?
---------------------------------------------------------------------------
by stof at 2012-07-17T09:47:38Z
Well, I think most of the time other blocks are not even customized based on the type :)
---------------------------------------------------------------------------
by Tobion at 2012-07-17T14:32:39Z
From my experience rendering the form components individually is easier and more flexible than customizing by ID or type.
But there are still use cases for customizing like library-like bundles (e.g. Bootstrap).
Commits
-------
8f99be3 [DoctrineBridge] Fixed the type guesser for doctrine 2.3
Discussion
----------
[DoctrineBridge] Fixed the type guesser for doctrine 2.3
Doctrine 2.3 now uses the drivers moved to Common, so the exception was not catched anymore and was breaking the guessing when a non-entity was used.
---------------------------------------------------------------------------
by craue at 2012-07-16T14:54:30Z
👍
---------------------------------------------------------------------------
by ddeboer at 2012-07-17T20:07:57Z
👍
---------------------------------------------------------------------------
by stof at 2012-07-17T20:17:01Z
@fabpot please merge this as 2.1 is currently broken when you rely on the form guessers for unmapped classes
Commits
-------
33f29ed [Form] '@group benchmark' for form performance tests
Discussion
----------
[Form] '@group benchmark' for form performance tests
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: [](http://travis-ci.org/asm89/symfony)
License of the code: MIT
I think a PR or note about this has been rejected before, but since build statuses on PRs sometimes seem to fail if travis is busy I think moving the form performance tests to `@group benchmark` should be reconsidered.
Edit: even master is currently failing on this
Commits
-------
cd7835d [Form] Cached the form type hierarchy in order to improve performance
2ca753b [Form] Fixed choice list hashing in DoctrineType
2bf4d6c [Form] Fixed FormFactory not to set "data" option if not explicitely given
7149d26 [Form] Removed invalid PHPDoc text
Discussion
----------
[Form] WIP Improved performance of form building
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: **Update the Silex extension**
This PR is work in progress and up for discussion. It increases the performance of FormFactory::createForm() on a specific, heavy-weight form from **0.848** to **0.580** seconds.
Before, the FormFactory had to traverse the hierarchy and calculate the default options of each FormType everytime a form was created of that type.
Now, FormTypes are wrapped within instances of a new class `ResolvedFormType`, which caches the parent type, the type's extensions and its default options.
The updated responsibilities: `FormFactory` is a registry and proxy for `ResolvedFormType` objects, `FormType` specifies how a form can be built on a specific layer of the type hierarchy (e.g. "form", or "date", etc.) and `ResolvedFormType` *does the actual building* across all layers of the hierarchy (by delegating to the parent type, which delegates to its parent type etc.).
---------------------------------------------------------------------------
by schmittjoh at 2012-07-12T18:25:40Z
Maybe ResolvedFormType
---------------------------------------------------------------------------
by jmather at 2012-07-13T02:56:38Z
I really like ResolvedFormType. That's the naming method I took for my tag parser that handes the same conceptual issue.
---------------------------------------------------------------------------
by axelarge at 2012-07-13T05:25:00Z
ResolvedFormType sounds very clear.
This change is great and I desperately hope to see more of this kind
---------------------------------------------------------------------------
by Baachi at 2012-07-13T06:41:26Z
Yes `ResolvedFormType` sounds good :) 👍
---------------------------------------------------------------------------
by fabpot at 2012-07-13T07:11:33Z
I like `ResolvedFormType` as well.
---------------------------------------------------------------------------
by henrikbjorn at 2012-07-13T07:46:48Z
👍 `ResolvedFormType` :shipit:
---------------------------------------------------------------------------
by stof at 2012-07-13T18:01:51Z
This looks good to me
By default, the UniqueEntityValidator maps the violation on the first
field of the UniqueEntity constraint. The new option allows to control
this behavior if a better mapping is suited.
Commits
-------
c1e4166 moved create of default form label to view layer
Discussion
----------
move create of default form label to view layer
A small optimization if you provide custom labels in the view layer (i.e. `{{ form_label(form.name, 'Your name') }}`
```
Bug fix: no
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: ~
Todo: ~
License of the code: MIT
Documentation PR: ~
```
---------------------------------------------------------------------------
by travisbot at 2012-06-24T14:45:17Z
This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1694310) (merged 37f0b774 into 0d4b02e4).
---------------------------------------------------------------------------
by travisbot at 2012-06-24T15:03:44Z
This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1694418) (merged c1e4166e into 0d4b02e4).
Commits
-------
23bad29 Added translation to placeholder and title attributes
Discussion
----------
Added translation to placeholder and title attributes
---------------------------------------------------------------------------
by schmittjoh at 2012-05-03T13:52:38Z
Better translate it where it is defined.
Dynamic translations are usually not desirable as they cannot be automatically extracted, and thus require more work.
---------------------------------------------------------------------------
by ruimarinho at 2012-05-03T13:57:30Z
@schmittjoh but isn't that the same case as with labels for instance? I don't think injecting the translator service into the form type would require less work than what this PR suggests.
---------------------------------------------------------------------------
by schmittjoh at 2012-05-03T14:02:02Z
Yeah, same thing.
There might be some cases where it's fine, but in general, we should try to not translate dynamic vars.
---------------------------------------------------------------------------
by ruimarinho at 2012-05-03T14:17:44Z
@schmittjoh I think that's one of those cases, since these attributes in particular (title and placeholder) are intended to aid the user with a brief description. I understand (and agree) with your concern regarding dynamic vars, but in my opinion this is a use case where it is worth it. Just my two cents :)
---------------------------------------------------------------------------
by stof at 2012-05-03T18:07:01Z
@schmittjoh the issue is that translating the label before the template would require injecting the translator in the form types (as the form label can be set there) and would force the user to duplicate the translation process if they pass the label explicitly in the template.
Commits
-------
a2b3d3c added cache service definition
Discussion
----------
[Doctrine Bridge] Added a method to load a cache definition
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
Following this discussion (https://github.com/doctrine/DoctrineBundle/pull/62), this will let DoctrineBundle, MongodbBundle and CouchdbBundle share the same code for cache definitions.
---------------------------------------------------------------------------
by dlsniper at 2012-04-30T06:56:49Z
+1 for this PR.
---------------------------------------------------------------------------
by stof at 2012-04-30T06:57:58Z
👍
---------------------------------------------------------------------------
by fabpot at 2012-04-30T15:41:05Z
Can you add a note abou this change in the CHANGELOG?
---------------------------------------------------------------------------
by stof at 2012-04-30T15:46:48Z
does it really need to be in the changelog ? End-users don't know about this at all. The only guys affected by this change are the maintainers of the different Doctrine bundles as they can remove some code now.
---------------------------------------------------------------------------
by fabpot at 2012-04-30T16:41:21Z
@stof: right
@bamarni: Can you squash your commits?
---------------------------------------------------------------------------
by bamarni at 2012-04-30T17:03:38Z
@fabpot : done
---------------------------------------------------------------------------
by dlsniper at 2012-04-30T17:22:07Z
@bamarni can you also do a patch for the docs after this gets merged so that people know about this change and know how to use it?
Thank you!
---------------------------------------------------------------------------
by bamarni at 2012-04-30T17:29:05Z
@dlsniper : no problem ;)
---------------------------------------------------------------------------
by fabpot at 2012-04-30T18:29:03Z
ping @beberlei
Commits
-------
246c885 [Form] Fixed: Default value of 'error_bubbling' is now determined by the 'single_control' option
d3bb4d0 [Form] Renamed option 'primitive' to 'single_control'
167e64f [Form] Fixed: Field attributes are not rendered in the label anymore. Label attributes are now passed in "label_attr"
68018a1 [Form] Dropped useless test that is guaranteed by OptionsParser tests and that needs to be adapted very often
649752c [Form] Fixed: CSRF token was not displayed on empty complex forms
c623fcf [Form] Fixed: CSRF protection did not run if token was missing
eb75ab1 [Form] Fixed results of the FieldType+FormType merge.
Discussion
----------
[Form] Fixed errors introduced in the FieldType+FormType merge
Bug fix: yes
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: #3994, #4000, #2294, #4118
Todo: -
---------------------------------------------------------------------------
by Tobion at 2012-04-22T15:39:20Z
`primitive` is a pretty abstract option name. It depends on the person what he considers primitive. Maybe more explicit naming or better documentation what it means.
---------------------------------------------------------------------------
by bschussek at 2012-04-22T15:47:29Z
Better suggestions?
The distinction here is between primitive and complex forms, where primitive forms are such forms that can be represented by a single HTML tag. This obviously needs to be documented.
---------------------------------------------------------------------------
by Tobion at 2012-04-22T15:49:45Z
Maybe `single_widget` or something like that.
---------------------------------------------------------------------------
by vicb at 2012-04-23T13:09:43Z
@Tobion @bschussek would `elementary` be better than `primitive` ?
---------------------------------------------------------------------------
by vicb at 2012-04-23T13:17:04Z
and `compound \ composite` better than `complex` ?
---------------------------------------------------------------------------
by bschussek at 2012-04-23T14:08:33Z
@vicb I fail to see how elementary/compound is easier to understand than primitive/complex. Maybe single_widget, but what's the opposite of this case? multi_widget?
---------------------------------------------------------------------------
by vicb at 2012-04-23T14:15:09Z
Actually I am fine with anything... as long as it is documented.
---------------------------------------------------------------------------
by bschussek at 2012-04-23T14:22:31Z
Still I think that this unveals a more profound naming problem. How do we (also in the documentation) name forms with children (formerly "forms") and forms without children (formerly "fields")?
Should we refer to them as
* forms and fields?
* complex and primitive forms?
* ...
We must first answer this question before we can find an intuitive option name. If the documentation always switches between different terminologies, neither will it be understandable nor will this option be easy to remember.
---------------------------------------------------------------------------
by vicb at 2012-04-23T15:10:32Z
> Still I think that this unveals a more profound naming problem. How do we (also in the documentation) name forms with children (formerly "forms") and forms without children (formerly "fields")?
To make it clear, I would rather say forms that **can have** children and forms that **can not have** children (i.e. Empty collections have no children but they can have and this is reason why you have to introduce those options, right ? - that could be a good example for the doc).
It will probably be better to refer to "complex" / "primitive" forms in the doc (and use the "form" / "field" terms to explain them).
Note: I think @Tobion concern is that "primitive" / "complex" could be pejorative terms (this is why I have proposed "elementary" / "compound").
---------------------------------------------------------------------------
by Tobion at 2012-04-23T16:00:54Z
1. primitive/complex is subjective (and could be pejorative too)
2. elementary/compound is more explicit so probably better than primitive/complex
3. I dislike this option in general. Does it make sense to change this option from a user perspective? I guess it's always the same as long as the widget structure stays the same. So it should be resolved at a higher level dynamically from the widget structure and not exposed to any configuration.
4. In documentation I would use the terms forms and fields. Because all people with HTML knowledge will understand that fields cannot have sub-fields whereas forms can. But since this distinction is not findable in code, it should be mentioned that all these are implemented as a form hierarchy.
---------------------------------------------------------------------------
by mvrhov at 2012-04-23T16:02:00Z
how about simple and complex?
---------------------------------------------------------------------------
by bschussek at 2012-04-23T16:06:33Z
@Tobion It does not make sense to change this option from the user perspective, still the overloading type has to propagate to FormType whether it is a form or a field, so that the default behaviour is correct.
A second option how to implement this is to add a method `isField` to FormTypeInterface that can be overloaded and receives the options. I don't really like to introduce new methods here unless absolutely required.
What about renaming the option "primitive" to "is_field"? The blocks in the template would then be named "form_widget_field" and "form_widget_form".
---------------------------------------------------------------------------
by tristanbes at 2012-04-25T14:01:06Z
Oh, I should've seen this before, i thought I was doing something wrong. (empty collections gets an input field bug)
Please big :UP: on this. When will it be merged ? @bschussek
---------------------------------------------------------------------------
by Tobion at 2012-04-25T15:30:28Z
+1 for "is_field" and "form_widget_field" but I would rather use "form_widget_compound" instead of "form_widget_form" which is quite strange.
---------------------------------------------------------------------------
by bschussek at 2012-04-26T16:34:04Z
@Tobion "simple" and "compound" then?
---------------------------------------------------------------------------
by Tobion at 2012-04-26T16:49:58Z
no "field" and "compound"
---------------------------------------------------------------------------
by bschussek at 2012-04-26T17:17:02Z
I don't like "field" for a simple reason: Consider the "date" type. We are typically speaking of the "date" field there. But technically, the "date" field is a compound field. So?
---------------------------------------------------------------------------
by Tobion at 2012-04-26T21:17:37Z
I don't understand the open question. You proposed "is_field" and "form_widget_field" yourself. So calling the template block "form_widget_field" is a comprehensible consequence of "is_field". I wouldn't call the date type with multiple inputs a field.
---------------------------------------------------------------------------
by tristanbes at 2012-04-26T21:52:39Z
We should take a decision cause right here i got all my forms that are broken because of the empty collection rendering as input field :-).
I guess we are many in that situation.
---------------------------------------------------------------------------
by bschussek at 2012-04-27T08:28:16Z
I renamed "primitive" to "single_control" now to match with the HTML specification which names all input elements (input, select etc.) "controls". The opposite is now "compound".
Meanwhile, I added a fix for #4118.
@fabpot This is ready for merge now.
---------------------------------------------------------------------------
by Tobion at 2012-04-27T10:22:49Z
Hm, I know naming things is hard and sometimes not really important. But since users need to know which block to override, it is essential to make it clear. I think there is still one issue.
The block is named `form_widget_single_control` in order, as you said, to abstract away if it's an input, select etc. But in fact it can only render `input` and nothing else. So this is misleading.
So you could also simply name it `form_widget_input`.
Apart from that I agree with everything.
The WebProcessor can now be registered as a kernel.request listener to
get the request instead of passing it as a constructor argument, which
was broken as the request is not yet available when the logger is
instantiated.
