-
Notifications
You must be signed in to change notification settings - Fork 57
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
Separate interpolation from verbatim text #130
Changes from 5 commits
829df5b
3818de1
f7d8ca4
924a9af
8ba02d7
1e381ba
7fd6482
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,8 +6,7 @@ defmodule Slime.Parser.EmbeddedEngine do | |
@type parser_tag :: binary | {:eex | binary, Keyword.t} | ||
@callback render(binary, Keyword.t) :: parser_tag | ||
|
||
@embedded_engine_regex ~r/^(?<indent>\s*)(?<engine>\w+):$/ | ||
@empty_line_regex ~r/^\s*$/ | ||
import Slime.Compiler, only: [compile: 1] | ||
|
||
@engines %{ | ||
javascript: Slime.Parser.EmbeddedEngine.Javascript, | ||
|
@@ -17,19 +16,9 @@ defmodule Slime.Parser.EmbeddedEngine do | |
} | ||
|> Map.merge(Application.get_env(:slime, :embedded_engines, %{})) | ||
|> Enum.into(%{}, fn ({key, value}) -> {to_string(key), value} end) | ||
@registered_engines Map.keys(@engines) | ||
|
||
def parse(header, lines) do | ||
case Regex.named_captures(@embedded_engine_regex, header) do | ||
%{"engine" => engine, "indent" => indent} when engine in @registered_engines -> | ||
indent = String.length(indent) | ||
{embedded_lines, rest} = split_lines(lines, indent) | ||
{{indent, render_with_engine(engine, embedded_lines)}, rest} | ||
_ -> nil | ||
end | ||
end | ||
|
||
def render_with_engine(engine, lines) when is_list(lines) do | ||
def render_with_engine(engine, line_contents) when is_list(line_contents) do | ||
lines = Enum.map(line_contents, &compile/1) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since engine developer should be aware of interpolation injections into engine body, I think it will be better to pass original list of raw text and interpolations in form of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. example for elixir engine, which now compiles into invalid eex:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! I didn't think about that; it certainly makes sense to decide on how to render interpolation on a per engine basis, as your example shows. I've added it as a test case. On a somewhat unrelated note, this should probably be mentioned in the release notes — it seems to me the Markdown example won't work anymore. |
||
embedded_text = case lines do | ||
[] -> "" | ||
[line | _] -> | ||
|
@@ -52,12 +41,6 @@ defmodule Slime.Parser.EmbeddedEngine do | |
apply(@engines[engine], :render, [embedded_text, [keep_lines: keep_lines]]) | ||
end | ||
|
||
defp split_lines(lines, indent_size) do | ||
Enum.split_while(lines, fn (line) -> | ||
line =~ @empty_line_regex || indent_size < indent(line) | ||
end) | ||
end | ||
|
||
defp indent(line) do | ||
String.length(line) - String.length(String.lstrip(line)) | ||
end | ||
|
@@ -73,11 +56,8 @@ defmodule Slime.Parser.EmbeddedEngine.Javascript do | |
""" | ||
|
||
@behaviour Slime.Parser.EmbeddedEngine | ||
import Slime.Parser, only: [parse_eex_string: 1] | ||
|
||
def render(text, _options) do | ||
{"script", children: [parse_eex_string(text)]} | ||
end | ||
def render(text, _options), do: {"script", children: [text]} | ||
end | ||
|
||
defmodule Slime.Parser.EmbeddedEngine.Css do | ||
|
@@ -86,10 +66,9 @@ defmodule Slime.Parser.EmbeddedEngine.Css do | |
""" | ||
|
||
@behaviour Slime.Parser.EmbeddedEngine | ||
import Slime.Parser, only: [parse_eex_string: 1] | ||
|
||
def render(text, _options) do | ||
{"style", attributes: [type: "text/css"], children: [parse_eex_string(text)]} | ||
{"style", attributes: [type: "text/css"], children: [text]} | ||
end | ||
end | ||
|
||
|
@@ -121,7 +100,5 @@ defmodule Slime.Parser.EmbeddedEngine.EEx do | |
|
||
@behaviour Slime.Parser.EmbeddedEngine | ||
|
||
def render(text, _options) do | ||
text | ||
end | ||
def render(text, _options), do: text | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,9 +25,6 @@ defmodule Slime.Parser.Transform do | |
"#" => %{attr: "id"} | ||
}) | ||
|
||
# TODO: separate dynamic elixir blocks by parser | ||
@quote_outside_interpolation_regex ~r/(^|\G)(?:\\.|[^#]|#(?!\{)|(?<pn>#\{(?:[^"}]++|"(?:\\.|[^"#]|#(?!\{)|(?&pn))*")*\}))*?\K"/u | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. RIP |
||
|
||
@type ast :: term | ||
@type index :: {{:line, non_neg_integer}, {:column, non_neg_integer}} | ||
|
||
|
@@ -99,6 +96,13 @@ defmodule Slime.Parser.Transform do | |
content: TextBlock.render_without_indentation(text)} | ||
end | ||
|
||
def transform(:text_item, input, _index) do | ||
case input do | ||
{:dynamic, [_, expression, _]} -> {:eex, to_string(expression)} | ||
{:static, text} -> to_string(text) | ||
end | ||
end | ||
|
||
def transform(:html_comment, input, _index) do | ||
indent = indent_size(input[:indent]) | ||
decl_indent = indent + String.length(input[:type]) | ||
|
@@ -138,20 +142,12 @@ defmodule Slime.Parser.Transform do | |
end | ||
end | ||
|
||
def transform(:text_block_line, input, _index) do | ||
[space, line] = input | ||
indent = indent_size(space) | ||
case line do | ||
{:simple, content} -> {indent, to_string(content), false} | ||
{:dynamic, content} -> {indent, to_string(content), true} | ||
end | ||
def transform(:text_block_line, [space, content], _index) do | ||
{indent_size(space), content} | ||
end | ||
|
||
def transform(:embedded_engine, [engine, _, lines], _index) do | ||
lines = case lines do | ||
{:empty, _} -> "" | ||
_ -> List.flatten(lines[:lines]) | ||
end | ||
def transform(:embedded_engine, [engine, _, content], _index) do | ||
lines = content[:lines] | ||
case EmbeddedEngine.render_with_engine(engine, lines) do | ||
{tag, content} -> %HTMLNode{name: tag, | ||
attributes: (content[:attributes] || []), | ||
|
@@ -166,12 +162,8 @@ defmodule Slime.Parser.Transform do | |
[line | lines] | ||
end | ||
|
||
def transform(:embedded_engine_line, input, _index) do | ||
to_string(input) | ||
end | ||
|
||
def transform(:inline_html, [_, content, children], _index) do | ||
%InlineHTMLNode{content: [content], children: children} | ||
%InlineHTMLNode{content: content, children: children} | ||
end | ||
|
||
def transform(:code, input, _index) do | ||
|
@@ -202,14 +194,6 @@ defmodule Slime.Parser.Transform do | |
def transform(:code_line, input, _index), do: to_string(input) | ||
def transform(:code_line_with_break, input, _index), do: to_string(input) | ||
|
||
def transform(:text_content, input, _index) do | ||
case input do | ||
{:dynamic, content} -> | ||
%EExNode{content: content |> to_string |> wrap_in_quotes, output: true} | ||
{:simple, content} -> content | ||
end | ||
end | ||
|
||
def transform(:dynamic_content, input, _index) do | ||
content = input |> Enum.at(3) |> to_string | ||
%EExNode{content: content, output: true} | ||
|
@@ -275,30 +259,18 @@ defmodule Slime.Parser.Transform do | |
end | ||
end | ||
|
||
def transform(:text, input, _index), do: to_string(input) | ||
def transform(:tag_name, input, _index), do: to_string(input) | ||
def transform(:attribute_name, input, _index), do: to_string(input) | ||
def transform(:crlf, input, _index), do: to_string(input) | ||
def transform(_symdol, input, _index), do: input | ||
|
||
def remove_empty_lines(lines) do | ||
Enum.filter(lines, fn | ||
({0, ""}) -> false | ||
(_) -> true | ||
end) | ||
end | ||
|
||
def expand_tag_shortcut(tag) do | ||
case Map.fetch(@shortcut, tag) do | ||
:error -> {tag, []} | ||
{:ok, spec} -> expand_shortcut(spec, tag) | ||
end | ||
end | ||
|
||
def wrap_in_quotes(content) do | ||
~s("#{String.replace(content, @quote_outside_interpolation_regex, ~S(\\"))}") | ||
end | ||
|
||
defp expand_attr_shortcut(type, value) do | ||
spec = Map.fetch!(@shortcut, type) | ||
expand_shortcut(spec, value) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,7 @@ tag_item <- space? (embedded_engine / inline_html / code / slime_tag); | |
tags <- (tag crlf*)+; | ||
nested_tags <- crlf+ indent tags dedent; | ||
|
||
inline_html <- &'<' text_content nested_tags?; | ||
inline_html <- &'<' text_item+ nested_tags?; | ||
|
||
slime_tag <- tag_shortcut tag_spaces space? tag_attributes_and_content; | ||
|
||
|
@@ -24,8 +24,6 @@ inline_text <- !eol text_block; | |
|
||
dynamic_content <- '=' '='? space? (!eol .)+; | ||
|
||
text_content <- dynamic:text_with_interpolation / simple:text; | ||
|
||
code <- output:('=' '='? tag_spaces? / '-') space? | ||
code:code_lines children:nested_tags? optional_else:code_else_condition?; | ||
code_else_condition <- crlf* space? '-' space? 'else' children:nested_tags?; | ||
|
@@ -68,16 +66,14 @@ string_with_interpolation <- '"' (interpolation / '\\' . / !'"' .)* '"'; | |
|
||
attribute_code <- (parentheses / brackets / braces / !(space / eol / ')' / ']' / '}') .)+; | ||
|
||
text_with_interpolation <- (text? interpolation)+ (!eol .)*; | ||
|
||
text <- ('\\' . / !('#{' / eol) .)*; | ||
text_item <- static:text / dynamic:interpolation; | ||
text <- ('\\' . / !('#{' / eol) .)+; | ||
interpolation <- '#{' (string / string_with_interpolation / !'}' .)* '}'; | ||
|
||
parentheses <- '(' (parentheses / !')' .)* ')'; | ||
brackets <- '[' (brackets / !']' .)* ']'; | ||
braces <- '{' (braces / !'}' .)* '}'; | ||
|
||
interpolation <- '#{' (string / string_with_interpolation / !'}' .)* '}'; | ||
|
||
comment <- html_comment / space? code_comment; | ||
|
||
html_comment <- indent:space? type:'/!' content:text_block; | ||
|
@@ -91,13 +87,10 @@ text_block <- text_block_line (crlf | |
text_block_nested_lines <- text_block_line (crlf ( | ||
indent lines:text_block_nested_lines dedent / lines:text_block_nested_lines | ||
))*; | ||
text_block_line <- space? (dynamic:text_with_interpolation / simple:text); | ||
text_block_line <- space? text_item*; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should it be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
embedded_engine <- tag_name ':' ( | ||
crlf indent lines:embedded_engine_lines dedent / empty:('' &eol) | ||
); | ||
embedded_engine_lines <- embedded_engine_line (crlf embedded_engine_lines)*; | ||
embedded_engine_line <- text_with_interpolation / text / ''; | ||
embedded_engine <- tag_name ':' (crlf indent lines:embedded_engine_lines dedent); | ||
embedded_engine_lines <- text_item* (crlf text_item*)*; | ||
|
||
tag_name <- [a-zA-Z0-9_-]+; | ||
attribute_name <- [a-zA-Z0-9_@:-]+; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
defmodule Integration.PhoenixTest do | ||
use ExUnit.Case | ||
|
||
test "inline tags" do | ||
rendered = | ||
~S(p: a data-click="#{action}" Click me) | ||
|> phoenix_html_render(action: "clicked") | ||
|
||
assert rendered == ~S(<p><a data-click="clicked">Click me</a></p>) | ||
end | ||
|
||
test "inline html with interpolation" do | ||
rendered = ~S""" | ||
p | ||
<a data-click="#{action}">Click me</a> | ||
""" |> phoenix_html_render(action: "clicked") | ||
|
||
assert rendered == ~S(<p><a data-click="clicked">Click me</a></p>) | ||
end | ||
|
||
test "verbatim text with inline html and interpolation" do | ||
rendered = ~S""" | ||
| Hey, | ||
<a data-click="#{action}">Click me</a>! | ||
""" |> phoenix_html_render(action: "clicked") | ||
|
||
assert rendered == ~s(Hey,\n <a data-click="clicked">Click me</a>!) | ||
end | ||
|
||
test "embedded engines with interpolation" do | ||
rendered = ~S""" | ||
javascript: | ||
document | ||
.querySelector("body") | ||
.insertAdjacentHTML('beforeend', '<p>#{dynamic_text}</p>'); | ||
""" |> phoenix_html_render(dynamic_text: "Dynamic text!") | ||
|
||
assert rendered == """ | ||
<script>document | ||
.querySelector("body") | ||
.insertAdjacentHTML('beforeend', '<p>Dynamic text!</p>');</script> | ||
""" |> String.trim_trailing("\n") | ||
end | ||
|
||
defp phoenix_html_render(slime, bindings) do | ||
slime | ||
|> Slime.Renderer.precompile | ||
|> EEx.eval_string(bindings, engine: Phoenix.HTML.Engine) | ||
|> Phoenix.HTML.safe_to_string | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to keep this check, it will be easier to add descriptive error in case of invalid engine name