Treat strict in array WP phpcs rule as error#2738
Conversation
WalkthroughThis pull request systematically converts non-strict Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🤖 Fix all issues with AI agents
In @classes/models/FrmEntryMeta.php:
- Around line 157-158: Update the in_array call in FrmEntryMeta.php so it uses
strict comparison: replace in_array( $field_id, $previous_field_ids ) with
in_array( $field_id, $previous_field_ids, true ); this matches the codebase
pattern used in FrmEntryValidate.php and FrmSpamCheckDenylist.php and ensures
type-safe matching of $field_id against $previous_field_ids.
In @classes/models/FrmEntryValues.php:
- Around line 339-340: Add an inline comment above the in_array call in
FrmEntryValues that documents the intentional non-strict comparison: explain
that in_array( $field->id, $array ) is non-strict to allow matching integer
$field->id against string values produced by include_fields (which are
concatenated integer IDs and later exploded into strings) for backward
compatibility, while the second check in_array( (string) $field->field_key,
$array, true ) is intentionally strict for field keys; also note why the
phpcs:ignore suppression is needed and when this type-coercion behavior is
expected.
🧹 Nitpick comments (13)
classes/models/fields/FrmFieldType.php (1)
1875-1876: Consider using strict comparison instead of suppressing the warning.While suppressing the PHPCS warning documents the intentional non-strict behavior, using strict comparison (
trueas the third parameter) would be more type-safe. If type coercion is needed during import, consider normalizing the types before comparison.♻️ Suggested refactor to use strict comparison
- // phpcs:ignore WordPress.PHP.StrictInArray.MissingTrueStrict - if ( ! $this->field || ! $value || in_array( $value, (array) $this->field->options ) ) { + if ( ! $this->field || ! $value || in_array( $value, (array) $this->field->options, true ) ) { return $value; }If type coercion is genuinely needed, ensure the import values and field options are normalized to the same type first, then use strict comparison.
classes/helpers/FrmListHelper.php (2)
979-980: Use strict comparison for string-based column checks.Since
$column_keyand the$hiddenarray both contain string column identifiers, strict comparison would be more appropriate and type-safe here.♻️ Proposed fix
- // phpcs:ignore WordPress.PHP.StrictInArray.MissingTrueStrict - if ( in_array( $column_key, $hidden ) ) { + if ( in_array( $column_key, $hidden, true ) ) { $class[] = 'hidden'; }
1233-1234: Use strict comparison for string-based column checks.Similar to the check at Line 980,
$column_nameand the$hiddenarray contain string identifiers, so strict comparison would be more appropriate.♻️ Proposed fix
- // phpcs:ignore WordPress.PHP.StrictInArray.MissingTrueStrict - if ( in_array( $column_name, $hidden ) ) { + if ( in_array( $column_name, $hidden, true ) ) { $classes .= ' hidden'; }classes/helpers/FrmFormMigratorsHelper.php (1)
18-19: Use strict comparison for class name checks.Both
$form['class']and the$dismissedarray contain string class names, so strict comparison would be safer and more explicit about type expectations.♻️ Proposed fix
- // phpcs:ignore WordPress.PHP.StrictInArray.MissingTrueStrict - return ! empty( $dismissed ) && in_array( $form['class'], $dismissed ); + return ! empty( $dismissed ) && in_array( $form['class'], $dismissed, true );classes/controllers/FrmEntriesController.php (1)
393-394: Use strict comparison for column identifier checks.The
$prev_hiddenvalues and$meta_valuearray both contain string column identifiers. Using strict comparison would be more type-safe and explicit.♻️ Proposed fix
- // phpcs:ignore WordPress.PHP.StrictInArray.MissingTrueStrict - if ( empty( $prev_hidden ) || in_array( $prev_hidden, $meta_value ) ) { + if ( empty( $prev_hidden ) || in_array( $prev_hidden, $meta_value, true ) ) { // Don't add blank cols or process included cols. continue; }classes/models/FrmEntry.php (1)
1148-1149: Use strict comparison for integer entry ID checks.Since
$idis explicitly cast to(int)and$frm_vars['saved_entries']stores integer entry IDs (as seen throughout the codebase), strict comparison is clearly appropriate and more correct here.♻️ Proposed fix
- // phpcs:ignore WordPress.PHP.StrictInArray.MissingTrueStrict - if ( isset( $frm_vars['saved_entries'] ) && is_array( $frm_vars['saved_entries'] ) && in_array( (int) $id, $frm_vars['saved_entries'] ) ) { + if ( isset( $frm_vars['saved_entries'] ) && is_array( $frm_vars['saved_entries'] ) && in_array( (int) $id, $frm_vars['saved_entries'], true ) ) { $update = false; }classes/models/FrmForm.php (1)
360-361: Consider using strict comparison instead of suppressing the lint warning.This complex conditional has multiple concerns:
- The lint suppression suggests non-strict behavior is intentional, but the logic would benefit from strict comparison to prevent type coercion between field IDs (which could be strings or integers).
- The condition is quite complex with nested logic that could be refactored for clarity.
Since field IDs should match exactly, strict comparison would be more robust:
♻️ Suggested refactor for clarity and strictness
+ $field_not_in_existing = !in_array( $fid->id, $existing_keys, true ); + $field_in_submitted = isset( $values['frm_fields_submitted'] ) && in_array( $fid->id, $values['frm_fields_submitted'], true ); + $options_set = isset( $values['options'] ); + - // phpcs:ignore WordPress.PHP.StrictInArray.MissingTrueStrict - if ( ! in_array( $fid->id, $existing_keys ) && ( isset( $values['frm_fields_submitted'] ) && in_array( $fid->id, $values['frm_fields_submitted'] ) ) || isset( $values['options'] ) ) { + if ( $field_not_in_existing && ( $field_in_submitted || $options_set ) ) { $values['item_meta'][ $fid->id ] = ''; }classes/helpers/FrmFieldsHelper.php (3)
701-702: Consider whether strict comparison should be used here.The phpcs:ignore suggests intentional non-strict behavior when checking if a default value matches the field value. However, since field values should match exactly (especially for option selections), strict comparison would be more robust to prevent type coercion issues (e.g.,
'0' == 0).
956-961: Review non-strict comparisons in conditional logic.Both
in_arraychecks in this function are intentionally non-strict (via phpcs:ignore). For conditional logic evaluation, this could lead to unexpected behavior with type coercion. For example, if$hide_optis the string'0'and$observed_valuecontains the integer0, non-strict comparison would match when it perhaps shouldn't.Consider whether strict comparison would be more appropriate for these conditional checks.
1593-1594: Verify the need for non-strict array_search.The non-strict
array_searchis used when looking for "other" option values. Since option values should match exactly, using strict comparison would be more robust and prevent type coercion issues.classes/helpers/FrmEntriesHelper.php (1)
580-581: Consider using strict comparison for array_search.The phpcs:ignore suppresses the strict comparison warning when searching for the "other" option value in a multi-select dropdown. Since you're matching option values that should be exact, strict comparison would be more robust and prevent type coercion issues (e.g., matching
'0'with0).classes/helpers/FrmXMLHelper.php (1)
2055-2066: Consider using strict comparison instead of suppressing the warning.The phpcs:ignore directives suppress the strict in_array warning, but since
$keyand the array elements are all strings (field names), strict comparison should work correctly here.♻️ Optional refactor to use strict comparison
- // phpcs:ignore WordPress.PHP.StrictInArray.MissingTrueStrict - if ( ! is_array( $setting ) && in_array( $key, $basic_fields ) ) { + if ( ! is_array( $setting ) && in_array( $key, $basic_fields, true ) ) { // Replace old IDs with new IDs $post_content[ $key ] = str_replace( $old, $new, $setting ); - // phpcs:ignore WordPress.PHP.StrictInArray.MissingTrueStrict - } elseif ( is_array( $setting ) && in_array( $key, $array_fields ) ) { + } elseif ( is_array( $setting ) && in_array( $key, $array_fields, true ) ) { foreach ( $setting as $k => $val ) { // Replace old IDs with new IDs $post_content[ $key ][ $k ] = str_replace( $old, $new, $val );stripe/helpers/FrmTransLiteListHelper.php (1)
488-490: Consider using strict comparison instead of suppressing the warning.Since
$entry_idis explicitly cast to(int)and$this->valid_entry_idsis documented asint[](line 20), strict comparison should work correctly here.♻️ Optional refactor to use strict comparison
$entry_id = (int) $item->item_id; - // phpcs:ignore WordPress.PHP.StrictInArray.MissingTrueStrict - $entry_is_deleted = ! in_array( $entry_id, $this->valid_entry_ids ); + $entry_is_deleted = ! in_array( $entry_id, $this->valid_entry_ids, true );
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
phpcs.xmlis excluded by!**/*.xml
📒 Files selected for processing (20)
classes/controllers/FrmEntriesController.phpclasses/helpers/FrmAppHelper.phpclasses/helpers/FrmEntriesHelper.phpclasses/helpers/FrmFieldsHelper.phpclasses/helpers/FrmFormMigratorsHelper.phpclasses/helpers/FrmListHelper.phpclasses/helpers/FrmStylesPreviewHelper.phpclasses/helpers/FrmXMLHelper.phpclasses/models/FrmAddon.phpclasses/models/FrmDb.phpclasses/models/FrmEntry.phpclasses/models/FrmEntryMeta.phpclasses/models/FrmEntryValues.phpclasses/models/FrmForm.phpclasses/models/FrmFormAction.phpclasses/models/FrmFormMigrator.phpclasses/models/FrmSettings.phpclasses/models/fields/FrmFieldType.phpclasses/views/frm-form-actions/_action_inside.phpstripe/helpers/FrmTransLiteListHelper.php
⏰ 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). (6)
- GitHub Check: PHP 7.4 tests in WP 6.9
- GitHub Check: PHP 8 tests in WP 6.9
- GitHub Check: Cypress
- GitHub Check: Cypress
- GitHub Check: PHP 8 tests in WP 6.9
- GitHub Check: PHP 7.4 tests in WP 6.9
🔇 Additional comments (14)
classes/helpers/FrmStylesPreviewHelper.php (2)
325-325: LGTM: Strict comparison is appropriate here.The strict comparison for
array_searchensures exact string matching when searching for the 'edit' dependency, which is the correct behavior for dependency array lookups.
392-393: Use strict comparison inarray_search()for consistency.Line 325 already uses strict comparison (
array_search('edit', $wp_admin_dependencies, true)) for the same operation on the same dependency array structure. This location should do the same rather than suppressing the lint warning. Since$keyis documented as astringparameter, strict comparison is appropriate and avoids potential type juggling issues.$index = array_search( $key, $dependencies, true );classes/models/FrmAddon.php (1)
419-419: LGTM: Strict comparison is correct for role checking.The strict comparison ensures exact string matching when checking if a role exists in the capabilities array, preventing any unintended type coercion in role-based access control.
classes/models/FrmForm.php (1)
270-270: LGTM: Strict comparison is appropriate for field key validation.The strict comparison ensures exact string matching when checking if a key is a valid form field, preventing any type coercion issues during form updates.
classes/models/FrmSettings.php (1)
621-621: LGTM! Strict comparison is appropriate here.The strict comparison for role checking is correct, as role names are strings. This change improves type safety and is consistent with the administrator check on line 616.
classes/views/frm-form-actions/_action_inside.php (1)
56-56: LGTM! Strict comparison correctly applied.The strict comparison for checking if an event is selected in the multi-select is appropriate. Event values are strings, so this change improves type safety without affecting functionality.
classes/models/FrmFormAction.php (1)
236-236: LGTM! Strict comparison is appropriate.The strict comparison for matching action groups is correct. Since
$this->id_baseis a string (lowercased on line 145) and the group actions array contains string identifiers, strict comparison improves type safety.stripe/helpers/FrmTransLiteListHelper.php (1)
368-368: LGTM! Strict comparison correctly applied.The strict comparison for checking hidden columns is appropriate, as both the column name and hidden array contain string values.
classes/helpers/FrmAppHelper.php (3)
311-311: LGTM! Strict comparison is appropriate here.The strict comparison for string role values ('-1', 'loggedout') is correct and improves type safety.
450-451: LGTM! Intentional loose comparison for form field matching.The phpcs:ignore directive is appropriate here. Form field values often need loose comparison to match string "1" with integer 1 from different sources (POST data vs database values).
3569-3569: LGTM! Strict comparison is appropriate for action names.The strict comparison for string action values is correct and improves type safety when checking navigation state.
classes/models/FrmDb.php (1)
96-98: LGTM! Appropriate use of phpcs:ignore for null checking.The phpcs:ignore directive is correct here for intentional loose comparison when checking for null values in SQL WHERE clause construction. The whitespace cleanup also improves readability.
classes/models/FrmFormMigrator.php (2)
611-612: LGTM! Intentional loose comparison for form ID matching.The phpcs:ignore directive is appropriate here. Using loose comparison for form IDs provides backward compatibility when IDs might be stored as either strings or integers in the import tracking data.
616-617: LGTM! Consistent approach for form ID lookups.The phpcs:ignore directive is appropriate and consistent with the in_array check above. Both use loose comparison for backward compatibility with form IDs that may be stored as different types.
…cs_rule_as_error Treat strict in array WP phpcs rule as error
1 participant
I fixed a few more of the violations, and left a few I was less sure about. I put ignore comments for those so the rest of the files are still covered.