Skip to:
Content

Opened 8 months ago

Closed 8 months ago

Last modified 8 months ago

#3206 closed defect (fixed)

bbp_form_topic_subscribed() is wrong when editing a reply

Reported by: wpdennis Owned by: johnjamesjacoby
Milestone: 2.6 Priority: high
Severity: normal Version: trunk
Component: API - Subscriptions Keywords: dev-feedback
Cc:

Description (last modified by netweb)

If a user subscribes to a topic and then edits one of his replies in this topic, bbp_get_form_topic_subscribed() will not return 'checked' as it should in reply-form.php.

The reason is that bbp_get_global_post_field( 'post_author', 'raw' ) returns a string here:

<?php
elseif ( bbp_is_topic_edit() || bbp_is_reply_edit() ) {

        // Get current posts author
        $post_author = bbp_get_global_post_field( 'post_author', 'raw' );

        // Post author is not the current user
        if ( bbp_get_current_user_id() !== $post_author ) {
                $topic_subscribed = bbp_is_user_subscribed( $post_author, bbp_get_topic_id() );

                // Post author is the current user
        } else {
                $topic_subscribed = bbp_is_user_subscribed( bbp_get_current_user_id(), bbp_get_topic_id() );
        }
}

And in the following the checks won't work, most critically the array_search() here, where $user_id is still a string:

<?php
function bbp_is_object_of_user( $object_id = 0, $user_id = 0, $meta_key = '', $meta_type = 'post' ) {
        $user_ids = bbp_get_users_for_object( $object_id, $meta_key, $meta_type );
        $retval   = is_numeric( array_search( $user_id, $user_ids, true ) );

        // Filter & return
        return (bool) apply_filters( 'bbp_is_object_of_user', $retval, $object_id, $user_id, $meta_key, $meta_type );
}

Bugfix: Add cast and reduce unnecessary code in bbp_get_form_topic_subscribed():

<?php
elseif ( bbp_is_topic_edit() || bbp_is_reply_edit() ) {
        // Get current posts author
        $post_author = (int) bbp_get_global_post_field( 'post_author', 'raw' );
        $topic_subscribed = bbp_is_user_subscribed( $post_author, bbp_get_topic_id() );
}

It should be enough to always check the post_author. The check bbp_get_current_user_id() !== $post_author is unnecessary because it says:

  • If the current user is not the author, take the author
  • If the user is the author, take the user.

In the end it's always the author.

To make sure bbp_is_object_of_user() works in all instances, an explicit cast should be considered, too. Otherwise bugs like this are really sneaky.

<?php
function bbp_is_object_of_user( $object_id = 0, $user_id = 0, $meta_key = '', $meta_type = 'post' ) {
        $user_ids = bbp_get_users_for_object( $object_id, $meta_key, $meta_type );
        $retval   = is_numeric( array_search( (int) $user_id, $user_ids, true ) );

        // Filter & return
        return (bool) apply_filters( 'bbp_is_object_of_user', $retval, $object_id, $user_id, $meta_key, $meta_type );
}

Attachments (1)

3206.patch (1.8 KB) - added by wpdennis 8 months ago.

Download all attachments as: .zip

Change History (7)

@wpdennis
8 months ago

#1 @netweb
8 months ago

  • Description modified (diff)
  • Milestone changed from Awaiting Review to 2.6

Thanks @wpdennis

#2 follow-up: @johnjamesjacoby
8 months ago

  • Owner set to johnjamesjacoby

I believe the !== $post_author check exists so that Moderators can edit other people's replies and edit that author's subscription status instead of their own.

#3 @johnjamesjacoby
8 months ago

In 6828:

Engagements: enforce absint() on function parameters in engagements API.

Also fixes a bug causing an array_search() to erroneously fail.

See #3206.

#4 @johnjamesjacoby
8 months ago

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

In 6829:

Subscriptions: cast function results to (int) so strict comparisons pass.

This change fixes a bug where subscription checkboxes and UI elements could show/use incorrect values.

Props wpdennis. Fixes #3206.

#5 in reply to: ↑ 2 @wpdennis
8 months ago

Replying to johnjamesjacoby:

I believe the !== $post_author check exists so that Moderators can edit other people's replies and edit that author's subscription status instead of their own.

Exactly, it's always the post author. The code is just a very complex way of saying take always the post author no matter what:

<?php
if ( bbp_get_current_user_id() !== $post_author ) {
                $topic_subscribed = bbp_is_user_subscribed( $post_author, bbp_get_topic_id() );

                // Post author is the current user
        } else {
                $topic_subscribed = bbp_is_user_subscribed( bbp_get_current_user_id(), bbp_get_topic_id() );
        }

As I wrote above:

  • If the current user is not the author, take the author
  • If the user is the author, take the user (who is the author)

There is no way (and no reason) this complex snippet is performed with any other id than the post author id.

#6 @johnjamesjacoby
8 months ago

In 6830:

Subscriptions: simplify edit logic for getting checked() value.

Props wpdennis. See #3206.

Note: See TracTickets for help on using tickets.