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

Use `#performed?` to terminate controller callbacks #25079

Merged
merged 1 commit into from Jun 7, 2016

Conversation

Projects
None yet
7 participants
@jeffkreeftmeijer
Contributor

jeffkreeftmeijer commented May 19, 2016

As (very) briefly discussed with @jeremy: https://twitter.com/bitsweat/status/732983029677719552.

Since 69009f4, ActionController::Metal::DataStreaming#send_file doesn't
set @_response_body anymore.

AbstractController::Callbacks used @_response_body in its callback
terminator, so it failed to halt the callback cycle when using #send_file
from a before_action.

Instead, it now uses #performed? on AbstractController::Base and
ActionController::Metal, which checks response.committed?, besides
checking if @_response_body is set, if possible.

Example application: https://gist.github.com/jeffkreeftmeijer/78ae4572f36b198e729724b0cf79ef8e

@rails-bot

This comment has been minimized.

rails-bot commented May 19, 2016

r? @eileencodes

(@rails-bot has picked a reviewer for you, use r? to override)

@jeffkreeftmeijer jeffkreeftmeijer force-pushed the jeffkreeftmeijer:send_file-halts-request-cycle branch May 23, 2016

@eileencodes

This comment has been minimized.

Member

eileencodes commented Jun 3, 2016

r? @jeremy

@rails-bot rails-bot assigned jeremy and unassigned eileencodes Jun 3, 2016

Use `#performed?` to terminate controller callbacks
Since 69009f, `ActionController::Metal::DataStreaming#send_file` doesn't
set `@_response_body` anymore.

`AbstractController::Callbacks` used `@_response_body` in its callback
terminator, so it failed to halt the callback cycle when using `#send_file`
from a `before_action`.

Instead, it now uses `#performed?` on `AbstractController::Base` and
`ActionController::Metal`, which checks `response.committed?`, besides
 checking if `@_response_body` is set, if possible.

Example application: https://gist.github.com/jeffkreeftmeijer/78ae4572f36b198e729724b0cf79ef8e

@jeffkreeftmeijer jeffkreeftmeijer force-pushed the jeffkreeftmeijer:send_file-halts-request-cycle branch to f650e03 Jun 3, 2016

@@ -150,6 +150,13 @@ def available_action?(action_name)
_find_action_name(action_name)
end
# Tests if a response body is set. Used to determine if the

This comment has been minimized.

@rafaelfranca

rafaelfranca Jun 3, 2016

Member

This should document the intention of the method. like just Tests if a response body is set. It doesn't matter how it was implemented or where it is used internally.

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Jun 3, 2016

Action view tests are broken.

@rafaelfranca rafaelfranca added this to the 5.0.0 milestone Jun 3, 2016

@sgrif

This comment has been minimized.

Member

sgrif commented Jun 7, 2016

Deleting the test. It was added by #17978 which adequately tests the necessary behavior elsewhere. It's failing here because calling get followed by process won't change the response object. The only reason it's calling get in the first place is because the controller in that file isn't checking whether request is nil in a before filter.

@sgrif sgrif merged commit f650e03 into rails:master Jun 7, 2016

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

sgrif added a commit that referenced this pull request Jun 7, 2016

Merge pull request #25079 from jeffkreeftmeijer/send_file-halts-reque…
…st-cycle

Use `#performed?` to terminate controller callbacks

sgrif added a commit that referenced this pull request Jun 7, 2016

Merge pull request #25079 from jeffkreeftmeijer/send_file-halts-reque…
…st-cycle

Use `#performed?` to terminate controller callbacks

sgrif added a commit that referenced this pull request Jun 7, 2016

Merge pull request #25079 from jeffkreeftmeijer/send_file-halts-reque…
…st-cycle

Use `#performed?` to terminate controller callbacks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment