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

Default rendering behavior if respond_to collector doesn't have a block. #22854

Merged
merged 1 commit into from Mar 11, 2016

Conversation

Projects
None yet
7 participants
@jcoyne
Contributor

jcoyne commented Dec 30, 2015

When a respond_to collector doesn't have a response, then a :no_content response should be rendered. This brings the default rendering behavior introduced by #19036 to controller methods
employing respond_to

@rails-bot

This comment has been minimized.

rails-bot commented Dec 30, 2015

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @pixeltrix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rafaelfranca rafaelfranca added this to the 5.0.0 milestone Dec 30, 2015

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Dec 30, 2015

I don't think this behavior is preferable. How is that different from:

def index
end

responding with no_content if the template is not present? That is exactly the expected behavior of #19036

@jcoyne

This comment has been minimized.

Contributor

jcoyne commented Dec 30, 2015

Don't you think that these two should behave the same way?

respond_to do |type|
    type.json
end
respond_to do |type|
   type.json { }
end
@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Dec 30, 2015

Yes, they should. And they should return :no_content if the template doesn't exist.

@jcoyne

This comment has been minimized.

Contributor

jcoyne commented Dec 30, 2015

@rafaelfranca prior to this PR the former example (no block) renders the default and raises ActionView::TemplateNotFound if the template doesn't exist. This PR just makes these two cases (no block, block that doesn't render) behave the same.

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Dec 30, 2015

So, yeah, we need to change the no block form to return :no_content.

@jcoyne jcoyne force-pushed the jcoyne:missing_template branch Dec 30, 2015

@jcoyne

This comment has been minimized.

Contributor

jcoyne commented Dec 30, 2015

@rafaelfranca I've reworked the PR.

@jcoyne jcoyne changed the title from Render default template if block doesn't render to Default rendering behavior if respond_to collector doesn't have a block. Dec 30, 2015

@dhh dhh assigned rafaelfranca and unassigned pixeltrix Feb 10, 2016

chancancode added a commit that referenced this pull request Feb 23, 2016

