gpuid-dynamic-population.php: Fixed an issue where dynamically populating Unique ID in Nested Forms caused memory exhaustion.
#1156
Conversation
WalkthroughAdds a per-field, per-request re-entrancy guard to the Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant WP as WordPress/GF
participant Filter as gform_field_value_uid
participant UID as UID Generator
WP->>Filter: Request dynamic value (formId, fieldId)
alt processing[key] exists
Note over Filter: Re-entrancy detected
Filter-->>WP: Return existing/short-circuited value
else
Filter->>Filter: Set processing[key] = true
Filter->>UID: Generate UID (unchanged logic)
UID-->>Filter: UID value
Filter->>Filter: Unset processing[key]
Filter-->>WP: Return UID
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
gp-unique-id/gpuid-dynamic-population.php (3)
14-20: Make the guard exception-safe and the key robust (object vs array).
- Wrap the guarded section in try/finally so the flag is always cleared even if an error occurs.
- Derive the form/field IDs defensively to handle both GF_Field objects and arrays.
Apply this diff:
static $processing = array(); -$key = $field->formId . '_' . $field->id; +$form_id = isset( $field->formId ) ? $field->formId : ( isset( $field['formId'] ) ? $field['formId'] : 0 ); +$field_id = isset( $field->id ) ? $field->id : ( isset( $field['id'] ) ? $field['id'] : 0 ); +$key = $form_id . '_' . $field_id; if ( isset( $processing[ $key ] ) ) { return $value; } $processing[ $key ] = true; +try { // Update what type of unique ID you would like to generate. Accepts 'alphanumeric', 'numeric', or 'sequential'. $type_of_id = 'alphanumeric'; @@ if ( function_exists( 'gp_unique_id' ) ) { @@ $value = $gw_uid; } -} - -unset( $processing[ $key ] ); -return $value; + return $value; +} finally { + unset( $processing[ $key ] ); +}Also applies to: 38-38
29-36: Consider per-field caching instead of a single page-level cache.
static $gw_uidcaches one UID for the entire request. If multiple fields (or forms) use the uid parameter on the same page, they’ll all get the same value. If that’s not intentional, cache by form/field key.Proposed change (builds on the earlier $key/$form_id variables):
- static $gw_uid = false; + static $uid_cache = array(); @@ - if ( ! $gw_uid ) { - $gw_uid = gp_unique_id()->get_unique( $field->formId, $field ); - } - $value = $gw_uid; + if ( ! isset( $uid_cache[ $key ] ) ) { + $uid_cache[ $key ] = gp_unique_id()->get_unique( $form_id, $field ); + } + $value = $uid_cache[ $key ];Please confirm whether the current “one UID per page load” behavior is required for your use case (especially with Nested Forms). If not, the above avoids cross-field reuse.
4-4: Fix typos in comments and URL.
- “documetnation” → “documentation”
- “Paramater” → “Parameter”
- Update the docs URL path spelling.
Apply this diff:
- * https://gravitywiz.com/documetnation/gravity-forms-unique-id/ + * https://gravitywiz.com/documentation/gravity-forms-unique-id/ @@ - * 4. Set "uid" as the value for the Paramater Name setting (screenshot: https://gwiz.io/2HhtBTa). + * 4. Set "uid" as the value for the Parameter Name setting (screenshot: https://gwiz.io/2HhtBTa).Also applies to: 10-10
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
gp-unique-id/gpuid-dynamic-population.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
gp-unique-id/gpuid-dynamic-population.php (1)
gravity-forms/gw-calculated-shipping.php (1)
field(91-117)
🔇 Additional comments (1)
gp-unique-id/gpuid-dynamic-population.php (1)
14-20: Good re-entrancy guard — fixes the infinite recursion.The static processing map with a per-form/field key correctly short-circuits recursive calls and prevents the memory exhaustion. Nice, focused fix.
SebastianWiz
changed the title
gpuid-dynamic-population.php: Fixed an issue where dynamically populating Unique ID in Nested Forms caused memory exhaustion.
…lating Unique ID in Nested Forms caused memory exhaustion.
SebastianWiz
force-pushed
the
seb/fix/88583-nested-forms-uid-memory-exhaustion
branch
from
507d4e1 to
6878357
Compare
|
@SebastianWiz Could you tell me more about how the recursion was occuring? |
|
@spivurno During debugging I could see that the static variable |
|
Ah, got it. Thanks for the extra details! |
SebastianWiz
deleted the
seb/fix/88583-nested-forms-uid-memory-exhaustion
branch
Context
⛑️ Ticket(s): https://secure.helpscout.net/conversation/3056792565/88583?viewId=8172236
Summary
The
gform_field_value_uidfilter was being called recursively when dynamically populating unique IDs in GP Nested Forms, resulting in memory exhaustion.Here's Samuel's loom showing the issue: https://www.loom.com/share/123f6ad1e9ab4e3ab55ae7d1328cb835
Fix: Added recursion protection to prevent infinite filter calls