Improvements to BP_Group_Extension

[boonebgorges]

This is a master ticket for 1.8 improvements to BP_Group_Extension. Items I plan to look at:

• Setting up and checking nonces automatically, instead of forcing plugin authors to manage it themselves
• Do a better job of checking whether to run display callbacks. Eg, there's no reason why plugin authors should have to check if ( ! bp_is_group_creation_step( $this->slug ) ) before rendering their edit_screen markup.
• The markup requirements on edit_screen() and create_screen() are slightly different, in particular regarding the Submit button. Think about ways this can be made more consistent.
• More intelligent defaults for enable_create_step, etc. Eg, if there's no create_step() method, we can pretty safely assume that enable_create_step should be disabled, at least by default. See also #4785

[boonebgorges]

In 6986:

In BP_Group_Extension, only fire the admin_screen_save() callback if it exists

This will avoid errors when the admin_screen() method is being used only to
display content, not to save settings.

See #4955

[boonebgorges]

In 6987:

Whitespace and coding standards in BP_Group_Extension::_register(). See #4955

[boonebgorges]

In 6988:

Only call BP_Group_Extension::create_screen* methods on proper creation step

By moving the bp_is_group_admin_screen() check into BP_Group_Extension, we
leave one fewer thing for plugins to miss in their implementations.

Load order issues mean that the calls to create_screen() and
create_screen_save() must be moved into standalone callbacks, and hooked
late in the load order so that we have access to information about the current
group creation step.

See #4955

[boonebgorges]

In 6992:

Pass the group id to the edit_ and create_ methods of BP_Group_Extension

This change creates parity with the admin_screen() and admin_screen_save()
methods, which will make it easier for plugin authors to abstract shared logic.

See #4955

[johnjamesjacoby]

Wondering if it makes more sense to set a variable in the current object for the current group_id rather than create new methods just to pass the group ID around. Thoughts?

[boonebgorges]

The purpose of the call_edit_screen() method is to pass the group_id param directly to the edit_screen() callback. We need the middleman because call_edit_screen() (and, previously, edit_screen() itself) is hooked to bp_template_content. The only way to pass a value to a callback on a WP action is to pass it as an additional parameter to do_action(), eg do_action( 'bp_template_content', bp_get_current_group_id() ). But doing this would require modifying template files, which won't work for backward compatibility (and is also ugly). Having the call_ method allows for full backward compatibility with existing themes and BP_Group_Extension plugins.

[boonebgorges]

4955.01.patch is a first pass at a major cleanup of BP_Group_Extension. Several of the problems I needed to solve had to be solved at the same time, thus the huge patch. A summary of the improvements:

1. Markup requirements are now standardized between edit_screen(), admin_screen(), and create_screen(). You no longer need to provide a Submit button in your edit_screen() markup - we provide it for you (though if an existing plugin *does* provide its own Submit button, we detect it and do not add a second one). Nonces are now created and checked automatically for all form contexts. And BP_Group_Extension is smart about checking whether your markup should appear in a given place - you don't have to check if ( bp_is_group_creation_step( $this->slug ) ) etc anymore, because we do it for you.

2. Because markup and logic requirements for the _screen() and _screen_save() functions are now identical, I've introduced support for generic fallback methods called settings_screen() and settings_screen_save(). When you define these methods in your extension, they'll provide the markup and save-logic for your Create, Edit, and Admin panels. If you provide the specific methods for a given context (say, edit_screen() and create_screen_save()), BP_Group_Extension detects it, and uses the more specific ones instead. So: maximum flexibility for plugin authors, while making it as simple as possible for the majority of plugins (which will share logic/markup between all those screens).

