Commits
-------
dd4d46a add limit to logger explosion
Discussion
----------
add limit to logger explosion
This limit is required to display complete query with e.g. "array" type in it.
ping @willdurand
Commits
-------
d243097 Run built-in server on dev environment
Discussion
----------
Run built-in server on dev environment
Bug fix: yes?
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Change the router of the built-in server command to run on dev environment.
The symfony standard edition doesn't have any `/` route by default (it's only available to dev), so by default, when ran, it gives a `404`, unless you explicitely add the `app_dev.php` front controller to the route.
Also, this server is meant to be run on dev only, so no need to run it with the prod front controller by default.
Commits
-------
cdba4cf [FrameworkBundle] Change XSD to allow string replacements on session args.
52f7955 [FrameworkBundle] Remove default from gc_* session configuration keys.
749593d [FrameworkBundle] Allow configuration of session garbage collection for session 'keep-alive'.
Discussion
----------
[2.1][FrameworkBundle] Allow configuration of session garbage collection
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #2171
Todo: -
---------------------------------------------------------------------------
by drak at 2012-03-21T21:56:20Z
@fabpot - this PR is ready for merge. It basically allows configuration of some session ini values that are necessary in controlling the session behaviour.
---------------------------------------------------------------------------
by dlsniper at 2012-03-21T22:57:18Z
@drak shouldn't all the options here: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php#L266 be available for configuration, or am I just reading the source wrong and they already are?
In this case should I make a separate PR to cover the rest or could you do it in this one?
---------------------------------------------------------------------------
by fabpot at 2012-03-23T14:56:22Z
@drak: the discussion is the ticket is very interesting and I think it should be part of a cookbook in the documentation. Can you take care of that before I merge this PR? Thanks.
---------------------------------------------------------------------------
by drak at 2012-03-25T15:32:59Z
@fabpot - yes - it's on the todo list. Will update this PR when done.
---------------------------------------------------------------------------
by drak at 2012-03-26T19:45:13Z
@fabpot - this is ready for merging, the documentation is done (the PR is in but I'll tweak it, but no need to wait to merge this PR). I will also add something extra to cookbook (I wrote docs for the component).
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
-------
b718960 HttpFoundation\HeaderBag Little improvement.
Discussion
----------
[HttpFoundation\HeaderBag] Removed unnecessary anonymous function
---------------------------------------------------------------------------
by vicb at 2012-03-24T16:07:00Z
Related issue: #3294
Commits
-------
10947cb [DoctrineBridge][Security] Fixes bug that prevents repository's refreshUser from being called
Discussion
----------
[Security][DoctrineBridge] Fixes bug that prevents repository's refreshUser from being called
---------------------------------------------------------------------------
by marcw at 2012-02-21T08:46:09Z
Updated. What do you guys think about this patch ?
---------------------------------------------------------------------------
by henrikbjorn at 2012-02-21T08:57:47Z
Isnt this a bit dangerous, the custom repository implementing refreshUser should always be called first right? You wouldnt specify the $property property if your class has custom implementations would you?
---------------------------------------------------------------------------
by marcw at 2012-02-21T09:05:08Z
@henrikbjorn At this time, the refreshUser method is never called from the custom repository, even if you don't specify the "property" property. This patch fixes this.
---------------------------------------------------------------------------
by marcw at 2012-02-21T09:44:06Z
Updated & Squashed.
---------------------------------------------------------------------------
by stof at 2012-02-21T10:03:33Z
@marcw please move the retrieval of the id in the ``else`` block, like in my comment as it is useless to do this logic for the case where the userProviderInterface is implemented (and it will answer to @vicb by making it impossible to write it with elseif)
---------------------------------------------------------------------------
by marcw at 2012-02-21T10:19:06Z
I'm not sure about this, but Isn't the check of the id essential here to ensure that the entity is a persisted one ?
---------------------------------------------------------------------------
by stof at 2012-02-21T10:21:55Z
@marcw if the interface is used, it means that the user wants to do the work himself. So you should really let him do the way he wants. If he does not use the id to refresh the user, he could choose not to include it in the serialized data.
Retrieving the id is needed for the ``find()`` call because we pass the id as argument and so we fail when the serialized data don't contain it
---------------------------------------------------------------------------
by marcw at 2012-02-21T10:33:30Z
@stof Roger that. I'll do the fix.
---------------------------------------------------------------------------
by marcw at 2012-02-21T10:41:58Z
Updated & Squashed, again.
---------------------------------------------------------------------------
by stof at 2012-02-21T11:00:44Z
btw, to answer to your previous question, the exception when retrieving the id does not check if the object is persisted (you need to reach teh DB for this, which is what find() does) but that the id is part of the serialized data to give a better error reporting.
---------------------------------------------------------------------------
by fabpot at 2012-03-07T19:39:33Z
ready to be merged now?
---------------------------------------------------------------------------
by henrikbjorn at 2012-03-08T07:21:37Z
would say so.
---------------------------------------------------------------------------
by dlsniper at 2012-03-25T11:58:34Z
Hi, can this be merged now or not?
Commits
-------
57de69f added an exception for failed processes
Discussion
----------
added an exception for failed processes
---------------------------------------------------------------------------
by Seldaek at 2012-03-19T07:27:56Z
So this is just there to use if you want to throw an exception when a process call failed in your application? It doesn't seem enabled by default, which I think is good anyway.
---------------------------------------------------------------------------
by stof at 2012-03-19T07:44:43Z
@Seldaek yeah, I guess this is a way to make it easier to reuse what he implemented for Assetic first.
---------------------------------------------------------------------------
by fabpot at 2012-03-23T15:08:26Z
How and when do you use such an exception?
---------------------------------------------------------------------------
by schmittjoh at 2012-03-23T17:22:16Z
It's intended for your own code to give you a nice and meaningful error message without having to repeat the same code whereever you are dealing with a Process:
```php
if (0 !== $proc->run()) {
throw new ProcessFailedException($proc);
}
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
-------
3f2b917 added a configurable extension base class
Discussion
----------
added a configurable extension base class
This is mostly a convenience class which provides first-class integration with the Config/Definition component.
Commits
-------
df11e62 [FrameworkBundle] Used $output->write() instead of echo
c3bf479 [FrameworkBundle] Used Process component
cfa2dff [FrameworkBundle] Changed server:run command description
e7d38c1 [FrameworkBundle] Changed PHP version detection (see: #3529)
4a3f6d5 [FrameworkBundle] Removed global variable from router script
519d431 [FrameworkBundle] Fixed built-in server router script
d9a0a17 [FrameworkBundle] Added server:run command
Discussion
----------
[FrameworkBundle] Added server:run command (PHP 5.4 built-in web server)
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: [](http://travis-ci.org/michal-pipa/symfony)
Fixes the following tickets: -
Todo: -
PHP 5.4 comes with [built-in web server](http://www.php.net/manual/en/features.commandline.webserver.php). I've created command which allows to easily run Symfony2 application using this new feature.
Usage:
server:run [-d|--docroot="..."] [-r|--router="..."] [address]
Arguments:
address Address:port (default: 'localhost:8000')
Options:
--docroot (-d) Document root (default: 'web/')
--router (-r) Path to custom router script
Help:
The server:run runs Symfony2 application using PHP built-in web server:
app/console server:run
To change default bind address and port use the address argument:
app/console server:run 127.0.0.1:8080
To change default docroot directory use the --docroot option:
app/console server:run --docroot=htdocs/
If you have custom docroot directory layout, you can specify your own
router script using --router option:
app/console server:run --router=app/config/router.php
See also: http://www.php.net/manual/en/features.commandline.webserver.php
It requires PHP 5.4, otherwise this command will be disabled.
I think that this is very convenient (especially for new users). All you have to do is download Symfony, install vendors and run this command. You don't have to configure "real" web server, in fact any other server is not required. You don't have cache and logs permission problem, because server runs with your local user permissions.
---------------------------------------------------------------------------
by blogsh at 2012-03-06T17:38:10Z
Great feature! I was about to write something like this when I saw that you have already started implementing this :)
Some issues:
1. Missing newlines at the end of the files
2. If I try this server command with the default Symfony Standard Edition Acme demo the links on the main page do not work. The demo link links to "//demo" and the configurator link to "//_configurator". If I go to `localhost:8000/demo` directly the page is rendered as usual and all sub links are generated correctly. I could solve the problem by adding one line:
$_SERVER['SCRIPT_FILENAME'] = 'ANYTHING';
require 'app_dev.php';
I'm not sure where this problem comes from. Do you experience the same behaviour? Otherwise I'll do some more investigations to find the source of the problem.
3 . I think it would be a nice feature if you would generate a router.php based on the setting of the --env flag if no custom router file has been specified. This way it would be easy to switch between dev and prod.
---------------------------------------------------------------------------
by michal-pipa at 2012-03-06T19:00:24Z
@blogsh
> Missing newlines at the end of the files
I've checked and I can see newlines at the end of files. Are you sure about this?
> If I try this server command with the default Symfony Standard Edition Acme demo the links on the main page do not work. The demo link links to "//demo" and the configurator link to "//_configurator". If I go to localhost:8000/demo directly the page is rendered as usual and all sub links are generated correctly. I could solve the problem by adding one line:
>
> $_SERVER['SCRIPT_FILENAME'] = 'ANYTHING';
> require 'app_dev.php';
>
> I'm not sure where this problem comes from. Do you experience the same behaviour? Otherwise I'll do some more investigations to find the source of the problem.
I can reproduce this by changing front controller name from `app.php` to `app_dev.php`. I'll investigate on this.
> I think it would be a nice feature if you would generate a router.php based on the setting of the --env flag if no custom router file has been specified. This way it would be easy to switch between dev and prod.
You can easily change environment specifying front controller in URL. It works exactly the same way as default Apache configuration. This is intended behavior, as it would be misleading if every server had different rewrite rules.
If you really want to change it, then you can write your own router and pass it as a value to `router` option.
---------------------------------------------------------------------------
by blogsh at 2012-03-06T19:13:55Z
Wasn't aware that github omits the trailing white line, sorry.
Normally I use a rather inflexible nginx configuration, so I also wasn't aware of this (rather obvious) trick of changing the url. Thanks for that.
---------------------------------------------------------------------------
by stof at 2012-03-06T22:12:16Z
@blogsh it does not omit it. It displays it in the Linux way where the newline char is part of the line (and so there is a message ``no newline at end of file`` in the diff when it is missing).
---------------------------------------------------------------------------
by michal-pipa at 2012-03-07T07:18:23Z
@blogsh I've fixed router script. Now you can use both front controllers.
---------------------------------------------------------------------------
by michal-pipa at 2012-03-07T07:34:58Z
I've also hardcoded front controller name in router script and removed global variable, as there was no way to unset it.
---------------------------------------------------------------------------
by michal-pipa at 2012-03-13T07:57:04Z
I've used Process component, but now I don't get any stdout output (only stderr).
---------------------------------------------------------------------------
by michal-pipa at 2012-03-13T18:01:58Z
I've replaced `echo` by `$output->write()` and removed `$process` as it was not used actually.
Commits
-------
0c9b2d4 use SecurityContextInterface instead of SecurityContext
Discussion
----------
[2.0][Security] use SecurityContextInterface instead of SecurityContext
see https://github.com/symfony/symfony/pull/3522 (this is a fix for the 2.0 branch)
---------------------------------------------------------------------------
by pminnieur at 2012-03-21T13:25:59Z
*ping* it still missed the 2.0.12 release ...
---------------------------------------------------------------------------
by stof at 2012-03-21T16:41:28Z
@pminnieur you PR has been merged into master, not into 2.0, so it will only be in 2.1
---------------------------------------------------------------------------
by pminnieur at 2012-03-21T16:43:02Z
I know, and this is a second PR for 2.0 branch.
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.
Commits
-------
1422133 [TwigBundle] Made docblock for findTemplate() more general and accurate
5910ac9 [TwigBundle] Added a use statement to shorten class name in a docblock
3e7eebd [TwigBundle] Improved ExceptionController docblocks
Discussion
----------
[TwigBundle] Improved ExceptionController docblocks
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: [](http://travis-ci.org/lencioni/symfony)
Fixes the following tickets: -
Todo: -
---------------------------------------------------------------------------
by lencioni at 2012-03-21T20:47:16Z
I obviously don't know what I'm doing here. :/
---------------------------------------------------------------------------
by vicb at 2012-03-21T20:47:39Z
no pb just rebase on master and force push
Commits
-------
f9f51a5 fixed cs
af65673 [Process] Added support for non-blocking process control 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
Discussion
----------
[Process] Added support for non-blocking process control
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes [](http://travis-ci.org/drak3/symfony)
Fixes the following tickets: #1049
Todo: -
Added methods to control long running processes (as described in issue #1049) 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
---------------------------------------------------------------------------
by Seldaek at 2012-03-23T10:52:30Z
Overall this seems like a good improvement. I didn't check the code in detail though.
---------------------------------------------------------------------------
by drak3 at 2012-03-23T11:21:45Z
@stof @Seldaek thanks, fixed
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
-------
da3a2c4 [Process] Fix command escaping
Discussion
----------
[Process] Fix command escaping
My bad, I misunderstood what escapeshellcmd was good for. This fixes it by properly escaping all args.
The test suite hangs here but it already did before, so I'm not sure if all tests pass.
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
-------
00ae766 [FrameworkBundle] added new french validators translations for the File constraint.
Discussion
----------
[FrameworkBundle] added new french validators translations for the File ...
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
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.
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: [](http://travis-ci.org/lencioni/symfony)
Fixes the following tickets: -
Todo: -
Relying on decrementing a counter has two problems. First, and most importantly, if the output buffering nesting level is greater than the counter, the function does not perform the expected task. Secondly, on systems where the counter is needed, a lot of unnecessary extra loops would potentially occur.
This approach checks to see if the level has stayed the same from the previous iteration and if it has it stops looping.
Commits
-------
fc7c7f6 [Form] Fix min/max length guessing for numeric types (fix#3091)
Discussion
----------
[Form] Fix min/max length guessing for numeric types (fix#3091)
Before this PR, the length was guessed from `strlen(min/max)`.
This is obviously false for float: `strlen("1.123") > strlen ("5")` then this guess is now low confidence only and is masked by a `null` medium confidence guess for floats (implemented in both doctrine ORM & validator).
This PR also includes some code reorg in order to improve readability.
I'll update Propel & Mongo if needed once this is merged.
_note: `5.000` did neither work because of `5e3`_
---------------------------------------------------------------------------
by Koc at 2012-03-19T23:42:01Z
Will `strlen` works correctly with multibyte strings?
---------------------------------------------------------------------------
by vicb at 2012-03-19T23:58:33Z
could numeric types be multibyte strings ?
---------------------------------------------------------------------------
by Koc at 2012-03-20T00:07:24Z
I thought it somehow concerns `Symfony\Component\Validator\Constraints\MaxLengthValidator` too.
---------------------------------------------------------------------------
by vicb at 2012-03-20T00:20:33Z
This PR is about numeric types only and the MaxLengthValidator is [multibyte safe:](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Validator/Constraints/MaxLengthValidator.php#L45)
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
-------
8642473 Changed instances of \DateTimeZone::UTC to 'UTC' as the constant is not valid a produces this error when DateTimeZone is instantiated: DateTimeZone::__construct() [<a href='datetimezone.--construct'>datetimezone.--construct</a>]: Unknown or bad timezone (1024)
Discussion
----------
[Locale] DateTimeZone called incorrectly by default
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: no (there were two tests that were failing previously, that still fail)
Fixes the following tickets: none
Todo: none
While running, a warning throws every single time when the code
```php
new \DateTimeZone(\DateTimeZone::UTC);
```
is encountered. It is normally caught as a thrown exception and then corrected here:
```php
// src/Symfony/Component/Locale/Stub/StubIntlDateFormatter.php:442
try {
$this->dateTimeZone = new \DateTimeZone($timeZoneId);
} catch (\Exception $e) {
$this->dateTimeZone = new \DateTimeZone('UTC');
}
```
However in my particular infrastructure, for whatever reason in production only, it causes an error to appear on shutdown in the logs. As ultimately the constant can NEVER pass, it should not be attempted with the constant. Instead, the correct 'UTC' should be passed in (as done in the catch statement).
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
-------
e6577de Added a 'post validation' event to the form component.
Discussion
----------
[Form] Add post-validate event
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: n/a
Fixes the following tickets: n/a
Todo: n/a
---------------------------------------------------------------------------
by fabpot at 2012-03-02T20:34:18Z
ping @bschussek
---------------------------------------------------------------------------
by vicb at 2012-03-04T09:19:53Z
I think this is a good idea (It was something missing to properly handle PersistentFile i.e. you should not persist invalid files)
---------------------------------------------------------------------------
by vicb at 2012-03-09T22:35:26Z
@jankramer please remove the second commit from this PR (see http://symfony.com/doc/current/contributing/code/patches.html) in order to make this mergeable.
---------------------------------------------------------------------------
by jankramer at 2012-03-10T09:26:04Z
@vicb done, sorry about that commit: overlooked the fact that it was on the same branch...
Commits
-------
eee5065 [TwigBundle] Workaround a flaw in the design of the configuration (normalization)
Discussion
----------
[TwigBundle] Workaround a flaw in the design of the configuration (norma...
...lization)
see #2823
@Seldaek please comment.
---------------------------------------------------------------------------
by Seldaek at 2012-03-09T20:52:47Z
It seems fine at first glance. I don't have time to look at it in detail right now sorry.
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
-------
0e4f789 changed test config
a98d554 [SecurityBundle] Allow switching to the user that is already impersonated (fix#2554)
Discussion
----------
[Security] Disabled exception when switching to the user that is already impersonated
Bug fix: yes-ish
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #2554
Todo: -
---------------------------------------------------------------------------
by vicb at 2012-03-13T14:31:45Z
@meandmymonkey thank you for your work on this issue. Would you have time to add functional tests ?
---------------------------------------------------------------------------
by meandmymonkey at 2012-03-13T14:49:52Z
Probably not today, but during the next few days, yes, of course.
---------------------------------------------------------------------------
by meandmymonkey at 2012-03-14T18:05:19Z
@vicb @schmittjoh Writing the tests I noticed switching to an non-existent user will not raise an exception. While it's not a security issue, it should raise an error for completeness sake, shouldn't it?
---------------------------------------------------------------------------
by vicb at 2012-03-14T20:28:52Z
I think it should (throw an `AuthenticationCredentialsNotFoundException`). _btw there is an extra `sprintf` in the original code that could be remove when attempting to exit_
---------------------------------------------------------------------------
by meandmymonkey at 2012-03-14T21:13:16Z
The problem with throwing an `AuthenticationCredentialsNotFoundException` (or any other security exception for that matter) is that it derives from `AuthenticationException`, which means it gets caught by the framework and redirects to the login form, which is not what we want in this case.
We need to throw something 500-ish at [L89](d40b3376ec/src/Symfony/Component/Security/Http/Firewall/SwitchUserListener.php (L89)), either a generic or a (new) custom Exception.
---------------------------------------------------------------------------
by meandmymonkey at 2012-03-14T21:43:57Z
IMHO a `LogicException`would be fine, like the one used at [L117](d40b3376ec/src/Symfony/Component/Security/Http/Firewall/SwitchUserListener.php (L117)), as the error is not really about a failed authentication.
---------------------------------------------------------------------------
by vicb at 2012-03-14T21:49:04Z
I agree and btw very good job on the tests !
---------------------------------------------------------------------------
by meandmymonkey at 2012-03-14T22:12:43Z
Thanks :)
---------------------------------------------------------------------------
by vicb at 2012-03-15T08:01:13Z
Could you squash the commits, prefix the commit message with `[SecurityBundle]` and add `(fix#2554)` at the end ?
---------------------------------------------------------------------------
by meandmymonkey at 2012-03-15T08:53:12Z
Done.
---------------------------------------------------------------------------
by vicb at 2012-03-15T09:19:09Z
@fabpot this PR looks good to me.
---------------------------------------------------------------------------
by fabpot at 2012-03-15T12:50:50Z
Tests do not pass when you run them all.
---------------------------------------------------------------------------
by meandmymonkey at 2012-03-15T13:41:45Z
@fabpot @vicb With this config change, they pass when run together.
What is weird though is that the reason seems to be that the config for the profiler gets overwritten when running all tests together, while being used correctly when run alone. Any idea what can cause this? They should be isolated from each other.
The new config from 0e4f789 works, but enables the profiler for all SecurityBundle Tests... which is not strictly necessary.
Disabled exception when switching to the user that is already impersonated, exception is now only thrown when trying to switch to a new user.
Added an Excption exception when switching fails because target user does not exist.
Added funtional tests for switching users.
Commits
-------
eb9bf05 [HttpFoundation] Remove hard coded assumptions and replace with API calls.
9a5fc65 [HttpFoundation] Add more tests.
68074a2 Changelog and upgrading changes.
7f33b33 Refactor SessionStorage to NativeSessionStorage.
b12ece0 [HttpFoundation][FrameworkBundle] Separate out mock session storage and stop polluting global namespace.
d687801 [HttpKernel] Mock must invoke constructor.
7b36d0c [DoctrineBridge][HttpFoundation] Refactored tests.
39526df [HttpFoundation] Refactor away options property.
21221f7 [FrameworkBundle] Make use of session API.
cb873b2 [HttpFoundation] Add tests and some CS/docblocks.
a6a9280 [DoctrineBridge] Refactor session storage to handler.
a1c678e [FrameworkBundle] Add session.handler service and handler_id configuration property.
1308312 [HttpFoundation] Add and relocate tests.
88b1170 [HttpFoundation] Refactor tests.
2257a3d [HttpFoundation] Move session handler classes.
0a064d8 [HttpFoundation] Refactor session handlers.
2326707 [HttpFoundation] Split session handler callbacks to separate object.
bb30a44 [HttpFoundation] Prepare to split out session handler callback from session storage.
Discussion
----------
[2.1] Support PHP 5.4 \SessionHandler
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
This patch allows us to add services, like an encryption layer into any session handler without having to alter or inherit any code across any session handler, internal or custom.
The `\SessionHandler` class exposes internal PHP's native internal session save handlers like files, memcache, and sqlite by wrapping the internal callbacks through the class giving user-space the chance to intercept, override and filter them by inheriting from `\SessionHandler`. I've written a pretty nice use-case at http://docs.php.net/sessionhandler which really shows the power of it. I never considered how to make proper use of the `\SessionHandler` in Symfony2 until I wrote the code example you see in that documentation and also because of the `AbstractSessionStorage` base class got in the way.
It's really trivial to enable support for this in Symfony2 but requires to separate out the actual handlers because inheritance is not suitable.
Obviously, the feature will only work with internal PHP-extension provided handlers under PHP 5.4 and will already work in PHP 5.3 with any custom handler (since they all implement `\SessionHandlerInterface`). Symfony2 will also be the first framework to support these amazing features :-D
The necessary changes are really small but beautiful:
The basic idea is this: 1d55d1ff14 removed inheritance and separates out the actual session handler callbacks - the part PHP processes internally.
This is supported by an internal proxy mechanism: 10a36c901e
In terms of BC, not much changes net from 2.0:
- We can restore the deprecated service ID: `session.storage.native`
- We add a new service ID `session.handler` (and configuration alias `handler_id`) for the actual session handlers. This defaults to the renamed `session.handler.native_file` session handler (same behaviour just new name and as it's a default there is no BC break).
---------------------------------------------------------------------------
by fabpot at 2012-03-03T12:15:10Z
Looks good to me. Can you update the CHANGELOG and UPGRADE file accordingly and start to update the documentation at symfony/symfony-docs? Thanks for your work, the session handling in Symfony2 is starting to become amazing!
---------------------------------------------------------------------------
by drak at 2012-03-04T11:09:31Z
@fabpot I will start working on documentation this week and get the CHANGELOG/UPGRADING committed shortly. I'll ping when done.
---------------------------------------------------------------------------
by drak at 2012-03-14T16:48:37Z
@fabpot - This PR is ready now.
It does not make sense to try and store session ini directives since they can be changes outside
of the class as they are part of the global state.
Coding stan
Revert service back to session.storage.native
Rename session.storage.native_file to session.handler.native_file (which is the default so no BC break from 2.0)
Commits
-------
17c3482 fixed timezone bug in DateTimeToTimestampTransformer
Discussion
----------
[FIX]fixed timezone bug in DateTimeToTimestampTransformer
After several trials, I found out that the original code
```php
$dateTime = new \DateTime(sprintf("@%s %s", $value, $this->outputTimezone));
```
would create a DateTime object with timezone being '0000', even though $this->outputTimezone is set to my local timezone.
so I expanded the code a bit and it's working now.
PHP Test code,
```PHP
$d = new DateTime("@1234567890 Asia/Tokyo");
echo date_format($d, 'Y/m/d H:i:s')."\n";
echo $d->getTimezone()->getName()."\n";
$d = new DateTime("now Asia/Hong_Kong");
echo date_format($d, 'Y/m/d H:i:s')."\n";
echo $d->getTimezone()->getName()."\n";
```
The output is as followed:
2009/02/13 23:31:30
+00:00
2012/03/13 03:35:55
Asia/Hong_Kong
This could be a bug of PHP,
---------------------------------------------------------------------------
by stealth35 at 2012-03-13T15:54:31Z
👍
Commits
-------
93cc9ef [Validator] Remove a race condition in the ClassMetaDataFactory (fix#3217)
Discussion
----------
[Validator] Remove a race condition (fix#3217)
#3581 for 2.0
Commits
-------
14a18ae [WebProfilerBundle] Optimized toolbar and profiler icons with optiPNG
Discussion
----------
[WebProfilerBundle] Optimized toolbar and profiler icons with optiPNG
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Optimized web toolbar and profiler icons (pngs) to slightly reduce PNG sizes. Lossless compression.
Commits
-------
aa53b88 Sets _format attribute only if it wasn't set previously by the user
Discussion
----------
Sets _format attribute only if it wasn't set previously by the user.
Fixes#2653
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
-------
99079ba Very small semantic changes improving understanding and readability.
Discussion
----------
Very small semantic changes improving understanding and readability.
The "may or may not" change may seem pedantic but it quantifies the use of the field; obviously a boolean is true or not but "may not be empty" made me wonder about it's intent so clarification seemed appropriate.
Change "return" to "returns" as the rest of the code in the class uses this syntax.
Change "contains" to "contain" in an exception message.
Commits
-------
919eee4 [Security] Regenerated the ACL SQL schema with the latest Doctrine version
Discussion
----------
[Security] Regenerated the ACL SQL schema with the latest Doctrine version
This regenerates the SQL schemas for all platforms supported by Doctrine as some changes were made in the DBAL code since the previous run of the script and a new platform has been added.
Commits
-------
ca70a35 [FrameworkBundle] Return Event
876cf96 [EventDispatcher] Add fluid interface on dispatch()
Discussion
----------
[2.1][EventDispatcher] Add fluid interface on dispatch()
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
This patch allows for code like the following:-
$response = $dispatcher->dispatch('foo', new FooEvent())->getResponse();
and
if ($dispatcher->dispatch('foo')->isStoppedPropagation()) {
// ...
}
Commits
-------
c8e74da [DoctrineBridge] Iterator->current() is not the same as current(Iterator)
Discussion
----------
[DoctrineBridge] Iterator->current() is not the same as current(Iterator)
More lively discussion from: doctrine/DoctrineMongoDBBundle#84.
The HTTP status code translation table was updated to include all HTTP status codes as defined by the IANA Hypertext Transfer Protocol (HTTP) Status Code Registry (http://www.iana.org/assignments/http-status-codes/).
Commits
-------
bfb5547 fixed docblock
bf75212 use SecurityContextInterface instead of SecurityContext
498b4b6 use SecurityContextInterface instead of SecurityContext
Discussion
----------
use SecurityContextInterface instead of SecurityContext
Bug fix: no
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: /
Todo: /
Abstract: it's not possible to exchange the `security.context` with another implementation without this change. You may not be able to extend the `SecurityContext` because `isGranted` is final, so you may implement your own context.
---------------------------------------------------------------------------
by pminnieur at 2012-03-06T17:37:27Z
PS: could you merge this back to 2.0 branch, too?
---------------------------------------------------------------------------
by stof at 2012-03-06T17:42:03Z
@pminnieur send a pull request to the 2.0 branch then
---------------------------------------------------------------------------
by lsmith77 at 2012-03-06T18:42:41Z
i guess this doesn't break BC as SecurityContext always implemented the SecurityContextInterface .. no?
---------------------------------------------------------------------------
by pminnieur at 2012-03-06T19:11:00Z
this would not break BC, correct. I may identify additonal places where its not typed against the Interface but the implementation, which is really annoying. I will update the PR tomorrow morning and also do a PR for the 2.0 branch.
---------------------------------------------------------------------------
by stof at 2012-03-06T22:04:09Z
As it is in the constructor, it is not a BC break indeed as overwritten constructors can have a different signature anyway. For other places, take care that it could be a BC issue for people extending the class
---------------------------------------------------------------------------
by pminnieur at 2012-03-06T22:11:28Z
as the `isGranted ` method in the `SecurityContext ` implementation provided by Symfony is declared `final`, it's not really extendable at all - which ultimately leads to the problem: its indirectly hard coupled ;-)
---------------------------------------------------------------------------
by stof at 2012-03-06T22:38:08Z
@pminnieur the BC break is not for people extending the SecurityContext but for people extending classes that typehint it
---------------------------------------------------------------------------
by pminnieur at 2012-03-07T10:45:55Z
JFYI: the `RememberMeListener ` also does not type hint the interface but the implementation itself (it's always a constructor argument). All the other `Security\Http\Firewall` listeners type hint against the interface. I will update the PR accordingly today and also create a second PR against the 2.0 branch.
---------------------------------------------------------------------------
by pminnieur at 2012-03-07T11:55:52Z
JFYI: same issue w/ JMSSecurityExtraBundle https://github.com/schmittjoh/JMSSecurityExtraBundle/pull/44
Usage would be to extend the Kernel, and set the errorReportingLevel prior to calling parent::__construct(). Not ideal, but this doesn't break BC and allows the user to defer the decision as late as possible. This can/should be handled better in 2.1.x
Commits
-------
afbb8f2 Fixed misleading help for "name" argument as search for services with wildcards is not implemented
Discussion
----------
[FrameworkBundle, Console] Changed help text for container:debug command
Fixed help for "name" argument as search for services with wildcards is not implemented in ContainerDebugCommand
Commits
-------
f718859 [HttpFoundation] Removes use of parameter in Request::getClientIp function.
Discussion
----------
[HttpFoundation] Removes use of parameter in Request::getClientIp function
made in reference to this : https://groups.google.com/forum/#!topic/symfony-devs/cnSLwdAQiSk
This quickly addresses the problem when the helper is constructed in a console environment without request scope. Ideally, the helper should be able to construct the absolute logout URL using data already available in the UrlGenerator's RequestContext and the $_SERVER environment variable; however, that will require copying some code from the Request class to create a base URI and path.
Fixes#3508
Commits
-------
b73c703 Reverting return type left by mistake
881d290 Updating use of DoctrineBundle Registry to use the proper path to Doctrine\Bundle\DoctrineBundle\Registry
Discussion
----------
Updating use of DoctrineBundle Registry to use the proper path
Pointed to the new class: Doctrine\Bundle\DoctrineBundle\Registry
---------------------------------------------------------------------------
by adrienbrault at 2012-03-01T22:12:42Z
I think the return type should stay ```Registry```
---------------------------------------------------------------------------
by rdohms at 2012-03-01T22:48:35Z
Yes, that was a mistake, reverted.
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
-------
4f8e8ef Improving performance on digit filtering
Discussion
----------
Improving performance on digit filtering
I haven't tested it on a productive system but I think it should be way faster to use filter_var() instead of preg_replace() for several reasons.
This is my first pull request for symfony and I don't know how you do those kind of performance tests but please verify my assumption if you can :-)
Maybe we can also use filter_var() to replace other regular expressions :-)
HTH =)
---------------------------------------------------------------------------
by drak at 2012-02-22T00:35:44Z
@Toflar - nice move +1
---------------------------------------------------------------------------
by drak at 2012-02-22T18:53:40Z
@Toflar - Maybe you can bench the changes using this as a template: https://gist.github.com/1356129
---------------------------------------------------------------------------
by Toflar at 2012-02-23T13:18:18Z
I have already. And it's way faster, otherwise I wouldn't have opened a pull request ;) But obviously it strongly depends on the length of the string and the environment. That's why I was wondering whether you have a general performance tests environment ;) Because the results strongly depend on other factors, there's - in my opinion - no point in exact results. If a general info is sufficient: my tests for the regex resulted in about 7 - 8 microseconds whereas the filter version only took 1.5 - 2 microseconds for the same string.
Commits
-------
265360d [DoctrineBridge] Simpler result checking in UniqueEntityValidator
Discussion
----------
[DoctrineBridge] Simpler result checking in UniqueEntityValidator
In 928e352d09, support for MongoDB cursors was implemented by converting an Iterable, non-ArrayAccess object to an array. The ArrayAccess check didn't seem purposeful, since cursors are only Iterable and ORM returns real arrays. Since we only need to access the first element of the cursor (and only in cases where the count is exactly 1), we can simply use current() to handle Iterables and arrays.
@henrikbjorn: Any thoughts on this? I was testing @stof's work in doctrine/DoctrineMongoDBBundle#68 and our Symfony submodule was a bit old, so I fixed UniqueEntityValidator on my local machine before I realized you had come up with a solution a few weeks ago.
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
-------
ba251d8 [Routing] Updated Router::match and Router::generate documentation
2ce15bd [Routing] Fixed Router::match documentation
Discussion
----------
[Routing] Fixed Router::match and Router::generate documentation
Documentation of Router::match has been deprecated/invalid.
---------------------------------------------------------------------------
by stof at 2012-03-01T17:41:41Z
even better way to fix this: replace it with ``{@inheritdoc}``
---------------------------------------------------------------------------
by blogsh at 2012-03-01T19:22:06Z
Okay, wasn't sure whether this is appreciated because it inherits the method over 3 corners :)
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.
Added blocks, updated links and references and fixed typos.
Note it is not possible to throw exceptions in the write or close methods of a session save handler.
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.
