WordPress.org

Make WordPress Core

Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#10803 closed defect (bug) (fixed)

get_post_meta doesn't return an array when requested if nothing with that key exists.

Reported by: westi Owned by: westi
Milestone: 2.9 Priority: normal
Severity: normal Version: 2.8.4
Component: General Keywords:
Focuses: Cc:
PR Number:

Description (last modified by westi)

e.g. get_post_meta($this->post_id, 'unique', false)

Where no metadata for key 'unique' exist on the post returns '' instead of array('');

Attachments (1)

meta-default.patch (3.4 KB) - added by azaozz 10 years ago.

Download all attachments as: .zip

Change History (14)

#1 @westi
10 years ago

  • Description modified (diff)

#2 @westi
10 years ago

Discussed in dev-chat today.

Should return array() in this case.

#3 @westi
10 years ago

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

(In [11948]) Return correct results for both single and array cases. Fixes #10803.

#4 @Denis-de-Bernardy
10 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I might be understanding this patch incorrectly, but it seems to me that this fix prevents it from distinguishing between a post meta that does not exist and one that has yet to be generated.

until 2.8, it's possible to do:

$children = get_post_meta($post_id, _children_cache);
if ( $children === '' )
  $children = cache_children($post_id); // might now contain array()
foreach ( $children as $child ) ...

if get_post_meta() returns an empty array instead, the above code no longer works.

since we're going to break things as a result, could we not make the mess return false instead?

#5 @hakre
10 years ago

or the ability to inject the default value on function call? then the calling code can configure the functions output in this case as needed. should make things more easy in any case.

#6 @Denis-de-Bernardy
10 years ago

that would work too, yeah. leaving by default, to avoid breaking things.

#7 @Denis-de-Bernardy
10 years ago

  • Severity changed from normal to major

#8 @azaozz
10 years ago

Yes, having a default return value that can be changed would make it more flexible like get_option(). Perhaps the same is needed for get_usermeta().

#9 @westi
10 years ago

  • Keywords 2nd-opinion removed
  • Resolution set to fixed
  • Severity changed from major to normal
  • Status changed from reopened to closed

-1 to adding a default argument.

We don't need this - the function already had clear defined behaviour and it didn't conform to it.

The example code shared above relied on a bug in the code.

You asked the function to return you an array of results and it didn't instead it returned you the result you would have got for a request of a single value if one had not been set.

#10 @Denis-de-Bernardy
10 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Nonetheless, after a certain amount of time bugs tend to become features, and this is a regression.

I don't mind the idea of breaking plugins, but if we do break things my opinion is that we should fix things properly. By properly, I mean make the meta functions return false, just like get_option() and get_transient(), instead of a potentially valid value, when the meta is not found.

#11 follow-up: @azaozz
10 years ago

Having a default value is a 'convenience' for APIs that store and retrieve something. It usually saves one conditional. Instead of doing:

$my_meta = get_post_meta($post_id, 'my_field', true);
if ( empty($my_meta) )
  $my_meta = 'default-data';

we can do:

$my_meta = get_post_meta($post_id, 'my_field', true, 'default-data');

#12 in reply to: ↑ 11 @westi
10 years ago

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

Replying to Denis-de-Bernardy:

Nonetheless, after a certain amount of time bugs tend to become features, and this is a regression.

In some cases yes. In this case I disagree - the bug is clear and obvious and should be fixed so that the function respects its arguments.

Replying to azaozz:

Having a default value is a 'convenience' for APIs that store and retrieve something. It usually saves one conditional. Instead of doing:

Agreed, they are a convenience although I'm not sure they are useful for 80% of the uses of *_meta where we are storing meta data about a post.

Anyway - they are the subject for a new ticket rather than re-using a specific bug ticket which has been fixed.

#13 @dd32
10 years ago

I'm still failing to see how this breaks anything, It now abides by what it was supposed to do.

Furthurmore, Peoples plugins SHOULDNT break because of this.

If you rely upon a false return from the function, as long as you havnt used typechecking, it still works, false == , false == array(), empty(before()) == empty(after()), The logic stays the same, With the benefit that people can now safely use the function directly in themes (As it was originally intended IIRC).

Note: See TracTickets for help on using tickets.