Make WordPress Core

Opened 8 years ago

Last modified 8 years ago

#40047 new enhancement

Add term_meta to WP_Term class getter method

Reported by: barryceelen's profile barryceelen Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.8
Component: Taxonomy Keywords: has-patch 2nd-opinion
Focuses: Cc:

Description

The WP_Post and WP_User classes allow getting a meta value by calling its getter function, eg. $post->my_meta_key. This functionality was omitted when introducing the WP_Term class.

Attachments (1)

40047.diff (375 bytes) - added by barryceelen 8 years ago.

Download all attachments as: .zip

Change History (6)

@barryceelen
8 years ago

#1 @barryceelen
8 years ago

  • Keywords has-patch added

#2 @barryceelen
8 years ago

Not sure whether the break; after the first switch statement is explicitly needed as it is preceded by a return.

#3 @boonebgorges
8 years ago

  • Keywords 2nd-opinion added

Thanks for the ticket and the patch, @barryceelen.

I intentionally left this feature out of the initial implementation of WP_Term because I find it odd. It has the potential to cause arbitrary backward compatibility problems, in the case where core wants to introduce a class property that matches a meta key in use by a plugin. It looks like @nacin independently thought something similar about WP_Comment (where there's also no meta support of this type): https://core.trac.wordpress.org/ticket/32619#comment:6

Is there a strong argument in favor of including it here and on comments? Or is it primarily for the (very) minor convenience, as well as consistency with user and post objects?

cc @flixos90, who mentioned this issue during WP_Term development, but whose comment I carefully ignored at the time :) https://core.trac.wordpress.org/ticket/14162#comment:21 I vaguely recall having a conversation with him about this, which may be somewhere in the Slack archives.

#4 @flixos90
8 years ago

@boonebgorges I think we need to discuss this in a broader scope as you mentioned comments do not support this either. I personally lean towards introducing this functionality, but I do not have a strong opinion for one side or the other here.
One should always be aware that any class property will take precedence. So I don't think the BC concerns are very critical here, especially since core rarely introduces new properties (since we never change the database schema).

In my own projects I often use wrapper classes for WordPress objects that make interaction with them easier, and if a type supports meta, I include this functionality as I consider it valuable. So it would not benefit me personally, but I think it's useful to anyone who uses the WP_Post/WP_User/WP_Comment/WP_Term objects as is.
On the other hand, multisite's WP_Site and WP_Network could have something like this too, for site and network options respectively (due to recent discussions between what conceptually differs between options and metadata). If these options became available in the same way as we're discussing meta, changes would be high that people would unexpectedly do many unnecessary switch_to_blog() calls. So the possibly high complexity / cost of retrieving such a value gets kind of obscured when one can assume these values are like class properties. Not a strong argument against it, but something to consider.

Maybe introducing a method get_meta() on all of these classes would be a better solution, for explicit and clear access, and they would still make it a bit easier to retrieve metadata compared to calling get_*_meta() with the object's ID.

Version 0, edited 8 years ago by flixos90 (next)

#5 @boonebgorges
8 years ago

So I don't think the BC concerns are very critical here, especially since core rarely introduces new properties (since we never change the database schema).

Perhaps not, but there's some precedent for adding properties that are not part of the schema. See eg $post->ancestors.

I'm not against having a helper method for fetching meta - get_meta() would be fine, or even using meta as the fallback in a general get() method. It's just the __get() implementation that feel inappropriate to me.

In any case, I agree that we need a general agreement about this across our object classes.

Note: See TracTickets for help on using tickets.