Port helpers from hanami-helpers, introduce TagHelper

[timriley]

Introduce these helper modules ported from hanami-helpers:

• EscapeHelper
• HTMLHelper (may be removed, see below)
• LinkToHelper
• NumberFormattingHelper

Also introduce a new TagHelper, ported from ActionView::Helpers::TagHelper, with the intention for this to replace HTMLHelper (see below for more).

These helpers are included in hanami-view (as opposed to the hanami gem) because they're of general utility to anyone building views, and we should make them available to anyone using hanami-view standalone. Other helpers that depend on a fully integrated Hanami app will go into the hanami gem via another PR.

These helper modules are not loaded by default. They will be distributed as opt-in extras: they can be required explicitly and included in custom part or scope classes as needed. The spec/integration/helpers_spec.rb integration test demonstrates this approach.

I've sequenced the commits in this PR so that they can be reviewed in order. The first commit copies verbatim the files from @jodosha's original helpers modernisation PR (hanami/helpers#199). From there, you can review the subsequent commits to see the meaningful changes, as opposed to reviewing the +2k new lines as one big blob.

Changes to helpers

The commits will show that I've tweaked the helpers in a few small ways to make them work here. The key changes are:

module_function

Wherever it is practical, I've adjusted every helper module to have its methods declared as module_function. This makes it possible to us the helpers directly (e.g. Hanami::View::Helpers::EscapeHelper.escape_html("...") without mixing them into your class, which would be preferable in certain situations.

NumberFormattingHelper

Remove dry-types dependency and instead use Ruby's Kernel coercion methods instead. These Kernel methods are what dry-types' Coercible::Integer and Coercible::Float types call anyway, so there's no need for the extra layer of code and extra gem dependency.

As a consequence, drop any custom error classes (because we don't need to catch and replace dry-types' errors), and instead just raise ArgumentError or TypeError in a way that is consistent with the underlying Kernel coercion methods.

EscapeHelper

Remove the need for a standalone Escape module by using module_function above all the methods in EscapeHelper.

Rename #escape_url into #sanitize_url, which better describes what it does.

Move the Escape.escape_uri method to being an escape_url helper method (wrapping the call to EscapeUtils.escape_uri).

Remove #escape_html_attribute. I’m not sure what value this brings over just calling #escape_html. It just seems confusing and there’s no equivalent in other Ruby web toolkits or frameworks.

HTMLHelper

I'm including notes of the changes to HTMLHelper for completeness, but in the section below, I propose we replace it with TagHelper.

Firstly, rename from HtmlHelper to HTMLHelper to properly reflect the acronym.

Inside HTMLBuilder, update our @buffer string handling to have it work properly with the frozen Hanami::View::HTML::SafeString instances returned by String#html_safe. So instead of creating a SafeString up-front, just create an ordinary mutable String and only return the SafeString via #to_s, which is called at the end of the HTML building.

TagHelper

This is a new helper module based on ActionView::Helpers::TagHelper.

My intention with this helper is to have it replace HTMLHelper, since TagHelper provides a much more template-friendly usage, wherein template content can be included in blocks. HTMLHelper, on the other hand, cannot be easily used within templates, since it runs any given blocks through instance_eval rather than expecting them to return a string from the template, which we're already establishing as a core expectation for hanami-view and its helpers.

To demonstrate this, here's what you have to do to use HTMLHelper inside a template:

<%= html.div do %>
  <%= html.p do %>
    <%= text("Hello world!") %>
  <% end %>
<% end %>

See how the text helper is needed to append content from within the block? This is not a natural expectation people would make about how to use these helpers from within templates. The inclination would be to do something like this:

<%= html.div do %>
  <%= html.p do %>
    Hello world!
  <% end %>
<% end %>

But this doesn't work at all: "Hello world!" is ignored and will not make it into the rendered template output, which would be just <div><p></p></div>.

However, with the tag helper, usage is more natural. This works as expected:

<%= tag.div do %>
  <%= tag.p do %>
    Hello world!
  <% end %>
<% end %>

Here the template renders <div><p>Hello world!</p></div> as expected.

What I think we've discovered here is an important principle for hanami-view helpers: they should be equally as functional and useful within templates as within plain Ruby environments.

TagHelper meets this principle, but HTMLHelper does not.

In addition, TagHelper comes with a range of other helpful behaviours, such as shortcuts for data: and aria: attributes, and other things like building class attribute strings from arrays as well as hashes of true/false values.

TagHelper is also a simpler, easier to understand helper: there's no instance_eval-style behaviour to worry about, which even tripped me up a number of times while trying to get HTMLHelper to work here. And given it already works with blocks providing content just like HTMLBuilder, the only difference is having to write tag. in front of each tag, which I think is a worthwhile tradeoff: tag is extra typing, yes, but it's both short and descriptive, and it makes it much clearer what is doing the tag generation.

I also ran into troubles with the HTMLBuilder used inside Hanami::Helpers::FormHelper (which I'm preparing in parallel in hanami/hanami#1305).

IIRC, I think the problem (again) came from trying to use the form helper inside templates, e.g.

<%= form_for("/posts") do |f| %>
  <div>
    Title:
    <%= f.text_field "post.title" %>
  </div>
  <div>
    Body:
    <%= f.text_area "post.body" %>
  </div>
<% end %>

In this case the f.text_field call would output itself at the time of that ERB expression, but also at the end of the form_for call, because every method on f would also append extra contents to the buffer from the FormBuilder' own HTMLBuilder instance. So the tags inside the block (the text_field and text_area) would be output twice.

In this case, I sorted it out by avoiding use of the HTMLBuilder entirely, and just shelling out to the top-level html helper instead: hanami/hanami@888d8e8. But either way, I think this illustrates how the HTMLHelper in its current structure is really suited to pure-ruby usage only, not usage within templates. This is potentially a remnant of the situation we had in Hanami 1.x, where we couldn't mix helpers with blocks and template content in the way we can today with hanami-view 2.0.

If you're happy with the switch to TagHelper, I'll make sure to remove HTMLHelper before merging this PR.

Mark strings from template-captured blocks as HTML safe

While preparing these helpers, notably the HTML/Tag helpers that will auto-escape strings returned from a yielded block based on their .html_safe? status, I realised that we needed blocks captured to strings via the templates automatically marked as .html_safe. After all, it's the entire purpose of our templates to generate HTML, so that HTML should be preserved and not escaped.

I've taken care of this in 8072592, via a little specialised buffer class that we use for Tilt's capture_generator across all three of our supported template engines. I'm very happy with how this part turned out!

Compatibility questions

I've a couple of compatibility-related questions at this point:

1. The API docs on these helpers say thing like @since 0.1.0. However, with this changed we've moved them into a new gem and into a new Ruby namespace. Should this change them to @since 2.0.0?
2. With the original #escape_url becoming #sanitize_url, and with Escape.escape_uri becoming #escape_url, this introduces a behavioural change for any existing user depending on the behaviour of the original #escape_url. Are we comfortable with this? Would release/upgrade notes be sufficient to notify users of this change? I do believe the new names are more self-descriptive, but I think we should be wary here. One option is to remove the #escape_url helper entirely (and keep #sanitize_url only). This would result in a NoMethodError for users, which would be an easy prompt to upgrade (or we could preserve #escape_url as an alias for #sanitize_url along with a deprecation message).

[jodosha]

@timriley I can't review ATM (traveling today), but escaping HTML attrs should be preserved as per OWASP recommendations: https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html

[timriley]

HTML attrs should be preserved as per OWASP recommendations

@jodosha — safe travels, and thanks for the quick bit of feedback!

I definitely understand the need to ensure we escape HTML tag attributes. My question here was really about whether we needed a separate method for it when our EscapeHelper#escape_html will probably already do the job?

From looking at the code in your original PR, #escape_html and #escape_attribute had almost completely parallel implementations, which seemed to introduce a lot of complexity, and left me confused as to why we had to have them both.

To help with the discussion here, let me demonstrate all the various ways of escaping HTML attribute that are present either already or as a result of this PR:

require "bundler/setup"
require "hanami/view"
require "hanami/view/erb/template"
require "hanami/view/helpers/escape_helper"
require "hanami/view/helpers/tag_helper"

Scope = Class.new {
  include Hanami::View::Helpers::EscapeHelper
  include Hanami::View::Helpers::TagHelper
}.new

def render(src)
  Hanami::View::ERB::Template.new { src }.render(Scope)
end

# 1. Escaping of manually constructed attributes courtesy of ERB expression tags (similar for Haml/Slim)

puts render(<<~ERB)
  <div class="<%= "<script>" %>"></div>
ERB
# <div class="&lt;script&gt;"></div>

# 2. Escaping via new `tag.attributes` helper

puts render(<<~ERB)
  <div <%= tag.attributes(class: "<script>") %>></div>
ERB
# <div class="&lt;script&gt;"></div>

# 3. Escaping via attribute arguments supplied to `tag` builder

puts render(<<~ERB)
  <%= tag.div(class: "<script>") %>
ERB
# <div class="&lt;script&gt;"></div>

# 4. Escaping via manual use of `escape_html` (i.e. within other helper methods)

module MyHelper
  include Hanami::View::Helpers::EscapeHelper

  def my_helper
    # This is here as a contrived demonstration only. For real usage, you'd be much better off using
    # `tag.div` here.
    %(<div class="#{escape_html("<script>")}"></div>)
  end
end
puts Class.new { include MyHelper }.new.instance_eval { my_helper }
# <div class="&lt;script&gt;"></div>

Given these, is there a reason we would still need a separate #escape_attribute helper?

[jodosha]

@timriley I'm fine to drop the previous implementation of HTML Helper but let's rename it from TagHelper to HtmlHelper, if possible.

---

Regarding HTML escape:

• Adding escape_utils as a runtime dependency made Hanami::Utils::Escape obsolete.
  • Please check if the existing specs in hanami-utils can expand the coverage for this PR, including the HTML attributes escaping.
  • 👉 If the new implementation makes the old HTML attributes escaping specs to pass, then let's remove it.
  • Consider removing the code from hanami-utils.
• hanami-assets will need the HTML builder.
  • In 1.x, hanami-assets used to depend on hanami-helpers to access the implementation
  • In 2.x, hanami-assets must depend on hanami-view. Are we OK with that? I am, just wanted you to know.

[jodosha]

@timriley Can we change this to a subclass of Hanami::View::Error?

[timriley]

@jodosha FWIW, these were unchanged from the original code that I ported from hanami-helpers. So they were ArgumentError originally, and I think that is still the most semantically correct error class to use here — it indicates incorrect/invalid arguments being passed to the method. The error here isn't due to some inner working of Hanami::View being compromised, it's simply a matter of the wrong combination of args for a method.

That said, I do think there's another opportunity to improve things here: we should provide a helpful error explanation along with the error, e.g.

raise ArgumentError, "you must provide link content via an argument or a block, not both"

(The exact message needs some finessing still, but you get the idea, I hope!)

What do you think?

[jodosha]

Suggested change

	array.flatten.map { |i| escape_html(i) }.join(sep).html_safe
	array.flat_map { |i| escape_html(i) }.join(sep).html_safe

[timriley]

I actually tried this! But it broke some specs. I'll get back to you here with what happened.

[timriley]

So here's the tests that fail when we do this:

Test:

expect(tag.input(value: [123, "abc", [789]])).to eq %(<input value="123 abc 789">)

Failure:

  1) Hanami::View::Helpers::TagHelper#tag tag attributes joins arrays of options and converts them to strings
     Failure/Error: expect(tag.input(value: [123, "abc", [789]])).to eq %(<input value="123 abc 789">)

       expected: "<input value=\"123 abc 789\">"
            got: "<input value=\"123 abc [789]\">"

       (compared using ==)
     # ./spec/unit/helpers/tag_helper_spec.rb:133:in `block (4 levels) in <top (required)>'

Test:

expect(escape_join(["<script>", ["<script>"]], " ")).to eq %(&lt;script&gt; &lt;script&gt;)

Failure:

  2) Hanami::View::Helpers::EscapeHelper#escape_join flattens the given array
     Failure/Error: expect(escape_join(["<script>", ["<script>"]], " ")).to eq %(&lt;script&gt; &lt;script&gt;)

       expected: "&lt;script&gt; &lt;script&gt;"
            got: "&lt;script&gt; [&quot;&lt;script&gt;&quot;]"

       (compared using ==)
     # ./spec/unit/helpers/escape_helper_spec.rb:45:in `block (3 levels) in <top (required)>'

