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

[dcavins]

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!

[dcavins]

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

[DJPaul]

Is create_screen used during the group creation wizard?

[dcavins]

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.)

[boonebgorges]

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.

[dcavins]

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.

[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.

[boonebgorges]

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.

[dcavins]

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.