Skip to content

Add new sniff to simplify truthy checks#2751

Merged
Crabcyborg merged 2 commits into
masterfrom
add_new_sniff_to_simplify_truthy_checks
Jan 9, 2026
Merged

Add new sniff to simplify truthy checks#2751
Crabcyborg merged 2 commits into
masterfrom
add_new_sniff_to_simplify_truthy_checks

Conversation

@Crabcyborg
Copy link
Copy Markdown
Contributor

@Crabcyborg Crabcyborg commented Jan 9, 2026

This sniff tries to remove uses of empty inside of if conditions where we know that the value exists. This update only targets function parameters.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jan 9, 2026

Warning

Rate limit exceeded

@Crabcyborg has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 10 minutes and 17 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

📥 Commits

Reviewing files that changed from the base of the PR and between aef1f5b and b82f495.

📒 Files selected for processing (1)
  • phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantEmptyOnParameterSniff.php

Walkthrough

A systematic refactoring replaces empty() conditional checks with direct PHP truthiness evaluations across controllers, helpers, and models. Additionally introduces a new PHP_CodeSniffer sniff to detect and auto-fix redundant empty() calls on function parameters.

Changes

Cohort / File(s) Summary
Controllers
classes/controllers/FrmApp...Controller.php, classes/controllers/FrmDashboard...Controller.php, classes/controllers/FrmEntries...Controller.php (5 methods), classes/controllers/FrmFields...Controller.php, classes/controllers/FrmFormActions...Controller.php (3 methods), classes/controllers/FrmForms...Controller.php (2 methods), classes/controllers/FrmStyles...Controller.php, classes/controllers/FrmXML...Controller.php
Replaced empty($var) / !empty($var) checks with direct truthiness checks (! $var / $var), affecting conditional branches that determine navigation display, field counters, column handling, and form merging logic. Truthiness now governs control flow instead of explicit emptiness detection.
Helpers
classes/helpers/FrmApp...Helper.php (7 methods + 1 visibility change), classes/helpers/FrmDashboard...Helper.php, classes/helpers/FrmFields...Helper.php, classes/helpers/FrmForms...Helper.php (4 methods), classes/helpers/FrmStyles...Helper.php, classes/helpers/FrmXML...Helper.php, stripe/helpers/FrmTransLiteApp...Helper.php
Replaced empty() / !empty() checks with truthiness evaluation across permission handling, HTML attribute processing, style application, and field rendering. One signature change: FrmAppHelper::generate_new_key promoted from private to public. Enhanced nonce handling in permission_nonce_error.
Models
classes/models/FrmCreate...File.php, classes/models/FrmDb.php (2 methods), classes/models/FrmEntry.php, classes/models/FrmEntry...Validate.php (3 methods), classes/models/FrmField.php (3 methods), classes/models/FrmForm...Api.php (2 methods), classes/models/FrmInstaller...Skin.php, classes/models/FrmSolution.php, classes/models/fields/FrmField...Default.php, classes/models/fields/FrmField...Type.php, stripe/models/FrmStrpLite...Auth.php
Refactored conditional guards from explicit empty() checks to boolean truthiness across file handling, database queries, entry validation, field type detection, and authentication flows. Alters how falsy values trigger default assignments or early returns.
New Sniff
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/Redundant...EmptyOnParameter...Sniff.php
Adds new PHP_CodeSniffer sniff implementing automatic detection and fixing of redundant empty() calls on function parameters. Provides two public methods (register, process) and three helper methods to identify and replace empty($param) / !empty($param) with direct truthiness expressions. Includes logic to handle negation, nested context verification, and safe token manipulation.
Tests
tests/phpunit/emails/test_FrmEmail.php
Minor semantic refinement in private test helper; replaced empty($property) with ! $property for consistency.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

  • Simplify some array checks #2722 — Applies the same empty() → truthiness refactor pattern across overlapping files (FrmFieldsController, FrmFormActionsController, FrmFormsController, FrmXMLController, FrmAppHelper).

  • Do not output invalid css vars #2296 — Modifies FrmStylesHelper::output_vars, which is also affected by this PR's guard condition change from empty($vars) to ! $vars.

  • Run some rector changes #2509 — Performs related refactoring-style conditional and value handling changes across FrmAppHelper.php and other overlapping controller/helper/model files.

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Description check ❓ Inconclusive No description was provided by the author, making it impossible to verify relevance to the changeset. Add a brief description explaining the purpose of the new sniff and why the empty() to truthiness refactoring across files is necessary.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: adding a new PHP_CodeSniffer sniff to simplify truthy checks by replacing redundant empty() calls.
Docstring Coverage ✅ Passed Docstring coverage is 98.21% which is sufficient. The required threshold is 80.00%.

