This PR was merged into the master branch.
Commits
-------
7ef2e9d Replaced ContainerAwareTraceableEventDispatcher with TraceableEventDispatcher
Discussion
----------
Replaced ContainerAwareTraceableEventDispatcher with TraceableEventDispatcher
The ContainerAwareTraceableEventDispatcher class was tied to both the
Symfony container and the HttpKernel profiler. It made it non reusable
in another context.
The new TraceableEventDispatcher only keeps the HttpKernel profiler
integration and is able to wrap any other event dispatcher. It makes it
reusable in frameworks using the Symfony HttpKernel component like
Silex.
The only drawback is that we don't have access to the listener
priorities in the collected data anymore (but the listeners are still
ordered correctly). The change is still worth it I think.
---------------------------------------------------------------------------
by stof at 2012-09-27T13:38:24Z
You should add some tests for your new TraceableDispatcher
The ContainerAwareTraceableEventDispatcher class was tied to both the
Symfony container and the HttpKernel profiler. It made it non reusable
in another context.
The new TraceableEventDispatcher only keeps the HttpKernel profiler
integration and is able to wrap any other event dispatcher. It makes it
reusable in frameworks using the Symfony HttpKernel component like
Silex.
The only drawback is that we don't have access to the listener
priorities in the collected data anymore (but the listeners are still
ordered correctly). The change is still worth it I think.
This PR was merged into the master branch.
Commits
-------
3b47088 fixed some classmap paths
Discussion
----------
fixed some classmap paths
Since #5213, an error occurs when requiring the locale or http-foundation package with composer, because it doesn't automatically prepend the target-dir to the classmaps.
Cf. here for example : https://travis-ci.org/#!/doctrine/DoctrineMongoDBBundle/jobs/2608238
cc @jalliot
* progress-helper:
[Console] added some basic tests for the ProgressHelper class
[Console] converted options to proper setters in ProgressHelper
[2.2][Console] Add ProgressHelper
This PR was merged into the master branch.
Commits
-------
8bc9f75 Make secret not be required
Discussion
----------
[FrameworkBundle] Make secret not be required
Bug fix: no
Feature addition: yes
Backwards compatibility break: no (questionable)
Symfony2 tests pass: yes
License of the code: MIT
Fixes the following tickets: -
Todo: -
License of the code: MIT
Documentation PR: -
Framework bundle currently requires that the `secret` key under `framework` be set. The end result is that `kernel.secret` is made available. This is, as far as I can tell, the only required configuration for Framework bundle.
The only thing that currently uses `kernel.secret` is the Form component and then only if CSRF protection is enabled.
In the spirit of making Framework more decoupled and not requiring things in the case you don't need them I would like to make framework secret optional.
I followed the pattern used by CSRF support for when Session is disabled to throw a `LogicException` stating that if CSRF support is enabled then the secret should be set.
For anyone who currently depends on `kernel.secret`, if someone ends up *not* defining `kernel.secret` there will be a dependency error on kernel configuration as `kernel.secret` will not be made available. The biggest downside to this that I could see is that the error message may be slightly confusing; it will complain that there is a dependency on `kernel.secret` when that is generally set by way of adding the Framework secret to the configuration.
As this relates to Symfony Standard Edition, there should be no changes as there is a default secret set there already anyway.
This PR was squashed before being merged into the master branch (closes#3501).
Commits
-------
4f3ded7 Actually this is worse
72a1c65 * Coding standards fixes * Changed `started` to `startTime` * Other fixes/edits
8249928 * Weeks/months/years is probably unrealistic * Set some sensible padding defaults * Use isset() instead of is_array()
37b62bf Fixing bug for elapsed time between 1 and 2 seconds
8fe4568 Special formatting for when there is no maximum set
75f532f Minor docblock updates
e436e1a Adding ProgressHelper for Console Component
Discussion
----------
[2.2][Console] Add ProgressHelper
[](http://travis-ci.org/leek/symfony)
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: Yes
Fixes the following tickets: -
Todo:
- Add unit tests
- Add documentation
--
I find myself needing some sort of progress indicator in most of my Console applications.
If this is something that could possibly be apart of Symfony, that would be great.
**Example:**
---------------------------------------------------------------------------
by jmikola at 2012-03-05T03:08:24Z
Do you have an example of this being used within a console command?
I'd be curious what the performance overhead is. My earliest console commands (nearly 2 years ago) would print status during each iteration (for a database migration) and I found the impact noticeable. After some time, I revised it to only print each X iterations, which often matched up with the batch size inserts/updates.
But for the last year, I've been using [declare(ticks=X)](http://php.net/manual/en/control-structures.declare.php) and have been quite happy with the results. By tuning the tick interrupt, the performance overhead is very small. It's especially helpful when dealing with processing code that is difficult to interrupt with a manual call to update the progress display, as PHP takes care of invoking the tick handler for me. I've thought about making such a console component helper for it, but I think the implementation is too invasive to abstract into a helper.
Here's an example of it being used in OrnicarMessageBundle's [MongoDBMigrateMetadataCommand](https://github.com/ornicar/OrnicarMessageBundle/blob/master/Command/MongoDBMigrateMetadataCommand.php).
---------------------------------------------------------------------------
by leek at 2012-03-05T04:05:29Z
@jmikola: Here is a simple example:
```php
<?php
// ...
protected function execute(InputInterface $input, OutputInterface $output)
{
$progress = $this->getHelperSet()->get('progress');
$progress->start($output, 50);
$i = 0;
while ($i++ < 50) {
usleep(mt_rand(20000, 200000));
$progress->advance();
}
$progress->finish();
}
```
The performance overhead shouldn't be much more than a standard `$output->write()` call. When used with a loop doing 1000's of iterations, you can set the `redrawFreq` option to something more appropriate to control how often the progress indicator is redrawn to the console.
---------------------------------------------------------------------------
by leek at 2012-03-10T10:05:32Z
Added some minor updates along with an example GIF of 2 of the progress bars (see edited PR).
---------------------------------------------------------------------------
by jmikola at 2012-03-10T15:22:29Z
Why does `1 sec` flash over to `1 secs` before `2 secs` is rendered?
---------------------------------------------------------------------------
by henrikbjorn at 2012-03-10T15:26:08Z
👍
---------------------------------------------------------------------------
by leek at 2012-03-10T16:07:08Z
@jmikola: Thanks! I didn't even notice that. Fixed.
---------------------------------------------------------------------------
by drak at 2012-03-11T09:04:58Z
What an amazing PR. I feel like I just have to write some code that uses this feature just because it's there!
---------------------------------------------------------------------------
by henrikbjorn at 2012-03-11T09:55:50Z
This is needed a lot, we have a bunch of import scripts where this is useful.
@fabpot what are your thoughts on this?
---------------------------------------------------------------------------
by francoispluchino at 2012-03-14T12:34:38Z
👍
---------------------------------------------------------------------------
by vicb at 2012-03-14T13:00:42Z
could you please order the properties & methods by visibility according to the Sf2 CS.
---------------------------------------------------------------------------
by leek at 2012-03-14T19:08:52Z
No problem - I'll make the requested changes tonight.
---------------------------------------------------------------------------
by stof at 2012-04-03T22:48:45Z
@fabpot ping
---------------------------------------------------------------------------
by stloyd at 2012-04-14T09:46:31Z
@fabpot Any hope to get this in 2.1 ?
---------------------------------------------------------------------------
by mvriel at 2012-05-15T19:28:34Z
👍
Tried it out by manually including it in my project and works like a charm
---------------------------------------------------------------------------
by blaugueux at 2012-05-23T18:46:15Z
Up ! It will be great to have this feature in the next release.
@fabpot ping
---------------------------------------------------------------------------
by guilhermeblanco at 2012-05-28T22:58:35Z
@fabpot tried on my app and everything works fine.
Any plans to merge this one into 2.1?
---------------------------------------------------------------------------
by damonjones at 2012-05-29T02:31:39Z
+1
This would be a very nice feature to have in 2.1.
---------------------------------------------------------------------------
by fabpot at 2012-05-29T06:18:57Z
This is scheduled for 2.2.
---------------------------------------------------------------------------
by Burgov at 2012-08-16T13:04:34Z
I have a service which downloads a file using wget though the console component, and reads the progress from stderr. Rather than advancing in steps, i'd like to be able to set the current progress. Something like this method might be a nice addition:
```php
public function setCurrent($value, $redraw = false)
{
$this->advance($value - $this->current, $redraw);
}
```
This PR was merged into the master branch.
Commits
-------
92e10a8 Updated HttpFoundation and Locale for proper Composer autoloading
Discussion
----------
Updated HttpFoundation and Locale for proper Composer autoloading
This PR uses better Composer autoloading strategy for the stubs in HttpFoundation and Locale.
It also fixes a bug inside HttpFoundation's composer.json file where the path for SessionHandlerInterface was wrong.
[](http://travis-ci.org/jalliot/symfony)
After merging this PR and updating the vendors of the SE, you can also merge symfony/symfony-standard#387
---------------------------------------------------------------------------
by datiecher at 2012-09-05T11:15:39Z
Any updates on this issue?
---------------------------------------------------------------------------
by jalliot at 2012-09-05T16:43:46Z
Well I guess it is up to @fabpot to decide now :)
---------------------------------------------------------------------------
by drak at 2012-09-07T11:59:22Z
> It also fixes a bug inside HttpFoundation's composer.json file where the path for SessionHandlerInterface was wrong.
If so should be part of a separate PR imo.
This PR was squashed before being merged into the master branch (closes#5518).
Commits
-------
3303ca2 WPB and WDT improvements
be194cb Changed icons to be a bit more consistent
08241b8 Added minimize option to Web Profiler panels
Discussion
----------
[2.2][WebProfilerBundle] Added minimize option to Web Profiler panels
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: ~
Todo: ~
License of the code: MIT
Documentation PR: ~
I've added a minimize option to the profiler bundle so that you can have more space to work with on the panels.
You can view it in action here:http://sf2demo.rodb.ro/app_dev.php/
Feedback is welcomed!
Thanks!
---------------------------------------------------------------------------
by dlsniper at 2012-09-15T13:36:51Z
I could add a remember option via a cookie if you think this would help, I know I'd want one, but I'm not sure about the general opinion about this. Let me know if I should do it.
---------------------------------------------------------------------------
by stof at 2012-09-15T17:05:58Z
The profiler is totally broken when minimizing the menu in your demo.
---------------------------------------------------------------------------
by lennerd at 2012-09-15T17:10:54Z
I would not make it disappear completely. So I think a combination of Cookie and a small visual for open it again would be great to have.
---------------------------------------------------------------------------
by dlsniper at 2012-09-15T17:15:06Z
@stof I've only enabled a few panels to work in the demo, rest defaults to DB panel. If this is what you mean then it's not broken, it's designed to do so. I've tested the thing on Opera, FF and Chrome (on Linux) before uploading the demo/PR so I'm not sure what's broken. Can you please provide a screenshot?
@lennerd I could be doing something like only display it in the upper left corner and appear on mouse over as an overlay. Would that be better?
---------------------------------------------------------------------------
by stof at 2012-09-15T17:21:22Z
@dlsniper what I mean is that the text of the menu does not disappear. It simply goes over the panel itself as the menu becomes smaller. And this appears for all panels I tried.
---------------------------------------------------------------------------
by stof at 2012-09-15T17:22:32Z
hmm, sorry. It is a browser cache issue. It seems like your server was sending cache headers for the assets, and as I already looked at the demo previously (for your DoctrineBundle PR), it kept the old CSS
---------------------------------------------------------------------------
by dlsniper at 2012-09-15T17:25:09Z
@stof no problem, the server is configured a bit more on caching side in order to speed it up and save bandwidth ;)
---------------------------------------------------------------------------
by lennerd at 2012-09-15T17:38:41Z
@dlsniper I would use the close button changing to maybe an arrow in the bottom right. So it's more intuitive and you can simply show and hide it if you only want to take a quick look at a small detail behind it.
---------------------------------------------------------------------------
by henrikbjorn at 2012-09-15T18:08:02Z
What about making this the default, the icons are self explanatory already. The title would then be the "link" text instead.
---------------------------------------------------------------------------
by dlsniper at 2012-09-15T20:30:51Z
@henrikbjorn I wouldn't make this by default as new people might find it a bit confusing. Hence the suggestion to use the cookie to remember the preference.
---
Also I'm trying not to break the current format of the menu too much as hiding all that stuff by hand is pain but if I'm allowed to break the current way of displaying the left menu then this is going to be easy.
What I didn't understood so far is why is the toolbar displayed on the top as well since we have it on the left side already so I've remove it from my current changes (will be up soonish).
---------------------------------------------------------------------------
by dlsniper at 2012-09-15T21:10:03Z
@lennerd what exactly do you mean by 'I would use the close button' there's no close button on the profiler page, only on the toolbar.
---------------------------------------------------------------------------
by lennerd at 2012-09-15T21:21:20Z
That was the button I was talking about. So that there is a little close button at the bottom right for toggling the toolbar.
---------------------------------------------------------------------------
by dlsniper at 2012-09-15T23:14:06Z
I've changed the way the menu minimizes now, it hides in the top left corner and it maintains its state on refresh. I'll do something similar for the toolbar tomorrow.
You can view it on the same URL.
Please do leave feedback. Thanks!
---------------------------------------------------------------------------
by lennerd at 2012-09-16T01:02:27Z
Sorry. I misunderstand your PR.
---------------------------------------------------------------------------
by stof at 2012-09-16T03:01:06Z
@dlsniper The toolbar is displayed at the top because it gives a quick overview without having to go in each panel. So removing it is a bad idea IMO.
And hiding everything is a bad idea IMO. It means navigating is impossible, making it usable when minimizing it (and btw, this would make the cookie a non-sense as it would hide the menu for subsequent pages)
---------------------------------------------------------------------------
by fabpot at 2012-09-16T06:38:08Z
-1 for removing the toolbar at the top
I prefer the first version where you only hide the menu text but leave the icons. Keeping the state in a cookie is also a must (that cookie might be used to store some other states in the profiler too).
---------------------------------------------------------------------------
by Partugal at 2012-09-16T08:14:11Z
i'm not see first version but show icons without text is more useful.
imho minimize trigger should be always placed on top as it showns in minized state
---------------------------------------------------------------------------
by Partugal at 2012-09-16T08:24:49Z
http://s14.postimage.org/qkdcr8d4h/image.png
---------------------------------------------------------------------------
by dlsniper at 2012-09-16T09:06:50Z
@fabpot I've just had a look on how the timeline stores the selected value and it's using the local storage capabilities. Should I drop the cookie and use the local storage as well to have some sort of uniformity?
Also is there any reason why no generic JS library is used? I'm thinking now about jQuery mostly but any other should do just fine I think. I'm not saying that we should use a library when displaying the WDT as it might bump into issues with the frontend but for the rest of the profiler I guess it wouldn't be a problem to use a library, no?
---------------------------------------------------------------------------
by fabpot at 2012-09-16T09:15:37Z
Let's use the local storage for better consistency. I don't want to embed a JS library as we only need basic JS scripts.
---------------------------------------------------------------------------
by lennerd at 2012-09-16T10:29:20Z
@dlsniper Do we need the up and down arrows any longer?
---------------------------------------------------------------------------
by dlsniper at 2012-09-16T14:24:27Z
I've added a minimize mode to the toolbar but the 'design' isn't the best all around, I'll try to improve it in the future.
It also remembers the state of the toolbar so that you don't need to hide it every time.
@lennerd we don't need the up/down arrows for now, I've removed on the last commit. Thank you for the new icons ;)
L.E.
I've made some sort of rounded corner/gradient background in for the minimized toolbar
---------------------------------------------------------------------------
by dlsniper at 2012-09-18T22:02:35Z
@pborreli thanks for the idea regarding to auto-minimize on window resize, I'll implement it soon, I don't really have time right now to add the event handling part to Sfjs.
L. E.
I'm not going to implement the auto-resize as it proved not to be that useful given the fixed width of the panel. If it proves to be a requested thing then I'll improve it if no one else does it before me :)
---------------------------------------------------------------------------
by dlsniper at 2012-09-19T20:52:43Z
If there's nothing else left to be changed/improved/added, I'll lift the WIP tag of this PR so that it can be merged if you consider it.
@fabpot, if this gets its way to the repository, should I rebase this before merging so that it catches the next Symfony 2.1 release as it doesn't break anything? I don't mean the very next release which I've read that it would be done when Doctrine will release their new version but the version after that.
---------------------------------------------------------------------------
by fabpot at 2012-09-19T20:56:45Z
This is a new feature, so it can only be included in the master branch.
---------------------------------------------------------------------------
by dlsniper at 2012-09-23T12:27:41Z
As soon as this feature goes in master I'll start working on adding AJAX requests to the toolbar to make it even more useful.
Let me know if this change is good to merge or needs more work.
Thanks @stof for all the input and @lennerd for the icons.
---------------------------------------------------------------------------
by stof at 2012-09-23T13:40:38Z
Adding which ajax requests to the toolbar ?
---------------------------------------------------------------------------
by dlsniper at 2012-09-23T13:49:43Z
'Userland' AJAX requests, so that one could access the information from an AJAX request more straight forward
This PR was merged into the master branch.
Commits
-------
7fe44da Whitespace corrections
6d30f20 Switched to using a method to get original class name that did not require string parsing
3c8d607 Changed test to use a longer form, complete check of the contents of the trace
de77c88 Whitespace correction
03a7bb9 Added a unit test to verify incomplete classes do not cause flatten exception to throw
e562418 Added a bit to convert incomplete objects in the error message
Discussion
----------
[HttpKernel] Added a bit to convert incomplete objects in the error message
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes*
\* no - errors in MongoDbSessionHandlerTest attempting to access private property, however running just src/Symfony/Component/HttpKernel does pass.
Objects of class __PHP_Incomplete_Class are only sometimes an object.
```
$object = unserialize('O:14:"BogusTestClass":0:{}');
$object instanceof __PHP_Incomplete_Class; // true
is_object($object); // false
gettype($object); // "object"
```
Since it is "not an object", the flatter attempts to turn it into a string, it triggers:
```
__PHP_Incomplete_Class could not be converted to string.
```
Which then hides the root error message.
---------------------------------------------------------------------------
by pborreli at 2012-09-18T16:16:33Z
have you seen that http://stackoverflow.com/questions/965611/forcing-access-to-php-incomplete-class-object-properties looks like you can still access the object even if it's a __PHP_Incomplete_Class with foreach
---------------------------------------------------------------------------
by rrehbeindoi at 2012-09-18T16:38:38Z
Thank you for the tip re: foreach.
* 2.1:
Added Base64 encoding, decoding to MongoDBProfilerStorage
Fix duplicated code and a field name
refactor src/Symfony/Component/Translation/Loader/MoFileLoader.php
fixed typo
Update src/Symfony/Component/Validator/Resources/translations/validators.pl.xlf
fixed issue #5596 (Broken DOM with the profiler's toolbar set in position top)
[Form] Fixed the testsuite for PHPUnit 3.6 as travis still uses it
added dirs generated by build-data.php in locale component to .gitignore
[Process] Fixed bug introduced by 7bafc69f38.
[Process][Tests] Prove process fail (Add more test case)
[Process][Tests] Prove process fail
[HttpFoundation] Fixed the tests
[DomCrawler] Added test for supported encodings by mbstring
[Config] Fixed preserving keys in associative arrays
[Console] Fixed return value for Command::run
[Locale] Fixed tests
[Console] Fix some input tests
[Filesystem] Fixed tests on Windows
[Config] Fixed tests on Windows
This PR was merged into the 2.1 branch.
Commits
-------
c120c4d Added Base64 encoding, decoding to MongoDBProfilerStorage
Discussion
----------
[HttpKernel] Added UTF8 encoding, decoding to MongoDBProfilerStorage
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
It didn't take long for us to hit a non-utf8 error using the new `MongoDBProfilerStorage`. I'm pretty sure the culprit was the Switfmailer DataCollector serializing a message with a PDF attachment.
I thought this would be a good failsafe, although one could ask whether mail message attachments should be serialized at all...
---------------------------------------------------------------------------
by krmcbride at 2012-09-28T17:28:02Z
Switched encoding/decoding to base64
This PR was merged into the 2.1 branch.
Commits
-------
d7623ae [DomCrawler] Added test for supported encodings by mbstring
Discussion
----------
[2.1][DomCrawler] Added test for supported encodings by mbstring
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
License of the code: MIT
---------------------------------------------------------------------------
by fabpot at 2012-09-25T09:35:18Z
As this is a bug fix, it should be done on the 2.0 branch. Thanks.
---------------------------------------------------------------------------
by stof at 2012-09-25T09:41:59Z
@fabpot 2.0 does not contain the code trying to convert the encoding.
This PR was merged into the master branch.
Commits
-------
45bf523 Optimize autoload prefix in composer.json
Discussion
----------
Optimize autoload prefix in composer.json
By having more specific autoload prefixes it is possible to reduce the
number of stat calls made. Also it prevents conflicts with similar
namespaces.
See ref: e58c6cdbf5
SessionHandlerInterface is left intentionally.
---------------------------------------------------------------------------
by sstok at 2012-09-28T08:41:25Z
@Seldaek Is this true? And if so shouldn't this be optimized in Composer it self?
---------------------------------------------------------------------------
by stof at 2012-09-28T08:48:05Z
@sstok I cannot be optimized in Composer itself. It already optimize things by putting most specific prefixes first.
Composer matches a prefix. The most specific the prefix is, the better (as it is less likely to match other classes).
For components, it is quite useless as we don't have component with a name being a prefix for the name of another one.
For the main package, it can indeed optimize things if someone was using ``Symfony_`` in an old-way package. This is not likely as there is no reason to have some new Symfony code using the PHP 5.2 naming and symfony1 was not using PSR-0. but it is indeed a working solution.
This was occuring in Zend Framework as using ``Zend`` for the namespace prefix of ZF2 would match when loading ``Zend_Registry`` (and this was causing issues when using ZF1 and ZF2 together as the PSR-0 path for ``Zend_Registry`` or ``Zend\Registry`` would be the same, thus loading the wrong file)
---------------------------------------------------------------------------
by Slamdunk at 2012-09-28T09:04:17Z
I know the main reason occurred in ZF doesn't happen in Symfony, but I see only benefits for taking it even so.
This PR was submitted for the master branch but it was merged into the 2.1 branch instead (closes#5602).
Commits
-------
a7d8a74 [Console] Fix duplicated code and a field name
Discussion
----------
[Console] Fix duplicated code and a field name
* Renamed a field name in CommandTesterTest
* Deleted duplicated code in testRenderException
---------------------------------------------------------------------------
by fabpot at 2012-09-27T12:16:40Z
Can you submit this PR on the 2.1 branch as this is where it should be fixed? Thanks.
This PR was submitted for the master branch but it was merged into the 2.1 branch instead (closes#5623).
Commits
-------
4a17053 refactor src/Symfony/Component/Translation/Loader/MoFileLoader.php
Discussion
----------
Refactor extract
proposed patch for what looked falsly as usage of undefined variables (#5620)
---------------------------------------------------------------------------
by stof at 2012-09-27T18:05:22Z
could you squash your commits ?
---------------------------------------------------------------------------
by pborreli at 2012-09-27T18:12:56Z
check http://symfony.com/doc/current/contributing/code/patches.html#step-3-submit-your-patch
---------------------------------------------------------------------------
by h4ck3rm1k3 at 2012-09-27T18:18:53Z
Wow, that is nice, thanks for taking the time to teach me these tricks.
Commits
-------
27b2df9 [Process] Fixed bug introduced by 7bafc69f38.
7a955c0 [Process][Tests] Prove process fail (Add more test case)
598dcf3 [Process][Tests] Prove process fail
Discussion
----------
[Process][Tests] Prove process fail with chained commands
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: no
Fixes the following tickets: -
Todo: Fix that
License of the code: MIT
This PR is against 2.1 branch. Previous PR was #5575
This PR try to hiligh a regression in Process component.
``` php
$process = new Process("echo -n 1 && echo -n 1");
// or $process = new Process("echo -n 1 ; echo -n 1");
$process->run();
var_dump('11' == $process->getOutput()); // false,
var_dump($process->getOutput()); // '1',
```
This test failed because of PR #5543 ; see 7bafc69f38 (L0R233)
---------------------------------------------------------------------------
by romainneutron at 2012-09-25T13:05:45Z
You've to revert the change that causes the fail (ie: remove https://github.com/symfony/symfony/blob/2.1/src/Symfony/Component/Process/Process.php#L233)
---------------------------------------------------------------------------
by romainneutron at 2012-09-25T13:06:56Z
BTW, removing this line re-open #5030
---------------------------------------------------------------------------
by stof at 2012-09-25T13:11:15Z
@lyrixx please add a commit reverting the addition of ``exec`` in the case of sigchild not being used (only this addition, not the full commit you linked) as it should fix your test.
---------------------------------------------------------------------------
by stof at 2012-09-25T13:12:21Z
@fabpot btw, this regression is quite important. As I said in the previous PR, it impacts composer in a bunch of places.
---------------------------------------------------------------------------
by romainneutron at 2012-09-25T13:30:07Z
You reverted too much things, you just had to remove line 233
---------------------------------------------------------------------------
by stof at 2012-09-25T13:42:49Z
@lyrixx I explicitly asked you to revert only the ``exec`` addition for the case without sigchild.
---------------------------------------------------------------------------
by lyrixx at 2012-09-25T13:55:57Z
@stof Sorry, i fixed that.
---------------------------------------------------------------------------
by romainneutron at 2012-09-25T13:56:26Z
@lyrixx just remove the two last commit, edit Process.php and remove line 233
---------------------------------------------------------------------------
by lyrixx at 2012-09-25T13:59:59Z
@romainneutron I think it's ok now.
---------------------------------------------------------------------------
by romainneutron at 2012-09-25T14:11:28Z
yep it's good :)
Commits
-------
2dcb2d7 [Filesystem] Fixed tests on Windows
Discussion
----------
[2.1][Filesystem] Fixed tests on Windows
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
License of the code: MIT
Commits
-------
fc1e844 [Locale] Fixed tests
Discussion
----------
[2.1][Locale] Fixed tests
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
License of the code: MIT
Commits
-------
65281fb [Config] Fixed tests on Windows
Discussion
----------
[2.1][Config] Fixed tests on Windows
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
License of the code: MIT
Commits
-------
b961ee3 [HttpFoundation] Fixed the tests
Discussion
----------
[HttpFoundation] Fixed the tests
b8a2f8c646 reverted the use of the username
and password in the getSchemeAndHost method but forgot to revert the
corresponding tests.
Commits
-------
8ab3054 added dirs generated by build-data.php in locale component to .gitignore
Discussion
----------
added dirs generated by build-data.php in locale component to .gitignore
This is to complete the PR #5411.
Paging @eriksencosta.
---------------------------------------------------------------------------
by eriksencosta at 2012-09-25T14:54:06Z
For me it's ok!
Batman?
---------------------------------------------------------------------------
by shieldo at 2012-09-25T14:55:38Z
Kapow! Thanks for checking it over!
---------------------------------------------------------------------------
by shieldo at 2012-09-25T15:41:05Z
As @stof pointed out, git does read .gitignore files in sub-paths. So I've modified the commit so the change is in the Locale component only.
Commits
-------
530bd22 fixed issue #5596 (Broken DOM with the profiler's toolbar set in position top)
Discussion
----------
fixed issue #5596 (Broken DOM with the profiler's toolbar set in position top)
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #5596
Todo: -
License of the code: MIT
Documentation PR: -
Whatever the toolbar position, the html code associated to it may be placed at the end of the page (and this will be better from a webperf point of view).
---------------------------------------------------------------------------
by stof at 2012-09-25T17:47:53Z
The spacer div will not be in the right place when using ``position: top`` as it will be at the end of the body.
---------------------------------------------------------------------------
by pborreli at 2012-09-25T17:53:03Z
@stof what is the spacer div ? i guess all the profiler is in absolute so i guess it should work (and I'm sure @xavierlacot already tested it :))
---------------------------------------------------------------------------
by pborreli at 2012-09-25T17:55:55Z
@xavierlacot so maybe we should refactor
```
$pos = $posrFunction($content, '</body>');
$content = $substrFunction($content, 0, $pos).$toolbar.$substrFunction($content, $pos);
```
with something like
```
$content = str_replace('</body>', $toolbar.'</body>', $content);
```
What do you think ?
---------------------------------------------------------------------------
by stof at 2012-09-25T17:58:35Z
@pborreli The toolbar is in position fixed. But to avoid hiding some of the content of your page, another div is added on with a margin, to force keeping some space after the content for the toolbar. With this change, the toolbar HTML is always at the end, so the 40px space is always added at the bottom of the page even if the toolbar is added at the top.
---------------------------------------------------------------------------
by pborreli at 2012-09-25T18:03:08Z
@stof maybe we should just fix the body/html margin-top in that case no ? or find a better solution, anyway I think the actual way to do it is bad, `<body>` and `</body>` are not even mandatory in html5, IMHO i would just put it at the end of file without any check, then fix it with some css and/or js
---------------------------------------------------------------------------
by stof at 2012-09-25T18:06:58Z
@pborreli Putting it at the end after the closing ``<html>`` tag would make the page invalid for people defining the markup fully. It is a bad idea.
And anyway, detecting the body tag is still important, to avoid injecting the toolbar in partial page content (be it ESI requests or parts loaded through AJAX).
Oh, and ``$content = str_replace('</body>', $toolbar.'</body>', $content);`` would not fix#5596 but make it worse: it would also inject the toolbar in the head even when being placed at the bottom (keeping it at the bottom too).
---------------------------------------------------------------------------
by pborreli at 2012-09-25T18:18:46Z
@stof for detecting ajax you already have `if ($request->isXmlHttpRequest()) {` called few lines before,
the proposal for `$content = str_replace('</body>', $toolbar.'</body>', $content);` was only a refactoring for the above PR
---------------------------------------------------------------------------
by stof at 2012-09-25T18:26:34Z
@pborreli ESI requests are not AJAX requests. So simply appending at the end would still break them.
And your code is *not* a refactoring. Your ``str_replace`` will replace **all** occurences of ``</body>``, not just the last one. See the related issue to understand why it makes a difference
---------------------------------------------------------------------------
by pborreli at 2012-09-25T18:38:11Z
ok I'm all wrong.
---------------------------------------------------------------------------
by xavierlacot at 2012-09-25T18:51:42Z
@stof, please review the last commit, which injects the wdt container at the top of the page in javascript, using the browser's DOM capacities. This fixes the spacer problem that you noticed.
---------------------------------------------------------------------------
by stof at 2012-09-25T18:55:51Z
Well, you are now breaking things when the spacer should be at the bottom as you are always putting the spacer at the top.
---------------------------------------------------------------------------
by stloyd at 2012-09-25T19:06:14Z
@xavierlacot Pass `position` variable to template and change:
```diff
- document.body.insertBefore(sfwdt, document.body.firstChild);
+ {% if position == 'bottom' -%}
+ document.body.appendChild(sfwdt);
+ {%- else -%}
+ document.body.insertBefore(sfwdt, document.body.firstChild);
+ {%- endif %}
---------------------------------------------------------------------------
by stof at 2012-09-25T20:18:31Z
@xavierlacot could you squash your commits ?
---------------------------------------------------------------------------
by xavierlacot at 2012-09-25T21:32:47Z
@stof done. Thanks for the review :-)
Commits
-------
ef288a2 [Form] Fixed the testsuite for PHPUnit 3.6 as travis still uses it
Discussion
----------
[Form] Fixed the testsuite for PHPUnit 3.6
Travis is still using it so this avoids making all build fail just because of it.
Commits
-------
c869a65 [Console] Fixed return value for Command::run
Discussion
----------
[2.1][Console] Fixed return value for Command::run
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
License of the code: MIT
API says that Command::run returns integer. This is also necessary if I want to run nested commands.
