-
Notifications
You must be signed in to change notification settings - Fork 153
fix/duplicate-event-on-update #1744
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
base: develop
Are you sure you want to change the base?
fix/duplicate-event-on-update #1744
Conversation
WalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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.
Actionable comments posted: 2
🧹 Nitpick comments (3)
includes/Integrations/Events_Calendar/Compatibility/TEC_V6_Compatibility.php (3)
540-540: Consider action hook naming for updates.The action hook
wpuf_tec_after_create_eventnow fires for both create and update operations. This could be misleading for developers who hook into this action expecting it to only fire on event creation.Consider one of these approaches:
- Rename the action to
wpuf_tec_after_save_eventto reflect both operations- Add a separate action
wpuf_tec_after_update_eventfor updates- Add a parameter to indicate whether it was a create or update operation
Example for option 3:
- do_action( 'wpuf_tec_after_create_event', $event_id, $args, $postarr, $meta_vars, $form_id, $form_settings ); + do_action( 'wpuf_tec_after_create_event', $event_id, $args, $postarr, $meta_vars, $form_id, $form_settings, $is_update );Or maintain backward compatibility with a new action:
do_action( 'wpuf_tec_after_create_event', $event_id, $args, $postarr, $meta_vars, $form_id, $form_settings ); if ( $is_update ) { do_action( 'wpuf_tec_after_update_event', $event_id, $args, $postarr, $meta_vars, $form_id, $form_settings ); }
497-503: Consider additional validation for update operation.The update path assumes
$event_id = $post_idwhen$eventis truthy, but it doesn't verify that the update actually succeeded or that the post still exists afterward. The create path is more defensive with theinstanceof \WP_Postcheck.Consider adding validation for the update path:
if ( $is_update ) { $event = tribe_events()->where( 'id', $post_id )->set_args( $args )->save(); - $event_id = $event ? $post_id : 0; + // Verify the post exists after update + $event_id = ( $event && get_post( $post_id ) ) ? $post_id : 0; } else {This ensures the event post actually exists before proceeding with tags and terms assignment.
32-43: Nonce verification is in place upstream—consider refactoring to use passed parameters instead of direct $_POST access.The nonce verification concern is resolved: check_ajax_referer( 'wpuf_form_add' ) is called at the start of submit_post() in Frontend_Form_Ajax.php, before any form processing occurs. This protects the entire call chain leading to convert_form_data_to_orm_format().
However, lines 32 and 42 in TEC_V6_Compatibility.php directly access
$_POSTdespite receiving sanitized$form_dataas a parameter. Consider refactoringconvert_form_data_to_orm_format()to rely on the passed parameters instead of redundantly accessing$_POSTdirectly. This improves consistency and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
includes/Integrations/Events_Calendar/Compatibility/TEC_V6_Compatibility.php(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: Inspections
includes/Integrations/Events_Calendar/Compatibility/TEC_V6_Compatibility.php
[error] 1-1: Processing form data without nonce verification. Command failed: vendor/bin/phpcs includes/Integrations/Events_Calendar/Compatibility/TEC_V6_Compatibility.php -q --report=checkstyle | cs2pr --graceful-warnings
| if ( ! empty( $args['categories'] ) ) { | ||
| wp_set_post_terms( $event->ID, $args['categories'], 'tribe_events_cat', false ); | ||
| wp_set_post_terms( $event_id, $args['categories'], 'tribe_events_cat', false ); | ||
| } |
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.
🧩 Analysis chain
Update detection logic looks correct.
The logic correctly identifies update operations by checking for a positive post ID in $postarr['ID']. This is the key fix that prevents duplicate event creation.
Please verify that TEC ORM handles gracefully when where('id', $post_id) is called with a non-existent post ID. Run this script to confirm the TEC ORM behavior:
🏁 Script executed:
#!/bin/bash
# Description: Search for TEC ORM update patterns and error handling in the codebase
# Search for other usages of tribe_events()->where() to understand the pattern
rg -n -A5 -B2 "tribe_events\(\)->where\(" --type=php
# Search for TEC ORM save() method usage and return value handling
rg -n -A3 "->save\(\)" --type=php -g "*TEC*.php"Length of output: 1497
I see partial results from the script. Let me search more thoroughly for TEC ORM error handling patterns and documentation.
🏁 Script executed:
#!/bin/bash
# Search for save() method usage patterns in PHP files (fixed syntax)
rg -n -A3 'save\(\)' --type=php -g '!node_modules' | head -50
# Search for any TEC ORM documentation or comments
rg -n 'tribe_events' --type=php -B2 -A2 | grep -E '(comment|doc|/\*|return|TEC)' | head -30
# Search for the tribe_events function definition or usage context
rg -n 'where.*save' --type=php -C2Length of output: 5275
Let me search more specifically for TEC ORM patterns and type handling in this file:
🏁 Script executed:
#!/bin/bash
# Search the entire TEC_V6_Compatibility.php file for WP_Post checks and save() patterns
cat includes/Integrations/Events_Calendar/Compatibility/TEC_V6_Compatibility.php | grep -n "WP_Post\|instanceof\|save\|->where"
# Also check if there are comments about TEC ORM or tribe_events return values
cat includes/Integrations/Events_Calendar/Compatibility/TEC_V6_Compatibility.php | grep -n -i "tec\|tribe_events\|return" | head -40Length of output: 4118
Inconsistent error handling between update and create paths.
The update operation at line 498-499 only checks if $event is truthy, whereas the create operation at line 501-502 explicitly validates that $event instanceof \WP_Post. To maintain consistency and prevent potential issues if save() returns a truthy non-Post value, update the error handling:
$event = tribe_events()->where( 'id', $post_id )->set_args( $args )->save();
$event_id = ( $event instanceof \WP_Post ) ? $post_id : 0;This mirrors the type safety check used in the create path and ensures the operation truly succeeded.
🤖 Prompt for AI Agents
In includes/Integrations/Events_Calendar/Compatibility/TEC_V6_Compatibility.php
around lines 466 to 469, the update path only checks that $event is truthy while
the create path explicitly validates $event instanceof \WP_Post; modify the
update handling to mirror the create path by checking the returned $event is an
instance of \WP_Post and set $event_id to $post_id only in that case (otherwise
set to 0), ensuring type-safe success detection after calling save().
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.
🧩 Analysis chain
Verify TEC ORM API return values for save() and create().
The branching logic correctly separates update and create operations, which is the primary fix for the duplicate event issue. However, the assumptions about return values need verification:
- Line 499: Assumes
save()returns a truthy value on success - Line 502: Assumes
create()returns aWP_Postinstance on success
Please verify the TEC ORM API documentation or search for the expected return values:
🌐 Web query:
The Events Calendar ORM save method return value
💡 Result:
Short answer: The ORM's save() has a mixed return and — per The Events Calendar docs — can return NULL when updating (this is a known/being-investigated issue). See the ORM update docs and overview. [1][2]
Sources:
- Update Arguments / save() note. [1]
- ORM overview (notes save() update currently not working for all repositories). [2]
🌐 Web query:
The Events Calendar ORM create method return value
💡 Result:
-
Using the ORM: tribe_events()->set_args( $args )->create() — returns the newly created WP_Post object on success, or false on failure. [1]
-
Legacy helper: tribe_create_event( $args ) — returns the new event ID (int) on success, or false on failure. [2]
Sources: The Events Calendar docs. [1] Create Events (ORM) — API docs. [2] tribe_create_event() reference.
Fix update path: save() returns NULL on success, breaking the event_id assignment.
The create path (lines 500-502) correctly handles the return value. However, the update path has a critical flaw:
The TEC ORM's save() method can return NULL when updating, which is a known/being-investigated issue. Since line 499 checks $event ? $post_id : 0;, a NULL return (even on success) will incorrectly set $event_id = 0, defeating the purpose of this fix.
Required fix: Replace the NULL-unsafe check at line 499 with a check that verifies the update succeeded, such as checking if the post exists after the update attempt or using a different validation approach.
Close issue , Related PRO PR
Fix Description: Event Update Issue in WPUF + Events Calendar Integration
Problem Summary
Editing an existing event via the WPUF Frontend Dashboard was creating a new event instead of updating the original.
Example: Editing event 6219 resulted in new events 6233, 6239, etc.
Root Cause
In both WPUF Free and Pro, the
save_event()method always triggered create() instead of save().Buggy Logic
This caused:
The post ID to be discarded
TEC ORM to always execute create()
The update flow never executed
Solution Implemented
Update detection and proper TEC ORM update handling were added.
The fix was applied in:
Pro →
TEC_Compatibility.phpFree →
TEC_V6_Compatibility.phpUpdated Logic
Key Improvements
1. Proper Update Detection
(lines 320–322 Pro, 467–469 Free)
Checks if an event ID exists
Determines whether to update or create
2. Correct TEC ORM Operations
(lines 343–349 Pro, 492–498 Free)
Update:
tribe_events()->where('id', $post_id)->set_args($args)->save()Create:
tribe_events()->set_args($args)->create()3. Unified Event ID Handling
(lines 345–348 Pro, 494–497 Free)
Standardized
$event_idreturn valueReplaced all
$event->IDreferences with$event_idTechnical Notes
This fix correctly uses The Events Calendar ORM API:
Create:
tribe_events()->set_args($args)->create()Update:
tribe_events()->where('id', $post_id)->set_args($args)->save()Matches TEC’s documented pattern:
Impact
Summary by CodeRabbit
Release Notes