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

Prepare for partial release. #16525

Merged
merged 1 commit into from
Aug 18, 2014
Merged

Conversation

kaspth
Copy link
Contributor

@kaspth kaspth commented Aug 16, 2014

  • Default to Rails::DeprecatedSanitizer in ActionView::Helpers::SanitizeHelper.
  • Add upgrade notes.
  • Add sanitizer to new applications Gemfiles.
  • Remove 'rails-dom-testing' as a dependency.

@kaspth
Copy link
Contributor Author

kaspth commented Aug 16, 2014

I don't understand why the build failed. I've added 'rails-deprecated_sanitizer' as a dependency. Shouldn't that make it work?

@kaspth
Copy link
Contributor Author

kaspth commented Aug 16, 2014

ah, the gem is unreleased.

@robin850 robin850 added this to the 4.2.0 milestone Aug 17, 2014
The new version updates `sanitize`, so it can take a `Loofah::Scrubber` for powerful scrubbing.
[See some examples of scrubbers here](https://github.com/flavorjones/loofah#loofahscrubber).

Two new scrubbers have also been added, they are `PermitScrubber` and `TargetScrubber`. Read more about in the [gems readme](https://github.com/rails/rails-html-sanitizer).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you could write:

Two new scrubbers have also been added: PermitScrubber and TargetScrubber. For further information, you can read the gem's readme.

Also could you please wrap your additions around 80 chars ? Thank you so far ! :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope I did it right. Thanks, Robin.

- Default to Rails::DeprecatedSanitizer in ActionView::Helpers::SanitizeHelper.
- Add upgrade notes.
- Add sanitizer to new applications Gemfiles.
- Remove 'rails-dom-testing' as a dependency.
@dhh
Copy link
Member

dhh commented Aug 17, 2014

Has the gem been released yet?

@rafaelfranca
Copy link
Member

on it.

rafaelfranca added a commit that referenced this pull request Aug 18, 2014
@rafaelfranca rafaelfranca merged commit 2042598 into rails:loofah Aug 18, 2014
@rafaelfranca rafaelfranca deleted the partial-release-prep branch August 18, 2014 00:22
@jeremy
Copy link
Member

jeremy commented Aug 18, 2014

❤️

Read the [gem's readme](https://github.com/rails/rails-html-sanitizer) for more information.

The documentation for `PermitScrubber` and `TargetScrubber` explains how you
can gain complete control over when and how elements should be stripped.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kaspth do we print a deprecation message for this? would be nice if we can figure out a sensible place to do this, so that...

  1. People who don't use the sanitize helpers at all will not see the warning
  2. People who use the old default will be nagged about the switch in the next version
  3. People who have the new gem installed will not see the warning

Similar to #16537. wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inserting a deprecation message here would satisfy those three constraints: https://github.com/rails/rails-deprecated_sanitizer/blob/master/lib/rails/deprecated_sanitizer/html-scanner/html/sanitizer.rb#L7

The subclasses super this method (or don't override it) and is therefore called on every sanitize call when using the deprecated behavior.

Here's a PR for this: kaspth/rails-deprecated_sanitizer/pull/4

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.

None yet

6 participants