Skip to:
Content

bbPress.org

Opened 18 months ago

Last modified 18 months ago

#3493 assigned defect (bug)

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 18 months ago.
3493_2.patch (436 bytes) - added by naxoc 18 months ago.

Download all attachments as: .zip

Change History (4)

@naxoc
18 months ago

#1 @johnjamesjacoby
18 months 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 18 months ago by johnjamesjacoby (previous) (diff)

#2 @naxoc
18 months 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?

Version 0, edited 18 months ago by naxoc (next)

@naxoc
18 months ago

Note: See TracTickets for help on using tickets.