Skip to:
Content

bbPress.org

Opened 16 months ago

Closed 16 months ago

Last modified 16 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 16 months ago.

Download all attachments as: .zip

Change History (7)

@wpdennis
16 months ago

#1 @netweb
16 months ago

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

Thanks @wpdennis

#2 follow-up: @johnjamesjacoby
16 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
16 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
16 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
16 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
16 months ago

In 6830:

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

Props wpdennis. See #3206.

Note: See TracTickets for help on using tickets.