Skip to:
Content

Opened 3 years ago

Closed 17 months ago

#4551 closed enhancement (fixed)

Use WP's metadata API

Reported by: sbrajesh Owned by:
Milestone: 2.0 Priority: normal
Severity: major Version: 1.2
Component: Component - Core Keywords:
Cc: sbrajesh

Description

Hi,
I am working on some code for a friend of mine and I need to filter on the total member count.
As these values are stored in the group meta, we need to filter on groups_get_groupmeta value.

Currently groups_get_groupmeta does not allow filtering. Since BuddyPress is following the footprints of WordPress, I believe we should have this functionality in BuddyPress. WordPress metadata api does provide early filtering.

If the devs approve this, I will put a patch quickly.

Attachments (5)

meta-api.patch (36.5 KB) - added by sbrajesh 3 years ago.
Meta API patch for Bp 1.6 Trunk
4551.patch (6.3 KB) - added by boonebgorges 17 months ago.
4551.2.patch (10.2 KB) - added by johnjamesjacoby 17 months ago.
Also matches up metadata function parameters ($delete_all, etc...)
4551.3.patch (19.0 KB) - added by johnjamesjacoby 17 months ago.
Same as 2, but also converts Activity component
4551.4.patch (9.3 KB) - added by boonebgorges 17 months ago.

Download all attachments as: .zip

Change History (36)

comment:1 @boonebgorges3 years ago

I'm OK with this. Can you please make sure that our filters work just like WP's (same syntax and location)? Also, if you could patch our other meta functions (activity, xprofile, blog), that would be great :)

comment:2 @sbrajesh3 years ago

Thank You Boone.
I will be more than happy to do that :)
Will be putting the patch by tomorrow :)

comment:3 @sbrajesh3 years ago

  • Component changed from Groups to Core
  • Keywords has-patch added; needs-patch removed

Hi,
Here is a patch attached which is modeled after wordpress meta api.
This patch introduces a meta api for BuddyPress. Any plugin can leverage the api to query for meta data.

Also, It includes BP_Meta_Query class which is a derivative of Wp_Meta_Query class and works exactly in the same manner.

This patch is against r6465 on the bp 1.6 branch. Please check and let us improve it. Is it acceptable to have this short of api ? Introducing the api does reduces the code for different component meta functions.

Please note, this patch includes updates for activity,blog,group meta using the new api but not for xprofile meta as xprofile metas have a little bit different organization strategy.

Looking forward to hear the feedbacks.

Please let me know if we should have it against the 1.7 trunk?

@sbrajesh3 years ago

Meta API patch for Bp 1.6 Trunk

comment:4 @DJPaul3 years ago

In general terms, patches for new features are best against Trunk because I think it is unlikely we will add a major feature or API change into a 1.6.x point release; we've been guilty of this in the past, but we've learnt that lesson :)

Looking forward to checking out the patch once I get some time. Quick question. Are we unable to use WP_Meta_Query directly? I see the patch only implements one method.

comment:5 @sbrajesh3 years ago

Hi Paul,
Thank you for taking a look and asking.
The problem with WP_Meta_Query is it uses the function _get_meta_table in the method get_sql which does not allow filtering, so we could not pass our custom table names there. Otherwise, we can use rest of its code.

The current patch introduces _bp_get_meta_table which allows filtering. If it is possible, and we can get a patch for it in wp core, we may not even need the meta data api and just use wordpress's built in meta data api. The only issue is the use of _get_meta_table in those functions which restricts us from using it.

I will be putting a patch for 1.7 trunk too. I put the patch for bp 1.6 trunk as my Friend Hans needed it for Bp 1.6 :)

comment:6 @DJPaul3 years ago

  • Keywords needs-refresh added
  • Milestone changed from Awaiting Review to Future Release

Has a patch, but needs to be reviewed and refreshed for trunk. Moving to future release.

comment:7 @DJPaul2 years ago

  • Milestone changed from Future Release to 1.8

comment:8 @DJPaul2 years ago

sbrajesh - interested in getting this into BP 1.8? Patch needs a refresh. :)

comment:9 @r-a-y2 years ago

  • Milestone changed from 1.8 to Future Release
  • Severity changed from minor to normal
  • Summary changed from groups_get_groupmeta should allow filtering the value to Use WP's metadata API
  • Version changed from 1.6.1 to 1.2

I like the idea behind the patch, but I don't like how we're duplicating the entire WP meta API to accomplish this because we are unable to override some variables.

sbrajesh is correct that we should pass a patch upstream to filter _get_meta_table() and to the various hardcoded variables in the *_metadata() functions like this.

This would prevent so much code duplication. The other issue I can see is maintaining backpat cache with our own cache keys (if that is even necessary).

I'll post a patch upstream and will update this ticket, but until then I'm pushing this back to Future Release.

comment:10 @DJPaul2 years ago

@r-a-y, @sbrajesh

Was a ticket ever made for this on the WP core trac? I'm a bit unclear as to what parameters we'd need filterable (beyond the linked example on line 114), but happy to do it if someone can give me a hand.

comment:11 @boonebgorges17 months ago

In 7833:

Unit tests for groupmeta functions.

Precursor for migrating to WP meta API. See #4551

comment:12 @boonebgorges17 months ago

In 7834:

Updates groupmeta unit tests to work with new WP API functions as well

The WP API has some additional caching requirements, so to make the tests work
for both the old and new functions, a few additional cache-clearings are
necessary.

See #4551

comment:13 @boonebgorges17 months ago

  • Keywords needs-testing 2nd-opinion added; needs-refresh removed
  • Milestone changed from Future Release to 2.0

4551.patch is a proof-of-concept for groupmeta. The strategy will apply directly to activitymeta and forummeta.

Technical notes:

  • I have preserved all the quirks of the existing meta functions (like returning an unstructured array of values when passing a null meta key to groups_get_groupmeta()). My first goal is to get it working exactly like the existing functions, then we can worry about improvements.
  • To make WP see the table name, I put it into the $wpdb global, as DJPaul suggests above.
  • WP's functions hardcode meta_id into their SQL queries. We use id for our primary columns. To get around this, I introduce a little str_replace() filter (bp_filter_metaid_column_name()) that gets added to 'query' just before we run the core function, and removed right after. This is the least elegant part of the patch, but it works well.
  • The functions now have pretty complete unit test coverage. 4551.patch passes the tests.

Would like another set of eyes on this before moving on to the other components.

@boonebgorges17 months ago

comment:14 @johnjamesjacoby17 months ago

This is great, Boone. Really nicely done.

4551.02.patch expands on Boone's concept above, but goes further into creating an API for registering metadata tables in the BP_Component class.

Indirectly as a result of the above changes, I've introduced a BP_Component method to help handle and store global table data to match the metadata table registration process.

@johnjamesjacoby17 months ago

Also matches up metadata function parameters ($delete_all, etc...)

comment:15 @ircbot17 months ago

This ticket was mentioned in IRC in #buddypress-dev by jjj. View the logs.

comment:16 @johnjamesjacoby17 months ago

  • Keywords needs-unit-tests commit added; needs-testing 2nd-opinion removed

I'm comfortable with this getting committed into trunk.

@johnjamesjacoby17 months ago

Same as 2, but also converts Activity component

comment:17 @boonebgorges17 months ago

In 7839:

Introduce meta_tables param for BP_Component

This utility method is used to set up configuration for $wpdb integration
and use of the WordPress metadata API

Also includes restructuring of the global_tables set in BP_Component.

See #4551

Props johnjamesjacoby

comment:18 @boonebgorges17 months ago

In 7840:

Migrate groupmeta functions to the WP metadata API

See #4551

Props boonebgorges, johnjamesjacoby

comment:19 @boonebgorges17 months ago

  • Keywords commit removed

Thanks, johnjamesjacoby.

4551.4.patch is the bp-activity portion of your previous patch. It's broken a number of the existing unit tests, and in addition we should have complete unit test coverage of the functions before committing. I can try to get on this in the next few days, but I'm posting .4 here for time being to keep track of it (and in case anyone gets to it first).

In the meantime, certain group cache-priming needs to be cleaned up.

@boonebgorges17 months ago

comment:20 @boonebgorges17 months ago

In 7843:

Modify bp_update_meta_cache() for new metadata cache schema

Now that we are using the WP metadata API, our cache keys have changed, and
the logic for priming meta caches must mirror these changes.

See #4551

comment:21 @boonebgorges17 months ago

In 7844:

Bust group object cache when updating groupmeta

This is important because some group metadata gets stored with the group
object (like last activity)

See #4551

comment:22 @boonebgorges17 months ago

In 7852:

In bp_activity_delete_meta(), clear entire activity item cache when deleting all meta

This fix will become moot after migrating to WP metadata API, but I'm fixing
it just to get the unit tests to pass in the meantime :)

See #4551

comment:23 @boonebgorges17 months ago

In 7853:

Unit tests for activitymeta functions

See #4551

comment:24 @boonebgorges17 months ago

In 7854:

Migrate activity meta functions to the WP metadata API.

See #4551

Props boonebgorges, johnjamesjacoby

comment:25 @boonebgorges17 months ago

In 7856:

Add unit tests for xprofile meta functions. See #4551

comment:26 @ircbot17 months ago

This ticket was mentioned in IRC in #buddypress-dev by boonebgorges. View the logs.

comment:27 @boonebgorges17 months ago

In 7862:

Migrate xprofilemeta functions to the WP metadata API

The case of XProfile metadata is complex, because xprofile groups, fields, and
data all store their metadata in a single table, differentiated by object_type.
This complication requires additional filters on WP's core metadata queries.

See #4551

comment:28 @boonebgorges17 months ago

In 7868:

Don't run meta_value through esc_sql() in bp_blogs_update_blogmeta()

The necessary sanitization happens in $wpdb, and the esc_sql() re-adds slashes
that were removed by stripslashes(). Fixing this bug just in time to refactor
it out :)

See #4551

comment:29 @boonebgorges17 months ago

In 7869:

Add tests for blogmeta functions.

See #4551

comment:30 @boonebgorges17 months ago

In 7870:

Migrate blogmeta functions to the WP metadata API

See #4551

comment:31 @boonebgorges17 months ago

  • Keywords has-patch needs-unit-tests removed
  • Resolution set to fixed
  • Severity changed from normal to major
  • Status changed from new to closed

All of our custom meta functions have now been migrated. Hooray!

A big bonus is that we now have decent test coverage for these functions. Double hooray!

A review of metadata caching is now needed, especially for those components where it was never fully implemented in the first place (xprofile and blogs). I've opened a new ticket for those who would like to follow along: #5398

Marking as resolved.

Note: See TracTickets for help on using tickets.