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.

Sign up for GitHub

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

Jump to bottom

[HttpKernel] Dont close the reponse stream in debug #19114

Merged
merged 1 commit into from
Jun 20, 2016
Merged

[HttpKernel] Dont close the reponse stream in debug #19114

merged 1 commit into from
Jun 20, 2016

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Jun 20, 2016 •
edited
Loading

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

Because it's terminate's job to clean the state, not the Response's,
and because the current behavior prevents getting any output on trailing errors on FPM especially.

@nicolas-grekas nicolas-grekas mentioned this pull request Jun 20, 2016
Closed
@xabbuh
Copy link
Member

xabbuh commented Jun 20, 2016

👍

1 similar comment
@fabpot
Copy link
Member

fabpot commented Jun 20, 2016

👍

@fabpot
Copy link
Member

fabpot commented Jun 20, 2016

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 2fbc200 into symfony:2.7 Jun 20, 2016
fabpot added a commit that referenced this pull request Jun 20, 2016
@fabpot
bug #19114 [HttpKernel] Dont close the reponse stream in debug (nicol…
a0cdcb0 
…as-grekas)

This PR was merged into the 2.7 branch.

Discussion
----------

[HttpKernel] Dont close the reponse stream in debug

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

Because it's `terminate`'s job to clean the state, not the `Response`'s,
and because the current behavior prevents getting any output on trailing errors on FPM especially.

Commits
-------

2fbc200 [HttpKernel] Dont close the output stream in debug
@nicolas-grekas nicolas-grekas deleted the no-flush-on-debug branch June 20, 2016 11:27
@pyrech
Copy link
Contributor

pyrech commented Jun 24, 2016

Isn't this a BC break for those using only the HTTP Foundation component?

@stof
Copy link
Member

stof commented Jun 24, 2016

it is indeed a BC break

@nicolas-grekas
Copy link
Member Author

@pyrech did you experience any issue or are you just wondering?

@pyrech
Copy link
Contributor

pyrech commented Jun 24, 2016

I have a small application that uses the Response::send() method and expects that fastcgi_finish_request() will be called. So I will need to update my code :) Really not a big deal for me but still a tiny BC break

@stof
Copy link
Member

stof commented Jun 24, 2016

IMO, we should revert this in 2.7. Tiny BC break are not acceptable in patch releases IMO (except when necessary to fix security issues according to our guidelines, but this is not a security issue)

@stof
Copy link
Member

stof commented Jun 24, 2016

btw, this change means that the behavior of your kernel.terminate event listener will be different in dev and prod, which might not be good. We should look into logging errors there, but still respecting the behavior of finishing the fastcgi request, to be closer to the prod

@nicolas-grekas
Copy link
Member Author

Debug mode is already significantly different from prod, and it is far more important to give feedback to the dev in case of any errors.

@Nicofuma Nicofuma mentioned this pull request Jun 26, 2016
Merged
4 tasks
This was referenced Jun 30, 2016
Merged
Merged
Merged
Merged
@jakzal
Copy link
Contributor

jakzal commented Jul 11, 2016

Looks like this actually broke something in PHPBB (see the referenced issue on their side).

@JEDIBC
Copy link

JEDIBC commented Jul 12, 2016

I agree with @stof . This behavior modification of kernel.terminate in dev is not a good thing. Furthermore the description in the changelog is absolutely not indicative of the repercussion on kernel.terminate.

@fabpot
Copy link
Member

fabpot commented Jul 13, 2016

reverted

fabpot added a commit that referenced this pull request Jul 13, 2016
@fabpot
Revert "bug #19114 [HttpKernel] Dont close the reponse stream in debu…
482e185 
…g (nicolas-grekas)"

This reverts commit a0cdcb0, reversing
changes made to 9c8a3e9.
nicolas-grekas added a commit that referenced this pull request Jul 17, 2016
@nicolas-grekas
Merge branch '2.7' into 2.8
7a57903 
* 2.7:
  [VarDumper] Fix dumping jsons casted as arrays
  PassConfig::getMergePass is not an array
  Revert "bug #19114 [HttpKernel] Dont close the reponse stream in debug (nicolas-grekas)"
  Fix the retrieval of the last username when using forwarding
  [Yaml] Fix PHPDoc of the Yaml class
  [HttpFoundation] Add OPTIONS and TRACE to the list of safe methods
  Update getAbsoluteUri() for query string uris
nicolas-grekas added a commit that referenced this pull request Jul 17, 2016
@nicolas-grekas
Merge branch '2.8' into 3.0
8a2d5cd 
* 2.8:
  [VarDumper] Fix dumping jsons casted as arrays
  PassConfig::getMergePass is not an array
  Revert "bug #19114 [HttpKernel] Dont close the reponse stream in debug (nicolas-grekas)"
  Fix the retrieval of the last username when using forwarding
  [Yaml] Fix PHPDoc of the Yaml class
  [HttpFoundation] Add OPTIONS and TRACE to the list of safe methods
  Update getAbsoluteUri() for query string uris
nicolas-grekas added a commit that referenced this pull request Jul 17, 2016
@nicolas-grekas
Merge branch '3.0' into 3.1
17de127 
* 3.0:
  [VarDumper] Fix dumping jsons casted as arrays
  PassConfig::getMergePass is not an array
  Revert "bug #19114 [HttpKernel] Dont close the reponse stream in debug (nicolas-grekas)"
  Fix the retrieval of the last username when using forwarding
  [Yaml] Fix PHPDoc of the Yaml class
  [HttpFoundation] Add OPTIONS and TRACE to the list of safe methods
  Update getAbsoluteUri() for query string uris

Conflicts:
	src/Symfony/Component/Yaml/Yaml.php
nicolas-grekas added a commit that referenced this pull request Jul 17, 2016
@nicolas-grekas
Merge branch '3.1'
cbd1915 
* 3.1:
  [VarDumper] Fix dumping jsons casted as arrays
  PassConfig::getMergePass is not an array
  Revert "bug #19114 [HttpKernel] Dont close the reponse stream in debug (nicolas-grekas)"
  [Serializer] Include the format in the cache key
  Fix the retrieval of the last username when using forwarding
  [Yaml] Fix PHPDoc of the Yaml class
  [HttpFoundation] Add OPTIONS and TRACE to the list of safe methods
  Update getAbsoluteUri() for query string uris

Conflicts:
	src/Symfony/Component/DependencyInjection/Compiler/PassConfig.php
	src/Symfony/Component/HttpFoundation/Tests/RequestTest.php
@lecajer
Copy link

lecajer commented Jul 19, 2016 •
edited
Loading

Thanks for reverting. In what versions of symfony/http-foundation and symfony/http-kernel is this fix available ?

@stof
Copy link
Member

stof commented Jul 19, 2016

@jeremlicious the revert is currently only available in dev versions. It will be available in the next patch releases (i.e. 3.1.3, 3.0.9, 2.8.9 and 2.7.18)

@ro0NL ro0NL mentioned this pull request May 20, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Bug HttpKernel
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@nicolas-grekas @xabbuh @fabpot @pyrech @stof @jakzal @JEDIBC @lecajer @javiereguiluz