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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve ERB dependency detection #13414

Merged
merged 3 commits into from
Jan 16, 2014

Conversation

britto
Copy link
Contributor

@britto britto commented Dec 19, 2013

Following the discussion started on #13116, I thought it might be worth giving a shot at some of the issues we have identified.

The current implementation can't handle some special cases of oddly-formatted Ruby. Now we are able to detect them:

  • Multi-line arguments on the render call
  • Strings containing quotes, e.g. "something's wrong"
  • Multiple kinds of identifiers - instance variables, class variables and globals
  • Method chains as arguments for the render call
  • "Glued" identifiers, e.g. render@things, render:partial => 'partial', etc.

Also, this fix reduces the rate of "false positives" which showed up when we had calls/access to identifiers containing render, like surrender and rendering.

Related: #13074.

Update:

There is still one edge case for false positives which will be virtually impossible to avoid whithout actually parsing the template: when we have render inside a string, as in:

<%= content_tag :p, 'Then we render something nice.' %>
<%# "somethings/something" %>

For obvious reasons 馃挘. But I guess that will not be such an issue. What we have got so far is probably about as far as we can go with a human-readable regex.

end

def test_finds_no_dependency_when_render_ends_the_name_of_another_method
template = FakeTemplate.new("<%# surrender 'to reason' %>", :erb)
Copy link
Member

Choose a reason for hiding this comment

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

馃

@josevalim
Copy link
Contributor

Oops, we got some failures in the build:

  1) Failure:
TemplateDigestorTest#test_extra_whitespace_in_render_named_partial [/home/travis/build/rails/rails/actionview/test/template/digestor_test.rb:174]:
digest didn't change
  2) Failure:
TemplateDigestorTest#test_extra_whitespace_in_render_record [/home/travis/build/rails/rails/actionview/test/template/digestor_test.rb:180]:
digest didn't change
  3) Failure:
TemplateDigestorTest#test_extra_whitespace_in_render_with_parenthesis [/home/travis/build/rails/rails/actionview/test/template/digestor_test.rb:186]:
digest didn't change

@britto
Copy link
Contributor Author

britto commented Dec 20, 2013

@josevalim thanks! unfortunately, I can't take a look into it right now. It seems to be some problem with multiple mathces. I'll get back to it ASAP.

@britto
Copy link
Contributor Author

britto commented Dec 21, 2013

Turns out String#scan gets greedy when we enable multiline-mode. I think a split & scan approach may be more adequate to tackle that.

@britto
Copy link
Contributor Author

britto commented Dec 23, 2013

Any clue on that Memcached issue? The suite passes locally.

@carlosantoniodasilva
Copy link
Member

@britto I've restarted the failing job, lets wait and see.

@britto
Copy link
Contributor Author

britto commented Dec 24, 2013

@carlosantoniodasilva great! 馃崗

The current implementation can't handle some special cases of oddly-formatted Ruby. Now we are able to detect them:

* Multi-line arguments on the `render` call
* Strings containing quotes, e.g. `"something's wrong"`
* Multiple kinds of identifiers - instance variables, class variables and globals
* Method chains as arguments for the `render` call

Also, this fix reduces the rate of "false positives" which showed up when we had calls/access to identifiers containing `render`, like `surrender` and `rendering`.
Each chunk of text coming after `render` is now handled individually as a possible list of arguments.
rafaelfranca added a commit that referenced this pull request Jan 16, 2014
@rafaelfranca rafaelfranca merged commit 662f8de into rails:master Jan 16, 2014
@arthurnn
Copy link
Member

鉂わ笍

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

6 participants