Lock down new `ImplicitRender` behavior for 5.0 RC
1. Conceptually revert #20276

   The feature was implemented for the `responders` gem. In the end,
   they did not need that feature, and have found a better fix (see
   plataformatec/responders#131).

   `ImplicitRender` is the place where Rails specifies our default
   policies for the case where the user did not explicitly tell us
   what to render, essentially describing a set of heuristics. If
   the gem (or the user) knows exactly what they want, they could
   just perform the correct `render` to avoid falling through to
   here, as `responders` did (the user called `respond_with`).

   Reverting the patch allows us to avoid exploding the complexity
   and defining “the fallback for a fallback” policies.

2. `respond_to` and templates are considered exhaustive enumerations

   If the user specified a list of formats/variants in a `respond_to`
   block, anything that is not explicitly included should result
   in an `UnknownFormat` error (which is then caught upstream to
   mean “406 Not Acceptable” by default). This is already how it
   works before this commit.

   Same goes for templates – if the user defined a set of templates
   (usually in the file system), that set is now considered exhaustive,
   which means that “missing” templates are considered `UnknownFormat`
   errors (406).

3. To keep API endpoints simple, the implicit render behavior for
   actions with no templates defined at all (regardless of formats,
   locales, variants, etc) are defaulted to “204 No Content”. This
   is a strictly narrower version of the feature landed in #19036 and
   #19377.

4. To avoid confusion when interacting in the browser, these actions
   will raise a `TemplateMissing` error for “interactive” requests
   instead. (The precise definition of “interactive” requests might
   change – the spirit here is to give helpful messages and avoid
   confusions.)

Closes #20666, #22854, #23062, #23077, #23564

[Godfrey Chan, Jon Moss, Mike Clark, Matthew Draper]

@chancancode chancancode referenced this pull request Feb 23, 2016

Merged

Lock down new `ImplicitRender` behavior for 5.0 RC #23827

2 of 2 tasks complete

kaspth added a commit to kaspth/rails that referenced this pull request Feb 23, 2016

Lock down new `ImplicitRender` behavior for 5.0 RC
1. Conceptually revert rails#20276

   The feature was implemented for the `responders` gem. In the end,
   they did not need that feature, and have found a better fix (see
   plataformatec/responders#131).

   `ImplicitRender` is the place where Rails specifies our default
   policies for the case where the user did not explicitly tell us
   what to render, essentially describing a set of heuristics. If
   the gem (or the user) knows exactly what they want, they could
   just perform the correct `render` to avoid falling through to
   here, as `responders` did (the user called `respond_with`).

   Reverting the patch allows us to avoid exploding the complexity
   and defining “the fallback for a fallback” policies.

2. `respond_to` and templates are considered exhaustive enumerations

   If the user specified a list of formats/variants in a `respond_to`
   block, anything that is not explicitly included should result
   in an `UnknownFormat` error (which is then caught upstream to
   mean “406 Not Acceptable” by default). This is already how it
   works before this commit.

   Same goes for templates – if the user defined a set of templates
   (usually in the file system), that set is now considered exhaustive,
   which means that “missing” templates are considered `UnknownFormat`
   errors (406).

3. To keep API endpoints simple, the implicit render behavior for
   actions with no templates defined at all (regardless of formats,
   locales, variants, etc) are defaulted to “204 No Content”. This
   is a strictly narrower version of the feature landed in rails#19036 and
   rails#19377.

4. To avoid confusion when interacting in the browser, these actions
   will raise a `TemplateMissing` error for “interactive” requests
   instead. (The precise definition of “interactive” requests might
   change – the spirit here is to give helpful messages and avoid
   confusions.)

Closes rails#20666, rails#22854, rails#23062, rails#23077, rails#23564

[Godfrey Chan, Jon Moss, Kasper Timm Hansen, Mike Clark, Matthew Draper]
@dhh

This comment has been minimized.

Member

dhh commented Feb 24, 2016

Superseded by #23827.

@dhh dhh closed this Feb 24, 2016

chancancode added a commit that referenced this pull request Feb 24, 2016

Lock down new `ImplicitRender` behavior for 5.0 RC
1. Conceptually revert #20276

   The feature was implemented for the `responders` gem. In the end,
   they did not need that feature, and have found a better fix (see
   plataformatec/responders#131).

   `ImplicitRender` is the place where Rails specifies our default
   policies for the case where the user did not explicitly tell us
   what to render, essentially describing a set of heuristics. If
   the gem (or the user) knows exactly what they want, they could
   just perform the correct `render` to avoid falling through to
   here, as `responders` did (the user called `respond_with`).

   Reverting the patch allows us to avoid exploding the complexity
   and defining “the fallback for a fallback” policies.

2. `respond_to` and templates are considered exhaustive enumerations

   If the user specified a list of formats/variants in a `respond_to`
   block, anything that is not explicitly included should result
   in an `UnknownFormat` error (which is then caught upstream to
   mean “406 Not Acceptable” by default). This is already how it
   works before this commit.

   Same goes for templates – if the user defined a set of templates
   (usually in the file system), that set is now considered exhaustive,
   which means that “missing” templates are considered `UnknownFormat`
   errors (406).

3. To keep API endpoints simple, the implicit render behavior for
   actions with no templates defined at all (regardless of formats,
   locales, variants, etc) are defaulted to “204 No Content”. This
   is a strictly narrower version of the feature landed in #19036 and
   #19377.

4. To avoid confusion when interacting in the browser, these actions
   will raise an `UnknownFormat` error for “interactive” requests
   instead. (The precise definition of “interactive” requests might
   change – the spirit here is to give helpful messages and avoid
   confusions.)

Closes #20666, #22854, #23062, #23077, #23564

[Godfrey Chan, Jon Moss, Kasper Timm Hansen, Mike Clark, Matthew Draper]

@chancancode chancancode reopened this Feb 25, 2016

@chancancode

This comment has been minimized.

Member

chancancode commented Feb 25, 2016

@jcoyne we didn't end up combining this into #23827, can you update this PR?

@jcoyne

This comment has been minimized.

Contributor

jcoyne commented Feb 25, 2016

@chancancode I'm taking a look.

@jcoyne jcoyne force-pushed the jcoyne:missing_template branch Feb 25, 2016

Render default template if block doesn't render
When a `respond_to` collector doesn't have a response, then a
`:no_content` response should be rendered. This brings the default
rendering behavior introduced by
#19036 to controller methods
employing `respond_to`

@jcoyne jcoyne force-pushed the jcoyne:missing_template branch to 48f140c Feb 25, 2016

@sgrif sgrif merged commit 48f140c into rails:master Mar 11, 2016

1 check passed

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

sgrif added a commit that referenced this pull request Mar 11, 2016

Merge pull request #22854 from jcoyne/missing_template
Default rendering behavior if respond_to collector doesn't have a block.

@jcoyne jcoyne deleted the jcoyne:missing_template branch Mar 11, 2016

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