underscore.string icon indicating copy to clipboard operation
underscore.string copied to clipboard

stripTags strips things that aren't tags

Open paulfalgout opened this issue 8 years ago • 20 comments

paulfalgout avatar Jan 07 '16 05:01 paulfalgout

Thanks for pointing us to this issue. Do you want to fix this bug?

stoeffel avatar Jan 13 '16 06:01 stoeffel

@stoeffel Regex isn't really my thing, so this heavily borrows from https://github.com/kvz/phpjs/blob/master/functions/strings/strip_tags.js

paulfalgout avatar Jan 13 '16 07:01 paulfalgout

Funny enough FWIW https://github.com/epeli/underscore.string/pull/266 is also heavily borrowing from the same source and would also solve this problem, but adds the ability to allow certain tags (why the author renamed allow to ignore, I'm not sure)

paulfalgout avatar Jan 13 '16 07:01 paulfalgout

@stoeffel any chance to get this one merged? This issue is kind of severe :/

JSteunou avatar Feb 24 '16 10:02 JSteunou

will review it tomorrow. sorry for the delay. we will be faster with our new build chain. On Feb 24, 2016 11:54, "Jérôme Steunou" [email protected] wrote:

@stoeffel https://github.com/stoeffel any chance to get this one merged? This issue is kind of severe :/

— Reply to this email directly or view it on GitHub https://github.com/epeli/underscore.string/pull/469#issuecomment-188190754 .

stoeffel avatar Feb 24 '16 11:02 stoeffel

Nice! Thank you for your work.

JSteunou avatar Feb 24 '16 11:02 JSteunou

I tried this with some additional tests and the last one fails.

  equal(stripTags('<h1 id="foo" data-foo="bar">hello world</body></h1>'), 'hello world');
  equal(stripTags('<web-component>hello world</web-component>'), 'hello world');
  equal(stripTags('<ReactComponent.Title>hello world</ReactComponent.Title>'), 'hello world');
  equal(stripTags('I have <I want, but that is > nothing'), 'I have <I want, but that is > nothing');
  78 passing (32ms)
  1 failing

  1)  #stripTags:

      AssertionError: 'I have  nothing' == 'I have <I want, but that is > nothing'
      + expected - actual

-       I have  nothing
+      I have <I want, but that is > nothing

      at Context.<anonymous> (tests/stripTags.js:13:3)


stoeffel avatar Feb 24 '16 14:02 stoeffel

I'm afraid this is the closest strip tags can be done with regex. True strip tags can only be done with parser.

JSteunou avatar Feb 24 '16 14:02 JSteunou

Yes, you are right. It only looks strange :smile_cat:

stoeffel avatar Feb 24 '16 14:02 stoeffel

Could you include a test for comments and maybe some tests like I described above? Otherwise this looks good. Thanks for the PR.

stoeffel avatar Feb 24 '16 15:02 stoeffel

a test for comments

Can you clarify this?

paulfalgout avatar Feb 24 '16 15:02 paulfalgout

I mean a test for <!-- a html comment --->. On Feb 24, 2016 16:54, "Paul Falgout" [email protected] wrote:

a test for comments

Can you clarify this?

— Reply to this email directly or view it on GitHub https://github.com/epeli/underscore.string/pull/469#issuecomment-188316209 .

stoeffel avatar Feb 24 '16 15:02 stoeffel

Ok good deal. I'll see about getting to this shortly

paulfalgout avatar Feb 24 '16 16:02 paulfalgout

nice. thanks On Feb 24, 2016 17:01, "Paul Falgout" [email protected] wrote:

Ok good deal. I'll see about getting to this shortly

— Reply to this email directly or view it on GitHub https://github.com/epeli/underscore.string/pull/469#issuecomment-188319931 .

stoeffel avatar Feb 24 '16 16:02 stoeffel

The more I think about this, the more I think we should consider deprecating striptags (like we did with sprintf). I think the problem stripTags is trying to solve is too big for _.str and a module like https://www.npmjs.com/package/striptags does a much better job. What are your thoughts?

stoeffel avatar Feb 25 '16 07:02 stoeffel

I'm good with that. My biggest beef with this function is that I assumed a little too much that it would work, but it was aggressively stripping things that were valid inputs from users. This implementation is better but I've gone in another direction myself as I can't have either a false sense of security or false positives. Probably for something as sensitive as this functionality it should come from a library that is very focused on the task.

paulfalgout avatar Feb 25 '16 07:02 paulfalgout

Ho did not know about this lib. Good idea. About sprintf & striptags, why deprecated it? I mean, I consider underscore.string like the Swiss army knife when it comes to deal with Strings and I like having all those features in one place, even if that means some features are just symbolic links to another library. I trust underscore.string enough to let it choose for me the best library for the job under the hood.

JSteunou avatar Feb 25 '16 08:02 JSteunou

About sprintf & striptags, why deprecated it?

Having dependencies means more maintenance. We would have to deal with issues for 3rd party libraries that aren't under _.str control. We would have to document the functionality of these modules, which is overhead and can lead to misleading documentation. With deprecating this functions and temporarily linking to the other module, we can make users aware of that module, which solves the problem much better.

stoeffel avatar Feb 25 '16 09:02 stoeffel

Make sense. Thank you.

JSteunou avatar Feb 25 '16 09:02 JSteunou

@epeli do you agree with deprecating stripTags?

stoeffel avatar Feb 25 '16 09:02 stoeffel