Ensure meta wrapper functions do not contain additional logic

[flixos90]

WordPress contains numerous wrapper functions for metadata that wrap the respective add_metadata() / update_metadata() / get_metadata() / delete_metadata() calls.

Those are:

• *_post_meta()
• *_term_meta()
• *_comment_meta()
• *_user_meta()
• *_site_meta() (on multisite)

These are convenience wrappers that developers are encouraged to use in favor of the more low-level functions. However, it has become more and more common to also include small pieces of additional logic in the wrapper functions that will not be executed when calling the low-level functions. This is a problem, because the low-level functions are still called in numerous locations in core and also by plugins (usually by areas that deal with metadata in a more abstract way). Particularly, the REST API meta field classes make use of them, so here it becomes a significant problem already. This means that in those cases, crucial functionality does not get executed, for example preventing access in case of an old database version or clearing the cache. Therefore my suggestion is to remove all extra logic from all wrapper functions and instead run it via filters that become part of default-filters.php. This ensures consistency for all metadata functions and their wrappers.

The following list shows all extra functionality that meta wrapper functions in core currently contain:

• The writing post meta wrappers all ensure the metadata is only changed for the root post (not for a revision), and they all clear the cache for the respective post ID.
• All term meta wrappers prevent access to the function if the database schema is too old. The writing term meta functions furthermore all clear the cache for the respective term ID.
• The writing comment meta wrappers all clear the cache for the respective comment ID.
• All site meta wrappers prevent access to the function if the database schema is too old. The writing site meta functions furthermore all clear the cache for the respective site ID.

All of the above, except for the post revision bit, could easily become a filter or action hook:

• Checks that possibly prevent access to metadata functions can use the add_{$type}_metadata, update_{$type}_metadata and delete_{$type}_metadata filters.
• Caches for the respective objects can be cleared by using the added_{$type}_meta, updated_{$type}_meta and deleted_{$type}_meta actions.

[spacedmonkey]

I think this is a duplicate of #43813 . I think we should move the convo. Also see this ​issue with WP-CLI.

[flixos90]

After talking to @johnjamesjacoby about this, we agreed to proceed with this. Let's use this ticket here since it goes a little further than #43813. It will likely fix that as well.

44467.diff​ is the implementation I envision for it. Some notes:

• For posts, terms, comments, and sites updating the cache has been removed from the wrapper functions and instead now happens through separate actions hooked into the respective added_{$meta_type}_meta, updated_{$meta_type}_meta, and deleted_{$meta_type}_meta hooks. Those only fire on success, so they replicate the current behavior.
• For terms and sites, the checks for whether that type of metadata is supported has been removed from the wrapper functions and instead now happens through separate filters hooked into the respective get_{$meta_type}_metadata, add_{$meta_type}_metadata, update_{$meta_type}_metadata, and delete_{$meta_type}_metadata hooks.
• In order to catch all possible access to the metadata tables, particularly for the point above, four new pre-filters were introduced: get_{$meta_type}_metadata_by_mid, update_{$meta_type}_metadata_by_mid, delete_{$meta_type}_metadata_by_mid, and update_{$meta_type}_metadata_cache. Those should be merged in a separate commit before the actual changes this ticket is for.
• With the additional filters, and the pre-filters and post-set-cache-actions being hooked in everywhere, it is furthermore ensured that a few more cases than before are properly covered.
• Rerouting the post ID if it is for a revision should remain in the post meta wrappers. There is no hook to integrate this directly into the meta functions, and more importantly it shouldn't be tied to it as it would otherwise prevent plugins to easily implement metadata for post revisions.
• Checking for whether a term is shared should remain in the term meta wrappers. It is really specific, and not as foundational that you would be required to prevent access to the term meta database table because of it.

I will work on unit tests after an initial review once we agree on whether this is the approach to proceed with.

[spacedmonkey]

