Skip to:
Content

Opened 3 years ago

Closed 2 years ago

#2963 closed defect (fixed)

@-mentions parsed wrongly, spanning whitespace and punctuation - since 2.5.9 (changeset 6015?)

Reported by: cjerrells Owned by: johnjamesjacoby
Milestone: 2.6 Priority: high
Severity: minor Version: 2.5.9
Component: Component - Users Keywords: needs-patch
Cc:

Description

Hi,

I just upgraded from 2.5.8 to 2.5.9 and noticed that an existing forum post which included two usernames separated by " & " got parsed a bit differently now, with the first username's link spanning over the " & ". (screenshot attached)

I believe this worked correctly (i.e. two separate username links, with non-linked " & " in between) on 2.5.8 so perhaps related to changeset 6015? A similar post with two usernames separated by " and " worked as expected.

Attachments (5)

Screen Shot 2016-06-15 at 12.20.33.png (37.9 KB) - added by cjerrells 3 years ago.
Incorrect parsing
Screen Shot 2016-06-15 at 12.20.46.png (25.4 KB) - added by cjerrells 3 years ago.
Correct parsing
Screen Shot 2016-06-24 at 17.54.35.png (30.8 KB) - added by cjerrells 3 years ago.
Another parsing case, brackets around @-mention
Screen Shot 2016-07-29 at 17.30.39.png (129.7 KB) - added by cjerrells 3 years ago.
Mis-parsing of @-mentions after links
2963-tests.diff (2.3 KB) - added by netweb 2 years ago.

Download all attachments as: .zip

Change History (19)

@cjerrells
3 years ago

Incorrect parsing

@cjerrells
3 years ago

Correct parsing

#1 @cjerrells
3 years ago

  • Summary changed from @-mentions parsed wrongly since (2.5.9) changeset 6015? to @-mentions parsed wrongly, spanning whitespace and punctuation - since 2.5.9 (changeset 6015?)

#2 @johnjamesjacoby
3 years ago

  • Component changed from General to Component - Users
  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 2.6
  • Owner set to johnjamesjacoby

Thanks for the report.

This is for sure related to that bit of code. Thankfully we have a bevy of unit-tests for this, so I'll dig in and make sure it's addressed asap.

@cjerrells
3 years ago

Another parsing case, brackets around @-mention

#3 @cjerrells
3 years ago

Great! Thanks @johnjamesjacoby.

I just added another screenshot: a valid username being @-mentioned, but (presumably due to brackets around it) didn't get detected and turned into a link.

@cjerrells
3 years ago

Mis-parsing of @-mentions after links

#4 @cjerrells
3 years ago

Added another example of mis-parsed @-mentions. I'm not sure if this also changed in 2.5.9, I'm afraid I can't test before/after. But it seems like @mentions get overlooked after a hyperlink. In the screenshot above "Started Learning" and "What is Rhythm" are hyperlinks, and the (valid) @-mentions after them don't get turned into links.

EDIT: Sorry, this is incorrect. This occurs when previewing a Post, but not when the same HTML is put into a bbPress post. So I think that means it's a BuddyPress issue rather than a bbPress one.

Last edited 3 years ago by cjerrells (previous) (diff)

#5 @johnjamesjacoby
2 years ago

  • Priority changed from normal to high

#6 in reply to: ↑ description @netweb
2 years ago

Replying to cjerrells:

Hi,

I just upgraded from 2.5.8 to 2.5.9 and noticed that an existing forum post which included two usernames separated by " & " got parsed a bit differently now, with the first username's link spanning over the " & ". (screenshot attached)

I believe this worked correctly (i.e. two separate username links, with non-linked " & " in between) on 2.5.8 so perhaps related to changeset 6015? A similar post with two usernames separated by " and " worked as expected.

