Decouple ActionView and Rails HTML Sanitizer to Allow Other Frameworks to Use ActionView

[tongueroo]

Motivation / Background

This Pull Request has been created because it decouples ActionView and Rails::HTML::Sanitizer. This allows other frameworks like Jets to use ActionView for rendering.

Detail

This Pull Request changes the actionview sanitize_helper.rb to remove

• rails/actionview/lib/action_view/helpers/sanitize_helper.rb

  Line 3 in 53438e9

  require "rails-html-sanitizer"

• rails/actionview/lib/action_view/helpers/sanitize_helper.rb

  Line 12 in 53438e9

  mattr_accessor :sanitizer_vendor, default: Rails::HTML4::Sanitizer

I've found that these are only spot in actionview that refers to Rails.

Instead, this PR sets the default config.action_view.sanitizer_vendor is set in the actionview railtie. This decouples actionview from Rails. It makes ActionView easier to use with other frameworks.

Thanks for considering this PR.

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
• Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
• Tests are added or updated if you fix a bug or add a feature.
• CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

[zzak]

This change breaks a lot of tests, so I think we cannot change it so easily.

[rafaelfranca]

I'm confused. How does this couple Action View to Rails? Rails is just an empty namespace. Isn't the full framework.

[rafaelfranca]

There is no reason to frameworks like Jets to not be able to use Action View as is. For sure this change doesn't make anything easier. The Rails namespace is empty and that specific reference to Rails doesn't couple them. We know that because the Action View tests run without the railtie being loaded.

If that is not the case let us know, but I don't think there is a need to implement this in order to Jets to use Action View.

[tongueroo]

All good if it's not merge, I understand. Working around it anyway.

[rafaelfranca]

I'm really curious where is the coupling, because it should not have any without this PR. Can you please explain what do you need to workaround? The Rails mention here that you are "removing" is just a name. There isn't any code. It could be Foo::Html::Sanitizers and the behavior would be the same. I'm not trying to reject the idea of you being able to use Action View, I'm just pointing that what you changed isn't coupling to anything other the the sanitizer itself.

Are you trying to use action view without the sanitizer? Because it is possible to use without Rails already and the sanitizer require doesn't load anything related to Rails.

[rafaelfranca]

I found rubyonjets/jets@faa5ce1. Can you explain what is the problem with the Rails constant being defined? We can totally rename the gem if that is the problem, but you would have a huge problem if Rails can't be defined because there are a lot of gems in the ecosystem that defines it.

[rafaelfranca]

Is the problem that there are code that does require 'foo/engine' if defined?(Rails)?

[tongueroo]

Wow. You figured it out before I was able to explain it. 🎉 Yup, that's it.

Found that once the Rails namespace is defined, even if it's empty, it makes it not possible for Jets to use other gems that follow a pattern like that:

some_gem/railtie.rb

require "some_gem/railtie" if defined?(Rails)

Once ActionView is loaded: Boom! The Rails constant is defined because the require "rails-html-sanitizer"

To allow Jets to use other gems, Jets makes sure that the Rails constant is not defined. There's even a spec for it. I could had explained it better initially.

The workaround: Jets intercepts Kernel.require and require "jets-html-sanitzer" instead. Not very proud of it. Please don't judge 🤣

Again, no sweat either way 👌

[rafaelfranca]

lol, not judging. I have seeing gems doing this but the right way that I found to solve this problem was to change those gems to check for the right thing:

require "some_gem/railtie" if defined?(Rails::Railtie)

Now, I totally get isn't easy to do this. I think we can rename the rails-html-sanitizer to be actionview-html-sanitizer. Or at least not define the rails constant.

I'll do it for next release.

But like I said, there are a few other gems that define that same module:

https://github.com/rails/rails-controller-testing/blob/master/lib/rails/controller/testing.rb#L6

Not sure if it will be feasible to really avoid the Rails constant to be defined given a gem could define it and break Jest in some strange way.

But of course, having action view already defining it is something we can fix by rename the gem.

[tongueroo]

That would be amazing! Would be able to get rid of the Kernel require hack 🙌

Here are related PRs for consideration to help with the rename:

• use actionview-html-sanitizer #49714
• Rename to actionview html sanitizer rails-html-sanitizer#171

For the curious, here's a comment I forgot about from a while back rubyonjets/jets#251 (comment) Lol. Totally get that there will be issues with gems still. It just gives Jets a chance to work with more of them. 👍