Nouveau: Settings > Email menu item not visible

[boonebgorges]

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

[r-a-y]

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?

[DJPaul]

I'm seeing the same behaviour as Boone.

[DJPaul]

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

[DJPaul]

It reappears if you change the slug here: ​https://github.com/buddypress/BuddyPress/blob/93b68a64452b896eab8a3afb42d70cbc81a08ad8/src/bp-settings/classes/class-bp-settings-component.php#L164

[boonebgorges]

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.

[DJPaul]

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

[boonebgorges]

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​

[boonebgorges]

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]

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.

[boonebgorges]

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

[r-a-y]

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.

[boonebgorges]

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.