WordPress.org

Make WordPress Core

Opened 4 weeks ago

Last modified 2 weeks ago

#44467 assigned defect (bug)

Ensure meta wrapper functions do not contain additional logic

Reported by: flixos90 Owned by: flixos90
Milestone: 5.0 Priority: normal
Severity: normal Version:
Component: Options, Meta APIs Keywords: 2nd-opinion has-patch needs-unit-tests
Focuses: multisite, rest-api Cc:

Description

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.

Attachments (2)

44467.diff (19.2 KB) - added by flixos90 2 weeks ago.
44467.2.diff (20.4 KB) - added by spacedmonkey 2 weeks ago.

Download all attachments as: .zip

Change History (5)

#1 @spacedmonkey
4 weeks ago

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

@flixos90
2 weeks ago

#2 @flixos90
2 weeks ago

  • Keywords has-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to 5.0
  • Owner set to flixos90
  • Status changed from new to assigned

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
2 weeks ago

#3 @spacedmonkey
2 weeks ago

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

Last edited 2 weeks ago by spacedmonkey (previous) (diff)
Note: See TracTickets for help on using tickets.