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

Action Pack Variants #12977

Merged
merged 2 commits into from Dec 3, 2013

Conversation

Projects
None yet
9 participants
@lukaszx0
Member

lukaszx0 commented Nov 21, 2013

By default, variants in the templates will be picked up if a variant is set and
there's a match. The format will be:

  app/views/projects/show.html.erb
  app/views/projects/show.html+tablet.erb
  app/views/projects/show.html+phone.erb

If request.variant = :tablet is set, we'll automatically be rendering the html+tablet template.

In the controller, we can also tailer to the variants with this syntax:

  class ProjectsController < ActionController::Base
    def show
      respond_to do |format|
        format.html do |html|
          @stars = @project.stars

          html.tablet { @notifications = @project.notifications }
          html.phone  { @chat_heads    = @project.chat_heads }
        end

        format.js
        format.atom
      end
    end
  end

The variant itself is nil by default, but can be set in before filters, like so:

  class ApplicationController < ActionController::Base
    before_action do
      if request.user_agent =~ /iPad/
        request.variant = :tablet
      end
    end
  end

This is modeled loosely on custom mime types, but it's specifically not intended
to be used together. If you're going to make a custom mime type, you don't need
a variant. Variants are for variations on a single mime types.

/cc @dhh @josevalim

@dhh

View changes

Show outdated Hide outdated actionpack/lib/action_controller/metal/rendering.rb
@dhh

View changes

Show outdated Hide outdated actionpack/lib/action_dispatch/http/request.rb
@jeremy

View changes

Show outdated Hide outdated actionpack/lib/action_dispatch/http/request.rb
@rafaelfranca

View changes

Show outdated Hide outdated actionpack/lib/action_controller/metal/mime_responds.rb
@rafaelfranca

View changes

Show outdated Hide outdated actionpack/lib/action_controller/metal/mime_responds.rb
@@ -187,7 +187,7 @@ def respond_to(*mimes, &block)
raise ArgumentError, "respond_to takes either types or a block, never both" if mimes.any? && block_given?

This comment has been minimized.

@rafaelfranca

rafaelfranca Nov 21, 2013

Member

Documentation of this module need to be updated.

@rafaelfranca

rafaelfranca Nov 21, 2013

Member

Documentation of this module need to be updated.

This comment has been minimized.

@lukaszx0

lukaszx0 Nov 21, 2013

Member

Well that behavior actually didn't change. This message is still valid as you still need to pass either types or block however it is extended and inside "types block" you can pass variant now, but it's optional. I think changing that message to mention variant will only introduce confusion. What do you think? How do you suggest to change it?

@lukaszx0

lukaszx0 Nov 21, 2013

Member

Well that behavior actually didn't change. This message is still valid as you still need to pass either types or block however it is extended and inside "types block" you can pass variant now, but it's optional. I think changing that message to mention variant will only introduce confusion. What do you think? How do you suggest to change it?

This comment has been minimized.

@rafaelfranca

rafaelfranca Nov 21, 2013

Member

I'm pointing the wrong line. I was talking about the respond_to Rdoc that needs to mention the variants now.

@rafaelfranca

rafaelfranca Nov 21, 2013

Member

I'm pointing the wrong line. I was talking about the respond_to Rdoc that needs to mention the variants now.

This comment has been minimized.

@lukaszx0

lukaszx0 Nov 21, 2013

Member

Ah, right. Yeah, I'm aware of that, will prepare something quickly for now
and we'll edit it afterwords if needed.

On Thu, Nov 21, 2013 at 3:21 PM, Rafael Mendonça França <
notifications@github.com> wrote:

In actionpack/lib/action_controller/metal/mime_responds.rb:

@@ -187,7 +187,7 @@ def respond_to(*mimes, &block)
raise ArgumentError, "respond_to takes either types or a block, never both" if mimes.any? && block_given?

I'm pointing the wrong line. I was talking about the respond_to Rdoc that
needs to mention the variants now.


Reply to this email directly or view it on GitHubhttps://github.com/rails/rails/pull/12977/files#r7824578
.

@lukaszx0

lukaszx0 Nov 21, 2013

Member

Ah, right. Yeah, I'm aware of that, will prepare something quickly for now
and we'll edit it afterwords if needed.

On Thu, Nov 21, 2013 at 3:21 PM, Rafael Mendonça França <
notifications@github.com> wrote:

In actionpack/lib/action_controller/metal/mime_responds.rb:

