Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sanitize(data) disabiguation #326

Merged
merged 2 commits into from
May 8, 2019

Conversation

epipheus
Copy link

@epipheus epipheus commented Apr 11, 2019

sanitize is such an overused method name that it binds to a lot of different things. Because datatables inherit from AjaxDatatablesRails::ActiveRecord which inherits from AjaxDatatablesRails::Base for some odd reason sanitize binds to ActionView's sanitize method which eventually gets to Loofah and Nokogiri which doesn't handle arrays and throws the error I reported in #325 surprisingly even when you call self.sanitize it does not call the method in the same namespace and file!! because technically the sanitize function that we want is AjaxDatatablesRails::Base's function, to disabiguate we could either rename the function (and because I don't know what else calls it that seemed like a bad idea), or we could help disambiguate by being explicit about which super's method to call. The only way I saw to do this is to call it like this:

AjaxDatatablesRails::Base.instance_method(:sanitize).bind(self).call(data).

That may make for unreadable code to some, we could always define another method that then calls the above. Or if you have another way to fix, have at it. But this breaks in Ruby 2.4.0 and rails 5.2.3 for me without this change.

…fferent things. Because datatables inherit from AjaxDatatablesRails::ActiveRecord which inherits from AjaxDatatablesRails::Base for some odd reason sanitize binds to ActionView's sanitize method which eventually gets to Loofah and Nokogiri which doesn't handle arrays and throws the error I reported in jbox-web#325 surprisingly even when you call self.sanitize it does not call the method in the same namespace and file!! because technically the sanitize function that we want is AjaxDatatablesRails::Base's function, to disabiguate we could either rename the function (and because I don't know what else calls it that seemed like a bad idea), or we could help disambiguate by being explicit about which super's method to call.  The only way I saw to do this is to call it like this: AjaxDatatablesRails::Base.instance_method(:sanitize).bind(self).call(data).  That may make for unreadable code to some, we could always define another method that then calls AjaxDatatablesRails::Base.instance_method(:sanitize).bind(self).call(data).  Or if you have another way to fix, have at it.  But this breaks in Ruby 2.4.0 and rails 5.2.3 for me without this change.
@n-rodriguez
Copy link
Member

n-rodriguez commented Apr 17, 2019

Hi there! Thanks for the patch!
I think you're might be using https://github.com/jbox-web/ajax-datatables-rails#using-view-helpers and for some reasons it calls the Rails sanitize method instead of our one.

Since sanitize method is a private method, and should not be used by people, and is covered by tests, and is called only once in as_json method, I think it would be better to rename it instead of using Ruby tricks to call it. Why not sanitize_data :)

@n-rodriguez n-rodriguez self-assigned this Apr 17, 2019
@epipheus
Copy link
Author

yeah. I think that's a safer bet. I just assumed it might be referenced by a bunch of other stuff and not mine to rename :). No worries. I appreciate all your hard work on this.

@n-rodriguez
Copy link
Member

n-rodriguez commented Apr 20, 2019

it might be referenced by a bunch of other stuff

It might be used by some people who want to use Datatable orthogonal data. But for now it's more a trick than an official supported feature. So let's go for a rename and a line in the changelog ;)

@n-rodriguez
Copy link
Member

Hi there! Any news?

@epipheus
Copy link
Author

epipheus commented May 5, 2019

oh I'm sorry.. sure I can make a new pull request.

@epipheus
Copy link
Author

epipheus commented May 5, 2019

OK. I didn't write a test for it but I did verify that the error does not come up. It's a pretty simple tweak so :). Yours to merge and close.

@n-rodriguez n-rodriguez merged commit f18129d into jbox-web:master May 8, 2019
@n-rodriguez
Copy link
Member

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants