redcloth
redcloth copied to clipboard
jgarber
Stop extending into ERB the `t` and `textilize`
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
This should work as hotfix:
require "redcloth"
class ERB::Util
alias t translate
module_function :t
end
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?
@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.
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.
@joshuasiler great, thanks for the suggestion! I will work on the patch very soon to deprecate it via flag.
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 Allowing the user to deprecate via a flag would effectively make the feature "opt-in"
How would you opt-in: https://github.com/jgarber/redcloth/pull/19/files#diff-69f5e2690debe0fabf6b62eb702d1852L40?
@joshuasiler and this code as well: https://github.com/jgarber/redcloth/pull/19/files#diff-d5bc76d7c8479896071db64fe1e3e9a0L22 ?
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.
@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).
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...
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
Also it looks like tests are failing - can you make sure they show green before we are ready to merge?
