Redesign ActionView::Template::Handlers::ERB.find_offset to handle edge cases #53657
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
martinemde
force-pushed
the
martinemde/find_offset-test-and-fix
branch
6 times, most recently
from
2e6c967 to
2482827
Compare
martinemde
force-pushed
the
martinemde/find_offset-test-and-fix
branch
4 times, most recently
from
c6e8a35 to
6541168
Compare
martinemde
force-pushed
the
martinemde/find_offset-test-and-fix
branch
from
6541168 to
2e6471a
Compare
byroot
reviewed
Nov 18, 2024
martinemde
force-pushed
the
martinemde/find_offset-test-and-fix
branch
from
2e6471a to
7006970
Compare
martinemde
changed the title
martinemde
force-pushed
the
martinemde/find_offset-test-and-fix
branch
from
7006970 to
0b8ea3e
Compare
martinemde
commented
Nov 18, 2024
martinemde
force-pushed
the
martinemde/find_offset-test-and-fix
branch
from
0b8ea3e to
f590e07
Compare
byroot
reviewed
Nov 21, 2024
|
I'm not very familiar with that code, but the change look good and the extra tests are appreciated. Let me know when CI is green and I'll merge + backport. |
martinemde
force-pushed
the
martinemde/find_offset-test-and-fix
branch
2 times, most recently
from
21cd9af to
a3fb9cc
Compare
|
Build is passing now. Thanks @byroot! (EDIT: ack, caught a spelling error, rebuilding) |
The code for finding offsets in the ERB templates outright fails in some cases, causing 500s in the error template. In other cases it will bail out early when it's possible to find and highlight correctly. This PR fixes many of the issues that regularly occur and correctly highlights more often. Also adds test coverage of the translate_location method on ActionView::Template, many of which failed before this fix.
martinemde
force-pushed
the
martinemde/find_offset-test-and-fix
branch
from
a3fb9cc to
aaebc86
Compare
byroot
added a commit
that referenced
this pull request
Nov 21, 2024
…-and-fix Redesign ActionView::Template::Handlers::ERB.find_offset to handle edge cases
byroot
added a commit
that referenced
this pull request
Nov 21, 2024
…-and-fix Redesign ActionView::Template::Handlers::ERB.find_offset to handle edge cases
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation / Background
The code for finding offsets in the ERB templates outright fails in some cases, causing 500s in the error template. In other cases it will bail out early when it's possible to find and highlight correctly.
Detail
This PR fixes many of the issues that regularly occur and correctly highlights more often. Also adds test coverage of the translate_location method on ActionView::Template, many of which failed before this fix.
Additional information
Problems in
better-htmlgem originally prompted me to dig into this, but this solves many rails-only highlight failures or crashes depending on the template. For example, the following template will cause the request to loop forever in current rails:Any template where the compiled code doesn't contain the template code will also loop forever (for example a bug in the template compiler or something changing spacing).
better-htmlalso reliably causes the error page to 500 because it has duplicate copies of the template code in the compiled template. The only way to fix this in better-html would be to fully patch this method to make it correct, which causes its own problems. The solution I suggest here supports better-html without doing anything explicit to do so. It merely supports the case where a string could occur multiple times in the compiled output which happens in vanilla rails, but less often than inbetter-htmlFixes: Shopify/better-html#121
Probably Fixes: #53388 (I found the same infinite loop and have added test coverage for most of these cases)
CC: @byroot and @jhawthorn because you have both been looking at related issues. FYI @tenderlove, it looks like you wrote the previous implementation.
Checklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]