This project is archived and is in readonly mode.
[PATCH] Remove unused String#html_safe!
From what I can discern, the String#html_safe!
method is currently not in use and provides no useful functionality
other than raising an exception on both strings and
ActiveSupport::SafeBuffer instances when called. The
method is deprecated in Rails 2.3.6, but hasn't yet been removed in
the master branch. This patch removes the method, which is the only
reference to the html_safe! method in all of
Rails.
-
Rohit Arondekar- Assigned user set to José Valim
Interesting.
I found the reason why html_safe! doesn't actually do anything besides raising an exception:
For performance reasons safe strings are implemented in a way that cannot offer an in-place html_safe! variant.
source: http://edgeguides.rubyonrails.org/active_support_core_extensions.ht...
i.m.h.o I think the method should be removed or at least the exception raised should explain why you cannot call html_safe! on a string.
On that note, +1 to the patch, applies cleanly and tests pass on 1.9.2-head and 1.8.7-p174
-
Neeraj SinghI would say that this code is self documenting and since html_safe is something a lot of people would encounter in rail3, this method should stay.
-
Cody FauserNeeraj,
I disagree.
html_safe!is not a usable method on any object. There is no better way to indicate that you can't call a method than to not implement it and the method is clearly deprecated in Rails 2.3.6. -
Rohit Arondekar
If the method is to be kept I would prefer it if the message is more self explanatory. This will avoid confusion and people filing bug reports.
If anybody agrees with me, I've attached a patch that should explain why the method can't be called. Tested it on 1.8.7-p174 and 1.9.2-head.
-
improve_exception_message.diff
913 Bytes
-
-
Rohit Arondekar
For the record I still agree with Cody though, if the method can't be called it shouldn't exist.
-
José ValimIn Rails 2.3.5, the official API was html_safe! However, we found out that it's actually slow and provided another implementation, so we needed to deprecate the old html_safe! method and stop modifying strings in place.
We preferred to use RuntimeError because we wanted to be clear that the html_safe! existed but we changed its behavior. If we raised NoMethodError, developers would be wondering what happened and if they were actually doing something wrong.
But since it was already deprecated on 2.3.6 and it shows a message on 2.3.8, it can be removed for 2.3.9.
I hope this clarify a bit. I will apply this patch later.
-
José Valim
- Milestone set to 2.3.9
- Assigned user changed from José Valim to Michael Koziarski
Assigning to Koz to give the final word here since html_safe is his baby.
-
Santiago Pastorino- State changed from new to resolved
- Assigned user changed from Michael Koziarski to Santiago Pastorino
This is already out on 2-3-stable since this commit
http://github.com/rails/rails/commit/60e82a3ca2fe6058292db659c34117...
-
Andrea Campi- Tag changed from patch activesupport xss to activesupport, patch, xss
- Importance changed from to Low
bulk tags cleanup