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

Avoid compiling ruby keywords into template locals #26672

Merged
merged 1 commit into from Oct 2, 2016

Conversation

Projects
None yet
4 participants
@schpet
Copy link
Contributor

schpet commented Sep 30, 2016

Summary

This is a change to ActionView::Template. It lets you pass ruby keywords to render:

<%= render 'my_partial', class: "cool" %>

It prevents confusing syntax errors. Normally you'd see a misleading error on the template's line 1, where there is no error:

SyntaxError (.../_my_partial.html.erb:1: syntax error, unexpected '='

Instead it will render successfully. If you use a keyword in your template, you get a syntax error on the line where you use it .

The keywords are still available in local_assigns, which is useful if your template takes in variables to use as html attributes, like class.

Other Information

I've also made this change available as a gem: https://rubygems.org/gems/actionview_template_safe_locals

@schpet schpet force-pushed the schpet:support_ruby_keywords_as_template_locals branch 2 times, most recently Sep 30, 2016

actionview/lib/action_view/template.rb Outdated
@@ -324,8 +329,10 @@ def handle_render_error(view, e) #:nodoc:
end

def locals_code #:nodoc:
locals = @locals.delete_if { |key| RUBY_KEYWORDS.include?(key.downcase) }

This comment has been minimized.

@matthewd

matthewd Sep 30, 2016

Member

Why downcase?

This comment has been minimized.

@schpet

schpet Sep 30, 2016

Author Contributor

@matthewd this will handle <%= render 'template', locals: { CLASS: "food", Alias: "bar" } which cause the same misleading syntax error.

I'd be OK changing this to not handle those edge cases though, please let me know.

This comment has been minimized.

@matthewd

matthewd Sep 30, 2016

Member

I'm just unclear why those would give the same error.

If the error they're giving is a more general "you can't set a constant here", then it seems we'd want to handle that separately -- Foo would be just as problematic as Alias.

Do we want to check for something like /\A(?!=[A-Z0-9])\w+\z/? (There's probably a better definition of a valid local identifier than that, somewhere.)

actionview/lib/action_view/template.rb Outdated
RUBY_KEYWORDS = %w(begin end alias and begin break case class def do else
elsif end ensure false for if in module next nil not or
redo rescue retry return self super then true undef
unless until when while yield).to_set

This comment has been minimized.

@matthewd

matthewd Sep 30, 2016

Member

I think we already have a list like this.. AS delegate, maybe?

Is there a practical way we can re-use it? Possible it makes more sense to keep a separate copy here, but let's at least explore our options.

This comment has been minimized.

@schpet

schpet Sep 30, 2016

Author Contributor

@matthewd thanks! there was a list in AS delegate, 683c907 addresses this

actionview/lib/action_view/template.rb Outdated
@@ -328,7 +325,8 @@ def handle_render_error(view, e) #:nodoc:
end

def locals_code #:nodoc:
locals = @locals.delete_if { |key| TEMPLATE_RESERVED_METHOD_NAMES.include?(key.downcase) }
locals = @locals.to_set - Module::DELEGATION_RESERVED_METHOD_NAMES

This comment has been minimized.

@kaspth

kaspth Sep 30, 2016

Member

iirc Array - already compares by hash for speed. The set conversion shouldn't be necessary.

This comment has been minimized.

@schpet

schpet Sep 30, 2016

Author Contributor

@kaspth with - I get an error: "no implicit conversion of Set into Array"
DELEGATION_RESERVED_METHOD_NAMES is a set. Am I able to subtract a set from an array without converting it?

actionview/lib/action_view/template.rb Outdated
@@ -324,8 +325,11 @@ def handle_render_error(view, e) #:nodoc:
end

def locals_code #:nodoc:
locals = @locals.to_set - Module::DELEGATION_RESERVED_METHOD_NAMES
locals = locals.select { |key| /\A[a-z_][a-zA-Z_0-9]*\z/.match?(key) }

This comment has been minimized.

@kaspth

kaspth Sep 30, 2016

Member

We need to require the match? core ext file that backports it all the way to Ruby 2.2.

This comment has been minimized.

@matthewd

matthewd Sep 30, 2016

Member

Or use grep

This comment has been minimized.

@matthewd

matthewd Sep 30, 2016

Member

Did you find a reference to guide the regexp? I was trying for something that might do the right thing on unicode characters.

This comment has been minimized.

@schpet

schpet Sep 30, 2016

Author Contributor

Did not find a reference for regexp. This regex is incorrect, was looking for something that would handle unicode but haven't found anything useful yet (e.g. something that matches a single emoji as a valid identifier.) Open to suggestions!

This comment has been minimized.

@matthewd

This comment has been minimized.

@schpet

schpet Sep 30, 2016

Author Contributor

excellent, thank you: e1c1274

@schpet schpet force-pushed the schpet:support_ruby_keywords_as_template_locals branch Sep 30, 2016

@schpet

This comment has been minimized.

Copy link
Contributor Author

schpet commented Sep 30, 2016

I've rebased this onto latest master, hoping that CI passes this time

@schpet

This comment has been minimized.

Copy link
Contributor Author

schpet commented Sep 30, 2016

CI failed, but it doesn't appear to be related to this.

@maclover7

This comment has been minimized.

Copy link
Member

maclover7 commented Sep 30, 2016

Restarted flaky builds, CI is green now. :)

@matthewd
Copy link
Member

matthewd left a comment

Looking great!

Couple of tiny things, then squash the commits and add a changelog entry, and I think we're good to go. 👍🏻

actionview/lib/action_view/template.rb Outdated
@@ -324,8 +325,11 @@ def handle_render_error(view, e) #:nodoc:
end

def locals_code #:nodoc:
locals = @locals.to_set - Module::DELEGATION_RESERVED_METHOD_NAMES

This comment has been minimized.

@matthewd

matthewd Oct 1, 2016

Member

Let's add a comment noting why we're doing this:

# Only locals with valid variable names get set directly. Others will still be
# available in local_assigns.
actionview/lib/action_view/template.rb Outdated
@@ -1,5 +1,6 @@
require "active_support/core_ext/object/try"
require "active_support/core_ext/kernel/singleton_class"
require "active_support/core_ext/module"

This comment has been minimized.

@matthewd

matthewd Oct 1, 2016

Member

Can we load just the delegation file? The two lines above show a precedent of being as narrow as practical.

@schpet schpet force-pushed the schpet:support_ruby_keywords_as_template_locals branch Oct 1, 2016

@schpet

This comment has been minimized.

Copy link
Contributor Author

schpet commented Oct 1, 2016

cool! I think it's all good now, let me know if I need to amend this with anything.

@maclover7 @kaspth @matthewd thanks so much for your time helping me with this!

@matthewd thanks for the super helpful feedback! i'm so happy this PR went from "filter out reserved words" to "only set variables that make sense" 💯

@schpet

This comment has been minimized.

Copy link
Contributor Author

schpet commented Oct 1, 2016

CI failed again, but it's unrelated to this patch

Change render to support any hash keys in locals
this lets you pass ruby keywords to templates:

    <%= render 'example', class: "cool" %>

    <%= render 'example', "spaces are" => "a-ok" %>

    <%= render 'example', Foo: "bar" %>

Previously you'd see confusing syntax errors like this:

    SyntaxError (.../_example.html.erb:1: syntax error, unexpected '='

Now you can reference invalid identifiers through local_assigns.

If you try to use an invalid keyword (e.g. class) in your template, you
get a syntax error on the line where you use it.

@schpet schpet force-pushed the schpet:support_ruby_keywords_as_template_locals branch to f9960f2 Oct 2, 2016

@schpet

This comment has been minimized.

Copy link
Contributor Author

schpet commented Oct 2, 2016

@matthewd i amended the changelog to add your name to it, since you provided the regexp and pointed me towards AS delegate ✌️

@matthewd matthewd merged commit 7b63f56 into rails:master Oct 2, 2016

2 checks passed

codeclimate Code Climate didn't find any new or fixed issues.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

matthewd added a commit that referenced this pull request Oct 2, 2016

Merge pull request #26672 from schpet/support_ruby_keywords_as_templa…
…te_locals

Avoid compiling ruby keywords into template locals
@matthewd

This comment has been minimized.

Copy link
Member

matthewd commented Oct 2, 2016

This seems harmless enough to backport, though it's borderline whether it's really a bug fix -- it should only change behaviour in cases that would previously error.

fea3b44

@schpet

This comment has been minimized.

Copy link
Contributor Author

schpet commented Oct 2, 2016

i wrote a blog post about my experience contributing to rails, and how it really exceeded my expectations. it mentions you folks ✌️
http://www.peterschilling.org/blackhole/babys-first-rails-commit/

@kaspth

This comment has been minimized.

Copy link
Member

kaspth commented Oct 3, 2016

@schpet They grow up so fast blows handkerchief — thanks for the kind words and we hope to see you contribute again 😎

pixeltrix added a commit that referenced this pull request Jan 15, 2017

Allow render locals to be assigned to instance variables
In #26672 we blocked use of Ruby keywords as identifiers for view
locals but inadvertently broke the use of instance variable names
as identifiers. Whilst not explicitly documented this behavior has
been around for a long time and there's no need to break it now.

Fixes #27480.

pixeltrix added a commit that referenced this pull request Jan 15, 2017

Allow render locals to be assigned to instance variables
In #26672 we blocked use of Ruby keywords as identifiers for view
locals but inadvertently broke the use of instance variable names
as identifiers. Whilst not explicitly documented this behavior has
been around for a long time and there's no need to break it now.

Fixes #27480.

(cherry picked from commit b5edc55)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.