Stop extending into ERB the `t` and `textilize` #19

dmitry · 2015-01-25T18:21:00Z

Prevent injection of the t and textilize methods into a ERB module through the Util module. When using the i18n gem in Rails it uses textilize method, instead of translate (aliased as t) helper. It will be still possible to inject textilize into a ERB after this commit with a manual code:

begin
  require 'erb'
  require 'redcloth/erb_extension'
  include ERB::Util
rescue LoadError
end

timoschilling · 2015-01-31T02:03:58Z

removing:

    alias t textilize
    module_function :t

should be enough

timoschilling · 2015-01-31T02:07:26Z

This should work as hotfix:

require "redcloth"
class ERB::Util
  alias t translate
  module_function :t
end

dmitry · 2015-04-07T12:23:34Z

@jgarber can you please look into the PR? Thank you!

dmitry · 2016-04-30T10:25:38Z

@joshuasiler please merge this one.

joshuasiler · 2016-04-30T14:07:05Z

I am happy to take a look, however I'm not 100% clear on what problem this is solving. Can you elaborate further? Won't this break expected functionality for some users?

dmitry · 2016-05-03T12:38:38Z

@joshuasiler any specific solution?

I think most of the users instead of t they are using textile, as normally t is reserved for the translations. We could do minor release, and tell the users in the gem after install message that they have to update their code from t to textile, in other case it will break their ERB views.

dmitry · 2016-05-03T12:39:33Z

Including include ERB::Util in the main thread not really good idea...

joshuasiler · 2016-05-03T17:12:06Z

This makes sense to me, however I'm reluctant to suddenly change existing behavior on users and force a refactor, just based on my bad experiences with other libraries doing so and causing extra work. What about a version of this patch that adds a config flag? The config flag would default to current behavior, but display a deprecation/recommendation message to users along with the suggested setting. We could then remove it completely in a later major release.

dmitry · 2016-05-04T08:01:37Z

@joshuasiler great, thanks for the suggestion! I will work on the patch very soon to deprecate it via flag.

nruth · 2016-05-06T14:14:25Z

This does look like it should be opt-in instead of default.
A major version change should stop anyone (familiar with semver) being surprised about the API changing, and there could be docs showing how to transition / new usage.

dmitry · 2016-11-10T16:38:23Z

@nruth any idea how it could be opt-in?

joshuasiler · 2016-11-29T15:59:56Z

@dmitry Allowing the user to deprecate via a flag would effectively make the feature "opt-in"

dmitry · 2016-11-29T17:17:48Z

How would you opt-in: https://github.com/jgarber/redcloth/pull/19/files#diff-69f5e2690debe0fabf6b62eb702d1852L40?

dmitry · 2016-11-29T17:18:12Z

@joshuasiler and this code as well: https://github.com/jgarber/redcloth/pull/19/files#diff-d5bc76d7c8479896071db64fe1e3e9a0L22 ?

joshuasiler · 2016-11-29T17:57:34Z

We have RbConfig in the stack - you could use that to define a config variable, and then check for that variable and only include your mod if it's turned on.

dmitry · 2016-11-29T22:08:52Z

