Skip to:
Content

bbPress.org

Opened 2 years ago

Closed 8 months ago

#3493 closed defect (bug) (fixed)

Default arg to "bbp_add_forums_roles()" can cause errors

Reported by: naxoc's profile naxoc Owned by: johnjamesjacoby's profile johnjamesjacoby
Milestone: 2.6.10 Priority: normal
Severity: normal Version: 2.2
Component: API - Roles/Capabilities Keywords: reporter-feedback 2nd-opinion dev-reviewed has-patch
Cc: naxoc

Description

The bbp_add_forums_roles() function has a default arg of null for $wp_roles. That causes problems when we try to use it in the loop in the function. This patch adds a check and initializes the $wp_roles var if necessary.

Attachments (2)

3493.patch (432 bytes) - added by naxoc 2 years ago.
3493_2.patch (436 bytes) - added by naxoc 2 years ago.

Download all attachments as: .zip

Change History (5)

@naxoc
2 years ago

#1 @johnjamesjacoby
2 years ago

  • Keywords reporter-feedback 2nd-opinion dev-reviewed added
  • Milestone changed from Awaiting Review to 2.6.10
  • Owner set to johnjamesjacoby
  • Status changed from new to assigned
  • Version set to 2.2

Interesting! 🤔

I have a question about your use case:

when we try to use it in the loop in the function

Are you able to share an example? I'd love to know more about how you're using bbPress roles!


History

(When implementing them, I suppose I'd not considered that bbp_add_forums_roles() would be called directly, or outside of anyplace that $wp_roles as a WP_Roles object would not be passed in.)

Usually, bbPress roles get invoked directly from WP_Roles (which is the $wp_roles global):

  • $wp_roles->__construct()
  • $wp_roles->for_site()
  • $wp_roles->init_roles()
  • wp_roles_init hook
  • bbp_roles_init hook
  • bbp_add_forums_roles()

Multisite blog-switching will add these to the beginning of the above stack:

  • switch_blog hook
  • wp_switch_roles_and_user()

Patch notes

Thank you again 🙏 for including a patch with this issue. I've applied & tested it, and (if I understand your use-case) it seems like it does what you (may) need it to, but with 1 caveat worth mentioning...

Calling wp_roles() will trigger the wp_roles_init action, and doing so where you've suggested will cause a single runtime recursion, where bbp_add_forums_roles() gets called from inside of bbp_add_forums_roles()the call is coming from inside the house! 👻

It is possible to conditionally "juggle" the hook in there to avoid recursing (and that may be necessary here) but typically that is an early indicator there may be a better approach that involves fewer compromises or work-arounds.

Edit: Regardless of anything, you have identified code that we can improve together. bbPress should not attempt to blindly call methods on the $wp_roles global variable without confirming they are callable. 💫

Last edited 2 years ago by johnjamesjacoby (previous) (diff)

#2 @naxoc
2 years ago

  • Cc naxoc added
  • Keywords has-patch added

I have a question about your use case:

when we try to use it in the loop in the function

Yes, sorry. That was not very clear. The loop I meant was the foreach further down in the function where the $wp_roles var is used. Not the WP-loop :)

the call is coming from inside the house! 👻

Woops! I didn't catch that one. Let's find a better solution. I guess it would be a breaking change to remove the default null arg, but how about this patch that just creates a stdClass if we need to?

Last edited 2 years ago by naxoc (previous) (diff)

@naxoc
2 years ago

#3 @johnjamesjacoby
8 months ago

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

In 7257:

Roles: prevent notices when calling bbp_add_forums_roles()

This change adds sanity checks around the $wp_roles parameter of the bbp_add_forums_roles() function.

It will now attempt to initialize the WP_Roles class just-in-time, but only if not already doing the wp_roles_init action (to prevent recursion).

If after all of that $wp_roles is still not what it needs to be, this function will now silently bail instead of proceeding (to prevent debug notices & errors).

Fixes #3493.

Props naxoc.

In branches/2.6, for 2.6.10.

Note: See TracTickets for help on using tickets.