-
Notifications
You must be signed in to change notification settings - Fork 3
Allow adding post and term meta by passing all as the object slug #241
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
Conversation
// Fix potential errors since we're allowing `$object_slugs` to be a string or array. | ||
if ( is_string( $object_slugs ) ) { | ||
$object_slugs = [ $object_slugs ]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could just as easily be a thrown exception. I opted for fixing the potential type error and trusting the value of the type as if it were a one value array, but I'm not married to this approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one thought to add
src/meta.php
Outdated
( | ||
is_array( $object_slugs ) && | ||
count( $object_slugs ) === 1 && | ||
'all' === $object_slugs[0] | ||
) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it's worth considering ['all']
to be a valid value here. this condition would return true with ['all']
and ['all', 'a-valid-slug']
which is a little confusing but would have the same result either way.
would it be worth making it apply to all object types if $object_slugs
is equal to 'all'
vs supporting an array with all
being the first value? It would make this code a bit cleaner but I see both arguments for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The grouping wouldn't return true here based on your array, due to the count( $object_slugs ) === 1
requirement. If there are multiple slugs, it passes through to the regular handling logic.
This logic is meant to only support all
being the only value of the array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@srtfisher Just to confirm, I made this 3v4l
// Fix potential errors since we're allowing `$object_slugs` to be a string or array. | ||
if ( is_string( $object_slugs ) ) { | ||
$object_slugs = [ $object_slugs ]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😄
As it says in the title, this allows us to quickly add post meta to all registered post types by passing
all
to theregister_meta_helper
function.As a consequence of this, this also allows us to set our
post-meta.json
config like so: