Skip to:
Content

Opened 17 months ago

Closed 2 months ago

#4680 closed defect (bug) (fixed)

@-mentions getting linked even when already inside a link

Reported by: boonebgorges Owned by: boonebgorges
Milestone: 2.0 Priority: low
Severity: minor Version:
Component: Core Keywords: has-patch
Cc:

Description

Imagine you're creating a link to a twitter profile in a blog post/page:

<a href="http://twitter.com/boone">@boone</a>

Imagine also that there's a local user with the user_nicename boone.

The @-mention filter finds @boone and does this:

<a href="http://twitter.com/boone"></a><a href="http://example.com/members/boone3/" rel="nofollow">@boone3</a>

Not 100% sure what the best fix is for this edge case. Probably: adjust the regex so that it skips @-mentions already inside of <a>...</a>. (not sure how this regex would look. Might have to do some PHP logic or break it into multiple regex matches) Thoughts?

Attachments (5)

4680.diff (1.7 KB) - added by ninnypants 3 months ago.
4680.1.diff (1.4 KB) - added by ninnypants 3 months ago.
4680.tests.diff (1.6 KB) - added by ninnypants 2 months ago.
4680.tests.1.diff (2.1 KB) - added by ninnypants 2 months ago.
4680.tests.1.2.diff (2.1 KB) - added by ninnypants 2 months ago.

Download all attachments as: .zip

Change History (19)

comment:1 sooskriszta6 months ago

Or, one could think about a wider implementation. Where the regex still converts @ mentions, but one can choose whether to convert the mention to BuddyPress (local), wordpress.com/org, Twitter or Facebook(?) mention...

Not sure exactly how the code would work though...thinking...

ninnypants3 months ago

comment:2 ninnypants3 months ago

The two issues that this seems to cause problems is in areas like mailto links and any @name inside of an <a> like @boonebgorges mentioned. I initially tried to solve it with a regular expression but the two situations are too different to be solved that way, and that still didn't solve the issue of replacements. The system should replace @names that show up in normal content, but not inside <a>.

In my solution I replaced offending content with a placeholder #BPAN with an incremented number attached to the end similar to how some markdown libraries do their replacements. This removes @names that we don't want replaced long enough for us do the replacements then they are added back into the correct location.

ninnypants3 months ago

comment:3 ninnypants3 months ago

  • Keywords has-patch added; needs-patch removed

4680.1.diff fixes an issue where if an @name is inside of a mailto link it will still be linked breaking the mailto link.

comment:4 boonebgorges3 months ago

  • Keywords needs-unit-tests added; dev-feedback removed
  • Milestone changed from Future Release to 2.0

Yup, this looks like a good technique. Thanks for the patch!

I'd like some unit tests before committing. ninnypants, if you're willing, please write up a couple tests. Otherwise I'll do it at some point during this cycle.

ninnypants2 months ago

comment:5 ninnypants2 months ago

I've written up some tests, but I'm not sure I put them in the right place.

comment:6 josswinn2 months ago

Just to add that I'm experiencing this problem, too. If there's anything I can do to help test on 1.9.2, let me know. I will have a look at the diff now.

Support thread: http://buddypress.org/support/topic/strange-injection-of-email-address-on-text-with-baseurl

comment:7 boonebgorges2 months ago

  • Keywords needs-unit-tests removed

Thanks, ninnypants - looks great!

comment:8 josswinn2 months ago

Should the code in this diff work when applied manually to bp-activity-filters.php (1.9.2)? It doesn't work for me. e.g. http://blogs.lincoln.ac.uk/test/

comment:9 josswinn2 months ago

Actually, it's fixed the mailto: links, but not when @lincoln.ac.uk is used in the body content. You can see that it only affects @lincoln.ac.uk (the root domain) and not @blah.org. It injects a random user's email address into the link.

comment:10 boonebgorges2 months ago

josswinn - If you're having issues, please share the exact content that you're testing with. Thanks!

comment:11 josswinn2 months ago

Background to the issue is here: http://buddypress.org/support/topic/strange-injection-of-email-address-on-text-with-baseurl

An example of the issue is here: http://blogs.lincoln.ac.uk/test/

The example is reflective of the issue *after* applying the patch from this thread. The patch has resolved the issue with mailto links as described earlier in this thread. However, the patch has not resolved a related issue whereby if I include @lincoln.ac.uk (the root domain name) in any post/page content, the BuddyPress Activity component automatically injects a hyperlink. An example is:

@lincoln.ac.uk

is converted to:

<p><a href='http://blogs.lincoln.ac.uk/amakrzanowska%40lincoln.ac.uk/' rel='nofollow'>@lincoln.ac.uk</a></p>

How it decides which user email to inject, I don't know. I have deleted a previous user it was injecting and it picked, seemingly randomly, another user email address to inject.

If I add

add_filter( 'bp_activity_do_mentions', '__return_false' );

to bp-custom.php, then the issue is resolved, but I also lose the 'mentions' feature of the activity component.

The issue doesn't occur on a similar test install that we use.

ninnypants2 months ago

ninnypants2 months ago

comment:12 ninnypants2 months ago

Just added a test with content similar to @josswinn's post, and can't recreate with the test suite.

comment:13 boonebgorges2 months ago

Thanks, ninnypants. I've found the same thing.

I'm about to commit the following:

  • ninnypants's fix
  • tests for the fix. I've adapted the tests added by ninnypants a little (removed the 'the_content' filters, moved them into a filters.php file) and added one to ensure that multiple @mentions are swapped correctly

josswinn, if you're having the problem on one installation but not the other, it suggests a configuration issue. It could very well be a bug in BuddyPress, but it does not appear to be directly related to the issue originally reported in this post, which has to do with @mentions embedded *in links*. Could I ask you to open a separate ticket with details to reproduce, and/or a link back to this ticket?

comment:14 boonebgorges2 months ago

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

In 7890:

Don't auto-link @mentions that appear inside the context of links

This prevents illegitimate double-linking when text like
href="mailto:foo@…" or <a href="http://twitter.com/bar">@bar</a> happen
to match a BP username like @bar.

Fixes #4680

Props ninnypants

Note: See TracTickets for help on using tickets.