Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HttpClient] Declare `$active` first to prevent weird issue #32989

Merged
merged 1 commit into from Aug 7, 2019

Conversation

@Kocal
Copy link
Contributor

commented Aug 6, 2019

Q A
Branch? 4.3
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? -
Fixed tickets #32833
License MIT
Doc PR no

In some undefined cases we can have the following error while using the WebTestCase/CurlHttpClient in PHPUnit:
Sélection_999(101)

This is really weird because $active is a reference and so it does not need to be declared before, but doing that fixes the issue. (#32833 (comment))

I can't add tests because we were not able to reproduce the issue...

@Kocal Kocal changed the title fix(http-client:curl): declare `$active` first to prevent weird issue [HttpClient] Declare `$active` first to prevent weird issue Aug 6, 2019

@nicolas-grekas nicolas-grekas added this to the 4.3 milestone Aug 6, 2019

@Kocal

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

Hum, it seems that this PR actually brokes tests...

Sélection_999(107)

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

Would using 0 break in the same way?

@Kocal

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

I don't have the time for now, I will check it out this evening.

@Kocal

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

Huh, so $active = null, $active = 0 and $active make tests for HttpClient timeout...

I don't understand why. It's a variable declared in a reference so it should be ok... Maybe because of a weird PHP configuration, it fails?
We really need to find a way to easily reproduce the issue and find a better fix than this... :/

@nicolas-grekas nicolas-grekas force-pushed the Kocal:fix/curl-httpclient-destruct branch from 71d9edc to 29758ab Aug 7, 2019

@nicolas-grekas
Copy link
Member

left a comment

(issue in tests was unrelated to this PR and has been fixed in #33005)

@nicolas-grekas nicolas-grekas force-pushed the Kocal:fix/curl-httpclient-destruct branch from 29758ab to ba030f0 Aug 7, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

Thank you @Kocal.

@nicolas-grekas nicolas-grekas merged commit ba030f0 into symfony:4.3 Aug 7, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Aug 7, 2019

bug #32989 [HttpClient] Declare `$active` first to prevent weird issu…
…e (Kocal)

This PR was squashed before being merged into the 4.3 branch (closes #32989).

Discussion
----------

[HttpClient] Declare `$active` first to prevent weird issue

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | -    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #32833    <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | no

In some undefined cases we can have the following error while using the WebTestCase/CurlHttpClient in PHPUnit:
![Sélection_999(101)](https://user-images.githubusercontent.com/2103975/62543336-0ad9e700-b85e-11e9-8b7f-d5b49e1d2d0d.png)

This is really weird because `$active` is a reference and so it does not need to be declared before, but doing that fixes the issue. (#32833 (comment))

I can't add tests because we were not able to reproduce the issue...

Commits
-------

ba030f0 [HttpClient] Declare `$active` first to prevent weird issue

@Kocal Kocal deleted the Kocal:fix/curl-httpclient-destruct branch Aug 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.