@@ -187,7 +187,7 @@ def respond_to(*mimes, &block)
raise ArgumentError, "respond_to takes either types or a block, never both" if mimes.any? && block_given?

I'm pointing the wrong line. I was talking about the respond_to Rdoc that
needs to mention the variants now.


Reply to this email directly or view it on GitHubhttps://github.com/rails/rails/pull/12977/files#r7824578
.

@jeremy

View changes

Show outdated Hide outdated actionpack/lib/action_dispatch/http/request.rb
@rafaelfranca

View changes

Show outdated Hide outdated actionpack/lib/action_controller/metal/rendering.rb
@rafaelfranca

View changes

Show outdated Hide outdated actionpack/lib/action_dispatch/http/request.rb
@rafaelfranca

View changes

Show outdated Hide outdated actionview/lib/action_view/rendering.rb
@jeremy

View changes

Show outdated Hide outdated actionpack/test/controller/mime/respond_to_test.rb
@jeremy

View changes

Show outdated Hide outdated actionpack/test/dispatch/request_test.rb
@jeremy

View changes

Show outdated Hide outdated actionpack/test/dispatch/request_test.rb
@jeremy

View changes

Show outdated Hide outdated actionview/lib/action_view/rendering.rb
@rafaelfranca

View changes

Show outdated Hide outdated actionview/lib/action_view/rendering.rb
@@ -240,7 +240,9 @@ def extract_handler_and_format(path, default_formats)
end
handler = Template.handler_for_extension(extension)
format = pieces.last && Template::Types[pieces.last]
format = pieces.last && pieces.last.split(EXTENSIONS[:variants], 2).first # remove variant from format

This comment has been minimized.

@rafaelfranca

rafaelfranca Nov 21, 2013

Member

maybe is good the extract a method to this

@rafaelfranca

rafaelfranca Nov 21, 2013

Member

maybe is good the extract a method to this

This comment has been minimized.

@lukaszx0

lukaszx0 Nov 21, 2013

Member

Is it? Well that method is is already an extraction - https://github.com/strzalek/rails/blob/a5c609f1290f2ca071f8980eb08f3529513bbdee/actionview/lib/action_view/template/resolver.rb#L231

Extracting this small pice of code won't be beneficial in any way in my opinion. What do you think?

@lukaszx0

lukaszx0 Nov 21, 2013

Member

Is it? Well that method is is already an extraction - https://github.com/strzalek/rails/blob/a5c609f1290f2ca071f8980eb08f3529513bbdee/actionview/lib/action_view/template/resolver.rb#L231

Extracting this small pice of code won't be beneficial in any way in my opinion. What do you think?

This comment has been minimized.

@rafaelfranca
@rafaelfranca
@rafaelfranca

View changes

Show outdated Hide outdated actionview/lib/action_view/rendering.rb
@jeremy

View changes

Show outdated Hide outdated actionpack/lib/action_controller/metal/rendering.rb
@carlosantoniodasilva

View changes

Show outdated Hide outdated actionpack/CHANGELOG.md
@lukaszx0

This comment has been minimized.

Show comment
Hide comment
@lukaszx0

lukaszx0 Nov 21, 2013

Member

Thanks for the review, it's very valuable for me! <3

I've just pushed commit with fixes addressing your comments (separate commit to have nice diff, i'll squash it before merging). I've left some open questions questions and didn't moved setting variant from _normalize_options as I don't know yet how to do it nicely and don't have time right now to continue working on this. Will be back working on this later today!

Member

lukaszx0 commented Nov 21, 2013

Thanks for the review, it's very valuable for me! <3

I've just pushed commit with fixes addressing your comments (separate commit to have nice diff, i'll squash it before merging). I've left some open questions questions and didn't moved setting variant from _normalize_options as I don't know yet how to do it nicely and don't have time right now to continue working on this. Will be back working on this later today!

@ghost ghost assigned lukaszx0 Nov 21, 2013

@lukaszx0

This comment has been minimized.

Show comment
Hide comment
@lukaszx0

lukaszx0 Nov 25, 2013

Member

While polishing this PR I've stuck with the very last thing - nice a logical place to set variant in lookup context (#12977 (comment)). Suprisingly for various reasons it's not that straight forward as one would expect. I got suggestion by @josevalim to change implementation a bit and instead of setting it in request object (request.variant) move this to controller itself and simply delegate this to lookup context (in action: self.variant). The only drawback here is that we won't be able to set variant in middleware easily, however we can overcome this issues coping request.variant value to self.variant at the very beginning.

