WordPress.org

Make WordPress Core

Opened 14 months ago

Closed 9 months ago

Last modified 8 months ago

#44467 closed defect (bug) (fixed)

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 has-unit-tests fixed-5.0
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 (4)

44467.diff (19.2 KB) - added by flixos90 14 months ago.
44467.2.diff (20.4 KB) - added by spacedmonkey 14 months ago.
44467.3.diff (19.3 KB) - added by flixos90 12 months ago.
44467.4.diff (19.4 KB) - added by flixos90 10 months ago.

Download all attachments as: .zip

Change History (20)

#1 @spacedmonkey
14 months ago

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

@flixos90
14 months ago

#2 @flixos90
14 months 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.

#3 @spacedmonkey
14 months 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 14 months ago by spacedmonkey (previous) (diff)

This ticket was mentioned in Slack in #core-restapi by spacedmonkey. View the logs.


13 months ago

This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.


13 months ago

#6 @flixos90
12 months ago

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.

#7 @flixos90
12 months ago

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.

Last edited 12 months ago by flixos90 (previous) (diff)

@flixos90
12 months ago

#8 @pento
11 months ago

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.

#9 @flixos90
11 months ago

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

@flixos90
10 months ago

#10 @flixos90
10 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

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.
Last edited 10 months ago by flixos90 (previous) (diff)

#11 @flixos90
10 months ago

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

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.

#12 @SergeyBiryukov
10 months ago

  • Keywords fixed-5.0 added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for trunk.

This ticket was mentioned in Slack in #core-restapi by desrosj. View the logs.


10 months ago

#14 @jeremyfelt
9 months ago

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

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.

#15 @dlh
9 months ago

#32605 was marked as a duplicate.

#16 @flixos90
8 months ago

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.

Note: See TracTickets for help on using tickets.