So the reason for the implementation is to allow arrays in arrays to be passed to #safe_join, making its behaviour similar to Ruby's own Array#join:

[123, "abc", [789]].join("--")
# => "123--abc--789"

The PR introducing this to Rails' equivalent helper is this one from June 2014: rails/rails#15654. Funnily enough, there was a question about flat_map there too! 😆

Given the parallel to Array#join, I think it'd make sense to leave this implementation as is.

I did just realise we could replace flatten.map with flatten.map! to avoid an extra array allocation, however, since the flatten is already giving us a new array. I'll push that up in another commit in just a moment.

[jodosha]

@timriley Why is it tag and not html?

[timriley]

Well, for starters, it was because we already had #html via HTMLHelper, and I was introducing this alongside it just to get your thoughts :) Additionally, it was already called #tag in the Rails implementation that I imported.

Now that you're OK with the existing HTMLHelper implementation being removed, we have the option to rename it, but I do wonder if tag is a better name here, because:

1. Its job is to generate a single tag string
2. It's one less character to type!
3. (Least important of all, but still a factor) Matching the name from Rails' helpers might help users understand its usage more readily

For these reasons, my personal inclination would to rename TagHelper to HTMLHelper (as you requested), but leave the helper method itself named as #tag.

I don't feel super strongly about this, though, so I'm keen to hear your thoughts given the above. Then we'll settle on the name.

