Commits
-------
1f33756 Update master
Discussion
----------
Allow HttpKernel ->render to be called directly
Hi,
I faced this problem in a funny situation.
Working on tests of individual components, I want to be able to test sub-requests. These elements are not mapped directly, relying on `FrameworkBundle` routing internal to achieve its goal.
The nature of my application leads to fully decoupled bundles, being a website responsible to define everything, from app configuration to routing elements. That way, an individual bundle controller don't know the route it should call, leading the test to be harder to do.
Together with this situation, it is also required in my testing scenario to be able to test the real world execution, then being an ESI include (a sub-request). This test then needs to be able to directly call controller to be rendered. That said, `HttpKernel` provides an API that does exactly what is required to achieve my goal, calling `render()` which triggers the sub-request and also accepts options which allows me to test the real world scenarios.
But as soon as you trigger the method, `ProfileListener` intercepts the kernel response to collect profiling information. Under this specific situation, since you called directly `render`, there's no master request, then leading the test to fail with the following PHP warning:
```
Warning: SplObjectStorage::offsetExists() expects parameter 1 to be object, boolean given in /var/www/nde/vendor/symfony/symfony/src/Symfony/Component/HttpKernel/EventListener/ProfilerListener.php line 120
```
To fix that, all that is needed is a check for a possible parent request. That's the purpose of this patch. =)
---------------------------------------------------------------------------
by stof at 2012-07-17T15:11:13Z
This looks good to me, but as said on IRC, a test should be added to avoid regressions
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
-------
610c602 [OptionsResolver] Slightly tweaked the performance of the Options class
Discussion
----------
[OptionsResolver] Slightly tweaked the performance of the Options class
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
Commits
-------
0d0a968 [Templating] Cached the result of escape() in order to improve performance (+470ms)
Discussion
----------
[Templating] Cached the result of escape() in order to improve performance
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
This improvement gains **400ms** of rendering speed on [this particular example page](http://advancedform.gpserver.dk/app_dev.php/taxclasses/1).
---------------------------------------------------------------------------
by lsmith77 at 2012-07-16T17:36:50Z
i guess we don't have to be concerned with increased memory usage here .. if at all we could offer a clear cache method in case someone is f.e. using this to generate tons of messages in a cron job.
---------------------------------------------------------------------------
by henrikbjorn at 2012-07-17T06:39:52Z
The example form is broken.
---------------------------------------------------------------------------
by bschussek at 2012-07-17T07:21:26Z
The source code for the form can be found [here](https://github.com/stof/symfony-standard/blob/twig_forms/src/AdvancedForm/CoreBundle/Form/TaxClassType.php).
---------------------------------------------------------------------------
by henrikbjorn at 2012-07-17T07:28:11Z
But i am guessing this is only for php not twig :P
---------------------------------------------------------------------------
by bschussek at 2012-07-17T07:41:07Z
Obviously..
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
-------
17ca9b6 [Form] Fixed DoctrineType to use getManagerForClass() if no EM name is given
Discussion
----------
[Form] Fixed DoctrineType to use getManagerForClass() if no EM name is given
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #4125
Todo: -
---------------------------------------------------------------------------
by stof at 2012-07-17T08:16:59Z
👍
Commits
-------
764b1de [FrameworkBundle] minor typo in controller action docblock
Discussion
----------
[FrameworkBundle] minor typo in controller action docblock
Commits
-------
b4d1bdf [Form] added a bc break note about the tag alias matching
Discussion
----------
[Form] added a bc break note about the tag alias matching
6489a65960 is a BC break if we were relying on the previous behavior.
Commits
-------
1f2f866 fixed the serialization of the SwitchUserRole
b55930a [Security] Implemented the Serializable interface in the Role class
Discussion
----------
[Security] Implemented the Serializable interface in the Role class
The Role class is serialized in the session for each role of the user. Implementing the Serializable interface allows to reduce the size of the data.
Commits
-------
df2406f [Security] Add note to changelog about BC break
01b2e39 [Security] Extract default logout success handling logic
Discussion
----------
[Security] Extract default logout success handling logic
Bug fix: no
Feature addition: no
Backwards compatibility break: yes, small one for people using the component
Symfony2 tests pass: [](http://travis-ci.org/asm89/symfony)
License of the code: MIT
As discussed earlier with @fabpot and @schmittjoh. This PR extracts the default logout success handling logic to a separate class that users can extend.
Note: build status is red, but that is because of a failing performance test in the form component? ..
Commits
-------
7d53909 Earlier PHP output buffer flush for non FPM environments
Discussion
----------
Earlier PHP output buffer flush for non FPM environments
In the Response::send() method you are calling the fastcgi_finish_request() in case it exists. This will provide a respectful performance boost when you have significant work being done by listeners acting on kernel terminal events; Sadly you are forgetting people that don't use FPM doing this.
The performance boost for a Vanilla PHP is not much: flushing earlier potentially helps higher layers such as the HTTPd or potential other cache layers: the sooner their buffer gets filled, the sooner they release information to the browser, even if the output buffer is still open. The explicit flush() is supposed to do exactly this.
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
-------
07992d3 [Validator] Added inheritDoc phpdoc for validate methods
Discussion
----------
[Validator] Added inheritDoc phpdoc for validate methods
Was instructed by @stof to do this for a PR on comparison validators and noticed none of the validators used inheritDoc.
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: n/a
Todo: I haven't looked around too much, but I assume if none of the validators followed this standard that there would be a fair few other classes not using. Obviously not a big issue though
License of the code: MIT
Documentation PR: n/a
Commits
-------
5ae0da0 Update changelogs
ff91b9a [FrameworkBundle] Make FlashBag the default.
Discussion
----------
[2.1][FrameworkBundle] Make FlashBag the default.
Bug fix: no
Feature addition: yes
Backwards compatibility break: yes (but only technically)
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
License of the code: MIT
The difference between `AutoExpireFlashBag` and `FlashBag` is simply that the first will expire flashes regardless of being displayed on the next pageload. This can result in lost messages. It was created simply for BC with 2.0.
`FlashBag` expires flashes once they are retrieved. This also makes it ESI compatible.
/cc @lsmith77
---------------------------------------------------------------------------
by jalliot at 2012-07-14T18:13:40Z
+1!
You should add it to the changelog and upgrade files though :)
Fabien Potencier
Tobias Schultze
Bernhard Schussek
Jonathan Ingram
Guilherme Blanco
Christophe Coevoet
Jordan Alliot
Bilal Amarni
