inconsistent table checks in notifications

[shawfactor]

so in the class class-bp-notifications-notification.php the check_access static function uses $bp->core->table_name_notifications whereas buddypress()->notifications->table_name. For consistency these should be the same.

This is causing problems in my setup...

Do you agree this is correct? And do you agree that buddypress()->notifications->table_name is the correct value for both?

I will create a ticket once I have confirmation on your opinion guys

[imath]

Hi @shawfactor

both should return the same value: wp_bp_notifications (when the table prefix is set to wp_). The reason about this is that Notifications before becoming an optional component were a core feature.

We've left it for back compatibility purpose in case people are still accessing directly this property.

The fix is not just a find/replace. We need to deprecate it.

What's the issue you have with it?

[johnjamesjacoby]

A quick search for $bp->core-> brings up 12 results, and all of them should probably be updated.

It is probably safe by now to simply remove this old code, but I never like the idea of totally obliterating backwards compatibility. Ya just never know.

To maintain back-compat, I can imagine adding a magic getter to BP_Core that reroutes touches to table_name_notifications to use buddypress()->notifications->table_name instead. That way everything uses the same thing.

The only reason I can think not to setup a magic getter here, is if BP_Component would ever want to introduce one, this would all need some adjusting.

[johnjamesjacoby]

Also, it's weird that BP_Core_Notification is still being used in some places. Those should be replaced in a separate ticket, specifically inside of bp_activity_at_message_notification().

[johnjamesjacoby]

1.diff does the following:

• Deprecates ->core->table_name_notifications
• Adds a magic getter to BP_Core, with a fallback if Notifications isn't active
• Audits the related queries and method params, and makes sure they work as intended, and updates them to our latest coding standards

[imath]

Awesome, thanks a lot @johnjamesjacoby 😍

[shawfactor]

Guys the current behaviour is hugely problematic for my startup as we are trying to build a SAAS solution with shared tables for all components except xprofile, but maintaining different buddypress pages on each site.

WE have achieved this by setting BP_ENABLE_MULTIBLOG, using the buddypress-multi-network plugin, and filtering most tables to be "global_tables". However $bp->core table_name is not filtered so some functionality simply does not work. eg setting notifications to read (using bp_notifications_mark_notification) uses $bp->core->table_name_notification in check_access and buddypress()->notifications->table_name to actually update the notification (and hence does not work).

if there is a way of overririding this until buddypress 7.0 I´d be appreciative if you could let me know.

In terms of the future I would advocate for a consistent approach that does not handle backwards compatability as that would minimise technical debt.

[imath]

Hi @johnjamesjacoby

Just tested the patch. It works great! Thanks a lot 👍

[imath]

In 12753:

Core: use only one global to store the Notifications DB table name

• Deprecate $bp->core->table_name_notifications.
• Add a magic getter to BP_Core, with a fallback if the Notifications component isn't active.
• Audit the related queries and method params, and makes sure they work as intended, and update them to our latest coding standards.

Props johnjamesjacoby

Fixes #8360