Commits
-------
ed028d5 [WebProfilerBundle] Made is_ajax available to the view when rendering panels
Discussion
----------
[Profiler] Ajax
The first commit should be merged as `app` is not always accessible in the twig template due to the ways the templating system is used. Then there is currently no way to check if we are dealing with an ajax request in the view.
The second commit use ajax to load the panels. This should make the interface more responsive as you don't have to load the layout each time + the panels are cached. Loading via AJAX would also work if your panel does not extend the ajax layout (legacy support) - this would be less efficient though as you would load the layout and filter it out afterwards.
I am not sure if the second commit is worth merging, maybe it is useless ?
---------------------------------------------------------------------------
by stof at 2012-02-12T20:40:16Z
@vicb please rebase
---------------------------------------------------------------------------
by stof at 2012-02-13T17:48:48Z
@vicb just FYI, this conflicts with master so you will need to rebased it before it can be merged.
Otherwise, what are the remaining points ?
---------------------------------------------------------------------------
by vicb at 2012-02-13T17:57:27Z
I am still wondering if the second commit is a good idea or not ?
---------------------------------------------------------------------------
by vicb at 2012-02-13T18:28:17Z
@stof isn't the branch based on the latest master ?
---------------------------------------------------------------------------
by stof at 2012-02-13T19:32:52Z
Well, github tells me it cannot be merged automatically. so either there is a conflict, either their conflict detection failed last time you pushed.
---------------------------------------------------------------------------
by vicb at 2012-02-13T22:20:06Z
I did fail.
Should be ok now.
---------------------------------------------------------------------------
by fabpot at 2012-02-14T23:27:08Z
I'm -1 on the second commit.
---------------------------------------------------------------------------
by vicb at 2012-02-15T07:44:25Z
Thanks all for the feedback.
@fabpot ready !
---------------------------------------------------------------------------
by stof at 2012-02-15T07:46:53Z
@vicb not ready: you reverted all use of ``is_ajax`` in the templates (and you did not renamed it to the underscored name preferred by @fabpot)
---------------------------------------------------------------------------
by vicb at 2012-02-15T07:54:30Z
Well I did revert the use of "`isajax`" (prefer not to mix CS here, the scope of this PR is not to fix CS) because it is not used (this should be applied to the Doctrine profiler).
_What I mean is that `isajax` in all the Sf templates w/o the associated js is useless, basically all or nothing_
---------------------------------------------------------------------------
by vicb at 2012-02-15T08:26:41Z
btw @fabpot it makes me wonder if underscored variable names is a good idea, this will force us to mix (i.e. `is_ajax` vs `request.isxmlhttprequest`). What do you think ?
---------------------------------------------------------------------------
by fabpot at 2012-02-15T10:09:20Z
I still prefer `is_ajax` as it makes things more readable.
---------------------------------------------------------------------------
by vicb at 2012-02-15T10:16:13Z
At a larger scale how do fix the inconsistency described in my previous message ?
Options are:
* fix twig cs
* create twig cs specific to sf2
* don't fix (= keep & live with some inconsistency)
---------------------------------------------------------------------------
by stof at 2012-02-15T10:22:13Z
@vicb we also use underscores for variables used in the form themes. the official Twig CS are basically the one used by Sf2 in the form theme
---------------------------------------------------------------------------
by fabpot at 2012-02-15T10:24:46Z
I don't see any inconsistencies here. One a variable name and the other is a method call/property name. So, my vote is a don't fix.
---------------------------------------------------------------------------
by vicb at 2012-02-15T10:28:53Z
I agree but then we loose one advertised benefit a twig: _"Easy to learn: The syntax is easy to learn and has been optimized to allow web designers to get their job done fast without getting in their way"_.
The designers should now be aware of the underlying implementation (i.e. Am I dealing with a variable or a function ?)
Edit: race condition here... I agree with @stof
---------------------------------------------------------------------------
by stof at 2012-02-15T10:45:49Z
@vicb they see that ``isXmlHttpRequest`` is not a variable. They are accessing it on the ``request`` variable (well, recurse here to reach the variable)
---------------------------------------------------------------------------
by fabpot at 2012-02-15T10:46:57Z
variables and functions are underscored.
---------------------------------------------------------------------------
by vicb at 2012-02-15T10:51:28Z
I think that the beauty of Twig comes from the fact that designers do not have to wonder if "something" is an array / an object / a variable / a method / a property.
But never mind, I'll update the PR.
---------------------------------------------------------------------------
by vicb at 2012-02-15T10:55:06Z
@fabpot would you mind if I open a PR against twig to check for existence of `collector::getNotCalledListeners()` when a designer writes `collector.not_called_listeners`, then we are all happy ?
---------------------------------------------------------------------------
by vicb at 2012-02-15T11:21:55Z
ready !
---------------------------------------------------------------------------
by fabpot at 2012-02-15T11:31:50Z
The problem is that the `Twig_Template::getAttribute()` is already the bottleneck
Commits
-------
b95284e [Profiler] Fix memcache(d)
Discussion
----------
[Profiler] Fix memcache(d) storages
This fixes an ambiguity...
The memcache(d) storages have a `$lifetime` option. The name indicates that we are talking about a ttl (in seconds). This is wrong is `$lifetime` > 2592000 (=30 days), see http://fr.php.net/manual/en/memcache.set.php.
Doctrine is also [affected](e9ab2d2cca).
The ambiguity also exists in the session storage but to a lesser extend as those storage directly use memcache(d) options rather than a `$lifetime`. @drak could you confirm ?
Hopefully the Cache Component will get it right (#3211).
Commits
-------
d077ede [HttpFoundation] Increase test coverage.
cbb3e69 [HttpFoundation] Increase test coverage.
Discussion
----------
[HttpFoundation] Increase session test coverage.
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
Commits
-------
c754f28 [DoctrineBridge] Rename data fixtures loader class
af84805 [DoctrineBridge] Suggest doctrine/data-fixtures dependency
e4243a1 [DoctrineBridge] Add common data fixtures loader
Discussion
----------
[DoctrineBridge] Add common data fixtures loader
Symfony does not depend on doctrine/data-fixtures, but having this class in the bridge would enable DoctrineMongoDBBundle (and possibly others) to load fixtures without requiring DoctrineFixturesBundle to be installed.
Additionally, DoctrineFixturesBundle seems to only consist of this class and a command for loading ORM fixtures. With this in the bridge, we can possibly eliminate DoctrineFixturesBundle altogether by merging its command into DoctrineBundle.
---------------------------------------------------------------------------
by stof at 2012-02-11T19:40:17Z
The reason to have a separate bundle for the ORM fixtures was that the ORM is released whereas the DataFixtures library is still in alpha versions. So we wanted to avoid having it in Symfony itself for the 2.0 release. It could maybe change now that we have the bundle in a separate repo.
The other solution could be to put all commands related to fixtures in DoctrineFixturesBundle but IIRC @beberlei rejected a PR trying to make the same command work for ORM and PHPCR.
@beberlei what do you think about these suggestions ? And what is missing in DataFixtures to release it ? It has not changed recently except for the addition of the typehint and an update of the PHPCR purger
---------------------------------------------------------------------------
by fabpot at 2012-02-14T23:30:23Z
The Symfony bridges provide integration between a third-party library and Symfony components. IIUC, this PR is only about Doctrine and as such it is not in the scope of the bridge. It should be done "somewhere" in the Doctrine namespace (what about common for instance?).
---------------------------------------------------------------------------
by stof at 2012-02-14T23:34:19Z
@fabpot no it is not a Doctrine-only code. This extended loader is about integrating the Doctrine DataFixtures library with the DI component to allow fixtures to be container-aware (it does absolutely nothing else fancy btw). So this *is* in the scope of the bridge.
---------------------------------------------------------------------------
by jmikola at 2012-02-15T00:40:12Z
I second @stof's point here. This class is specifically for loading fixtures into application with a service container. Likewise, that is why the base class it inherits is in the common data-fixtures library.
Since this is common to both ORM and ODM, the most logical home for it would be DoctrineCommonBundle, and I believe that's what the bridge is :)
---------------------------------------------------------------------------
by stof at 2012-02-15T01:53:17Z
@jmikola not even a DoctrimeCommonBundle IMO. This is not about integrating things with the fullstack framework but with one component
On the advice of @schmittjoh, this commit adds a LogoutException class for use by LogoutListener if the CSRF token is invalid.
The handling in the Security component's ExceptionListener is modeled after AccessDeniedException, which gets wrapped in an AccessDeniedHttpException in the absence of handler service or error page (I didn't think it was appropriate to re-use those for LogoutException).
Using "securitybundletest" as the default environment for the functional test's kernel causes a PHP fatal error redeclaring the class "appSecuritybundletestDebugProjectContainer" when multiple tests (with unique names) are executed. In lieu of forcing tests to specify their own environment explicitly, we can simply append the test name into the environment.
Note: this bug may be related to PHPUnit executing multiple tests within the same process.
As each firewall is configured, its logout listener (if any) will be registered with the LogoutUrlHelper service. In a template, this helper may be used to generate relative or absolute URL's to a particular firewall's logout path. A CSRF token will be appended to the URL as necessary.
The Twig extension composes the helper service to avoid code duplication (see: #2999).
This adds several new options to the logout listener, modeled after the form_login listener:
* csrf_parameter
* intention
* csrf_provider
The "csrf_parameter" and "intention" have default values if omitted. By default, "csrf_provider" is empty and CSRF validation is disabled in LogoutListener (preserving BC). If a service ID is given for "csrf_provider", CSRF validation will be enabled. Invalid tokens will result in an InvalidCsrfTokenException being thrown before any logout handlers are invoked.
Commits
-------
cea2c7e removed unneeded local variable
924f378 updated changelog
72d5805 changed route name
41cc0d6 [FrameworkBundle] added support for HInclude
Discussion
----------
[FrameworkBundle] added support for HInclude
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: discuss
Example: https://github.com/kbond/symfony-standard/tree/hinclude
**Reopened this as I broke #2903**
References:
- http://groups.google.com/group/symfony-devs/browse_thread/thread/b74e587d6f2f87b0
- http://groups.google.com/group/symfony-devs/browse_thread/thread/8776a9833d4a5f79
- #2903
- #2865
[](http://travis-ci.org/kbond/symfony)
---------------------------------------------------------------------------
by kbond at 2012-02-11T20:27:22Z
unless there is anything else I think this is ready, want me to squash again?
---------------------------------------------------------------------------
by fabpot at 2012-02-11T21:07:33Z
@kbond: Can you add some information about the changes in the CHANGELOG?
---------------------------------------------------------------------------
by Tobion at 2012-02-11T21:33:32Z
Do I see it correctly that we cannot set a default template on a per hinclude tag basis? But only global?
That's not really usefull when javascript is disabled because it should resemble the content to be included as an alternative.
---------------------------------------------------------------------------
by stof at 2012-02-11T21:42:15Z
@Tobion currently it is not possible. But changing the content on a tag basis may require changing the way the render tag look like (as there is no content in the tag currently) so this needs further discussion and @fabpot said he wants to merge a first implementation without it. See the discussion above.
Commits
-------
9d6eb82 [Routing] Fix a bug in the TraceableUrlMatcher
9fc8d28 [FrameworkBundle] Fix a bug in the RedirectableUrlMatcher
4fcf9ef [Routing] Small optimization in the UrlMatcher
abc2141 [Routing] Added a missing property declaration
d86e1eb [Routing] Remove a weird dependency
Discussion
----------
[Routing] Remove a dependency on a derived class, fixes, optim
Subset of #3296 which should be acceptable.
Travis is happy.
The side effect of removing the dependency is that the `UrlMatcher` does not throw an exception any more when the scheme does not match the required scheme. I think it is better because:
* it removes a dependency on a derived class,
* it was an undocumented "feature",
* other thrown excs are component specific while this one was raw SPL.
---------------------------------------------------------------------------
by vicb at 2012-02-09T14:43:02Z
let me know what should go in 2.0 as well.
Commits
-------
b3fd2fa [Propel] Added Propel to Stopwatch
Discussion
----------
[Propel] Added Propel to Stopwatch
I've added the Stopwatch feature, everything is ready on the PropelBundle.
The trick is to log `prepare` queries in Propel, that way we got first the prepared statement, and then the executed query. That's why there is a `$isPrepare` boolean.
I kept BC if people don't update the PropelBundle too.
William
---------------------------------------------------------------------------
by stof at 2012-02-14T12:16:51Z
@willdurand toggling a flag for each call seems a bit hackish to me. Is there no better way to do it ?
---------------------------------------------------------------------------
by willdurand at 2012-02-14T12:21:38Z
Unfortunately no... But it's quite safe as we cannot change logged methods.
There is neighter start/stop methods, nor typed messages.
Le 14 févr. 2012 à 13:16, Christophe Coevoet<reply@reply.github.com> a écrit :
> @willdurand toggling a flag for each call seems a bit hackish to me. Is there no better way to do it ?
>
> ---
> Reply to this email directly or view it on GitHub:
> https://github.com/symfony/symfony/pull/3352#issuecomment-3959592
---------------------------------------------------------------------------
by stof at 2012-02-14T12:26:04Z
@willdurand then let's use this for propel 1. But please improve the logging interface for Propel 2 :)
---------------------------------------------------------------------------
by willdurand at 2012-02-14T12:34:28Z
Sure! I've added that on my todolist…
2012/2/14 Christophe Coevoet <
reply@reply.github.com
>
> @willdurand then let's use this for propel 1. But please improve the
> logging interface for Propel 2 :)
>
> ---
> Reply to this email directly or view it on GitHub:
> https://github.com/symfony/symfony/pull/3352#issuecomment-3959729
>
Commits
-------
e5edf5a [Console] Fixed CS
8abf506 [Console] Added abbreviation into search for bad command / namespace
c6203bc [Console] Added namespace suggest on bad namespace name
117359a [Console] fixed CS according to PR comment
dd0d97e [Console] Added suggest on bad command name
Discussion
----------
[Console] Added suggest on bad command name
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: namespace ?
Added something like in `git` : if user type a wrong command and if a close alternative exists, Command compenent will display a list of similar command(s).
Note : It does not work with namespace. If this PR will be merged, I could work on namespace.
see : https://github.com/fabpot/Twig/blob/master/lib/Twig/Environment.php#L1003
---------------------------------------------------------------------------
by fabpot at 2012-02-11T18:54:49Z
I think we need it to also work on namespace before merging. Is it possible?
---------------------------------------------------------------------------
by henrikbjorn at 2012-02-11T19:01:06Z
could maybe use similar_text ?
---------------------------------------------------------------------------
by lyrixx at 2012-02-11T19:01:55Z
Yes.
I will work on it asap
---------------------------------------------------------------------------
by lyrixx at 2012-02-11T20:06:43Z
I added code for namespace
@henrikbjorn I did the same logic as in twig.
---------------------------------------------------------------------------
by lyrixx at 2012-02-11T20:27:48Z
Note : Travis tests failed : http://travis-ci.org/#!/lyrixx/symfony/builds/663216
```before_script: Execution of 'php vendors.php' took longer than 600 seconds and was terminated.
Consider rewriting your stuff in AssemblyScript, we've heard it handles Web Scale™```
But tests are OK on my laptop
---------------------------------------------------------------------------
by stof at 2012-02-11T20:41:15Z
Well, it may be due to github issues during the setup of the vendors. There is some issues regularly because of the DDoS attack.
---------------------------------------------------------------------------
by lyrixx at 2012-02-11T20:58:07Z
Yes, i guessed it :-) that's why i notice it work on my laptop
---------------------------------------------------------------------------
by fabpot at 2012-02-11T23:11:08Z
This code won't work if you use abbreviations instead of the full namespace or command name.
---------------------------------------------------------------------------
by lyrixx at 2012-02-12T23:30:04Z
I added code to manage abbreviations. But I'm not sure what you are expecting. Can you try it and give me some feedback ?
P.S. : Travis failed again, but tests pass on my laptop.
Commits
-------
8935dec Added support for SVG mime type
Discussion
----------
Added support for SVG mime type
Hi, MimeTypeExtensionGuesser doesn't have a default type for SVG files, I've added this in.
Craig
Commits
-------
beb4fc0 [WIP][Locale] StubIntlDateFormatter::parse was throwing exception instead of returning Boolean false like intl implementation
b61dff7 fixed CS
Discussion
----------
[WIP][Locale] StubIntlDateFormatter::parse was throwing exception instead of returning Boolean false like intl implementation
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: 
Fixes the following tickets: #2781
Todo: A test fail in 32 bit environment, executed tests only with PHP 5.3.2 and ext-intl ICU 4.2 based
Failed test:
1) Symfony\Tests\Component\Locale\Stub\StubIntlDateFormatterTest::testFormatWithDefaultTimezoneIntl
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'1969-12-31 21:00:00'
+'1969-12-31 16:00:00'
Commits
-------
7474293 memcache profiler storage support added
Discussion
----------
[HttpKernel] [FrameworkBundle] Memcache(d) Profiler Storage added
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
There are 2 memcache PHP extensions: Memcache and MemcacheD (with "D" at the end) - both are supported.
How to use Memcache Profiler Storage (Memcache php extension is used):
change (or add if there isn't) "dsn" in framework/profiler section in config_dev.yml
```
...
framework:
...
profiler:
...
dsn: memcache://127.0.0.1/11211
...
```
How to use Memcached Profiler Storage (MemcacheD php extension is used):
change "dsn" in framework/profiler section in config_dev.yml
```
...
framework:
...
profiler:
...
dsn: memcached://127.0.0.1/11211
...
```
Last changes:
- memcached support addedd
- optimized performance (serialization done in extension, index is created with ```append``` function)
- updated to last version of Profiler (find by method, avoid duplications)
- done squash on commits
---------------------------------------------------------------------------
by stloyd at 2011-12-01T23:36:02Z
You need to add check for index name size, AFAIK memcache will fail if key is longer than 250 characters.
Also please do an `squash` for all those commits.
---------------------------------------------------------------------------
by pulzarraider at 2011-12-02T00:15:28Z
@stloyd Thanks. I will add the check for key length.
I am just starting with git. Could you please add some tutorial about squash to a documentation page: http://symfony.com/doc/2.0/contributing/code/patches.html ? It will help me (and maybe some others) to do it correct way.
---------------------------------------------------------------------------
by stof at 2011-12-02T00:19:01Z
http://help.github.com/rebase/
---------------------------------------------------------------------------
by pulzarraider at 2011-12-03T18:56:11Z
Thanks @stof, rebase done.
---------------------------------------------------------------------------
by dlsniper at 2011-12-11T14:00:17Z
Hi,
Would it be possible to either use Memcached instead of Memcache or make it configurable to use either Memcache or Memcached?
I've did a little digging on the benefits of using Memcached over Memcache (like for example: http://stackoverflow.com/questions/1442411/using-memcache-vs-memcached-with-phphttp://devzone.zend.com/1869/zendcon-sessions-episode-040-memcached-the-better-memcache-interface/ ) and maybe this will also help in not having two extensions installed for people who are using Memcached already.
Regards.
---------------------------------------------------------------------------
by pulzarraider at 2011-12-11T16:15:58Z
@dlsniper thanks for great comment. I will add memcached support.
---------------------------------------------------------------------------
by stof at 2011-12-12T20:49:00Z
@pulzarraider what is the status of this PR ? Is it still a WIP ?
---------------------------------------------------------------------------
by pulzarraider at 2011-12-12T22:58:48Z
@stof Yes, it's still WIP. I'm working on a memcached (with D at the end) support. It will be finished in the next few days.
---------------------------------------------------------------------------
by dlsniper at 2011-12-15T12:51:52Z
@pulzarraider if I can help you with the PR let me know.
---------------------------------------------------------------------------
by pulzarraider at 2012-01-08T20:22:24Z
@dlsniper @stof I've finally added memcached support and done some optimizations. Memcache(d) profiler storage is now ready.
---------------------------------------------------------------------------
by dlsniper at 2012-01-08T22:12:29Z
I'm glad you finished this @pulzarraider
Thanks! for your hard work!
+1 for this PR
@stof, @fabpot is it good to go on master?
---------------------------------------------------------------------------
by pulzarraider at 2012-01-28T19:45:56Z
@stof, @fabpot ping
Commits
-------
3dd3d58 [EventListener] Fix an issue with sub-requests
71bf279 cleanup
acdb325 [StopWatch] Provide a cleaner API
acd1287 [Stopwatch] rename the section event to avoid collisions
eb540be [Profiler] Allow profiling the terminate event
4ccdc53 [HttpKernel] Cleanup of PdoProfilerStorage
814876f [HttpKernel] Tweak the code of the ProfilerListener
Discussion
----------
[Profiler] Allow profiling the terminate event
This PR is mainly about allowing to profile the terminate event (i.e. see it in the timeline panel)
There are some other tweaks.
---------------------------------------------------------------------------
by vicb at 2012-02-02T14:43:20Z
please don't merge for now. good question. bad answer.
---------------------------------------------------------------------------
by vicb at 2012-02-06T15:05:46Z
While first commits were focused on problem solving, the last brings a clean API with the ability to re-open an existing section in order to add events (re-setting event origins and merging them were just hacks).
Should be ready to be merged.
_Edit: Sorry, couldn't resist adding a private helper class again!_
---------------------------------------------------------------------------
by stof at 2012-02-06T18:30:09Z
@vicb you should stop adding such classes defined in the same file. Otherwise we will have to change the CS (and to stop telling we respect the PSR-0 standard)
---------------------------------------------------------------------------
by vicb at 2012-02-06T18:33:36Z
Once again PSR-0 is about autoloading which is exactly why I do not want in such cases. CS are an other matter and yes I think they should be changed to allow this (and I am going to submit a PR right now).
The only argument I could accept is whether this class should be private or not.
---------------------------------------------------------------------------
by vicb at 2012-02-06T19:57:06Z
Thanks for your valuable feedback @stof
---------------------------------------------------------------------------
by fabpot at 2012-02-11T20:53:03Z
Have you tested it on a project? Because it breaks my simple examples (where I have some sub-requests).
---------------------------------------------------------------------------
by vicb at 2012-02-12T09:47:23Z
my bad, should be ok now.
Commits
-------
6e9f886 [WebProfilerBundle] moved variable initialization from condition
Discussion
----------
[WebProfilerBundle] moved variable initialization from condition
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
I got fatal error for the original condition on PHP 5.3.8:
Fatal error: Call to a member function getFlashBag() on a non-object in /home/context/httpd/vendor/symfony/src/Symfony/Bundle/WebProfilerBundle/Controller/ProfilerController.php on line 150
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
-------
88b826d [Propel] Fixed typo, removed useless use statement, used getData() instead of casting a PropelCollection
46d28cd [Propel] Fixed the CollectionToArray transformer
1f20fb1 [Propel] Removed useless code
3910735 [Propel] Avoid to duplicate objects
d69144c [Propel] Refactored the CollectionToArray transformer
1706671 [Propel] Fixed naming to reflect Doctrine bridge
1f277df [Propel] Removed useless ModelToIdTransformer
Discussion
----------
Cleaned the propel bridge (+ fixes)
I've fixed the `ModelChoiceList` with `multiple=true`, and I removed useless code.
This PR will ensure everything works fine, but it requires the following fix for Propel: https://github.com/propelorm/Propel/pull/286.
---------------------------------------------------------------------------
by willdurand at 2012-02-11T20:04:10Z
@cedriclombardot this PR will fix your issues with Sf2 + Propel in your admingen
@bschussek nevermind my comments on Twitter, it seems ok now
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.
Rename ArraySessionStorage to make it clear the session is a mock for testing purposes only.
Has BC class for ArraySessionStorage
Added sanity check when starting the session.
Fixed typos and incorrect php extension test method
session_module_name() also sets session.save_handler, so must use extension_loaded() to check if module exist
or not.
Respect autostart settings.
Session object now implements SessionInterface to make it more portable.
AbstractSessionStorage and SessionSaveHandlerInterface now makes implementation
of session storage drivers simple and easy to write for both custom save handlers
and native php save handlers and respect the PHP session workflow.
This commit outsources the flash message processing to it's own interface.
Overall flash messages now can have multiple flash types and each type can
store multiple messages. For convenience there are now four flash types
by default, INFO, NOTICE, WARNING and ERROR.
There are two concrete implementations: one preserving the old behaviour of
flash messages expiring exactly after one page load, regardless of being
displayed or not; and the other where flash messages persist until explicitly
popped.
This commit outsources session attribute storage to it's own class.
There are two concrete implementations, one with structured namespace storage and the other
without.
Commits
-------
ac59db7 cleanup
64ea95d [WebProfilerBundle] Add redirection info to the router panel
826bd23 [FrameworkBundle] fix phpDoc of ControllerResolver::createController()
e3cf37f [HttpFoundation] RedirectResponse: add the ability to retrieve the target URL, add unit tests
50c85ae [WebProfiler] Add info to the router panel
Discussion
----------
[WIP][Profiler] Routing
former #3206 part 3 (depends on part 1 - #3280)
The goal of this PR is to fix#3264 by adding redirection infos on the router panel.
Done:
* Add info on the target url / route
To do:
* Display an accurate URL matching process (when using the RedirectableUrlMatcher)
Commits
-------
0d4d7e0 [WebProfilerBundle] Make the toolbar use the common JS
a440279 [WebProfilerBundle] Adds panel pages
762d90d [Profiler] Buid a common infrastructure
Discussion
----------
[Profiler] Provide a common infrastructure
former #3206 part 3
* base JS (provides ajax, toggle, css class helpers),
* panel pages (used only by the Doctrine panel for now).
Successfuly tested with the (future version of the) Doctrine panel.
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
-------
0a4519d [Form] Fixed duplicate errors on forms with "error_bubbling"=false
Discussion
----------
[Form] Fixed duplicate errors on forms with "error_bubbling"=false
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #2308
Todo: -
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
-------
411a0cc [Validator] Added GroupSequenceProvider to changelog
815c769 [Validator] Renamed getValidationGroups to getGroupSequence
d84a2e4 [Validator] Updated test expectations
9f2310b [Validator] Fixed typos, renamed hasGroupSequenceProvider
e0d2828 [Validator] GroupSequenceProvider tests improved, configuration changed
c3b04a3 [Validator] Changed GroupSequenceProvider implementation
6c4455f [Validator] Added GroupSequenceProvider
Discussion
----------
[Validator] Added GroupSequenceProvider
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: 
As discussed in #3114 I implemented the "GroupSequenceProvider" pattern for the validator component. It allows the user to select certain validation groups based on the current state of an object. Here is an example:
/**
* @Assert\GroupSequenceProvider("UserGroupSequnceProvider")
*/
class User
{
/**
* @Assert\NotBlank(groups={"Premium"})
*/
public function getAddress();
public function hasPremiumSubscription();
}
class UserGroupSequenceProvider implements GroupSequenceProviderInterface
{
public function getValidationGroups($user)
{
if ($user->hasPremiumSubscription()) {
return array('User', 'Premium');
} else {
return array('User');
}
}
}
With this patch there are two mechanisms to define the group sequence now. Either you can use @GroupSequence to define a static order of validation groups or you can use @GroupSequenceProvider to create dynamic validation group arrays.
The ClassMetadata therefore has methods now which implement quite similar things. The question is whether it would make sense to interpret the static group sequence as a special case and create something like a DefaultGroupSequenceProvider or StaticGroupSequenceProvider which is assigned by default. This would cause a BC break inside the validator component.
---------------------------------------------------------------------------
by bschussek at 2012-01-28T13:39:54Z
I like the implementation, but I think we should differ a little bit from Java here.
1. `GroupSequenceProviderInterface` should be implemented by the domain classes themselves (`User`), not by a separate class.
2. As such, the parameter `$object` from `getValidationGroups($object)` can be removed
3. `ClassMetadata::setGroupSequenceProvider()` should accept a boolean to activate/deactivate this functionality. Also the check for the interface (does the underlying class implement it?) should be done here
Apart from that, special cases need to be treated:
* A definition of a group sequence and a group sequence provider in the same `ClassMetadata` should not be allowed. Either of them must not be set.
* Metadata loaders must take care of settings made by parent classes. If `Animal` is extended by `Dog`, `Animal` defines a group sequence (or group sequence provider) and `Dog` a group sequence provider (or group sequence), only the setting of `Dog` should apply
---------------------------------------------------------------------------
by blogsh at 2012-01-28T21:25:37Z
Changes of the latest commit:
- GroupSequenceProviderInterface has to be implemented by the domain class
- The annotation/configuration options let the user define whether the provider is activated or not (is this neccessary at all?)
- An error is thrown if the user wants to use static group sequences and the provider simultaneously
At the moment neither the static group sequence nor the provider is inherited from parent classes or interfaces. I don't know if it would make sense to enable this feature. There could be problems if a user wants to define a static group sequence in the parent class and a sequence provider in the child class.
---------------------------------------------------------------------------
by bschussek at 2012-01-30T13:07:04Z
> There could be problems if a user wants to define a static group sequence in the parent class and a sequence provider in the child class.
In this case, the setting in the child class should override the setting of the parent class.
But we can leave this open for now. As it seems, [this issue is unresolved in Hibernate as well](https://hibernate.onjira.com/browse/HV-467).
---------------------------------------------------------------------------
by blogsh at 2012-01-30T22:54:41Z
Okay, finally I managed to upload the latest commit. If you got a bunch of notifications or so I'm sorry, but I had to revert some accidental changes in the commit :(
I've rewritten the tests and have removed the "active" setting in the XML configuration.
---------------------------------------------------------------------------
by blogsh at 2012-02-02T15:24:01Z
Okay, typos are fixed now and `hasGroupSequenceProvider` has been renamed to `isGroupSequenceProvider`. I also had to adjust some tests after the rebase with master.
---------------------------------------------------------------------------
by bschussek at 2012-02-03T09:25:19Z
Looks good.
@fabpot 👍
---------------------------------------------------------------------------
by fabpot at 2012-02-03T09:46:52Z
Can you add a note in the CHANGELOG before I merge? Thanks.
---------------------------------------------------------------------------
by blogsh at 2012-02-09T12:31:27Z
@fabpot done
Commits
-------
ba3b321 [ClassLoader] Added a class map file generator utility
Discussion
----------
[ClassLoader] Added a class map file generator utility
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #2407
---------------------------------------------------------------------------
by lsmith77 at 2011-11-30T19:33:52Z
@thesalla do you have time/interest to finish this up?
---------------------------------------------------------------------------
by thesalla at 2011-12-01T09:55:50Z
sure (will attempt at least), but no sooner than tomorrow
---------------------------------------------------------------------------
by stof at 2011-12-12T20:41:41Z
@thesalla @fabpot what is the state of this PR ?
---------------------------------------------------------------------------
by thesalla at 2011-12-17T19:36:58Z
If you don't have any other suggestions or ideas, I'd consider it finished.
---------------------------------------------------------------------------
by lsmith77 at 2012-01-24T09:27:12Z
ping
---------------------------------------------------------------------------
by fabpot at 2012-02-02T08:21:38Z
@thesalla: Can you rename the ClassMapDumper class to ClassMapGenerator and then squash your commit before the merge? Thanks.
---------------------------------------------------------------------------
by thesalla at 2012-02-04T12:55:48Z
@fabpot done.
Commits
-------
928e352 Change the array access used in UniqueEntityValidator
Discussion
----------
[DoctrineBridge] Change the array access used in UniqueEntityValidator
MongoDB ODM Cursor does not implement ArrayAccess and therefor using
`$result[0]` will fail. `reset()` rewinds the array and returns the
first element value.
DoctrineMongoDBBundle#77
/cc @beberlei
---------------------------------------------------------------------------
by henrikbjorn at 2012-02-06T08:09:25Z
Made a mistake, the findBy call would still return a cursor. Changed the findBy to findOneBy we only want one result anyway.
---------------------------------------------------------------------------
by stof at 2012-02-06T08:11:26Z
findOneBy is wrong: you will not detect duplicate anymore if you return a single element
---------------------------------------------------------------------------
by henrikbjorn at 2012-02-06T08:28:03Z
@stof before it was only the first result that was used anyway so if it had found 3 results it would only have used the first one. Performance is apparently the biggest issue with findOneBy so this have been reverted.
---------------------------------------------------------------------------
by stof at 2012-02-06T08:31:17Z
@henrikbjorn no, we use other results too: if ``count()`` is not 1, the validation fails
---------------------------------------------------------------------------
by stof at 2012-02-06T08:36:06Z
Btw, running the testsuite would have tell you there is an issue when using findOneBy as it was breaking a test :)
---------------------------------------------------------------------------
by bschussek at 2012-02-06T10:45:44Z
Why doesn't this validator use a SELECT COUNT anyway?
---------------------------------------------------------------------------
by stof at 2012-02-06T10:57:00Z
@bschussek because we need to check if an existing value is for the same object or another one. Otherwise, the valdiation would fail as soon as you are editing an existing object without changing the unique value
---------------------------------------------------------------------------
by henrikbjorn at 2012-02-06T11:03:40Z
@stof @bschussek other changes that should be done? Else it should be ready to be merged ?
---------------------------------------------------------------------------
by henrikbjorn at 2012-02-06T13:00:57Z
@stof done an rebased.
---------------------------------------------------------------------------
by bschussek at 2012-02-06T13:04:44Z
👍 /cc @fabpot
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
-------
78c1451 [Validator] Throwing exception in ConstraintValidator::setMessage() if initialize() wasn't called
Discussion
----------
[Validator] Throwing exception in ConstraintValidator::setMessage() if initialize() wasn't called
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
---------------------------------------------------------------------------
by Maks3w at 2012-02-05T09:52:21Z
One problem was that. Now I initialized the unit test with a execution
context mockup and works good.
Another problem I found is in the rules for upgrade from 2.0 to
2.1:
In v2.0: with setMessages() only one argument is mandatory ($message)
In v2.1: With the new way, addViolation(), then you need 2 arguments.
But, in v2.0 with the new way, addViolation, required 3 arguments.
So for preserve forward and backward compatibility, you need pass 3
arguments to addViolation() I think that it is helpfull and could be written in the
doc.
At the last one ¿What is the purpose of the third argument? ¿Can I pass
any object as invalid value?
Thanks
---------------------------------------------------------------------------
by bschussek at 2012-02-05T10:45:16Z
The last argument should always be the validated value that caused the constraint to fail.
Commits
-------
80682ba Fixed CS
007de8c [Tests] [Propel] Added some tests for the ModelChoiceList class
8257efd [Propel] Refactored the whole ModelChoiceList
Discussion
----------
Fix propel bridge
Ok, so I rewrote the `ModelChoiceList` based on the `EntityChoiceList`, and I provided the same test suite.
Sorry for the last PR, it was a fix to avoid fatal..
Cheers,
William
fixed cs
Small refactoring for Finder support
If class name found, return
Find multiple classes and namespaces in the same file
fixed problems with inheritance and non-php files
Renamed ClassMapDumper to ClassMapGenerator
fixed error with splfileinfo
Commits
-------
4847d3a renamed command
e97af0b code fixes
df94282 [FrameworkBundle] removed unnecessary DebugCommand
fa32885 [SecurityBundle] added configuration info
2f8ad93 [MonologBundle] added configuration info
9757958 [FrameworkBundle] added configuration info
58939f1 [TwigBundle] added configuration docs
8dc40e4 [FrameworkBundle] added config:dump console command
Discussion
----------
Config dump command
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #1663
Todo: add more config info/examples
[](http://travis-ci.org/kbond/symfony)
This is a config dump command based on the additions of PR #1099. This was initially part of that PR and there is some discussion there about it (https://github.com/symfony/symfony/pull/1099)
### Usage:
1. dump by root node: ``app/console config:dump framework``
2. dump by bundle name: ``app/console config:dump FrameworkBundle``
A few issues/notes:
* Only dumps to yaml
* Only 1 configuration per bundle (this was brought by @stof here: https://github.com/symfony/symfony/pull/1099#issuecomment-1242993)
* Works out of the box for most bundles but not ones that use a non-standard ``Configuration`` class (such as the assetic bundle). In this case ``Extension::getConfiguration()`` must be configurated.
I have used it to create some (most) of the config reference docs. It works fine but I find it somewhat crude, any suggestions to improve it would be appreciated.
---------------------------------------------------------------------------
by kbond at 2012-01-24T21:00:43Z
Few more issues:
1. Should I abstract the logic to a "normalizer" class that converts the Configuration class into a manageable array? I struggle with this idea because isn't that what ``TreeBuilder`` basically is?
2. @stof made a good point that ``config:dump`` doesn't really describe what this does. Would dumping your config be useful? Perhaps ``config:dump framework`` dumps the config for your project while ``config:dump --ref framework`` dumps the default reference?
---------------------------------------------------------------------------
by stof at 2012-01-24T21:18:15Z
@kbond you cannot really dump the config. Part of it does not go through these extensions at all. And it does not make much sense anyway IMO.
the command as is does the right job IMO (i.e. dumping a reference for the extension). But its name should be improved
---------------------------------------------------------------------------
by kbond at 2012-01-24T21:20:51Z
``config:reference`` perhaps?
---------------------------------------------------------------------------
by fabpot at 2012-02-02T10:05:19Z
This command is about displaying the default configuration for a given bundle. So, what about `config:dump-reference`? As I understand, the command name is the last element to figure out before merging, right?
---------------------------------------------------------------------------
by stof at 2012-02-02T10:19:49Z
@fabpot indeed.
---------------------------------------------------------------------------
by stof at 2012-02-02T10:34:16Z
and +1 for ``config:dump-reference``
---------------------------------------------------------------------------
by Tobion at 2012-02-02T12:08:03Z
why not use the words you chose yourself: `config:dump-default`
I think it's more explicit.
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.
