Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use register_meta() correctly including full compatibility for the changes #6700

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

felixarntz
Copy link
Contributor

The register_meta() function in core has undergone several changes in the past:

  • Before 4.6, it only accepted a sanitize callback and an auth callback passed as individual parameters and registered them for all posts of all post types.
  • Before the upcoming 5.0 (i.e. currently), it accepts an array of arguments and registers that data for all posts of all post types. See https://core.trac.wordpress.org/changeset/37924.
  • Starting with 5.0, it will support registering metadata for only a specific post type. Which is what you'd actually want, because both download and payment metadata are only relevant for a subset of posts, namely those which have one of those post types. Furthermore, it is now recommended to use the wrapper function register_post_meta() when registering metadata for posts. See https://core.trac.wordpress.org/changeset/43378.

EDD should use that function correctly, depending on which WordPress version is currently in use.

Proposed Changes:

  1. Introduce a convenience wrapper method register_post_meta() which handles the above changes correctly. It ensures that the function is always called correctly, depending on the WordPress version used. The post types to register metadata can be passed to it as a string or an array. Those of course will only take effect if WordPress version 5.0 or newer is being used. Otherwise, although somewhat sub-optimal, it will register the metadata for all posts of all post types, simply because previously there was no other way to do so (this is also what the plugin currently does).
  2. Use that new register_post_meta() method in the plugin instead of calling core's register_meta() directly. This makes its usage streamlined and also allows to get rid of the manual checks to add the sanitize filters in case of pre-4.6, which happens in the current code and is quite ugly.
  3. To see, which post types need to have download metadata registered, the edd_download_metabox_post_types filter is used. This filter was previously only run in edd_add_download_meta_box() to see whether the download meta box should appear for a post type. By showing the download meta box, you essentially add support for that metadata to the respective post type, so it makes sense to use that data for metadata registration as well. For convenience, a edd_get_download_meta_post_types() function is introduced that simply runs the filter.
  4. For registering payment metadata, only the edd_payment post type is used because that is the only place where that data needs to be present at this point.

Felix Arntz added 2 commits July 1, 2018 22:09
@JJJ
Copy link
Contributor

JJJ commented Jul 2, 2018

A lot of this is obsolete for 3.0, though we should continue to register known meta keys for custom meta data database tables.

WordPress 5.0 won’t be our minimum version for a while, so we can’t rely on the new functions without also introducing our own wrappers, which defeats the purpose IMO.

EDD meta keys are namespaced, so there’s low risk of collision on a per-post-type basis.

But, in general, I want us to use the best approach available to us.

@felixarntz
Copy link
Contributor Author

felixarntz commented Jul 2, 2018

A lot of this is obsolete for 3.0, though we should continue to register known meta keys for custom meta data database tables.

Ahh, bummer I wasn't aware of this :)

WordPress 5.0 won’t be our minimum version for a while, so we can’t rely on the new functions without also introducing our own wrappers, which defeats the purpose IMO.

I'm not sure I understand. The wrapper method I introduce in the PR ensures that the way it works in 5.0 is only used when 5.0 is actually available. Could you elaborate on your concern?

@JJJ
Copy link
Contributor

JJJ commented Sep 19, 2022

I'm not sure I understand. The wrapper method I introduce in the PR ensures that the way it works in 5.0 is only used when 5.0 is actually available. Could you elaborate on your concern?

I was being a dumb-dumb. I thought at the time 5.0 had introduced a new function, not new arguments. Your wrapper approach totally makes sense.

Now that 3.0 is out and efforts are on 3.1 and beyond, it is a good idea IMO to re/view|consider|prioritize and audit all of the various core meta keys & values for registration & consistency 🤝🫶

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants