Commits
-------
7474293 memcache profiler storage support added
Discussion
----------
[HttpKernel] [FrameworkBundle] Memcache(d) Profiler Storage added
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
There are 2 memcache PHP extensions: Memcache and MemcacheD (with "D" at the end) - both are supported.
How to use Memcache Profiler Storage (Memcache php extension is used):
change (or add if there isn't) "dsn" in framework/profiler section in config_dev.yml
```
...
framework:
...
profiler:
...
dsn: memcache://127.0.0.1/11211
...
```
How to use Memcached Profiler Storage (MemcacheD php extension is used):
change "dsn" in framework/profiler section in config_dev.yml
```
...
framework:
...
profiler:
...
dsn: memcached://127.0.0.1/11211
...
```
Last changes:
- memcached support addedd
- optimized performance (serialization done in extension, index is created with ```append``` function)
- updated to last version of Profiler (find by method, avoid duplications)
- done squash on commits
---------------------------------------------------------------------------
by stloyd at 2011-12-01T23:36:02Z
You need to add check for index name size, AFAIK memcache will fail if key is longer than 250 characters.
Also please do an `squash` for all those commits.
---------------------------------------------------------------------------
by pulzarraider at 2011-12-02T00:15:28Z
@stloyd Thanks. I will add the check for key length.
I am just starting with git. Could you please add some tutorial about squash to a documentation page: http://symfony.com/doc/2.0/contributing/code/patches.html ? It will help me (and maybe some others) to do it correct way.
---------------------------------------------------------------------------
by stof at 2011-12-02T00:19:01Z
http://help.github.com/rebase/
---------------------------------------------------------------------------
by pulzarraider at 2011-12-03T18:56:11Z
Thanks @stof, rebase done.
---------------------------------------------------------------------------
by dlsniper at 2011-12-11T14:00:17Z
Hi,
Would it be possible to either use Memcached instead of Memcache or make it configurable to use either Memcache or Memcached?
I've did a little digging on the benefits of using Memcached over Memcache (like for example: http://stackoverflow.com/questions/1442411/using-memcache-vs-memcached-with-phphttp://devzone.zend.com/1869/zendcon-sessions-episode-040-memcached-the-better-memcache-interface/ ) and maybe this will also help in not having two extensions installed for people who are using Memcached already.
Regards.
---------------------------------------------------------------------------
by pulzarraider at 2011-12-11T16:15:58Z
@dlsniper thanks for great comment. I will add memcached support.
---------------------------------------------------------------------------
by stof at 2011-12-12T20:49:00Z
@pulzarraider what is the status of this PR ? Is it still a WIP ?
---------------------------------------------------------------------------
by pulzarraider at 2011-12-12T22:58:48Z
@stof Yes, it's still WIP. I'm working on a memcached (with D at the end) support. It will be finished in the next few days.
---------------------------------------------------------------------------
by dlsniper at 2011-12-15T12:51:52Z
@pulzarraider if I can help you with the PR let me know.
---------------------------------------------------------------------------
by pulzarraider at 2012-01-08T20:22:24Z
@dlsniper @stof I've finally added memcached support and done some optimizations. Memcache(d) profiler storage is now ready.
---------------------------------------------------------------------------
by dlsniper at 2012-01-08T22:12:29Z
I'm glad you finished this @pulzarraider
Thanks! for your hard work!
+1 for this PR
@stof, @fabpot is it good to go on master?
---------------------------------------------------------------------------
by pulzarraider at 2012-01-28T19:45:56Z
@stof, @fabpot ping
Commits
-------
3dd3d58 [EventListener] Fix an issue with sub-requests
71bf279 cleanup
acdb325 [StopWatch] Provide a cleaner API
acd1287 [Stopwatch] rename the section event to avoid collisions
eb540be [Profiler] Allow profiling the terminate event
4ccdc53 [HttpKernel] Cleanup of PdoProfilerStorage
814876f [HttpKernel] Tweak the code of the ProfilerListener
Discussion
----------
[Profiler] Allow profiling the terminate event
This PR is mainly about allowing to profile the terminate event (i.e. see it in the timeline panel)
There are some other tweaks.
---------------------------------------------------------------------------
by vicb at 2012-02-02T14:43:20Z
please don't merge for now. good question. bad answer.
---------------------------------------------------------------------------
by vicb at 2012-02-06T15:05:46Z
While first commits were focused on problem solving, the last brings a clean API with the ability to re-open an existing section in order to add events (re-setting event origins and merging them were just hacks).
Should be ready to be merged.
_Edit: Sorry, couldn't resist adding a private helper class again!_
---------------------------------------------------------------------------
by stof at 2012-02-06T18:30:09Z
@vicb you should stop adding such classes defined in the same file. Otherwise we will have to change the CS (and to stop telling we respect the PSR-0 standard)
---------------------------------------------------------------------------
by vicb at 2012-02-06T18:33:36Z
Once again PSR-0 is about autoloading which is exactly why I do not want in such cases. CS are an other matter and yes I think they should be changed to allow this (and I am going to submit a PR right now).
The only argument I could accept is whether this class should be private or not.
---------------------------------------------------------------------------
by vicb at 2012-02-06T19:57:06Z
Thanks for your valuable feedback @stof
---------------------------------------------------------------------------
by fabpot at 2012-02-11T20:53:03Z
Have you tested it on a project? Because it breaks my simple examples (where I have some sub-requests).
---------------------------------------------------------------------------
by vicb at 2012-02-12T09:47:23Z
my bad, should be ok now.
Commits
-------
fe62401 optimized string starts with checks
Discussion
----------
optimized string starts with checks
Doing this with strpos() is slightly faster than substr().
```
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
```
---------------------------------------------------------------------------
by vicb at 2012-01-11T19:58:27Z
How faster ? even if the string is long and do not contain an occurrence of the sub-string ?
Looks like micro-(not)-optimizations to me.
---------------------------------------------------------------------------
by kriswallsmith at 2012-01-11T20:04:26Z
The difference is about 0.1s when repeated 1M times.
---------------------------------------------------------------------------
by vicb at 2012-01-11T20:08:12Z
% would be better (machine & env independant), what string size, what match offset ?
I personally vote against (`substr` is more meaningful to me and I do not like micro-optims)
---------------------------------------------------------------------------
by kriswallsmith at 2012-01-11T20:12:34Z
I personally consider this a coding standard but don't want to bikeshed here :)
---------------------------------------------------------------------------
by vicb at 2012-01-11T20:28:08Z
I have [tried](https://gist.github.com/1596588) at home.
`strpos ` **is** faster unless you have a very long string, probably because you do not need to create a new string, interesting, thanks for the tip.
---------------------------------------------------------------------------
by Tobion at 2012-01-11T22:40:18Z
I think strpos() is more useful. Say you want to change the string you have to replace 2 variables (the text and the length parameter) when using substr(). It could also introduce bugs when they don't match. With strpos() it's only the text.
---------------------------------------------------------------------------
by robocoder at 2012-01-11T22:43:22Z
alternate micro-optimization that doesn't create a temporary string:
```
strncmp($v, "@", 1) === 0
```
---------------------------------------------------------------------------
by Tobion at 2012-01-11T22:47:12Z
@robocoder probably the fastest solution but needs to be benchmarked
fix CS
fix CS + remove unneeded else
add documentation, change protected methods as private
rename var
throw exception for invalid name, index fix
memcache profiler storage support added, fix CS and minor bugs
fix CS
removed unneeded else
- memcached support added
- improved performance (serialization, index)
updated code to last version of Profiler
Commits
-------
887c0e9 moved EngineInterface::stream() to a new StreamingEngineInterface to keep BC with 2.0
473741b added the possibility to change a StreamedResponse callback after its creation
8717d44 moved a test in the constructor
e44b8ba made some cosmetic changes
0038d1b [HttpFoundation] added support for streamed responses
Discussion
----------
[HttpFoundation] added support for streamed responses
To stream a Response, use the StreamedResponse class instead of the
standard Response class:
$response = new StreamedResponse(function () {
echo 'FOO';
});
$response = new StreamedResponse(function () {
echo 'FOO';
}, 200, array('Content-Type' => 'text/plain'));
As you can see, a StreamedResponse instance takes a PHP callback instead of
a string for the Response content. It's up to the developer to stream the
response content from the callback with standard PHP functions like echo.
You can also use flush() if needed.
From a controller, do something like this:
$twig = $this->get('templating');
return new StreamedResponse(function () use ($templating) {
$templating->stream('BlogBundle:Annot:streamed.html.twig');
}, 200, array('Content-Type' => 'text/html'));
If you are using the base controller, you can use the stream() method instead:
return $this->stream('BlogBundle:Annot:streamed.html.twig');
You can stream an existing file by using the PHP built-in readfile() function:
new StreamedResponse(function () use ($file) {
readfile($file);
}, 200, array('Content-Type' => 'image/png');
Read http://php.net/flush for more information about output buffering in PHP.
Note that you should do your best to move all expensive operations to
be "activated/evaluated/called" during template evaluation.
Templates
---------
If you are using Twig as a template engine, everything should work as
usual, even if are using template inheritance!
However, note that streaming is not supported for PHP templates. Support
is impossible by design (as the layout is rendered after the main content).
Exceptions
----------
Exceptions thrown during rendering will be rendered as usual except that
some content might have been rendered already.
Limitations
-----------
As the getContent() method always returns false for streamed Responses, some
event listeners won't work at all:
* Web debug toolbar is not available for such Responses (but the profiler works fine);
* ESI is not supported.
Also note that streamed responses cannot benefit from HTTP caching for obvious
reasons.
---------------------------------------------------------------------------
by Seldaek at 2011/12/21 06:34:13 -0800
Just an idea: what about exposing flush() to twig? Possibly in a way that it will not call it if the template is not streaming. That way you could always add a flush() after your </head> tag to make sure that goes out as fast as possible, but it wouldn't mess with non-streamed responses. Although it appears flush() doesn't affect output buffers, so I guess it doesn't need anything special.
When you say "ESI is not supported.", that means only the AppCache right? I don't see why this would affect Varnish, but then again as far as I know Varnish will buffer if ESI is used so the benefit of streaming there is non-existent.
---------------------------------------------------------------------------
by cordoval at 2011/12/21 08:04:21 -0800
wonder what the use case is for streaming a response, very interesting.
---------------------------------------------------------------------------
by johnkary at 2011/12/21 08:19:48 -0800
@cordoval Common use cases are present fairly well by this RailsCast video: http://railscasts.com/episodes/266-http-streaming
Essentially it allows faster fetching of web assets (JS, CSS, etc) located in the <head></head>, allowing those assets to be fetched as soon as possible before the remainder of the content body is computed and sent to the browser. The end goal is to improve page load speed.
There are other uses cases too like making large body content available quickly to the service consuming it. Think if you were monitoring a live feed of JSON data of newest Twitter comments.
---------------------------------------------------------------------------
by lsmith77 at 2011/12/21 08:54:35 -0800
How does this relate the limitations mentioned in:
http://yehudakatz.com/2010/09/07/automatic-flushing-the-rails-3-1-plan/
Am I right to understand that due to how twig works we are not really streaming the content pieces when we call render(), but instead the entire template with its layout is rendered and only then will we flush? or does it mean that the render call will work its way to the top level layout template and form then on it can send the content until it hits another block, which it then first renders before it continues to send the data?
---------------------------------------------------------------------------
by stof at 2011/12/21 09:02:53 -0800
@lsmith77 this is why the ``stream`` method calls ``display`` in Twig instead of ``render``. ``display`` uses echo to print the output of the template line by line (and blocks are simply method calls in the middle). Look at your compiled templates to see it (the ``doDisplay`` method)
Rendering a template with Twig simply use an output buffer around the rendering.
---------------------------------------------------------------------------
by fabpot at 2011/12/21 09:24:33 -0800
@lsmith77: We don't have the Rails problem thanks to Twig as the order of execution is the right one by default (the layout is executed first); it means that we can have the flush feature without any change to how the core works. As @stof mentioned, we are using `display`, not `render`, so we are streaming your templates for byte one.
---------------------------------------------------------------------------
by fabpot at 2011/12/21 09:36:41 -0800
@Seldaek: yes, I meant ESI with the PHP reverse proxy.
---------------------------------------------------------------------------
by fabpot at 2011/12/21 09:37:34 -0800
@Seldaek: I have `flush()` support for Twig on my todo-list. As you mentioned, It should be trivial to implement.
---------------------------------------------------------------------------
by fzaninotto at 2011/12/21 09:48:18 -0800
How do streaming responses deal with assets that must be called in the head, but are declared in the body?
---------------------------------------------------------------------------
by fabpot at 2011/12/21 09:52:12 -0800
@fzaninotto: What do you mean?
With Twig, your layout is defined with blocks ("holes"). These blocks are overridden by child templates, but evaluated as they are encountered in the layout. So, everything works as expected.
As noted in the commit message, this does not work with PHP templates for the problems mentioned in the Rails post (as the order of execution is not the right one -- the child template is first evaluated and then the layout).
---------------------------------------------------------------------------
by fzaninotto at 2011/12/21 10:07:35 -0800
I was referring to using Assetic. Not sure if this compiles to Twig the same way as javascript and stylesheet blocks placed in the head - and therefore executed in the right way.
---------------------------------------------------------------------------
by fabpot at 2011/12/21 10:34:59 -0800
@Seldaek: I've just added a `flush` tag in Twig 1.5: 1d6dfad4f5
---------------------------------------------------------------------------
by catchamonkey at 2011/12/21 13:29:22 -0800
I'm really happy you've got this into the core, it's a great feature to have! Good work.
Commits
-------
cae7db0 Be more tolerant and also accept <esi:include ...></esi:include>, also if it is not 100% standards compliant.
Discussion
----------
Be more tolerant and also accept <esi:include ...></esi:include>
I know this is not 100% standards compliant, but:
We need to do some XHTML processing on the output using PHP's DOM extension and the underlying libxml2.
libxml2 seems to be unable to keep the <esi:include /> tag as such and will expand it to ```<esi:include ...></esi:include>```.
Note this has nothing to do with having LIBXML_NOEMPTYTAG set (http://php.net/manual/de/domdocument.savexml.php). Rather it seems to be a problem for libxml that it cannot recognize <esi:include> as an "EMPTY" tag (in the DTD sense) because it is not defined in a standard xhtml1-strict DTD.
Commits
-------
4afc6ac Updated CHANGELOG-2.1
3d3239c Added Filesystem Component mention in composer.json
5775a0a Added composer.json
b26ae4a Added README
fbe9507 Added LICENSE
818a332 [Component] Moved Filesystem class to its own component
Discussion
----------
Filesystem component
Related to #2946
William
---------------------------------------------------------------------------
by stof at 2011/12/22 10:58:25 -0800
you need to add the new component in the ``replace`` section of the main composer.json, and you also need to add it as a dependency for FrameworkBundle as it defines a service using it.
---------------------------------------------------------------------------
by stof at 2011/12/22 10:59:34 -0800
and you need to update the changelog file
---------------------------------------------------------------------------
by willdurand at 2011/12/22 11:06:04 -0800
@stof thanks. Is it ok ?
---------------------------------------------------------------------------
by stof at 2011/12/22 11:13:31 -0800
mentioning the move only once in the changelog would probably be enough (and it is especially not needed in the FrameworkBundle section IMO) but otherwise it's fine
To stream a Response, use the StreamedResponse class instead of the
standard Response class:
$response = new StreamedResponse(function () {
echo 'FOO';
});
$response = new StreamedResponse(function () {
echo 'FOO';
}, 200, array('Content-Type' => 'text/plain'));
As you can see, a StreamedResponse instance takes a PHP callback instead of
a string for the Response content. It's up to the developer to stream the
response content from the callback with standard PHP functions like echo.
You can also use flush() if needed.
From a controller, do something like this:
$twig = $this->get('templating');
return new StreamedResponse(function () use ($templating) {
$templating->stream('BlogBundle:Annot:streamed.html.twig');
}, 200, array('Content-Type' => 'text/html'));
If you are using the base controller, you can use the stream() method instead:
return $this->stream('BlogBundle:Annot:streamed.html.twig');
You can stream an existing file by using the PHP built-in readfile() function:
new StreamedResponse(function () use ($file) {
readfile($file);
}, 200, array('Content-Type' => 'image/png');
Read http://php.net/flush for more information about output buffering in PHP.
Note that you should do your best to move all expensive operations to
be "activated/evaluated/called" during template evaluation.
Templates
---------
If you are using Twig as a template engine, everything should work as
usual, even if are using template inheritance!
However, note that streaming is not supported for PHP templates. Support
is impossible by design (as the layout is rendered after the main content).
Exceptions
----------
Exceptions thrown during rendering will be rendered as usual except that
some content might have been rendered already.
Limitations
-----------
As the getContent() method always returns false for streamed Responses, some
event listeners won't work at all:
* Web debug toolbar is not available for such Responses (but the profiler works fine);
* ESI is not supported.
Also note that streamed responses cannot benefit from HTTP caching for obvious
reasons.
Commits
-------
3ae976c fixed CS
84ad40d added cache clear hook
Discussion
----------
[Cache][2.1] Added cache clear hook
Allows bundles to hook into the `cache:clear` command by using the `kernel.cache_clearer` tag instead of using the `event_dispatcher` service.
See #1884
Bug fix: No
Feature addition: Yes
Backwards compatibility break: No
Symfony2 tests pass: Yes
Fixes the following tickets: #1884
References the following tickets: #1884
---------------------------------------------------------------------------
by dustin10 at 2011/12/16 11:03:54 -0800
Rebased to squash all commits into one.
---------------------------------------------------------------------------
by lsmith77 at 2011/12/17 05:27:29 -0800
@fabpot: we figured that priorities wouldn't be needed for cleaning .. haven't tested the PR, but conceptually it looks good to me and aside from the priority stuff its modeled after the cache warners.
---------------------------------------------------------------------------
by dustin10 at 2011/12/19 09:46:26 -0800
@fabpot Updated to pass cache dir to `clear` method.
---------------------------------------------------------------------------
by dustin10 at 2011/12/19 10:02:21 -0800
@stof and @fabpot Another thought I just had. Should the `$this->getContainer()->get('cache_clearer')->clear($realCacheDir);` call in the `CacheClearCommand` be done before the warming?
---------------------------------------------------------------------------
by stof at 2011/12/19 10:03:59 -0800
indeed. the clearing should be done before the warming.
---------------------------------------------------------------------------
by dustin10 at 2011/12/19 10:19:28 -0800
Squashed all commits into one. Let me know if there is anything else.
---------------------------------------------------------------------------
by dustin10 at 2011/12/19 10:31:50 -0800
Fixed extra lines.
Commits
-------
1e370d7 typo fix
93d8d44 added some more infos about Config
27efd59 added READMEs for the bridges
34fc866 cosmetic tweaks
d6af3f1 fixed README for Console
6a72b8c added basic README files for all components
Discussion
----------
added basic README files for all components and bridges
heavily based on http://fabien.potencier.org/article/49/what-is-symfony2 and the official Symfony2 documentation
---------------------------------------------------------------------------
by jmikola at 2011/11/03 13:36:07 -0700
Great work. For syntax highlighting on the PHP snippets, you could add "php" after the three backticks.
---------------------------------------------------------------------------
by lsmith77 at 2011/11/03 13:41:29 -0700
done
---------------------------------------------------------------------------
by stealth35 at 2011/11/03 13:49:31 -0700
Nice job, but you also need to add `<?php`
ex :
``` php
<?php
use Symfony\Component\DomCrawler\Crawler;
$crawler = new Crawler();
$crawler->addContent('<html><body><p>Hello World!</p></body></html>');
print $crawler->filter('body > p')->text();
```
---------------------------------------------------------------------------
by lsmith77 at 2011/11/03 13:56:57 -0700
done
---------------------------------------------------------------------------
by ericclemmons at 2011/11/03 19:57:57 -0700
@lsmith77 Well done! This makes consumption of individual components that much easier, *especially* now that `composer.json` files have been added.
---------------------------------------------------------------------------
by lsmith77 at 2011/11/04 01:18:23 -0700
ok .. fixed the issues you mentioned @fabpot
---------------------------------------------------------------------------
by lsmith77 at 2011/11/11 15:00:27 -0800
@fabpot anything else left? seems like an easy merge .. and imho there is considerable benefit for our efforts to spread the word about the components with this PR merged.
---------------------------------------------------------------------------
by drak at 2011/11/11 18:54:13 -0800
You know, it might be a nice idea to put a link to the documentation for each component if there is some at symfony.com
---------------------------------------------------------------------------
by lsmith77 at 2011/11/12 00:59:14 -0800
i did that in some. but i might have missed a few places.
On 12.11.2011, at 03:54, Drak <reply@reply.github.com> wrote:
> You know, it might be a nice idea to put a link to the documentation for each component if there is some at symfony.com
>
> ---
> Reply to this email directly or view it on GitHub:
> https://github.com/symfony/symfony/pull/2561#issuecomment-2715762
---------------------------------------------------------------------------
by breerly at 2011/11/21 10:28:36 -0800
Pretty excited with this.
---------------------------------------------------------------------------
by dbu at 2011/11/24 00:02:50 -0800
is there anything we can help with to make this ready to be merged?
---------------------------------------------------------------------------
by lsmith77 at 2011/12/18 02:39:23 -0800
@fabpot: seriously .. if you are not going to deliver something "better" and don't provide a reason what is wrong with this .. then its beyond frustrating. i obviously do not claim that these README's are perfect (and certainly still no replacement for proper documentation), but I do claim that in their current form they are a radical step forward to potential users of the Symfony2 components.
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)
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
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.
The Firewall is now executed after the Router. This was needed to have access
to the locale and other request attributes that are set by the Router. This
change implies that all Firewall specific URLs have proper (empty) routes like
`/login_check` and `/logout`.
* 2.0:
[HttpKernel] fixed Content-Length header when using ESI tags (closes#2623)
[HttpFoundation] added an exception to MimeTypeGuesser::guess() when no guesser are available (closes#2636)
[Security] fixed HttpUtils::checkRequestPath() to not catch all exceptions (closes#2637)
[DoctrineBundle] added missing default parameters, needed to setup and use DBAL without ORM
[Transation] Fix grammar.
[TwigBundle] Fix trace to not show 'in at line' when file/line are empty.
* 2.0:
[Form] fixed previous merge
[Form] simplified previous merge
Also identify FirePHP by the X-FirePHP-Version header
[TwigBundle] Extract output buffer cleaning to method
[TwigBundle] Do not clean output buffering below initial level
Fixed rendering of FileType (value is not a valid attribute for input[type=file])
Added tests for string fix in DateTimeToArrayTransformer (8351a11286).
Added check for array fields to be integers in reverseTransform method. This prevents checkdate from getting strings as arguments and throwing incorrect ErrorException when submitting form with malformed (string) data in, for example, Date field. #2609
[Translation] removed unneeded methods
[Translation] added detection for circular references when adding a fallback catalogue
[DomCrawler] trim URI in getURI
[Yaml][Tests] Fixed missing locale string for Windows platforms which caused test to fail
This resulted in issues with PHPUnit 3.6, which will buffer all output and clean them in the end. Since
we cleaned their buffer, the subsequent clean would raise a warning. This is documented in issue 390 of
the PHPUnit tracker.
Closes#2531.
Commits
-------
6b872cf Check if cache_warmer service is available before doing the actual cache warmup
40fb76d [Framework] removed wrong listener
Discussion
----------
fix cache warump exception when service is not available
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: N/A
fixes [Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException]
You have requested a non-existent service "cache_warmer". in console when FrameworkBundle is removed from kernel.
That allows projects that only use HttpFoundation and not HttpKernel to be able to
enforce the HTTP specification "rules".
$request = Request::createFromGlobals();
$response = new Response();
// do whatever you want with the Respons
// enforce HTTP spec
$response->prepare($request);
$response->send();
Within Symfony2, the prepare method is automatically called by the ResponseListener.
Commits
-------
1b57727 removed unused use statements
fd67c78 updated implementation to re-use the existing build() method
59e2e97 improves extensibility between bundles
Discussion
----------
[RFC] Improving extensibility between bundles
This is a quick draft for improving extensibility between different bundles.
The idea behind this is that an extension can provide configurable settings that other bundles can change.
---------------------------------------------------------------------------
by Seldaek at 2011/10/07 13:28:13 -0700
I am not yet sure what I would use it for, but I like the idea.
---------------------------------------------------------------------------
by lsmith77 at 2011/10/07 13:45:19 -0700
can you show a bit more how to use this?
---------------------------------------------------------------------------
by lsmith77 at 2011/10/07 13:47:38 -0700
oh it appears this is an example?
f4e76640a0 (diff-9)
---------------------------------------------------------------------------
by schmittjoh at 2011/10/07 13:57:00 -0700
yes
On Fri, Oct 7, 2011 at 10:47 PM, Lukas Kahwe Smith <
reply@reply.github.com>wrote:
> oh it appears this is an example?
>
>
> f4e76640a0 (diff-9)
>
> --
> Reply to this email directly or view it on GitHub:
> https://github.com/symfony/symfony/pull/2349#issuecomment-2328078
>
---------------------------------------------------------------------------
by schmittjoh at 2011/10/11 01:11:39 -0700
@fabpot, do you have an opinion on this, 👍👎?
Commits
-------
ba6bd4b [HttpKernel] Fix the FileProfileStorage, according to the tests
1654473 [HttpKernel] Create a test to outline the problem with file storage
Discussion
----------
[HttpKernel] Fix the file storage
The file storage was not correctly fetching children back.
* First the test showing the problem
* Second the fix to the test
Solution is to add a file for each stored profile file, containing the list of children tokens.
* 2.0:
bumped Symfony version to 2.0.3-DEV
updated VERSION for 2.0.2
update CONTRIBUTORS for 2.0.2
updated CHANGELOG for 2.0.2
updated vendors for 2.0.2
merged branch helmer/target_path (PR #2228)
Commits
-------
a0329c3 Added lifetime/cleanup support.
365e73a Fixed the find() method and changed the way the profile data is stored.
beeec5e Allow socket dsn (for example mongodb:///tmp/mongodb-27017.sock).
218eaba Fixed storage of time value.
85c3806 Added support for sorting by time like other profiler storage implementations.
73692c6 Fixed MongoDbProfilerStorage::find() when passing empty parameters.
4cd2dec Use token as identifier to make usage of the automatically created index.
Discussion
----------
[2.1] [HttpKernel] MongoDB profiler updates
I fixed one issue within the MongoDbProfilerStorage::find() function and made some more changes.
---------------------------------------------------------------------------
by snc at 2011/09/11 02:28:35 -0700
Please don't merge this in yet. There are some more commits pending...
---------------------------------------------------------------------------
by fabpot at 2011/09/14 01:07:39 -0700
@snc: is it ready for a merge?
---------------------------------------------------------------------------
by snc at 2011/09/14 01:20:32 -0700
Unfortunately not... while testing I found out that the currently merged in implementation does not work completely. The web profiler search function errors because the find function returns the wrong data. I fixed this already but now I have some strange "maximum function nesting level reached" errors when viewing profiles with children. I will work on it later today.
---------------------------------------------------------------------------
by snc at 2011/09/14 13:27:50 -0700
Now only one thing is missing... the generated container code looks like this:
`$this->services['profiler'] = $instance = new \Symfony\Component\HttpKernel\Profiler\Profiler(new \Symfony\Component\HttpKernel\Profiler\MongoDbProfilerStorage('mongodb://localhost/sf2-mongo-profiler/profiler', '', '', 86400), $a);`
The current constructor only uses the first parameter (dsn). What about the username, password and lifetime? Username and passwort can already be passed via the dsn... but the lifetime feature is not part of the interface... should I implement it?
---------------------------------------------------------------------------
by fabpot at 2011/09/15 11:03:02 -0700
The `lifetime` is used to cleanup the database (see https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/Profiler/PdoProfilerStorage.php#L136). So, it should probably be implemented for MongoDB as well (but it can probably be done in another PR).
---------------------------------------------------------------------------
by snc at 2011/09/19 13:42:52 -0700
Sorry for the delay, lifetime support is now implemented. What do you think about an AbstractProfilerStorageTest class to share some testing code between the different implementations (of cause in a separate PR)?
* 2.0:
[HttpKernel] fixed typo
fixed previous merge, done the same change to other occurences
fixes usage of mb_*
Profiler session import fixed.
[Process] workaround a faulty implementation of is_executable on Windows
Swedish translation fix.
[Locale] Fix#2179 StubIntlDateFormatter support yy format
Fixed fourth argument of Filesystem->mirror()
Commits
-------
41b7a19 Updated the tests so that tests will be marked as skipped when there is no MongoDB server present!
233c7db Updated the code to follow the symfony coding standards
7b24de5 Updated the code to follow the symfony coding standard using stof his remarks
fbcbdde - Fixed a small bug - Updated some phpdoc
00fdfec Added a MongoDbProfilerStorage engine
Discussion
----------
[2.1] [HttpKernel] MongoDb storage for Profiler
As a documentbased database like MongoDB is [supposedly fantastic in logging](http://blog.mongodb.org/post/172254834/mongodb-is-fantastic-for-logging) I implemented a storage engine for the profiler that should enable us to use this database as storage for this.
Activate it using this way:
framework:
profiler:
dsn: mongodb://user:pass@location/database/collection
---------------------------------------------------------------------------
by stof at 2011/07/24 11:23:06 -0700
btw, the MongoDB session storage has already be rejected from the core so this should probably be moved to the DoctrineMongoDBBundle (even if it uses only the PHP extension and not Doctrine). @fabpot thoughts about this ?
---------------------------------------------------------------------------
by Wotre at 2011/07/24 11:52:56 -0700
Just my personal opinion, if it is prefered that way I will move this into the DoctrineMongoDBBundle.
While it is reasonable to bundle all Mongo related things together, I do believe that in the case of logging we want to avoid as many depencies as possible. Some exceptions can occur pretty early inside the framework, and it would be a shame if those aren't logged because this layer is written on top of doctrine. I'm not exactly familliar enough with the symfony internals as I only started using it a few days ago, but I can imagine that this can make a difference with some exceptions.
---------------------------------------------------------------------------
by stof at 2011/07/24 11:59:10 -0700
I don't ask you to use Doctrine in this code. It is fine to use the extension directly if it is enough.
Btw, the profiler is *not* used early. :)
---------------------------------------------------------------------------
by Wotre at 2011/07/26 10:45:05 -0700
So... Any final remark whether this should be moved to [the DoctrineMongoDBBundle](https://github.com/symfony/DoctrineMongoDBBundle) or not?
If it has to be moved, any comment on where in that bundle this should be put?
Also, if it has to be moved, how can we arrange the configuration using DI? Currently I've put a line in the FrameworkExtension file to use this engine for everything with a $dsn starting with mongodb; I imagine this kind of ugly depency can't really exist between the FrameworkBundle and another one.
Although it seems completely illogical to me, I will move it, but I do need some directions on how to elegantly do this...
---------------------------------------------------------------------------
by stof at 2011/07/26 11:03:04 -0700
@fabpot what do you think ?
---------------------------------------------------------------------------
by stof at 2011/09/04 01:28:48 -0700
@fabpot what do you think about the place where this should be done ?
Commits
-------
9f0bd03 [HttpKernel] Update tests for FileProfilerStorage
b7032bc [HttpKernel] Update FileProfileStorage to search from EOF
188a5fa [HttpKernel] Override the existing tokens in FileProfilerStorage
b1b1424 [HttpKernel] Delete folders in the profiler cache
88bc3ec [HttpKernel] Fixes standards of FileProfilerStorage
affe66c Merge remote-tracking branch 'origin/master' into new-profiler-storage
ea916c3 [HttpKernel] Coding convention for the file profiler storage
9ae2c8d [HttpKernel] CS in file storage
b415efd [HttpKernel] Add a test for semicolon in file storage test
1c1215f [HttpKernel] Use subfolders for better storage in file storage of profiler
4b1dc1f [HttpKernel] Fix the folder attribute of file storage to private
70f73e1 [HttpKernel] Fix tests for the file storage of profiler
d5313d9 [HttpKernel] Add tests for the file profiler storage
09fc0a2 [HttpKernel] Add Symfony credits to the file storage class for the profiler
d1d5892 [HttpKernel] Finalize the file storage for the profiler
2f65cf2 Add POC for file storage system
Discussion
----------
[2.1] [HttpKernel] File storage for profiler
Symfony2 has some problems when dealing with multiple concurrency queries in the SQLite storage, resulting in a timeout error or terrible lack.
I've implemented after discussions with @fabpot a filesystem storage.
Enable it in your project with :
framework:
profiler:
dsn: "file:%kernel.cache_dir%/profiler"
I also studied the possibility to store only big data string in files and rest in the SQLite, but not concluant.
Results of my measures (4 concurrency, 120 total) :
* SQLite with data : 1057ms
* SQLite without data : 615ms
* MySQL : 40ms
* This File storage : 54ms
---------------------------------------------------------------------------
by alexandresalome at 2011/07/22 12:01:10 -0700
An idea for the find method : a csv file containing ip;url;token
The iteration could be done over a big file, without loading the whole file in memory.
---------------------------------------------------------------------------
by alexandresalome at 2011/07/23 14:22:32 -0700
OK new version, with as explained previously : a CSV file containing the index + file for each profile.
The speed is similar to the speed of MySQL, and no memory overhead should occur with this solution.
---------------------------------------------------------------------------
by alexandresalome at 2011/07/23 14:37:14 -0700
Hm... Created tests, duplicated from SqliteProfilerStorageTest.
Any idea on how to put this code in common ? Is it usual to create a base class for 2 tests ?
---------------------------------------------------------------------------
by alexandresalome at 2011/07/23 14:48:39 -0700
Just tested with 24.000 requests, the 24.001'th request still takes less than 50ms to execute.
The index file is about 2Mb, and iterating the whole file is fast.
---------------------------------------------------------------------------
by alexandresalome at 2011/07/23 14:53:19 -0700
I've filled the file with 120Mb of data, requests are still less than 50ms for executing.
Iterating the index takes more than 30s (so it crashed), but it's because of the amount of lines. 30 seconds = 1,400,000 lines in this computer. The file = 1,500,000 lines
---------------------------------------------------------------------------
by alexandresalome at 2011/07/23 14:56:54 -0700
I've tested it with Linux, is someone can test with Windows
---------------------------------------------------------------------------
by stloyd at 2011/07/24 00:32:32 -0700
IMO to speedup it a bit more and not end up with "crash" (to not end with "limit" of files per directory, also to many files in dir slow down every OS) you should use same method to write as Twig, split up files in to directories. If you do this you can speed up index more, because you can create smaller one per directory.
Also you should fix CS (coding standards).
---------------------------------------------------------------------------
by stloyd at 2011/07/24 02:10:20 -0700
Tested on Win 7, seems ok. Similar speed to sqlite, dunno why ;-) but used a bit less of memory.
---------------------------------------------------------------------------
by alexandresalome at 2011/07/24 02:13:21 -0700
Did you tried with concurrent requests ? It makes sense when you use assetic
and your browser hits the application 4 times simultaneously for CSS
generation
---------------------------------------------------------------------------
by alexandresalome at 2011/07/24 02:17:23 -0700
I used Apache Benchmark for producing results :
ab -c 4 -n 120 URL
---------------------------------------------------------------------------
by alexandresalome at 2011/07/24 02:56:55 -0700
OK I used subfolders, based on last characters (because the first part of token is mostly the same between queries.
---------------------------------------------------------------------------
by stof at 2011/09/04 01:27:15 -0700
@fabpot any news about it ? Can it be merged ?
Commits
-------
cc098a3 [HttpKernel] Add support for xdebug.file_link_format to Debug\ExceptionHandler.php
Discussion
----------
[HttpKernel] Add support for xdebug.file_link_format to Debug\ExceptionHandler
Format file and line as url, if xdebug.file_link_format is set. Inspired by #1893
If not, as classes can be loaded during the boot, there is no way to be sure that
a class will not be already loaded by a third party bundle.
If the Kernel is already booted, we don't included the compiled classes.
Commits
-------
5e80c68 fixes a naming inconsistency
8cfca15 added change to upgrade file
4123ec4 updated some missing references
Discussion
----------
Fix inconsistent naming
---------------------------------------------------------------------------
by jalliot at 2011/07/15 08:15:01 -0700
I think you forgot one commit (the one effectively changing Session and that you reverted in the main repo)
---------------------------------------------------------------------------
by schmittjoh at 2011/07/15 09:07:17 -0700
You're right, fixed now.
Commits
-------
11369eb Fixed phpdoc
dbe1854 Added a AccessDeniedHttpException to wrap the AccessDeniedException.
Discussion
----------
Added a AccessDeniedHttpException to wrap the AccessDeniedException.
This is a proposal to fix#1631
It wraps the AccessDeniedException in an AccessDeniedHttpException when the firewall is not able to handle it itself. This allows getting a 403 response using the standard exception listener in this case.
Note that the app should not throw the AccessDeniedHttpException itself but keep using the AccessDeniedException to let the Security component check if the user is already fully authenticated or if it should give a chance to authenticate.
---------------------------------------------------------------------------
by fabpot at 2011/07/11 07:10:12 -0700
For reference, I've tried something more radical some time ago here: https://github.com/symfony/symfony/pull/369.
---------------------------------------------------------------------------
by stof at 2011/07/11 07:22:07 -0700
my implementation is what @schmittjoh suggested in the comments on your PR.
Commits
-------
f7d0f65 RFC2616 changes
b9a218a [HttpFoundation] set Content-Length header to the length of content
Discussion
----------
[HttpFoundation] set Content-Length header to the length of content
I can't think of why this could be bad but if somebody knows please chime in.
The good thing is that with this change keepalive will work out of the box.
---------------------------------------------------------------------------
by Seldaek at 2011/07/06 05:34:51 -0700
That sounds like a great change. I think it might explain/fix the issues I've encountered with AppCache on my production box. Never had time to look into it, but IIRC I noticed the missing Content-Length, and it seemed to load forever.
---------------------------------------------------------------------------
by fabpot at 2011/07/06 06:46:50 -0700
The `Content-Length` is automatically added by servers like Apache. Moreover, sometimes, you should not add it: http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.4
---------------------------------------------------------------------------
by lenar at 2011/07/06 07:54:45 -0700
It is not added automatically by default. Yes, in case of Apache it is actually added if deflate module is enabled and if that module decides to compress the content (decision based on content-type).
About RFC2616: I will read it and add changes to this PR if applicable.
---------------------------------------------------------------------------
by fabpot at 2011/07/06 08:38:14 -0700
e943fde2ef
---------------------------------------------------------------------------
by Seldaek at 2011/07/06 08:45:22 -0700
@lenar all you have to do is skip setting the Content-Length for `1xx`, `204`, and `304` responses I believe.
---------------------------------------------------------------------------
by Seldaek at 2011/07/06 08:46:54 -0700
But this should maybe be done in sendHeaders() à la `fixContentType`, because you can't be sure about the statusCode before that.
---------------------------------------------------------------------------
by lenar at 2011/07/06 13:55:33 -0700
I propose this based on what I read and understood from RFC2616.
---------------------------------------------------------------------------
by mheleniak at 2011/07/10 03:57:26 -0700
+1
Commits
-------
6786e81 [HttpFoundation] code factorization in UploadedFile
Discussion
----------
[HttpFoundation] code factorization in UploadedFile
As both #1542 and #1544 have been merged.
Commits
-------
5b0f1da [HttpKernel] made WebTestCase methods static
Discussion
----------
[HttpKernel] made WebTestCase methods static
This makes it possible to load fixture data in `::setUpBeforeClass()` which makes tests run much faster.
Also, `createClient()` is not protected instead of public; I'm not sure why it was public in the first place.
Commits
-------
cdf4b6a Checked log levels
a45d3ee Reverted last commit
529381b ControllerNotFound: Changed log level from info to error. Also moved throw exception code block up, to prevent the message from beeing logged multiple times.
7c29e88 Changed log level of "Matched route ..." message from info to debug
dca09fd Changed log level of "Using Controller ..." message from info to debug
Discussion
----------
Log levels
Just wanted to ask if the log level INFO is still correct for these messages?
As there are only four log levels left (DEBUG, INFO, WARNING, ERROR), DEBUG might be the more appropriate level for these messages now.
Let me give an example: An application is logging user actions (maybe to database) in order to assure comprehensibility, e. g. "User %s deleted post %d", "User %s written a message to user %s". These are not warnings of course, so the only suitable log level is INFO.
But they will be thrown together with these very common (at least two per request?) "Using controller..." and "Matched route..." messages when choosing INFO as log level.
---------------------------------------------------------------------------
by Seldaek at 2011/05/24 07:13:18 -0700
Agreed, this stuff is framework debug information.
---------------------------------------------------------------------------
by fabpot at 2011/05/24 08:53:24 -0700
Why do you want to change these two specific ones? The framework uses the INFO level at other places too. Is it a good idea to say that the framework only logs with DEBUG?
---------------------------------------------------------------------------
by stof at 2011/05/24 09:12:53 -0700
Doctrine logs at the INFO level too and I think it is useful to keep it as INFO. Being able to see the queries without having all DEBUG messages of the event dispatcher and security components is useful IMO.
---------------------------------------------------------------------------
by Seldaek at 2011/05/25 02:30:24 -0700
Yeah, that's true, maybe we just need to reintroduce (again, meh:) NOTICE between INFO and WARNING.
@kaiwa Of course the other way could be that you just add your DB handler to the app logger stack. That could be done in a onCoreRequest listener or such, basically you'd have to call `->pushHandler($yourDBHandler)` on the `monolog.logger.app` service. That way your messages will flow to it, but it won't receive noise from the framework stuff since those log on monolog.logger.request and other log channels.
---------------------------------------------------------------------------
by fabpot at 2011/05/25 02:48:26 -0700
@Seldaek: I don't think we need another level. We just need to come up with a standard rules about the usage of each level. Adapted from log4j:
* ERROR: Other runtime errors or unexpected conditions.
* WARN: Use of deprecated APIs, poor use of API, 'almost' errors, other runtime that are undesirable or unexpected, but not necessarily "wrong" (unable to write to the profiler DB, ).
* INFO: Interesting runtime events (security infos like the fact the user is logged-in or not, SQL logs, ...).
* DEBUG: Detailed information on the flow through the system (route match, security flow infos like the fact that a token was found or that remember-me cookie is found, ...).
What do you think?
---------------------------------------------------------------------------
by stloyd at 2011/05/25 02:53:38 -0700
+1 for this standard (also this PR can be merged then), but we should review code for other "wrong" log levels usage (if everyone accept this standard)
---------------------------------------------------------------------------
by fabpot at 2011/05/25 02:55:07 -0700
I won't merge this PR before all occurrences of the logger calls have been reviewed carefully and changed to the right level.
---------------------------------------------------------------------------
by kaiwa at 2011/05/25 02:58:44 -0700
@fabpot: Just noticed these two occurring for every request in my log file. You are right, there are other places where this changes must be applied if we will change the log level.
@stof: Hmm, i see. It is not possible to set the logger separately for each bundle, is it? That maybe would solve the problem. If somebody is interested in seeing the queries, he could set the log handler level to DEBUG for doctrine bundle, but still use INFO for the framwork itself. Plus he could even define a different output file or a completely different handler.
I'm not sure if something like that is possible already (?) or realizable at all... just came into my mind.
---------------------------------------------------------------------------
by Seldaek at 2011/05/25 03:01:07 -0700
Just FYI, from Monolog\Logger (which has CRITICAL and ALERT):
* Debug messages
const DEBUG = 100;
* Messages you usually don't want to see
const INFO = 200;
* Exceptional occurences that are not errors
* This is typically the logging level you want to use
const WARNING = 300;
* Errors
const ERROR = 400;
* Critical conditions (component unavailable, etc.)
const CRITICAL = 500;
* Action must be taken immediately (entire service down)
* Should trigger alert by sms, email, etc.
const ALERT = 550;
The values kind of match http error codes too, 4xx are expected errors that are not really important (404s etc) and 5xx are server errors that you'd better fix ASAP. I'm ok with the descriptions, but I think alert and critical should be included too. I'll probably update Monolog docblocks to match whatever ends up in the docs.
---------------------------------------------------------------------------
by Seldaek at 2011/05/25 03:03:21 -0700
@kaiwa you can do a lot, but not from the default monolog configuration entry, I'm not sure if we can really make that fully configurable without having a giant config mess. Please refer to my [comment above](https://github.com/symfony/symfony/pull/1073#issuecomment-1234316) to see how you could solve it. Maybe @fabpot has an idea how to make this more usable though.
---------------------------------------------------------------------------
by stof at 2011/05/25 03:19:43 -0700
@Seldaek the issue is that the different logging channels are only know in the compiler pass, not in the DI extension. So changing the level in the extension is really hard IMO.
Thus, the handlers are shared between the different logging channels (needed to open the log file only once for instance, or to send a single mail instead of one per channel) and the level is handled in the handlers, not the logger.
I'm +1 for the standard, by adding the distinction between 400 and 500 status calls using ERROR and CRITICAL (which is already the case in the code).
@kaiwa do you have time to review the calls to the logger between DEBUG and INFO or do you prefer I do it ? For instance, the Security component currently logs all message at DEBUG level and some of them should be INFO.
---------------------------------------------------------------------------
by kaiwa at 2011/05/25 04:31:04 -0700
@stof ok i'll do that
---------------------------------------------------------------------------
by kaiwa at 2011/05/25 12:22:51 -0700
Need some help :) I came across `ControllerNameParser::handleControllerNotFoundException()` which leads to redundant log messages currently:
>[2011-05-25 20:53:16] request.INFO: Unable to find controller "AppBaseBundle:Blog" - class "App\BaseBundle\Controller\BlogController" does not exist.
>[2011-05-25 20:53:16] request.ERROR: InvalidArgumentException: Unable to find controller "AppBaseBundle:Blog" - class "App\BaseBundle\Controller\BlogController" does not exist. (uncaught exception) at /home/ruth/symfony3/src/Symfony/Bundle/FrameworkBundle/Controller/ControllerNameParser.php line 87
Is it necessary to call `$this->logger->info($log);` if the InvalidArgumentException will be logged anyway?
---------------------------------------------------------------------------
by stof at 2011/05/25 12:39:22 -0700
Well, the issue is that the ControllerNameParser logs messages and then uses them to throw an exception. I guess the logging call should be removed as it is redundant with the one of the ExceptionListener. @fabpot thoughts ?
---------------------------------------------------------------------------
by kaiwa at 2011/05/27 11:39:25 -0700
I checked all debug, info and log calls. Sometimes it is hard to distinguish between the levels, so it would be great if someone reviews @cdf4b6a. @stof, maybe you want to take a look?
---------------------------------------------------------------------------
by kaiwa at 2011/05/31 12:52:07 -0700
@stof, thanks for your comments. I added some replies above, please let me know your suggestions.
---------------------------------------------------------------------------
by stof at 2011/05/31 14:04:22 -0700
@kaiwa As I said before, all the security logging calls should be DEBUG (most of them) or INFO (the one syaing that authentication succeeded for instance), but not WARN or ERROR as the exception don't go outside the firewall.
* Seldaek/events:
[EventDispatcher] Removed temporary code
[FrameworkBundle] Improved code readability
[FrameworkBundle] Clarified code and fixed regression
Update Core and Security events to latest model
[EventDispatcher] Allow registration of arbitrary callbacks
[EventDispatcher] Remove useless code
[EventDispatcher] Minor memory optimization to getListeners()
[FrameworkBundle] Small optimization, remove some function calls
The main benefit is that in XML/YML files we have common syntax (i.e. core.controller, form.pre_bind) that properly namespaces event names (before: onCoreController was ok, preBind was not).
On the other hand in PHP land we also have namespaced events, CoreEvents::controller, FormEvents::preBind, before it was Events::onCoreController, Events::onPreBind, we now have more context.
* danielholmes/functional_test_changes:
[FrameworkBundle] fixed CS
[FrameworkBundle][HttpKernel] added a default tearDown on the WebTestCase which will shut down the currently used kernel (if there is one) in Web functional tests
This allows people to filter easily between 404 type of responses (that are mostly for users) and real errors in their application (where they probably want to get an email notification
This feature added complexity to the framework but wasn't used in the core anyway.
You can still use the Map class loader in your application though. But most of the time, using the APC
autoloader is just better.
* kriswallsmith/kernel/bundle-extension:
[HttpKernel] added check of default extension alias convention
[AsseticBundle] coding standard and comment tweaks
[HttpKernel] added BundleInterface::getContainerExtension() which is implicitly loaded