✏️ 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
classes/helpers/FrmStylesHelper.php (1)

373-383: Fix apparent PHP parse errors (extra ) in conditionals).

As written, these if/elseif lines won’t parse.

Proposed fix
-		if ( str_starts_with( $hex, 'rgba(' ) ) {
+		if ( str_starts_with( $hex, 'rgba(' ) ) {
 			$rgba                   = str_replace( ')', '', str_replace( 'rgba(', '', $hex ) );
 			list ( $r, $g, $b, $a ) = array_map( 'trim', explode( ',', $rgba ) );
 			$r                      = max( 0, min( 255, $r + $steps ) );
 			$g                      = max( 0, min( 255, $g + $steps ) );
 			$b                      = max( 0, min( 255, $b + $steps ) );
 			return 'rgba(' . $r . ',' . $g . ',' . $b . ',' . $a . ')';
 		}

...

-		} elseif ( str_contains( $color, 'rgb(' ) ) {
+		} elseif ( str_contains( $color, 'rgb(' ) ) {
 			$color = str_replace( 'rgb(', 'rgba(', $color );
 			$color = str_replace( ')', ',1)', $color );

Also applies to: 861-872

classes/helpers/FrmAppHelper.php (1)

2793-2807: generate_new_key: current max can produce $num_chars + 1 length and needs input validation now that it’s public.
Example: $num_chars = 2 ⇒ max 36**2 = 1296base_convert(1296, 10, 36) === "100" (3 chars). Also, $num_chars < 2 can make random_int($min, $max) invalid.

Proposed fix (range correctness + clamp)
 public static function generate_new_key( $num_chars ) {
-	$max_slug_value = 36 ** $num_chars;
-
-	// We want to have at least 2 characters in the slug.
-	$min_slug_value = 37;
-	return base_convert( random_int( $min_slug_value, $max_slug_value ), 10, 36 );
+	$num_chars = max( 2, (int) $num_chars );
+
+	// Base-36 values with exactly $num_chars digits are in:
+	// [ 36**($num_chars-1), (36**$num_chars) - 1 ].
+	$min_slug_value = 36 ** ( $num_chars - 1 );
+	$max_slug_value = ( 36 ** $num_chars ) - 1;
+
+	return base_convert( random_int( $min_slug_value, $max_slug_value ), 10, 36 );
 }
🤖 Fix all issues with AI agents
In @classes/controllers/FrmFieldsController.php:
- Around line 92-97: The check before merging field options doesn't guard
against non-array input so array_merge in include_new_field can throw a
TypeError; update the condition to verify $field_options is an array (e.g.,
is_array($field_options)) before calling array_merge and otherwise ignore or
cast/normalize it; locate the include_new_field method and the array_merge call
on $field_values['field_options'] (after FrmFieldsHelper::setup_new_vars) and
ensure only arrays are merged.

In @classes/models/FrmField.php:
- Line 1096: The change from using ! empty($order_by) to checking $order_by
directly causes the string "0" to be treated as false and skips prepending '
ORDER BY '; restore the previous semantics by using ! empty($order_by) (or
explicitly check $order_by !== '' && $order_by !== null) in the conditional
around str_contains so that "0" and similar non-empty strings are still treated
as present when building the ORDER BY clause in FrmField.php (the condition that
currently reads if ( $order_by && ! str_contains( $order_by, 'ORDER BY' ) )
should be updated).

In
@phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantEmptyOnParameterSniff.php:
- Around line 1-16: The file
Formidable_Sniffs_CodeAnalysis_RedundantEmptyOnParameterSniff has PHP CS Fixer
violations (phpdoc_types_order and blank_line_before_statement); run
./vendor/bin/php-cs-fixer fix against this file or manually reorder types in any
PHPDoc blocks and add the required blank lines before statements so phpdoc type
annotations follow the configured order and a blank line is present where the
fixer expects (adjust docblocks and spacing in the
Formidable_Sniffs_CodeAnalysis_RedundantEmptyOnParameterSniff file/namespace).
🧹 Nitpick comments (8)
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantEmptyOnParameterSniff.php (2)

