Skip to content

Commit

Permalink
No need to escape URI encoded result as URI encodes <, >, and double …
Browse files Browse the repository at this point in the history
…quotes
  • Loading branch information
José Valim committed Jul 11, 2019
1 parent bfe2587 commit 55ffb45
Show file tree
Hide file tree
Showing 5 changed files with 18 additions and 29 deletions.
22 changes: 11 additions & 11 deletions lib/ex_doc/formatter/html/autolink.ex
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
defmodule ExDoc.Formatter.HTML.Autolink do
@moduledoc false
import ExDoc.Formatter.HTML.Templates, only: [h: 1, enc_h: 1]
import ExDoc.Formatter.HTML.Templates, only: [h: 1, enc: 1]

@type language :: :elixir | :erlang | :markdown
@type kind :: :function | :module | :mix_task
Expand Down Expand Up @@ -251,7 +251,7 @@ defmodule ExDoc.Formatter.HTML.Autolink do
put_placeholder(form, url, placeholders)

{name, arity} in typespecs ->
n = URI.encode("#{name}")
n = enc("#{name}")
url = "#t:#{n}/#{arity}"
put_placeholder(form, url, placeholders)

Expand All @@ -278,14 +278,14 @@ defmodule ExDoc.Formatter.HTML.Autolink do
end

defp type_remote_url(@erlang_docs = source, module, name, _args) do
module = enc_h("#{module}")
name = URI.encode("#{name}")
module = enc("#{module}")
name = enc("#{name}")
"#{source}#{module}.html#type-#{name}"
end

defp type_remote_url(source, alias, name, args) do
name = URI.encode("#{name}")
"#{source}#{enc_h(inspect(alias))}.html#t:#{name}/#{length(args)}"
name = enc("#{name}")
"#{source}#{enc(inspect(alias))}.html#t:#{name}/#{length(args)}"
end

defp typespec_string_to_link(string, url) do
Expand Down Expand Up @@ -438,10 +438,10 @@ defmodule ExDoc.Formatter.HTML.Autolink do

cond do
match in locals ->
"[#{text}](##{prefix}#{URI.encode(function)}/#{arity})"
"[#{text}](##{prefix}#{enc(function)}/#{arity})"

match in docs_refs ->
"[#{text}](#{module}#{extension}##{prefix}#{URI.encode(function)}/#{arity})"
"[#{text}](#{module}#{extension}##{prefix}#{enc(function)}/#{arity})"

match in @basic_type_strings ->
"[#{text}](#{elixir_docs}#{@basic_types_page})"
Expand All @@ -450,11 +450,11 @@ defmodule ExDoc.Formatter.HTML.Autolink do
"[#{text}](#{elixir_docs}#{@built_in_types_page})"

match in @kernel_function_strings ->
"[#{text}](#{elixir_docs}Kernel#{extension}##{prefix}#{URI.encode(function)}/#{arity})"
"[#{text}](#{elixir_docs}Kernel#{extension}##{prefix}#{enc(function)}/#{arity})"

match in @special_form_strings ->
"[#{text}](#{elixir_docs}Kernel.SpecialForms" <>
"#{extension}##{prefix}#{URI.encode(function)}/#{arity})"
"#{extension}##{prefix}#{enc(function)}/#{arity})"

module in modules_refs ->
if module_id not in skip_warnings_on and id not in skip_warnings_on do
Expand All @@ -468,7 +468,7 @@ defmodule ExDoc.Formatter.HTML.Autolink do
all

doc = module_docs(:elixir, module, lib_dirs) ->
"[#{text}](#{doc}#{module}.html##{prefix}#{URI.encode(function)}/#{arity})"
"[#{text}](#{doc}#{module}.html##{prefix}#{enc(function)}/#{arity})"

true ->
all
Expand Down
10 changes: 1 addition & 9 deletions lib/ex_doc/formatter/html/templates.ex
Original file line number Diff line number Diff line change
Expand Up @@ -87,15 +87,7 @@ defmodule ExDoc.Formatter.HTML.Templates do
end

