gpi-waiting-list.php: Fixed an issue where the waiting list message was not displaying correctly in entry views.
#1197
Conversation
… was not displaying correctly in entry views.
WalkthroughAdds a filter handler to annotate product entries with waitlist messages in order summaries and centralizes waitlist detection into a new private helper used by multiple waitlist-related methods, with guards to avoid duplicate message injection. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
gp-inventory/gpi-waiting-list.php (1)
140-155: Prefer strict comparisons + guard against missing choices.
$choice['text'] != $value/$choice['value'] != $valuecan behave oddly with type juggling; andforeach ( $field->choices as $choice )can warn if choices is null. Consider strict compares + a quickempty( $field->choices )early return.private function is_entry_item_waitlisted( $entry, $field, $value ) { if ( gp_inventory_type_choices()->is_applicable_field( $field ) ) { + if ( empty( $field->choices ) || ! is_array( $field->choices ) ) { + return false; + } foreach ( $field->choices as $choice ) { - if ( $choice['text'] != $value && $choice['value'] != $value ) { + if ( $choice['text'] !== $value && $choice['value'] !== $value ) { continue; } return (bool) gform_get_meta( $entry['id'], sprintf( 'gpi_is_waitlisted_%d_%s', $field->id, sanitize_title( $choice['value'] ) ) ); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
gp-inventory/gpi-waiting-list.php(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
gp-inventory/gpi-waiting-list.php (3)
gp-inventory/gpi-weighted-inventory.php (1)
is_applicable_field(102-104)gp-nested-forms/gpnf-require-unique-values.php (1)
is_applicable_field(80-85)gravity-forms/gw-simple-approval.php (1)
is_applicable_form(80-85)
| if ( $this->is_entry_item_waitlisted( $entry, $field, $value ) && strpos( $value, $this->waitlist_message ) === false ) { | ||
| $value .= ' ' . $this->waitlist_message; | ||
| } | ||
|
|
||
| $is_waitlisted = gform_get_meta( $entry['id'], sprintf( 'gpi_is_waitlisted_%d_%s', $field->id, sanitize_title( $choice['value'] ) ) ); | ||
| return $value; | ||
| } |
There was a problem hiding this comment.
Avoid strpos() on non-strings.
If $value is ever non-string (or null), strpos() can throw a TypeError on newer PHP versions. A lightweight cast (or is_string() check) keeps this robust.
- if ( $this->is_entry_item_waitlisted( $entry, $field, $value ) && strpos( $value, $this->waitlist_message ) === false ) {
+ $value_str = (string) $value;
+ if ( $this->is_entry_item_waitlisted( $entry, $field, $value_str ) && strpos( $value_str, $this->waitlist_message ) === false ) {
- $value .= ' ' . $this->waitlist_message;
+ $value_str .= ' ' . $this->waitlist_message;
}
- return $value;
+ return $value_str;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In gp-inventory/gpi-waiting-list.php around lines 162 to 167, the code calls
strpos($value, $this->waitlist_message) which can throw a TypeError if $value is
not a string or is null; guard the call by either casting $value to string
before using strpos (e.g. $valStr = (string) $value) or checking
is_string($value) first, then append the waitlist message only when appropriate;
ensure the function still returns the original $value type when no change is
needed.
| if ( $available <= 0 ) { | ||
| $message = $this->waitlist_message; | ||
| $field->description = '<div class="gpi-available-inventory-message" style="padding-bottom: 13px;">' . $message . '</div>' . $field->description; | ||
| $message = $this->waitlist_message; | ||
| if ( strpos( $field->description, $this->waitlist_message ) === false ) { | ||
| $field->description = '<div class="gpi-available-inventory-message" style="padding-bottom: 13px;">' . $message . '</div>' . $field->description; | ||
| } |
There was a problem hiding this comment.
strpos( $field->description, ... ) can TypeError if description is null.
Cast to string (or default to empty string) before strpos(). Also, if waitlist_message is meant to be plain text, escape it when injecting into HTML.
if ( $available <= 0 ) {
$message = $this->waitlist_message;
- if ( strpos( $field->description, $this->waitlist_message ) === false ) {
- $field->description = '<div class="gpi-available-inventory-message" style="padding-bottom: 13px;">' . $message . '</div>' . $field->description;
+ $description = (string) $field->description;
+ if ( strpos( $description, $this->waitlist_message ) === false ) {
+ $field->description = '<div class="gpi-available-inventory-message" style="padding-bottom: 13px;">' . esc_html( $message ) . '</div>' . $description;
}
$field->isWaitlisted = true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ( $available <= 0 ) { | |
| $message = $this->waitlist_message; | |
| $field->description = '<div class="gpi-available-inventory-message" style="padding-bottom: 13px;">' . $message . '</div>' . $field->description; | |
| $message = $this->waitlist_message; | |
| if ( strpos( $field->description, $this->waitlist_message ) === false ) { | |
| $field->description = '<div class="gpi-available-inventory-message" style="padding-bottom: 13px;">' . $message . '</div>' . $field->description; | |
| } | |
| if ( $available <= 0 ) { | |
| $message = $this->waitlist_message; | |
| $description = (string) $field->description; | |
| if ( strpos( $description, $this->waitlist_message ) === false ) { | |
| $field->description = '<div class="gpi-available-inventory-message" style="padding-bottom: 13px;">' . esc_html( $message ) . '</div>' . $description; | |
| } |
🤖 Prompt for AI Agents
In gp-inventory/gpi-waiting-list.php around lines 276 to 280,
strpos($field->description, $this->waitlist_message) can throw a TypeError when
description is null and the waitlist_message is injected into HTML unescaped;
ensure $field->description is treated as a string (e.g., default to '' or cast
to (string)) before calling strpos, and escape the waitlist message when
concatenating into HTML (use the appropriate escaping function such as esc_html
or htmlspecialchars) so you pass safe text into the markup.
… was not displaying correctly in entry views.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
gp-inventory/gpi-waiting-list.php (3)
289-292: Good duplicate prevention; add defensive guards for robustness.The duplicate check at line 290 correctly prevents double-injection of the waitlist message, addressing the issue mentioned in PR objectives.
Minor: Guard
strpos()against null description and escape HTML output.Line 290's
strpos($field->description, ...)can TypeError if description is null, and line 291 injects$messageinto HTML without escaping. While the current literal is safe, defensive coding and proper escaping are best practices.Apply this diff:
if ( $available <= 0 ) { $message = $this->waitlist_message; - if ( strpos( $field->description, $this->waitlist_message ) === false ) { - $field->description = '<div class="gpi-available-inventory-message" style="padding-bottom: 13px;">' . $message . '</div>' . $field->description; + $description = (string) $field->description; + if ( strpos( $description, $this->waitlist_message ) === false ) { + $field->description = '<div class="gpi-available-inventory-message" style="padding-bottom: 13px;">' . esc_html( $message ) . '</div>' . $description; } $field->isWaitlisted = true; }
162-167: Defensive guard needed:strpos()can TypeError on non-string values.If
$valueis ever non-string (null, int, array),strpos()throws a TypeError in PHP 8+. While Gravity Forms typically provides strings for product field values, adding a defensive cast or check makes this robust.Apply this diff to add type safety:
- if ( $this->is_entry_item_waitlisted( $entry, $field, $value ) && strpos( $value, $this->waitlist_message ) === false ) { - $value .= ' ' . $this->waitlist_message; + $value_str = (string) $value; + if ( $this->is_entry_item_waitlisted( $entry, $field, $value_str ) && strpos( $value_str, $this->waitlist_message ) === false ) { + $value_str .= ' ' . $this->waitlist_message; } - return $value; + return $value_str;
140-155: Add guard for missing$entry['id']to prevent notices/errors.The helper accesses
$entry['id']at lines 146 and 151 without verifying it exists. This can cause PHP notices or unexpected behavior when called with incomplete entries (e.g., preview/draft contexts, incomplete submissions).Apply this diff to add the guard:
private function is_entry_item_waitlisted( $entry, $field, $value ) { + if ( empty( $entry['id'] ) ) { + return false; + } + if ( gp_inventory_type_choices()->is_applicable_field( $field ) ) { foreach ( $field->choices as $choice ) { if ( $choice['text'] != $value && $choice['value'] != $value ) { continue; } return (bool) gform_get_meta( $entry['id'], sprintf( 'gpi_is_waitlisted_%d_%s', $field->id, sanitize_title( $choice['value'] ) ) ); } } if ( gp_inventory_type_simple()->is_applicable_field( $field ) || gp_inventory_type_advanced()->is_applicable_field( $field ) ) { return (bool) gform_get_meta( $entry['id'], sprintf( 'gpi_is_waitlisted_%d', $field->id ) ); } return false; }
🧹 Nitpick comments (1)
gp-inventory/gpi-waiting-list.php (1)
169-199: Excellent guard implementation; consider defensive typing forstrpos().The entry, product_info, field, and reference cleanup guards requested in past reviews have all been properly implemented. The method now safely handles incomplete data.
Minor: Add defensive cast for
$product['name']beforestrpos().Line 192 calls
strpos($product['name'], ...)which can throw TypeError in PHP 8+ if$product['name']is non-string. While Gravity Forms typically ensures this is a string, a defensive cast adds robustness.Optional improvement:
- if ( $this->is_entry_item_waitlisted( $entry, $field, $product['name'] ) && strpos( $product['name'], $this->waitlist_message ) === false ) { - $product['name'] .= ' ' . $this->waitlist_message; + $product_name = (string) $product['name']; + if ( $this->is_entry_item_waitlisted( $entry, $field, $product_name ) && strpos( $product_name, $this->waitlist_message ) === false ) { + $product['name'] = $product_name . ' ' . $this->waitlist_message; }
Context
⛑️ Ticket(s): https://secure.helpscout.net/conversation/3165078090/94962?viewId=3808239
Summary
Issue: The waiting list message was not displaying consistently across entry views. Choice-based fields with "Show value" enabled were missing the message on the Entry List page because only the choice text was being compared, not the value. The Entry Details order summary never showed the message because it uses its own display logic. Additionally, single product fields displayed the message twice on the form due to the filter running multiple times.
Fix: Updated
add_waitlist_message_to_entry_value()to compare against both choice text and value. Added agform_product_infofilter to handle the order summary section. Extracted the waitlist check logic into a sharedis_entry_item_waitlisted()helper to reduce duplication between the two methods. Added a duplicate check inadd_waiting_list_to_single_product()to prevent double messages.