Skip to:
Content

BuddyPress.org

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#6821 closed defect (bug) (wontfix)

Wrong number of links

Reported by: mahype Owned by:
Milestone: Priority: normal
Severity: trivial Version:
Component: Core Keywords: has-patch close
Cc:

Description

In the function "bp_core_check_for_moderation" the number of links will be checked. It gets the value by

$max_links = get_option( 'comment_max_links' );

It's getting approved by this code:

if ( $num_links >= $max_links ) {
    return false;
}

It should be:

if ( $num_links > $max_links ) {
    return false;
}

to work fine.

Attachments (1)

6281.diff (434 bytes) - added by mahype 4 years ago.

Download all attachments as: .zip

Change History (13)

@mahype
4 years ago

#1 @mahype
4 years ago

  • Keywords has-patch added

#2 @hnla
4 years ago

So we're saying if equal to the value return false so in effect if the strict sense of a number of links allowed is taken e.g $max_links = 10 we would only ever actually allow '9'.

Looks like a good catch and needs correcting, thanks for the ticket & patch.

#3 @boonebgorges
4 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from Awaiting Review to 2.5

Good catch. See also #6719.

This is an area crying out for unit tests :)

#4 @hnla
4 years ago

Just been looking over the function in core, and it prompted me to test comment links allowed qnt.

Some things need possible review and second opinions:

If I set a relatively low links allows value in WP Discussion settings e.g three and then create a status update containing three links in my user account or from activity dir I get a warning message that there was a problem adding my update and to please try again.

This message is wrong we should be clear what has happened, WP should hold the comment for moderation, we do not but do actually publish it but not obviously so so in effect we are NOT moderating or preventing these updates being posted.

#5 @boonebgorges
4 years ago

This message is wrong we should be clear what has happened

See #6719.

#6 @hnla
4 years ago

:) exactly what I was looking at - after the fact!

So we should patch this ticket to remove the equal to operator and then really turn attention to the bigger issue of the non existent moderation while purporting to be doing so while posting a confusing message.

#7 @mahype
4 years ago

The message which was declined, is also not really declined. It appears as last update in the profile.

#8 @hnla
4 years ago

@mahype Yes this is true, I was looking at this yesterday and passed some comments in the BP Slack channel along the lines of wondering whether this whole aspect of moderation in reality ought not to just be pulled out until such time as it can be built properly and I think there's some agreement in principle.

So that might mean that in fact we just allow links regardless of what limits imposed via the WP discussion settings.

I'm going to try and look at this further, see what issues or difficulties, if any, may arise from simply removing this function altogether.

Comments and feedback on that possible course of action would be welcome!

#9 @r-a-y
4 years ago

  • Keywords close added; needs-unit-tests removed

I'd say this is a wontfix since we abide by the options in the admin area's "Settings > Discussion" page.

This is the description for the Comment Moderation option:

Hold a comment in the queue if it contains X or more links. (A common characteristic of comment spam is a large number of hyperlinks.)

X or more equates to >=. (Side note: I know we do not have a queue feature yet.)

#10 @hnla
4 years ago

@r-a-y Semantics are all wrong then and confusing but that's not necessarily BP's fault:
'comment_max_links' says whatever the value contained is is the 'max' allowed, WP contradicts itself.

#11 @r-a-y
4 years ago

  • Milestone 2.5 deleted
  • Resolution set to wontfix
  • Status changed from new to closed

This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.


4 years ago

Note: See TracTickets for help on using tickets.