Skip to content
This repository has been archived by the owner on Sep 27, 2023. It is now read-only.

html in error messages displayed #107

Closed
timelsass opened this issue Mar 1, 2019 · 15 comments
Closed

html in error messages displayed #107

timelsass opened this issue Mar 1, 2019 · 15 comments
Assignees
Labels
Status: Future Release This issue will be fixed in the future release of the plugin, or an enhancement will be added in the Status: Good First Issue This is a good first issue, something a new contributor can work on. Type: Bug There is a bug in the plugin.
Milestone

Comments

@timelsass
Copy link
Member

image

@dingo-d dingo-d self-assigned this Mar 1, 2019
@dingo-d dingo-d added Type: Bug There is a bug in the plugin. issue: has PR labels Mar 1, 2019
@dingo-d dingo-d added this to the 1.0.0 milestone Mar 1, 2019
@dingo-d
Copy link
Member

dingo-d commented Mar 1, 2019

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

@timelsass
Copy link
Member Author

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

timelsass referenced this issue in timelsass/theme-sniffer Mar 2, 2019
@timelsass
Copy link
Member Author

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

@timelsass
Copy link
Member Author

added a7b6ae1 to check for iframe before output

dingo-d added a commit that referenced this issue Mar 2, 2019
@dingo-d
Copy link
Member

dingo-d commented Mar 2, 2019

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

@dingo-d
Copy link
Member

dingo-d commented Mar 2, 2019

Merged the PR and it works 👍 Thanks!

@dingo-d dingo-d closed this as completed Mar 2, 2019
@DannyCooper
Copy link

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

image

image

@timelsass
Copy link
Member Author

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
Copy link
Member

dingo-d commented Mar 6, 2019

Maybe we could strip tags before outputting the message?

@dingo-d dingo-d reopened this Mar 6, 2019
@dingo-d dingo-d added Status: Future Release This issue will be fixed in the future release of the plugin, or an enhancement will be added in the Status: Good First Issue This is a good first issue, something a new contributor can work on. and removed issue: has PR labels Mar 6, 2019
@joyously
Copy link

joyously commented Mar 6, 2019

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
Copy link
Member

dingo-d commented Mar 6, 2019

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

@joyously
Copy link

joyously commented Mar 6, 2019

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
Copy link
Member Author

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

timelsass referenced this issue in timelsass/theme-sniffer Mar 6, 2019
@dingo-d
Copy link
Member

dingo-d commented Mar 30, 2019

This has been fixed?

@timelsass
Copy link
Member Author

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Status: Future Release This issue will be fixed in the future release of the plugin, or an enhancement will be added in the Status: Good First Issue This is a good first issue, something a new contributor can work on. Type: Bug There is a bug in the plugin.
Projects
None yet
Development

No branches or pull requests

4 participants