[timriley]

Actually, another reason to leave this named as TagHelper is that it also is aware of and supports building non-HTML tags, like SVG tags, demonstrated by the SVG_SELF_CLOSING_ELEMENTS constant in TagBuilder. And of course it could also be used for building XML or any other kind of SGML-style tags.

I was reminded of this just as I was cleaning things up in this PR.

So at this point I'd be inclined just to leave everything named as is: TagHelper#tag.

Let me know what you think :)

[timriley]

@jodosha:

Adding escape_utils as a runtime dependency made Hanami::Utils::Escape obsolete.

• Please check if the existing specs in hanami-utils can expand the coverage for this PR, including the HTML attributes escaping.
• 👉 If the new implementation makes the old HTML attributes escaping specs to pass, then let's remove it.
• Consider removing the code from hanami-utils.

I can confirm that all the tests from spec/unit/hanami/utils/escape_spec.rb in Hanami-utils are covered here in spec/unit/helpers/escape_helper/escape_html_spec.rb and spec/unit/helpers/escape_helper/sanitize_url_spec.rb.

I've just pushed up 84adad9 which (temporarily) adds comments to our specs to highlight the small number of differences.

I feel comfortable with the differences:

• Most come down to us relying upon Temple::Utils.escape_html_safe instead of doing the manual character-by-character replacement approach that is present in hanami-utils' Escape.html method. I think our approach here is preferable because that makes the escaping consistent across both templates (which also use Temple's escaping) and helpers. The differences seem minor and no less safe.
• At this point we're no longer forcing everything into UTF-8 encoding. Personally, this feels a little heavy-handed to me, and not really necessary for HTML escaping utilities? Releasing 2.0 with some updated helpers might be a good opportunity to jettison that extra complexity?

