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

Add a sanitize package #7824

Closed
ntwb opened this issue Jul 9, 2018 · 3 comments
Closed

Add a sanitize package #7824

ntwb opened this issue Jul 9, 2018 · 3 comments
Assignees
Labels
[Package] Sanitize /packages/sanitize

Comments

@ntwb
Copy link
Member

ntwb commented Jul 9, 2018

@adamsilverstein commented on Tue Jul 18 2017

These could use some unit tests.


@nylen commented on Tue Jul 25 2017

When are these functions expected to be used? In particular the combination of "strip tags and encode HTML entities" seems a bit strange to me.


@ntwb commented on Wed Aug 16 2017

I've added a handful of basic tests for stripTags via WordPress/packages@7e360f0


@codecov[bot] commented on Fri Aug 18 2017

Codecov Report

Merging #12 into master will decrease coverage by 12.06%.
The diff coverage is 30%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #12       +/-   ##
===========================================
- Coverage     100%   87.93%   -12.07%     
===========================================
  Files           6        9        +3     
  Lines          48       58       +10     
  Branches        7       10        +3     
===========================================
+ Hits           48       51        +3     
- Misses          0        5        +5     
- Partials        0        2        +2
Impacted Files Coverage Δ
packages/sanitize/src/sanitizeText.js 0% <0%> (ø)
packages/sanitize/src/index.js 0% <0%> (ø)
packages/sanitize/src/stripTags.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d7c1b5e...6c93f80. Read the comment docs.


@adamsilverstein commented on Fri Aug 18 2017

When are these functions expected to be used? In particular the combination of "strip tags and encode HTML entities" seems a bit strange to me.
@nylen -
In WordPress core, the functions are used in wp-admin/js/press-this.js. sanitizeText is called in getTitleText to safely set the untrusted title text: $( '#post_title' ).val( getTitleText() );. stripTags is used in that function and also might be generally useful: it uses regexs to strip tags from content.


@notnownikki commented on Mon Aug 21 2017

I just added a decodeEntities util to Gutenberg, and it was suggested it might be relevant here too.

Commit is at f08c5f4 , if it's useful I'm happy to work on contributing it here.


@adamsilverstein commented on Tue Oct 10 2017

@notnownikki that does seem useful, not sure it belongs with sanitize? I had originally put these helpers in wp.utils, only later separating them out.

@aduth
Copy link
Member

aduth commented Mar 20, 2019

@adamsilverstein I'm trying to reinterpret the requirements from WordPress/packages#12 , which mentions press-this.js.

Should we consider a package if / when the time comes we decide to port that into the mono-repository? Ties back into some of the conversation in yesterday's JavaScript chat about not proactively porting modules.

There's also a combination of escape-html (introduced by @ellatrix in #7890) and html-entities (introduced by @gziolo in #7977) which can cover some of the proposed requirements.

tl;dr Can we close this?

@adamsilverstein
Copy link
Member

This was related to extracting existing core functionality on wp.sanitize into a package (see https://github.com/WordPress/wordpress-develop/blob/master/src/js/_enqueues/wp/sanitize.js). Not sure how much value these have or how they compare to the helpers you mentioned.

I only found one place where we actually use these functions in core: https://github.com/WordPress/wordpress-develop/blob/267c79a5182b3eebdb319874bb5e3ae168a9a6ac/src/js/_enqueues/wp/theme-plugin-editor.js#L119 (although there are probably some other places where we should be using these. As you mentioned, these functions originally came from PressThis which has now been removed from core entirely.

tl;dr; Yea, sure - we can go ahead and close this.

@aduth
Copy link
Member

aduth commented Apr 8, 2019

Thanks for checking it out, @adamsilverstein

@aduth aduth closed this as completed Apr 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Sanitize /packages/sanitize
Projects
None yet
Development

No branches or pull requests

3 participants