Add phpcs ignore comments where strict comparison is not happening#2745
Conversation
|
Warning Rate limit exceeded@Crabcyborg has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 27 minutes and 51 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (19)
WalkthroughThis PR systematically adds PHPCS ignore directives for strict comparison warnings throughout the codebase and replaces loose comparisons with strict type-checked equivalents across helpers, models, controllers, and view files. A single public API change increases method visibility. Most modifications are non-functional linting annotations; some comparisons gain explicit integer type casting for stricter evaluation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
classes/views/frm-fields/front-end/dropdown-field.php (1)
62-66: Use strict comparison for option value checks.The condition
$opt == ''should use strict comparison (=== '') to avoid treating valid option values like'0'as empty. This ensures only actual empty strings are skipped.♻️ Proposed fix
- // phpcs:ignore Universal.Operators.StrictComparisons - if ( $placeholder && $opt == '' && ! $skipped ) { + if ( $placeholder && $opt === '' && ! $skipped ) { $skipped = true; continue; }
🤖 Fix all issues with AI agents
In @classes/helpers/FrmAppHelper.php:
- Around line 1831-1833: Remove the unnecessary phpcs:ignore comment on the
is_true function: in the is_true( $value ) method remove the trailing "//
phpcs:ignore Universal.Operators.StrictComparisons" since the code uses strict
comparisons and an explicit (int) cast, so the suppression is misleading; just
keep the return expression as-is without the phpcs comment and run lint/tests to
confirm no warnings.
- Around line 3797-3807: In maybe_highlight_menu, change the loose comparison of
$_REQUEST['post_type'] to a strict comparison to match the $post->post_type ===
$post_type check: replace the != check with !==, remove the phpcs:ignore
comment, and ensure both checks compare strings strictly (i.e., use !== for
$_REQUEST['post_type'] and keep $post->post_type !== $post_type) so the function
consistently uses strict comparisons.
- Around line 3027-3030: The condition in FrmAppHelper that currently checks if
a value is missing or loose-equals an empty string (if ( ! isset( $values[ $opt
] ) || $values[ $opt ] == '' )) can incorrectly treat 0, '0' or false as empty;
update this to use a strict comparison so only an actual empty string is
replaced: change the expression to ( ! isset( $values[ $opt ] ) || $values[ $opt
] === '' ) in the block that assigns $values[$opt] from
$post_values['options'][$opt] or $default (referencing $values, $opt,
$post_values and $default in FrmAppHelper).
In @classes/helpers/FrmStylesHelper.php:
- Around line 205-208: The list-item class check uses a loose comparison; change
the conditional that builds the li class from comparing
$style->post_content['collapse_icon'] == $key to a strict comparison
($style->post_content['collapse_icon'] === $key) so the active class only
applies when both value and type match the radio input (which is validated
strictly by checked()); update the expression around the li element that
references $style->post_content['collapse_icon'] and $key accordingly.
In @classes/views/shared/mb_adv_info.php:
- Around line 64-67: The conditional in the loop uses mixed equality operators:
replace the loose comparison `$f->field_options['data_type'] == ''` with a
strict comparison `=== ''` so the if statement consistently uses strict equality
for `$f->type` and `$f->field_options['data_type']`, preventing unintended type
coercion when checking `$f->field_options['data_type']`.
In @classes/views/styles/components/templates/text-toggle.php:
- Line 27: Replace the loose equality check in the checked(...) call with a
strict comparison: remove the phpcs:ignore and compare $option['value'] and
$field_value using === after normalizing their types (for example, cast both to
string or to the expected type) so the condition becomes ( $is_default_checked
&& 0 === $index ) || (string) $option['value'] === (string) $field_value; keep
the rest of the checked(...) arguments unchanged and preserve the existing logic
around $is_default_checked and $index.
In @stripe/models/FrmTransLiteDb.php:
- Around line 38-41: The version check uses loose comparison between
$this->db_version and $old_db_version (which may be a string from get_option());
change the if in FrmTransLiteDb to use a strict comparison and explicit
casting—e.g., cast the retrieved $old_db_version (or $this->db_version) to int
and compare with ===—so replace the current loose equality check with a strict,
typed comparison involving (int)$old_db_version (or (int)$this->db_version).
🧹 Nitpick comments (31)
css/_single_theme.css.php (1)
203-203: Consider using strict comparison instead of suppressing the warning.The phpcs:ignore directive is correctly placed, but the underlying comparison
$submit_width == ''could be made stricter using===. While suppressing the warning is valid, the broader PR appears to fix comparisons in other files rather than just ignoring them. For consistency and robustness, using===is the preferred approach.Proposed fix: Use strict comparison
- width:<?php echo esc_html( ( $submit_width == '' ? 'auto' : $submit_width ) . $important ); // phpcs:ignore Universal.Operators.StrictComparisons ?>; + width:<?php echo esc_html( ( $submit_width === '' ? 'auto' : $submit_width ) . $important ); ?>;classes/views/frm-entries/errors.php (1)
11-12: Consider fixing the loose comparison instead of suppressing the linting rule.While the loose comparison
!=is safe here (sinceisset()protects against null values), suppressing PHPCS warnings without fixing the underlying issue is not ideal. This approach doesn't improve code quality—it just hides warnings.Consider one of these alternatives instead:
Use strict comparison (best practice):
if ( isset( $message ) && $message !== '' ) {Use idiomatic PHP (cleaner):
if ( ! empty( $message ) ) {The
empty()approach is particularly preferable here since it's a single function call and more concisely expresses the intent.classes/views/shared/toggle.php (1)
81-82: Consider refactoring for type safety instead of suppressing the warning.While the phpcs:ignore directive silences the warning, the loose comparison can be made more explicit and type-safe. The current logic prevents showing a label when
$on_labelis1(integer) or"1"(string), but this behavior could be clearer.Consider one of these alternatives:
- Cast to int in the comparison (preserves loose equality intent explicitly):
- <?php // phpcs:ignore Universal.Operators.StrictComparisons ?> - <?php if ( $show_labels && $on_label != 1 ) { ?> + <?php if ( $show_labels && (int) $on_label !== 1 ) { ?>
- Normalize the type earlier (better for maintainability):
At line 20, explicitly handle the type:
$on_label = $args['on_label'] ?? 1; // Normalize: if it's the default sentinel value, keep as int; otherwise treat as string label if ( 1 === $on_label || '1' === $on_label ) { $on_label = 1; }Then use strict comparison at line 82:
- <?php // phpcs:ignore Universal.Operators.StrictComparisons ?> - <?php if ( $show_labels && $on_label != 1 ) { ?> + <?php if ( $show_labels && $on_label !== 1 ) { ?>Note: If the intent is to allow
"1"to be displayed as a label, the current loose comparison prevents that. Clarifying the expected behavior would help determine the best approach.square/controllers/FrmSquareLiteActionsController.php (2)
112-116: Consider using strict comparison and removing redundancy.The
$amount == 000check is redundant afterempty($amount)sinceempty()already catches0,'0',null, and other falsy values. Additionally, suppressing the linting warning doesn't address the underlying type-safety concern in payment validation logic.♻️ Refactor to use strict comparison
- // phpcs:ignore Universal.Operators.StrictComparisons - if ( empty( $amount ) || $amount == 000 ) { + if ( empty( $amount ) ) { $response['error'] = __( 'Please specify an amount for the payment', 'formidable' ); return $response; }If you need to explicitly check for zero as distinct from empty/null, consider:
- // phpcs:ignore Universal.Operators.StrictComparisons - if ( empty( $amount ) || $amount == 000 ) { + $numeric_amount = (float) $amount; + if ( empty( $amount ) || $numeric_amount === 0.0 ) { $response['error'] = __( 'Please specify an amount for the payment', 'formidable' ); return $response; }
447-451: Prefer strict comparison with explicit type casting.The loose comparison between form IDs suppresses the linting warning but doesn't ensure type safety. For consistency with the rest of the codebase (e.g., line 32 uses strict comparison with casting), consider using strict comparison with explicit type casting.
♻️ Use strict comparison
- // phpcs:ignore Universal.Operators.StrictComparisons - if ( $params['form_id'] == $params['posted_form_id'] ) { + if ( (int) $params['form_id'] === (int) $params['posted_form_id'] ) { // This form has already been posted, so we aren't on the first page. return; }classes/models/FrmEntryValidate.php (1)
186-187: Consider using!empty($value)or strict comparison for clarity.The loose
$value != ''comparison will treat0,'0',false, andnullas empty. If this is intentional, the ignore comment is fine. However, if you only want to skip validation for actual empty strings, consider:- // phpcs:ignore Universal.Operators.StrictComparisons - if ( $value != '' ) { + if ( $value !== '' ) {Alternatively,
!empty($value)would be more idiomatic if you truly want to skip all falsy values.classes/models/fields/FrmFieldUserID.php (1)
75-76: Use strict comparison for consistency and to prevent unintended type coercion.This file already uses strict comparison at line 141 (
'' === $value) with an explicit comment: "Allow for an empty User ID. Return early to prevent it from getting set to 0." The loose comparison at line 76 ('' == $args['value']) contradicts this pattern and could allow the numeric value0to bypass validation due to type coercion, since'' == 0evaluates totrue.♻️ Suggested refactor to match line 141 pattern
- // phpcs:ignore Universal.Operators.StrictComparisons - if ( '' == $args['value'] ) { + if ( '' === $args['value'] ) { return array(); }stripe/controllers/FrmTransLiteActionsController.php (1)
503-504: LGTM! Consider strict comparison as an optional improvement.The phpcs suppression is consistent with the PR's approach. However, since both
form_idvalues are integers, using strict inequality (!==) would be more precise and avoid potential type coercion edge cases.♻️ Optional refactor to strict comparison
- // phpcs:ignore Universal.Operators.StrictComparisons - if ( empty( $previous_entry ) || $previous_entry->form_id != $field->form_id ) { + if ( empty( $previous_entry ) || $previous_entry->form_id !== $field->form_id ) { return $values; }classes/views/frm-entries/sidebar-shared.php (1)
92-93: LGTM! Consider strict comparison as an optional improvement.The phpcs suppression is appropriate for this linting cleanup PR. Since both
updated_byanduser_idare integer user IDs, using strict inequality (!==) would be more precise.♻️ Optional refactor to strict comparison
- <?php // phpcs:ignore Universal.Operators.StrictComparisons ?> - <?php if ( $entry->updated_by && $entry->updated_by != $entry->user_id ) { ?> + <?php if ( $entry->updated_by && $entry->updated_by !== $entry->user_id ) { ?> <div class="misc-pub-section">tests/phpunit/base/FrmUnitTest.php (1)
352-353: LGTM! Consider strict comparison orempty()as optional improvements.The phpcs suppression aligns with the PR's goals. Note that
$page == ''will match multiple falsy values (empty string, 0, false, null). For clarity and precision, consider using$page === ''to match only empty strings, orempty($page)if the intent is to catch any empty value.♻️ Optional refactor options
Option 1: Strict comparison if only empty string is intended
- // phpcs:ignore Universal.Operators.StrictComparisons - if ( $page == '' ) { + if ( $page === '' ) { $page = home_url( '/' ); }Option 2: Use
empty()if intent is to catch all falsy values- // phpcs:ignore Universal.Operators.StrictComparisons - if ( $page == '' ) { + if ( empty( $page ) ) { $page = home_url( '/' ); }classes/models/FrmFieldValueSelector.php (1)
287-290: LGTM! Verify intended behavior for option values.The phpcs suppression is appropriate. Note that
$value == ''will skip options with values of empty string, 0, false, or null. If0is a valid option value in your field options, this could inadvertently hide it. Consider using$value === ''if only empty strings should be skipped.♻️ Optional refactor if 0 is a valid option value
- // phpcs:ignore Universal.Operators.StrictComparisons - if ( $value == '' ) { + if ( $value === '' ) { continue; }stripe/controllers/FrmStrpLiteActionsController.php (3)
124-128: LGTM! Consider strict comparison for payment validation.The phpcs suppression is acceptable, though note that
000is simply0(octal notation). The loose comparison$amount == 000will match empty string, 0, '0', and false. For payment validation, being explicit about type would be safer. Consider using(float) $amount <= 0or$amount === '000'depending on whether$amountis expected to be a string or number at this point.♻️ Optional refactor for clarity
- // phpcs:ignore Universal.Operators.StrictComparisons - if ( empty( $amount ) || $amount == 000 ) { + if ( empty( $amount ) || (float) $amount <= 0 ) { $response['error'] = __( 'Please specify an amount for the payment', 'formidable' ); return $response; }
351-354: LGTM! Consider strict comparison for plan ID matching.The phpcs suppression is consistent with the PR approach. Since both values are Stripe plan ID strings, using strict inequality (
!==) would be more precise and avoid type coercion issues.♻️ Optional refactor to strict comparison
- // phpcs:ignore Universal.Operators.StrictComparisons - if ( $plan_opts['id'] != $settings['plan_id'] ) { + if ( $plan_opts['id'] !== $settings['plan_id'] ) { $settings['plan_id'] = FrmStrpLiteSubscriptionHelper::maybe_create_plan( $plan_opts ); }
379-383: LGTM! Consider strict comparison for form ID matching.The phpcs suppression is appropriate for this PR. Since both values are form IDs, using strict equality (
===) would prevent type coercion issues between string '123' and integer 123.♻️ Optional refactor to strict comparison
- // phpcs:ignore Universal.Operators.StrictComparisons - if ( $params['form_id'] == $params['posted_form_id'] ) { + if ( (int) $params['form_id'] === (int) $params['posted_form_id'] ) { // This form has already been posted, so we aren't on the first page. return; }classes/models/FrmFieldFormHtml.php (2)
229-231: Consider using strict comparison or simplifying the condition.The condition
( $description && $description != '' )is redundant—the first check already ensures$descriptionis truthy. If the intent is to check for non-empty strings, use$description !== ''directly, or rely solely on the truthy check. The loose comparison!=can cause unexpected behavior with values like'0'.♻️ Suggested simplification
$this->maybe_add_description_id(); $description = FrmAppHelper::maybe_kses( $this->field_obj->get_field_column( 'description' ) ); - FrmShortcodeHelper::remove_inline_conditions( ( $description && $description != '' ), 'description', $description, $this->html ); // phpcs:ignore Universal.Operators.StrictComparisons + FrmShortcodeHelper::remove_inline_conditions( $description !== '', 'description', $description, $this->html );
244-247: Use strict comparison for string checks.The condition
$description != ''should use strict comparison (!==) to avoid type coercion issues. With loose comparison, values like0orfalsecould produce unexpected results.♻️ Proposed fix
- // phpcs:ignore Universal.Operators.StrictComparisons - if ( $description != '' ) { + if ( $description !== '' ) { $this->add_element_id( 'description', 'desc' ); }classes/controllers/FrmStylesController.php (2)
229-232: Consider using strict comparison with explicit type casting.While the phpcs:ignore suppresses the warning, the loose comparison
1 == $stylewill match both integer1and string"1". For clearer intent and type safety, consider:if ( empty( $style ) || (int) $style === 1 ) {This makes the type coercion explicit and avoids potential issues with other truthy values.
245-248: Consider using strict comparison with explicit type casting.Similar to line 230, the loose comparison could be replaced with a strict one:
if ( (int) $style === 1 ) { $style = 'default'; }This provides better type safety and clearer intent.
stripe/controllers/FrmStrpLiteEventsController.php (2)
190-193: Simplify the boolean comparison.Instead of
== true, you can directly check the property's truthiness:if ( $this->invoice->cancel_at_period_end ) { $this->subscription_canceled( 'future_cancel' ); }This is more idiomatic PHP and avoids the need for the phpcs:ignore comment.
428-437: Consider using strict comparison for numeric values.The loose inequality
!=could allow unexpected type juggling. Since both$amountand$amount_refundedshould be numeric, consider:$partial = (int) $amount !== (int) $amount_refunded;This ensures type safety and removes the need for the phpcs:ignore directive.
classes/helpers/FrmFormsListHelper.php (1)
205-205: Consider using strict comparison for string equality.Since both
$statusand$form_typeare strings (sanitized withsanitize_title), you can use strict comparison:$class = $status === $form_type ? ' class="current"' : '';This removes the need for the phpcs:ignore comment and improves type safety.
classes/models/FrmForm.php (1)
1089-1091: Consider using strict comparison with explicit casting for IDs.Since both
$form->idand$values['posted_form_id']are form IDs, use explicit integer comparison:if ( (int) $form->id === (int) $values['posted_form_id'] ) {This aligns with the pattern established elsewhere in the codebase (see line 1192) and removes the need for the phpcs:ignore comment.
stripe/controllers/FrmTransLiteAppController.php (1)
90-90: Good use of strict comparison with explicit casting, but phpcs:ignore may be unnecessary.The explicit integer casting followed by strict comparison
(int) $check_payment->id !== (int) $last_payment->idis the correct pattern for comparing IDs. However, thephpcs:ignore Universal.Operators.StrictComparisonscomment appears unnecessary since you're now using strict comparison (!==).Consider removing the phpcs:ignore comment:
$new_payment = (int) $check_payment->id !== (int) $last_payment->id;stripe/models/FrmStrpLiteAuth.php (1)
336-339: Documented intentional loose comparisons for IDs and amountsThe added
Universal.Operators.StrictComparisonsignores correctly mark places where loose==/!=is intentional (form IDs from POST and Stripe amount/string sentinels), with no behavioral change. If you ever revisit this, a future refactor could normalize amounts to integers before comparison to avoid relying on==, but that’s not required for this PR.Also applies to: 373-377, 394-397, 545-547
classes/helpers/FrmFieldsHelper.php (1)
624-647:label_positionfalsy check slightly broadens the fallback conditionSwitching from an explicit non-empty check to
if ( $position )means values like0or'0'will now be treated as “no position set” and fall back to the style setting. That’s probably fine since valid positions are textual (top,none,no_label,inside), but if any caller passes'0'as a sentinel, behavior would change subtly.classes/helpers/FrmXMLHelper.php (1)
1077-1083: Stricterpost_idvspost['ID']comparison is fine; PHPStan warning is cosmeticThe new
if ( isset( $post['ID'] ) && (int) $post_id === (int) $post['ID'] )keeps the previous numeric==semantics while making the check explicit and type-safe. The PHPStan “casting to int something that's already int” warning here is mostly stylistic; if you want to silence it, you can safely drop the cast on$post_idand keep the cast on$post['ID']:Optional tweak to satisfy PHPStan without behavior change
- if ( isset( $post['ID'] ) && (int) $post_id === (int) $post['ID'] ) { + if ( isset( $post['ID'] ) && $post_id === (int) $post['ID'] ) {classes/helpers/FrmAppHelper.php (5)
1341-1354: Consider strict comparison to avoid edge case issues.The loose comparison
$value != ''with a phpcs:ignore comment could mask potential bugs. If a query variable is set to0or'0', it would incorrectly be treated as empty and trigger the fallback logic. Consider using strict comparison:- // phpcs:ignore Universal.Operators.StrictComparisons - if ( $value != '' ) { + if ( $value !== '' && $value !== null ) {This would more explicitly handle the intended empty cases while preserving numeric zero as a valid value.
2445-2453: Consider strict comparisons to avoid form value matching bugs.The loose comparisons with phpcs:ignore comments could cause unexpected behavior:
in_array( $current, $values )without the third parameter uses loose comparison, which can causein_array(0, ['string'])to return true.$values == $currentcan match different types incorrectly (e.g.,'0' == false).Consider making these strict:
- // phpcs:ignore WordPress.PHP.StrictInArray.MissingTrueStrict, Universal.Operators.StrictComparisons - return ( is_array( $values ) && in_array( $current, $values ) ) || ( ! is_array( $values ) && $values == $current ); + if ( is_array( $values ) ) { + // Convert array values to strings for comparison with form data + $values = array_map( 'strval', $values ); + $current = (string) $current; + return in_array( $current, $values, true ); + } + return (string) $values === (string) $current;This ensures type-safe comparisons while handling form data that comes as strings.
3432-3438: Use strict comparison for pagination logic.The loose comparison for page number checking is unnecessary since
$current_pshould always be numeric:- // phpcs:ignore Universal.Operators.StrictComparisons - if ( $current_p == 1 ) { + if ( $current_p === 1 || (int) $current_p === 1 ) {Or more simply, cast once at function entry:
public static function get_first_record_num( $r_count, $current_p, $p_size ) { + $current_p = (int) $current_p; - // phpcs:ignore Universal.Operators.StrictComparisons - if ( $current_p == 1 ) { + if ( $current_p === 1 ) {This avoids the phpcs:ignore while ensuring type safety.
3507-3514: Strict comparison needed for array key validation.The loose comparison
$name == ''could incorrectly treat'0'as an empty key, which is a valid array key in PHP:- // phpcs:ignore Universal.Operators.StrictComparisons - if ( $name == '' ) { + if ( $name === '' ) {Using strict comparison ensures only actual empty strings trigger the array append logic, preserving
'0'as a valid named key.
3562-3577: Use strict comparison for page matching.The loose comparison for page matching is unnecessary and could cause type coercion issues:
- // phpcs:ignore Universal.Operators.StrictComparisons - if ( $current_page != $page ) { + if ( $current_page !== $page && (string) $current_page !== (string) $page ) {Or normalize types earlier:
public static function select_current_page( $page, $current_page, $action = array() ) { + $page = (string) $page; + $current_page = (string) $current_page; - // phpcs:ignore Universal.Operators.StrictComparisons - if ( $current_page != $page ) { + if ( $current_page !== $page ) {This ensures type-safe comparison while avoiding the phpcs:ignore directive.
📜 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 (35)
classes/controllers/FrmStylesController.phpclasses/helpers/FrmAppHelper.phpclasses/helpers/FrmEntriesListHelper.phpclasses/helpers/FrmFieldsHelper.phpclasses/helpers/FrmFormsListHelper.phpclasses/helpers/FrmListHelper.phpclasses/helpers/FrmStylesHelper.phpclasses/helpers/FrmXMLHelper.phpclasses/models/FrmDb.phpclasses/models/FrmEntry.phpclasses/models/FrmEntryValidate.phpclasses/models/FrmField.phpclasses/models/FrmFieldFormHtml.phpclasses/models/FrmFieldValueSelector.phpclasses/models/FrmForm.phpclasses/models/FrmMigrate.phpclasses/models/fields/FrmFieldUrl.phpclasses/models/fields/FrmFieldUserID.phpclasses/views/frm-entries/_sidebar-shared-pub.phpclasses/views/frm-entries/errors.phpclasses/views/frm-entries/sidebar-shared.phpclasses/views/frm-fields/front-end/dropdown-field.phpclasses/views/shared/mb_adv_info.phpclasses/views/shared/toggle.phpclasses/views/styles/components/templates/text-toggle.phpcss/_single_theme.css.phpsquare/controllers/FrmSquareLiteActionsController.phpstripe/controllers/FrmStrpLiteActionsController.phpstripe/controllers/FrmStrpLiteEventsController.phpstripe/controllers/FrmTransLiteActionsController.phpstripe/controllers/FrmTransLiteAppController.phpstripe/helpers/FrmTransLiteAppHelper.phpstripe/models/FrmStrpLiteAuth.phpstripe/models/FrmTransLiteDb.phptests/phpunit/base/FrmUnitTest.php
🧰 Additional context used
🧬 Code graph analysis (4)
classes/models/FrmFieldFormHtml.php (1)
classes/helpers/FrmShortcodeHelper.php (2)
FrmShortcodeHelper(9-156)remove_inline_conditions(146-155)
classes/views/styles/components/templates/text-toggle.php (1)
classes/helpers/FrmAppHelper.php (1)
checked(2433-2437)
classes/helpers/FrmEntriesListHelper.php (1)
classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-5051)pro_is_installed(391-393)
stripe/models/FrmStrpLiteAuth.php (1)
classes/controllers/FrmFormsController.php (2)
FrmFormsController(6-3722)just_created_entry(2628-2632)
🪛 GitHub Check: PHPStan
classes/helpers/FrmXMLHelper.php
[failure] 1077-1077:
Casting to int something that's already int.
classes/models/FrmField.php
[failure] 795-795:
Casting to int something that's already int.
⏰ 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). (3)
- GitHub Check: Cypress
- GitHub Check: PHP 7.4 tests in WP 6.9
- GitHub Check: PHP 8 tests in WP 6.9
🔇 Additional comments (29)
classes/models/FrmMigrate.php (2)
566-567: LGTM: Properly annotated loose comparison.The phpcs:ignore comment is correctly placed and uses the appropriate standard name. The loose equality check on
default_templateis acknowledged by the annotation.Consider whether future refactors could adopt strict comparison (
===) if$form->default_templateis guaranteed to be an integer from the database, but the current approach is acceptable as-is.
867-868: LGTM: String comparison annotations are correct.The phpcs:ignore comment correctly suppresses the strict comparison warning for both the
!=and==operators applied to HTML strings. Since both operands are strings, loose comparison operators pose minimal type-juggling risk here, though strict operators would align with modern PHP best practices.classes/models/fields/FrmFieldUrl.php (1)
85-85: Verify that loose comparison is intentional for field validation.The phpcs:ignore comment suppresses the strict comparison warning on
$this->field->required == '1', but this approach masks rather than fixes the underlying issue. For field validation logic, explicit type checking is safer.Consider whether the comparison should instead be converted to strict comparison with explicit casting (e.g.,
(int) $this->field->required === 1or(string) $this->field->required === '1') to ensure type safety, particularly since this affects required field validation.To verify the safety of this loose comparison, could you clarify:
- What is the expected data type of
$this->field->required—is it always the string'1'or could it be the integer1?- Are there other cases in the codebase where
$this->field->requiredis compared that use strict comparison, which would indicate an inconsistent approach?You may also want to check if other similar required field checks elsewhere in the codebase use strict or loose comparison for consistency.
classes/models/FrmEntryValidate.php (4)
94-95: LGTM!The phpcs ignore comment appropriately documents the intentional use of loose comparison to handle cases where
item_keymight benull,false, or an empty string.
160-160: LGTM!Good fix—
count()always returns an integer, so using strict comparison (===) is the correct approach rather than adding an ignore comment.
170-171: LGTM!The phpcs ignore comment is appropriate here. The
requiredproperty may be stored as either'1'(string) or1(integer) depending on how the field was saved, so loose comparison handles both cases.
751-751: Visibility change is intentional and properly documented.This method's change from
privatetopublicin version 6.21 is already documented via the@sinceannotation in the docstring. The method is actively used by external code (e.g.,FrmSpamCheckWPDisallowedWords::FrmSpamCheckWPDisallowedWords()), confirming that public access is necessary and appropriate.Likely an incorrect or invalid review comment.
classes/helpers/FrmListHelper.php (1)
483-484: LGTM! phpcs:ignore annotations are appropriate.The phpcs:ignore annotations suppress strict comparison warnings for intentional loose comparisons (action parameters, mode strings, and pagination numbers). These are non-functional changes that maintain existing behavior while addressing linting concerns.
Also applies to: 538-539, 722-726, 730-734, 1279-1279
classes/views/frm-entries/_sidebar-shared-pub.php (1)
41-42: LGTM! phpcs:ignore annotation is appropriate.The annotation suppresses a strict comparison warning for datetime comparison. Since
updated_atandcreated_atare datetime strings that can be safely compared with loose equality, this non-functional linting annotation is appropriate.classes/models/FrmDb.php (2)
390-390: LGTM! Strict comparison is appropriate.Changing from loose (
==) to strict (===) comparison for string comparison between$groupand$tableis correct. Both variables contain string values from table name parsing, so strict comparison is safer and maintains identical behavior.
437-438: LGTM! phpcs:ignore annotations are appropriate.The annotations suppress strict comparison warnings for empty string checks. These loose comparisons (
== '') intentionally catch multiple falsy values (null, false, empty string, 0), which is the desired behavior for filtering out empty arguments.Also applies to: 521-522
classes/helpers/FrmEntriesListHelper.php (2)
180-180: LGTM! Strict comparison is appropriate.Changing from loose (
!=) to strict (!==) comparison for the empty string check is correct. The$svariable comes fromsanitize_text_field(), which always returns a string, so strict comparison is safer and maintains identical behavior.
382-383: LGTM! phpcs:ignore annotation is appropriate.The annotation suppresses a strict comparison warning for the column name check. This loose comparison is intentional to allow flexibility when matching action columns.
stripe/helpers/FrmTransLiteAppHelper.php (1)
222-222: The casting to int is safe and reasonable defensive programming.
interval_countis consistently initialized to1across the database schema (default), models (default1), and form fields. The Subscription model sanitizes this value withabsint(), which ensures it's always numeric. The HTML form restricts input totype="number"withmin="1", preventing null or empty string from being submitted in normal use. No evidence exists in the codebase ofinterval_countbeing intentionally set to null or empty string.classes/helpers/FrmFormsListHelper.php (1)
91-91: Good improvement to strict comparison.The change from
!=to!==ensures that the search term is checked with type safety. This prevents potential issues where0or other falsy values might be treated as empty strings.classes/models/FrmForm.php (4)
946-949: Good addition of strict integer comparison.The explicit cast
(int) $limit === 1makes the type coercion clear and ensures that both integer1and string"1"are handled correctly. This is a solid improvement over loose comparison.
1192-1194: Excellent use of strict comparison with explicit casting.This change demonstrates the correct pattern for comparing IDs: explicit type casting followed by strict comparison. This ensures type safety while handling cases where IDs might be stored as strings or integers.
156-160: Array comparison with loose equality is acceptable here.Using
!=for array comparison is a common PHP pattern that checks both structure and values. The phpcs:ignore is appropriate as!==would only check if they're the same array instance, not if they have the same content.
543-547: Array comparison with loose equality is acceptable here.Similar to line 157, the loose comparison
!=is appropriate for comparing array contents. The phpcs:ignore correctly suppresses a false positive warning.classes/models/FrmEntry.php (3)
85-88: Looseitem_keyvsnamecomparison is safely documentedAdding the
Universal.Operators.StrictComparisonsignore here is appropriate; the existing loose==comparison is intentional and behavior is unchanged.
567-573: Stricteritem_idvsentry->idcomparison looks safeCasting both sides to
intand using===keeps semantics equivalent to the prior loose compare for numeric IDs while improving type safety inget_meta.
663-668: PHPCS ignore on$limit == ''is consistent with current behaviorKeeping a loose comparison against the empty string (and just suppressing the sniff) maintains existing behavior for falsy/empty limits while documenting the intent.
stripe/models/FrmStrpLiteAuth.php (1)
212-224: Stricter form ID comparison inadd_hidden_token_fieldis correctUsing
$posted_form !== (int) $form->idafterabsinton the POSTed value makes the form ID check type-safe without changing behavior for valid numeric IDs.classes/helpers/FrmFieldsHelper.php (1)
282-286: PHPCS ignores correctly document intentional loose comparisonsAll the added
Universal.Operators.StrictComparisons(and related) ignores in these blocks simply annotate existing, intentional uses of loose==/!=and non-strictin_array(for blank messages, custom HTML, checkbox defaults, conditional logic, shortcode defaults, and legacyupdate_emailflags). Runtime behavior is unchanged and consistent with prior logic.Also applies to: 297-300, 703-705, 892-897, 1274-1276, 1352-1357, 1583-1587, 1600-1606, 2245-2249
classes/models/FrmField.php (2)
893-946: Stricter type filtering and limit handling in field queries look correctUsing
$type !== $result->typeinget_all_types_in_formis safe since both come from field type strings, and preservingif ( $limit == 1 )(plus the' LIMIT 1'check ingetAll) keeps legacy limit semantics intact while satisfying PHPCS via ignores. No behavioral regressions expected.Also applies to: 1103-1104
610-613: PHPCS ignores here correctly preserve existing loose-comparison semanticsThe added
Universal.Operators.StrictComparisonsignores induplicate(),is_required(), andis_option_value_in_object()simply document intentional non-strict comparisons used for historical flags (requiredstored as'0'/'1') and key mangling. Behavior is unchanged and consistent with prior logic.Also applies to: 639-641, 1387-1388, 1472-1474
classes/helpers/FrmXMLHelper.php (1)
792-799: PHPCS ignores correctly mark intentional loose comparisons in XML import/exportAll added
Universal.Operators.StrictComparisonsignores (placeholder migration, field creation result checks, style change detection, post-date matching, custom style export, default HTML trimming, and legacyupdate_emailflags) simply annotate existing loose==/!=usages that are relied on for backward compatibility. No runtime behavior changes are introduced.Also applies to: 814-828, 959-973, 1449-1452, 1695-1708, 1868-1874, 2218-2249
classes/helpers/FrmAppHelper.php (2)
2283-2285: Good strict comparison implementation.The change to
(int) $role === 1properly handles both string and integer inputs while maintaining type safety. This ensures that string'1'is correctly identified as the administrator role.
2323-2325: Good strict comparison for role matching.The strict comparison
$role === $needed_roleensures exact role name matching without type coercion, which is appropriate for permission checks.
Crabcyborg
deleted the
add_phpcs_ignore_comments_where_strict_comparison_is_not_happening
branch
…ere_strict_comparison_is_not_happening Add phpcs ignore comments where strict comparison is not happening
1 participant
This update also fixes some more cases where strict comparison is not being used.