I cannot reproduce this with bbPress /trunk, both @user1 & @user2 and @user1 and @user2` generates the correct markup:

<div class="bbp-reply-content">
        <p>
                Test <a href="http://example.com/forums/users/user1/" class="bbp-user-id-1 bbp-user-mention">@user1</a> &amp; <a href="http://example.com/forums/users/user2/" class="bbp-user-id-2 bbp-user-mention">@user2</a>
                <br> 
                Test <a href="http://example.com/forums/users/user1/" class="bbp-user-id-1 bbp-user-mention">@user1</a> and <a href="http://example.com/forums/users/user2/" class="bbp-user-id-2 bbp-user-mention">@user2</a>
        </p>
</div>

Replying to cjerrells:

Great! Thanks @johnjamesjacoby.

I just added another screenshot: a valid username being @-mentioned, but (presumably due to brackets around it) didn't get detected and turned into a link.

We currently exclude usernames wrapped in angle brackets, i.e: <@username>

Note: See follow up comment regarding this use case

Replying to cjerrells:

Added another example of mis-parsed @-mentions. I'm not sure if this also changed in 2.5.9, I'm afraid I can't test before/after. But it seems like @mentions get overlooked after a hyperlink. In the screenshot above "Started Learning" and "What is Rhythm" are hyperlinks, and the (valid) @-mentions after them don't get turned into links.

EDIT: Sorry, this is incorrect. This occurs when previewing a Post, but not when the same HTML is put into a bbPress post. So I think that means it's a BuddyPress issue rather than a bbPress one.

Confirming, I cannot reproduce this with bbPress /trunk

@netweb
2 years ago

#7 @netweb
2 years ago

In 2963-tests.diff I've added some tests for comment:6 above, they fail though the cause of the failures is the test logic in both:
BBP_Tests_Common_Functions_Make_Clickable::test_bbp_make_clickable_multiple_mention_hits
BBP_Tests_Common_Functions_Make_Clickable::test_bbp_make_clickable_multiple_mention_misses

These tests currently run bbp_make_clickable() on the compiled link, rather than running on the matched @username, as such I "believe" the patch fixes both occurrences and adds a couple more tests based on my same logic.

The end result of the tests "expects" an extra @, the links are correct for each mention so I think the tests are correct and we just need to adjust the code in bbp_make_mentions_clickable() and bbp_make_mentions_clickable_callback() to have the tests pass

  • https://cldup.com/NxpDDtAsYI.png

My sanity is questionable on all this right at this moment, I'm finishing up for the day and wanted to document where I was at thus far! IF no one else checks my sanity overnight I'll revisit this tomorrow ;P

#8 @netweb
2 years ago

In 6374:

Build/Test Tools: Revert unintended unit test change included in commit [6373]

Antiprops netweb.
See #3085, #2963.

#9 @johnjamesjacoby
2 years ago

In 6377:

Formatting: More tests for at-mentions.

Props netweb. See #2963.

#10 @johnjamesjacoby
2 years ago

In 6378:

Formatting: Add rel-nofollow to tests. See #2963.

#11 @johnjamesjacoby
2 years ago

In 6380:

Mentions: Balance tags before making things clickable.

Then, make mentions clickable without "requiring" prepending whitespace. This allows other types of characters to be immediately before the @ control character.

See #2963.

#12 @johnjamesjacoby
2 years ago

In 6381:

Mentions: Move topic/reply content filters into the same section of filters.php.

Also add wp_make_content_images_responsive filter, to match core content areas.

See #2963.

#13 @johnjamesjacoby
2 years ago

In 6382:

Mentions: Add tests for square and round brackets.

See #2963.

#14 @johnjamesjacoby
2 years ago

  • Resolution set to fixed
  • Status changed from new to closed

@cjerrells, thanks again for being patient with us.

I'm confident this is at least improved, if not fixed entirely.

I've also opened up a ticket in the WordPress core Trac to investigate something somewhat related:

https://core.trac.wordpress.org/ticket/40191

Note: See TracTickets for help on using tickets.