Commits
-------
7c2f11f Merge pull request #1 from pminnieur/post_response
9f4391f [HttpKernel] fixed DocBlocks
2a61714 [HttpKernel] added PostResponseEvent dispatching to HttpKernel
915f440 [HttpKernel] removed BC breaks, introduced new TerminableInterface
7efe4bc [HttpKernel] Add Kernel::terminate() and HttpKernel::terminate() for post-response logic
Discussion
----------
[HttpKernel] Add Kernel::terminate() and HttpKernel::terminate() for post-response logic
This came out of a discussion on IRC about doing stuff post-response, and the fact that right now there is no best practice, and it basically requires adding code after the `->send()` call.
It's an attempt at fixing it in an official way. Of course terminate() would need to be called explicitly, and added to the front controllers, but then it offers a standard way for everyone to listen on that event and do things without slowing down the user response.
---------------------------------------------------------------------------
by stof at 2011/12/06 02:41:26 -0800
We discussed it on IRC and I suggested a way to avoid the BC break of the interface: adding a new interface (``TerminableInterface`` or whatever better name you find) containing this method.
HttpKernel, Kernel and HttpCache can then implement it without breaking the existing apps using the component (Kernel and HttpCache would need an instanceof check to see if the inner kernel implements the method)
For Symfony2 users it will mean they have to change their front controller to benefit from the new event of course, but this is easy to do.
Btw, Silex can then be able to use it without *any* change for the end users as it can be done inside ``Application::run()``
---------------------------------------------------------------------------
by pminnieur at 2011/12/06 11:47:03 -0800
@Seldaek: I opened a pull request so that the discussion on IRC is fulfilled and no BC breaks exist: https://github.com/Seldaek/symfony/pull/1/files
---------------------------------------------------------------------------
by fabpot at 2011/12/07 07:59:49 -0800
Any real-world use case for this?
---------------------------------------------------------------------------
by Seldaek at 2011/12/07 08:10:31 -0800
Doing slow stuff after the user got his response back without having to implement a message queue. I believe @pminnieur wanted to use it to send logs to loggly?
---------------------------------------------------------------------------
by pminnieur at 2011/12/07 09:08:41 -0800
Its a good practice to defer code execution without the introduction of a new software layer (like gearman, amqp, whatever tools people use to defer code execution) which may be way too much just for the goal of having fast responses, whatever my code does.
My real world use case which made me miss this feature the first time:
> I have a calendar with a scheduled Event. For a given period of time, several Event entities will be created, coupled to the scheduled event (the schedule Event just keeps track of `startDate`, `endDate` and the `dateInterval`). Let's say we want this scheduled Event to be on every Monday-Friday, on a weekly basis, for the next 10 years.
This means I have to create `10*52*5` Event entities before I could even think about sending a simple redirect response. If I could defer code execution, I'd only save the scheduled Event, send the redirect response and after that, I create the `10*52*5` entities.
The other use case was loggly, yes. Sending logging data over the wire before the response is send doesn't make sense in my eyes, so it could be deferred after the response is send (this especially sucks if loggly fails and i get a 500 --the frontend/public user is not interested in a working logging facility, he wants his responses).
---------------------------------------------------------------------------
by mvrhov at 2011/12/07 10:07:03 -0800
This would help significantly, but the real problem, that your process is busy and unavailable for the next request, is still there.
---------------------------------------------------------------------------
by fabpot at 2011/12/07 10:15:18 -0800
I think this is the wrong solution for a real problem.
Saying "Its a good practice to defer code execution without the introduction of a new software layer" is just wrong.
It is definitely a good practice to defer code execution, but you should use the right tool for the job.
I'm -1.
---------------------------------------------------------------------------
by pminnieur at 2011/12/07 10:25:44 -0800
It should just give a possibility to put unimportant but heavy lifting code behind the send request with ease. With little effort people could benefit from the usage of `fastcgi_finish_request` without introducing new software, using `register_shutdown_function` or using `__destruct `(which works for simple things, but may act weird with dependencies).
It should not simulate node.js ;-) I agree that the real problem is not solved, but small problems could be solved easily. I personally don't want to setup RabbitMQ or whatever, maintain my crontab or any other software that may allow me to defer code execution.
---------------------------------------------------------------------------
by Seldaek at 2011/12/08 01:08:32 -0800
@fabpot: one could say that on shared hostings it is still useful because they generally don't give you gearman or \*MQs. Anyway I think it'd be nice to really complete the HttpKernel event cycle.
---------------------------------------------------------------------------
by pminnieur at 2011/12/08 01:48:57 -0800
not only on shared hostings, sometimes teams/projects just don't have the resources or knowledge or time to setup such an infrastructure.
---------------------------------------------------------------------------
by videlalvaro at 2011/12/08 01:53:06 -0800
I can say we used `fastcgi_finish_request` quite a lot at poppen with symfony 1.x. It certainly helped us to send data to Graphite, save XHProf runs, send data to RabbitMQ, and so on.
For example we used to connect to RabbitMQ and send the messages _after_ calling `fastcgi_finish_request` so the user never had to wait for stuff like that.
Also keep in mind that if you are using Gearman or RabbitMQ or whatever tool you use to defer code execution… you are not deferring the network connection handling, sending data over the wire and what not. I know this is obvious but is often overlooked.
So it would be nice to have an standard way of doing this.
---------------------------------------------------------------------------
by henrikbjorn at 2011/12/13 01:42:23 -0800
This could have been useful recently while implementing a "Poor mans cronjob" system. The solution was to do a custom Response object and do the stuff after send have been called with a Connection: Close header and ignore_user_abort(); (Yes very ugly)
Previously, Boolean defaults were printed as strings, which lead to true and false being printed as "1" and "", respectively. With this change, they are now printed as "true" and "false".
Commits
-------
0776b50 removed supports(De)Serializiation()
72b9083 SerializerAwareNormalizer now only implements SerializerAwareInterface
97389fa use Serializer specific RuntimeException
cb495fd added additional unit tests for deserialization
967531f fixed various typos from the refactoring
067242d updated serializer tests to use the new interfaces
d811e29 CS fix
351eaa8 require a (de)normalizer inside the (de)normalizable interfaces instead of a serializer
c3d6123 re-added supports(de)normalization()
078f7f3 more typo fixes
c3a711d abstract class children should also implement dernormalization
2a6741c typo fix
d021dc8 refactored encoder handling to use the supports*() methods to determine which encoder handles what format
f8e2787 refactored Normalizer interfaces
58bd0f5 refactored the EncoderInterface
b0daf35 split off an EncoderInterface and NormalizerInterface from SerializerInterface
Discussion
----------
[Serializer] split off an EncoderInterface and NormalizerInterface from SerializerInte
Bug fix: no
Feature addition: no
Backwards compatibility break: yes (but not inside a stable API)
Symfony2 tests pass: 
Fixes the following tickets: #2153
The purpose is to make it easier for other implementations that only implement parts of the interface due to different underlying approaches like the JMSSerializerBundle.
---------------------------------------------------------------------------
by schmittjoh at 2011/11/01 03:36:17 -0700
Actually, you can keep the current interface and I will just provide an adapter, sth like the following:
```php
<?php
class SymfonyAdapter implements SymfonyInterface
{
public function __construct(BundleInterface $serializer) { /* ... */ }
// symfony serializer methods mapped to bundle methods
}
```
I like to provide an adapter instead of implementing the interface directly since the bundle can be used standalone right now, and I don't want to add a dependency on the component just for the sake of the interface.
However, I do not completely see the purpose of the component. When would someone be recommended to use it? Everything the component does, the bundles does at the same level with the same complexity or simplicity (however you want to view that).
---------------------------------------------------------------------------
by lsmith77 at 2011/11/01 03:40:55 -0700
standalone in what way? you mean even out of the context of Symfony? In that context imho you should ship that code outside of a Bundle.
Regardless, how will that adaptor work? How would you implement methods like ``getEncoder()``? Afaik you can't and this is what this PR is about, splitting the interface to enable people to more finely specify what they provide.
---------------------------------------------------------------------------
by schmittjoh at 2011/11/01 04:03:56 -0700
I would just throw exceptions when something is not supported.
The more important question though is what is the goal of the component in the long-term, i.e. what problems is it supposed to solve, or in which cases should it be used?
Because right now it seems to me - correct me if I'm wrong - that the only purpose is that people don't have to install an extra library. However, that might even be frustrating for users because they need to migrate their code to the bundle as soon as they need to customize the serialization process which you need in 99% of the cases. For deserialization, the situation in the component is even worse. So, if my assessment is correct here (i.e. component to get started fast, if you need more migrate to the bundle), I think it would be better and less painful to have them start with the bundle right away.
---------------------------------------------------------------------------
by lsmith77 at 2011/11/01 04:15:10 -0700
Well then imho it would be better to split the interface.
I think the serializer component is sufficient for many situations and imho its easier to grok. Furthermore the normalizer/encoder concept it can be used in situations where JMSSerializerBundle cannot be used.
And splitting up the interfaces has exactly the goal of reducing the "frustrations" caused by out growing the the component.
---------------------------------------------------------------------------
by schmittjoh at 2011/11/01 04:29:39 -0700
I don't agree, but it's a subjective thing anyway.
So, whatever interface you come up with (preferably as few methods as possible), I will provide an adapter for it.
---------------------------------------------------------------------------
by fabpot at 2011/11/07 08:45:25 -0800
What's the status of this PR?
---------------------------------------------------------------------------
by lsmith77 at 2011/11/07 10:28:14 -0800
from my POV its good to go. but would like a nod from someone else in terms of the naming of the new interfaces
On 07.11.2011, at 17:45, Fabien Potencier <reply@reply.github.com> wrote:
> What's the status of this PR?
>
> ---
> Reply to this email directly or view it on GitHub:
> https://github.com/symfony/symfony/pull/2530#issuecomment-2655889
---------------------------------------------------------------------------
by stof at 2011/11/08 11:37:40 -0800
@lsmith77 what about doing the same for the ``NormalizerInterface`` instead of adding a new interface with a confusing name ? The Serializer class could implement ``Normalizer\NormalizerInterface`` by adding the 2 needed methods instead of duplicating part of the interface.
The next step is to refactor the Serializer class so that it choose the encoder and the decoder based on the ``support*`` methods. But this could probably be done in a separate PR.
---------------------------------------------------------------------------
by lsmith77 at 2011/11/08 11:51:27 -0800
yeah .. i wanted to do that once we are in agreement on the encoder stuff. question then is if we should again split off Denormalization. i guess yes.
---------------------------------------------------------------------------
by lsmith77 at 2011/11/08 12:06:34 -0800
ok done ..
---------------------------------------------------------------------------
by lsmith77 at 2011/11/08 12:59:51 -0800
i guess the next big task is to add more tests .. had to fix way too few unit tests with all this shuffling around .. will also help validating the concept. i should also test this out in a production application.
---------------------------------------------------------------------------
by lsmith77 at 2011/11/14 13:27:48 -0800
@ericclemmons can you also have a look at this PR and potentially help me adding tests?
---------------------------------------------------------------------------
by fabpot at 2011/12/07 07:32:06 -0800
@lsmith77: Is it ready to be merged? Should I wait for more unit tests?
---------------------------------------------------------------------------
by lsmith77 at 2011/12/07 07:34:56 -0800
If you merge it I am afraid I might get lazy and not write tests. This is why I changed the topic to WIP. I promise to finish this on the weekend.
Note however I was planning to write the tests for 2.0 and send them via a separate PR.
Once that PR is merged into 2.0 and master. I would then refactor them to work for this PR.
This way both 2.0 and master will have tests.
---------------------------------------------------------------------------
by fabpot at 2011/12/07 07:42:15 -0800
@lsmith77: sounds good. Thanks.
---------------------------------------------------------------------------
by lsmith77 at 2011/12/11 12:02:12 -0800
@fabpot ok i am done from my end.
@schmittjoh would be great if you could look over the final interfaces one time and give your blessing that you will indeed be able to provide implementations for these interfaces inside JMSSerializerBundle (even if just via an adapter)
---------------------------------------------------------------------------
by stof at 2011/12/12 12:43:49 -0800
@schmittjoh can you take a look as requested by @lsmith77 ?
---------------------------------------------------------------------------
by schmittjoh at 2011/12/13 03:33:23 -0800
Are the supports methods necessary? This is what I'm using in the bundle:
https://github.com/schmittjoh/JMSSerializerBundle/blob/master/Serializer/SerializerInterface.php
---------------------------------------------------------------------------
by lsmith77 at 2011/12/13 04:08:49 -0800
@schmittjoh without them determining if something is supported will always require an exception, which is pretty expensive. especially if one iterates over a data structure this can cause a lot of overhead.
---------------------------------------------------------------------------
by schmittjoh at 2011/12/13 04:24:18 -0800
my question was more if you have a real-world use case where this is useful?
On Tue, Dec 13, 2011 at 1:08 PM, Lukas Kahwe Smith <
reply@reply.github.com
> wrote:
> @schmittjoh without them determining if something is supported will always
> require an exception, which is pretty expensive. especially if one iterates
> over a data structure this can cause a lot of overhead.
>
> ---
> Reply to this email directly or view it on GitHub:
> https://github.com/symfony/symfony/pull/2530#issuecomment-3122157
>
---------------------------------------------------------------------------
by lsmith77 at 2011/12/13 04:28:08 -0800
yes .. this serializer .. since it traverses the tree and decides what is the current normalizer one by one (aka not via visitors as in your implementation). without the supports*() methods it would need to have the normalizer throw exceptions, but this is not exceptional, its the normal code flow to have to iterate to find the correct normalizer.
---------------------------------------------------------------------------
by schmittjoh at 2011/12/13 04:30:36 -0800
can we split it off into a second interface?
On Tue, Dec 13, 2011 at 1:28 PM, Lukas Kahwe Smith <
reply@reply.github.com
> wrote:
> yes .. this serializer .. since it traverses the tree and decides what is
> the current normalizer one by one (aka not via visitors as in your
> implementation). without the supports*() methods it would need to have the
> normalizer throw exceptions, but this is not exceptional, its the normal
> code flow to have to iterate to find the correct normalizer.
>
> ---
> Reply to this email directly or view it on GitHub:
> https://github.com/symfony/symfony/pull/2530#issuecomment-3122315
>
---------------------------------------------------------------------------
by lsmith77 at 2011/12/13 04:33:27 -0800
hmm .. i guess we could .. these methods in a way are implementation specific and are mainly public because its different objects interacting with each other, though for users of the lib they can also be convenient at times.
---------------------------------------------------------------------------
by lsmith77 at 2011/12/14 09:13:53 -0800
ok i reviewed things again and just removed those two methods, since the possibility for these methods to be feasible is too tied to the implementation and for this particular implementation supportsEncoding() and supportsDecoding() are still available.
so all ready to be merged ..
---------------------------------------------------------------------------
by lsmith77 at 2011/12/14 09:15:44 -0800
hmm i realized one thing just now:
cb495fd7a3
that commit should also be included in 2.0 .. i am not sure what the most elegant way is to make that happen ..
---------------------------------------------------------------------------
by fabpot at 2011/12/14 10:10:16 -0800
@lsmith77: commit cb495fd7a3 cannot be cherry picked in 2.0 as is as the tests do not pass: "Fatal error: Call to undefined method Symfony\Component\Serializer\Serializer::supportsDenormalization() in tests/Symfony/Tests/Component/Serializer/SerializerTest.php on line 150"
---------------------------------------------------------------------------
by lsmith77 at 2011/12/14 10:11:55 -0800
ah of course .. i just removed that method :) .. then never mind .. all is well.
Commits
-------
600066e [Templating] fixed 'scheme://' not detected as absolute path
e6f2687 [HttpKernel] fixed 'scheme://' not detected as absolute path
b50ac5b [Config] fixed 'scheme://' not detected as absolute path
Discussion
----------
[Config][HttpKernel][Templating] 'scheme://' paths not detected as absolute
Bug fix: yes
Feature addition: no
Backwards compatibility break: no (99%)
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
The method ```isAbsolutePath``` does not detect URL schemes as absolute. This makes imposible the use of wrappers to access remote files or the use of files (mostly configuration or templates) stored on phar archives (uses the scheme ```phar://``` in the path).
Three classes implement this methods: ```Symfony\Component\Config\FileLocator```, ```Symfony\Component\HttpKernel\Util\Filesystem``` and ```Symfony\Component\Templating\Loader\FilesytemLoader```. All are updated. Also includes a new check on all related tests (```Symfony\Component\HttpKernel\Util\Filesystem``` lacks of test).
Commits
-------
cd24fb8 change explode's limit parameter based on known variable content
b3cc270 minor optimalisations for explode
Discussion
----------
[FrameworkBundle][CssSelector][HttpFoundation][HttpKernel] [Security][Validator] Minor optimizations for "explode" function
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
I added limit parameter in some places, where it may be usefull. I did not check the context of what values may have been exploded. So to not break anything, I added +1 to limit parameter.
If you find out that in some places limit (or limit+1) is not important or meaningless, write a comment please and I will fix it.
---------------------------------------------------------------------------
by fabpot at 2011/12/07 06:56:49 -0800
Adding +1 just to be sure to not break anything is clearly something we won't do. What is the benefit of doing that anyway?
---------------------------------------------------------------------------
by pulzarraider at 2011/12/07 13:50:24 -0800
The main idea of making this PR was to notify about some places that may run faster with just adding one parameter to explode function.
If in code is someting like: ```list($a, $b) = explode(':', $s);```
Function ```explode``` will create n-items (depends on ```$s```), but we need in code only the first two items. There is no reason to let ```explode``` create more items in memory that are NEVER used in our code. The limit parameter is there for these situations, so let's use it.
I know that it is microoptimization and may look unimportant, but we are writing a framework - so people expect that code will be as fast as possible without this kind of mistakes.
As I've noticed above, I know that +1 is not ideal solution, but the fastest without debugging the code. I expect that someone (with good knowledge of that code) will look at it and write in comments if variable may contain 1 comma (dot or someting on what is doing the explode) or maybe 2 in some situations or more.
Anyway, +1 will not break anything, because same items are created as it is now, but no unnecessary item is created.
---------------------------------------------------------------------------
by fabpot at 2011/12/07 23:14:59 -0800
I'm +1 for adding the number to avoid problems but I'm -1 on the optimization side of things as it won't optimize anything.
---------------------------------------------------------------------------
by helmer at 2011/12/08 12:46:49 -0800
*.. The main idea of making this PR was to notify about some places that **may** run faster ..*
I am also unsure the optimization is really an optimization, care to benchmark (with meaningful inputs)? As for the limit+1 thing, why would you want to +1 it? The number of ``list`` arguments should always reflect the ``limit`` parameter, no?
---------------------------------------------------------------------------
by pulzarraider at 2011/12/08 23:11:34 -0800
@helmer please try this simple benchmark:
```
<?php
header('Content-Type: text/plain; charset=UTF-8');
define('COUNT', 10000);
$source_string = 'aaaaaaaaaaaaaaaaaaaa:bbbbbbbbbbbbbbbbbbbbb:cccccccccccccccccccccccc:dddddddddddddddddddddd:eeeeeeeeeeeeeeeeeeeeeeeee:fffffffffffffffffffffffffff';
$start = microtime(true);
for ($i = 0; $i < COUNT; $i++) {
list($a, $b) = explode(':', $source_string);
}
$end = microtime(true)-$start;
echo 'without limit: '.$end."\n";
$start = microtime(true);
for ($i = 0; $i < COUNT; $i++) {
list($a, $b) = explode(':', $source_string, 2);
}
$end = microtime(true)-$start;
echo 'with limit: '.$end."\n";
```
My results are:
```
without limit: 0.057228803634644
with limit: 0.028676986694336
```
That is 50% difference (with APC enabled). Of course the result depends on the length of source string and if it's too short, the difference may be none or very very small. That's why I said, that it **may** run faster and is just a micro optimization.
---------------------------------------------------------------------------
by pulzarraider at 2011/12/08 23:18:12 -0800
@helmer And why +1? It depends on a code:
```
$source_string = 'aaaaaaaaaaaaaaaaaaaa:bbbbbbbbbbbbbbbbbbbbb:cccccccccccccccccccccccc';
list($a, $b) = explode(':', $source_string, 2);
var_dump($a, $b);
```
and
```
$source_string = 'aaaaaaaaaaaaaaaaaaaa:bbbbbbbbbbbbbbbbbbbbb:cccccccccccccccccccccccc';
list($a, $b) = explode(':', $source_string, 3);
var_dump($a, $b);
```
gives different results. That's why the content of the variable must be known.
---------------------------------------------------------------------------
by helmer at 2011/12/09 00:08:28 -0800
@pulzarraider Thanks for the benchmark, seems like a gain enough. Although, we are more likely having a scenario of:
``explode(':', 'a🅱️c')`` vs ``explode(':', 'a🅱️c', 3)`` with a ``COUNT`` of 10, where the difference is not even in microseconds anymore :)
The limit addition alters the behaviour though, ie suddenly you can define a controller [logical name](http://symfony.com/doc/current/book/routing.html#controller-string-syntax) as ´´AcmeBlogBundle:Blog:show:something``, and things go downhill from there on.
All that aside, I'm +1 for setting the limit to the exact number of ``list`` parameters, but certainly not number+1, this is just too wtfy (as you said, this was a safety thing, but I reckon for this PR to be merged it needs to be +0).
---------------------------------------------------------------------------
by drak at 2011/12/09 08:28:58 -0800
Overall `list()` is ugly as it's not very explicit. Even though it would mean extra lines, it's better to `explode()` then explicitly assign variables:
```
$parts = explode(':', $foo);
$name = $parts[0];
$tel = $parts[1];
```
`list()` is one of those bad relics from the PHP past...
---------------------------------------------------------------------------
by fabpot at 2011/12/11 10:07:47 -0800
@drak: why is `list` not explicit? It is in fact as explicit as the more verbose syntax you propose.
---------------------------------------------------------------------------
by pulzarraider at 2011/12/11 13:08:50 -0800
@drak: I agree with @fabpot. In speech of benchmarks ```list``` is faster then using a helper variable.
@fabpot, @helmer I've changed explode's limit to be correct (without +1) and removed some changes from this PR, where I can't find out what the content of variable may be. Unit tests pass, so I think it's ready for merge.
Commits
-------
5f22268 [Profiler] Sync with master
1aef4e8 Adds collecting info about request method and allowing searching by it
Discussion
----------
[WebProfiler] Add ability to filter data by request method
Bug fix: no
Feature addition: yes
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: #1515
For discussion & description checkout: #1515 & #2279
---------------------------------------------------------------------------
by fabpot at 2011/12/11 10:02:41 -0800
After merging this PR, the toolbar is not displayed anymore for me.
---------------------------------------------------------------------------
by stof at 2011/12/12 14:18:20 -0800
@fabpot the toolbar works for me using this branch
Commits
-------
3759ff0 [Locale] StubNumberFormatter allow to parse 64bit number in 64bit mode
Discussion
----------
[Locale] StubNumberFormatter allow to parse 64bit number in 64bit mode
Bug fix: yes
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: [](http://travis-ci.org/stealth35/symfony)
Fixes the following tickets: #2735
---------------------------------------------------------------------------
by stealth35 at 2011/12/01 06:47:32 -0800
@Seldaek should be better now
---------------------------------------------------------------------------
by stealth35 at 2011/12/02 04:22:42 -0800
@fabpot done
---------------------------------------------------------------------------
by fabpot at 2011/12/02 04:28:24 -0800
Tests do not pas for me (on a Mac):
1) Symfony\Tests\Component\Locale\Stub\StubNumberFormatterTest::testParseTypeInt64StubWith64BitIntegerInPhp64Bit
->parse() TYPE_INT64 does not use true 64 bit integers, using only the 32 bit range.
Failed asserting that 2147483648 matches expected -2147483648.
.../tests/Symfony/Tests/Component/Locale/Stub/StubNumberFormatterTest.php:819
---------------------------------------------------------------------------
by stealth35 at 2011/12/02 04:50:20 -0800
@fabpot, could you send me the return of this code
``` php
<?php
$formatter = new \NumberFormatter('en', \NumberFormatter::DECIMAL);
$value = $formatter->parse('2,147,483,648', \NumberFormatter::TYPE_INT64);
var_dump($value);
$value = $formatter->parse('-2,147,483,649', \NumberFormatter::TYPE_INT64);
var_dump($value);
```
---------------------------------------------------------------------------
by fabpot at 2011/12/02 06:06:21 -0800
int(-2147483648)
int(2147483647)
---------------------------------------------------------------------------
by stealth35 at 2011/12/02 06:10:28 -0800
It's nosens, but the Stub should follow Intl ... so I fix that
---------------------------------------------------------------------------
by stealth35 at 2011/12/11 08:48:25 -0800
It's OK now
This changes helps the common use case of fetching the current user and better complies with the Law of Demeter (http://en.wikipedia.org/wiki/Law_of_Demeter).
Before (still works):
$token = $context->getToken();
$user = $token ? $token->getUser() : null;
After:
$user = $context->getUser();
Commits
-------
e06cea9 [HttpFoundation] Cookie values should not be restricted
Discussion
----------
[HttpFoundation] Cookie values should not be restricted
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
The restriction I removed makes no sense IMO because we do not use setrawcookie() to send cookies. setrawcookie() does throw a warning when the cookie value contains incorrect characters, but not setcookie(). The latter will just urlencode() the value so it becomes valid. This is also what is done by `Cookie::__toString`, so this could be used in combination with header() to just send raw cookies that are valid, even with values that are invalid in their decoded form.
PHP urldecodes cookies on input, so it all works fine.
Commits
-------
70e9332 added check for invalid user providers
Discussion
----------
[security] added check for invalid user providers
I've added an exception if an invalid user provider is passed into a context listener.
The FOSFacebookBundle configuration has been documented wrong for a long time. This exception prevent users from configuring the security layer incorrectly.
```
Bug fix: no
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: ~
Todo: ~
```
---------------------------------------------------------------------------
by fabpot at 2011/12/06 00:40:08 -0800
What about doing that in the 2.0 branch?
---------------------------------------------------------------------------
by kriswallsmith at 2011/12/06 03:23:59 -0800
It is possible it will break some applications -- a non-user provider that is not first in the array may never be called.
Commits
-------
da0773c Note DependencyInjection exception changes in the 2.1 changelog
9a090bc [DependencyInjection] Fix class check and failure message in PhpDumper test
2334596 [DependencyInjection] Use component's SPL classes in dumped service container
3c02ea2 [DependencyInjection] Use exception class for API doc generation
47256ea [DependencyInjection] Make exceptions consistent when ContainerBuilder is frozen
b7300d2 [DependencyInjection] Fix up @throws documentation
123f514 [DependencyInjection] Use component-specific SPL exceptions
cf2ca9b [DependencyInjection] Create additional SPL exceptions
ba8322e [DependencyInjection] Format base exception classes consistently
Discussion
----------
[DependencyInjection] Refactor usage of SPL exceptions
```
Bug fix: no
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: -
```
This was something discussed on the mailing list recently and a topic at the last IRC meeting. The DependencyInjection component was what I had in mind while discussing this with @lsmith a few weeks ago, as I found that it already had some local SPL exceptions, which weren't being used directly (only as base classes for the compiler exceptions). This PR replaces uses of core SPL exceptions with the component's equivalents. I've listed it as BC-breaking to be safe, but I don't think it should cause any trouble, since the new exceptions are sub-classes of those originally used.
I purposely avoided changing the exceptions in the dumped PHP container. If we agree that's worth doing, it would be a trivial addition to the PR.
---------------------------------------------------------------------------
by jmikola at 2011/12/04 22:38:47 -0800
One question I came across while implementing this PR: the doc blocks in ContainerInterface reference InvalidArgumentException (actually the component's local SPL equivalent), but there is no practical need for that class to be used in the file. How will the API doc generator handle this? Do we need to change the doc block reference to the full class path?
---------------------------------------------------------------------------
by fabpot at 2011/12/05 01:20:31 -0800
Why have you not replaced the exception in the dumped container code? I don't see any drawback.
---------------------------------------------------------------------------
by stof at 2011/12/05 03:39:25 -0800
it would even be better to be consistent in the generated code
---------------------------------------------------------------------------
by jmikola at 2011/12/05 09:48:05 -0800
I'll update the exceptions in the dumped container code as well. Thanks for the feedback.
---------------------------------------------------------------------------
by jmikola at 2011/12/05 10:41:21 -0800
Ok, this should be ready to review and merge. Tests needed some updating, but they still pass.
Commits
-------
7c1cbb9 [Config] Use LoaderResolverInterface for type-hinting
48b084e fixed typo
8ad94fb merged branch hhamon/doctrine_bridge_cs (PR #2775)
240796e [Bridge] [Doctrine] fixed coding conventions.
7cfc392 check for session before trying to authentication details
648fae7 merged branch proofek/domcrawlerform-radiodisabled (PR #2768)
3976b7a [DoctrineBridge] fixed CS
9a04783 merged branch beberlei/SecurityEntityRepositoryIdentifierFix (PR #2765)
3c83b89 [DoctrineBridge] Catch user-error when the identifier is not serialized with the User entity.
36c7d03 Fixed GH-2720 - Fix disabled atrribute handling for radio form elements
Discussion
----------
[Config] Use LoaderResolverInterface for type-hinting
```
Bug fix: no
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: -
```
I've listed this as a BC break because we're changing the argument type-hint, but I think it's unlikely to affect anyone.
Some methods previously threw LogicExceptions when the ContainerBuilder was frozen. Using BadMethodCallException (a descendant of LogicException) in all cases improves consistency and preserves BC.
This replaces existing use of core SPL exceptions with the equivalent classes defined within the component. Although method documentation has been changed, this change should be BC since the component-specific SPL exceptions extend their core counterpart.
This commit purposely omits any changes to the PhpDumper, which throws several core SPL exceptions.
Commits
-------
36c7d03 Fixed GH-2720 - Fix disabled atrribute handling for radio form elements
Discussion
----------
Fixed GH-2720 - Fix disabled atrribute handling for radio form elements
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: - GH-2720
I wasn't really sure about the correct approach. This one is very minimalistic and following the existing concept of not duplicating nodes of the same name, but only storing multiple values for the same node. If you think that should be changed, let me know. Hints appreciated.
Thanks
Commits
-------
8710a13 Added example to the change log file
c9a2b49 Fixed xml encoder test script, and group `item` tags into an array
a0561e5 Replaced `item` with `*item` when parsing XML string
Discussion
----------
Replaced `item` with `*item` when parsing XML string
---------------------------------------------------------------------------
by fabpot at 2011/11/23 22:14:12 -0800
Tests do not pass:
1) Symfony\Tests\Component\Serializer\Encoder\XmlEncoderTest::testDecode
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
'key2' => 'val'
- 'A B' => 'bar'
'Barry' => Array (...)
+ 'item' => Array (...)
)
'qux' => '1'
)
.../tests/Symfony/Tests/Component/Serializer/Encoder/XmlEncoderTest.php:173
---------------------------------------------------------------------------
by fabpot at 2011/11/24 22:57:37 -0800
I don't understand the patch anymore. I don't see any use of `*item` in the code.
---------------------------------------------------------------------------
by excelwebzone at 2011/11/24 23:04:07 -0800
I run some testing and you can't use '*item' XML parser reject it. So I modified it to convert it to an array.. Look at the test script change
---------------------------------------------------------------------------
by fabpot at 2011/11/24 23:13:30 -0800
So, you probably need to change the CHANGELOG as well? You should add an example which shows a before/after example.
---------------------------------------------------------------------------
by excelwebzone at 2011/11/24 23:15:51 -0800
Yes, forgot to change that..
---------------------------------------------------------------------------
by fabpot at 2011/11/25 01:27:42 -0800
ping @Seldaek, @lsmith77
---------------------------------------------------------------------------
by Seldaek at 2011/11/25 04:16:43 -0800
There are other meta-names available in the XmlEncoder, @-something for attributes, then there is something happening with a # but I'm not quite sure what. I'm just saying, maybe *item isn't the best name, if it introduces a third metacharacter. Apart from that I'm fine with it.
---------------------------------------------------------------------------
by excelwebzone at 2011/11/25 08:45:31 -0800
Maybe we can rename it to `wildcard` instead
---------------------------------------------------------------------------
by excelwebzone at 2011/11/25 15:12:09 -0800
Any chance we can push this throw?
---------------------------------------------------------------------------
by lsmith77 at 2011/11/27 04:06:25 -0800
here is the old PR #2682
@Seldaek: i think your comment was made for an older version of the patch.
overall I am fine with the change, the Serializer component takes a fairly simple approach. it is also not designed to really produce XML or JSON cleanly from the same data. it will really only be able to output a clean API for one or the other with the same data structure.
---------------------------------------------------------------------------
by excelwebzone at 2011/12/01 06:25:24 -0800
@fabpot can we merge this change
Commits
-------
63e2a99 [CssSelector] Fixed Issue for XPathExprOr: missing prefix in string conversion
Discussion
----------
[CssSelector] Issue for XPathExprOr: missing prefix in string conversion
Hi there,
I created a small and dumb test for the issue. I looked at the original implementation and i think the problem is, that private properties are used in the parent class for xPathExprOr. so that the prefix cannot be accessd with $this->prefix in XPathExprOr
However I think the distribution for the prefix should be put in the parts of the or-sub-expressions the way it is shown in the test.
Hope this helps.
Best regards
Philipp
Commits
-------
9e6a10a [Form] Added FormError::getMessage() and use it in Form class
Discussion
----------
[Form] Added FormError::getMessage()
---------------------------------------------------------------------------
by ericclemmons at 2011/11/29 18:38:40 -0800
Should this go through the translator, similar to how `field_errors` renders error messages?
> https://github.com/symfony/symfony/blob/master/src/Symfony/Bridge/Twig/Resources/views/Form/form_div_layout.html.twig#L253
---------------------------------------------------------------------------
by stof at 2011/11/29 23:11:24 -0800
``getErrorsAsString`` is there for a debugging purpose so injecting the translator in the Form class just for it seems wrong. And the logic used here is exactly what the identity translator does.
Bug fix: yes
Feature add: no
Symfony2 tests pass: yes
Symfony2 tests added: yes
In general without this exception generated by php dumper container class, will cause PHP fatal error, bacause method call will look like this: `$instance->(/* arguments*/);`.
When controller is a Closure ControllerResolver::getArguments tries to
make a ReflectionMethod of the __invoke method. But because it's an
internal function, the parameters method isDefaultValueAvailable will
return always false, even if isOptional return true.
Commits
-------
09562df Update CHANGELOG for 2.1, describe new auth events
cf09c2d added authentication success/failure events
Discussion
----------
[Security] Implementation of a "failed login" event, replaces: PR #1307
As I have to use this feature I have completed its implementation.
Bugfix: no
Feature addition: yes
Symfopny2 tests pass: yes
Replaces/closes PR: #1307
---------------------------------------------------------------------------
by schmittjoh at 2011/11/18 23:57:56 -0800
Usually, this event is used for the wrong reasons (to customize what happens on authentication failure). Can you move your implementation to the AuthenticationProviderManager instead?
see https://github.com/schmittjoh/symfony/blob/master/src/Symfony/Component/Security/Core/Authentication/AuthenticationProviderManager.php#L103
---------------------------------------------------------------------------
by canni at 2011/11/19 06:00:36 -0800
Good point :) I'll not rewrite yours work, I've cherry-picked yours commits. (BTW you added call to `setEventDispatcher` on `security.authentication.manager` to commit related to some different work ;)
---------------------------------------------------------------------------
by fabpot at 2011/11/22 00:12:19 -0800
The new files are missing the LICENSE header. As far as I can see, @schmittjoh fork has a different license from the Symfony one. This needs to be clarified before I can merge this PR.
---------------------------------------------------------------------------
by schmittjoh at 2011/11/22 01:53:09 -0800
No biggy, MIT is fine here.
---------------------------------------------------------------------------
by canni at 2011/11/22 01:57:51 -0800
@fabpot done
---------------------------------------------------------------------------
by fabpot at 2011/11/22 02:22:47 -0800
@canni: Can you update the CHANGELOG file (to reference the changes and the BC breaks -- like the move of KernelEvents for instance).
---------------------------------------------------------------------------
by canni at 2011/11/22 02:40:33 -0800
@fabpot: no problem & done
PS I haven't realized that namespace change of `SecurityEvents` is actually a BC Break, thx for pointing this.
---------------------------------------------------------------------------
by fabpot at 2011/11/22 03:06:17 -0800
@canni: What about keeping a `SecurityEvents` class in the `Http` namespace that just extends the new one. That way, we don't break BC.
---------------------------------------------------------------------------
by canni at 2011/11/22 03:53:01 -0800
@fabpot: that will force us to remove `final` keyword form one of classes.
Maybe we can add new, not extending class e.g.: `GeneralSecurityEvents` or `AuthenticationEvents`, that way we dont break BC and dont introduce confusion in naming?
---------------------------------------------------------------------------
by canni at 2011/11/22 05:53:15 -0800
@fabpot: I've removed the BC break, and squashed schmittjoh commits, to keep things nice and clear.
Commits
-------
e7215ae Fix composer.json
Discussion
----------
Fix composer.json files
>=2.1 does not include the 2.1.0-dev versions, since those are lesser than 2.1.0 stable, so nothing is really working right now.
Commits
-------
11b6156 updated unittest
a931e21 get correct client IP from X-forwarded-for header
Discussion
----------
[HttpFoundation] Get correct client IP when using trusted proxy (Varnish)
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
Note: This is reopened PR #2686 for 2.0 branch.
If using trusted proxy (Varnish, ...) the client IP must be identified from X-Forwarded-For header. The header has de-facto standard format:
X-Forwarded-For : client1, proxy1, proxy2,
where the value is a comma+space separated list of IP addresses, the left-most being the farthest downstream client, and each successive proxy that passed the request adding the IP address where it received the request from. See: http://en.wikipedia.org/wiki/X-Forwarded-For
Function getClientIp should return only one client IP, not a list of all nonimportant IPs as it's now. Similar example can be seen in Cake framework: http://api.cakephp.org/view_source/request-handler-component/#line-477
There are many ways how to chose the first IP from X-Forwarded-For header. Any other faster and more reliable way is welcome.