@doc false
def enc_h(binary) do
binary
|> URI.encode()
|> h()
end

defp is_enc?(binary) do

This comment has been minimized.

Copy link
@eksperimental

eksperimental Jul 11, 2019

Contributor

@josevalim while reviewing the code for my last PR, I noticed that is_enc?/1 this is a private function, and it can still be called from a template. Is this desired?

This comment has been minimized.

Copy link
@josevalim

josevalim Jul 11, 2019

Member

Yes, because the template is embedded inside the current module.

This comment has been minimized.

Copy link
@eksperimental

eksperimental Jul 11, 2019

Contributor

I see. But is this a recommended practice? should we warn in case it is not?
Additionally, is it documented somewhere? should we document it?
Thanks.

This comment has been minimized.

Copy link
@josevalim

josevalim Jul 11, 2019

Member

I think there is some confusion. EEx templates are compiled as regular functions inside modules. So all of the rules for regular functions applies to the Elixir code inside templates.

This comment has been minimized.

Copy link
@eksperimental

eksperimental Jul 11, 2019

Contributor

Clear. I will read the docs and see if its not mentioned, state exactly as you are explaining it here.

This comment has been minimized.

Copy link
@cybrox

cybrox Jul 11, 2019

Contributor

Regarding the "warning":

  • If the function was exported but did not have an explicit @doc false, static code analysis like credo would complain.
  • If the function was used by a template in another module and not exported, the compiler would complain.
h(binary) != enc_h(binary)
end
def enc(binary), do: URI.encode(binary)

@doc """
Create a JS object which holds all the items displayed in the sidebar area
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<div class="summary-row">
<div class="summary-signature">
<a href="<%=enc_h module_node.id %>.html"><%=h module_node.title %></a>
<a href="<%=enc module_node.id %>.html"><%=h module_node.title %></a>
<%= if deprecated = module_node.deprecated do %>
<span class="deprecated" title="<%= h(deprecated) %>">deprecated</span>
<% end %>
Expand Down
11 changes: 4 additions & 7 deletions lib/ex_doc/formatter/html/templates/detail_template.eex
Original file line number Diff line number Diff line change
@@ -1,12 +1,9 @@
<section class="detail" id="<%=enc_h HTML.link_id(node.id, node.type) %>">
<%= if is_enc? HTML.link_id(node.id, node.type) do %>
<span id="<%=h HTML.link_id(node.id, node.type) %>"></span>
<% end %>
<section class="detail" id="<%=enc HTML.link_id(node.id, node.type) %>">
<%= for default <- get_defaults(node) do %>
<span id="<%=enc_h HTML.link_id(default, node.type) %>"></span>
<span id="<%=enc HTML.link_id(default, node.type) %>"></span>
<% end %>
<div class="detail-header">
<a href="#<%=enc_h HTML.link_id(node) %>" class="detail-link" title="Link to this <%= pretty_type(node) %>">
<a href="#<%=enc HTML.link_id(node) %>" class="detail-link" title="Link to this <%= pretty_type(node) %>">
<span class="icon-link" aria-hidden="true"></span>
<span class="sr-only">Link to this <%= pretty_type(node) %></span>
</a>
Expand Down Expand Up @@ -34,6 +31,6 @@
</div>
<% end %>
<section class="docstring">
<%= node.rendered_doc |> link_detail_headings(enc_h(HTML.link_id(node))) %>
<%= node.rendered_doc |> link_detail_headings(enc(HTML.link_id(node))) %>
</section>
</section>
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<div class="summary-row">
<div class="summary-signature">
<a href="#<%=enc_h HTML.link_id(node) %>"><%=h node.signature %></a>
<a href="#<%=enc HTML.link_id(node) %>"><%=h node.signature %></a>
<%= if deprecated = node.deprecated do %>
<span class="deprecated" title="<%= h(deprecated) %>">deprecated</span>
<% end %>
Expand Down

0 comments on commit 55ffb45

Please sign in to comment.