Commits
-------
2a90871 [Console] Removed previously introduced BC break.
90a2a6e [Console] Undecorated formatter must update style stack too.
bd7e01a [Console] Fixed output formatter test broken by new implementation.
a1add4b [Console] Updated output formatter to use style stack.
4f298dd [Console] Added formatter style stack.
93ffe54 [Console] Added getters to output formatter style (and its interface).
48e6b49 [Console] Updated formatter test to match styles bug fix.
ad334b6 [Console] Fixed empty style appliance.
31d5fe5 [Console] Fixed output formatter docblock.
Discussion
----------
[Console] Fixes formatter nested style appliance.
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
When outputing styled text in the console, you sometimes face to a confusing behavior: style tags cannot be nested. If tou try something like `<fg=blue>Hello <fg=red>world</fg=red>!</fg=blue>`, the trailing `!` will not be styled.
This PR introduce a new FormatterOutputStyleStack to keep open/closed styles informations up-to-date. It slightly changes OutputFormatter implementation which no longer uses `OutputFormatterStyle::apply()` method, but the new `OutputFormatterStyle::getTerminalSequence()`.
**Question:** I don't une `OutputFormatterStyleInterface` but `OutputFormatterStyle` to type `OutputFormatterStyleStack` methods arguments (to avoid BC break on the interface). Do you think it's right?
**Notice:** I also needed to fix some tests broken by new implementation.
---------------------------------------------------------------------------
by stof at 2012-03-16T10:27:56Z
Adding new methods in an interface is a BC break for people implementing it
---------------------------------------------------------------------------
by jfsimon at 2012-03-16T10:33:21Z
@stof indeed... this is a problem, should I remove them? If I do so, I should use `OutputFormatterStyle` instead of the interface to type arguments in `OutputFormatterStyleStack` right?
Commits
-------
24a0d0a [DependencyInjection] Support Yaml calls without arguments
Discussion
----------
[DependencyInjection] Support Yaml calls without arguments
Commits
-------
9ef5e95 Add connection name in the propel data collector
Discussion
----------
Add connection name in the propel data collector
Bug fix: no
Feature addition: yes, This will allow to explain a propel query on a specific connection
Backwards compatibility break: no
Symfony2 tests pass: yes
- Require PR propelorm/Propel#315
- Related to PR propelorm/PropelBundle#129
cc @willdurand
---------------------------------------------------------------------------
by willdurand at 2012-03-16T18:17:26Z
@fabpot please, let me merge Propel related PRs before that one, thanks!
---------------------------------------------------------------------------
by willdurand at 2012-03-26T08:38:36Z
@fabpot good to go from my point of view
Commits
-------
5ae76f1 [HttpFoundation] Update documentation.
910b5c7 [HttpFoudation] CS, more tests and some optimization.
b0466e8 [HttpFoundation] Refactored BC Session class methods.
84c2e3c [HttpFoundation] Allow flash messages to have multiple messages per type.
Discussion
----------
[2.1][HttpFoundation] Multiple session flash messages
Bug fix: no
Feature addition: yes
Backwards compatibility break: yes, but this already happened in #2583. BC `Session` methods remain unbroken.
Symfony2 tests pass: yes
Fixes the following tickets: #1863
References the following tickets: #2714, #2753, #2510, #2543, #2853
Todo: -
This PR alters flash messages so that it is possible to store more than one message per flash type using the `add()` method or by passing an array of messages to `set()`.
__NOTES ABOUT BC__
This PR maintains BC behaviour with the `Session` class in that the old Symfony 2.0 methods will continue to work as before.
---------------------------------------------------------------------------
by drak at 2012-02-13T06:28:33Z
I think this is ready for review @fabpot @lsmith77
---------------------------------------------------------------------------
by lsmith77 at 2012-02-14T19:30:39Z
the FlashBag vs. AutoExpireFlashBag behavior and setup difference should probably also be explained in the upgrading log
---------------------------------------------------------------------------
by drak at 2012-02-15T04:43:14Z
@lsmith77 Those differences are explained already in the changelog
* Added `FlashBag`. Flashes expire when retrieved by `get()` or `all()`.
This makes the implementation ESI compatible.
* Added `AutoExpireFlashBag` (default) to replicate Symfony 2.0.x auto expire behaviour of messages auto expiring
after one page page load. Messages must be retrived by `get()` or `all()`.
---------------------------------------------------------------------------
by Crell at 2012-02-19T17:35:34Z
Drak asked me to weigh in here with use cases. Drupal currently has a similar session-stored-messaging system in place that I'd like to be able to replace with Flash messages. We frequently have multiple messages within a single request, however, so this change is critical to our being able to do so.
For instance, when saving an article in Drupal there is, by default, a "yay, you saved an article!" type message that gets displayed. If you also have the site configured to send email when a post is updated, you may see a "email notifications sent" message (depending on your access level). If you have a Solr server setup for search, and you're in debug mode, there will also be a "record ID X added to Solr, it should update in 2 minutes" message. And if there's a bug somewhere, you'll also get, as an error message rather than notice message, a "Oops, E_NOTICE on line 54" message.
Form validation is another case. If you have multiple errors in a single form, we prefer to list all of them. So if you screw up 4 times on a form, you may get 4 different error messages showing what you screwed up so you can fix it in one go instead of several.
Now sure, one could emulate that by building a multi-message layer on top of single-layer messages, but, really, why? "One is a special case of many", and there are many many cases where you'll want to post multiple messages. Like, most of Drupal. :-)
---------------------------------------------------------------------------
by lsmith77 at 2012-03-06T20:55:51Z
@fabpot is there any information you still need before merging this? do you want more discussion in which case you might want to take this to the mailing list ..
---------------------------------------------------------------------------
by drak at 2012-03-08T18:54:13Z
Another plus for this PR is that it requires no extra lines of code in templates etc to display the flashes, see https://github.com/symfony/symfony/pull/3267/files#diff-1
---------------------------------------------------------------------------
by drak at 2012-03-15T06:38:21Z
Rebased against current `master`, should be mergeable again..
---------------------------------------------------------------------------
by evillemez at 2012-03-17T03:08:41Z
+1 to this, I have an extended version of HttpFoundation just for this... would love to get rid of it.
Commits
-------
bd02554 [HttpFoundation] SPL IteratorAggregate+Countable on *Bags
665fdeb [HttpFoundation] SPL on ParameterBag
Discussion
----------
[HttpFoundation] SPL on ParameterBag
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Added a couple SPL interfaces to ParameterBag, added shortcuts to working with the parameters. For example:
```php
<?php
$post = Request::createFromGlobal()->request;
echo "There are {count($post)} POST variables\n";
foreach ($post as $key => $val) {
echo "{$key}: {$val}\n";
}
```
Thoughts?
---------------------------------------------------------------------------
by stealth35 at 2012-03-07T13:09:11Z
You already have the `all` method
``` php
<?php
$post = Request::createFromGlobals()->request->all();
echo "There are ", count($post), " POST variables\n";
foreach ($post as $key => $val) {
echo "{$key}: {$val}\n";
}
```
---------------------------------------------------------------------------
by cboden at 2012-03-07T13:50:22Z
Yes, but when in the context of working with the Request object (or POST ParamegerBag), it's 1 more call and loose variable to set.
ParameterBag is a container, these common SPL interfaces give standard PHP container methods to it.
---------------------------------------------------------------------------
by lsmith77 at 2012-03-07T18:42:41Z
makes sense to me ..
---------------------------------------------------------------------------
by vicb at 2012-03-09T15:45:40Z
Probably makes sense. Could you check if any other `*Bag.php` needs to be updated so that it could ba an atomic merge.
---------------------------------------------------------------------------
by cboden at 2012-03-09T15:48:40Z
Whoops, good catch @vicb. I made a poor assumption all the *Bags extended ParameterBag, while only some do. I will post an update shortly.
Commits
-------
c4ee947 Native Redis Session Storage update
665f593 NativeRedisSessionStorage added
Discussion
----------
[HttpFoundation] Native Redis Session Storage
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
---------------------------------------------------------------------------
by lstrojny at 2012-03-04T23:15:43Z
Does Symfony (or any of its dependencies) has Redis support in any form whatsoever? If not this might be a good point to decide which clients to support
---------------------------------------------------------------------------
by lsmith77 at 2012-03-04T23:36:11Z
well ideally we just get this cache interface stuff done .. for this use case it would be perfect.
---------------------------------------------------------------------------
by pulzarraider at 2012-03-05T00:35:59Z
There is RedisProfilerStorage available (based on phpredis). I prefer and write code for [phpredis](https://github.com/nicolasff/phpredis).
It's recommended by [official Redis homepage](http://redis.io/clients#PHP). [In this benchmark](http://dev.af83.com/2011/01/01/which-php-library-to-use-with-redis-the-benchmark.html
) is fastest and less memory consumpting.
But if somebody prefer predis (with phpiredis), rediska or something other widely used, there are no limitations to add support of it to Symfony.
My opinion is, that the C extension should be supported at first, because of good performance and native session storage support. Redis is quite young and the process of creating PHP clients is comparable to Memcache.
There were created pure PHP Memcache clients in the past (Google found for example [this](http://www.phpclasses.org/browse/file/20284.html) and [this](http://code.blitzaffe.com/pages/phpclasses/files/memcached_client_52-12)), but they are not being used now. Everyone, who is seriously thinking about performance, is using only the C Redis/Memcache(d)/... extensions.
---------------------------------------------------------------------------
by drak at 2012-03-05T07:40:06Z
+1 on this PR. Needs a test written though.
I don't think there is any need to wait for #3493 imo. I'll deal with it if this is merged before #3493.
Are there any PHP ini settings for this for this driver or is everything via the `session.save_path` directive? (A quick look at the C code seems to indicate there are no explicit ini directives).
---------------------------------------------------------------------------
by lstrojny at 2012-03-05T12:14:34Z
@pulzarraider I don’t necessarily disagree with the usage of phpredis, I just wanted to bring up the issues of various clients and people having different preferences about them.
---------------------------------------------------------------------------
by fabpot at 2012-03-05T14:46:22Z
@pulzarraider Can you add some unit tests before I merge?
---------------------------------------------------------------------------
by pulzarraider at 2012-03-11T20:19:57Z
@drak No there are no php.ini settings. Only RedisArray has some, but it's another feature.
@fabpot I've added simple test based on other session storage tests.
I planned to create a RedisSessionStorage, too, but I have no time for it now. This can be added later in another PR as it's independent from NativeRedisSessionStorage.
---------------------------------------------------------------------------
by drak at 2012-03-12T02:21:25Z
The code looks OK to me.
---------------------------------------------------------------------------
by fabpot at 2012-03-15T06:05:27Z
#3493 has been merged now.
---------------------------------------------------------------------------
by pulzarraider at 2012-03-16T23:21:27Z
Code updated.
Added methods to control long running processes to the Process class:
- A non blocking start method to startup a process and return
immediately
- A blocking waitForTermination method to wait for the processes
termination
- A stop method to stop a process started with start
All status-getters like getOutput were changed to return real-time data
Commits
-------
4a43453 remove callback from constructor and create method
601b87c add basic validation of callback name
266f76d rename jsonp to callback, defaults to null
38b79a7 add data and callback setter to JsonResponse
6788224 add JSONP support to JsonResponse
Discussion
----------
add JSONP support to JsonResponse
---------------------------------------------------------------------------
by schmittjoh at 2012-03-19T17:56:24Z
I think a ``setCallback()`` method would be more expressive, and easier to use:
```php
<?php
return JsonResponse::create($myData)->setCallback('foo');
// vs
return new JsonResponse($myData, 200, array(), 'foo');
```
What do you think?
---------------------------------------------------------------------------
by havvg at 2012-03-19T18:07:38Z
Looks good to me, I'll add it.
---------------------------------------------------------------------------
by dlsniper at 2012-03-19T19:38:45Z
+1 for this one :)
---------------------------------------------------------------------------
by vicb at 2012-03-19T23:09:50Z
Sorry for nitpicking but what about:
* some validation on the function name ?
* renamming `jsonp` -> `callback` ?
---------------------------------------------------------------------------
by havvg at 2012-03-20T09:16:32Z
@vicb What do you mean with "some validation on the function name"? I can't follow you there.
---------------------------------------------------------------------------
by vicb at 2012-03-20T09:22:49Z
I mean a valid JS function name
---------------------------------------------------------------------------
by havvg at 2012-03-20T09:34:59Z
Ah I see, I searched for it, and ended up with those results:
* The most complete: http://stackoverflow.com/questions/2008279/validate-a-javascript-function-name#answer-9392578
* and a less accurate one: http://www.geekality.net/2011/08/03/valid-javascript-identifier/
I'm not sure whether to put this into the `JsonResponse` itself, or to add somewhere else (where, if so?).
---------------------------------------------------------------------------
by vicb at 2012-03-20T09:45:20Z
I would go for a regexp only (ignoring reserved words); The idea would be not to use this to run arbitrary JS code.
---------------------------------------------------------------------------
by fabpot at 2012-03-21T21:33:36Z
Now that you have added the `setCallback` method, I would remove the constructor argument as it makes the signature quite long. As we have the `create` method anyway, it's more explicit and clearer to use the `setCallback` .
---------------------------------------------------------------------------
by havvg at 2012-03-21T21:37:51Z
So remove the callback argument from both, the constructor and the `create` method or only from the constructor?
---------------------------------------------------------------------------
by havvg at 2012-03-21T21:38:30Z
Ehr.. never mind :-)
Commits
-------
836d12b Fixing typo.
ac2a187 Improved feedback for Upload Validator to cover all PHP error states. This way we don't get a unclear "upload error" message unless its something completely unexpected.
Discussion
----------
Improved feedback for Upload Validator to cover all PHP error states.
The upload validator only sets individual messages for 2 out of the 7 error states PHP suports for uploading files. Which means when you have any of those 5 stats you get a standard error message and have to really dig into the code to read the error state.
I added messages for every state, so that you will always get a detailed message.
---------------------------------------------------------------------------
by stloyd at 2012-03-19T13:54:04Z
You should probably also extend translations in `FrameworkBundle`.
---------------------------------------------------------------------------
by rdohms at 2012-03-19T14:04:50Z
@stloyd what's the best way to do that? I obviously don't speak all languages. Could you point me to a best practices in this case?
---------------------------------------------------------------------------
by stof at 2012-03-19T15:58:17Z
@rdohms update the translations for the languages you speak. Other people will contribute with update later eventually :)
---------------------------------------------------------------------------
by rdohms at 2012-03-19T22:34:50Z
Fixed the typo, only other language i can update is portuguese, but it needs more work. I'll update it on a separate PR later on.
Anything else for this PR?
---------------------------------------------------------------------------
by mvrhov at 2012-03-20T05:41:00Z
@rdohms: just put English strings into other lanugages
---------------------------------------------------------------------------
by rdohms at 2012-03-20T07:35:56Z
@mvrhov is there a quick way to do this? or is it copying and pasting into every language and adjusting the string IDs?
---------------------------------------------------------------------------
by mvrhov at 2012-03-20T09:02:59Z
AFAIK you'll have to copy paste. String ids and source are the same in all translations so it really is just c/p after you update the first one.
---------------------------------------------------------------------------
by rdohms at 2012-03-20T09:56:48Z
@mvrhov so which is the most updated one you would say? i see a lot of them missing bit and pieces :P
---------------------------------------------------------------------------
by mvrhov at 2012-03-20T14:00:21Z
Sorry no idea.
---------------------------------------------------------------------------
by stof at 2012-03-20T19:09:10Z
@mvrhov Please don't do this. The translation component has a fallback mecanism so putting the EN string in an incomplete translation file is a bad idea as it forbids using a fallback.
---------------------------------------------------------------------------
by rdohms at 2012-03-20T20:14:50Z
@stof i figured as much.
Any other concerns before we push this PR further?
Commits
-------
93d2a4f [Translation] Ignore xliff entries with no "target" node
Discussion
----------
[Translation] Ignore xliff entries with no "target" node
This PR will ignore some entries with no "target" node when a xliff file is loaded.
```xml
<xliff xmlns="urn:oasis:names:tc:xliff:document:1.2" version="1.2">
<file source-language="en" datatype="plaintext" original="file.ext">
<body>
<trans-unit id="1">
<source>foo</source>
<target>bar</target>
</trans-unit>
<trans-unit id="2">
<source>extra</source>
</trans-unit>
</body>
</file>
</xliff>
```
Before:
```php
array(
'foo' => 'bar',
'extra' => ''
);
```
After:
```php
array(
'foo' => 'bar'
);
```
---------------------------------------------------------------------------
by fabpot at 2012-03-21T17:35:51Z
Wouldn't it be better to throw an exception for such cases? A `trans-unit` without a `target` is useless anyway, no?
---------------------------------------------------------------------------
by aerialls at 2012-03-21T20:25:19Z
I'm using transifex (https://www.transifex.net/home/) and when a user doesn't translate an entry, transifex fills up a `trans-unit` node without a `target` children.
Commits
-------
4d4ef24 [Console] Stop parsing options after encountering "--" token
Discussion
----------
[Console] Stop parsing options after encountering "--" token
This enables support for arguments with leading dashes (e.g. "-1"), as supported by getopt in other languages.
[](http://travis-ci.org/jmikola/symfony)
The test suite currently fails due to 7a54fe41ca. ArgvInputTest passes, and these changes don't appear to break anything else.
Aside: This got me thinking about how one would pass an option value of "-1". I suppose for input options with `VALUE_OPTIONAL`, it would be ambiguous if "-1" followed; however, `VALUE_REQUIRED` should probably require that the next token is captured as the option value. In my tests, a required option value with a leading dash was interpreted as another option. The workaround for all of this is to use the space-less syntax (e.g. `-f=-1`).
---------------------------------------------------------------------------
by fabpot at 2012-03-17T08:43:15Z
AFAIK, the `--` should disable both option and argument parsing, no?
---------------------------------------------------------------------------
by jmikola at 2012-03-18T02:13:51Z
If that were the case, what would be the point of using `--` at all? :)
* http://wiki.bash-hackers.org/dict/terms/end_of_options
* http://perldoc.perl.org/Getopt/Long.html#Mixing-command-line-option-with-other-arguments
Commits
-------
0c83c5d [Form] Alternate syntax for form_theme
Discussion
----------
[RFC][Form] Alternate syntax for form_theme
before
`{% form_theme form _self "::base.html.twig" %}`
after
`{% form_theme form with "::base.html.twig" %}`
`{% form_theme form with varTheme %}`
`{% form_theme form with [_self, "::base.html.twig"] %}`
_the former syntax is still supported_
---------------------------------------------------------------------------
by stof at 2012-03-12T15:42:32Z
do you really need ``with`` ?
---------------------------------------------------------------------------
by vicb at 2012-03-12T15:50:41Z
it's not needed but I find it more clear (It can be drop if a consensus is reached)
---------------------------------------------------------------------------
by fabpot at 2012-03-12T17:05:46Z
+1 for `with`. Documentation for master should be updated as well.
---------------------------------------------------------------------------
by Tobion at 2012-03-13T02:26:22Z
+1 for `with`, but the syntax without array like `{% form_theme form with "::base.html.twig" %}` should also be supported
---------------------------------------------------------------------------
by vicb at 2012-03-13T07:16:55Z
`[]` are nice as they clearly indicate the ability to use multiple themes (which I think is yet to be documented). We'll pick the most popular syntax only.
---------------------------------------------------------------------------
by stof at 2012-03-13T08:16:40Z
@vicb supporting a string instead of an array should be possible when you need only one element. supporting several ones and turning it into an array is the mistake we made for 2.0
---------------------------------------------------------------------------
by hhamon at 2012-03-13T08:16:45Z
+1 for the new syntax
---------------------------------------------------------------------------
by vicb at 2012-03-13T08:29:45Z
@stof @Tobion what about using the former syntax then ?
---------------------------------------------------------------------------
by Baachi at 2012-03-13T08:32:09Z
+1 for new syntax. But it should be possible to use strings instead of arrays.
---------------------------------------------------------------------------
by stof at 2012-03-13T08:33:07Z
@vicb Having one wyntax using ``with`` and the other without will confuse users IMO. this is why I suggested allowing to pass a Twig array without adding an extra word
---------------------------------------------------------------------------
by stof at 2012-03-13T08:40:02Z
@Baachi not stringS as it is precisely what we are trying to solve :)
---------------------------------------------------------------------------
by Baachi at 2012-03-13T08:42:03Z
Oh sry. I mean __string__. :)
---------------------------------------------------------------------------
by fabpot at 2012-03-13T11:16:30Z
+1 for supporting a string or an array with the new syntax as using only one element is probably the most common use case. But then, why not supporting any valid Twig expression?
---------------------------------------------------------------------------
by vicb at 2012-03-13T11:54:51Z
Something like the latest commit ? (Tests have to be updated).
@fabpot What is the best place to handle array / non-array ? This is currenlty handled in the node but the parser might be a better place.
---------------------------------------------------------------------------
by fabpot at 2012-03-13T13:23:08Z
@vicb: I would just remove the special array case in the node as it's not needed anymore.
---------------------------------------------------------------------------
by fabpot at 2012-03-13T13:24:15Z
... and update FormExtension::setTheme() to also accept a string in which case we convert it to an array there.
---------------------------------------------------------------------------
by schmittjoh at 2012-03-13T14:26:17Z
I'd prefer a named argument instead of an ubiquitous "with" keyword which does not really tell me what's coming next.
Something like ``{% form_theme _form templates=[a, b, c] %}``. This is pretty nicely done for the assetic tags "javascripts", and "stylesheets".
---------------------------------------------------------------------------
by Tobion at 2012-03-13T16:04:26Z
@schmittjoh it would only make sense if there are multiple named arguments. With only one available it seems redundant.
Also `{% form_theme _form templates="template.html.twig" %}` is bad.
---------------------------------------------------------------------------
by vicb at 2012-03-14T07:59:08Z
I tend to agree with @Tobion but I'll have a closer look at assetic to see if we can make things more consistent.
---------------------------------------------------------------------------
by Seldaek at 2012-03-14T10:36:15Z
This would be more consistent with assetic, but assetic isn't really consistent with anything else in twig, although I see the benefits in that particular case for swapping and omitting parameters.
---------------------------------------------------------------------------
by schmittjoh at 2012-03-14T15:49:37Z
My goal was not really consistency, but I simply find it more obvious,
self-explanatory and easier to understand if you name things explicitly. We
are using the "with" keyword in several places and each time something
different is expected.
To me explicit naming is superior, but just my 2c
On Wed, Mar 14, 2012 at 4:36 AM, Jordi Boggiano <
reply@reply.github.com
> wrote:
> This would be more consistent with assetic, but assetic isn't really
> consistent with anything else in twig, although I see the benefits in that
> particular case for swapping and omitting parameters.
>
> ---
> Reply to this email directly or view it on GitHub:
> https://github.com/symfony/symfony/pull/3576#issuecomment-4495732
>
---------------------------------------------------------------------------
by Tobion at 2012-03-14T16:48:01Z
When I first saw this tag I didn't understand the role of first parameter.
So if we use johannes suggestion it should rather be `{% form_theme form=myForm templates=[a, b, c] %}`
---------------------------------------------------------------------------
by mvrhov at 2012-03-14T18:09:09Z
Before we complicate this any further can I add another thing here.
Moving to dedicated issue: Inflexible form theming #3598
---------------------------------------------------------------------------
by vicb at 2012-03-14T18:20:54Z
@mvrhov that is not the good place to discuss this (both this particular issue and GH as this is a support request).
_Have you tried `{% form_theme form.subForm ... %}`_
---------------------------------------------------------------------------
by vicb at 2012-03-15T07:39:14Z
Where do you think we should go:
1. `{% form_theme form with [_self, "::base.html.twig"] %}`
2. `{% form_theme form=form src=[_self, "::base.html.twig"] %}`
Let's discuss the structure first & not the details (i.e. src vs templates).
---------------------------------------------------------------------------
by Baachi at 2012-03-15T07:52:51Z
I tend to ```{% form_theme form with [_self, "::base.html.twig"] %}```, because its more consistent to the twig syntax.
---------------------------------------------------------------------------
by fabpot at 2012-03-15T13:10:56Z
@vicb: I like 1) more than 2) as this how the built-in tags work.
To keep BC even further, can we just remove the `with` keyword? To make it BC, we just need to have a look at extra parameters and add it to an array if they exist.
---------------------------------------------------------------------------
by Tobion at 2012-03-15T13:19:52Z
For newcomers 2) is definitely easier to understand. But it would also only make sense if you can change the parameter order, so `{% form_theme form=form src=[_self, "::base.html.twig"] %}` == ` {% form_theme src=[_self, "::base.html.twig"] form=form %}`. At the same time it reduces consistency. So for experienced developers option 1) [without "with"] is less redundant and preferable.
---------------------------------------------------------------------------
by vicb at 2012-03-15T13:53:49Z
@fabpot removing the `with` will make `Parser::parsePostfixException()` scream when providing an array of themes.
Commits
-------
5fa1c70 [json-response] Add a JsonResponse class for convenient JSON encoding
Discussion
----------
[json-response] Add a JsonResponse class for convenient JSON encoding
Usage example:
$data = array(user => $user->toArray());
return new JsonResponse($data);
---------------------------------------------------------------------------
by drak at 2012-02-16T11:51:11Z
@fabpot - maybe we could benefit with a bit more sub-namespacing in this component. One for Response for example and probably one for Request.
---------------------------------------------------------------------------
by Seldaek at 2012-02-16T15:07:31Z
@drak Please no. Moving the session was already a pain IMO since it was type-hinted in a few places (lack of interface, and interface doesn't include flash stuff still). Creating BC breaks just for fun like that is annoying for interop of bundles. It doesn't matter whether we have 10 or 15 classes in one directory.
---------------------------------------------------------------------------
by drak at 2012-02-17T08:33:46Z
@francodacosta The most optimal place is `__toString()`.
@Saldaek It just looks like the whole namespace is getting more cluttered. I suggest it because things like Request/Response objects are surely only going to grow over time. There is always the possibility to make BC for moved and renamed classes so there doesn't have to be any extra complications for making things look cleaner. Anyway, just a thought :-)
---------------------------------------------------------------------------
by stof at 2012-02-17T14:47:40Z
@drak Changing the namespace of a class is a BC break. The request and the response are used in many more places than the Session so it would be a real pain to update this. And the component is tagged with ``@api`` so BC breaks are forbidden without a good reason. The session refactoring was one as it was really an issue in the implementation, but simply renaming the class is not.
---------------------------------------------------------------------------
by fabpot at 2012-03-05T15:03:53Z
I'm -1 for adding this to the core. It does not add much value and why add a special response for JSON and not other formats?
---------------------------------------------------------------------------
by Seldaek at 2012-03-05T18:38:05Z
I think it's useful because it's a class we need in almost every project, and I don't think we're alone. It's super simple but makes me wonder every time why I have to recreate it. I don't want an additional bundle just for 3lines of code. Similarly I would say a JsonpResponse would be great, or maybe just an optional $callback arg to the json response to enable jsonp mode.
I just had someone ask me on irc how to do JSONP so while I think it's obvious and I'm sure you'd think that too, it obviously isn't to newcomers. The Response stuff is hidden behind those render methods & such and people don't realize they can simply subclass. If a few examples were in core it would be both helpful for learning and useful on a day to day basis.
As for other formats, well JSON is typically used nowadays, except when you want more fancy XML APIs, but for that the JMSSerializerBundle + FOSRestBundle are superior and we can't achieve such things in a few lines of code. I could also see a BinaryResponse or DownloadResponse or such that has proper "force-download" headers and accepts any binary stream, but that's another debate.
---------------------------------------------------------------------------
by dragoonis at 2012-03-05T19:43:05Z
I'm +1 for the concept but not commenting on how it should be implemented I'll leave that to other people.
Typically when you want to force a download you have to do ``content-disposition: attachment; filename="filehere.pdf"``
Modifying some response headers and the likes automatically for the user by returning a DownloadResponse object would be very handy..
I'm +1 for @Seldaek's point about examples of sub-classing for specific use cases. It will help with demonstrating how to do custom stuff the right way rather than people coming up with their own contraptions.
---------------------------------------------------------------------------
by stof at 2012-03-05T20:14:39Z
btw, regarding the BinaryResponse, there is a pending PR about it: #2606
---------------------------------------------------------------------------
by simensen at 2012-03-05T21:07:33Z
I'm +1 for providing reference implementations fo custom Response cases. I wanted to find best practices for handling JSONP requests/responses and couldn't find anything at all on the topic. I thought maybe extending Response might be useful but wasn't sure if that could be done safely or should be done at all.
---------------------------------------------------------------------------
by lsmith77 at 2012-03-05T22:28:01Z
@stof i think @drak was suggesting moving the class, but leaving an empty class extending from the new class in the old location to maintain BC
---------------------------------------------------------------------------
by stof at 2012-03-05T23:55:36Z
@lsmith77 This would force Symfony to use the BC class so that it does not break all typehints in existing code
---------------------------------------------------------------------------
by lsmith77 at 2012-03-06T00:22:15Z
BC hacks are never nice .. the goal would just be to eventually have all those classes and more importantly all new ones in a subnamespace. actually it might be easier to just leave all the classes in the old location and create new ones extending from the old ones. anyway .. personally i am also not such a big fan of these specialized responses .. but i guess i see FOSRestBundle as the alternative answer which makes me biased.
---------------------------------------------------------------------------
by Seldaek at 2012-03-06T07:57:36Z
I'm using FOSRestBundle when it's needed, but when you just have a small scale app that needs one or two json responses for specialized stuff it is slightly overkill. And again, newcomers probably won't know about it, and encouraging using it for simple use cases isn't exactly the best learning curve we can provide.
---------------------------------------------------------------------------
by COil at 2012-03-06T23:12:15Z
+1 for this. I have implemented such a function in all my sf1 projects, it will be the same for sf2.
---------------------------------------------------------------------------
by fabpot at 2012-03-15T13:22:27Z
Closing this PR in favor of a cookbook that explains how a developer can override the default Response class (this JSON class being a good example). see symfony/symfony-docs#1159
---------------------------------------------------------------------------
by Seldaek at 2012-03-15T13:25:08Z
Meh. Forcing people to copy paste code from the cookbook in every second project isn't exactly a step forward with regard to ease of use and user-friendliness.
---------------------------------------------------------------------------
by Seldaek at 2012-03-15T13:26:48Z
I mean following this logic, things like the X509 authentication should just be put in cookbooks too because almost nobody needs that. We have tons of code in the framework, I don't get the resistance with adding such a simple class which makes code more expressive.
---------------------------------------------------------------------------
by fabpot at 2012-03-15T13:53:07Z
because X509 authentication is not easy to get it right. Sending a JSON response is as simple as it can get:
new Response(json_encode($data), 200, array('Content-Type' => 'application/json'));
---------------------------------------------------------------------------
by marijn at 2012-03-15T13:54:25Z
Perhaps we need a `Symfony\Extensions\{Component}` namespace for things that don't necessarily belong in the core but are truly useful...
---------------------------------------------------------------------------
by Seldaek at 2012-03-15T14:03:40Z
I still fail to see why it doesn't belong in core.. There are tons of little helpers here and there, a base controller class made only of proxies, and then this gets turned down because it is simple to do it yourself? Sure it is simple, but it's repetitive and boring too. And while it's simple when you know your way around, some people aren't really sure how to do it.
The whole point of a framework is to avoid repetitive bullshit and be more productive. @fabpot do you have any real arguments against? I can see that you don't see a big use to it, fair enough, but do you see any downside at all?
Commits
-------
1ec075d [ClassLoader] Fixed version compare
8fb529c [ClassLoader] Fixed ClassMapGenerator and added suport for traits
Discussion
----------
[ClassLoader] Fixed ClassMapGenerator and added suport for traits
---------------------------------------------------------------------------
by hason at 2012-03-08T10:49:53Z
@fabpot, @Seldaek ``PHP_VERSION_ID`` or ``version_compare``?
---------------------------------------------------------------------------
by Seldaek at 2012-03-08T11:42:20Z
Ultimately @fabpot can call it, but I'm pro version_compare because it's just typically used for those checks, which may not make it more readable but makes it less WTF since it's a common pattern.
---------------------------------------------------------------------------
by drak at 2012-03-08T13:43:18Z
I prefer `version_compare()` with `phpversion()` as it's way more readable and obvious what it is.
---------------------------------------------------------------------------
by fabpot at 2012-03-08T17:06:25Z
+1 for `version_compare()`
---------------------------------------------------------------------------
by hason at 2012-03-09T07:19:10Z
@fabpot done
Commits
-------
dee47b1 [DoctrineBridge] Add minimal tests for DBAL session storage driver
Discussion
----------
[2.1][DoctrineBridge] Add minimal tests for DBAL session storage driver
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
This is intentionally only for the `master` branch because the class is different between 2.0 and master. This test is the minimal but at least will mean any refactoring changes in dependencies get caught.
Commits
-------
eb759c5 [Propel1] Fixed data collector
Discussion
----------
[Propel1] Fixed data collector
---------------------------------------------------------------------------
by jeremyFreeAgent at 2012-03-05T16:25:58Z
Sorry for the two previous pull requests :(
Commits
-------
49a8654 [Security] Use LogoutException for invalid CSRF token in LogoutListener
a96105e [SecurityBundle] Use assertCount() in tests
4837407 [SecurityBundle] Fix execution of functional tests with different names
66722b3 [SecurityBundle] Templating helpers to generate logout URL's with CSRF tokens
aaaa040 [Security] Allow LogoutListener to validate CSRF tokens
b1f545b [Security] Refactor LogoutListener constructor to take options
c48c775 [SecurityBundle] Add functional test for form login with CSRF token
Discussion
----------
[Security] Implement support for CSRF tokens in logout URL's
```
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
```
[](http://travis-ci.org/jmikola/symfony)
This derived from #3006 but properly targeting on the master branch.
This exposes new configuration options to the logout listener to enable CSRF protection, as already exists for the form login listener. The individual commits and their extended messages should suffice for explaining the logical changes of the PR.
In addition to changing LogoutListener, I also created a templating helper to generate logout URL's, which includes a CSRF token if necessary. This may or may not using routing, depending on how the listener is configured since both route names or hard-coded paths are valid options.
Additionally, I added unit tests for LogoutListener and functional tests for both CSRF-enabled form logins and the new logout listener work.
Kudo's to @henrikbjorn for taking the time to document CSRF validation for form login listeners (see [here](http://henrik.bjrnskov.dk/symfony2-cross-site-request-forgery/)). The [Logout CSRF Protection](http://www.yiiframework.com/wiki/190/logout-csrf-protection/) article on the Yii Framework wiki was also helpful in drafting this.
---------------------------------------------------------------------------
by jmikola at 2011-12-31T07:50:31Z
Odd that Travis CI reported a build failure for PHP 5.3.2, but both 5.3 and 5.4 passed: http://travis-ci.org/#!/jmikola/symfony/builds/463356
My local machine passes as well.
---------------------------------------------------------------------------
by jmikola at 2012-02-06T20:05:30Z
@schmittjoh: Please let me know your thoughts on the last commit. I think it would be overkill to add support for another handler service and/or error page just for logout exceptions.
Perhaps as an alternative, we might just want to consider an invalid CSRF token on logout imply a false return value for `LogoutListener::requiresLogout()`. That would sacrifice the ability to handle the error separately (which a 403 response allows us), although we could still add logging (currently done in ExceptionListener).
---------------------------------------------------------------------------
by jmikola at 2012-02-13T17:41:33Z
@schmittjoh: ping
---------------------------------------------------------------------------
by fabpot at 2012-02-14T23:36:22Z
@jmikola: Instead of merging symfony/master, can you rebase?
---------------------------------------------------------------------------
by jmikola at 2012-02-15T00:00:49Z
Will do.
---------------------------------------------------------------------------
by jmikola at 2012-02-15T00:05:48Z
```
[avocado: symfony] logout-csrf (+9/-216) $ git rebase master
First, rewinding head to replay your work on top of it...
Applying: [SecurityBundle] Add functional test for form login with CSRF token
Applying: [Security] Refactor LogoutListener constructor to take options
Applying: [Security] Allow LogoutListener to validate CSRF tokens
Applying: [SecurityBundle] Templating helpers to generate logout URL's with CSRF tokens
Applying: [SecurityBundle] Fix execution of functional tests with different names
Applying: [SecurityBundle] Use assertCount() in tests
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Applying: [Security] Use LogoutException for invalid CSRF token in LogoutListener
[avocado: symfony] logout-csrf (+7) $ git st
# On branch logout-csrf
# Your branch and 'origin/logout-csrf' have diverged,
# and have 223 and 9 different commit(s) each, respectively.
#
nothing to commit (working directory clean)
[avocado: symfony] logout-csrf (+7) $
```
After rebasing, my merge commits disappeared. Is this normal?
---------------------------------------------------------------------------
by stof at 2012-02-15T00:15:07Z
Are you sure they disappeared ? Diverging from the remote branch is logical (you rewrote the history and so changed the commit id) but are you sure it does not have the commits on top of master ? Try ``git log master..logout-scrf``
If your commut are there, you simply need to force the push for the logout-csrf branch (take care to push only this branch during the force push to avoid messing all others as git won't warn you when asking to force)
---------------------------------------------------------------------------
by stof at 2012-02-15T00:17:09Z
ah sorry, you talked only about the merge commit. Yeah it is normal. When reapplying your commits on top of master, the merge commit are not kept as you are reapplying the changes linearly on top of the other branch (and deleting the merge commit was the reason why @fabpot asked you to rebase instead of merging btw)
---------------------------------------------------------------------------
by jmikola at 2012-02-15T00:18:00Z
The merge commits are not present in `git log master..logout-csrf`. Perhaps it used those merge commits when rebasing, as there were definitely conflicts resolved when I originally merged in symfony/master (@fabpot had made his own changes to LogoutListener).
I'll force-push the changes to my PR brange. IIRC, GitHub is smart enough to preserve inline diff comments, provided they were made through the PR and not on the original commits.
---------------------------------------------------------------------------
by jmikola at 2012-02-15T00:19:38Z
That worked well. In the future, I think I'll stick to merging upstream in and then rebasing afterwards. Resolving conflicts is much easier during a merge than interactive rebase.
---------------------------------------------------------------------------
by jmikola at 2012-02-23T18:46:13Z
@fabpot @schmittjoh: Is there anything else I can do for this PR? I believe the exception was the only outstanding question (see: [this comment](https://github.com/symfony/symfony/pull/3007#issuecomment-3835716)).
Commits
-------
100d59b Modified Memcache(d) dsn to be more intuitive. Chnged Exception texts in other storages.
Discussion
----------
[HttpKernel] Modified Memcache(d)ProfilerStorage dsn to be more intuitive
Bug fix: no
Feature addition: -
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
Before:
```
#app/config/config_dev.yml
...
framework:
...
profiler:
...
dsn: memcache://127.0.0.1/11211
...
```
Now:
```
#app/config/config_dev.yml
...
framework:
...
profiler:
...
dsn: memcache://127.0.0.1:11211
...
```
If Memcache host is IPv6 address:
```
#app/config/config_dev.yml
...
framework:
...
profiler:
...
dsn: memcache://[::1]:11211
...
```
I changed texts of some exceptions to be more consistent, too.
Commits
-------
7444fdf Feedback fixes
54cfd44 Restore bypass_shell by default with windows compat
38df47a Fix env inheritance and added tests
f555c62 [Process] Add windows compatibility to Process component
c4e8ff7 [Process] Always escape commands properly and remove windows-specific handling
9e237f6 [Process] Add ProcessBuilder::create() for more fluidity in the interface until 5.4
4882777 [Process] Code clean up
Discussion
----------
ProcessBuilder clean up
- Code cleanup
- Added create() static method for easy creation until we can do `$process = (new ProcessBuilder())->add()->getProcess();`
- Removed windows wrapping of commands. This does not belong there IMO. If assetic needs that it should add it, and if it's generally beneficial to everyone then we should add it to Process, but having it implicitly only when using ProcessBuilder makes on sense.
---------------------------------------------------------------------------
by beberlei at 2012-02-16T16:10:15Z
I agree on the windows stuff. I know it fixes a bunch of issues in Assetic, but it also caused my tons of headaches in my windows commands that didnt need strict escaping. Also this messes with parameters in Powershell for example, when you have "foo /bar:baz" then it makes this to ""foo" "/bar:baz"" which in some circumstances fails. Its all messy.
---------------------------------------------------------------------------
by schmittjoh at 2012-02-16T17:53:30Z
Can you move the wrapping to the Process class instead? It's generally causing no bad side effects, but fixes a few issues in the proc_open implementation. It is also necessary for Assetic, and potentially other tools to work on Windows.
---------------------------------------------------------------------------
by Seldaek at 2012-02-16T17:56:02Z
Sure, although "generally" sounds a bit scary in your sentence :)
What about the bypass_shell option?
---------------------------------------------------------------------------
by schmittjoh at 2012-02-16T18:02:12Z
"generally" means I don't know of any, but what I do know is that the alternative you are suggesting is not working. Have there been any bug reports on Assetic/symfony/your own code that "cmd" wrapping causes problems?
---------------------------------------------------------------------------
by Seldaek at 2012-02-16T18:04:59Z
No no, don't get me wrong, I'm not suggesting this should be removed. I'm just saying it should be done for all processes or none, but not just for those run via the ProcessBuilder because that's a good recipe for WTFs.
---------------------------------------------------------------------------
by schmittjoh at 2012-02-16T18:09:38Z
Yeah, I understand, and it makes sense.
What I would suggest is to move it to the process class, and let a wider audience test this to see if we get any bug reports on strange behavior etc.
---------------------------------------------------------------------------
by Seldaek at 2012-02-16T18:12:00Z
Still not sure about the bypass_shell option though. And @beberlei mentioned problems? Can you expand on that?
---------------------------------------------------------------------------
by Seldaek at 2012-02-16T18:16:34Z
Added back to Process, with a switch so if anyone runs into problems they can easily disable it.
---------------------------------------------------------------------------
by Seldaek at 2012-02-22T10:59:58Z
Ping @fabpot - I think this is ready now
Ping @kriswallsmith if this gets merged please update Assetic stuff to restore the bypass_shell option if it's really needed.
---------------------------------------------------------------------------
by kriswallsmith at 2012-02-22T12:41:15Z
Posting a PR under "code cleanup" that tinkers with a class that is inherently difficult to test for regression and has been tested by the community for over a year is… a bit hard to swallow, honestly. Everything is there for a reason and should not be tinkered with lightly.
For example, it's important that the `$env` variable default to `null` so the current environment is inherited by default — why change that?
I don't know what the `bypass_shell` option does, but @pierrejoye does… which is why he put it there.
I'm okay with adding an "enhanced Windows compatibility" switch, but I personally think is should be on the builder, not `Process`. The builder is where we manipulate the strings that compose the command line, not in `Process`. You're introducing manipulation of the command line to `Process`, which blurs the responsibilities of the two classes.
I'm also okay with the static factory method :)
---------------------------------------------------------------------------
by Seldaek at 2012-02-22T13:19:40Z
@kriswallsmith (Sorry about the confusing title) My concern is just that if you use Process then decide to "upgrade" to the ProcessBuilder, you suddenly have a change of behavior that might break stuff without you noticing. I just want to avoid this unexpected behavior.
As for the $env stuff, I added a couple tests now, and then expanded that ternary operator a bit.. It actually was broken before. It passed null if you had no env set, but even if you did not call `inheritEnvironmentVariables`. If you want to inherit by default - which I agree it should - then why was `inheritEnv = false` in the constructor? I changed it too and now there is hopefully less confusion.
Restored bypass_shell=true unless it's explicitly set to false.
---------------------------------------------------------------------------
by kriswallsmith at 2012-02-22T13:25:23Z
We should also add the PHPUnit `@backupGlobals enabled` annotation while we're in here.
---------------------------------------------------------------------------
by kriswallsmith at 2012-02-22T13:31:41Z
@Seldaek Looks better, thanks for the changes. If `enhanceWindowsCompatibility` is going to live on `Process` we should expose the switch on the builder as well. Speaking of `enhanceWindowsCompatibility`… is there a more descriptive name for that? What exactly does that do, any why would anyone want to switch it off? The name is so vague we might as well call it `enableMagicalWindowsFix()`.
---------------------------------------------------------------------------
by pierrejoye at 2012-02-22T13:33:55Z
I really do not think that having a flag to enable portability is a
good idea, at all.
I do not remember the context right now but a flag is definitively a
bad idea (you will need other on other platforms).
I will take a look again at this next week (end of), as I am still OOF.
On Wed, Feb 22, 2012 at 2:31 PM, Kris Wallsmith
<reply@reply.github.com>
wrote:
> @Seldaek Looks better, thanks for the changes. If `enhanceWindowsCompatibility` is going to live on `Process` we should expose the switch on the builder as well. Speaking of `enhanceWindowsCompatibility`… is there a more descriptive name for that? What exactly does that do, any why would anyone want to switch it off? The name is so vague we might as well call it `enableMagicalWindowsFix()`.
>
> ---
> Reply to this email directly or view it on GitHub:
> https://github.com/symfony/symfony/pull/3381#issuecomment-4103882
--
Pierre
@pierrejoye | http://blog.thepimp.net | http://www.libgd.org
---------------------------------------------------------------------------
by Seldaek at 2012-02-22T13:42:56Z
backupGlobals seems to be enabled by default.
As for the enhanceWindowsCompatibility, yes. It's a poor name, but no I don't have any idea for a better one, because nobody could explain me what it does. People just scream that it's necessary.
@pierrejoye: If you or anyone can conclusively confirm that this stuff is always better, then we always do it. If it's not then it must be optional, and if it's not a flag then what? The point of the component is to abstract the proc_open horrors. If people have to know about windows quirks with regard to proc_open to use it, then it's not a very useful abstraction.
Additionally, if it *is* always better to use those portability fixes, then why isn't php doing it itself?
---------------------------------------------------------------------------
by pierrejoye at 2012-02-22T13:47:02Z
On Wed, Feb 22, 2012 at 2:42 PM, Jordi Boggiano
<reply@reply.github.com>
wrote:
> backupGlobals seems to be enabled by default.
>
> As for the enhanceWindowsCompatibility, yes. It's a poor name, but no I don't have any idea for a better one, because nobody could explain me what it does. People just scream that it's necessary.
> @pierrejoye: If you or anyone can conclusively confirm that this stuff is always better, then we always do it. If it's not then it must be optional, and if it's not a flag then what? The point of the component is to abstract the proc_open horrors. If people have to know about windows quirks with regard to proc_open to use it, then it's not a very useful abstraction.
proc_open has many quirks, not only on windows. That's why it should
work and detect what is needed, that may force you to slightly change
the split between builder and process.
> Additionally, if it *is* always better to use those portability fixes, then why isn't php doing it itself?
BC, like it or not (I do not).
However we cannot change past versions, so today code has to deal it
with it anyway.
I will take a look at what you are trying to fix here next week, if
you have any other requests regarding proc_open&portability, let me
know :)
Cheers,
--
Pierre
@pierrejoye | http://blog.thepimp.net | http://www.libgd.org
---------------------------------------------------------------------------
by Seldaek at 2012-02-22T13:54:38Z
Ok so it sounds to me like the current code is correct, it tries to fix
things as best as we know how to by default, and just gives you a way to
disable things in the odd case we messed up and some of those fixes are
harmful to some use cases.
---------------------------------------------------------------------------
by fabpot at 2012-03-02T21:38:18Z
@Seldaek @kriswallsmith is it ready for merge now?
---------------------------------------------------------------------------
by kriswallsmith at 2012-03-02T21:42:22Z
I'm still not happy with the name of `enhanceWindowsCompatibility`. We need to be more specific about what that does. It sounds like a marketing term right now ;)
---------------------------------------------------------------------------
by Seldaek at 2012-03-05T13:44:56Z
Agreed, but I can't think of anything better. It is indeed esoteric magic fixes that should work better but nobody seems 100% sure about it, so I think it's fairly accurate.
Commits
-------
88ccb9b Added another corner case
7c4343f Added 4 assertions related to simple URLs containing ? and #
Discussion
----------
[Validator] added assertions related to simple URLs containing ? and #
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
This adds 5 assertions for corner cases when validating a URL containing `?` and/or `#`.
Note: this does not actually fix any bugs, it just adds a few more cases.
I hope I've sent this to the correct branch - it's not a bug fix so I've not sent it to 2.0.x, but it's not a feature either...
Commits
-------
ed8c1c0 Fixed AbstractProfilerStorageTest and some minor CS changes.
1ac581e Overwrite the profile data if the token already exists like in the other implementations.
198d406 Return profiler results sorted by time in descending order like in the other implementations.
9d8e3f2 Refactored profiler storage tests to share some code.
Discussion
----------
[WIP] Refactored profiler tests including some storage fixes
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
While refactoring the tests I came across some inconsistencies. Two of them are already fixed in this PR.
One thing left is the [MongoDbProfilerStorageTest::testCleanup()](9d8e3f2da4/tests/Symfony/Tests/Component/HttpKernel/Profiler/MongoDbProfilerStorageTest.php (L51)) test which fails in all other storage implementations. The mongodb implementation uses the `time` value from the profiler data to clean up the storage while the others additionally save a `created_at` value which is then used. For me this `created_at` value does not make any sense and I would suggest to change the other implementations to use the `time` value for cleaning up. What do you think?
---------------------------------------------------------------------------
by pulzarraider at 2012-02-27T06:55:06Z
+1 for refactoring profiler tests, I will update my RedisProfilerStorage after your changes will be merged.
---------------------------------------------------------------------------
by snc at 2012-02-28T20:05:12Z
Any suggestions about the cleanup issue?
Commits
-------
471b564 auto_start should be false
6e2a7da Support session cookie options with cookie_ prefix
e0fba80 Properly merge session cookie_* parameters
Discussion
----------
Set session.cookie_* parameters properly
Bug fix: yes
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: /
Cookie parameters in $options are not prefixed with cookie_ the same is true for data returned from session_get_cookie_params.
I've marked this as BC because the options that get dumped into the container have different name. But I don't think anybody was actually changing them or accessing them in their bundles.
P.S. @drak also desires some credits for this PR as I incorporated some lines written by him in one of the iterations.
---------------------------------------------------------------------------
by drak at 2012-02-23T14:24:42Z
@mvrhov - what does this fix exactly? It looks like a different way of doing the same thing but now there is no default value on `cookie_httponly`.
---------------------------------------------------------------------------
by mvrhov at 2012-02-23T15:09:17Z
Like I said in description. $option contains some cookie options and none of them has cookie_ prefix.
And this prefix is needed in two cases:
- to properly merge defaults and override them with what user set
- in a foreach for for proper ini_set
Sorry non native speaker an a bit hard to explain, could you ping me in a couple of hours on IRC if this still doesn't make any sense.
---------------------------------------------------------------------------
by drak at 2012-02-23T15:29:41Z
@mvrhov - I wrote some tests for this particular code and I still don't see what this PR fixes. I'll try to catch you on IRC later on but can't guarantee it.
---------------------------------------------------------------------------
by mvrhov at 2012-02-23T16:02:41Z
added test
---------------------------------------------------------------------------
by drak at 2012-02-24T08:30:51Z
Just for reference for those reading this ticket, `session_set_cookie_params()` alters the runtime ini settings it corresponds to see http://docs.php.net/manual/en/function.session-set-cookie-params.php so we agreed to remove the special handling that was present since it is redundant.
---------------------------------------------------------------------------
by dlsniper at 2012-02-28T22:19:32Z
Hi, Is this patch relevant or not after all?
ping @drak @mvrhov
Thanks :)
---------------------------------------------------------------------------
by drak at 2012-02-29T03:34:22Z
It is relevant. Maybe I'll do the cleanup this PR by forking it if @mvrhov doesn't have time.
---------------------------------------------------------------------------
by mvrhov at 2012-02-29T05:40:47Z
Fixed the typo and changed the false to ture as reported in comments. I've also rebased. I'll see what I can do about config file change later today. Sorry for the delay, been too busy for the past week.
---------------------------------------------------------------------------
by mvrhov at 2012-02-29T08:49:23Z
I've also done the config part.
---------------------------------------------------------------------------
by mvrhov at 2012-02-29T11:01:14Z
Ok, this should be it.
---------------------------------------------------------------------------
by drak at 2012-03-01T00:59:16Z
@fabpot - looks good from my side.
Commits
-------
71493a2 [DoctrineBridge] Compiler pass for registering event listeners/subscribers
f15dde6 [DoctrineBridge] ContainerAwareEventManager class
Discussion
----------
[DoctrineBridge] ContainerAwareEventManager class
```
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
```
[](http://travis-ci.org/jmikola/symfony)
This allows services to be registered (and lazily loaded) with Doctrine Common's EventManager.
It is ported from @schmittjoh's previous commits here: doctrine/DoctrineBundle#23. I'd like to integrate this with DoctrineMongoDBBundle, so the Bridge once again seemed like an ideal alternative to duplicating code.
---------------------------------------------------------------------------
by jmikola at 2012-02-23T20:37:51Z
Per conversation with @stof in doctrine/DoctrineBundle#23, I'm also going to integrate the compiler pass (an abstract version both bundles can use) into this PR.
---------------------------------------------------------------------------
by jmikola at 2012-02-23T21:56:47Z
Just realized there's an issue with the naming assumptions, since Doctrine ORM uses "doctrine" as its registry service ID but "doctrine.dbal" as its event manager prefix. Fixing.
This was imported from DoctrineBundle (see: doctrine/DoctrineBundle#23), since it can be used by other Doctrine bundles, too. It utilizes the ContainerAwareEventManager from f15dde6c59.
Commits
-------
e6e9b5a [Routing] Return the _route parameter from ApacheUrlMatcher
Discussion
----------
[Routing] Return the _route parameter from ApacheUrlMatcher
---------------------------------------------------------------------------
by fabpot at 2012-02-22T23:13:49Z
Can you squash your commits before I merge? Thanks.
---------------------------------------------------------------------------
by arnaud-lb at 2012-02-23T09:12:45Z
sure, done
Commits
-------
b269e27 [Config] Improve handling of PrototypedArrayNode defaults
4feba09 [Config] implements feedback
bc122bd [Config] Fix nested prototyped array nodes
675e5eb [Config] Take advantage of the new PrototypedArrayNode API in the core bundles
cba2c33 [Config] Improve error messages & extensibility
bca2b0e [Config] Improve PrototypedArrayNode default value management
Discussion
----------
[Config] Improve prototype nodes usability, error messages, extensibility
### First commit
*Before* (you should set multiple defalutValues)
```php
<?php
$root
->arrayNode('node')
->prototype('array')
// when the node is not set
->defaultValue(array('foo' => 'bar')
->children()
// when the key is not set
->scalarNode('foo')->defaultValue('bar')->end()
$root
->arrayNode('node')
->prototype('array')
// when the node is not set
->defaultValue(array('defaults' => array('foo1' => 'bar1', 'foo2' => 'bar2')
->children()
->arrayNode('bar')
// when the node is not set
->addDefautsIfNotSet()
// when some values are not set (node being set)
->scalarNode('foo1')->defaultValue('bar1')->end()
->scalarNode('foo2')->defaultValue('bar2')->end()
```
*after*
```php
<?php
$root
->arrayNode('node')
->addDefaultChildrenWhenNoneSet()
->prototype('array')
->children()
->scalarNode('foo')->defaultValue('bar')->end()
$root
->arrayNode('node')
->addDefaultChildrenWhenNoneSet()
->prototype('array')
->children()
->arrayNode('bar')
->scalarNode('foo1')->defaultValue('bar1')->end()
->scalarNode('foo2')->defaultValue('bar2')->end()
```
*more* (exclusive configs)
```php
<?php
$root
->arrayNode('node')
// Add a default node named 'defaults'
->addDefaultChildrenWhenNoneSet()
// Add a default node named 'foo'
->addDefaultChildrenWhenNoneSet('foo')
// Add two default nodes named 'foo', 'bar'
->addDefaultChildrenWhenNoneSet(array('foo', 'bar'))
// Add two default nodes
->addDefaultChildrenWhenNoneSet(2)
```
### Second commit
Improves error messages (print the path to the error) & extensibility.
@schmittjoh I would appreciate you feedback on both the commits. Do you think a boolean $throw switch on `getNode` would make sense (i.e. to prevent throwing excs in prod ?).
---------------------------------------------------------------------------
by schmittjoh at 2012-02-20T15:43:18Z
The error improvements seem uncontroversial.
I'm not so convinced by the other changes though. What if the prototype is a map and not a simple list?
---------------------------------------------------------------------------
by vicb at 2012-02-20T16:07:51Z
I think there's one caveat left in the code as it is now that I will fix (nested prototypes).
Could you please give me more details on the use case you are referring to ?
You do not have to use the new feature but It can be really helpful [here](https://github.com/symfony/symfony/pull/3225/files#L4R38) for example.
---------------------------------------------------------------------------
by schmittjoh at 2012-02-20T17:20:02Z
What I mean is something like this:
```php
->arrayNode("foo")
->useAttributeAsKey("name")
->prototype(/* ...
```
---------------------------------------------------------------------------
by vicb at 2012-02-20T17:28:01Z
What would be wrong then ? (that's the use case I link in my previous msg)
---------------------------------------------------------------------------
by schmittjoh at 2012-02-20T17:28:55Z
How would adding defaults look like?
---------------------------------------------------------------------------
by vicb at 2012-02-20T17:36:35Z
Check the "more" part of the PR message.
In the linked use case, it would add a "defaults" server using the default host / port / weight. In this case I do not care about the name but the values are important to help alias the equivalent configs. You can override the "defaults" name by using a parameter.
---------------------------------------------------------------------------
by vicb at 2012-02-20T17:47:27Z
```php
<?php
// [...]
->arrayNode('servers')
->addDefaultChildrenWhenNodeSet()
->useAttributeAsKey('name')
->prototype('array')
->children()
```
---------------------------------------------------------------------------
by schmittjoh at 2012-02-20T17:47:54Z
What I was thinking about is having two nodes with different default values. Right now, both nodes while having different keys would still have the same default values which does not make much sense to me. However, we can address this in another PR.
One thing that we should fix though is that we should require keys in case of a map, and forbid them in case of a list. It might make sense to split it into different methods. Like the following examples make no sense (but are possible atm):
```php
->arrayNode("foo")
->useAttributeAsKey("name")
->addDefaultChildrenIfNotSet(5)
->arrayNode("foo")
->addDefaultChildrenIfNotSet("foo")
->prototype("scalar")->end()
```
Another minor nitpick, please rename "when" to "if".
---------------------------------------------------------------------------
by vicb at 2012-02-20T18:03:19Z
@schmittjoh thank you for your feedback.
message-2:
* I think the first case is fine (children "1" to "5"). Sometimes you just don't care about the names so it should not be forbidden.
* I also think the second case is fine as you would write `foo: value` in your config file anyway.
Let me know your thoughts about the previous statements.
Agree to change when to if.
message-1:
Will change
---------------------------------------------------------------------------
by vicb at 2012-02-20T18:06:33Z
I think "IfNoneSet" is more accurate than "IfNotSet" ?
---------------------------------------------------------------------------
by schmittjoh at 2012-02-20T18:09:59Z
If you call "useAttributeAsKey" it automatically means that the keys are meaningful to you (otherwise there is no point in calling it). In such a case, keys should be explicitly given.
On the other hand, if you do not call it, then the keys are ignored/dropped by the Config component. So if you give a key, it is an obvious error that we should catch. The second case I linked would look like ``foo: [value]`` in contrast to ``foo: { foo: value }``.
---------------------------------------------------------------------------
by schmittjoh at 2012-02-20T18:14:44Z
I'm not feeling strongly about this, but "IfNotSet" is more consistent with
"addDefaultsIfNotSet" and basically reads as "if array node is not set, do
...". Your example would refer to the children and read as "if none
(children) have been defined, do ...".
On Mon, Feb 20, 2012 at 12:06 PM, Victor Berchet <
reply@reply.github.com
> wrote:
> I think "IfNoneSet" is more accurate than "IfNotSet" ?
>
> ---
> Reply to this email directly or view it on GitHub:
> https://github.com/symfony/symfony/pull/3403#issuecomment-4058579
>
---------------------------------------------------------------------------
by vicb at 2012-02-20T18:30:21Z
message-2:
* Agree on first point, will change
* You could specify the keys in your config file if the prototype is an array (you used a scalar). Should we implement a switch in the validation (i.e. array / not array) or just go with numeric / null arg as you suggest ?
message-1:
> Your example would refer to the children and read as "if none (children) have been defined, do ..."
QED
---------------------------------------------------------------------------
by vicb at 2012-02-20T22:11:05Z
@schmittjoh I have implemented your suggestions (other than the "NoneSet"). Let me know if you think this is ok. Thanks.
---------------------------------------------------------------------------
by schmittjoh at 2012-02-21T03:24:19Z
Looks good to me.
As an additional improvement we might consider to allow to prepopulate an prototyped with values. For example, in the FOSRestBundle there is a case where this could be used.
```php
->arrayNode('formats')
->prepopulateValues(array('application/json' => 'json', 'application/xhtml+xml' => 'xml'))
->useAttributeAsKey('name')
->prototype('scalar')->canBeUnset()->end()
```
This could be done in a separate PR however and is not strictly related to these improvements.
---------------------------------------------------------------------------
by vicb at 2012-02-21T07:51:59Z
@schmittjoh that would be a great addition but I think need some thinking (i.e. the name, `initialValues` ?, should we handle duplicates, how - in case we are not using attribue as key, ...) so let's make an other PR, I'd like this one to be merged asap as I need this for the Cache Bundle.
@fabpot ready
Commits
-------
fb2bb65 [HttpFoundation] Fix session.cache_limiter is not set correctly
Discussion
----------
[HttpFoundation] Fix session.cache_limiter is not set correctly
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
Fixes a regression after the session refactoring where extra cache control http headers are sent.
This was previously handled by [calling session_cache_limiter(false) in NativeSessionStorage](https://github.com/symfony/symfony/blob/2.0/src/Symfony/Component/HttpFoundation/SessionStorage/NativeSessionStorage.php#L81)
---------------------------------------------------------------------------
by drak at 2012-02-21T12:23:48Z
@fabpot - this code can be merged imo.
Commits
-------
6fbd290 Improved unit tests for MemcacheSessionStorage
b4c5323 Added comma to array initializer, reverted permissions back to 644
3dd851a Use correct parameters
0e01418 Fix default if no serverpool is provided
2a65121 Fix several issues in MemccheSessionStorage which prevented it from being used correctly
Discussion
----------
Fix several issues in MemcacheSessionStorage
Apperently this could never have worked unless someone passed wrong arguments to the options.
---------------------------------------------------------------------------
by mazen at 2012-02-19T07:58:52Z
```
[marcel@development symfony]$ phpunit tests/Symfony/Tests/Component/HttpFoundation/Session/Storage/MemcacheSessionStorageTest.php
PHPUnit 3.6.10 by Sebastian Bergmann.
Configuration read from /www/includes/vendor/symfony/phpunit.xml.dist
......
Time: 0 seconds, Memory: 3.75Mb
OK (6 tests, 11 assertions)
```
---------------------------------------------------------------------------
by lsmith77 at 2012-02-19T16:10:13Z
cc @drak
---------------------------------------------------------------------------
by drak at 2012-02-19T17:44:00Z
Looks like we could do with some tests for the constructor that also test the defaults and the internal properties. And also more extensively tests the mock to test the addServer behaviour.
---------------------------------------------------------------------------
by helmer at 2012-02-19T18:02:03Z
@mazen You've changed file permissions from 644->755 ..
---------------------------------------------------------------------------
by drak at 2012-02-21T12:25:11Z
@fabpot - with the extra tests added in 6fbd290 I believe this code is ready for merge.
Commits
-------
0a176eb [FrameworkBundle] Fix configuration errors
6745b28 [Config] Throw exceptions on invalid definition
fb27de0 [Config] cleanup
Discussion
----------
[Config] Cleanup, error detection, fixes
see #3357
---------------------------------------------------------------------------
by stloyd at 2012-02-15T10:56:00Z
@vicb As you added new exceptions, IMO you should add some tests to cover it.
---------------------------------------------------------------------------
by vicb at 2012-02-15T10:56:50Z
good point, I'll do.
---------------------------------------------------------------------------
by vicb at 2012-02-15T13:49:44Z
@stloyd that was a great idea, I realized I had miss a case. It has been added and should be covered by UT + fixes made.
I am done with the fixes, should be ready to merge.
And time to give the `PrototypedArrayNode` some more usability now.
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: -
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).
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
-------
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
-------
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
-------
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'
