Add a sanitize package by adamsilverstein · Pull Request #12 · WordPress/packages

adamsilverstein · 2017-07-18T13:14:21Z

These could use some unit tests.

aduth

Could also do for a README.md

aduth · 2017-07-18T19:18:21Z

packages/sanitize/src/index.js

+ * @return Sanitized text. False on failure.
+ */
+export function sanitizeText( text ) {
+	var _text = wp.utils.stripTags( text ),


How should we handle this dependency? Ideally it'd be referenced by Node module, but at the very least we need to ensure that the corresponding script handle is enqueued whenever the sanitize package is used.

oh, yea - good catch. i'll switch this to referencing as a node module

this has been refactored and is now imported

aduth · 2017-07-18T19:18:37Z

packages/sanitize/src/index.js

+ */
+export function sanitizeText( text ) {
+	var _text = wp.utils.stripTags( text ),
+		textarea = document.createElement( 'textarea' );


Could we reuse a shared reference of a textarea element? Is there any overhead to creating a new element every time this is invoked?

I'm sure there is some overhead and I like that suggestion, would that possible get destroyed though in a SPA context?

Calypso uses a single shared reference without any trouble:

https://github.com/Automattic/wp-calypso/blob/2bdafc81fb2e2d7fd6027ded360b9641d57564a0/client/lib/formatting/decode-entities/browser.js#L1-L7

Addressed in 4eb3d86

aduth · 2017-07-18T19:20:10Z

packages/sanitize/src/index.js

+
+	try {
+		textarea.innerHTML = _text;
+		_text = wp.utils.stripTags( textarea.value );


A potential optimization to consider is bypassing this step if there are no encoded entities to convert.

See related: Automattic/wp-calypso#9725

Good suggestion, for now the code here exactly matches what we have in core.

nylen · 2017-07-24T20:25:35Z

packages/sanitize/src/index.js

+/**
+ * Strip HTML tags.
+ *
+ * @param {string} text Text to have the HTML tags striped out of.


Typo: striped should be stripped

right, thanks.

Fixed in 6c93f80

nylen · 2017-07-24T20:28:31Z

packages/sanitize/src/index.js

+ *
+ * @return Sanitized text. False on failure.
+ */
+export function sanitizeText( text ) {


This needs a better name and description. sanitizeText - in what way, against what? convert HTML entities - into what?

If I read this correctly, I think the answers are "remove tags and encode HTML entities". I'm having a hard time thinking of a good function name, though. Maybe we should just create the encodeHTMLEntities building block first, and not worry about the combination of the two functions until we need it? This would also be more easily testable.

We need the combined function to maintain existing functionality in PressThis. I'll try to improve the naming and descriptions here. The unit tests would also help make the usages clearer.

I updated the inline docs a bit to hopefully clear up how these functions work.

nylen · 2017-07-24T20:29:06Z

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.

…ari-voiceover Append a no-break space to repeated messages.

ntwb · 2017-08-16T08:21:43Z

I've added a handful of basic tests for stripTags via 7e360f0

codecov · 2017-08-16T08:23:04Z

Codecov Report

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

@@             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.

… into add-wp-sanitize

adamsilverstein · 2017-08-17T18:20:54Z

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 · 2017-08-21T09:38:10Z

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

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

adamsilverstein · 2017-10-09T13:08:38Z

@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.

ntwb · 2018-07-09T13:19:09Z

Issue moved to WordPress/gutenberg #7824 via ZenHub

Add a sanitize package

a506837

aduth reviewed Jul 18, 2017

View reviewed changes

ntwb added the [Package] Sanitize label Jul 19, 2017

nylen reviewed Jul 24, 2017

View reviewed changes

omarreiss pushed a commit that referenced this pull request Aug 1, 2017

Merge pull request #12 from Yoast/stories/af/11-repeated-messages-saf…

4e77280

…ari-voiceover Append a no-break space to repeated messages.

aduth mentioned this pull request Aug 15, 2017

Don't re-encode already encoded post titles WordPress/gutenberg#2407

Merged

tests: Add initial tests

7e360f0

ntwb and others added 3 commits August 17, 2017 10:18

tests: Add more tests and tweak description names

0ab72e9

Merge branch 'master' of github.com:WordPress/packages

13973ae

Merge branch 'add-wp-sanitize' of github.com:adamsilverstein/packages…

b02226f

… into add-wp-sanitize

adamsilverstein added 8 commits August 17, 2017 14:22

start at 1

7fd8b29

break out functions into files

77c70db

Improve doc blocks

066886f

central file for package

ce62cd5

fix export of stripTags

ff892f7

fix up import for tests which now pass

e504523

sanitizeText: reuse the textarea

4eb3d86

stripped not striped

6c93f80

aduth mentioned this pull request Nov 2, 2017

I18n: Make preview popup loader strings translateable WordPress/gutenberg#3301

Closed

ntwb mentioned this pull request Jul 9, 2018

Add a sanitize package WordPress/gutenberg#7824

Closed

ntwb closed this Jul 9, 2018

aduth mentioned this pull request Jul 16, 2018

Packages: Extract entities package WordPress/gutenberg#7977

Merged

Conversation

adamsilverstein commented Jul 18, 2017

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nylen Jul 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nylen commented Jul 24, 2017

Uh oh!

ntwb commented Aug 16, 2017

Uh oh!

codecov bot commented Aug 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

adamsilverstein commented Aug 17, 2017

Uh oh!

notnownikki commented Aug 21, 2017

Uh oh!

adamsilverstein commented Oct 9, 2017

Uh oh!

ntwb commented Jul 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nylen Jul 24, 2017 •

edited

Loading

codecov bot commented Aug 16, 2017 •

edited

Loading