-
Notifications
You must be signed in to change notification settings - Fork 92
gw-update-posts.php: Fixed issue where snippet returned ID instead of value when GPPA dynamically populates post field.
#1024
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
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.
Approved.
4d4a70f to
be97d6f
Compare
be97d6f to
dcf64ac
Compare
WalkthroughThe changes improve the handling of custom taxonomies in the Changes
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (8)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
dcf64ac to
1060730
Compare
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
gravity-forms/gw-update-posts.php (2)
145-175: Refine non-hierarchical taxonomy handling for potential name collisions.This block correctly differentiates between hierarchical and non-hierarchical taxonomies and updates term references accordingly. However, storing term names instead of slugs for non-hierarchical taxonomies can cause ambiguities if multiple terms share the same name. Consider switching to term slugs or verifying term uniqueness to mitigate potential collisions.
- $terms = $this->get_term_names_by_ids( $terms, $taxonomy ); + $terms = $this->get_term_slugs_by_ids( $terms, $taxonomy );As a follow-up, you could implement a helper method similar to “get_term_names_by_ids” that retrieves slugs instead of names. This ensures consistent handling even when term names are not unique.
205-225: Consider returning term slugs for non-hierarchical taxonomies.While retrieving and returning
$tag->nameworks, it may be safer to return$tag->slugas it’s guaranteed to be unique within the taxonomy. Multiple terms could share the same display name, causing unexpected collisions when setting post terms by name.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
gravity-forms/gw-update-posts.php(3 hunks)
🔇 Additional comments (5)
gravity-forms/gw-update-posts.php (5)
183-183: Comment is fine.The additional inline comment clarifies the rationale for passing
falsefor $wp_error and $fire_after_hooks inwp_update_post. No issues identified.
188-203: Hierarchical check is correct and efficient.This new method accurately determines if a taxonomy is hierarchical by retrieving its object and examining the
hierarchicalproperty. It’s a straightforward and valid approach.
346-349: Form ID check is clean and straightforward.This conditional correctly restricts the logic to the current form ID and avoids unintended side effects in other forms.
351-354: Early return for non-taxonomy object types.Skipping ID transformation for posts ensures only taxonomy-based fields are affected. This aligns with the intended fix for returning name values in GPPA fields when dealing with post objects.
409-414: Verify the hard-coded post ID.You are instantiating the class with
'post_id' => 1, which might be for testing or demo purposes. If this is intended for production usage, ensure post ID 1 is always valid and accessible.Are you certain this is the correct post ID in all cases, or would a dynamic approach be preferable?
1060730 to
b57f52d
Compare
veryspry
left a comment
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.
@malayladu this is untested by me, but the code looks good. I did add a couple of suggestions and a question. I'm going to go ahead and approve so that you can move this along as soon as you decide what (if anything) to do with suggestions 🙂
…of value when GPPA dynamically populates post field.
b57f52d to
9208e14
Compare
|
@veryspry I've made changes based on the feedback. Ready for review and then S&M! |
veryspry
left a comment
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.
@malayladu thank you! And also approved
Context
⛑️ Ticket(s): https://secure.helpscout.net/conversation/2844608881/77895
Summary
By default, when we use GPPA to get a term, like category of a post, it returns the Term name. However, when this snippet which is used to update posts, is also active on the website, Populate Anything will return the Term ID when getting the category of a post instead of the name. The set up is what I showed in the video.
So the expected behaviour is that, even with the snippet active on the website, Populate Anything should still return the Term Name when populating Terms of a post.
Here's a video demo of the issue
https://www.loom.com/share/afe25a1453834809aa249fc342d52205?sid=dc46a634-5708-4627-b91c-789a6109a4ce