[EventDispathcer] Fix removeListener
merged branch vicb/event (PR #7023)

This PR was squashed before being merged into the 2.0 branch (closes #7023).

Commits
-------

87f3db7 [EventDispathcer] Fix removeListener

Discussion
----------

[EventDispathcer] Fix removeListener

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | ~
| License       | MIT
| Doc PR        | ~

Todo

- [x] Add a UT

I won't have time to add a test before next Friday but this PR could save some debugging (especially with Silex & Closures)

---------------------------------------------------------------------------

by vicb at 2013-02-09T23:32:51Z

Probably related to https://bugs.php.net/bug.php?id=62976, I need to do some more investigation - I use 546. anyway the fix is valid for whatever php version.

---------------------------------------------------------------------------

by vicb at 2013-02-10T10:24:46Z

This **is** related to the PHP bug mentioned above, see http://3v4l.org/Q6WKj:

```
Output for 5.3.18 - 5.3.21, 5.4.8 - 5.5.0alpha4
bool(false)

Output for 5.3.0 - 5.3.17, 5.4.0 - 5.4.7
Notice: Object of class Klass could not be converted to int in /in/Q6WKj on line 9
Notice: Object of class Closure could not be converted to int in /in/Q6WKj on line 9
int(0)
```

@fabpot anything more needed to merge this ?

---------------------------------------------------------------------------

by fabpot at 2013-02-10T10:26:52Z

Is it possible to add a test?

---------------------------------------------------------------------------

by vicb at 2013-02-10T10:29:34Z

It is, for php versions < fixed version, I'll do that

---------------------------------------------------------------------------

by vicb at 2013-02-10T10:42:01Z

@fabpot ready Sir !

---------------------------------------------------------------------------

by vicb at 2013-02-10T10:44:35Z

well I can probably add an assert, please wait !
[EventDispatcher] Added assertion.
merged branch jakzal/2.0-event (PR #7038)

This PR was merged into the 2.0 branch.

Commits
-------

4ce9ac3 [EventDispatcher] Added assertion.

Discussion
----------

[EventDispatcher] Added assertion

re #7023, I think it actually makes sense to add an assertion here. It reveals the intent of the test (listener is not removed).

---------------------------------------------------------------------------

by vicb at 2013-02-10T11:32:06Z

I don't think the assertion would fail with the former code, would it ?

---------------------------------------------------------------------------

by vicb at 2013-02-10T11:34:30Z

I mean an error would be generated even before the assertion.

---------------------------------------------------------------------------

by jakzal at 2013-02-10T11:37:05Z

Yes, it would fail before getting to the assertion with: *Object of class Closure could not be converted to int*.

However, this is something good to test for (and document - test is a documentation). We're not checking if type is taken into account in other tests. This test might still fail if code inside removeListener() changed.

---------------------------------------------------------------------------

by vicb at 2013-02-10T11:42:29Z

I don't really understand your point and think it is a bit useless here but I am not against your change - I don't argue that test is doc though.

---------------------------------------------------------------------------

by jakzal at 2013-02-10T15:38:09Z

Assertion is indeed useless for the bug you discovered and fixed. I think it's still worth to have it there for  other reason:
* test readability and completeness - with an assertion it's more clear that we don't expect the listener to be removed with the `removeListener()` call if passed argument doesn't match the one added before

If you still don't see my point just close this PR :)

---------------------------------------------------------------------------

by vicb at 2013-02-10T17:34:35Z

What I mean is that you are unit testing php and it is not a job for sf. So it is not strictly required but as it doesn't hurt, let's merge your change.

Jakub Zalas <notifications@github.com> wrote:

>Assertion is indeed useless for the bug you discovered and fixed. I
>think it's still worth to have it there for  other reason:
>* test readability and completeness - with an assertion it's more clear
>that we don't expect the listener to be removed with the
>`removeListener()` call if passed argument doesn't match the one added
>before
>
>If you still don't see my point just close this PR :)
>
>
>---
>Reply to this email directly or view it on GitHub:
>https://github.com/symfony/symfony/pull/7038#issuecomment-13351469
[DependencyInjection] fixed tests (refs #7017)
[Process] Fix regression introduced in #6620 / 880da01c49a9255f5022ab7e18bca38c18f56370, fixes #7082
merged branch Seldaek/regressionfix (PR #7102)

This PR was merged into the 2.0 branch.

Commits
-------

179cd58 [Process] Fix regression introduced in #6620 / 880da01c49a9255f5022ab7e18bca38c18f56370, fixes #7082

Discussion
----------

[Process][2.0] getcwd failure fix

Fix regression introduced in #6620
Fixes #7082

For reference, here is the current behavior I saw:

PHP 5.4.11, windows:

```
5.4.11\php.exe -r "chdir('c:\\'); var_dump(getcwd()); $p = proc_open('pwd', [['pipe', 'r'], ['pipe', 'w'], ['pipe', 'w']], $pipes, null); var_dump(stream_get_contents($pipes[1]));"
string(3) "C:\\"
string(14) "/c/Users/seld\n"
```

(I use pwd which is a unix util so it dumps a funny path, but don't look at that)

PHP 5.5alpha4, windows (seems fixed):

```
5.5.0a4\php.exe  -r "chdir('c:\\'); var_dump(getcwd()); $p = proc_open('pwd', [['pipe', 'r'], ['pipe', 'w'], ['pipe', 'w']], $pipes, null); var_dump(stream_get_contents($pipes[1]));"
string(3) "C:\"
string(3) "/c\n"
```

PHP 5.3.10, ubuntu:

```
php -r "chdir('/'); var_dump(getcwd()); \$p = proc_open('pwd', array(array('pipe', 'r'), array('p
ipe', 'w'), array('pipe', 'w')), \$pipes, null); var_dump(stream_get_contents(\$pipes[1]));"
string(1) "/"
string(2) "/\n"
```

Since the permission issue that #6620 originally was fixing is most likely not gonna happen on windows, this seems like a safe enough compromise. Ideally a check for PHP<5.5 should be introduced, but I would like to be sure it's been fixed and is not just a lucky coincidence (/cc @PierreJoye)

I would recommend merging fast and maybe adding the version check later, since it breaks composer create-project on windows. The workaround being: cd in the dir and run `composer install` again to finalize the project setup.

---------------------------------------------------------------------------

by vicb at 2013-02-17T20:12:17Z

Thanks @Seldaek !

Would you mind creating an issue for the version check so that it doesn't get lost ?

---------------------------------------------------------------------------

by Seldaek at 2013-02-18T10:43:56Z

@vicb done.

---------------------------------------------------------------------------

by vicb at 2013-02-18T11:45:16Z

thanks !
[DomCrawler] lowered parsed protocol string (fixes #6986)
merged branch cloppy/Dom-Crawler-fix (PR #7127)

This PR was merged into the 2.0 branch.

Commits
-------

cb03074 [DomCrawler] lowered parsed protocol string (fixes #6986)

Discussion
----------

[DomCrawler] lowered parsed protocol string (fixes #6986)

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #6986
| License       | MIT
| Doc PR        | symfony/symfony-docs#123

Are UTs needed for this?