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

Update to use cached functions #368

Closed
wants to merge 1 commit into from

Conversation

jasonbahl
Copy link
Contributor

As part of a VIP GO code review, we were asked to update our codebase to use cached functions. We updated our controlled code, and are working to submit PRs to codebases we don't directly control

As part of a VIP GO code review, we were asked to update our codebase to use cached functions. We updated our controlled code, and are working to submit PRs to codebases we don't directly control
$terms = wp_get_object_terms( $post_id, $this->coauthor_taxonomy, array( 'fields' => 'ids' ) );
return ! empty( $terms ) && ! is_wp_error( $terms );
$term_objects = get_the_terms( $post_id, $this->coauthor_taxonomy );
$terms = ( ! is_wp_error( $term_objects ) ) ? wp_list_pluck( $term_objects, 'term_id' ) : array();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The list pluck is unnecessary here. We can just do return ! is_wp_error( $term_objects ) && ! empty( $term_objects )

@mjangda
Copy link
Member

mjangda commented Jul 20, 2016

I think we should drop the uses of wpcom_vip_get_term_by (i.e. VIP-specific functions) here (the id lookups use get_term anyway, which should be cached already?)

@bcampeau
Copy link
Contributor

For the instances where we're using id as the first param, we don't even really need get_term_by at all. get_term would be sufficient there and that's definitely a cached function.

For the other usage by slug, I'm fine with the check for the existence of wpcom_vip_get_term_by since there really isn't another way to make it a cached function.

@jasonbahl
Copy link
Contributor Author

Going through some old open PRs of mine. . .is this worth merging with some changes or should I close this?

@rebeccahum
Copy link
Contributor

Closing as wpcom_vip_get_term_by() is deprecated now, see: https://vip.wordpress.com/functions/wpcom_vip_get_term_by/

@rebeccahum rebeccahum closed this Feb 12, 2019
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.

None yet

4 participants