@dhh @jeremy What do you think?

Member

lukaszx0 commented Nov 25, 2013

While polishing this PR I've stuck with the very last thing - nice a logical place to set variant in lookup context (#12977 (comment)). Suprisingly for various reasons it's not that straight forward as one would expect. I got suggestion by @josevalim to change implementation a bit and instead of setting it in request object (request.variant) move this to controller itself and simply delegate this to lookup context (in action: self.variant). The only drawback here is that we won't be able to set variant in middleware easily, however we can overcome this issues coping request.variant value to self.variant at the very beginning.

@dhh @jeremy What do you think?

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Nov 25, 2013

Member

I'd prefer to have it set the same way that format is set. It's an extension to the format, and the format is set on the request. I don't think we need to change this. If anything, self.variant is going to be less clear IMO. Request#variant implies that this is a per request setting as well.

On Nov 25, 2013, at 2:21, Łukasz Strzałkowski notifications@github.com wrote:

While polishing this PR I've stuck with the very last thing - nice a logical place to set variant in lookup context (#12977 (comment)). Suprisingly for various reasons it's not that straight forward as one would expect. I got suggestion by @josevalim to change implementation a bit and instead of setting it in request object (request.variant) move this to controller itself and simply delegate this to lookup context (in action: self.variant). The only drawback here is that we won't be able to set variant in middleware easily, however we can overcome this issues coping request.variant value to self.variant at the very beginning.

@dhh @jeremy What do you think?


Reply to this email directly or view it on GitHub.

Member

dhh commented Nov 25, 2013

I'd prefer to have it set the same way that format is set. It's an extension to the format, and the format is set on the request. I don't think we need to change this. If anything, self.variant is going to be less clear IMO. Request#variant implies that this is a per request setting as well.

On Nov 25, 2013, at 2:21, Łukasz Strzałkowski notifications@github.com wrote:

While polishing this PR I've stuck with the very last thing - nice a logical place to set variant in lookup context (#12977 (comment)). Suprisingly for various reasons it's not that straight forward as one would expect. I got suggestion by @josevalim to change implementation a bit and instead of setting it in request object (request.variant) move this to controller itself and simply delegate this to lookup context (in action: self.variant). The only drawback here is that we won't be able to set variant in middleware easily, however we can overcome this issues coping request.variant value to self.variant at the very beginning.

@dhh @jeremy What do you think?


Reply to this email directly or view it on GitHub.

@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Nov 25, 2013

Member

Ditto. The request format has a well traveled path from Rack env to middleware to controller action to view resolver. The request variant walks the same path. It the variant diverges and needs some special handling, that's a strong sign that we should double-check why and adjust course so format & variant continue to behave alike.

From the implementation so far, it looks like your sticking point may be due to seeing that request formats are set on the lookup context before processing an action: https://github.com/rails/rails/blob/master/actionpack/lib/action_controller/metal/rendering.rb#L5-L9 If the variant is set before processing, then you can't set the request.variant within an action. No good.

Trouble is, the way we manage the format today is pretty awkward. We set it on the lookup context (aka, the controller) because we don't want the view to "know about" requests. Fine, ok. To fix, set the format and variant on the lookup context just before rendering. The likely candidate for that is _normalize_render.

Member

jeremy commented Nov 25, 2013

Ditto. The request format has a well traveled path from Rack env to middleware to controller action to view resolver. The request variant walks the same path. It the variant diverges and needs some special handling, that's a strong sign that we should double-check why and adjust course so format & variant continue to behave alike.

From the implementation so far, it looks like your sticking point may be due to seeing that request formats are set on the lookup context before processing an action: https://github.com/rails/rails/blob/master/actionpack/lib/action_controller/metal/rendering.rb#L5-L9 If the variant is set before processing, then you can't set the request.variant within an action. No good.

Trouble is, the way we manage the format today is pretty awkward. We set it on the lookup context (aka, the controller) because we don't want the view to "know about" requests. Fine, ok. To fix, set the format and variant on the lookup context just before rendering. The likely candidate for that is _normalize_render.

@lukaszx0

This comment has been minimized.

Show comment
Hide comment
@lukaszx0

lukaszx0 Nov 25, 2013

Member

Thanks guys. I agree with you, it was Jose's suggestion to overcome problem with finding nice place to inject this functionality.

