search

WPTT / theme-sniffer

Theme Sniffer plugin using sniffs.
MIT License
270 stars 3 forks source link

html in error messages displayed #107

Closed timelsass closed 5 years ago

timelsass commented 5 years ago

image

dingo-d commented 5 years ago

Probably a parsing error, good catch. If you can provide a theme that generated that, that would be great 🙂

timelsass commented 5 years ago

here's twentyninteen the example code from https://developer.wordpress.org/reference/functions/add_menu_page/ added for testing: twentynineteen.zip

timelsass commented 5 years ago

It looks like that change would introduce another issue of loading unwanted iframe content: image

timelsass commented 5 years ago

added https://github.com/WPTRT/theme-sniffer/pull/113/commits/a7b6ae11ea314d2d88cb1f71a42b54e831a6cd9d to check for iframe before output

dingo-d commented 5 years ago

I guess there was a reason after all for futting .text() instead of .html() after all :D

dingo-d commented 5 years ago

Merged the PR and it works :+1: Thanks!

DannyCooper commented 5 years ago

I'm seeing something similar to this, the authors code isn't great but still...

image

image

timelsass commented 5 years ago

Ah, yeah that makes sense - I'll have to take a look at what all the messages are actually outputting to come up with a solution. I didn't give thought to if/when HTML is being output from other sniffs. I'm not sure why that one message includes HTML around the function either since the rest don't seem to do that.

dingo-d commented 5 years ago

Maybe we could strip tags before outputting the message?

joyously commented 5 years ago

So it's just that the messaging to the user is sometimes wanting HTML and most times should show HTML? At the point of output, the code doesn't know what's in the message. I don't think it should be escaping there. This is a case where late escaping hurts. The individual messages should be escaped where needed, since that's the point the code knows it's needed or not. The user should see the actual code that was in error, so it should not be stripped. And if there needs to be something in bold, that should be available also.

dingo-d commented 5 years ago

Stripped in the theme sniffer 🙂 so that we don't have html in the output. I should have been more precise

joyously commented 5 years ago

That was my point: sometimes it is correct to have HTML in the output. The message should not be stripped since it can contain HTML that is part of the message. If you strip the HTML out, the message doesn't convey the correct thing.

I think the fix that already went in for this was the wrong fix. The fix should have been to remove the <strong> tags from that one message, because the more common case is that messages need to contain exact code from the theme accurately report the problem.

timelsass commented 5 years ago

yes I agree with @joyously - especially given that no other messages really appear to be wrapping things in HTML - that one should definitely follow suit. Sent over PR for WPThemeReview

dingo-d commented 5 years ago

This has been fixed?

timelsass commented 5 years ago

the upstream has been for that message - I was using some HTML for some things in the license check messages though. Since we control the output of the messages we create, I think we can look at messages.message.source to verify it's our message, and then enable using html for the ones we have control over.