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

Camo Filter doesn't return doc when disabled #102

Closed
spicycode opened this issue Jan 21, 2014 · 6 comments
Closed

Camo Filter doesn't return doc when disabled #102

spicycode opened this issue Jan 21, 2014 · 6 comments

Comments

@spicycode
Copy link
Contributor

During some testing this morning I started using the disable_asset_proxy option. It seems when you pass that in the CamoFilter just returns nil, instead of the doc causing the rest of the filter chain to break.

@jch
Copy link
Contributor

jch commented Jan 22, 2014

Hmmm, ya this line https://github.com/jch/html-pipeline/blob/master/lib/html/pipeline/camo_filter.rb#L25 should return doc. I noticed that the disable_asset_proxy option is not currently documented. I'm not sure if disabling filters at runtime is common, but maybe it makes more sense to generalize this functionality at the Pipeline level rather than at the Filter level:

SomePipeline = HTML::Pipeline.new [
  HTML::Pipeline::CamoFilter,
  HTML::Pipeline::MarkdownFilter
]

SomePipeline.call("some input content", :disable_filters => [:camo])

Thoughts?

@spicycode
Copy link
Contributor Author

@jch At the moment we are using it to disable the filter in enterprise land, but keep it everywhere else. I think a higher level disable filters could be great as well.

@josh
Copy link
Contributor

josh commented Jan 27, 2014

0e32670

@jch
Copy link
Contributor

jch commented Jan 27, 2014

@josh thanks for the fix, could you put them in PR's in the future? I like to link to specific PR's in the change logs.

@josh
Copy link
Contributor

josh commented Jan 27, 2014

Yeah, sorry, it was a quick fix for .com purposes. We just hit the same issue, d'oh! But I kinda agree about just removing the filter if you aren't using it.

@jch
Copy link
Contributor

jch commented Jan 30, 2014

Closing. Slated for the next release v1.6.0. Waiting on #105 before I cut that.

@jch jch closed this as completed Jan 30, 2014
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

No branches or pull requests

3 participants