@jodosha — would you mind taking a look at that commit and my notes above and letting me know what you think?

In the meantime, I'll start a PR to remove Escape from hanami-utils.

[timriley]

In 2.x, hanami-assets must depend on hanami-view. Are we OK with that? I am, just wanted you to know.

Yep, I'm good with this!

If we discover later that this dependency is somehow problematic, we can always adjust things then, but I like the idea of our view-related things living together in our view gem.

[timriley]

@jodosha I've finished with all my polishing here now. This is ready to go pending your final feedback on the couple of open threads above 😄

[timriley]

Just one note regarding EscapeUtils: in 820e997 I actually removed it, and the EscapeHelper#escape_url method that used it.

As far as I can tell, EscapeUtils dependency was actually introduced in hanami/helpers#199, so it was never actually shipped in any released Hanami gem. And the one method that used it in that PR (Hanami::Helpers::Escape.uri) was only used in one place by the HtmlHelper implementation that we have since removed from this PR.

Given this, I didn't think we should keep it.

Users who want this behaviour in their apps can include EscapeUtils themselves, or reach for standard library alternatives like ERB::Util.url_encode.

Importantly, by removing the #escape_url method that briefly existed in this PR, it also means that any Hanami 1.x users upgrading will see a NoMethodError for escape_url (whose 1.x implementation was renamed to #sanitize_url in this PR) instead of finding a more permissive method in its place, which would be dangerous.

[timriley]

@jodosha Addressed those last two open issues, thanks for those. Would you mind weighing in on my remaining comments so we can get to the point of merging this? 😄