264-269: Remove unreachable condition.

Lines 266-268 are dead code. If execution reaches this point, $code cannot be T_OPEN_PARENTHESIS because that case was already handled and returned at line 249-262. The check $code !== T_OPEN_PARENTHESIS is always true here.

♻️ Proposed fix
 			// If we hit an open parenthesis, check if it belongs to if/elseif.
 			if ( $code === T_OPEN_PARENTHESIS ) {
 				// Check what's before this parenthesis.
 				$beforeParen = $phpcsFile->findPrevious( T_WHITESPACE, $i - 1, null, true );
 
 				if ( false !== $beforeParen ) {
 					$beforeCode = $tokens[ $beforeParen ]['code'];
 
 					if ( $beforeCode === T_IF || $beforeCode === T_ELSEIF ) {
 						return true;
 					}
 				}
 
 				// This parenthesis doesn't belong to if/elseif.
 				return false;
 			}
 
-			// If we hit something else (like another function call), stop.
-			if ( $code !== T_OPEN_PARENTHESIS ) {
-				return false;
-			}
+			// If we hit something else (like another function call), stop.
+			return false;
 		}

182-197: Correct the arrow function support condition.

The method should handle arrow functions (T_FN, PHP 7.4+) in addition to T_FUNCTION and T_CLOSURE. Arrow functions do have scope_opener and scope_closer set in PHP_CodeSniffer's tokenization, so the fix requires only adding T_FN to the condition—no special scope_condition handling is needed.

