Use strict for more array checks#2712
Conversation
WalkthroughThis pull request systematically adds strict type checking to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
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: 1
🤖 Fix all issues with AI Agents
In @classes/models/FrmEntryFormatter.php:
- Line 878: The comparison uses get_field_type() against lowercased entries in
include_extras but doesn't normalize the field type; update the comparisons to
use strtolower( $field_value->get_field_type() ) (and similarly within
is_extra_field()) so the match is case-insensitive and robust, ensuring you call
strtolower on the field type before in_array checks against
include_extras/skip_fields.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
classes/controllers/FrmAppController.phpclasses/controllers/FrmFormActionsController.phpclasses/controllers/FrmTestModeController.phpclasses/helpers/FrmAppHelper.phpclasses/helpers/FrmFormsHelper.phpclasses/models/FrmEntryFormatter.phpclasses/models/FrmEntryMeta.phpclasses/models/FrmEntryShortcodeFormatter.phpclasses/models/FrmForm.phpclasses/views/frm-entries/sidebar-shared.phpstripe/controllers/FrmTransLiteActionsController.phptests/phpunit/entries/test_FrmShowEntryShortcode.php
🧰 Additional context used
🧬 Code graph analysis (2)
classes/helpers/FrmFormsHelper.php (1)
classes/helpers/FrmFieldsHelper.php (1)
label_position(622-645)
classes/models/FrmEntryShortcodeFormatter.php (1)
classes/models/FrmEntryFormatter.php (1)
skip_fields(309-311)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: PHP 7.4 tests in WP 6.9
- GitHub Check: PHP 8 tests in WP 6.9
- GitHub Check: Cypress
- GitHub Check: PHP 8 tests in WP 6.9
- GitHub Check: PHP 7.4 tests in WP 6.9
- GitHub Check: Cypress
- GitHub Check: Cypress
- GitHub Check: PHP 7.4 tests in WP 6.9
- GitHub Check: PHP 8 tests in WP 6.9
🔇 Additional comments (16)
classes/controllers/FrmTestModeController.php (1)
221-227: Strict license-type check looks correctUsing
in_array(..., ..., true)here is appropriate:FrmAddonsController::license_type()returns string license slugs, and strict comparison avoids any accidental matches from non-string values while preserving intended behavior.classes/helpers/FrmFormsHelper.php (1)
1102-1106: Stricter label-position check is appropriate
FrmFieldsHelper::label_position()returns string positions, so adding the strict flag toin_arrayhere makes the intent clearer and prevents any cross-type matches without changing normal behavior.classes/models/FrmEntryMeta.php (1)
614-619: Year/month/day filter now correctly uses strict comparisonConstraining
$fieldwithin_array(..., ..., true)is safe here since the keys are expected to be the literal strings'year','month', or'day'. This tightens the guard without affecting valid callers.classes/controllers/FrmFormActionsController.php (1)
276-287: Strictarray_searchon action IDs is safe and clearer
$action_control->id_baseand the$default_shownentries are string slugs, so adding the strict flag toarray_searchis type-correct and avoids any future surprises with numeric-like IDs.classes/helpers/FrmAppHelper.php (1)
4423-4434: Strict filtering of reserved image option attributes is correctUsing
in_array( $key, ['type', 'class', 'data-value'], true )matches the string-only nature of custom attribute keys and makes the intent explicit without changing expected behavior.stripe/controllers/FrmTransLiteActionsController.php (1)
568-568: LGTM! Strict type checking improves type safety.The addition of strict comparison for the gateway check is a good practice. Since gateway names are always strings, this prevents potential edge cases where loose type coercion could cause unexpected behavior.
classes/models/FrmForm.php (1)
1107-1107: LGTM! Strict comparison for action validation.The strict type checking is appropriate here. The action value comes from sanitized request parameters and should always be a string, making this a safe improvement to type safety.
classes/controllers/FrmAppController.php (1)
68-68: LGTM! Strict type checking for admin action validation.The strict comparison is appropriate here. Since
$actioncomes fromFrmAppHelper::simple_get()with'sanitize_title', it will always be a string, making this a safe and beneficial change.classes/views/frm-entries/sidebar-shared.php (1)
132-132: LGTM! Strict comparison for key filtering.The strict type checking is appropriate for filtering array keys. Since array keys are always strings in this context, this change improves type safety without affecting functionality.
tests/phpunit/entries/test_FrmShowEntryShortcode.php (2)
1066-1066: LGTM! Test updated with strict comparison.Good to see the test code updated to match the strict comparison pattern used in production code. Field types are always strings, so this change is safe and maintains consistency.
1110-1110: LGTM! Consistent strict comparison in tests.This second occurrence maintains consistency with the same strict comparison pattern. The change aligns with the broader PR objective of enforcing strict type checks throughout the codebase.
classes/models/FrmEntryShortcodeFormatter.php (2)
250-250: LGTM: Strict type checking appropriately added.The strict comparison ensures exact string matching for field types, which is the correct behavior since
$this->skip_fieldscontains string literals and$field->typeis expected to be a string.
301-301: LGTM: Consistent with the change on line 250.The strict comparison is appropriate here for the same reasons as line 250. Both locations now consistently use strict type checking when filtering field types.
classes/models/FrmEntryFormatter.php (3)
670-670: LGTM: Strict type checking appropriately added.The strict comparison ensures exact string matching when checking if a field type is in the
$this->single_cell_fieldsarray. Since both the array and the field type are strings, this is the correct approach.
691-691: LGTM: Consistent with the change on line 670.The strict comparison is appropriate here for the same reasons as line 670.
865-865: LGTM: Strict type checking appropriately added.The strict comparison ensures exact string matching when checking if a field is a skip field type. Since
skip_fields()returns an array of strings andget_field_type()returns a string, this change is safe.
| */ | ||
| protected function is_extra_field_included( $field_value ) { | ||
| return in_array( $field_value->get_field_type(), $this->include_extras ); | ||
| return in_array( $field_value->get_field_type(), $this->include_extras, true ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if get_field_type() returns are compared with lowercase values consistently
# and verify case handling for field types across the codebase
# Find all field type definitions and usages
echo "=== Checking field type constant definitions ==="
rg -nP "const\s+.*FIELD.*TYPE.*=.*['\"]" --type=php -A 1
echo -e "\n=== Checking field type property assignments ==="
rg -nP "(->type|\\['type'\\])\s*=\s*['\"]" --type=php -C 2
echo -e "\n=== Checking get_field_type method implementations ==="
ast-grep --pattern 'function get_field_type($$$) { $$$ }'
echo -e "\n=== Checking other uses of include_extras ==="
rg -nP "include_extras" --type=php -C 3Repository: Strategy11/formidable-forms
Length of output: 14980
🏁 Script executed:
# Find the context around line 878 in FrmEntryFormatter.php
sed -n '860,895p' classes/models/FrmEntryFormatter.php
# Search for get_field_type method implementations
rg -n "function get_field_type" --type=php -A 3
# Check FrmField class for get_field_type
rg -n "class FrmField" --type=php -A 50 | head -100
# Find where is_extra_field_included is called to understand $field_value parameter
rg -n "is_extra_field_included" --type=php -B 5 -A 2Repository: Strategy11/formidable-forms
Length of output: 10794
🏁 Script executed:
# Find get_field_attr implementation in FrmFieldValue
sed -n '1,250p' classes/models/FrmFieldValue.php | grep -n "get_field_attr" -A 5
# Also check the broader FrmFieldValue class
rg -n "get_field_attr" classes/models/FrmFieldValue.php -A 5
# Check where field type comes from - look for field object structure
rg -n "->type\s*=" --type=php -B 2 -A 1 | head -50
# Check if there's any uppercase field type anywhere in the data flow
rg -n "type.*[A-Z]" --type=php | grep -i field | head -20Repository: Strategy11/formidable-forms
Length of output: 6099
🏁 Script executed:
# Check FrmField class for field creation and type handling
sed -n '1,100p' classes/models/FrmField.php | head -100
# Look for field type normalization or constraints
rg -n "field.*type" classes/models/FrmField.php | grep -i "lower\|upper\|case\|normalize" | head -10
# Check database schema definitions
find . -name "*migrate*" -o -name "*database*" -o -name "*schema*" | grep -i php | head -10
# Look for field type assignments in the database/import code
rg -n "\['type'\]\s*=" --type=php classes/models/ -B 2 -A 1 | head -60Repository: Strategy11/formidable-forms
Length of output: 4408
🏁 Script executed:
# Search for field creation and database migration code
find . -path ./node_modules -prune -o -name "*migrate*.php" -type f -print
# Check FrmMigrate and database related files
ls -la classes/models/FrmFormMigrator.php classes/models/FrmMigrate.php 2>/dev/null || echo "Files not found"
# Check if field type is ever validated or normalized during save
rg -n "function update\|function insert" classes/models/FrmField.php -A 10 | head -50
# Search for any place where field type is set to uppercase
rg -n "strtoupper.*type\|type.*strtoupper" --type=php | head -10
# Check how field types are used in comparisons throughout the code
rg -n "get_field_type()\s*==\|get_field_type()\s*===" --type=php | head -10
# Check all places where include_extras is used in comparisons
rg -n "in_array.*include_extras\|include_extras.*in_array" --type=php -B 2 -A 2Repository: Strategy11/formidable-forms
Length of output: 235
🏁 Script executed:
# Check the FrmMigrate file for field type handling
grep -n "type" classes/models/FrmMigrate.php | grep -i "field\|strtolower\|lower" | head -20
# Check FrmFormMigrator for field type handling
grep -n "type" classes/models/FrmFormMigrator.php | head -30
# Look for database operations with field type
rg -n "INSERT INTO.*frm_fields\|UPDATE.*frm_fields" --type=php -B 2 -A 3 | head -50
# Check for any field type case sensitivity issues in tests
rg -n "get_field_type" tests/ --type=php -B 2 -A 2 | head -40
# Check skip_fields to understand what values it compares against
rg -n "skip_fields" classes/models/FrmEntryFormatter.php -B 5 -A 10Repository: Strategy11/formidable-forms
Length of output: 5951
Case normalization needed for consistent field type comparison.
Line 878 uses strict comparison to check if get_field_type() matches values in include_extras, which is explicitly lowercased at line 324. However, get_field_type() returns the field type directly from the field object without case normalization. While field types throughout the codebase are currently lowercase, the code relies on this implicit assumption rather than enforcing it explicitly.
For safety and maintainability, either:
- Normalize the result of
get_field_type()before comparison:strtolower( $field_value->get_field_type() ) - Add a comment documenting that field types are guaranteed to be lowercase
- Normalize field types at the point they're stored/retrieved
Note that the same pattern exists at line 865 in is_extra_field(), which compares against the hardcoded lowercase values in skip_fields().
🤖 Prompt for AI Agents
In @classes/models/FrmEntryFormatter.php at line 878, The comparison uses
get_field_type() against lowercased entries in include_extras but doesn't
normalize the field type; update the comparisons to use strtolower(
$field_value->get_field_type() ) (and similarly within is_extra_field()) so the
match is case-insensitive and robust, ensuring you call strtolower on the field
type before in_array checks against include_extras/skip_fields.
…ecks Use strict for more array checks
1 participant
No description provided.