@joshuasiler where it should be configured in the application? In config.ru before environment/application requirement? This changes are made when the gem loads, normally with Bundle.require method (at least in rails application.rb https://github.com/bundler/bundler/blob/master/lib/bundler.rb#L108).

joshuasiler · 2017-01-23T23:42:19Z

Good question, and no easy answers. You're right it has to be set fairly early. Adding an optional config file would be one option. Or you could have an optional function to call like RedCloth.disable_erb_extensions() that applies the hotfix above...

rafaelfranca · 2017-06-14T21:28:14Z

lib/redcloth.rb

-begin
-  require 'erb'
-  require 'redcloth/erb_extension'
-  include ERB::Util


This like also includes ERB::Util in the Object class. Even that t method is not removed I think this line should be removed.

Isn't it removed in the commit?

dmitry · 2017-06-15T15:24:26Z

Can we proceed on this issue? I would like to push changes to the master.

kmcphillips

Adding methods to Object is pretty not great and is causing us problems with internationalization too:

Object.new.method(:t)
=> #<Method: Object(ERB::Util)#t(textilize)>

kmcphillips · 2017-06-15T19:10:32Z

spec/erb_spec.rb


 describe "ERB helper" do
  it "should add a textile tag to ERB" do
-    template = %{<%=t "This new ERB tag makes is so _easy_ to use *RedCloth*" %>}
+    template = %{<%= textile "This new ERB tag makes is so _easy_ to use *RedCloth*" %>}


This should be textilize.

I will change that now.

kmcphillips · 2017-06-15T19:11:46Z

spec/erb_spec.rb

@@ -1,10 +1,11 @@
 require File.dirname(__FILE__) + '/spec_helper'
+require 'redcloth/erb_extension'


Requiring this here requires it globally. Not just for this test. Better would be to create a test class and include it in there.

Any idea how to do that? To preserve the same functionality (auto-inclustion to the ERB helpers), I believe it's impossible to do. This could be changed to use another module, include it into a ERB::Utils and then remove module later when test unit is completed. Let me know your thoughts.

joshuasiler · 2017-06-15T23:35:23Z

This looks like a reasonable change, I'm open to merging. As a final step can you submit an update to the readme that explains this change? I'll release as 4.4

Prevent injection of the `t` and `textilize` methods into a `ERB` module through the `Util` module. When using the `i18n` gem in Rails it uses textilize method, instead of `translate` (aliased as `t`) helper. It will be still possible to inject textilize into a ERB after this commit with a manual code: ``` begin require 'erb' require 'redcloth/erb_extension' include ERB::Util rescue LoadError end ```

joshuasiler · 2017-06-16T19:47:09Z

Also it looks like tests are failing - can you make sure they show green before we are ready to merge?

kmcphillips · 2017-07-18T20:47:18Z

@dmitry Are you able to complete this?

GregThoma · 2024-06-01T11:55:01Z

Attention: BREAKING CHANGE RedCloth 5.x series

dmitry mentioned this pull request Jan 31, 2015

Labels suddenly stopped working on login screen activeadmin/activeadmin#3777

Closed

rafaelfranca reviewed Jun 14, 2017

View reviewed changes

kmcphillips reviewed Jun 15, 2017

View reviewed changes

dmitry added 8 commits June 16, 2017 18:31

Remove t alias from ERB, can conflict with I18n

50049da

Change t to textile in spec

cdd02b0

Require erb extension in spec for textile helper

a12a118

Remove t alias from ERB, can conflict with I18n

f58e1a1

Fix to textilize

a25fa78

Remove t alias from ERB, can conflict with I18n

a262b5b

Change t to textile in spec

7c47155

dmitry force-pushed the patch-1 branch from e3cfa9c to 7c47155 Compare June 16, 2017 17:34

try to fix

0b4bcc6

heliocola mentioned this pull request Jan 24, 2024

Feature: Automatically add an ID to headings (for dynamic TOC/ anchors) #85

Open

heliocola mentioned this pull request Apr 18, 2024

Move the t alias to its own extension file #90

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop extending into ERB the `t` and `textilize` #19

Stop extending into ERB the `t` and `textilize` #19

dmitry commented Jan 25, 2015

timoschilling commented Jan 31, 2015

timoschilling commented Jan 31, 2015

dmitry commented Apr 7, 2015

dmitry commented Apr 30, 2016 •

edited

Loading

joshuasiler commented Apr 30, 2016

dmitry commented May 3, 2016

dmitry commented May 3, 2016

joshuasiler commented May 3, 2016 •

edited

Loading

dmitry commented May 4, 2016

nruth commented May 6, 2016

dmitry commented Nov 10, 2016

joshuasiler commented Nov 29, 2016

dmitry commented Nov 29, 2016

dmitry commented Nov 29, 2016

joshuasiler commented Nov 29, 2016

dmitry commented Nov 29, 2016

joshuasiler commented Jan 23, 2017

rafaelfranca Jun 14, 2017

dmitry Jun 15, 2017

dmitry commented Jun 15, 2017

kmcphillips left a comment

kmcphillips Jun 15, 2017

dmitry Jun 15, 2017

kmcphillips Jun 15, 2017

dmitry Jun 15, 2017

joshuasiler commented Jun 15, 2017

joshuasiler commented Jun 16, 2017

kmcphillips commented Jul 18, 2017

GregThoma commented Jun 1, 2024

		@@ -1,10 +1,11 @@
		require File.dirname(__FILE__) + '/spec_helper'
		require 'redcloth/erb_extension'

Stop extending into ERB the t and textilize #19

Are you sure you want to change the base?

Stop extending into ERB the t and textilize #19

Conversation

dmitry commented Jan 25, 2015

timoschilling commented Jan 31, 2015

timoschilling commented Jan 31, 2015

dmitry commented Apr 7, 2015

dmitry commented Apr 30, 2016 • edited Loading

joshuasiler commented Apr 30, 2016

dmitry commented May 3, 2016

dmitry commented May 3, 2016

joshuasiler commented May 3, 2016 • edited Loading

dmitry commented May 4, 2016

nruth commented May 6, 2016

dmitry commented Nov 10, 2016

joshuasiler commented Nov 29, 2016

dmitry commented Nov 29, 2016

dmitry commented Nov 29, 2016

joshuasiler commented Nov 29, 2016

dmitry commented Nov 29, 2016

joshuasiler commented Jan 23, 2017

rafaelfranca Jun 14, 2017

Choose a reason for hiding this comment

dmitry Jun 15, 2017

Choose a reason for hiding this comment

dmitry commented Jun 15, 2017

kmcphillips left a comment

Choose a reason for hiding this comment

kmcphillips Jun 15, 2017

Choose a reason for hiding this comment

dmitry Jun 15, 2017

Choose a reason for hiding this comment

kmcphillips Jun 15, 2017

Choose a reason for hiding this comment

dmitry Jun 15, 2017

Choose a reason for hiding this comment

joshuasiler commented Jun 15, 2017

joshuasiler commented Jun 16, 2017

kmcphillips commented Jul 18, 2017

GregThoma commented Jun 1, 2024

Stop extending into ERB the `t` and `textilize` #19

Stop extending into ERB the `t` and `textilize` #19

dmitry commented Apr 30, 2016 •

edited

Loading

joshuasiler commented May 3, 2016 •

edited

Loading