Issue posting comment on links using buddypress

[camaro4d]

I’m experimenting some issues posting Comments on the last version of buddypress (2.3.3) and buddypress links (0.9.4.1) with WordPress 4.3.1 and template Twenty fourteen, Twenty Thirteen, Twenty Thirteen, Simple Portal and Epic:

When I try to send a comment (as admin user or other user), I get the message: “There was a problem posting your update. Please try again”

I can add comments from activity board but not into the news page. the comment made in the activity board doesn´t show up in the link page

I desactivated all the plugins, use default theme and other buddypress friendly themes and still not working

I install a virgen fresh brand new wordpress 4.3.1, then install buddypress and then buddypress-links and the issue is still there. Waiting for your opinion and help. Thanks a million

with Betaversion in BUDDYPRESS TRUNK, the error message doesn’t show up, but the comments are not shown on the page, the number of comments does appear.

I’ve already report that on buddypress wordpress page: ​https://wordpress.org/support/topic/issue-posting-comment-on-links?replies=5#post-7453186

Test page link: ​http://goo.gl/Q0l9XZ

Thanks for your job!

[camaro4d]

seems that there is no activity shown on activity page for the admin. Ok for other users

[boonebgorges]

I looked into this a bit. There's a bug in the buddypress-links plugin. The function signature of bp_links_dtheme_activity_custom_update() looks like this:

function bp_links_dtheme_activity_custom_update( $object, $item_id, $content )

but it should look like this:

function bp_links_dtheme_activity_custom_update( $check, $object, $item_id, $content )

where $check is the false value sent to the 'bp_activity_custom_update' filter, which should be replaced by the activity ID. (The corresponding add_filter() call should also expect 4 arguments rather than 3.)

I'm closing this as it's not a BP bug, but please feel free to direct the developer to this ticket if he's got any questions.

[camaro4d]

Thanks a million Boonebgorges for your time and help, I reported this to the developer.

[MrMaz]

I looked into this and found two different filter calls for bp_activity_custom_update, each with a different number of args as of r10184.

Three args:
​https://buddypress.trac.wordpress.org/browser/trunk/src/bp-activity/bp-activity-actions.php?rev=10184#L356

Four args:
​https://buddypress.trac.wordpress.org/browser/trunk/src/bp-templates/bp-legacy/buddypress-functions.php?rev=10184#L960

Was this intentional? Need guidance on how to handle this in my plugin and/or on what kind of patch you are looking for.

[imath]

You're right ​https://buddypress.trac.wordpress.org/browser/trunk/src/bp-activity/bp-activity-actions.php?rev=10184#L356 misses the first arg.

It has been corrected into buddypress-functions.php in a previous version but unfortunately forgotten here.

6630.patch should fix it.

[MrMaz]

attachment:6630.patch​

This works for me, thanks :)

[MrMaz]

This is how I implemented backwards compat. Leave the filter function args blank, and use list() to get the args. Make sure add_filter() allows 4 args.

function my_activity_custom_update() {

    if ( func_num_args() === 4 ) {
        list( $activity_id, $object, $item_id, $content ) = func_get_args();
    } else {
        $activity_id = false;
        list( $object, $item_id, $content ) = func_get_args();
    }

    // create custom activity item
    $my_activity_id = my_create_custom_activity( $activity_id );

    return $my_activity_id;    
}
add_filter( 'bp_activity_custom_update', 'my_activity_custom_update', 10, 4 );

[boonebgorges]

6630.patch looks like the wrong fix to me.

The discrepancy between the two uses of the filter was introduced in [9160]. The filter was originally introduced to the AJAX callback in [2552], using 3 params, not 4.

In [2692], the same filter was added to bp_activity_action_post_update(), using the same pattern of 3 parameters.

It's not a very well-designed filter, and has probably caused bugs over the years. Perhaps it ought to have been written with false for the default parameter from the start. But I don't think there's a pressing reason to change it - it's certainly possible to use it the way it is. (MrMaz managed to use it!) [9160] looks like a regression to me.

I'm not sure whether a different approach is needed for #6021. If so, it probably looks like this:

...
} else {
    $posted_object = $_POST['object'];
    $activity_id = apply_filters( 'bp_activity_custom_update', $posted_object, $item_id, $content );
    if ( $posted_object === $activity_id ) {
        $activity_id = null;
    }
}

If we go this route, the filter needs to be carefully documented to explain this quirk.

Alternatively, let's introduce a new filter that does what [9160] does.

[MrMaz]

Just some feedback on my case... testing the number of arguments is the best/safest clue I have to check for the regression issue because I *think* that sometimes $_POST['object'] can be empty/false or at least eval to false. So I can't simply check if the first arg is a bool/int vs string. Even if that was reliable, I am still faced with handling a variable number of args.

Comparing versions doesn't help because it's called with a different number of args from different locations in the same version.

My vote would be to revert it to three args and document when the regression was introduced and when corrected in the filter doc block, which is what I *think* @boonebgorges is saying. That being said... I already have to leave the compat code in my plugin forever, so it's kind of moot.

As far as moving forward, I couldn't find enough context as to what problem #6021 was actually correcting, so I can't comment on that.

[r-a-y]

Alternatively, let's introduce a new filter that does what [9160] does.

tl;dr - This is what I would go with as well.

---

I think the filter we are most concerned about is the one in bp-legacy since that relies on AJAX / JS being enabled and is the one that is going to be used most of the time.

The filter in bp_activity_action_post_update() only fires / occurs if JS is disabled. I think we can leave that filter alone since that is rarely going to be used.

@MrMaz - Doing a list() check would be applicable if you wanted to support older versions of BuddyPress <= 2.1, or the bp-default theme (​which doesn't include r9160), or if you were posting custom activity updates without JS.

r9160 was introduced since v2.2 so if we move back to 3 arguments, this will just cause more issues for devs.

I see the following options:

1. Rename the filter in bp-legacy. Problem with this is devs would need to add another filter just for bp-legacy. Only advanced plugins like MrMaz's and imath's that are re-using the "What's New" activity form would need to use this filter. To me, this is quite rare.
2. Rename the filter in bp_activity_action_post_update() to something else. Problem with this is bp-default still uses the same filter name with different arguments.
3. Do not alter any code, but add in-depth phpDoc to the filter in bp-legacy. This could just confuse devs even more.

Option 1 is probably the easiest, but plugins would need to update their code to use the new bp-legacy filter.

Would be particularly interested in MrMaz and imath's feedback here.

[MrMaz]

As I mentioned in my last comment, I already have to check for number of args since the same filter is called with a different number of args from two different locations in several releases. So as far as my case goes, it's a moot point for back compat. I already had to bake it in, and I have to leave it there for a long time.

Whatever is decided to go with moving forward can be easily handled since I can then compare BP versions (assuming the number of args is made to be consistent for every filter call).

Whether to reverse regression bugs (especially which are caught this late) is a policy issue. I support whatever decision you guys make regarding that.

[tw2113]

This is another issue that I think may be obsolete with bp-nouveau. @hnla @mercime do you see that being a likely case?

[hnla]

There was a lot of discussion, then ticket punted to later milestones then to future release, if this was a pressing issue then I would have expected more in the intervening 20 months.

Given no activity and fact it will simply linger around I'm closing this ticket if anyone sees particular value in keeping it in 'Future Release' please re-open.

As for Nouveau, not sure if this is an issue, but would expect not.