I was already working on a patch for this myself, so my patch is a fresh take on this.
In 44467.2.diff there are the following changes.

• Added the following actions add_metadata, update_metadata, delete_metadata.
• Add clean_object_last_changed this is a very simple function, that has a case statement in it. It is a lookup function, for each meta type to get the object type. This function has a filter in it, for better support for custom meta types.
• Added update_{$meta_type}_meta_cache filter.
• Added checks for term meta to make sure it is installed / not shared term.
• Added checks for sited meta to make sure it is installed / multisite.

My patches uses filters for all this logic, making it very easy to change / extend. This is a big patch 20k, it needs tests and testing. Would love @johnjamesjacoby and @flixos90 on this one. :D

[flixos90]

Comparing the two patches, to a large extend they are very similar. Both rely on filters to handle all the necessary functionality. Some observations / differences:

• 44467.diff​ adds filters for setting the last_changed value more granularly while 44467.2.diff​ is more abstracted. Since there is no clear pattern though on whether object types even use last_changed (core's do, but that doesn't mean all others necessarily do), I think we should keep this specific. Furthermore, the abstracted approach requires the introduction of additional actions (without including $meta_type in the name), and I don't particularly like that, given that there are already a bunch of actions being triggered for the very same thing. Furthermore, it wouldn't make sense having their names in present tense (add_metadata ...) because the existing actions triggered at the same time are named in past tense (added_{$meta_type}_meta ...)
• 44467.diff​ takes care of the functions that change metadata by meta ID. Those aren't handled in 44467.2.diff​.
• When handling term meta, 44467.diff​ does not touch the current check for a shared term, which returns a WP_Error, while 44467.2.diff​ puts that check in a filter. This would generally make sense, however WP_Error is not a possible return value of any metadata function, and in the patch's implementation would actually be cast to a boolean, which would make it true. For backward-compatibility reasons, that check should remain in the wrapper function.

Other than that, the patches work quite similar, only have a bit different function names etc. For the above reasons I suggest proceeding with 44467.diff​.

[flixos90]

44467.3.diff​ is a refreshed patch that applies cleanly against latest trunk. A few bugs have fixed, and all existing unit tests pass.

Left to do is adding some new unit tests specifically for the added filters and actions.

[pento]

Let's get this done for 5.0, as Gutenberg's heavy REST API usage is likely to cause these problems to surface in a more widespread fashion.

[flixos90]

Heads up, I plan to get this ready next Monday.

[flixos90]

44467.4.diff​ is an updated patch against the 5.0 branch.

• It removes support for site meta, since this won't be in 5.0. #45091 is a follow-up that should add support for the 5.1 release.
• It adds tests for the new integrations via the hooks.

[flixos90]

In 43729:

REST API: Move object type-specific metadata integrations from the wrapper functions to the low-level Meta API functions.

Object type-specific actions that should happen before or after modification of metadata have so far been part of the respective wrapper functions. By using action and filter hooks, this changeset ensures they are always executed, even when calling the lower-level Meta API functions directly, which the REST API does as a prime example.

Props flixos90, spacedmonkey.
Fixes #44467.

[SergeyBiryukov]

Reopening for trunk.

[jeremyfelt]

In 43982:

REST API: Move object type-specific metadata integrations from the wrapper functions to the low-level Meta API functions.

Object type-specific actions that should happen before or after modification of metadata have so far been part of the respective wrapper functions. By using action and filter hooks, this changeset ensures they are always executed, even when calling the lower-level Meta API functions directly, which the REST API does as a prime example.

Merges [43729] to trunk.

Props flixos90, spacedmonkey.
Fixes #44467.

[dlh]

#32605 was marked as a duplicate.

[flixos90]

In 44468:

Multisite: Move site-specific metadata integrations from the wrapper functions to the low-level Meta API functions.

This complements the work in [43729] and prepares site metadata for future REST API support.

Props spacedmonkey.
Fixes #45091. See #44467.