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

Display a more helpful error message when an ERB template has a Ruby syntax error. #35308

Conversation

Projects
None yet
4 participants
@erose
Copy link
Contributor

erose commented Feb 18, 2019

Summary

Currently, if code that is intended to be interpolated into a template has a syntax error (e.g. <%= foo( %>), Rails displays (what I find to be) a confusing error message. The error message refers to a syntax error in code the user hasn't written, and references an unrelated line of the template.

This PR is an attempt to resolve this issue.

Resolves #34995. (My own issue.)

Other Information

This is my first time contributing to Rails, so I would certainly appreciate critiques of my coding style and/or my overall approach.

I wasn't able to achieve my ideal user experience; that would be displaying the offending line in context in the "extracted source" box, rather than displaying a blank box, as occurs both before and after my change.

I couldn't see a way to accomplish this without refactors (because ActionDispatch::ExceptionWrapper#source_extracts relies on parsing the backtrace in order to display the correct context, and our backtrace is polluted with generated code), and felt I would probably make significant errors in refactoring a codebase I was totally new to. But if there is a way to accomplish it, I would love to know.

In the end I decided to go with this approach as it seemed like an improvement on the status quo; it might give the user more hints as to what is happening.

Below I've attached screenshots of the user experience in the case where the user has made a Ruby syntax error in an ERB template and visits the page in development mode.

Before:

image

After:

image

@erose

This comment has been minimized.

Copy link
Contributor Author

erose commented Mar 14, 2019

Anything else I can do to help with this?

Tagging @sikachu as you commented on the original issue I filed.

@sikachu
Copy link
Member

sikachu left a comment

I think the code looks good. Thanks!

@rafaelfranca rafaelfranca merged commit e3f5f1c into rails:master Mar 28, 2019

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@erose

This comment has been minimized.

Copy link
Contributor Author

erose commented Mar 29, 2019

Thanks for the review, @sikachu !

@matthewd

This comment has been minimized.

Copy link
Member

matthewd commented Mar 29, 2019

I like the idea of improving this error, but does dumping the entire view source into the message really achieve that? It looks nice for a one-line-view example, but for a more realistic-size view it seems like it's going to be at least as much of an imposing mess as the old one.

@sikachu

This comment has been minimized.

Copy link
Member

sikachu commented Mar 29, 2019

@matthewd so you mean .. we could just show

Encountered a syntax error while rendering template around line 10

[ source code block ]

and that would be enough?

@erose

This comment has been minimized.

Copy link
Contributor Author

erose commented Mar 29, 2019

@matthewd Thanks for the feedback! I agree this might be an issue, although I'm not yet convinced that in that situation the error message after this change would be worse than the error message was before this change.

What would be a good example case to try this on? Perhaps some multi-line call like:

<%= render('test_partial', locals: {
  foo: 1,
  bar: [
    :a, :b, :c
  ],
%>

?

@matthewd

This comment has been minimized.

Copy link
Member

matthewd commented Mar 29, 2019

Ideally, the source code block would contain the view source around the line of interest (does it currently? the screenshot looks like it's empty), but otherwise yeah.

(Though in the reported case, where a paren has been left open, the line of actual interest is the one where that happened, but the line Ruby will consider interesting is the outside-of-template end. Nothing we can do about that, and it's no different to an equivalent error in any other source file.)

@erose I'm thinking of an error of that form, inserted into the middle of a view file that looks more like https://github.com/discourse/discourse/blob/master/app/views/layouts/application.html.erb.

@erose

This comment has been minimized.

Copy link
Contributor Author

erose commented Mar 30, 2019

You're right -- with my change, this file:

<!DOCTYPE html>
<html lang="<%= html_lang %>" class="<%= html_classes %>">
  <head>
    <meta charset="utf-8">
    <title><%= content_for?(:title) ? yield(:title) : SiteSetting.title %></title>
    <meta name="description" content="<%= @description_meta || SiteSetting.site_description %>">
    <meta name="discourse_theme_ids" content="<%= theme_ids&.join(",") %>">
    <meta name="discourse_current_homepage" content="<%= current_homepage %>">
  </head>

  <body>
    <%= render(
      'test_partial',
       locals: {
        foo: 1,
        bar: 2,
       },
    %>
  </body>
</html>

yields an error screen like this

image

which I think is worse than the situation beforehand.

In that light, I'd like to revert this change. How can that be achieved?

@erose

This comment has been minimized.

Copy link
Contributor Author

erose commented Mar 30, 2019

Thank you so much for pointing this out!

@matthewd

This comment has been minimized.

Copy link
Member

matthewd commented Mar 30, 2019

I'd be keen to roll forward, rather than back, if you're interested: to me the substance of your change is "maybe we should actually consider how this error looks", which I'm 10/10 on board for.

Is source_fragment breaking because of more complex issues with the backtrace, or is it just empty because it's trying to show lines 8-12 of a 7 line file?

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.