Commits
-------
c195957 [Components] Tests/Autoloading fixes
Discussion
----------
Fix components
See #4141
----
This PR:
* configures each component to use composer to manage "dev" dependencies instead of env variables;
* adds phpunit configuration file on Filesystem component;
* fixes READMEs.
It's mergeable without any problems, but I would recommend to wait a fix in Composer in order to use `self.version` in `require`/`require-dev` sections.
Note: I kept `suggest` sections because it makes sense but this PR doesn't aim to provide useful explanations for each entry. It could be another PR, not that one.
---------------------------------------------------------------------------
by willdurand at 2012-04-30T20:43:13Z
@fabpot I reviewed each component, one by one. Now `phpunit` always works, even if tests are skipped. A simple `composer install --dev` allows to run the complete test suite. Each commit is well separated from the others. I guess, everything is ok now.
---------------------------------------------------------------------------
by Tobion at 2012-04-30T20:47:00Z
Please squash, as it makes no sense to have the same commit for each component.
---------------------------------------------------------------------------
by fabpot at 2012-05-01T14:26:11Z
Can you squash your commits before I merge? Thanks.
---------------------------------------------------------------------------
by willdurand at 2012-05-01T14:29:38Z
done
---------------------------------------------------------------------------
by fabpot at 2012-05-01T15:48:25Z
It does not seem that the commits are squashed.
---------------------------------------------------------------------------
by willdurand at 2012-05-01T15:54:08Z
done
* Switched to Composer to manage "dev" dependencies
* Fixed READMEs
* Excluded vendor in phpunit.xml.dist files
* Fixed message in bootstrap.php files
* Added autoloader for the component itself
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.
Commits
-------
8bdff01 [DoctrineBridge][Form] added collection guess for array Doctrine type and array constraint type
Discussion
----------
[Form] [DoctrineBridge] Better field type guessing for array doctrine type and array validator type
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #1692
Todo: -
---------------------------------------------------------------------------
by bschussek at 2012-04-18T08:45:17Z
Could you please add an entry to the CHANGELOG and squash your commits into one?
---------------------------------------------------------------------------
by pvanliefland at 2012-04-18T17:20:39Z
Done
CSRF fields are now only added when the view is built. For this reason we already know if
the form is the root form and avoid to create unnecessary CSRF fields for nested fields.
Commits
-------
be2456b [Form] [Tests] Used assertCount()
4120f13 [Form] Added all() method to the FormBuilder class
Discussion
----------
[Form] Added all() method to the FormBuilder class
In order to perform some introspection on a FormBuilder instance,
we need to be able to get its children. Almost everything is accessible
in this class, but the children are not.
This PR adds a all() method to respect the current API (get(), remove(),
...).
---------------------------------------------------------------------------
by bschussek at 2012-04-12T09:54:04Z
👍
In order to perform some introspection on a FormBuilder instance,
we need to be able to get its children. Almost everything is accessible
in this class, but the children are not.
This PR adds a all() method to respect the current API (get(), remove(),
...).
Commits
-------
8329087 [Form] Moved calculation of ChoiceType options to closures
5adec19 [Form] Fixed typos
cb87ccb [Form] Failing test for empty_data option BC break
b733045 [Form] Fixed option support in Form component
Discussion
----------
[Form] Fixed option support in Form component
Bug fix: yes
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: #3354, #3512, #3685, #3694
Todo: -
This PR also introduces a new helper `DefaultOptions` for solving option graphs. It accepts default options to be defined on various layers of your class hierarchy. These options can then be merged with the options passed by the user. This is called *resolving*.
The important feature of this utility is that it lets you define *lazy options*. Lazy options are specified using closures that are evaluated when resolving and thus have access to the resolved values of other (potentially lazy) options. The class detects cyclic option dependencies and fails with an exception in this case.
For more information, check the inline documentation of the `DefaultOptions` class and the UPGRADE file.
@fabpot: Might this be worth a separate component? (in total the utility consists of five classes with two associated tests)
---------------------------------------------------------------------------
by beberlei at 2012-04-05T08:54:10Z
"The important feature of this utility is that it lets you define lazy options. Lazy options are specified using closures"
What about options that are closures? are those differentiated?
---------------------------------------------------------------------------
by bschussek at 2012-04-05T08:57:35Z
@beberlei Yes. Closures for lazy options receive a Symfony\Component\Form\Options instance as first argument. All other closures are interpreted as normal values.
---------------------------------------------------------------------------
by stof at 2012-04-05T11:09:49Z
I'm wondering if these classes should go in the Config component. My issue with it is that it would add a required dependency to the Config component and that the Config component mixes many different things in it already (the loader part, the resource part, the definition part...)
---------------------------------------------------------------------------
by sstok at 2012-04-06T13:36:36Z
Sharing the Options class would be great, and its more then one class so why not give it its own Component folder?
Filesystem is just one class, and that has its own folder.
Great job on the class bschussek 👏
---------------------------------------------------------------------------
by bschussek at 2012-04-10T12:32:34Z
@fabpot Any input?
---------------------------------------------------------------------------
by bschussek at 2012-04-10T13:54:13Z
@fabpot Apart from the decision about the final location of DefaultOptions et al., could you merge this soon? This would make my work a bit easier since this one is a blocker.
---------------------------------------------------------------------------
by fabpot at 2012-04-10T18:08:18Z
@bschussek: Can you rebase on master? I will merge afterwards. Thanks.
This demonstrates the issue described in symfony/symfony#3354. FieldType no longer has access to the child type's data_class option, which makes it unable to create the default closure for empty_data.
Commits
-------
f9a486e [Validator] Added support for pluralization of the SizeLengthValidator
c0715f1 [FrameworkBundle], [TwigBundle] added support for form error message pluralization
7a6376e [Form] added support for error message pluralization
345981f [Validator] added support for plural messages
Discussion
----------
[Validator] Added support for plural error messages
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Todo: create translations for en and update others (FrameworkBundle)
[](http://travis-ci.org/hason/symfony)
---------------------------------------------------------------------------
by fabpot at 2011-05-14T20:41:01Z
@bschussek: What's your opinion?
---------------------------------------------------------------------------
by stof at 2011-09-04T13:14:29Z
@hason could you rebase your branch on top of master and update the PR ?
You also need to change the messages in the constraint that uses the pluralization to a pluralized format.
---------------------------------------------------------------------------
by stof at 2011-10-16T18:06:22Z
@hason ping
---------------------------------------------------------------------------
by stof at 2011-11-11T14:58:19Z
@hason ping again
---------------------------------------------------------------------------
by stof at 2011-12-12T20:39:10Z
@hason ping again. Can you update your PR ?
---------------------------------------------------------------------------
by hason at 2011-12-12T21:29:14Z
@stof I hope that I will update PR this week.
---------------------------------------------------------------------------
by bschussek at 2012-01-15T19:07:32Z
Looks good to me.
---------------------------------------------------------------------------
by canni at 2012-02-02T17:28:54Z
@hason can you update this PR and squash commits, it conflicts with current master
---------------------------------------------------------------------------
by hason at 2012-02-09T07:21:41Z
@stof, @canni Rebased.
What is the best solution for the translation of messages?
1. Change messages in the classes and all xliff files?
2. Keep messages in the classes and change all xliff files?
---------------------------------------------------------------------------
by stof at 2012-02-09T08:19:41Z
The constraints contain the en message so you will need to modify them to update the message
---------------------------------------------------------------------------
by hason at 2012-02-09T08:55:55Z
I prefer second option. The Validator component should be decoupled from the Translation component. The constraints contain the en message which is also the key for Translation component. We should create validators.en.xlf in the FrameworkBundle for en message. I think that this is better solution. What do you think?
---------------------------------------------------------------------------
by stof at 2012-04-04T02:22:02Z
@hason Please rebase your branch. It conflicts with master because of the move of the tests
@fabpot ping
Commits
-------
c4e68a3 [Form] Moved logic of addXxx()/removeXxx() methods to the PropertyPath class
Discussion
----------
[Form] Moved logic of addXxx()/removeXxx() methods to the PropertyPath class
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #3732
Todo: -
The addXxx()/removeXxx() methods should now be called correctly in ChoiceType and CollectionType.
PropertyPath now favors addXxx()/removeXxx() over setXxx() for collections. For example:
```
$propertyPath = new PropertyPath('article.tags');
// Tries to use addTag()/removeTag() and only uses setTags() (et al.)
// if not found
$propertyPath->setValue($article, $tags);
```
For other languages than English or very irregular plurals, a custom singular can be set by separating it with a pipe:
```
$propertyPath = new PropertyPath('article.genera|genus');
```
---------------------------------------------------------------------------
by bschussek at 2012-04-07T12:40:39Z
Again, the failing build is not my fault.
Commits
-------
61d792e [Form] Changed checkboxes in an expanded multiple-choice field to not include the choice index
bc9bc4a [Form] Fixed behavior of expanded multiple-choice field when submitted without ticks
2e07256 [Form] Simplified choice list API
2645120 [Form] Fixed handling of expanded choice lists, checkboxes and radio buttons with empty values ("")
Discussion
----------
[Form] Fixed handling of empty values in checkbox/radio/choice type
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #3839, #3366
Todo: -
This PR fixes the processing of checkboxes and radio buttons with empty "value" attributes as well as of expanded choice forms with empty values. Additionally, some unnecessary complexity has been removed of the new ChoiceList API.
---------------------------------------------------------------------------
by stof at 2012-04-10T13:56:12Z
You probably need to change some things in the CHANGELOG file too
---------------------------------------------------------------------------
by bschussek at 2012-04-10T14:39:24Z
No. This is an update of a previous post-2.0 PR, the CHANGELOG is still accurate.
---------------------------------------------------------------------------
by stof at 2012-04-10T14:46:47Z
well, doesn't it require changes to the description of previous changes related to the ChoiceList ? It does in the UPGRADE file so it is weird if the CHANGELOG does not require any change.
---------------------------------------------------------------------------
by bschussek at 2012-04-10T14:56:05Z
Feel free to check yourself :)
Setting a property path like "article.tags" will now automatically try to
favor addTag() and removeTag() over setTags(), if found. If you want to
set up a property path with an irregular singular that is not detected,
you can use "|" to separate the plural from the singular form in the
path: "article.genera|genus".
Another consequence of this commit is that the MergeCollectionListener has
been simplified a lot. Forms returning an array or a collection will
always result in adders/removers being called now without having to add
this listener.
Commits
-------
e6577de Added a 'post validation' event to the form component.
Discussion
----------
[Form] Add post-validate event
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: n/a
Fixes the following tickets: n/a
Todo: n/a
---------------------------------------------------------------------------
by fabpot at 2012-03-02T20:34:18Z
ping @bschussek
---------------------------------------------------------------------------
by vicb at 2012-03-04T09:19:53Z
I think this is a good idea (It was something missing to properly handle PersistentFile i.e. you should not persist invalid files)
---------------------------------------------------------------------------
by vicb at 2012-03-09T22:35:26Z
@jankramer please remove the second commit from this PR (see http://symfony.com/doc/current/contributing/code/patches.html) in order to make this mergeable.
---------------------------------------------------------------------------
by jankramer at 2012-03-10T09:26:04Z
@vicb done, sorry about that commit: overlooked the fact that it was on the same branch...
Commits
-------
cde34fd [Form] Throwing an AlreadyBoundException in `add`, `remove`, `setParent`, `bind` and `setData` if called on a bound form
Discussion
----------
[Form] Throwing an AlreadyBoundException in `add`, `remove`, `setParent`, `bind` and `setData` if called on a bound form
Bug fix: yes
Feature addition: no
Backwards compatibility break: **yes**
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
The above mentioned methods now throw an exception because when invoked on a bound form they might cause strange side effects. You should rely on event listeners instead of modifying bound forms.
See also #3022
Commits
-------
cb6fdb1 [HttpFoundation] removed Session::close()
c59d880 Docblocks.
b8df162 Correct instanceof condition.
8a01dd5 renamed getFlashes() to getFlashBag() to avoid clashes
282d3ae updated CHANGELOG for 2.1
0f6c50a [HttpFoundation] added some method for a better BC
146a502 [FrameworkBundle] added some service aliases to avoid some BC breaks
93d81a1 [HttpFoundation] removed configuration for session storages in session.xml as we cannot provide a way to configure them (like before this PR anyway)
74ccf70 reverted 5b7ef11650 (Simplify session storage class names now we have a separate namespace for sessions)
91f4f8a [HttpFoundation] changed default flash bag to auto-expires to keep BC
0494250 removed unused use statements
7878a0a [HttpFoundation] renamed pop() to all() and getAll() to all()
0d2745f [HttpFoundation] Remove constants from FlashBagInterface
dad60ef [HttpFoundation] Add back get defaults and small clean-up.
5b7ef11 [HttpFoundation] Simplify session storage class names now we have a separate namespace for sessions.
27530cb [HttpFoundation] Moved session related classes to own sub-namespace.
4683915 [HttpFoundation] Free bags from session storage and move classes to their own namespaces.
d64939a [DoctrineBridge] Refactored driver for changed interface.
f9951a3 Fixed formatting.
398acc9 [HttpFoundation] Reworked flashes to maintain same behaviour as in Symfony 2.0
f98f9ae [HttpFoundation] Refactor for DRY code.
9dd4dbe Documentation, changelogs and coding standards.
1ed6ee3 [DoctribeBridge][SecurityBundle][WebProfiler] Refactor code for HttpFoundation changes.
7aaf024 [FrameworkBundle] Refactored code for changes to HttpFoundation component.
669bc96 [HttpFoundation] Added pure Memcache, Memcached and Null storage drivers.
e185c8d [HttpFoundation] Refactored component for session workflow.
85b5c43 [HttpFoundation] Added drivers for PHP native session save handlers, files, sqlite, memcache and memcached.
57ef984 [HttpFoundation] Added unit and functional testing session storage objects.
3a263dc [HttpFoundation] Introduced session storage base class and interfaces.
c969423 [HttpFoundation] Added FlashBagInterface and concrete implementation.
39288bc [HttpFoundation] Added AttributesInterface and AttributesBagInterface and concrete implementations.
Discussion
----------
[2.1][HttpFoundation] Refactor session handling and flash messages
Bug fix: yes
Feature addition: yes
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: #2607, #2591, #2717, #2773
References the following tickets: #2592, #2543, #2541, #2510, #2714, #2684
Todo: -
__Introduction__
This extensive PR is a refactor with minimal BC breaks of the `[HttpFoundation]` component's session management which fixes several issues in the current implementation. This PR includes all necessary changes to other bundles and components is documented in the `CHANGELOG-2.1` and `UPGRADING-2.1`.
__Summary of Changes__
__Session:__
- Session object now implements `SessionInterface`
__Attributes:__
- Attributes now handled by `AttributeBagInterface`
- Added two AttributeBag implementations: `AttributeBag` replicates the current Symfony2 attributes behaviour, and the second, `NamespacedAttributeBag` introduces structured namespaced representation using '/' in the key. Both are BC. `FrameworkBundle` defaults to the old behaviour.
__Flash messages:__
- Flash messages now handled by `FlashBagInterface`
- Introduced `FlashBag` which changes the way flash messages expire, they now expire on use rather than automatically, useful for ESI.
- Introduced `AutoExpireFlashBag` (default) which replicates the old automatic expiry behaviour of flash messages.
__Session Storage:__
- Introduced a base object, `AbstractSessionStorage` for session storage drivers
- Introduced a `SessionSaveHandlerInterface` when using custom session save handlers
- Introduced a `NullSessionStorage` driver which allows for unsaved sessions
- Introduced new session storage drivers for Memcache and Memcached
- Introduced new session storage drivers for PHP's native SQLite, Memcache and Memcached support
__General:__
- Fixed bugs where session attributes are not saved and all cases where flash messages would get lost
- Wrote new tests, refactored related existing tests and increased test coverage extensively.
__Rationale/Details__
I'll explain more detail in the following sections.
__Unit Tests__
All unit and functional tests pass.
__Note on Functional Testing__
I've introduced `MockFileSessionStorage` which replaces `FilesystemSessionStorage` to emulate a PHP session for functional testing. Essentially the same methodology of functional testing has been maintained but without interrupting the other session storage drivers interaction with real PHP sessions. The service is now called `session.storage.mock_file`.
__Session Workflow__
PHP sessions follow a specific workflow which is not being followed by the current session management implementation and is responsible for some unpredictable bugs and behaviours.
Basically, PHP session workflow is as follows: `open`, `read`, `write`, `close`. In between these can occur, `destroy` and `garbage collection`. These actions are handled by `session save handlers` and one is always registered in all cases. By default, the `files` save handler (internally to PHP) is registered by PHP at the start of code execution.
PHP offers the possibility to change the save handler to another internal type, for example one provided by a PHP extension (think SQLite, Memcache etc), or you can register a user type and set your own handlers. However __in all cases__ PHP requires the handlers.
The handlers are called when the following things occur:
- `open` and `read` when `session_start()` or the session autostarts when PHP outputs some display
- `destroy` when `session_regenerate_id(true)` is called
- `write` and `close` when PHP shuts down or when `session_write_close()` is called
- `garbage collection` is called randomly according to configurable probability
The next very important aspect of this PR is that `$_SESSION` plays an important part in this workflow because the contents of the $_SESSION is populated __after__ the `read` handler returns the previously saved serialised session data. The `write` handler is sent the serialised `$_SESSION` contents for save. Please note the serialisation is a different format to `serialize()`.
For this reason, any session implementation cannot get rid of using `$_SESSION`.
I wrote more details on this issue [here](https://github.com/symfony/symfony/issues/2607#issuecomment-2858300)
In order to make writing session storage drivers simple, I created a light base class `AbstractSessionStorage` and the `SessionSaveHandlerInterface` which allows you to quickly write native and custom save handler drivers.
__Flash Messages [BC BREAK]__
Flash messages currently allow representation of a single message per `$name`. Fabien designed the original system so that `$name` was equivalent to flash message type. The current PR changes the fact that Flash messages expire explicitly when retrieved for display to the user as opposed to immediately on the next page load.
The last issue fixes potential cases when flash messages are lost due to an unexpected intervening page-load (an error for example). The API `get()` has a flag which allows you to override the `clear()` action.
__Flash message translation__
This PR does not cover translation of flash messages because messages should be translated before calling the flash message API. This is because flash messages are used to present messages to the user after a specific action, and in any case, immediately on the next page load. Since we know the locale of the request in every case we can translate the message before storing. Secondly, translation is simply a string manipulation. Translation API calls should always have the raw untranslated string present because it allows for extraction of translation catalogs. For a complete answer see my answer [here](https://github.com/symfony/symfony/pull/2543#issuecomment-2858707)
__Session attribute and structured namespacing__
__This has been implemented without changing the current default behaviour__ but details are below for the alternative:
Attributes are currently stored in a flat array which limits the potential of session attributes:
Here are some examples to see why this 'structured namespace' methodology is extremely convenient over using a flat system. Let's look at an example with csrf tokens. Let's say we have multiple csrftokens stored by form ID (allowing multiple forms on the page and tabbed browsing).
If we're using a flat system, you might have
'tokens' => array('a' => 'a6c1e0b6',
'b' => 'f4a7b1f3')
With a flat system when you get the key `tokens`, you will get back an array, so now you have to analyse the array. So if you simply want to add another token, you have to follow three steps: get the session attribute `tokens`, have to add to the array, and lastly set the entire array back to the session.
$tokens = $session->get('tokens');
$tokens['c'] = $value;
$session->set('tokens', $tokens);
Doable, but you can see it's pretty long winded.
With structured namespacing you can simply do:
$session->set('c', $value, '/tokens');
There are several ways to implement this, either with an additional `$namespace` argument, or by treating a character in the `$key` as a namespacer. `NamespacedAttributeBag` treats `/` as a namespacer so you can represent `user.tokens/a` for example. The namespace character is configurable in `NamespacedAttributeBag`.
---------------------------------------------------------------------------
by marijn at 2011-12-18T15:43:17Z
I haven't read the code yet but the description from this PR and your line of thought seem very well structured.
Seems like a big +1 for me.
---------------------------------------------------------------------------
by lsmith77 at 2011-12-19T16:01:19Z
@deviantintegral could you look over this to see if it really addresses everything you wanted with PR #2510 ?
---------------------------------------------------------------------------
by deviantintegral at 2011-12-24T20:12:03Z
I've read through the documentation and upgrade notes, and I can't see anything that's obviously missing from #2510. Being able to support multiple flashes per type is the most important, and the API looks reasonable to me. Drupal does support supressing repeat messages, but that can easily be implemented in our code unless there's a compelling case for it to be a part of Symfony.
I wonder if PHP memcache support is required in Symfony given the availability of memcached. I'm not familiar with how other parts of Symfony handle it, but there is often quite a bit of confusion between the two PHP extensions. It could be simpler to remove one, or add a bit of info describing or linking to why there are two nearly identical classes.
Is it possible to make one class inherit from the other (memcached is a child of memcache)?
---------------------------------------------------------------------------
by Fristi at 2011-12-24T20:29:46Z
Interesting, maybe add: session events as I did with the current impl: https://github.com/Fristi/SessionBundle
---------------------------------------------------------------------------
by drak at 2011-12-25T00:50:03Z
@deviantintegral - I agree about the confusion between memcache and memcached but actually, it is necessary to support both because `memcached` is not available everywhere. For example on Debian Lenny and RHEL/CentOS 5, only memcache is available by default. This would preclude a massive amount of shared hosting environments. Also, it is not possible to inherit one from the other, they are completely different drivers.
@Fristi - I also thought about the events, but they do not belong as part of the standalone component as this would create a coupling to the event dispatcher. The way you have done it, ie, in a bundle is the right way to achieve it.
---------------------------------------------------------------------------
by matheo at 2011-12-25T01:12:00Z
Impressive work, looks like a big improvement and deserves a big +1
---------------------------------------------------------------------------
by datiecher at 2011-12-26T11:57:12Z
Took some time to grok all the changes in this PR but all in all it is a keeper. Specially the new flash message API, it's really nicer to work with it then the previous one.
Nicely done @drak!
---------------------------------------------------------------------------
by lsmith77 at 2012-01-02T15:00:00Z
@fabpot did you have time to review this yet? with all the work @drak has done its important that he gets some feedback soon. its clear this PR breaks BC in ways we never wanted to allow. but i think this PR also clearly explains why its necessary none the less.
---------------------------------------------------------------------------
by drak at 2012-01-02T15:41:53Z
@fabpot - I have removed the WIP status from this PR now and rebased against the current master branch.
---------------------------------------------------------------------------
by Tobion at 2012-01-07T07:13:38Z
From what I read from the IRC chat logs, the main concern of @fabpot is whether we really need multiple flash messages per type. I'm in favor of this PR and just want to add one point to this discussion.
At the moment you can add multiple flash messages of different type/category/identifier. For example you can specify one error message and one info message after an operation. I think most agree that this can be usefull.
But then it makes semantically no sense that you currently cannot add 2 info messages. This approach feels a bit half-done.
So I think this PR eliminates this paradox.
---------------------------------------------------------------------------
by drak at 2012-01-07T09:11:07Z
For reference there is also a discussion started by @lsmith77 on the mailing list at https://groups.google.com/forum/#!topic/symfony-devs/cy4wokD0mQI
---------------------------------------------------------------------------
by dlsniper at 2012-01-07T16:02:15Z
@drak I could also add the next scenario that I currently have to live with, in addition to @lsmith77 ones.
I had this issue while working on our shopping cart implementation for a customer where the customer wanted to show the unavailability of the items as different lines in the 'flash-error' section of the cart. We had to set an array as the 'flash message' in order to display that information.
So in this case for example having the flash messages types as array would actually make more sense that sending an array to the flasher. Plus the the other issue we had was that we also wanted to add another error in the message but we had to do a check to see if the flash message is an array already or we need to make it an array.
I think it's better not to impose a limit of this sort and let the users be able to handle every scenario, even if some are rare, rather that forcing users to overcome limitations such as these.
I really hope this PR gets approved faster and thanks everyone for their hard work :)
---------------------------------------------------------------------------
by Tobion at 2012-01-07T21:01:07Z
@dlsniper I think you misinterpreted my point.
---------------------------------------------------------------------------
by dlsniper at 2012-01-07T21:04:04Z
@Tobion I'm sorry I did that, I'll edit the message asap. Seems no sleep in 26 hours can cause brain not to function as intended :)
---------------------------------------------------------------------------
by lsmith77 at 2012-02-01T14:38:52Z
FYI the drupal guys are liking this PR (including the flash changes):
http://drupal.org/node/335411
---------------------------------------------------------------------------
by drak at 2012-02-01T14:51:33Z
@lsmith77 Fabien asked me to remove the changes to the flash messages so that they are as before - i.e. only one flash per name/type /cc @fabpot
---------------------------------------------------------------------------
by fabpot at 2012-02-01T14:58:23Z
To be clear, I've asked to split this PR in two parts:
* one about the session refactoring (which is non-controversial and should be merged ASAP)
* this one with only the flash refactoring
---------------------------------------------------------------------------
by drak at 2012-02-02T11:29:26Z
@fabpot this is ready to be merged now. I will open a separate PR later today for the flash messages as a bucket.
---------------------------------------------------------------------------
by fabpot at 2012-02-02T11:34:39Z
I must have missed something, but I still see a lot of changes related to the flash messages.
---------------------------------------------------------------------------
by drak at 2012-02-02T11:39:10Z
When I spoke to you you said you wanted to make the commit with flash messages with one message per name/type rather than multiple. The old flash messages behaviour is 100% maintained in `AutoExpireFlashBag` which can be the default in the framework if you wish. The `FlashBag` implementation makes Symfony2 ESI compatible.
---------------------------------------------------------------------------
by stof at 2012-02-02T11:47:38Z
@drak splitting into 2 PRs means you should not refactor the flash messages in this one but in the dedicated one.
---------------------------------------------------------------------------
by drak at 2012-02-02T12:29:43Z
@stof Yes. I discussed with Fabien over chat there are basically no changes
to flashes in `FlashBag` and `AutoExpireFlashBag` maintains the exact
behaviour as before. The FlashBag just introduces ESI compatible flashes.
There is no way to refactor the sessions without moving the flash messages
to their own bag. The next PR will propose the changes to flashes that
allow multiple messages per name/type. I can size the PR down a little
more removing the new storage drivers and so on to make the PR smaller but
that's really as far as I can go. To be clear, while the API has changed a
little for flashes, the behaviour is the same.
Commits
-------
22c8f80 [Form] Fixed issues mentioned in the PR comments
3b1b570 [Form] Fixed: The "date", "time" and "datetime" types can be initialized with \DateTime objects
88ef52d [Form] Improved FormType::getDefaultOptions() to see default options defined in parent types
b9facfc [Form] Removed undefined variables in exception constructor
Discussion
----------
[Form] Fixed: "date", "time" and "datetime" fields can be initialized with \DateTime objects
Bug fix: yes
Feature addition: yes
Backwards compatibility break: **yes**
Symfony2 tests pass: yes
Fixes the following tickets: #3288
Todo: -
Fixed exception that was thrown when doing:
$builder->add('createdAt', 'date', array('data' => new \DateTime()));
On a side note, the options passed to `FieldType::getDefaultOptions` now always also contain the default options of any parent types. This is necessary if you want to be independent of how `getDefaultOptions` is implemented in the parent type and still rely on the already defined values.
As a result, `FieldType::getParent` doesn't see default options anymore. This shouldn't be a big problem, because this method only relies on options in few cases. If it does, options now need to be checked for existence with `isset` before being used (BC break).
---------------------------------------------------------------------------
by bschussek at 2012-02-09T16:14:46Z
@fabpot Ready to merge.
---------------------------------------------------------------------------
by bschussek at 2012-02-10T12:15:04Z
@fabpot Ready to merge
Commits
-------
da2447e [Form] Fixed MergeCollectionListener when used with a custom property path
b56502f0 [Form] Added getParent() to PropertyPath
7e5104e [Form] Fixed MergeCollectionListener for the case that the form's data is updated by the data mapper (as happening in CollectionType)
Discussion
----------
[Form] Fixed MergeCollectionListener for the case that the form's data is updated by the data mapper
Bug fix: yes
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #3293, #1499
Todo: -
This fixes CollectionType to properly use adders and removers. Apart from that, adders and removers now work with custom property paths. PropertyPath was extended for a method `getParent`.
---------------------------------------------------------------------------
by bschussek at 2012-02-10T07:42:13Z
@fabpot Ready to merge.
Commits
-------
6a45a41 [Form] Fixed Form::bindRequest() when used on a form without children
Discussion
----------
[Form] Fixed Form::bindRequest() when used on a form without children
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #2553
Todo: -
Commits
-------
b65a997 [Form] Added test ChoiceList::testGetChoicesForValuesCorrectOrderingOfResult for correct ordering check
a60daff [Form] Fixed incorrect sorting in ChoiceList::getChoicesForValues
Discussion
----------
[Form] Fixed incorrect sorting in ChoiceList::getChoicesForValues
ChoiceList::getChoicesForValues return collection with sorting, that not corresponding to the sorting for input keys
---------------------------------------------------------------------------
by bschussek at 2012-02-03T21:40:49Z
Can you adapt the test cases to fail for this case please? (probably by use of assertSame instead of assertEquals in one of the existing tests)
---------------------------------------------------------------------------
by grizlik at 2012-02-04T08:41:44Z
I need to create a new test or modify an existing one?
---------------------------------------------------------------------------
by bschussek at 2012-02-04T09:58:04Z
Something like this in ChoiceListTest:
public function testGetChoicesForValuesReverseOrderedValues()
{
$values = array('2', '1');
$this->assertSame(array($this->obj3, $this->obj2), $this->list->getChoicesForValues($values));
}
Commits
-------
8321593 [Form] DRYed ChoiceType
0753cee [Form] Fixed read_only attribute for expanded fields
Discussion
----------
[Form] Fixed read_only attribute for expanded fields
Expanded choice widgets lose the knowledge of read_only attribute value.
Fixes bug introduced by #3193
---------------------------------------------------------------------------
by helmer at 2012-02-02T16:24:50Z
Please hold before merging, @bschussek had some thoughts about my changes in ``ChoiceType``, waiting for feedback.
---------------------------------------------------------------------------
by bschussek at 2012-02-02T16:33:12Z
I'm fine with the refactoring then, but please split it into two commits at least. The changes in ChoiceType have nothing in common with the actual issue here.
---------------------------------------------------------------------------
by helmer at 2012-02-02T19:40:39Z
Tests added.
---------------------------------------------------------------------------
by bschussek at 2012-02-03T10:14:32Z
Great, thanks.
@fabpot 👍
Commits
-------
8714d79 [Form] Simplified code in MergeCollectionListener
8ab982a [Form] Fixed: Custom add and remove method are not invoked if disallowed
02f61ad [Form] Renamed choice and collection options "adder_prefix" and "remover_prefix" to "add_method" and "remove_method" and allowed to specify full method names
b393774 [Form] Used direct method access in MergeCollectionListener instead of Reflection to avoid problems when using class hierarchies
d208f4e [Form] Made it possible to use models with only either addXxx() or removeXxx()
Discussion
----------
[Form] Fixed edge cases in MergeCollectionListener
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
Fixes an issue mentioned in the comments of #3239
see https://github.com/symfony/symfony/pull/3239#issuecomment-3776312
---------------------------------------------------------------------------
by bschussek at 2012-02-02T12:12:17Z
Wait a minute before merging this.
---------------------------------------------------------------------------
by bschussek at 2012-02-02T13:01:55Z
@fabpot Ready to merge
Commits
-------
9b0245b [Form] Made prefix of adder and remover method configurable. Adders and removers are not called if "by_reference" is disabled.
49d1464 [Form] Implemented MergeCollectionListener which calls addXxx() and removeXxx() in your model if found
7837f50 [Form] Added FormUtil::singularify()
Discussion
----------
[Form] Forms now call addXxx() and removeXxx() in your model
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #1540
Todo: adapt documentation
Adds functionality for calling `addXxx` and `removeXxx` method in your model. All types returning a collection of values are affected: "collection", "choice" (with multiple selection) and "entity" (with multiple selection).
Example:
class Article
{
public function addTag($tag) { ... }
public function removeTag($tag) { ... }
public function getTags($tag) { ... }
}
And the controller:
$form = $this->createFormBuilder($article)
->add('tags')
->getForm();
Upon modifying the form, addTag() and removeTag() are now called appropiately.
---------------------------------------------------------------------------
by stof at 2012-02-01T18:23:49Z
Really great
---------------------------------------------------------------------------
by vicb at 2012-02-01T18:24:04Z
Great !!
Two suggestions:
* make "add" and "remove" configurable,
* introduce a base class for the remove listeners with (final?) `::getSubscribedEvents()` and `::getEventPriorities()`
---------------------------------------------------------------------------
by haswalt at 2012-02-01T18:57:46Z
+1 this
---------------------------------------------------------------------------
by daFish at 2012-02-01T19:54:46Z
+1
---------------------------------------------------------------------------
by michelsalib at 2012-02-01T20:55:37Z
Can wait to have it!
It will save lots of time trying to solve WTF effects and making workarounds.
---------------------------------------------------------------------------
by bschussek at 2012-02-02T09:37:12Z
@vicb: Your first point is done. The second, I don't understand.
---------------------------------------------------------------------------
by stof at 2012-02-02T09:40:50Z
@bschussek your branch conflicts with master according to github
---------------------------------------------------------------------------
by vicb at 2012-02-02T09:52:40Z
@bschussek my point is that I can stand hard-coded priorities which are error prone. A better solution might be to introduce constants (in `FormEvents` / `FormEventPriorities` ?) with meaningful names.
---------------------------------------------------------------------------
by bschussek at 2012-02-02T10:21:52Z
@stof Rebased
@vicb I know, but who is responsible for managing priorities? There is no central entitty that can do this. (btw this is a general problem of the priority system of the EventDispatcher)
@fabpot Ready to merge.
---------------------------------------------------------------------------
by vicb at 2012-02-02T10:23:28Z
@bschussek doesn't each form has is own dispatcher so there is no need for a global registry here, something local to the form could be good enough.
The listener is used by the Collection type as well as the Choice and Entity type (with multiple
selection). The effect is that you can have for example this model:
class Article
{
public function addTag($tag) { ... }
public function removeTag($tag) { ... }
public function getTags($tag) { ... }
}
You can create a form for the article with a field "tags" of either type "collection" or "choice"
(or "entity"). The field will correctly use the three methods of the model for displaying and
editing tags.
Commits
-------
2e4ebe4 [Validator] Renamed methods addViolationAtRelativePath() and getAbsolutePropertyPath() in ExecutionContext
9153f0e [Validator] Deprecated ConstraintValidator methods setMessage(), getMessageTemplate() and getMessageParameters()
0417282 [Validator] Fixed typos
a30a679 [Validator] Made ExecutionContext immutable and introduced new class GlobalExecutionContext
fe85bbd [Validator] Simplified ExecutionContext::addViolation(), added ExecutionContext::addViolationAt()
f77fd41 [Form] Fixed typos
1fc615c Fixed string access by curly brace to bracket
a103c28 [Validator] The Collection constraint adds "missing" and "extra" errors to the individual fields now
f904a9e [Validator] Fixed: GraphWalker does not add constraint violation if error message is empty
1dd302c [Validator] Fixed ConstraintViolationList::__toString() to not include dots in the output if the root is empty
1678a3d [Validator] Fixed: Validator::validateValue() propagates empty validation root instead of the provided value
Discussion
----------
[Validator] Improved "missing" and "extra" errors of Collection constraint
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #2615
Todo: -
Instead of a single violation
Array:
The fields "foo", "bar" are missing
various violations are now generated.
Array[foo]:
This field is missing
Array[bar]:
This field is missing
Apart from that, the PR contains various minor fixes to the Validator.
---------------------------------------------------------------------------
by bschussek at 2012-02-02T09:14:52Z
@fabpot Ready for merge.
Commits
-------
7cecb4e [Form] added support for parent of FormBuilder
Discussion
----------
[Form] added support for parent of FormBuilder
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
In some cases of building forms, it is good to know the attributes of the parent builder. In my case I want to automaticaly add the subscriber to the fields, whose parent builder has a concrete attribute.
Replace #2882
Commits
-------
de253dd [Form] read_only and disabled attributes
Discussion
----------
[Form] read_only and disabled attributes (closes#1974)
1. Removed ``readOnly`` property from ``Form``, as it is no longer required
2. Introduced ``disabled`` property to ``Form``, behaves exactly like ``readOnly`` used to
3. Added ``disabled`` property to fields, defaults to ``false``, renders as ``disabled="disabled"``
4. A field with positive ``read_only`` property now renders as ``readonly="readonly"``
---------------------------------------------------------------------------
by helmer at 2012-01-26T17:46:17Z
I changed ``Form`` and ``FormBuilder`` property ``readOnly`` to ``disabled``. On second thought, this is perhaps not such good change - while readOnly somewhat implied the use-case, disabled no longer does.
Perhaps something else, like ``bindable`` (as not to confuse with read_only attribute of Fields)?
@bschussek, others, any thoughts?
---------------------------------------------------------------------------
by bschussek at 2012-01-31T06:53:59Z
Please prefix commits with the affected component, if applicable.
---------------------------------------------------------------------------
by helmer at 2012-01-31T08:41:03Z
@bschussek Prefixed. Please also see see to [this question](https://github.com/symfony/symfony/pull/3193#issuecomment-3673074)
Commits
-------
601d462 [Form] Added getValidators() to Form class
Discussion
----------
[Form] Added getValidators() to Form class
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
I am working implementing client side validation in a bundle that adds validation rules via a form builder. I am currently unable to pull the validators from the Form class which is making the implementation incredibly difficult.
I noticed that the transformers are currently exposed and based on some feedback I got on IRC I am guessing these weren't exposed simply because no one needed them at the time.
---------------------------------------------------------------------------
by mlively at 2011-12-28T20:54:41Z
Side note: It would be incredibly helpful to have this in the current branch of symfony. I implemented it on master because it is a new feature, but I would be more than happy to patch it into 2.0 if you would like.
---------------------------------------------------------------------------
by canni at 2012-01-11T10:15:46Z
👍
As this is new feature it cannot fit into 2.0 series, but 2.1 is just few clicks ahead, maybe this feature will fit into ;)
---------------------------------------------------------------------------
by bschussek at 2012-01-31T08:23:05Z
ping @fabpot
👍
---------------------------------------------------------------------------
by stof at 2012-01-31T08:51:26Z
@mlively could you rebase your branch ? it conflicts with the current master
---------------------------------------------------------------------------
by mlively at 2012-01-31T21:38:39Z
Yes, I can do that might be later today.
A new ExecutionContext is now created everytime that GraphWalker::walkConstraint() is
launched. Because of this, a validator B launched from within a validator A can't break
A anymore by changing the context.
Because we have a new ExecutionContext for every constraint validation, there is no point
in modifying its state anymore. Because of this it is now immutable.
Commits
-------
57cc531 [Form] Improved PHPDocs of choice lists
9e7e2af [Form] Fixed PHPDoc: Used {@inheritdoc} where applicable
2c530cc Fixed typos in UPGRADE file
7899bea Added examples to UPGRADE
d346ae6 Improved choice list sections of UPGRADE and CHANGELOG
a676598 [Form] Added class LazyChoiceList
Discussion
----------
[Form] Added LazyChoiceList
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
Adds a ChoiceList implementation that satisfies people who formerly extended ArrayChoiceList and loaded choices lazily in its `load` method.
---------------------------------------------------------------------------
by craue at 2012-01-30T12:56:49Z
👍
---------------------------------------------------------------------------
by craue at 2012-01-30T14:55:38Z
Not sure if it's a bug in this PR or in #3156, but the labels get replaced by their keys:
```php
<?php
use Symfony\Component\Form\Extension\Core\ChoiceList\ChoiceList;
use Symfony\Component\Form\Extension\Core\ChoiceList\LazyChoiceList;
use Symfony\Component\Form\Extension\Core\ChoiceList\SimpleChoiceList;
class MyChoiceList extends LazyChoiceList {
protected function loadChoiceList() {
$choices = array(
'bla' => 'blaaaaaahhhh',
);
return new SimpleChoiceList($choices, array(), ChoiceList::COPY_CHOICE, ChoiceList::COPY_CHOICE);
}
public function getChoices() {
$choices = parent::getChoices();
// $choices is array('bla' => 'bla')
return $choices;
}
}
```
If it's not this PR, I can of course open a new ticket for that. But I'm only working with `LazyChoiceList`s.
---------------------------------------------------------------------------
by stof at 2012-01-30T16:07:41Z
@craue the ``SimpleChoiceList`` is an implementation using the same string as label and value. If you need different ones, you need to use the ``ChoiceList`` implementation.
---------------------------------------------------------------------------
by craue at 2012-01-30T16:22:06Z
@stof: That would make `SimpleChoiceList` useless for almost any case. Are you sure?
---------------------------------------------------------------------------
by craue at 2012-01-30T16:33:31Z
The bug even occurs when using
```
return new ChoiceList(array_keys($choices), array_values($choices), array(), ChoiceList::COPY_CHOICE, ChoiceList::COPY_CHOICE);
```
---------------------------------------------------------------------------
by stof at 2012-01-30T16:40:19Z
well, the SimpleChoiceList is for simple cases (thus its naming) where you want the same for the label and the values. And if you look at the class, you will see it extends the ChoiceList implementation which is the generic one.
---------------------------------------------------------------------------
by craue at 2012-01-30T16:53:36Z
For me, "simple" would mean that it just takes the array given, using keys as indices and values as labels. No fancy stuff messing around with anything. :D But, is there anything wrong in this code or in mine? @bschussek: Please enlighten me.
---------------------------------------------------------------------------
by bschussek at 2012-01-30T17:16:58Z
You are both wrong :) `getChoices` does not return the choices with their associated labels anymore. What you get now are the choice indices as array keys and the choice values as array values. How both are determined depends on the index and value generation strategy, which, in your case, are both COPY_CHOICE.
The difference between simple and complex choice lists is, that simple choice lists can only contain scalar values as choices, while other choice lists (such as ObjectChoiceList, EntityChoiceList) may contain objects as choices.
Choice labels are now stored in ChoiceView objects, which are returned by the various `get*Views` methods.
---------------------------------------------------------------------------
by craue at 2012-01-30T18:07:43Z
It's pretty annoying having provided an array with keys and values when initializing the `ChoiceList` but being unable to retrieve it again. Guess I just over-used or even abused those choice lists as kind of (not only form related) lookup tables.
---------------------------------------------------------------------------
by bschussek at 2012-01-30T19:27:21Z
@craue: What's your use case?
---------------------------------------------------------------------------
by craue at 2012-01-30T20:10:16Z
I just used choice lists extensively, even for not directly form-related stuff. In one case, I'm using two of them (which are also used individually) to build up a third one. That went well using the old `ArrayChoiceList`s and their `getChoices` method. Just thinking about creating another set of model classes which just contain my lists. So for only one select field in a form it'll take three classes then: (A) a list, (B) a choice list based on A, (C) a choice form type based on B. Oh well ... :D
---------------------------------------------------------------------------
by craue at 2012-01-30T21:31:32Z
Anyway, this PR for `LazyChoiceList` is great, so please merge it. ;)
---------------------------------------------------------------------------
by craue at 2012-01-31T14:00:46Z
@bschussek: Is it ready to be merged?
---------------------------------------------------------------------------
by bschussek at 2012-01-31T16:59:17Z
Yes
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.
The regexp in MoneyType doesn't work if currency format has no decimal
(like JPY) and doesn't work either if the currency symbol is unicode
This change fixes both issues and adds a unit test
ad (1): HTML4 "id" attributes are limited to strings starting with a letter and containing only letters, digits, underscores, hyphens, periods and colons.
ad (2): Property paths contain three special characters needed for correct parsing: left/right bracket and period.
The rules for form naming are:
* Names may start with a letter, a digit or an underscore. Leading digits or underscores will be stripped from the "id" attributes.
* Names must only contain letters, digits, underscores, hyphens and colons.
* Root forms may have an empty name.
Solves #1919 and #3021 on a wider scope.
Commits
-------
0b7e2e0 Support for DELETE method in forms
Discussion
----------
[Form] Support DELETE HTTP verb
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: none
Todo: -
As `Symfony\Component\HttpFoundation\Request` already support DELETE requests nicely by parsing the request for us, support for the HTTPs DELETE verb can be easily done.
---------------------------------------------------------------------------
by mvrhov at 2012-01-20T06:00:49Z
This is wrong. The body for DELETE method is supposed to be empty or if present ignored.
Also the DELETE is supposed to remove the resource identified by uri, so the same code as for GET should be executed.
---------------------------------------------------------------------------
by lstrojny at 2012-01-20T08:56:22Z
I don’t think that’s the case. The HTTP standard does not state explicitly that DELETE does not have a body. See this [StackOverflow thread](http://stackoverflow.com/questions/2539394/rest-http-delete-and-parameters)
Commits
-------
0513eb1 [Form] Pass translation domain to the sub-forms when choice list is expanded
Discussion
----------
[Form] Pass translation domain to the sub-forms when choice list is expanded
* Bug fix: yes
* Tests pass: yes
* Feature addition: no
* BC compatibility break: no
When you have a select list with ``translation_domain``, you loose translations by expanding the list.
---------------------------------------------------------------------------
by stof at 2012-01-21T14:55:31Z
👍
---------------------------------------------------------------------------
by fabpot at 2012-01-21T16:51:17Z
Why not doing that in the 2.0 branch instead?
---------------------------------------------------------------------------
by stof at 2012-01-21T17:26:32Z
@fabpot because the support of translation domains is a 2.1 feature
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
This is cleanup after enabling empty form names, now form with empty name
will not render the default `id="form"` container attribute.
Developers can extend/override this behaviour by standard form theming methods.
BC Break: no
Feature addition: yes
Symfony2 tests pass: yes
Fixes the following issues: #2790
Todo: need more testing
This PR enables usage of empty string as a form name (only at root level).
Commits
-------
49d2685 [Form] Add default validation to TextType field (and related)
Discussion
----------
[Form] Add default transformer to TextType field (and related)
Bug fix: yes&no (?)
Feature addition: yes (?)
BC break: no
Symfony2 tests pass: yes
Fixes the following tickets: #1962.
---------------------------------------------------------------------------
by stloyd at 2011/12/19 03:43:37 -0800
@fabpot ping ;-)
---------------------------------------------------------------------------
by fabpot at 2011/12/19 10:58:20 -0800
Is it really needed? I have a feeling that it enforces unneeded constraints, but I can be wrong of course.
---------------------------------------------------------------------------
by hlecorche at 2011/12/20 02:31:03 -0800
It's needed because with TextType field, and without the ValueToStringTransformer, the user data (when sending the form) can be an array !!!
For example:
- if there is a TextType field
- and if there is a MaxLengthValidator
- and if the user data (when sending the form) is an array
So the exception "Expected argument of type string, array given in src\Symfony\Component\Validator\Constraints\MaxLengthValidator.php at line 40" is thrown
Commits
-------
1e370d7 typo fix
93d8d44 added some more infos about Config
27efd59 added READMEs for the bridges
34fc866 cosmetic tweaks
d6af3f1 fixed README for Console
6a72b8c added basic README files for all components
Discussion
----------
added basic README files for all components and bridges
heavily based on http://fabien.potencier.org/article/49/what-is-symfony2 and the official Symfony2 documentation
---------------------------------------------------------------------------
by jmikola at 2011/11/03 13:36:07 -0700
Great work. For syntax highlighting on the PHP snippets, you could add "php" after the three backticks.
---------------------------------------------------------------------------
by lsmith77 at 2011/11/03 13:41:29 -0700
done
---------------------------------------------------------------------------
by stealth35 at 2011/11/03 13:49:31 -0700
Nice job, but you also need to add `<?php`
ex :
``` php
<?php
use Symfony\Component\DomCrawler\Crawler;
$crawler = new Crawler();
$crawler->addContent('<html><body><p>Hello World!</p></body></html>');
print $crawler->filter('body > p')->text();
```
---------------------------------------------------------------------------
by lsmith77 at 2011/11/03 13:56:57 -0700
done
---------------------------------------------------------------------------
by ericclemmons at 2011/11/03 19:57:57 -0700
@lsmith77 Well done! This makes consumption of individual components that much easier, *especially* now that `composer.json` files have been added.
---------------------------------------------------------------------------
by lsmith77 at 2011/11/04 01:18:23 -0700
ok .. fixed the issues you mentioned @fabpot
---------------------------------------------------------------------------
by lsmith77 at 2011/11/11 15:00:27 -0800
@fabpot anything else left? seems like an easy merge .. and imho there is considerable benefit for our efforts to spread the word about the components with this PR merged.
---------------------------------------------------------------------------
by drak at 2011/11/11 18:54:13 -0800
You know, it might be a nice idea to put a link to the documentation for each component if there is some at symfony.com
---------------------------------------------------------------------------
by lsmith77 at 2011/11/12 00:59:14 -0800
i did that in some. but i might have missed a few places.
On 12.11.2011, at 03:54, Drak <reply@reply.github.com> wrote:
> You know, it might be a nice idea to put a link to the documentation for each component if there is some at symfony.com
>
> ---
> Reply to this email directly or view it on GitHub:
> https://github.com/symfony/symfony/pull/2561#issuecomment-2715762
---------------------------------------------------------------------------
by breerly at 2011/11/21 10:28:36 -0800
Pretty excited with this.
---------------------------------------------------------------------------
by dbu at 2011/11/24 00:02:50 -0800
is there anything we can help with to make this ready to be merged?
---------------------------------------------------------------------------
by lsmith77 at 2011/12/18 02:39:23 -0800
@fabpot: seriously .. if you are not going to deliver something "better" and don't provide a reason what is wrong with this .. then its beyond frustrating. i obviously do not claim that these README's are perfect (and certainly still no replacement for proper documentation), but I do claim that in their current form they are a radical step forward to potential users of the Symfony2 components.
* 2.0:
[FrameworkBundle] Added functional tests.
[Form] Added missing use statements (closes#2880)
[Console] Improve input definition output for Boolean defaults
[SecurityBundle] Changed environment to something unique.
2879: missing space between catch and the brace
#2688: Entities are generated in wrong folder (doctrine:generate:entities Namespace)
[TwigBundle] Fix the exception message escaping
Commits
-------
9e6a10a [Form] Added FormError::getMessage() and use it in Form class
Discussion
----------
[Form] Added FormError::getMessage()
---------------------------------------------------------------------------
by ericclemmons at 2011/11/29 18:38:40 -0800
Should this go through the translator, similar to how `field_errors` renders error messages?
> https://github.com/symfony/symfony/blob/master/src/Symfony/Bridge/Twig/Resources/views/Form/form_div_layout.html.twig#L253
---------------------------------------------------------------------------
by stof at 2011/11/29 23:11:24 -0800
``getErrorsAsString`` is there for a debugging purpose so injecting the translator in the Form class just for it seems wrong. And the logic used here is exactly what the identity translator does.
Commits
-------
78e9b2f [Form] Fixed textarea_widget (W3C standards)
Discussion
----------
[Form] Fixed textarea_widget (W3C standards)
Textarea widget included the "pattern" attribute but is not valid by W3C standards.
(See PR 2666 - New PR because rebase inside the 2.0 branch)
---------------------------------------------------------------------------
by fabpot at 2011/11/18 09:01:41 -0800
@hlecorche: Thanks for your work on this issue. Can you update the unit tests to be sure that this case is covered? If you're not comfortable with this, just tell me and I will do it myself
---------------------------------------------------------------------------
by hlecorche at 2011/11/19 02:51:06 -0800
@fabpot: I did'nt commited because I am not sure. I changed the "tests/Symfony/Tests/Component/Form/AbstractLayoutTest.php" file :
public function testTextarea()
{
$form = $this->factory->createNamed('textarea', 'na&me', 'foo&bar', array(
'property_path' => 'name',
'pattern' => 'foo',
));
$this->assertWidgetMatchesXpath($form->createView(), array(),
'/textarea
[@name="na&me"]
[not(@pattern)]
[.="foo&bar"]
'
);
}
Is it correct?
Commits
-------
36cebf0 Fix infinite loop on circullar reference in form factory
Discussion
----------
[BugFix][Form]Throw exception on form name circulal ref
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Closes: #2673
When FormType method `getName()` returns the same value as `getParent()` we're asking about trouble, and land into infinite loop.
Commits
-------
5b30812 See this issue : https://github.com/symfony/symfony/issues/2433
Discussion
----------
See this issue : https://github.com/symfony/symfony/issues/2433
I changed the access speficiers to `protected`, which makes easier to extend this class if one needs to like I did.
---------------------------------------------------------------------------
by greg0ire at 2011/11/10 06:55:12 -0800
Precision on the problem I had : I wanted to use a `CollectionType` and display a collection element attribute as the label for this element. I had no choice but to extend `ResizeFormListener` and `CollectionType`.
Commits
-------
57e1aeb Fixed undefined index notice in readProperty() method (PropertyPath)
Discussion
----------
Fixed undefined index notice in readProperty() method (PropertyPath)
Hi,
For some reasons, I get `notice` errors on `readProperty()` with Propel:
Notice: Undefined index: 0 in /Users/william/projects/Propel/testProjects/symfony2/vendor/symfony/src/Symfony/Component/Form/Util/PropertyPath.php line 284
The `PropelObjectCollection` implements `ArrayAccess`, the `readProperty()` method does not check if the given `index` exists so the `notice` error is thrown. I suppose to check whether the index exists or not has to be added.
Regards,
William
---------------------------------------------------------------------------
by fabpot at 2011/09/27 23:42:07 -0700
The patch is probably not what we want to do. First, I suppose that you are not creating the propertyPath by hand. If that is the case, we need to understand why the property path does not exist. Then, even if we might want to check the existence of the index, if it does not exist, we should probably throw an exception instead of just ignoring the problem.
---------------------------------------------------------------------------
by willdurand at 2011/09/28 01:14:49 -0700
My bad. This is a Propel bug due to `ArrayObject`. It throws a notice error if the index is not found in `offsetGet()` which is wrong according to the `ArrayAccess` interface. If the index is not found, we have to return `null`.
@fabpot Are you agree with that (for the `null` value) ?
---------------------------------------------------------------------------
by fabpot at 2011/09/28 01:17:09 -0700
My point is that it should never happen under normal circumstances.
---------------------------------------------------------------------------
by willdurand at 2011/09/28 01:23:55 -0700
@fabpot Not sure to get it.
The fact is that it tries to get the value (`getValue()`) of a fresh object, just added to the `collection` when I'm submitting a form with a `CollectionType` and a new entry in it.
I mean it tries to get this new object (not yet persisted, not yet in the collection) in the collection (`getValue()` -> `readProperty()`) which implements `ArrayAccess` but this object cannot be in the collection at this time.
Am I wrong ?
And, without this notice error thrown by Propel, I probably never opened this issue...
---------------------------------------------------------------------------
by willdurand at 2011/09/29 06:40:34 -0700
@fabpot: you can try this example: http://www.propelorm.org/cookbook/symfony2/mastering-symfony2-forms-with-propel.html#manytomany_relations in order to make your own tests. Will it be enough?
As I said, it throws a weird notice for the reasons above.
---------------------------------------------------------------------------
by jaugustin at 2011/10/04 12:58:10 -0700
any news on this ?
@fabpot did you have time to look at the test case ?
---------------------------------------------------------------------------
by cedriclombardot at 2011/11/09 14:29:42 -0800
@fabpot: can we have news about this ?
Commits
-------
79ae3fc [Form] fixed radio and checkbox when data is not bool
Discussion
----------
[Form] fixed checkbox view
The checkbox view was being built based on app data, not client data. This fixes it.
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
---------------------------------------------------------------------------
by fabpot at 2011/11/16 13:31:09 -0800
`RadioType` suffers from the same problem, no?
---------------------------------------------------------------------------
by kriswallsmith at 2011/11/16 13:32:50 -0800
Yeah, I'll fix that too.
---------------------------------------------------------------------------
by kriswallsmith at 2011/11/16 13:43:29 -0800
Updated to include `RadioType`.
* 2.0:
[Form] fixed previous merge
[Form] simplified previous merge
Also identify FirePHP by the X-FirePHP-Version header
[TwigBundle] Extract output buffer cleaning to method
[TwigBundle] Do not clean output buffering below initial level
Fixed rendering of FileType (value is not a valid attribute for input[type=file])
Added tests for string fix in DateTimeToArrayTransformer (8351a11286).
Added check for array fields to be integers in reverseTransform method. This prevents checkdate from getting strings as arguments and throwing incorrect ErrorException when submitting form with malformed (string) data in, for example, Date field. #2609
[Translation] removed unneeded methods
[Translation] added detection for circular references when adding a fallback catalogue
[DomCrawler] trim URI in getURI
[Yaml][Tests] Fixed missing locale string for Windows platforms which caused test to fail