♻️ Proposed fix
 	private function findContainingFunction( File $phpcsFile, $stackPtr ) {
 		$tokens = $phpcsFile->getTokens();
 
 		for ( $i = $stackPtr - 1; $i >= 0; $i-- ) {
-			if ( $tokens[ $i ]['code'] === T_FUNCTION || $tokens[ $i ]['code'] === T_CLOSURE ) {
+			if ( $tokens[ $i ]['code'] === T_FUNCTION || $tokens[ $i ]['code'] === T_CLOSURE || $tokens[ $i ]['code'] === T_FN ) {
 				// Check if the stackPtr is within this function's scope.
 				if ( isset( $tokens[ $i ]['scope_opener'], $tokens[ $i ]['scope_closer'] ) ) {
 					if ( $stackPtr > $tokens[ $i ]['scope_opener'] && $stackPtr < $tokens[ $i ]['scope_closer'] ) {
 						return $i;
 					}
 				}
 			}
 		}
 
 		return false;
 	}
classes/controllers/FrmXMLController.php (1)

189-195: Consider consistent truthiness checks.

Line 190 now mixes truthiness ($form) with empty() checks (! empty( $form[ $value ] )). For consistency with the broader refactor pattern, consider updating both parts:

♻️ Consistent pattern
-	if ( $form && ! empty( $form[ $value ] ) ) {
+	if ( $form && isset( $form[ $value ] ) && $form[ $value ] ) {

Note: Using isset() first prevents potential PHP notices when accessing array keys.

classes/controllers/FrmDashboardController.php (1)

193-205: Truthiness check looks fine here; consider either enforcing array CTA or honoring $type.

if ( $cta ) is equivalent for the expected $cta array usage, but if this method is called with a non-array truthy value, you’ll still inject it into view args. Also, $type is currently ignored (always 'default').

Proposed tweak (keep truthiness style, add type + basic shape guard)
 public static function view_args_build_counter( $heading, $cta = array(), $value = 0, $type = 'default' ) {
 	$counter_args = array(
 		'heading' => $heading,
 		'counter' => $value,
-		'type'    => 'default',
+		'type'    => $type,
 	);

-	if ( $cta ) {
+	if ( $cta && is_array( $cta ) ) {
 		$counter_args['cta'] = $cta;
 	}

 	return $counter_args;
 }
classes/controllers/FrmFormActionsController.php (1)

613-620: Consider asserting $old_actions is an array before iterating.

With truthiness checks, a non-array truthy value would still pass and then foreach would iterate oddly (or error, depending on PHP version/settings).

Proposed tweak
 public static function delete_missing_actions( $old_actions ) {
-	if ( $old_actions ) {
+	if ( $old_actions && is_array( $old_actions ) ) {
 		foreach ( $old_actions as $old_id ) {
 			wp_delete_post( $old_id );
 		}
 		FrmDb::cache_delete_group( 'frm_actions' );
 	}
 }
classes/helpers/FrmStylesHelper.php (1)

474-479: ! $vars is OK, but consider enforcing array type for $vars.

This avoids accidental non-array truthy values flowing into array_diff / foreach.

Proposed tweak (no empty(), keep truthiness style)
 public static function output_vars( $settings, $defaults = array(), $vars = array() ) {
-	if ( ! $vars ) {
+	if ( ! is_array( $vars ) || ! $vars ) {
 		$vars = self::get_css_vars( array_keys( $settings ) );
 	}
classes/controllers/FrmFormsController.php (1)

297-301: Defaulting from $_POST on any falsy $values is OK; consider tightening to “not array”.

This avoids surprising behavior if a caller accidentally passes 0/'' and triggers a POST read.

Proposed tweak
 public static function update( $values = array() ) {
-	if ( ! $values ) {
+	if ( ! is_array( $values ) || ! $values ) {
 		$values = $_POST; // phpcs:ignore WordPress.Security.NonceVerification.Missing
 	}
classes/helpers/FrmAppHelper.php (1)

1005-1040: decode_amp: consider guarding non-string inputs before str_contains.
If $string can ever be non-string (e.g., int/object), str_contains( $string, '&' ) can throw a TypeError. A defensive is_string check would make this more robust.

Proposed hardening
 private static function decode_amp( &$string ) {
 	// Don't bother if there are no entities - saves a lot of processing
-	if ( ! $string || ! str_contains( $string, '&' ) ) {
+	if ( ! is_string( $string ) || $string === '' || ! str_contains( $string, '&' ) ) {
 		return;
 	}
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 880c2b4 and aef1f5b.

⛔ Files ignored due to path filters (1)
  • phpcs.xml is excluded by !**/*.xml
📒 Files selected for processing (28)
  • classes/controllers/FrmAppController.php
  • classes/controllers/FrmDashboardController.php
  • classes/controllers/FrmEntriesController.php
  • classes/controllers/FrmFieldsController.php
  • classes/controllers/FrmFormActionsController.php
  • classes/controllers/FrmFormsController.php
  • classes/controllers/FrmStylesController.php
  • classes/controllers/FrmXMLController.php
  • classes/helpers/FrmAppHelper.php
  • classes/helpers/FrmDashboardHelper.php
  • classes/helpers/FrmFieldsHelper.php
  • classes/helpers/FrmFormsHelper.php
  • classes/helpers/FrmStylesHelper.php
  • classes/helpers/FrmXMLHelper.php
  • classes/models/FrmCreateFile.php
  • classes/models/FrmDb.php
  • classes/models/FrmEntry.php
  • classes/models/FrmEntryValidate.php
  • classes/models/FrmField.php
  • classes/models/FrmFormApi.php
  • classes/models/FrmInstallerSkin.php
  • classes/models/FrmSolution.php
  • classes/models/fields/FrmFieldDefault.php
  • classes/models/fields/FrmFieldType.php
  • phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantEmptyOnParameterSniff.php
  • stripe/helpers/FrmTransLiteAppHelper.php
  • stripe/models/FrmStrpLiteAuth.php
  • tests/phpunit/emails/test_FrmEmail.php
🧰 Additional context used
🧬 Code graph analysis (3)
classes/models/FrmEntryValidate.php (1)
js/src/admin/admin.js (1)
  • exclude (3183-3183)
classes/models/FrmDb.php (1)
js/src/admin/admin.js (1)
  • limit (7910-7910)
stripe/models/FrmStrpLiteAuth.php (1)
classes/models/FrmUsage.php (1)
  • actions (468-488)
🪛 GitHub Actions: Inspections
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantEmptyOnParameterSniff.php

[error] 1-1: PHP CS Fixer found 1 file that can be fixed (phpdoc_types_order, blank_line_before_statement). Exit code 8. Run './vendor/bin/php-cs-fixer fix' to apply fixes.

⏰ 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 (46)
stripe/models/FrmStrpLiteAuth.php (1)

698-701: LGTM — semantically equivalent guard clause.

Since $actions is a function parameter (always defined), ! $actions behaves identically to empty( $actions ) for the expected array type. Both correctly guard against empty arrays and other falsy values before the foreach loop.

classes/models/FrmSolution.php (1)

657-659: LGTM! Simplified truthy check is appropriate.

The change from empty($options) to ! $options is semantically equivalent for the expected array type and aligns with the PR's objective to simplify truthy checks.

classes/models/FrmEntry.php (1)

466-472: LGTM! Refactor maintains existing behavior.

The change from empty($entry) to ! $entry is semantically equivalent here and aligns with the PR's goal of standardizing truthiness checks. The logic correctly handles:

  • Numeric entry IDs → fetches entry object
  • Falsy values → normalizes to false
  • Existing objects → leaves unchanged

The edge case of entry ID 0 being treated as falsy is not a practical concern since WordPress entry IDs start at 1.

One optional consideration: the 'false' === $entry check handles the string literal 'false', which seems like an unusual input. If you know this is no longer needed, it could be removed for clarity.

stripe/helpers/FrmTransLiteAppHelper.php (2)

367-369: LGTM with minor semantic note.

The change from empty($format) to ! $format simplifies the truthy check. Note that this introduces a subtle behavior change: the string '0' will now be treated as a valid format instead of triggering the default. Since '0' is not a practical date format, this edge case is unlikely to cause issues.


411-419: The behavior change claim is incorrect. The simplification from !empty($value) to $value does not change behavior for string '0'. In PHP, the string '0' is falsy in boolean context, so both conditions evaluate to false and produce identical results. The change is still a valid simplification—it's shorter and equivalent—but there is no behavioral difference to verify.

classes/models/FrmFormApi.php (2)

256-258: LGTM! Truthy check simplification is correct.

The refactoring from empty($addons) to ! $addons is semantically equivalent for arrays and preserves the existing behavior.


412-414: LGTM! Consistent truthy check simplification.

The refactoring from empty($addons) to ! $addons is semantically equivalent for arrays and maintains consistency with the change at line 256.

classes/models/fields/FrmFieldType.php (1)

960-967: Truthiness check is appropriate for this refactor.

The change from empty() to ! $values is safe and consistent with the PR's systematic refactoring. All two callers of get_options() pass empty arrays (array()), which are falsy in both conditions. Both checks behave identically for all actual usage in the codebase.

classes/models/FrmEntryValidate.php (3)

115-117: LGTM! Truthy check simplification is correct.

The change from ! empty( $exclude ) to $exclude is safe for the bool|string[] type. Both conditions behave identically since empty arrays are falsy in PHP.


521-532: Truthy check simplification looks good, with one edge case note.

The change from empty( $pattern ) to ! $pattern is appropriate here. Note that this actually fixes a subtle edge case: if the first pattern part is '0', the old code would incorrectly treat the second iteration as the first (since empty('0') is TRUE), while the new code correctly handles it as a subsequent iteration.

This edge case is unlikely in regex pattern contexts, so the change is safe and improves correctness.


552-555: Truthy check simplification is correct, but note the type hint inconsistency.

The change from ! empty( $exclude ) to $exclude is safe and works correctly for boolean context.

However, there's a pre-existing type documentation issue: Line 540 declares $exclude as bool, but it actually receives bool|string[] from the validate() function (line 17, passed on line 50). The simplified truthy check works correctly for both types since empty arrays are falsy in PHP, but the type hint should be updated for accuracy.

Consider updating the type hint on line 540 to match the actual usage:

/**
 * Check for spam.
 *
 * @param bool|string[] $exclude
 * @param array $values
 * @param array $errors By reference.
 *
 * @return void
 */
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantEmptyOnParameterSniff.php (4)

39-41: LGTM!

The token registration is correct for detecting empty() calls.


51-98: LGTM!

The validation logic properly handles edge cases with appropriate early returns. Skipping array access cases ($var['key']) is a sensible simplification.


120-171: LGTM!

The fix logic correctly handles both negated and non-negated cases, properly manages whitespace removal, and uses beginChangeset()/endChangeset() for atomic operations.


207-225: LGTM!

The parameter extraction correctly identifies T_VARIABLE tokens within the function signature, which excludes type hints and other non-parameter tokens.

classes/models/FrmDb.php (2)

426-432: LGTM - Refactoring maintains consistent behavior.

The change from ! empty() to direct truthiness checks is safe here because:

  • Both $order_by and $limit are function parameters with default values (empty string), so they're always defined
  • For all relevant falsy values ('', 0, '0', null, false), both approaches behave identically
  • This aligns with the existing esc_limit() method (line 605) which also uses a truthiness check

The refactoring preserves the existing behavior while simplifying the code.


426-432: The refactoring is safe. The RedundantEmptyOnParameterSniff is specifically designed to apply only to function parameters with default values, where the distinction between empty string, 0, and false is not semantically meaningful. For $order_by and $limit in this function, truthiness checks correctly replace empty() without introducing undefined variable warnings or losing intent—an empty string and 0 both mean "no ordering" or "no limit."

classes/models/FrmField.php (1)

924-924: No breaking changes in practice; simplification is safe for current usage patterns.

While the change from ! empty( $limit ) to $limit introduces a semantic difference (string "0" would be treated as falsy instead of truthy), this has no practical impact. Examination of all callers shows:

  • No calls pass 0 or "0" as limit values
  • Only explicit limit usage is integer 1 (FrmSubmitHelper.php:48)
  • Default parameter is '', which behaves identically in both checks
  • All other calls omit the limit parameter entirely

The parameter type hint (int|string) appears to be vestigial; actual usage is limited to integers and empty string. The change is semantically sound for the codebase's actual calling patterns.

Note: Line 964 in get_all_for_form still uses the if ( ! $limit ) pattern while lines 924 and 975 now use if ( $limit ) — consider standardizing if consistency is desired.

Likely an incorrect or invalid review comment.

tests/phpunit/emails/test_FrmEmail.php (1)

710-713: LGTM! Truthiness check is appropriate here.

The change from empty($property) to ! $property is functionally equivalent for this use case where $property is expected to be either a non-empty string or falsy when not provided.

classes/models/FrmCreateFile.php (1)

157-165: LGTM! Truthiness check is appropriate for file path defaulting.

The change from empty($file) to ! $file is functionally equivalent and appropriate for this file path parameter check.

classes/controllers/FrmStylesController.php (1)

1345-1360: LGTM! Truthiness check is appropriate here.

The change from empty($screen) to ! $screen is functionally equivalent for checking whether a screen object/string was provided or needs to be fetched via get_current_screen().

classes/models/FrmInstallerSkin.php (1)

72-93: LGTM! Truthiness check is appropriate for error handling.

The change from ! empty($errors) to $errors is functionally equivalent for checking whether an error (string or WP_Error object) exists before processing.

classes/helpers/FrmDashboardHelper.php (1)

188-188: LGTM - Truthiness check is semantically equivalent here.

The change from empty( $buttons ) to ! $buttons is appropriate. Since $buttons has a default value of array(), it's always defined, and both checks evaluate identically for arrays (empty arrays are falsy in PHP).

classes/models/fields/FrmFieldDefault.php (1)

24-26: LGTM - Expanded falsy handling is appropriate for type defaulting.

The change from empty( $type ) to ! $type broadens the set of falsy values that trigger the default type assignment (including 0, '0', etc.). This is consistent with PHP truthiness semantics and appropriate for this context, as field types are expected to be non-empty strings like 'text', 'email', etc.

classes/controllers/FrmAppController.php (1)

261-263: LGTM - Truthiness check maintains identical behavior.

The change from empty( $show_nav ) to ! $show_nav is semantically equivalent here. The variable is always defined (set via get_param() on line 259), and both checks evaluate identically for all falsy values including '0', which is falsy in PHP.

classes/helpers/FrmFieldsHelper.php (1)

2749-2751: LGTM - Truthiness check is cleaner and equivalent.

The change from ! empty( $should_hide_bulk_edit ) to $should_hide_bulk_edit is semantically equivalent and more concise. Both evaluate identically for all practical boolean/truthy values of the parameter.

classes/helpers/FrmXMLHelper.php (2)

407-414: LGTM - Truthiness check is appropriate for array iteration guard.

The change from ! empty( $form_fields ) to $form_fields is semantically equivalent. The variable is assigned earlier (line 251 or 258) and is either false or an array of fields. Both checks prevent iteration when the value is falsy.


503-540: LGTM - Truthiness check is appropriate for object validation.

The change from ! empty( $this_form ) to $this_form is semantically equivalent. The variable is assigned earlier (line 248) via maybe_get_form() and returns either a form object or false. Both checks correctly determine whether to update or create a new field.

classes/controllers/FrmFormActionsController.php (2)

359-363: Guard change is OK assuming $form is always an object/false.

if ( ! $form ) is fine here and keeps the intent clear.


695-703: Entry guard change looks fine; keep an eye on $entry type expectations.

if ( ! $entry || ... ) is equivalent for 0, '', false, null and avoids calling into draft checks for invalid entries.

classes/controllers/FrmFormsController.php (1)

2485-2496: if ( $id ) is fine as a guard here.

Given $id can be an id or key, truthiness matches the intended “only query when something is provided”.

classes/helpers/FrmFormsHelper.php (4)

97-101: LGTM: truthy check works for expected usage.

$class being '0' still correctly skips; other non-empty strings behave the same.


338-346: LGTM: $values truthiness reads cleanly here.

Keeps the intended “use provided array or fall back to request” behavior for the expected input types.


1114-1119: LGTM: guard reads better and stays behaviorally consistent for typical inputs.


1356-1379: LGTM: if ( $link_details ) is fine for the expected array shape.

classes/controllers/FrmEntriesController.php (5)

338-355: check_hidden_cols: truthiness swap looks safe for $prev_value here.
$prev_value is a defined parameter, so switching from empty() to ! shouldn’t introduce “undefined var” differences, and falsy handling remains effectively the same for expected types.


440-448: base_column_key: ok, but confirm callers never intentionally pass "0" as a meaningful menu name.
! $menu_name treats "0" as “missing” (same as empty()), which is usually fine for menu names but worth sanity-checking.


579-602: remove_excess_cols: condition change is fine given $result is normalized to an array upstream.
Since hidden_columns() forces non-arrays to array(), ! $result is equivalent to empty($result) in practice.


610-649: display_list: truthiness guard is fine and preserves intent.
$message is a local variable (always set), so ! $message is a clean simplification for the import-message flow.


750-817: process_entry: verify $errors is never a sentinel string like "0" (unlikely), otherwise behavior is unchanged.
For typical $errors shapes (array/string/false), ! $errors matches the old empty() check; worth confirming no caller uses "0" to mean “has errors”.

classes/helpers/FrmAppHelper.php (6)

1152-1165: allowed_html: elseif ( $allowed ) is a reasonable simplification.
For expected inputs (array of tags or empty), this matches prior behavior while being clearer.


2847-2855: setup_edit_vars: ! $post_values is fine for the intended “missing/empty array” check.
Given $post_values is expected to be an array, this is equivalent to the previous empty() usage.


2890-2901: prepare_field_arrays: if ( $fields ) is a clean simplification.
Handles '' and [] as “no fields” and avoids iterating empty inputs.


3060-3067: custom_style_value: truthiness check is fine, but ensure $post_values is always an array in callers.
If any callsite passes a non-array truthy value, isset( $post_values['options']... ) would warn/error; assuming it’s always an array, this change is good.


3569-3584: select_current_page: condition simplification looks correct.
! $action cleanly covers empty arrays, keeping behavior consistent with the old empty($action) style.


2408-2429: permission_nonce_error: all callsites correctly pass the nonce action as the 3rd parameter; implementation is safe.

All 10 callsites consistently pass nonce actions (e.g., 'frm_save_form_nonce', 'frm_ajax', 'import-xml-nonce') rather than nonce values, which matches the implementation's use of the third parameter as the action argument to wp_verify_nonce(). No breaking changes to existing integrations.

The optional refactoring suggestion to rename $nonce to $nonce_action remains valid for improving code clarity and reducing ambiguity.

Comment thread classes/controllers/FrmFieldsController.php
Comment thread classes/models/FrmField.php
@Crabcyborg Crabcyborg merged commit 3c24187 into master Jan 9, 2026
15 of 16 checks passed
@Crabcyborg Crabcyborg deleted the add_new_sniff_to_simplify_truthy_checks branch January 9, 2026 14:16
stephywells pushed a commit that referenced this pull request Apr 4, 2026
…uthy_checks

Add new sniff to simplify truthy checks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant