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

Adds check for term before capability check #2230

Merged
merged 3 commits into from
Jun 25, 2021

Conversation

msawicki
Copy link
Contributor

@msawicki msawicki commented Jun 9, 2021

Description of the Change

The action_sync_on_object_update method that's hooked into set_object_terms needs to be able to handle an array of term slugs. The reason being is that wp_set_object_terms()'s second parameter can be string|int|array. See docs. When a string is passed, wp_set_object_terms wraps it in an array, thus becoming an array of slugs that's later passed to set_object_terms. Using the term_exists() function here provides some flexibility since it accepts ID, slug, or name. Without this check, when a slug is passed into wp_set_object_terms() (Polylang does this when setting a language) it will throw a php Notice "Trying to get property 'term_id' of non-object" in SyncManager.php:110. This can be replicated with Polylang and ElasticPress installed with the Terms Feature enabled. From there, creating a new post will give you a php notice noted above.

I don't think there is a current issue files for this but please let me know if I'm incorrect and I'll link the two.

Alternate Designs

NA

Benefits

No php Notices

Possible Drawbacks

Extra database queries in the term_exists() function but I think this a cleaner approach than various type checks.

Verification Process

I made this change in my local environment and ran several check/tests updating and adding new post content.

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Applicable Issues

NA

Changelog Entry

Added a check for term exists to get the term id by slug, id or name before a capabilities check is done.

@msawicki msawicki changed the title Adds check for taxonomy before capability check Adds check for term before capability check Jun 14, 2021
@felipeelia
Copy link
Member

Hi @msawicki, first of all, thank you very much for the Pull Request! To make the code comply with our standards, do you mind changing
if( ! $term_info ) {
to
if ( ! $term_info ) {
?
We've also merged a few things recently, so it would be great if you could update your branch as well.

Thanks!

@msawicki
Copy link
Contributor Author

@felipeelia Updated! Thanks for the feedback.

@felipeelia felipeelia self-requested a review June 25, 2021 12:12
@felipeelia felipeelia merged commit b607dd0 into 10up:develop Jun 25, 2021
@felipeelia felipeelia added this to the 3.6.0 milestone Jul 5, 2021
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