Commits
-------
cd24fb8 change explode's limit parameter based on known variable content
b3cc270 minor optimalisations for explode
Discussion
----------
[FrameworkBundle][CssSelector][HttpFoundation][HttpKernel] [Security][Validator] Minor optimizations for "explode" function
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
I added limit parameter in some places, where it may be usefull. I did not check the context of what values may have been exploded. So to not break anything, I added +1 to limit parameter.
If you find out that in some places limit (or limit+1) is not important or meaningless, write a comment please and I will fix it.
---------------------------------------------------------------------------
by fabpot at 2011/12/07 06:56:49 -0800
Adding +1 just to be sure to not break anything is clearly something we won't do. What is the benefit of doing that anyway?
---------------------------------------------------------------------------
by pulzarraider at 2011/12/07 13:50:24 -0800
The main idea of making this PR was to notify about some places that may run faster with just adding one parameter to explode function.
If in code is someting like: ```list($a, $b) = explode(':', $s);```
Function ```explode``` will create n-items (depends on ```$s```), but we need in code only the first two items. There is no reason to let ```explode``` create more items in memory that are NEVER used in our code. The limit parameter is there for these situations, so let's use it.
I know that it is microoptimization and may look unimportant, but we are writing a framework - so people expect that code will be as fast as possible without this kind of mistakes.
As I've noticed above, I know that +1 is not ideal solution, but the fastest without debugging the code. I expect that someone (with good knowledge of that code) will look at it and write in comments if variable may contain 1 comma (dot or someting on what is doing the explode) or maybe 2 in some situations or more.
Anyway, +1 will not break anything, because same items are created as it is now, but no unnecessary item is created.
---------------------------------------------------------------------------
by fabpot at 2011/12/07 23:14:59 -0800
I'm +1 for adding the number to avoid problems but I'm -1 on the optimization side of things as it won't optimize anything.
---------------------------------------------------------------------------
by helmer at 2011/12/08 12:46:49 -0800
*.. The main idea of making this PR was to notify about some places that **may** run faster ..*
I am also unsure the optimization is really an optimization, care to benchmark (with meaningful inputs)? As for the limit+1 thing, why would you want to +1 it? The number of ``list`` arguments should always reflect the ``limit`` parameter, no?
---------------------------------------------------------------------------
by pulzarraider at 2011/12/08 23:11:34 -0800
@helmer please try this simple benchmark:
```
<?php
header('Content-Type: text/plain; charset=UTF-8');
define('COUNT', 10000);
$source_string = 'aaaaaaaaaaaaaaaaaaaa:bbbbbbbbbbbbbbbbbbbbb:cccccccccccccccccccccccc:dddddddddddddddddddddd:eeeeeeeeeeeeeeeeeeeeeeeee:fffffffffffffffffffffffffff';
$start = microtime(true);
for ($i = 0; $i < COUNT; $i++) {
list($a, $b) = explode(':', $source_string);
}
$end = microtime(true)-$start;
echo 'without limit: '.$end."\n";
$start = microtime(true);
for ($i = 0; $i < COUNT; $i++) {
list($a, $b) = explode(':', $source_string, 2);
}
$end = microtime(true)-$start;
echo 'with limit: '.$end."\n";
```
My results are:
```
without limit: 0.057228803634644
with limit: 0.028676986694336
```
That is 50% difference (with APC enabled). Of course the result depends on the length of source string and if it's too short, the difference may be none or very very small. That's why I said, that it **may** run faster and is just a micro optimization.
---------------------------------------------------------------------------
by pulzarraider at 2011/12/08 23:18:12 -0800
@helmer And why +1? It depends on a code:
```
$source_string = 'aaaaaaaaaaaaaaaaaaaa:bbbbbbbbbbbbbbbbbbbbb:cccccccccccccccccccccccc';
list($a, $b) = explode(':', $source_string, 2);
var_dump($a, $b);
```
and
```
$source_string = 'aaaaaaaaaaaaaaaaaaaa:bbbbbbbbbbbbbbbbbbbbb:cccccccccccccccccccccccc';
list($a, $b) = explode(':', $source_string, 3);
var_dump($a, $b);
```
gives different results. That's why the content of the variable must be known.
---------------------------------------------------------------------------
by helmer at 2011/12/09 00:08:28 -0800
@pulzarraider Thanks for the benchmark, seems like a gain enough. Although, we are more likely having a scenario of:
``explode(':', 'a🅱️c')`` vs ``explode(':', 'a🅱️c', 3)`` with a ``COUNT`` of 10, where the difference is not even in microseconds anymore :)
The limit addition alters the behaviour though, ie suddenly you can define a controller [logical name](http://symfony.com/doc/current/book/routing.html#controller-string-syntax) as ´´AcmeBlogBundle:Blog:show:something``, and things go downhill from there on.
All that aside, I'm +1 for setting the limit to the exact number of ``list`` parameters, but certainly not number+1, this is just too wtfy (as you said, this was a safety thing, but I reckon for this PR to be merged it needs to be +0).
---------------------------------------------------------------------------
by drak at 2011/12/09 08:28:58 -0800
Overall `list()` is ugly as it's not very explicit. Even though it would mean extra lines, it's better to `explode()` then explicitly assign variables:
```
$parts = explode(':', $foo);
$name = $parts[0];
$tel = $parts[1];
```
`list()` is one of those bad relics from the PHP past...
---------------------------------------------------------------------------
by fabpot at 2011/12/11 10:07:47 -0800
@drak: why is `list` not explicit? It is in fact as explicit as the more verbose syntax you propose.
---------------------------------------------------------------------------
by pulzarraider at 2011/12/11 13:08:50 -0800
@drak: I agree with @fabpot. In speech of benchmarks ```list``` is faster then using a helper variable.
@fabpot, @helmer I've changed explode's limit to be correct (without +1) and removed some changes from this PR, where I can't find out what the content of variable may be. Unit tests pass, so I think it's ready for merge.
Commits
-------
07fa82d [Form] Revert changes impacting perfomance and readability
b709551 [Order] Make Form::types and FormView::types use the same order (Parent > Child)
e56dad6 [Form] simplify the code
bdd755e [Form] Fix the exception message when no block is found
c68c511 [Form] Make theming form prototypes consistent (start by looking for a '_<id>_<section>' block)
9ec9960 [Form] Simplify the code
4e3e276 [Form] Make the prototype view child of the collection view
Discussion
----------
[Form] Make the prototype view child of the collection view
This PR should be a base for discussion.
The [current implementation](https://github.com/symfony/symfony/pull/1188) has some drawbacks because the prototype view is not a child of the collection view:
* The 'multipart' attribute is not propagated from the prototype to the collection,
* The prototype view do not use the theme from the collection.
Those 2 points are fixed by the proposed implementation and one more benefit is that the template markup might be easier to work with:
before:
```html
<div id="form_emails">
<div>
<label for="form_emails_0">0</label>
<input type="email" id="form_emails_0" name="form[emails][0]" value="a@b.com">
</div>
<script type="text/html" id="form_emails_prototype">
<div>
<label for="$$name$$">$$name$$</label>
<input type="email" id="$$name$$" name="$$name$$" value="" />
</div>
</script>
</div>
```
after:
```html
<div id="form_emails">
<div>
<label for="form_emails_0">0</label>
<input type="email" id="form_emails_0" name="form[emails][0]" value="a@b.com">
</div>
<script type="text/html" id="form_emails_prototype">
<div>
<label for="form_emails_$$name$$">$$name$$</label>
<input type="email" id="form_emails_$$name$$" name="form[emails][$$name$$]" value="" />
</div>
</script>
</div>
```
@kriswallsmith I'd like to get your feedback on this PR. thanks.
---------------------------------------------------------------------------
by stof at 2011/06/14 07:01:01 -0700
@fabpot any ETA about merging it ? Using the prototype currently is a pain to build the name. The change makes it far easier
---------------------------------------------------------------------------
by fabpot at 2011/06/14 07:09:46 -0700
The templates are much better but I'm a bit concerned that we need to add the logic into the Form class directly. That looks quite ugly. If there is no other way, I will merge it.
---------------------------------------------------------------------------
by vicb at 2011/06/14 07:14:32 -0700
I have found no better way... I am testing some minor tweaks I want to submit.
---------------------------------------------------------------------------
by kriswallsmith at 2011/06/14 07:34:25 -0700
I'm not happy with the code in Form.php either... would creating a PrototypeType accomplish the same thing?
---------------------------------------------------------------------------
by vicb at 2011/06/14 07:42:07 -0700
@kriswallsmith tried and dismissed, the id and name are bad & you have to go for `render_widget(form.get('proto'))` in the template. That should be fixeable but not any better.
---------------------------------------------------------------------------
by kriswallsmith at 2011/06/14 07:45:21 -0700
What do you mean the id and name are bad? If we have a distinct type for the prototype, can't we do whatever we want using buildView() and the template?
---------------------------------------------------------------------------
by vicb at 2011/06/14 07:53:31 -0700
@kriswallsmith the id would be smthg like `form_emails_$$name$$_prototype` but yes we should be able to do whatever we want but the code might end up being more complex.
I am done with the tweaks but still open to feedback on this PR.
---------------------------------------------------------------------------
by kriswallsmith at 2011/06/14 08:08:21 -0700
Yes, that is the type of name I would expect.
---------------------------------------------------------------------------
by kriswallsmith at 2011/06/14 08:08:33 -0700
Oops -- I mean id.
---------------------------------------------------------------------------
by kriswallsmith at 2011/06/14 08:09:42 -0700
Maybe I'm confused what id you're referring to. I'll try to spend some time on this today.
---------------------------------------------------------------------------
by vicb at 2011/06/14 08:23:56 -0700
That should be the id of the `<input>`, the id of the script would be `form_emails_$$name$$_prototype_prototype` (if prototype is the name of the nested node).
I am trying to setup a branch with my code (playing with git & netbeans local history)
---------------------------------------------------------------------------
by vicb at 2011/06/14 08:46:25 -0700
@kriswallsmith https://github.com/vicb/symfony/tree/kris/proto if that can help (there are still changes in Form.php)
---------------------------------------------------------------------------
by kriswallsmith at 2011/06/14 08:47:08 -0700
Thanks, I'll take a look.
---------------------------------------------------------------------------
by vicb at 2011/06/15 00:48:38 -0700
I would have expected it to be faster however `array_map` is about twice slower... reverted !
If some of the nested views are rendered individually they should not be rendered again when calling form_rest.
A typical would be when some nested file views are rendered, form_rest should not render them again.
It is still possible to render a label once the widget has been rendered. This is for checkboxes and radios
where the widget is typically rendered before the label.
The consequence of this commit is that variables are accessible that have been passed to a surrounding form helper.
Example template:
{% block my_widget_label %}
<label>{{ label }}
{% endblock %}
{% block my_widget_row %}
{# It is not necessary to explicitely pass through the label variable #}
{{ form_label(form) }}
{{ form_widget(form) }}
{% endblock %}
Example usage:
{{ form_row(form.mywidget, { 'label': 'My Widget' }) }}
[Form] Fixed {get,set,has}Var references in templating php
[Form] Added getVars to FormView to ease usage in Twig. Also added some phpdoc and cleaned up the get method by adding a default value
[Form] Fix
[Form] Delete file generated by test
