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

Fix response headers set in templates not be sent. #11087

Merged
merged 3 commits into from Aug 24, 2017

Conversation

Projects
None yet
6 participants
@markstory
Member

markstory commented Aug 24, 2017

When response properties are set in the view templates, they should show up in the controller's response as well.

Fixes #11084

Fix response headers set in templates not be sent.
When response properties are set in the view templates, they should show
up in the controller's response as well.

Fixes #11084
@lorenzo

This comment has been minimized.

Show comment
Hide comment
@lorenzo

lorenzo Aug 24, 2017

Member

Seems like this breaks a bunch of tests. The changes look fine to me though

Member

lorenzo commented Aug 24, 2017

Seems like this breaks a bunch of tests. The changes look fine to me though

markstory and others added some commits Aug 24, 2017

Handle immutable responses better.
Wait until the afterDispatch event to grab the controller response
property. Event listeners in the shutdown or afterDispatch events may
have replaced the controller's response.
@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Aug 24, 2017

Codecov Report

Merging #11087 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #11087      +/-   ##
============================================
+ Coverage     94.87%   94.87%   +<.01%     
  Complexity    12838    12838              
============================================
  Files           437      437              
  Lines         32731    32732       +1     
============================================
+ Hits          31052    31053       +1     
  Misses         1679     1679
Impacted Files Coverage Δ Complexity Δ
src/Http/ActionDispatcher.php 100% <100%> (ø) 19 <0> (ø) ⬇️
src/Controller/Controller.php 99.45% <100%> (ø) 77 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91274dd...3587231. Read the comment docs.

codecov-io commented Aug 24, 2017

Codecov Report

Merging #11087 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #11087      +/-   ##
============================================
+ Coverage     94.87%   94.87%   +<.01%     
  Complexity    12838    12838              
============================================
  Files           437      437              
  Lines         32731    32732       +1     
============================================
+ Hits          31052    31053       +1     
  Misses         1679     1679
Impacted Files Coverage Δ Complexity Δ
src/Http/ActionDispatcher.php 100% <100%> (ø) 19 <0> (ø) ⬇️
src/Controller/Controller.php 99.45% <100%> (ø) 77 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91274dd...3587231. Read the comment docs.

@ADmad ADmad added this to the 3.5.1 milestone Aug 24, 2017

@markstory markstory merged commit e64e0a9 into master Aug 24, 2017

6 checks passed

codecov/patch 100% of diff hit (target 94.87%)
Details
codecov/project 94.87% (+<.01%) compared to 91274dd
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
stickler-ci No lint errors found.

@markstory markstory deleted the issue-11084 branch Aug 24, 2017

@vonboth

This comment has been minimized.

Show comment
Hide comment
@vonboth

vonboth Aug 25, 2017

Also affects 3.4.X ...

vonboth commented Aug 25, 2017

Also affects 3.4.X ...

@markstory

This comment has been minimized.

Show comment
Hide comment
@markstory

markstory Aug 25, 2017

Member

@vonboth 3.4 is only getting security patches going foward.

Member

markstory commented Aug 25, 2017

@vonboth 3.4 is only getting security patches going foward.

@nrother

This comment has been minimized.

Show comment
Hide comment
@nrother

nrother Sep 20, 2017

Contributor

Am I right that this disabled the possibility to set the response body in the view, instead of returning it? $response->withStringBody() looks like it would always overwrite the body, so if View::render()return nullthe body is discarded.

This seems to affect the possibility to use a CallbackStream as a response for streaming responses. Or is there another possibility for that?

Documentations for the return value of View::render says "Rendered content or null if content already rendered and returned earlier." btw.

Contributor

nrother commented Sep 20, 2017

Am I right that this disabled the possibility to set the response body in the view, instead of returning it? $response->withStringBody() looks like it would always overwrite the body, so if View::render()return nullthe body is discarded.

This seems to affect the possibility to use a CallbackStream as a response for streaming responses. Or is there another possibility for that?

Documentations for the return value of View::render says "Rendered content or null if content already rendered and returned earlier." btw.

@markstory

This comment has been minimized.

Show comment
Hide comment
@markstory

markstory Sep 20, 2017

Member

This seems to affect the possibility to use a CallbackStream as a response for streaming responses. Or is there another possibility for that?

This should still be possible if you don't render a view as well.

Member

markstory commented Sep 20, 2017

This seems to affect the possibility to use a CallbackStream as a response for streaming responses. Or is there another possibility for that?

This should still be possible if you don't render a view as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment