WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

#14766 closed task (blessed) (fixed)

New Hooks for add / update / delete / get metadata in 3.1

Reported by: sc0ttkclark Owned by: markjaquith
Milestone: 3.1 Priority: normal
Severity: normal Version: 3.0.1
Component: Plugins Keywords: has-patch
Focuses: Cc:

Description

I suggest adding new hooks for each of these functions. See the attached patch for the changes I'm suggesting here. Please note, I'm using the {ACTION}_{METATYPE}_metadata format for the filter names since some actions are being applied with the {ACTION}_{METATYPE}_meta which would cause conflicts.

What does this allow? This allows plugins to intercept this function and return their own values and / or do their own functions for Post Types, among the other data types.

Attachments (5)

meta_filters.patch (1.9 KB) - added by sc0ttkclark 4 years ago.
SVN Patch for /trunk/wp-includes/meta.php
wp-includes-meta-filters.patch (1.9 KB) - added by sc0ttkclark 4 years ago.
Updated filter to use null instead of false
wp-includes-meta-filters-refresh.patch (1.9 KB) - added by sc0ttkclark 3 years ago.
Refreshed patch to use latest revision of trunk, relative to WP root folder, and returning (bool) $check as per @scribu's suggestion
wp-includes-meta-filters-revised.patch (655 bytes) - added by sc0ttkclark 3 years ago.
Removed use of maybe_unserialized - it's been conveyed that this should be handled by the plugin itself
wp-includes-meta-filters-fixed.patch (3.0 KB) - added by sc0ttkclark 3 years ago.
Fixing delete use of $meta_value vs $_meta_value as used in other meta saving actions; Removed use of maybe_unserialize in get_metadata; Now $meta_value uses stripslashes_deep before filters are called in add / update / delete

Download all attachments as: .zip

Change History (34)

sc0ttkclark4 years ago

SVN Patch for /trunk/wp-includes/meta.php

comment:1 sc0ttkclark4 years ago

  • Version 3.0.1 deleted

comment:2 markjaquith4 years ago

  • Milestone changed from Awaiting Review to 3.1
  • Owner set to markjaquith
  • Status changed from new to assigned
  • Type changed from enhancement to task (blessed)
  • Version set to 3.0.1

I'll bless and take responsibility for this one.

comment:3 sc0ttkclark4 years ago

L'chaim!

comment:4 follow-up: sc0ttkclark4 years ago

It's been suggested by @nacin that instead of the boolean false check, it should pass null through and check against null. I'm cool with that, as generally something that returns nothing returns null.

comment:5 in reply to: ↑ 4 mikeschinkel4 years ago

+1 to the ticket.

Replying to sc0ttkclark:

It's been suggested by @nacin that instead of the boolean false check, it should pass null through and check against null. I'm cool with that, as generally something that returns nothing returns null.

What value does null provide over false?

comment:6 sc0ttkclark4 years ago

The difference being if a filter returns no value at all, null will still be returned.

comment:7 nacin4 years ago

The difference being that you can return false through the filter.

comment:8 sc0ttkclark4 years ago

True that!

sc0ttkclark4 years ago

Updated filter to use null instead of false

comment:9 scribu3 years ago

  • Keywords has-patch added; metadata removed

Two things:

  • shouldn't each filter return $check instead of true?
  • maybe it would be good if each filter had a short explicative comment above it?

comment:10 sc0ttkclark3 years ago

@scribu - Not following you on your first item, $check returns null if no filters are applied or if a filter returns nothing, if a filter returns anything else then the following line of code would run.

comment:11 scribu3 years ago

$check = apply_filters( "add_{$meta_type}_metadata" , null, $object_id, $meta_key, $meta_value, $unique ); 
if ( null !== $check ) 
  return true;

I'm saying that the third line should probably be return $check.

comment:12 scribu3 years ago

Or even return (bool) $check;

comment:13 sc0ttkclark3 years ago

@scribu - What comments do you think should be added above the filters? Previous actions / filters don't have code-specific comments above them, is this a new best-practice?

sc0ttkclark3 years ago

Refreshed patch to use latest revision of trunk, relative to WP root folder, and returning (bool) $check as per @scribu's suggestion

comment:14 scribu3 years ago

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

(In [15983]) Add filters to *_metadata() functions. Props sc0ttkclark. Fixes #14766

comment:15 nacin3 years ago

Shouldn't the onus be on the plugin to return unserialized data?

comment:16 sc0ttkclark3 years ago

I believe having the fail safe in place is a good idea, but for best-practice to be to return the data in the format desired by the function.

comment:17 nacin3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

In many locations throughout core, we simply trust the plugin to return the right data, and if they don't, then the sky may fall on their head. I don't see why the plugin can't unserialize if they need to, because in many cases they may not need to.

comment:18 sc0ttkclark3 years ago

If the standard is to do that throughout all of core, then that should be followed.

Does there need to be a new patch or can that simply just be modified by scribu or you to remove the unserialize check?

comment:19 scribu3 years ago

It would be nice if you could submit a follow-up patch.

comment:20 sc0ttkclark3 years ago

Sure will do tonight.

comment:21 sc0ttkclark3 years ago

Before I go completing the patch, anyone have anything against this:

	$check = apply_filters( "get_{$meta_type}_metadata", null, $object_id, $meta_key, $single );
	if ( null !== $check ) {
		if ( $single && is_array( $check ) )
			return $check[0];
		else
			return $check;
	}

comment:22 sc0ttkclark3 years ago

Wait a second... @nacin - you may be wrong here - this is what's happening below my code in:

	if ( isset($meta_cache[$meta_key]) ) {
		if ( $single )
			return maybe_unserialize( $meta_cache[$meta_key][0] );
		else
			return array_map('maybe_unserialize', $meta_cache[$meta_key]);
	}

So why remove my use of maybe_unserialize?

sc0ttkclark3 years ago

Removed use of maybe_unserialized - it's been conveyed that this should be handled by the plugin itself

comment:23 sc0ttkclark3 years ago

Attached revised patch - removed use of maybe_unserialize

comment:24 scribu3 years ago

We have to be consistent. So either:

apply maybe_serialize() to $meta_value before calling the filter in add_metadata() and update_metadata()

OR

remove maybe_unserialize() from get_metadata().

comment:25 scribu3 years ago

But since this is meant to be an all-or-nothing type of filter, we should go with wp-includes-meta-filters-revised.patch

comment:26 scribu3 years ago

[14308] caused a bug. $meta_value should still be stripslash()ed before being passed along to any filter.

sc0ttkclark3 years ago

Fixing delete use of $meta_value vs $_meta_value as used in other meta saving actions; Removed use of maybe_unserialize in get_metadata; Now $meta_value uses stripslashes_deep before filters are called in add / update / delete

comment:27 sc0ttkclark3 years ago

Attached patch - Fixing delete use of $meta_value vs $_meta_value as used in other meta saving actions; Removed use of maybe_unserialize in get_metadata; Now $meta_value uses stripslashes_deep before filters are called in add / update / delete

comment:28 scribu3 years ago

(In [16017]) Fix inconsistencies in metadata filters. Props sc0ttkclark. See #14766

comment:29 sc0ttkclark3 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.