Skip to:
Content

BuddyPress.org

#7822 closed defect (bug) (fixed)

Nouveau: Settings > Email menu item not visible

Reported by: boonebgorges Owned by: boonebgorges
Milestone: 3.0 Priority: highest
Severity: blocker Version:
Component: Settings Keywords: has-patch
Cc:

Description

The Settings > Email item isn't visible, unless I manually type the URL /settings/notifications. Can anyone else confirm?

Attachments (4)

Screenshot_2018-05-11_16-26-57.png (23.0 KB) - added by boonebgorges 19 months ago.
Screenshot_2018-05-11_16-27-13.png (24.1 KB) - added by boonebgorges 19 months ago.
7822.diff (35.7 KB) - added by boonebgorges 19 months ago.
7822.empty.patch (1.7 KB) - added by r-a-y 19 months ago.

Download all attachments as: .zip

Change History (17)

#1 @r-a-y
19 months ago

  • Keywords reporter-feedback added

Works over here. Tested on Twenty Seventeen and Twenty Twelve.

Could it be caused by a plugin? Does it exist in the HTML source or could it be hidden by CSS?

#2 @DJPaul
19 months ago

  • Keywords reporter-feedback removed

I'm seeing the same behaviour as Boone.

#3 @DJPaul
19 months ago

Repro'd on twenty seventeen. It's not included in the HTML.

#5 @boonebgorges
19 months ago

It's bp_settings_remove_email_subnav(). Because Nouveau has a different technique for registering notification settings, BP mistakenly thinks that something's missing. Give me a minute to come up with something.

#6 @DJPaul
19 months ago

As noted on Slack, we need to check other uses of has_action() in this way while we're at it.

#7 @DJPaul
19 months ago

  • Severity changed from normal to blocker

@boonebgorges
19 months ago

#8 @boonebgorges
19 months ago

  • Keywords has-patch added

Turns out this has nothing to do with Nouveau. It's a result of the screen-function loading changes from @r-a-y. Those changes moved the bp_notification_settings callbacks into their own conditionally-loaded files. But this meant that bp_notification_settings had no hooked functions, and as a result, bp_settings_remove_email_subnav() was doing its thing.

The fix should just be to move the functions back out into a place where they're always loaded. I've chosen bp-*-notifications.php. Seem OK? @r-a-y Would be good to get your eyes on before commit. 7822.diff

#9 @boonebgorges
19 months ago

I've reviewed other instances of has_action() throughout BP, and none of them should be affected in the same way (none have had their corresponding add_action() calls moved into conditionally-loaded files).

@r-a-y
19 months ago

#10 follow-up: @r-a-y
19 months ago

Can duplicate on a fresh install.

The reason why my main development testing site failed to pick up this issue is due to the fact that I had a plugin registering on the 'bp_notification_settings' hook at all times.

Here's a cheeky idea. Why not register a dummy hook to 'bp_notification_settings' for each component needing to display the "Settings > Email" tab to appease the has_action() check? See empty.patch.

I guess the other issue is whether a plugin would need to access this <table> markup outside the "Settings > Email" page.

#11 in reply to: ↑ 10 @boonebgorges
19 months ago

Replying to r-a-y:

Can duplicate on a fresh install.

The reason why my main development testing site failed to pick up this issue is due to the fact that I had a plugin registering on the 'bp_notification_settings' hook at all times.

Here's a cheeky idea. Why not register a dummy hook to 'bp_notification_settings' for each component needing to display the "Settings > Email" tab to appease the has_action() check? See empty.patch.

I guess the other issue is whether a plugin would need to access this <table> markup outside the "Settings > Email" page.

Eesh, that is cheeky. I don't like it :) The has_action() checks are already a kludge. Your trick would make it so that has_action( 'bp_notification_settings' ) is no longer an accurate check, since anyone wanting to remove all of these notifications sections would have to remove both the notification callback *and* __return_empty_string. My suggestion is much more straightforward (though obviously a much bigger patch, and a very small step backward in your quest to reduce the amount of code loaded).

#12 @r-a-y
19 months ago

I would argue that the chances someone would want to dynamically remove the "Email" tab by using remove_action() is low. See #5441 for where we implemented has_action() and the original rationale behind it.

However, I understand that my patch was more of a last-ditch effort to preserve the lazyloading. Thought I would give it a shot! :)

Let's go with your patch.

#13 @boonebgorges
19 months ago

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

In 12087:

Move notification settings callbacks out of lazyloaded screen function files.

Loading these add_action() callbacks conditionally caused the
has_action() check in bp_settings_remove_email_subnav() to think
that no components had registered notification settings.

Props r-a-y.
Fixes #7822.

Note: See TracTickets for help on using tickets.