@jeremy nicely described what's going on and suggested _normalize_render. So if the _normalize_render is a blessed place, it means that we've made a circle and end up where we begun - the initial commit, where variant was set in _normalize_options (which is called by _normalize_render: https://github.com/rails/rails/blob/master/actionpack/lib/abstract_controller/rendering.rb#L103-L107).

The best place for this to happen, in my opinion would be _process_format, done in here: lukaszx0@630e853#diff-7f9aef6572a262683ec9c1fda47118ebL37. However with this change, implicit rendering with variant set (possibly the most common use case) won't work, this needs to be set before this line: https://github.com/rails/rails/blob/master/actionpack/lib/abstract_controller/rendering.rb#L22 (like _normalize_render), not after.

Can it stay the way it was initially proposed then 😉 ?

Member

lukaszx0 commented Nov 25, 2013

Thanks guys. I agree with you, it was Jose's suggestion to overcome problem with finding nice place to inject this functionality.

@jeremy nicely described what's going on and suggested _normalize_render. So if the _normalize_render is a blessed place, it means that we've made a circle and end up where we begun - the initial commit, where variant was set in _normalize_options (which is called by _normalize_render: https://github.com/rails/rails/blob/master/actionpack/lib/abstract_controller/rendering.rb#L103-L107).

The best place for this to happen, in my opinion would be _process_format, done in here: lukaszx0@630e853#diff-7f9aef6572a262683ec9c1fda47118ebL37. However with this change, implicit rendering with variant set (possibly the most common use case) won't work, this needs to be set before this line: https://github.com/rails/rails/blob/master/actionpack/lib/abstract_controller/rendering.rb#L22 (like _normalize_render), not after.

Can it stay the way it was initially proposed then 😉 ?

@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Nov 25, 2013

Member

_process_format is called after rendering, no?

_normalize_options is specifically purposed to taking an incoming options has and normalize it into an outgoing one. Introducing new options into the normalized hash is out of scope. It is in scope for _normalize_render though.

Member

jeremy commented Nov 25, 2013

_process_format is called after rendering, no?

_normalize_options is specifically purposed to taking an incoming options has and normalize it into an outgoing one. Introducing new options into the normalized hash is out of scope. It is in scope for _normalize_render though.

@lukaszx0

This comment has been minimized.

Show comment
Hide comment
@lukaszx0

lukaszx0 Nov 26, 2013

Member

I've rebased and force pushed commit with all comments addressed.

If there's no other issues, I'll happily merge this :shipit:

Member

lukaszx0 commented Nov 26, 2013

I've rebased and force pushed commit with all comments addressed.

If there's no other issues, I'll happily merge this :shipit:

@lukaszx0

This comment has been minimized.

Show comment
Hide comment
@lukaszx0

lukaszx0 Nov 26, 2013

Member

@jeremy regarding defined?(request). I'm afraid we need to keep it. We're using request in AbstractController which, as name suggest, is abstract thus in rails world it's either base of ActionController which provides request or ActionMailer which does not. I agree it doesn't look very nice, but I think it needs to stay.
Here's diff with attempt to remove this check which made me realize that it should stay - https://gist.github.com/strzalek/68b407edc52b608fb797 What do you think?

@rafaelfranca removed additional hash lookup and added docs 🍻

Member

lukaszx0 commented Nov 26, 2013

@jeremy regarding defined?(request). I'm afraid we need to keep it. We're using request in AbstractController which, as name suggest, is abstract thus in rails world it's either base of ActionController which provides request or ActionMailer which does not. I agree it doesn't look very nice, but I think it needs to stay.
Here's diff with attempt to remove this check which made me realize that it should stay - https://gist.github.com/strzalek/68b407edc52b608fb797 What do you think?

@rafaelfranca removed additional hash lookup and added docs 🍻

@rafaelfranca

View changes

Show outdated Hide outdated actionpack/lib/action_controller/metal/mime_responds.rb
@rafaelfranca

View changes

Show outdated Hide outdated actionpack/lib/action_controller/metal/mime_responds.rb
@rafaelfranca

View changes

Show outdated Hide outdated actionpack/lib/action_controller/metal/mime_responds.rb
@lukaszx0

This comment has been minimized.

Show comment
Hide comment
@lukaszx0

lukaszx0 Nov 26, 2013

Member

Thanks @rafaelfranca, I've just pushed fixes to docs.

Member

lukaszx0 commented Nov 26, 2013

Thanks @rafaelfranca, I've just pushed fixes to docs.

@lukaszx0

This comment has been minimized.

Show comment
Hide comment
@lukaszx0
Member

lukaszx0 commented Nov 26, 2013

:shipit:

@jeremy

View changes

Show outdated Hide outdated actionpack/CHANGELOG.md
@jeremy

View changes

Show outdated Hide outdated actionpack/CHANGELOG.md
@jeremy

View changes

Show outdated Hide outdated actionpack/CHANGELOG.md
@jeremy

View changes

Show outdated Hide outdated actionpack/lib/action_controller/metal/mime_responds.rb
@jeremy

View changes

Show outdated Hide outdated actionpack/lib/action_dispatch/http/mime_negotiation.rb
html.phone { render text: "phone" }
end
end
end

This comment has been minimized.

@jeremy

jeremy Nov 27, 2013

Member

What happens when someone does

respond_to do |variant|
  variant.tablet do
    ...

by accident, not understanding the respond_to nesting?

@jeremy

jeremy Nov 27, 2013

Member

What happens when someone does

respond_to do |variant|
  variant.tablet do
    ...

by accident, not understanding the respond_to nesting?

This comment has been minimized.

@jeremy

jeremy Nov 27, 2013

Member

Responder blocks can be used to handle multiple formats. What about multiple variants?

respond_to do |format|
  format.html do |html|
    html.tablet
    html.any(:ipod, :iphone) { ... }
    html.all { ... }
  end
end
@jeremy

jeremy Nov 27, 2013

Member

Responder blocks can be used to handle multiple formats. What about multiple variants?

respond_to do |format|
  format.html do |html|
    html.tablet
    html.any(:ipod, :iphone) { ... }
    html.all { ... }
  end
end

This comment has been minimized.

@lukaszx0

lukaszx0 Nov 27, 2013

Member
  1. I'm afraid we can't do anything here. There's no way to distinguish and guess user behavior. User will get error with hash with tablet in formats array which should give him idea what's wrong (something like that {:locale => [], :formats => [:tablet], :variants => [], :handlers => [:erb]}

  2. We don't support any/all for variants, this should be easy to add.

@lukaszx0

lukaszx0 Nov 27, 2013

Member
  1. I'm afraid we can't do anything here. There's no way to distinguish and guess user behavior. User will get error with hash with tablet in formats array which should give him idea what's wrong (something like that {:locale => [], :formats => [:tablet], :variants => [], :handlers => [:erb]}

  2. We don't support any/all for variants, this should be easy to add.

This comment has been minimized.

@jeremy

jeremy Nov 27, 2013

Member
  1. What does the error look like if someone does this now?
  2. It's fine to not support that for the first version. However, when someone tries html.all, we will interpret that as an html+all variant. So it'd be a good idea to disallow any and all variant names, at least.
@jeremy

jeremy Nov 27, 2013

Member
  1. What does the error look like if someone does this now?
  2. It's fine to not support that for the first version. However, when someone tries html.all, we will interpret that as an html+all variant. So it'd be a good idea to disallow any and all variant names, at least.

This comment has been minimized.

@lukaszx0

lukaszx0 Nov 27, 2013

Member
  1. NameError: uninitialized constant Mime::TABLET
  2. 👍
@lukaszx0

lukaszx0 Nov 27, 2013

Member
  1. NameError: uninitialized constant Mime::TABLET
  2. 👍

This comment has been minimized.

@jeremy

jeremy Nov 27, 2013

Member
  1. can definitely be improved. We can test whether the MIME type is registered. If it isn't, raise a NoMethodError with a helpful message rather than bubbling up an internal NameError. For example: "To respond to a custom format, register it as a MIME type first: http://guides.rubyonrails.org/action_controller_overview.html#restful-downloads. If you meant to respond to a variant like :tablet or :phone, not a custom format, be sure to nest your variant response within a format response: format.html { |html| html.tablet { ..."
@jeremy

jeremy Nov 27, 2013

Member
  1. can definitely be improved. We can test whether the MIME type is registered. If it isn't, raise a NoMethodError with a helpful message rather than bubbling up an internal NameError. For example: "To respond to a custom format, register it as a MIME type first: http://guides.rubyonrails.org/action_controller_overview.html#restful-downloads. If you meant to respond to a variant like :tablet or :phone, not a custom format, be sure to nest your variant response within a format response: format.html { |html| html.tablet { ..."

This comment has been minimized.

@lukaszx0

lukaszx0 Nov 27, 2013

Member

Yeah, agreed. I'll work on that.

On Wed, Nov 27, 2013 at 2:54 PM, Jeremy Kemper notifications@github.comwrote:

In actionpack/test/controller/mime/respond_to_test.rb:

  • def variant_with_format_and_custom_render
  • request.variant = :mobile
  • respond_to do |type|
  •  type.html { render text: "mobile" }
    
  • end
  • end
  • def multiple_variants_for_format
  • respond_to do |type|
  •  type.html do |html|
    
  •    html.tablet { render text: "tablet" }
    
  •    html.phone  { render text: "phone" }
    
  •  end
    
  • end
  • end
  1. can definitely be improved. We can test whether the MIME type is
    registered. If it isn't, raise a NoMethodError with a helpful message
    rather than bubbling up an internal NameError. For example: "To
    respond to a custom format, register it as a MIME type first:
    http://guides.rubyonrails.org/action_controller_overview.html#restful-downloads.
    If you meant to respond to a variant like :tablet or :phone, not a
    custom format, be sure to nest your variant response within a format
    response: format.html { |html| html.tablet { ..."


Reply to this email directly or view it on GitHubhttps://github.com/rails/rails/pull/12977/files#r7957207
.

@lukaszx0

lukaszx0 Nov 27, 2013

Member

Yeah, agreed. I'll work on that.

On Wed, Nov 27, 2013 at 2:54 PM, Jeremy Kemper notifications@github.comwrote:

In actionpack/test/controller/mime/respond_to_test.rb:

  • def variant_with_format_and_custom_render
  • request.variant = :mobile
  • respond_to do |type|
  •  type.html { render text: "mobile" }
    
  • end
  • end
  • def multiple_variants_for_format
  • respond_to do |type|
  •  type.html do |html|
    
  •    html.tablet { render text: "tablet" }
    
  •    html.phone  { render text: "phone" }
    
  •  end
    
  • end
  • end
  1. can definitely be improved. We can test whether the MIME type is
    registered. If it isn't, raise a NoMethodError with a helpful message
    rather than bubbling up an internal NameError. For example: "To
    respond to a custom format, register it as a MIME type first:
    http://guides.rubyonrails.org/action_controller_overview.html#restful-downloads.
    If you meant to respond to a variant like :tablet or :phone, not a
    custom format, be sure to nest your variant response within a format
    response: format.html { |html| html.tablet { ..."


Reply to this email directly or view it on GitHubhttps://github.com/rails/rails/pull/12977/files#r7957207
.

This comment has been minimized.

@jeremy

jeremy Dec 2, 2013

Member

@strzalek ping - how's the error handling going?

@jeremy

jeremy Dec 2, 2013

Member

@strzalek ping - how's the error handling going?

assert_raise ArgumentError do
request.variant = "mobile"
end
end

This comment has been minimized.

@jeremy

jeremy Nov 27, 2013

Member

Why is the variant required to be a Symbol?

When we're providing an API to set a specialized format based on a user request, we're just begging for developers to set the variant to user-provided data. With security in mind, working with Strings is the API I'd expect if we're assigning data from the outside world.

Alternatively, declaring a whitelist of allowed variants would allow us to safely call .to_sym on any String assigned to request.variant as long as it's whitelisted.

@jeremy

jeremy Nov 27, 2013

Member

Why is the variant required to be a Symbol?

When we're providing an API to set a specialized format based on a user request, we're just begging for developers to set the variant to user-provided data. With security in mind, working with Strings is the API I'd expect if we're assigning data from the outside world.

Alternatively, declaring a whitelist of allowed variants would allow us to safely call .to_sym on any String assigned to request.variant as long as it's whitelisted.

This comment has been minimized.

@lukaszx0

lukaszx0 Nov 27, 2013

Member

We want to avoid situation in which users will blindly or mistakenly pass hashes or arrays as variants (request.variant = params[:users]). Converting this to string if it's hash/array can only cause problems. We need to have type check here and can't rely on user and do typecast on our side.

I don't know if we should whitelist variants though. First thought is no, but I can imagine that we can come up with list of ~5 (?) commonly used variants, which will be used in 90% cases (I can guess that mobile, tablet will land on that list for sure). Don't have opinion here right now.

@lukaszx0

lukaszx0 Nov 27, 2013

Member

We want to avoid situation in which users will blindly or mistakenly pass hashes or arrays as variants (request.variant = params[:users]). Converting this to string if it's hash/array can only cause problems. We need to have type check here and can't rely on user and do typecast on our side.

I don't know if we should whitelist variants though. First thought is no, but I can imagine that we can come up with list of ~5 (?) commonly used variants, which will be used in 90% cases (I can guess that mobile, tablet will land on that list for sure). Don't have opinion here right now.

This comment has been minimized.

@tenderlove

tenderlove Mar 11, 2014

Member

Converting this to string if it's hash/array can only cause problems.

Can you describe the specific problems? I have doubts.

@tenderlove

tenderlove Mar 11, 2014

Member

Converting this to string if it's hash/array can only cause problems.

Can you describe the specific problems? I have doubts.

@carlosantoniodasilva

View changes

Show outdated Hide outdated actionview/lib/action_view/testing/resolvers.rb
@lukaszx0

This comment has been minimized.

Show comment
Hide comment
@lukaszx0

lukaszx0 Nov 29, 2013

Member
Member

lukaszx0 commented Nov 29, 2013

@carlosantoniodasilva

View changes

Show outdated Hide outdated actionpack/lib/abstract_controller/rendering.rb
@carlosantoniodasilva

View changes

Show outdated Hide outdated actionpack/lib/action_controller/metal/mime_responds.rb
"Check user-provided value against a whitelist first, then set the variant:"+
"request.variant = :tablet if params[:some_param] == 'tablet'"
end
end

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Dec 3, 2013

Member

Are we going to stick with symbols or make these strings internally?

@carlosantoniodasilva

carlosantoniodasilva Dec 3, 2013

Member

Are we going to stick with symbols or make these strings internally?

This comment has been minimized.

@lukaszx0

lukaszx0 Dec 3, 2013

Member

I would stay with symbols.

@lukaszx0

lukaszx0 Dec 3, 2013

Member

I would stay with symbols.

@carlosantoniodasilva

View changes

Show outdated Hide outdated actionpack/lib/action_dispatch/http/request.rb
@carlosantoniodasilva

View changes

Show outdated Hide outdated actionview/lib/action_view/template/resolver.rb
@lukaszx0

This comment has been minimized.

Show comment
Hide comment
@lukaszx0

lukaszx0 Dec 3, 2013

Member

@dhh @jeremy I've just pushed updates 🚢

Member

lukaszx0 commented Dec 3, 2013

@dhh @jeremy I've just pushed updates 🚢

@carlosantoniodasilva

View changes

Show outdated Hide outdated actionpack/lib/action_controller/metal/mime_responds.rb
@carlosantoniodasilva

View changes

Show outdated Hide outdated actionpack/lib/action_controller/metal/mime_responds.rb

lukaszx0 added some commits Dec 3, 2013

Action Pack Variants
By default, variants in the templates will be picked up if a variant is set
and there's a match. The format will be:

  app/views/projects/show.html.erb
  app/views/projects/show.html+tablet.erb
  app/views/projects/show.html+phone.erb

If request.variant = :tablet is set, we'll automatically be rendering the
html+tablet template.

In the controller, we can also tailer to the variants with this syntax:

  class ProjectsController < ActionController::Base
    def show
      respond_to do |format|
        format.html do |html|
          @stars = @project.stars

          html.tablet { @notifications = @project.notifications }
          html.phone  { @chat_heads    = @project.chat_heads }
        end

        format.js
        format.atom
      end
    end
  end

The variant itself is nil by default, but can be set in before filters, like
so:

  class ApplicationController < ActionController::Base
    before_action do
      if request.user_agent =~ /iPad/
        request.variant = :tablet
      end
    end
  end

This is modeled loosely on custom mime types, but it's specifically not
intended to be used together. If you're going to make a custom mime type,
you don't need a variant. Variants are for variations on a single mime
types.

jeremy added a commit that referenced this pull request Dec 3, 2013

@jeremy jeremy merged commit 501acab into rails:master Dec 3, 2013

@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Dec 3, 2013

Member

❤️

Member

jeremy commented Dec 3, 2013

❤️

@aag1091

This comment has been minimized.

Show comment
Hide comment
@aag1091

aag1091 commented Dec 24, 2013

<3 thank you @strzalek

@lukaszx0 lukaszx0 deleted the lukaszx0:action-pack-variants branch Feb 12, 2014

@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Mar 11, 2014

Member

wtf?

Edit: Sorry, I'll be more specific than "wtf". Why doesn't this call super if name != @variant? Is there a way we can implement this without using method_missing?

wtf?

Edit: Sorry, I'll be more specific than "wtf". Why doesn't this call super if name != @variant? Is there a way we can implement this without using method_missing?

This comment has been minimized.

Show comment
Hide comment
@lukaszx0

lukaszx0 Mar 11, 2014

Member

It was changed and modified already a few times (because we were adding new features). Right now we don't have VariantFilter and it now we have VariantsCollector and whole thing works a bit differently: https://github.com/rails/rails/blob/master/actionpack/lib/action_controller/metal/mime_responds.rb#L512-L544

Member

lukaszx0 replied Mar 11, 2014

It was changed and modified already a few times (because we were adding new features). Right now we don't have VariantFilter and it now we have VariantsCollector and whole thing works a bit differently: https://github.com/rails/rails/blob/master/actionpack/lib/action_controller/metal/mime_responds.rb#L512-L544

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Mar 11, 2014

Member

@strzalek awesome. Thanks for the explanation! I'll poke at the new code for a bit. ❤️

Member

tenderlove replied Mar 11, 2014

@strzalek awesome. Thanks for the explanation! I'll poke at the new code for a bit. ❤️

@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Mar 11, 2014

Member

Is this something we're really really really really sure isn't going to see user input? I mean really really really sure? Note how symbol count increases when we call upcase:

irb(main):003:0> Symbol.all_symbols.count
=> 3167
irb(main):004:0> :himom.upcase
=> :HIMOM
irb(main):005:0> Symbol.all_symbols.count
=> 3169
irb(main):006:0>

Is this something we're really really really really sure isn't going to see user input? I mean really really really sure? Note how symbol count increases when we call upcase:

irb(main):003:0> Symbol.all_symbols.count
=> 3167
irb(main):004:0> :himom.upcase
=> :HIMOM
irb(main):005:0> Symbol.all_symbols.count
=> 3169
irb(main):006:0>

This comment has been minimized.

Show comment
Hide comment
@lukaszx0

lukaszx0 Mar 11, 2014

Member

I think we can cast it to string here to avoid this problem.

Member

lukaszx0 replied Mar 11, 2014

I think we can cast it to string here to avoid this problem.

This comment has been minimized.

Show comment
Hide comment
@amolpujari

amolpujari Mar 11, 2014

great to see and learn. I recently saw one of my rails consoles as

Loading development environment (Rails 3.1.10)

Frame number: 0/4
[1] pry(main)> Symbol.all_symbols.count
=> 47415
[2] pry(main)> 

And I was simply thinking, can rails completely avoid symbols?

amolpujari replied Mar 11, 2014

great to see and learn. I recently saw one of my rails consoles as

Loading development environment (Rails 3.1.10)

Frame number: 0/4
[1] pry(main)> Symbol.all_symbols.count
=> 47415
[2] pry(main)> 

And I was simply thinking, can rails completely avoid symbols?

@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Mar 11, 2014

Member

What technical reason requires that we store the variant as a symbol? Can't we just call to_s on this an deal with strings internally?

What technical reason requires that we store the variant as a symbol? Can't we just call to_s on this an deal with strings internally?

This comment has been minimized.

Show comment
Hide comment
@lukaszx0
Member

lukaszx0 replied Mar 11, 2014

This comment has been minimized.

Show comment
Hide comment
@lukaszx0

lukaszx0 Mar 11, 2014

Member

I would love to keep it consistent. We keep format as symbol and it would be good to have variant as symbol as well.

Member

lukaszx0 replied Mar 11, 2014

I would love to keep it consistent. We keep format as symbol and it would be good to have variant as symbol as well.

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Mar 11, 2014

Member

I prefer we keep neither as symbols. Working with symbols inside the framework is a huge pain from a security perspective. Also, inside the framework it doesn't matter if we use strings.

Member

tenderlove replied Mar 11, 2014

I prefer we keep neither as symbols. Working with symbols inside the framework is a huge pain from a security perspective. Also, inside the framework it doesn't matter if we use strings.

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Mar 11, 2014

Member

@strzalek I read both comments. Neither of them provided technical reasons for the requirement that this be a symbol internally. Please describe exactly why it needs to be a symbol, otherwise I want to cast these to strings and remove the exception.

Member

tenderlove replied Mar 11, 2014

@strzalek I read both comments. Neither of them provided technical reasons for the requirement that this be a symbol internally. Please describe exactly why it needs to be a symbol, otherwise I want to cast these to strings and remove the exception.

This comment has been minimized.

Show comment
Hide comment
@lukaszx0

lukaszx0 Mar 11, 2014

Member

I have no problem casting it to string as long as we'll do it for both format and variant.

Member

lukaszx0 replied Mar 11, 2014

I have no problem casting it to string as long as we'll do it for both format and variant.

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