3. Instead of configuring your extension using a hodge-podge of object properties (like $this->enable_nav_item = true; in your __construct() method), I've converted to using an array of config options, which should then be passed to parent::init() at the end of the extension constructor. This has several major benefits:
   • It keeps all config in a single place
   • It mirrors the way that BP_Component extensions work
   • It allows us to use wp_parse_args() style logic to parse custom params with defaults. One very cool feature here is that I've added a recursive version of wp_parse_args(), which means that your config array only has to contain the specific values that you want to change, even if they're one or two levels deep in the array - BP_Group_Extension will parse them intelligently with the default config. (It's like child theming for your config arrays :) )

See the patch, and the inline documentation, for more details on how these arguments look. (They're mostly ported directly over, except for those that are specific to 'edit', 'create', and 'admin', in which case they've been moved to a structured 'screens' array.)

4. Complete backward compatibility with existing BP_Group_Extension plugins. I had to do a couple of tricks to ensure that plugin configured the old way could be converted to the new way (see the 'Legacy Support' section of the patch). I've written unit tests for the trickiest bits of this backward compatibility layer. Testing with a dozen or so plugins that use BP_Group_Extension in different ways.

5. Basic reorganization and modularizing. BP_Group_Extension is currently a rat's nest, especially the _register() method and its inline logic. I have reorganized the class so that it makes much more sense. I've broken logic out into its own separate (and testable) methods. And I've written inline docs for the entire thing.

I'm attaching three files:

1. 4955.01.patch is the patch, which includes unit tests
2. bp-group-extension.php is a clean copy of the newly reorganized class. You might find this a bit easier to read, if you're going to get into the nitty-gritty.
3. bpge_test.zip is a small plugin that demonstrates how awesome the BP_Group_Extension class. It contains two extensions: one is basic, and one is a bit more customized. You might consider starting with this plugin *first* (before spending much time with the patch), so that you can see firsthand how the changes in 4955.01.patch make BP_Group_Extension so much more flexible and pleasant to use.

Thanks in advance for your feedback.

[johnjamesjacoby]

Peer review

[johnjamesjacoby]

Patch looks good to me, let's think about getting this in soon. My updated patch is largely functionally the same as 4955.01.patch, with a few edits:

• Random code clean-up.
• Bail early where possible to avoid the extra indentation.
• Added more strict comparison checking.
• Added default values to class variables and method parameters.
• Fixed a $this->is_initialized that should be $this->initialized.
• Fixed a 'false;' that should be 'return false;

[johnjamesjacoby]

We should think about pulling the parse_args_r() function out of the class, and into a function. We can spruce it up with filters ala bbp_parse_args() in bbPress also, and replace all instances of wp_parse_args() in bbPress core.

[johnjamesjacoby]

Also, long term (1.9?) it would be sahweet if the core functionality of the Groups component was extracted out to use the BP_Group_Extension class. Would make core a great example for others to learn from, ala register_post_type() in WordPress core.

[boonebgorges]

In 6997:

Refactor and reorganization of BP_Group_Extension

The shiny new BP_Group_Extension class boasts the following features:

• The *_screen* methods (edit_screen(), edit_screen_save(), etc) now share identical requirements for markup and logic. You no longer need to provide a Submit button for edit_screen() but not for create_screen(); BuddyPress provides one for you in each context. (For backward compatibility, we verify that your edit_screen() method does not already have a Submit button before auto-adding our own.) Nonces are now created and checked automatically for all form contexts. And it is no longer necessary to check whether you are on the correct group creation or admin step before outputting your markup - BP_Group_Extension does it for you.
• Introduces support for fallback methods settings_screen() and settings_screen_save(). When you define these methods in your extension, they will provide the markup and form-saving logic for your Create, Edit, and Admin panels. If you provide specific methods for a given context (say, edit_screen() and create_screen_save()), BP_Group_Extension detects it, and uses the more specific ones instead. This should make it much easier to write DRY code in your BP_Group_Extension classes, while maintaining maximum flexibility.
• Configuration should now be set using a config array, which is then passed to parent::init() at the end of your class constructor. This technique more closely mirrors the way that BuddyPress and WordPress handle configuration elsewhere. BP_Group_Extension parses your config array to arbitrary depth, so that you only need to pass those values that you wish to change from the defaults.
• Complete backward compatibility for legacy BP_Group_Extension plugins.
• Improved organization, documentation, and unit tests.

Props johnjamesjacoby for detailed feedback.

See #4955

[boonebgorges]

I'm marking this one complete, as I've made all the changes that I'd set out to in the original ticket. Please open separate tickets for new issues.

[r-a-y]

In 7106:

In BP_Group_Extension, when the edit screen's position is set, use that
position when adding the group admin tab.

See #4955.

[needle]

@boonebgorges Am leaving a comment here even though the ticket is closed because I'd like to check out your demo code in bpge_test.zip​. However, when I download it, it unpacks into bpge_test.zip.cpgz, which unpacks into bpge_test.zip recursively! Could you please reattach? Many thanks - the new class looks great.

[boonebgorges]

needle - Weird, it's unzipping fine for me. I've rezipped and attached as bpge_test_2.zip - let me know if that works for you.

[needle]

boonebgorges, nope, still does same thing on my OSX 10.8 machine

[boonebgorges]

OK, I'm going to attach the plugin file itself, which you will want to embed in a plugin that loads at the right time.

[needle]

Perfect, thanks Boone