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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from Mar 28, 2019
Merged

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

merged 1 commit into from Mar 28, 2019

Conversation

erose
Copy link
Contributor

@erose 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

@rails-bot rails-bot bot added the actionview label Feb 18, 2019
@erose erose mentioned this pull request Feb 18, 2019
Closed
@erose
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
sikachu approved these changes Mar 28, 2019
View reviewed changes
Copy link
Member

@sikachu sikachu left a comment

Choose a reason for hiding this comment

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

I think the code looks good. Thanks!

@rafaelfranca rafaelfranca merged commit e3f5f1c into rails:master Mar 28, 2019
@erose
Copy link
Contributor Author

erose commented Mar 29, 2019

Thanks for the review, @sikachu !

@matthewd
Copy link
Member

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
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
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
Copy link
Member

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
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
Copy link
Contributor Author

erose commented Mar 30, 2019

Thank you so much for pointing this out!

@matthewd
Copy link
Member

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?

@yahonda yahonda mentioned this pull request Apr 19, 2019
Closed
@kaspth kaspth mentioned this pull request May 27, 2019
Closed
@erose
Copy link
Contributor Author

erose commented Sep 27, 2019

I took another look at this today -- it seems to me this issue has been addressed since I was last here. Thanks to whomever did this work!

@eugeneius eugeneius mentioned this pull request Dec 21, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sikachu sikachu sikachu approved these changes

Assignees
No one assigned
Labels
actionview
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Calling methods in .erb files can cause a confusing error when a parenthesis is missing
4 participants
@erose @matthewd @sikachu @rafaelfranca