Comments

Jeff Burnz created an issue. See original summary.

joelpittet’s picture

Priority: Normal » Major
Status: Active » Needs review
FileSize
623 bytes

Thanks @Jeff Burnz for finding this. Here's a patch to fix it.

joelpittet’s picture

Issue tags: +Needs tests

We will need to add a regression test which should be fairly straight forward. Just check for existence for a tag on that page when there are comments on a topic.

joelpittet’s picture

The last submitted patch, 4: forum_list_html_twig-2636074-4-test-only.patch, failed testing.

Berdir’s picture

Why #prefix instead of two #markup elements?

TwigExtension has a safeJoin(). I've wondered before if we shouldn't offer something similar as an API, e.g. on markup? I know that I've had a use case for this, but I can't remember where.

It's not very nice, but it would allow us to fix this without changing anything about the structure..

joelpittet’s picture

@Berdir I just made that choice because it was being used as a prefix, IMO. I want that safeJoin thing too but it seems like some think it may be a security risk from what I gather, the discussion
#2501975: Determine how to update code that currently joins strings in SafeMarkup::set() and #2554073: Allow #markup to be an array of strings and join them safely

And somewhat related:
#2579091: Make safe_join Twig filter return a Markup object

With #prefix or another #markup we are using xss admin filtering the results in more or less the same way. (#prefix doesn't check for #plain_text or #allowed_tags.

mikeker’s picture

Probably not in-scope for this issue, but there looks like a lot of HTML being generated in template_preprocess_forums(). Should this template be refactored so that HTML can be overwritten by other themes?

joelpittet’s picture

@mikeker I was thinking the same thing;) Maybe we can do that in 8.1.x, not sure how we are going to wrangle markup changes yet. Would you mind opening up an issue for that so we can tackle that. Even just moving the table markup to a template would be way more useful for themers than the #type table, IMO

@Berdir if you prefer two #markup for some reason I'll change it if that makes it RTBCable. I'm not concerned either way.

mikeker’s picture

Status: Needs review » Closed (duplicate)
Related issues: +#2488886: Forum - "new replies" message is escaped

I'm marking this as a dup of #2488886: Forum - "new replies" message is escaped as there is a much longer thread in that issue discussing this change. That issue has been stalled since May so hopefully new eyes will get it moving again. Feel free to reverse that if you feel strongly.

joelpittet’s picture

Seems like a good call, that's an older issue. thanks for triaging these @mikeker

Jeff Burnz’s picture

Wow, thanks for finding that, I looked and looked as well, cheers mikeker.