Skip to:
Content

BuddyPress.org

Opened 5 months ago

Closed 3 months ago

#8360 closed defect (bug) (fixed)

inconsistent table checks in notifications

Reported by: shawfactor Owned by: johnjamesjacoby
Milestone: 7.0.0 Priority: normal
Severity: normal Version:
Component: Core Keywords: has-patch commit
Cc:

Description

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

Attachments (1)

8360.1.diff (12.7 KB) - added by johnjamesjacoby 5 months ago.

Download all attachments as: .zip

Change History (12)

#1 @imath
5 months ago

  • Keywords reporter-feedback added
  • Milestone changed from Awaiting Review to Under Consideration

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?

This ticket was mentioned in Slack in #buddypress by imath. View the logs.


5 months ago

#3 @johnjamesjacoby
5 months ago

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.

Last edited 5 months ago by johnjamesjacoby (previous) (diff)

#4 @johnjamesjacoby
5 months ago

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

Last edited 5 months ago by johnjamesjacoby (previous) (diff)

#5 @johnjamesjacoby
5 months ago

  • Keywords has-patch added
  • Owner set to johnjamesjacoby
  • Status changed from new to assigned

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

#6 @imath
5 months ago

  • Milestone changed from Under Consideration to 7.0.0

Awesome, thanks a lot @johnjamesjacoby 😍

#7 @johnjamesjacoby
5 months ago

  • Keywords needs-testing added

#8 @shawfactor
5 months ago

  • Keywords reporter-feedback removed

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.

This ticket was mentioned in Slack in #buddypress by imath. View the logs.


4 months ago

#10 @imath
4 months ago

  • Keywords commit added; needs-testing removed

Hi @johnjamesjacoby

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

#11 @imath
3 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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

Note: See TracTickets for help on using tickets.