Commits
-------
8dc78bd [Form] Fixed YODA issues
600cec7 [Form] Added missing entries to CHANGELOG and UPGRADE
b154f7c [Form] Fixed docblock and unneeded use statement
399af27 [Form] Implemented checks to assert that values and indices generated in choice lists match their requirements
5f6f75c [Form] Fixed outstanding issues mentioned in the PR
7c70976 [Form] Fixed text in UPGRADE file
c26b47a [Form] Made query parameter name generated by ORMQueryBuilderLoader unique
18f92cd [Form] Fixed double choice fixing
f533ef0 [Form] Added ChoiceView class for passing choice-related data to the view
d72900e [Form] Incorporated changes suggested in PR comments
28d2f6d Removed duplicated lines from UPGRADE file
e1fc5a5 [Form] Restricted form names to specific characters to (1) fix generation of HTML IDs and to (2) avoid problems with property paths.
87b16e7 [Form] Greatly improved ChoiceListInterface and all of its implementations
Discussion
----------
[Form] Improved ChoiceList implementation and made form naming more restrictive
Bug fix: yes
Feature addition: yes
Backwards compatibility break: **yes**
Symfony2 tests pass: yes
Fixes the following tickets: #2869, #3021, #1919, #3153
Todo: adapt documentation
The changes in this PR are primarily motivated by the fact that invalid form/field names lead to various problems.
1. When a name contains any characters that are not permitted in HTML "id" attributes, these are invalid
2. When a name contains periods ("."), form validation is broken, because they confuse the property path resolution
3. Since choices in expanded choice fields are directly translated to field names, choices applying to either 1. or 2. lead to problems. But choices should be unrestricted.
4. Unless a choice field is not expanded and does not allow multiple selection, it is not possible to use empty strings as choices, which might be desirable in some occasions.
The solution to these problems is to
* Restrict form names to disallow unpermitted characters (solves 1. and 2.)
* Generate integer indices to be stored in the HTML "id" and "name" attributes and map them to the choices (solves 3.). Can be reverted to the old behaviour by setting the option "index_generation" to ChoiceList::COPY_CHOICE
* Generate integer values to be stored in the HTML "value" attribute and map them to the choices (solves 4.). Can be reverted to the old behaviour by setting the option "value_generation" to ChoiceList::COPY_CHOICE
Apart from these fixes, it is now possible to write more flexible choice lists. One of these is `ObjectChoiceList`, which allows to use objects as choices and is bundled in the core. `EntityChoiceList` has been made an extension of this class.
$form = $this->createFormBuilder()
->add('object', 'choice', array(
'choice_list' => new ObjectChoiceList(
array($obj1, $obj2, $obj3, $obj4),
// property path determining the choice label (optional)
'name',
// preferred choices (optional)
array($obj2, $obj3),
// property path for object grouping (optional)
'category',
// property path for value generation (optional)
'id',
// property path for index generation (optional)
'id'
)
))
->getForm()
;
---------------------------------------------------------------------------
by kriswallsmith at 2012-01-19T18:09:09Z
Rather than passing `choices` and a `choice_labels` arrays to the view would it make sense to introduce a `ChoiceView` class and pass one array of objects?
---------------------------------------------------------------------------
by stof at 2012-01-22T15:32:36Z
@bschussek can you update your PR according to the feedback (and rebase it as it conflicts according to github) ?
---------------------------------------------------------------------------
by bschussek at 2012-01-24T00:15:42Z
@kriswallsmith fixed
Fixed all outstanding issues. Would be glad if someone could review again, otherwise this PR is ready to merge.
---------------------------------------------------------------------------
by fabpot at 2012-01-25T15:17:59Z
Is it ready to be merged?
---------------------------------------------------------------------------
by Tobion at 2012-01-25T15:35:50Z
Yes I think so. He said it's ready to be merged when reviewed.
---------------------------------------------------------------------------
by bschussek at 2012-01-26T02:30:36Z
Yes.
---------------------------------------------------------------------------
by bschussek at 2012-01-28T12:39:00Z
Fixed outstanding issues. Ready for merge.
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.
The locale management does not require sessions anymore.
In the Symfony2 spirit, the locale should be part of your URLs. If this is the case
(via the special _locale request attribute), Symfony will store it in the request
(getLocale()).
This feature is now also configurable/replaceable at will as everything is now managed
by the new LocaleListener event listener.
How to upgrade:
The default locale configuration has been moved from session to the main configuration:
Before:
framework:
session:
default_locale: en
After:
framework:
default_locale: en
Whenever you want to get the current locale, call getLocale() on the request (was on the
session before).
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 !
