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

Fix for #5440 #5480

Merged
merged 2 commits into from
Mar 17, 2012
Merged

Fix for #5440 #5480

merged 2 commits into from
Mar 17, 2012

Conversation

drogus
Copy link
Member

@drogus drogus commented Mar 16, 2012

This fixes situation where rendering template to string sets rendered_format to the format rendered there. This is ok to have consistent formats rendered in partials, but it breaks on next renders if format is explicitly set or on last render where default format does not necessarily need to be the format of first rendered template.

cc @josevalim

@@ -18,6 +18,14 @@ def render(*args) #:nodoc:
response_body
end

def render_to_body(options)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move this to AC::Rendering or AbsC::Rendering? I would say AbsC::Rendering.

@drogus
Copy link
Member Author

drogus commented Mar 17, 2012

@josevalim ok, so only this line need to be left when I rebased to newest 3-2-stable (I thought I did it, but apparently not ;) ): https://github.com/rails/rails/pull/5480/files#L0R103 Does it look good now?

This looks a bit weird and frankly speaking I have a gut feeling that the entire thing could be refactored to be more nice in general (starting with the usage in renderers), but probably this is not safe just before rc.

Also, do we need to merge it also with master?

@@ -100,6 +100,7 @@ def render_to_string(*args, &block)
# :api: plugin
def render_to_body(options = {})
_process_options(options)
lookup_context.rendered_format = nil if options[:formats]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for nitpicking, but let's move this to _render_template? This logic only makes sense when rendering a template. :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you're totally right, I was not sure if render_to_body handles also some other cases that need to also have rendered_format cleared

@josevalim
Copy link
Contributor

Yes, let's just move the logic to _render_template and it looks perfect now. 👍 for merging it on 3-2-stable and master.

Overall, I actually don't think this looks weird. Rails 3-2-stable added the possibility of passing :formats along with render where previously you had to pass render :template => "foo.html". .html was handled inside the resolver which didn't allow the format to leak but could cause weird results (like the formats being :xml and you rendering a :html file). I think current behavior and logic is better. :)

This fixes situation where rendering template to string
sets `rendered_format` to the format rendered there.
This is ok to have consistent formats rendered in partials,
but it breaks on next renders if format is explicitly set
or on last render where default format does not necessarily
need to be the format of first rendered template.
@drogus
Copy link
Member Author

drogus commented Mar 17, 2012

Overall, I actually don't think this looks weird.

Ok, I will need to look at the entire code again later. Should I push it also to master?

@spastorino
Copy link
Contributor

@drogus yeah, we should apply this on master

josevalim added a commit that referenced this pull request Mar 17, 2012
@josevalim josevalim merged commit e5b46cf into rails:3-2-stable Mar 17, 2012
josevalim added a commit that referenced this pull request Mar 17, 2012
@spastorino
Copy link
Contributor

I've ported to master

@drogus
Copy link
Member Author

drogus commented Mar 17, 2012

@spastorino thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants