workbench_moderation_workbench_block() generates invalid HTML. All message values are put inside em elements, including the moderation form. The moderation form is block-level and cannot go inside the inline-level em element. Fix attached.

CommentFileSizeAuthor
#22 workbench_moderation-invalid_html-1411892-22-D7.patch1.13 KBLiam Morland
#21 workbench_moderation-invalid_html-1411892-21-D7.patch1.13 KBLiam Morland
#20 workbench_moderation-invalid_html-1411892-20-D7.patch1.13 KBLiam Morland
#17 workbench_moderation_invalid_html.patch1.13 KBLiam Morland
#16 workbench_moderation_invalid_html.patch1.09 KBLiam Morland
#13 workbench_moderation_invalid_html.patch1.05 KBLiam Morland
#7 workbench_moderation_invalid_html.patch1.09 KBLiam Morland
#6 invalid-html-1411892-6.patch981 bytesstevector
workbench_moderation_invalid_html.patch787 bytesLiam Morland
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Pls’s picture

Pls CreditAttribution: Pls commented
Status: Needs review » Reviewed & tested by the community

Patch works great and fixes the problem for markup.

hass’s picture

hass CreditAttribution: hass commented
Status: Reviewed & tested by the community » Needs work

What are you trying to do here? I do not understand the substr logic. Do you like to use strip_tags() for the message?

Liam Morland’s picture

Liam Morland
Primary language English
Location Ontario, CA 🇨🇦
CreditAttribution: Liam Morland commented

The call to substr() is just a quick way to determine if the "message" actually a moderation form which shouldn't be wrapped in an em element.

hass’s picture

hass CreditAttribution: hass commented

Looks not reliable...

Liam Morland’s picture

Liam Morland
Primary language English
Location Ontario, CA 🇨🇦
CreditAttribution: Liam Morland commented

How about:

if ($message['message'] === check_plain($message['message'])) {
stevector’s picture

stevector
he/him
CreditAttribution: stevector commented
Status: Needs work » Needs review
FileSize
invalid-html-1411892-6.patch981 bytes

Thanks for identifying this!

The invalid HTML is definitely a problem and if a patch like this is the best way to fix it then we'll go with it. I'm posting a revised patch here. It checks for '

I'm open to any opinions on which is more appropriate.

Check_plain() is not an option because that would wreck the form's markup.

Liam Morland’s picture

Liam Morland
Primary language English
Location Ontario, CA 🇨🇦
CreditAttribution: Liam Morland commented
FileSize
workbench_moderation_invalid_html.patch1.09 KB

Yes, that would work too.

Here is a more robust approach.

stevector’s picture

stevector
he/him
CreditAttribution: stevector commented

Liam, I'm not following that last patch.

I also now see that my last comment had invalid html in it and was thus cut off!

I tried to say

Thanks for identifying this!

The invalid HTML is definitely a problem and if a patch like this is the best way to fix it then we'll go with it. I'm posting a revised patch here. It checks for '<form' because it's possible that a link would be used here. I've also switch the strpos instead of substr because that felt more straight forward.

I'm open to any opinions on which is more appropriate.

Check_plain() is not an option because that would wreck the form's markup.

Liam Morland’s picture

Liam Morland
Primary language English
Location Ontario, CA 🇨🇦
CreditAttribution: Liam Morland commented

check_plain() is just used to check if the message is plain text or HTML. If it is HTML, it goes through as-is. If it is plain text, it gets wrapped in an em element.

My last patch uses CSS instead of wrapping messages in an em element. It looks the same, but has slightly different semantics.

Your patch in #6 would work for the form. If a message had any other block-level HTML, it wouldn't work. The check_plain() or CSS approaches would work for any content.

hass’s picture

hass CreditAttribution: hass commented

check_plain() is just used to check if the message is plain text or HTML. If it is HTML, it goes through as-is. If it is plain text, it gets wrapped in an em element.

Ahhh, NO! All tags are converted to safe HTML. This means < become &lt; and so on. If a %placeholder inside t() is used it get's surrounded by <em> (format_string).

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

Related, I'm not sure why this code is continuing to use t() since there's no real words in this text. I think this should just be calling drupal_placeholder() manually?

Liam Morland’s picture

Liam Morland
Primary language English
Location Ontario, CA 🇨🇦
CreditAttribution: Liam Morland commented

Ahhh, NO! All tags are converted to safe HTML.

Yes, but the output of check_plain() is not put into the page. The only thing that is done with it is it the compared to the original string. If $message and check_plain($message) are the same, then it is safe to wrap it in an em element.

Related, I'm not sure why this code is continuing to use t()

Yes, it is the message and the label that needs to be translated. Thanks for noticing that.

Liam Morland’s picture

Liam Morland
Primary language English
Location Ontario, CA 🇨🇦
CreditAttribution: Liam Morland commented
FileSize
workbench_moderation_invalid_html.patch1.05 KB

Here is an updated patch that removes the call to t() which doesn't do anything anyway because there is no text in that string. (If text is not being translated, that is a different issue and removing t() does not have any effect on that.)

hass’s picture

hass CreditAttribution: hass commented

I cannot speak 100% for arabic/hebrew/chinese/japanese, but it's possible that we need to keep this wrapped in t().

Liam Morland’s picture

Liam Morland
Primary language English
Location Ontario, CA 🇨🇦
CreditAttribution: Liam Morland commented

Could be. In that case, use the patch in #7. The only difference is the wrapping in t().

Liam Morland’s picture

Liam Morland
Primary language English
Location Ontario, CA 🇨🇦
CreditAttribution: Liam Morland commented
Issue summary: View changes
FileSize
workbench_moderation_invalid_html.patch1.09 KB

Reroll.

Liam Morland’s picture

Liam Morland
Primary language English
Location Ontario, CA 🇨🇦
CreditAttribution: Liam Morland commented
FileSize
workbench_moderation_invalid_html.patch1.13 KB

Sorry, this is the reroll.

The last submitted patch, 16: workbench_moderation_invalid_html.patch, failed testing.

Liam Morland’s picture

Liam Morland
Primary language English
Location Ontario, CA 🇨🇦
CreditAttribution: Liam Morland commented

The patch in #17 is fine; the error is about the patch in #16.

Liam Morland’s picture

Liam Morland
Primary language English
Location Ontario, CA 🇨🇦
CreditAttribution: Liam Morland at University of Waterloo commented
FileSize
workbench_moderation-invalid_html-1411892-20-D7.patch1.13 KB

Re-roll. This also applies to 7.x-3.x.

Liam Morland’s picture

Liam Morland
Primary language English
Location Ontario, CA 🇨🇦
CreditAttribution: Liam Morland at University of Waterloo commented
FileSize
workbench_moderation-invalid_html-1411892-21-D7.patch1.13 KB

Re-roll.

Liam Morland’s picture

Liam Morland
Primary language English
Location Ontario, CA 🇨🇦
CreditAttribution: Liam Morland at University of Waterloo commented
Version: 7.x-1.x-dev » 7.x-3.x-dev
FileSize
workbench_moderation-invalid_html-1411892-22-D7.patch1.13 KB