Use erubi instead of erubis, upgrade haml/haml_lint

[jrafanie]

Fixes deprecation:

DEPRECATION WARNING: ActionView::Template::Handlers::Erubis is
deprecated and will be removed from Rails 5.2. Switch to
ActionView::Template::Handlers::ERB::Erubi instead. (called from <top
(required)> at
/Users/joerafaniello/Code/manageiq/config/application.rb:31)

Rails changed their default templating engine from erubis to erubi and
started deprecating it in 5.1:
rails/rails#27757

Even though we're only using erubis in WinRM, which is outside of
actionview, we're getting the deprecation because:

1. We require hamlit in our Gemfile
2. hamlit tries to require haml in the template here
3. It loads the haml railtie
4. Which loads the erubis template:
   https://github.com/haml/haml/blob/4.0.7/lib/haml/railtie.rb#L21
5. Which autoloads the deprecated constant:
   https://github.com/haml/haml/blob/4.0.7/lib/haml/helpers/safe_erubis_template.rb#L3

Haml 5.0.0 fixes this loading of the deprecated constant:
haml/haml#895

[jrafanie]

Extracted from #18076

[jrafanie]

@himdel do you know how to verify this on master? It's needed for rails 5.1 but I'd like to get this in since it should be compatible with 5.0.

[miq-bot]

Checked commit jrafanie@2c50725 with ruby 2.3.3, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 1 offense detected

Gemfile

• ❗ - Line 47, Col 1 - Bundler/OrderedGems - Gems should be sorted in an alphabetical order within their section of the Gemfile. Gem haml should appear before hamlit.

[bdunne]

@jrafanie Do these dependencies belong in manageiq-ui-classic?

[jrafanie]

@jrafanie Do these dependencies belong in manageiq-ui-classic?

Honestly, I don't remember why haml-lint/hamlit were dependencies here. Maybe @himdel @mzazrivec @skateman know? Maybe we had some rake task here that was moved to ui-classic and those dependencies can now move? 🙏

Note, at one time, we were choosing to use hamlit over haml, it's unclear why it's loading haml from hamlit unless hamlit just replaces/wraps some parts of haml but still depends on it. 🤷‍♂

#3690
#5646
#9167

[skateman]

@jrafanie hamlit used to be faster than haml, I think it's okay to move the stuff to the ui-classic repo.

[mzazrivec]

hamlit used to be faster than haml

It used to be faster? As in it's no longer true?

[skateman]

@mzazrivec according to their latest benchmarks, it still is. The parser is the same though.

[mzazrivec]

OK. Anyway, we don't need these 3 gems in core, do we?

[skateman]

@mzazrivec well, I'm not sure about erubi but the other 2 could live in ui-classic.

[himdel]

If this is switching us back to haml, we will have problems.

haml and hamlit are not compatible, they treat %div{:foo => true} and %div{:foo => false} differently.

[jrafanie]

If this is switching us back to haml, we will have problems.

It's been a few months since I worked on this. Let me double check my logic in the description. I don't see how haml was being required in the first place. I don't know how much of haml was being used and where but it was definitely loading the haml railtie which led to the deprecation warning. Maybe only in test or dev mode? Let me clarify and update this.

[jrafanie]

So, the problem is hamlit tries to require haml in the template here and because we have haml in our dependency tree due to haml_lint (for dev), it will be able to require it. Because this causes the rails 5.1 deprecated constant to be loaded, we get this deprecation warning.

Since haml_lint doesn't have a version that forces haml >= 5.0 (where this is fixed), we have to either:

• add haml it as a direct dependency so we can say >= 5.0 (like this PR)
• drop haml_lint entirely (so haml doesn't get in the dependency tree)
• close this PR and ignore/live with this warning whenever you run tests or rails server/console/etc.

@mzazrivec @skateman @himdel I was trying to get rid of this warning before rails 5.1 lands in master and everyone sees this and wastes time looking at it. My point is even if we're not using haml in dev, it is being required... so we should verify the assertion that we are indeed using hamlit and not haml.

Let me know what you'd like to do.

[skateman]

I'm okay with getting rid of all the linters from the repo, we can just move them to our Gemfile.dev.rb and problem solved. I don't think anyone else uses haml-lint except of me on a daily basis

[jrafanie]

I'm okay with getting rid of all the linters from the repo, we can just move them to our Gemfile.dev.rb and problem solved. I don't think anyone else uses haml-lint except of me on a daily basis

This is the correct answer 😆 PLEASE!

[himdel]

Sorry for the lag...

@himdel do you know how to verify this on master?

Yup, the difference between haml and hamlit is essentially this:

$ haml -s
%div{:foo => true, :bar => false, :quux => '', :baz => nil}
  Whatever
^D
<div foo quux=''>
Whatever
</div>

vs

$ hamlit compile -
%div{:foo => true, :bar => false, :quux => '', :baz => nil}
  Whatever
^D
_buf = []; _buf << ("<div bar='false' baz='' foo='true' quux=''>\nWhatever\n</div>\n".freeze); 
; _buf = _buf.join("".freeze)

.. so,

-haml
+hamlit
-<div foo quux=''>
+<div bar='false' baz='' foo='true' quux=''>

Adding such a thing in app/views/dashboard/show.html.haml gives me the latter
=> confirming this PR still uses hamlit 👍

[himdel]

Do these dependencies belong in manageiq-ui-classic?

Heh heh, that's the reason issue #1 is still open :).
ManageIQ/manageiq-ui-classic#1 (comment)

Theoretically we should not be needing haml or hamlit in core.

(And I agree with @skateman that we don't really use the linters in a task or on travis, so they can go.)

[himdel]

So... idk.. I'm fine with this as is,
but if dropping haml-lint achieves the same.. even better :) (I think :))

[jrafanie]

Ok, @skateman @himdel I'll drop the haml_lint in a followup PR. It doesn't look like hamlit uses erubis/erubi so we should be good to go. Thanks for helping me through the background on this one!

[jrafanie]

Ok, I opened #18886 after confirming that dropping just haml_lint fixes this warning. Thanks all!