Skip to:
Content

BuddyPress.org

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#7386 closed defect (bug) (fixed)

BP_Group_Extension: Create screen is not passed the new group ID.

Reported by: dcavins Owned by: boonebgorges
Milestone: 2.8 Priority: normal
Severity: normal Version: 2.7.2
Component: Groups Keywords: has-patch commit
Cc: dcavins

Description

When building a new group extension, the create_screen( $group_id = null ) signature suggests that a group ID will be passed to your method (or the catch-all edit_screen()). The group extension's group ID is calculated in BP_Group_Extension::get_group_id(), which uses bp_get_new_group_id() (a fetcher for $bp->groups->new_group_id) on the create screens. However, $bp->groups->new_group_id isn't set until add_action( 'bp_actions', 'groups_action_create_group' ) which is much later than most group extensions are loaded (often on plugins_loaded).

I'm attaching the simplest patch I could think of, which adds a cookie check to BP_Group_Extension::get_group_id(). We could also change the action on which groups_action_create_group() is fired to something much earlier, but this seems perilous. We could also revisit the group creation steps entirely, I guess.

Thanks for your feedback!

Attachments (2)

7386.1.diff (1.0 KB) - added by dcavins 5 years ago.
Change BP_Group_Extension::get_group_id() to check cookies for new group ID.
7386.diff (1.8 KB) - added by boonebgorges 5 years ago.

Download all attachments as: .zip

Change History (9)

@dcavins
5 years ago

Change BP_Group_Extension::get_group_id() to check cookies for new group ID.

#1 follow-up: @DJPaul
5 years ago

Is create_screen used during the group creation wizard?

#2 in reply to: ↑ 1 @dcavins
5 years ago

Replying to DJPaul:

Is create_screen used during the group creation wizard?

Yes. During the group creation process, BP_Group_Extension uses create_screen() if you've defined one in your extension, edit_screen() if you haven't. (This assumes you've added a create step.)

Last edited 5 years ago by dcavins (previous) (diff)

@boonebgorges
5 years ago

#3 @boonebgorges
5 years ago

7386.diff is another take: set the global in BP_Groups_Component::setup_globals(). It's probably possible to move a good deal of the global manipulation in groups_action_create_group() to setup_globals(), but this seems like the most lightweight method. In any case, it seems more like "right" fix to me.

#4 @dcavins
5 years ago

  • Keywords commit added

Boone's patch puts the logic in a logical place and it solves the problem. Thanks!

I'm sort of in favor of doing a more wholesale strip of the oddly isolated groups_action_create_group(). Now that you've moved some logic into setup_globals(), moving more of the create screen setup into that method would make the create screens seem more like a part of the groups component.

Last edited 5 years ago by dcavins (previous) (diff)

#5 follow-up: @boonebgorges
5 years ago

I'm sort of in favor of doing a more wholesale strip of the oddly isolated groups_action_create_group(). Now that you've moved some logic into setup_globals(), moving more of the create screen setup into that method would make the create screens seem more like a part of the groups component.

I agree - this would be a worthwhile thing to pursue, as it would make the internals of the Groups component more consistent and easier to debug. But it also has the potential to introduce bugs, so it should be pursued with more caution than we need to make this minor fix. So let's handle it separately.

#6 @boonebgorges
5 years ago

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

In 11313:

Groups: Set new_group_id global during group component setup.

This ensures that the global value is set early enough to be passed
to the create_screen() method of BP_Group_Extension objects.

Props dcavins.
Fixes #7386.

#7 in reply to: ↑ 5 @dcavins
5 years ago

Replying to boonebgorges:

I'm sort of in favor of doing a more wholesale strip of the oddly isolated groups_action_create_group(). Now that you've moved some logic into setup_globals(), moving more of the create screen setup into that method would make the create screens seem more like a part of the groups component.

I agree - this would be a worthwhile thing to pursue, as it would make the internals of the Groups component more consistent and easier to debug. But it also has the potential to introduce bugs, so it should be pursued with more caution than we need to make this minor fix. So let's handle it separately.

Sounds good to me. I'm trying to see what parts of the group creation process we can write tests for, so we can make changes with some confidence, otherwise we'll be back to guessing how we might affect other plugins and the creation process generally.

Thanks for making this commit, which fixes this immediate problem in a less hack-ish way than my suggested fix.

Note: See TracTickets for help on using tickets.