Opened 11 years ago
Closed 11 years ago
#5557 closed enhancement (fixed)
BP_Group_Extension settings_screen_save
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 2.1 | Priority: | low |
Severity: | minor | Version: | |
Component: | Groups | Keywords: | needs-patch |
Cc: | david.cavins@… |
Description
If your BP_Group_Extension-based group plugin uses an "enable" setting or allows the user to change the plugin's slug or navigation tab label via the plugin's group admin pane, the navigation tabs do not reflect the changes upon submitting the admin form. (It seems that the group navigation tab is being rendered before the form is processed.)
Plugin authors must force a redirect at the end of the save routine to re-render the page, like
bp_core_redirect( bp_get_group_permalink( groups_get_group( array( 'group_id' => $group_id ) ) ) . 'admin/' . $this->slug );
Can we improve this behavior, or is it reasonable to expect plugin authors to force the redirect when necessary? (Similarly, I really like that BP handles the nonces since 1.8.)
Change History (8)
#3
@
11 years ago
Something else to consider is that if the plugin author (me) is only using the catch-all settings_screen_save
and forces a redirect at the end, that redirect will also be fired when saving from the WordPress group admin interface, which is not ideal, since it usually redirects the user to the group's front-end admin screen. And I imagine that if it occurs before some other piece is saved, there could be conflicts and lost data.
I'm mostly noting this for myself, so that the new "should we redirect" logic takes into account where the save request is coming from.
#4
@
11 years ago
that redirect will also be fired when saving from the WordPress group admin interface, which is not ideal, since it usually redirects the user to the group's front-end admin screen
Yes, but under the hood, BP knows whether you're in edit vs admin mode, even if you use the generic settings_screen_save()
. I don't see a serious problem here (though I'd have to look more closely to see how it'd work best).
And I imagine that if it occurs before some other piece is saved, there could be conflicts and lost data.
This seems a bit more worrisome. During group creation and group editing, it should not be an issue, since the only data displayed on the screen is related to the current group tab. On group admin, it's more complex, because many items are sent over the same POST request. It probably will *not* make sense to do the redirect when in the Dashboard (I think BP does it anyway).
This ticket was mentioned in IRC in #buddypress-dev by boonebgorges. View the logs.
11 years ago
#7
@
11 years ago
- Milestone changed from 2.2 to 2.1
Just coming back around to this. It turns out that the patch is actually quite simple. Basically, we have to account for three possibilities:
- The group extension handles the redirect on its own, and calls
die;
afterward. In this case, our proposed redirect would never be called, since we'd put it incall_edit_screen_save()
, after the 'screen_save_callback' had been invoked. - The group extension does not do any redirect. In this case, our proposed redirect would kick in.
- The group extension handles a redirect, but does *not* die afterward. This is the only tricky case, because doing a double redirect has the potential to break various things (like the bp_core_add_message() items that are intended to be displayed on the next pageload).
However, I did some tests using a simple plugin that falls into category 3, and found that the redirect was actually completing and halting the pageload before the BP_Group_Extension redirect was hit. So I think that 3 actually is the same as 1 for all intents and purposes.
That said, there could be a difference in this redirect behavior in some edge cases, so it's still worth having a failsafe. Because this is straightforward, I'm going to go ahead and sneak it back into 2.1.
Cool idea. There'll be some tricks required to make sure that we don't do double-redirects, but you're right that this would be a real benefit to developers. The proper place to look will be in
call_edit_screen_save()
and the corresponding methods for other contexts.