Skip to:
Content

BuddyPress.org

Opened 3 years ago

Closed 17 months ago

#8556 closed defect (bug) (fixed)

BP_Notifications_Notification::get - error for meta_query

Reported by: shawfactor's profile shawfactor Owned by: espellcaste's profile espellcaste
Milestone: 12.0.0 Priority: normal
Severity: normal Version:
Component: Toolbar & Notifications Keywords: needs-testing has-patch has-unit-tests
Cc:

Description

Team I have identified an error with BP_Notifications_Notification::get

When you do a meta query using the above static method the notifciation id is no longer avilable in the result as it won't cast as an integer

my code:

$meta_query = array(

array(

'key' => 'foo_key',
'compare' => 'NOT EXISTS',

)
);

$args = array(

'user_id' => $user_ids_array,
'meta_query' => $meta_query,
'per_page' => '1',
'order_by' => 'date_notified',
);


$notifications = BP_Notifications_Notification::get($args);

ithe issue saeems to be that it won't cats as an integer on line

$results[$key]->id = (int) $results[$key]->id;

Change History (16)

#1 @shawfactor
3 years ago

actually the problem is caused by the join which creates two id fields in the result. The fix is quite easy.

Simply change the original select statement to

SELECT.

$select_sql = "SELECT n.*";

So that only the $bp->notifications->table_name fields are selected

#2 @shawfactor
3 years ago

actually fixing this also requires patching the method get_where_sql in the same class to ensure that the id field is never ambiguous, when you do a join with the meta table.

Last edited 3 years ago by shawfactor (previous) (diff)

#3 @espellcaste
2 years ago

  • Component changed from Core to Toolbar & Notifications
  • Keywords needs-patch needs-testing needs-unit-tests added
  • Milestone changed from Awaiting Review to Up Next
  • Owner set to espellcaste
  • Status changed from new to accepted

#4 @espellcaste
2 years ago

  • Milestone changed from Up Next to 11.0.0

#5 @espellcaste
19 months ago

  • Milestone changed from 11.0.0 to Up Next

Bumping this to the next release.

#6 @espellcaste
17 months ago

I confirmed this issue, and I'm working on a possible solution.

This ticket was mentioned in PR #49 on buddypress/buddypress by renatonascalves.


17 months ago
#7

  • Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed

When doing a meta_query, with the NOT EXISTS param, one can get the id columns from the bp_notifications and bp_notifications_meta tables. See the example below.

This query:
{{{php
BP_Notifications_Notification::get(

[

'user_id' => 1,
'meta_query' => [

[

'key' => 'foo',
'compare' => 'NOT EXISTS'

]

],

]

);
}}}
Will create this mysql query:
{{{mysql
SELECT

*

FROM

wp_bp_notifications n
LEFT JOIN wp_bp_notifications_meta ON (n.id = wp_bp_notifications_meta.notification_id

AND wp_bp_notifications_meta.meta_key = 'foo')

WHERE

user_id IN(5)
AND component_name IN('activity', 'blogs', 'friends', 'groups', 'members', 'messages', 'xprofile')
AND is_new = 1
AND(wp_bp_notifications_meta.notification_id IS NULL)

ORDER BY

date_notified

}}}

The result ends up with two id columns. Mysql is returning the one with NULL values.

https://i0.wp.com/user-images.githubusercontent.com/19148962/209993874-69e508ea-b25d-40bd-aa51-6342c45e5ea6.png

This pull request adds a fix for that, plus unit tests.

Trac ticket: https://buddypress.trac.wordpress.org/ticket/8556

#8 @espellcaste
17 months ago

actually fixing this also requires patching the method get_where_sql in the same class to ensure that the id field is never ambiguous when you do a join with the meta table.

@shawfactor I found that not to be the case. After adding n.*, we don't get duplicate column ids anymore, so I haven't seen anything to patch related to this feedback.

renatonascalves commented on PR #49:


17 months ago
#9

changes about src/bp-activity/classes/class-bp-activity-activity.php + the corresponding Unit Test are not concerning the ticket and consist of a better way to write the same code (returning early if a condition is not matched instead of only doing something if the condition matches). So I think these changes should be removed from this PR.

1 - I'll keep the unit test since IT IS related to the task at hand (making sure the same issue is not present in the Activity component, hence the unit test). The other change was so minor that I don't see why we need to remove it out. But I'm happy to remove it if you feel strongly. I'm just not that _orthodox_ when it comes to small changes here and there ¯\_(ツ)_/¯

2 - @imath 🤔 I think this needs a bit more discussion due to its very narrow use case. Currently:

  • The default request does not return data from the bp_notifications_meta table;
  • No data is being used inside BuddyPress from the bp_notifications_meta table when using that request;
  • The data is only available when using the meta_query param using the EXISTS value.
  • When using the NOT EXISTS value, the data is null: there is no id (meta id), notification_id, meta_key, meta_value;

So it seems your suggestion is to:

  • refactor the ::get method to return this data only when using the meta_query param using the EXISTS value.

Confirm?!

renatonascalves commented on PR #49:


17 months ago
#10

By the way, I'm happy to commit number 1 in a separate SVN commit since that's your main contention.

@imath commented on PR #49:


17 months ago
#11

I confirm 🙂.

#12 @shawfactor
17 months ago

Does the proposed commit fix the underlying issue?

Ie being able to use not exists on a meta key when querying the notifications? This is vital as most other WordPress query apis support this and it is very useful if you want to send notifications to external apis

#13 @espellcaste
17 months ago

@shawfactor Yes! :)

#14 @imath
17 months ago

  • Milestone changed from Up Next to 12.0.0

#15 @espellcaste
17 months ago

In 13396:

Activity: adding test to confirm BP_Activity_Activity::get returns the proper data when using the meta_query argument.

See #8556

#16 @espellcaste
17 months ago

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

In 13397:

Notifications: returns the proper items when using the argument.

returns the notifications, together with the notification data, when the query is performed using the argument with the .

Closes https://github.com/buddypress/buddypress/pull/49
Fixes #8556

Note: See TracTickets for help on using tickets.