Skip to:
Content

BuddyPress.org

Opened 10 years ago

Closed 10 years ago

#5557 closed enhancement (fixed)

BP_Group_Extension settings_screen_save

Reported by: dcavins's profile dcavins Owned by: boonebgorges's profile boonebgorges
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)

#1 @boonebgorges
10 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 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.

#2 @dcavins
10 years ago

  • Cc david.cavins@… added

#3 @dcavins
10 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 @boonebgorges
10 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.


10 years ago

#6 @DJPaul
10 years ago

  • Milestone changed from 2.1 to 2.2

#7 @boonebgorges
10 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:

  1. 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 in call_edit_screen_save(), after the 'screen_save_callback' had been invoked.
  2. The group extension does not do any redirect. In this case, our proposed redirect would kick in.
  3. 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.

#8 @boonebgorges
10 years ago

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

In 8934:

BP_Group_Extension should automatically handle redirects after the 'edit' screen save routine

After handling POSTed settings save on the 'edit' view of a BP_Group_Extension
plugin, plugins were previously required to handle their own redirects
(primarily to avoid "are you sure you want to resubmit the data?" browser
prompts when reloading the page). This created a disparity between the 'edit'
screen and the 'create' and 'admin' screens, the latter two of which handled
their own redirect logic.

This changeset performs the necessary redirect after calling the edit screen
save callback. There is some logic in place to ensure that no double redirects
take place.

Fixes #5557

Note: See TracTickets for help on using tickets.