Stop making useless casts#2626
Conversation
WalkthroughBroadens PHPDoc type annotations across many files (e.g., int→int|string, array→array|object), removes several explicit casts ((array), (string)), and adds a small runtime guard in generate_xml; phpstan.neon removed its uselessCast override. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
classes/controllers/FrmXMLController.php (1)
463-483: Psalm error:$typeis now documented asarray<string>|string, butforeachassumes array.Widening the
$typedocblock toarray<string>|stringis reasonable given how this method is used, andprepare_types_array()correctly normalizes it with(array) $type. However, Psalm can’t see that the by‑ref call guarantees an array, so it flagsforeach ( $type as $tb_type )as “PossiblyInvalidIterator” (see the pipeline failure).You can keep the broader param type and fix the static analysis by asserting the post‑condition before the loop, e.g.:
public static function generate_xml( $type, $args = array() ) { global $wpdb; self::prepare_types_array( $type ); + + /** @var array<string> $type */ + $tables = array( 'items' => $wpdb->prefix . 'frm_items',This has no runtime effect but tells Psalm that
$typeis an array at that point. Alternatively, you could re-cast locally ($type = (array) $type;) if you prefer not to rely on the by‑ref helper.
♻️ Duplicate comments (2)
classes/models/FrmPersonalData.php (1)
120-120: Same verification needed as line 79.This has the same concern regarding
FrmDb::get_col()return type. Theempty()check on line 114 provides protection, but verify the method always returns an array.classes/controllers/FrmXMLController.php (1)
611-627: Docblock widening for$typeinprepare_types_arrayis consistent with callers.Allowing
array<string>|stringfor$typeinprepare_types_array()matches howgenerate_xml()now documents its own$typeparameter and is safe given the explicit(array) $typenormalization inside this helper. Once the Psalm concern above is addressed, this change should be non‑controversial.
🧹 Nitpick comments (3)
classes/models/FrmEntryMeta.php (1)
395-417: Return typemixedis accurate; optionally narrow to a union laterChanging the
getAlldocblock return fromarraytomixedbetter matches the current implementation, which can return different shapes depending on$limitand$stripslashes, viaFrmDb::check_cache. No runtime behavior changes here.If you want stricter static analysis in the future, you could consider documenting a more explicit union (e.g.,
array|object|nullor similar) once the exact contract ofFrmDb::check_cacheis nailed down, but this is not required for this PR.classes/models/FrmPersonalData.php (1)
141-141: Consider restoring "of" for clarity.The updated PHPDoc "array Entry ids" is grammatically awkward compared to the previous "array of entry ids". While minor, the original phrasing was clearer.
Apply this diff to restore clarity:
- * @return array Entry ids + * @return array of entry idsclasses/models/FrmFieldOption.php (1)
90-101: Cast removal is safe for documented types; only edge case is if$saved_valuecan benull.Removing
(string)from the'' !== (string) $this->saved_valuecheck keeps behavior identical for the expected types (string / numeric string). The only behavior change would be if$saved_valuewere evernullor another non-string falsy value, in which case the label would now resolve to an empty string instead ofFrmAppHelper::get_no_label_text(). If$saved_valueis guaranteed to be a string per the class contract, this is fine; otherwise, consider normalizing it to a string when setting it, or keeping the cast.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
classes/controllers/FrmDashboardController.php(2 hunks)classes/controllers/FrmFormsController.php(1 hunks)classes/controllers/FrmXMLController.php(2 hunks)classes/helpers/FrmAppHelper.php(1 hunks)classes/helpers/FrmEntriesHelper.php(1 hunks)classes/helpers/FrmStylesHelper.php(1 hunks)classes/helpers/FrmXMLHelper.php(1 hunks)classes/models/FrmAddon.php(1 hunks)classes/models/FrmDb.php(1 hunks)classes/models/FrmEntry.php(5 hunks)classes/models/FrmEntryMeta.php(1 hunks)classes/models/FrmFieldFormHtml.php(1 hunks)classes/models/FrmFieldOption.php(1 hunks)classes/models/FrmForm.php(2 hunks)classes/models/FrmMigrate.php(1 hunks)classes/models/FrmNotification.php(1 hunks)classes/models/FrmPersonalData.php(4 hunks)classes/models/FrmStyle.php(1 hunks)classes/models/FrmTableHTMLGenerator.php(1 hunks)classes/models/fields/FrmFieldType.php(1 hunks)classes/widgets/FrmShowForm.php(1 hunks)phpstan.neon(0 hunks)
💤 Files with no reviewable changes (1)
- phpstan.neon
🧰 Additional context used
🧬 Code graph analysis (3)
classes/models/FrmNotification.php (1)
classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-5084)kses(1059-1063)
classes/models/FrmFieldOption.php (1)
classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-5084)get_no_label_text(4005-4007)
classes/models/FrmForm.php (1)
classes/models/FrmDb.php (2)
FrmDb(6-870)get_results(330-332)
🪛 GitHub Actions: Psalm Code Analysis
classes/controllers/FrmXMLController.php
[error] 495-495: Psalm: PossiblyInvalidIterator. Cannot iterate over string. See https://psalm.dev/165.
⏰ 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 trunk
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Cypress
- GitHub Check: Cypress
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
🔇 Additional comments (22)
classes/models/FrmMigrate.php (1)
555-569: Change is consistent withmigrate_to_17; just confirmget_fields_with_size()always returns an arrayUsing
foreach ( $fields as $f )without the(array)cast bringsmigrate_to_86in line withmigrate_to_17, which already iterates the result ofget_fields_with_size()without casting. Given the@return arraydocblock onget_fields_with_size(), this looks correct and removes a redundant cast.The only thing to double‑check is that
FrmDb::get_results()never returnsnull/false; otherwise this could start emitting “Invalid argument supplied for foreach()” notices where previously the cast would turn non‑arrays into an empty array.If that contract is guaranteed, this change is good as‑is.
classes/models/FrmNotification.php (1)
72-72: LGTM! Unnecessary cast removed correctly.The
(array)cast on$tempwas indeed useless sincestr_replace()returns an array when its subject parameter is an array (which$sent_tois guaranteed to be from lines 68-69).classes/models/fields/FrmFieldType.php (1)
169-169: LGTM! Improved type documentation accuracy.The broadened return type
array|object|stringmore accurately reflects what this method can return. Since field columns can contain various data types including objects (nested configurations), arrays (options), or scalar values, this documentation improvement provides better type information.classes/models/FrmAddon.php (1)
741-747: Removal of(array)cast inlast_checked()is safe and keeps the return type invariantGiven the normalization block:
if ( $last_checked && ! is_array( $last_checked ) ) { $last_checked = array( 'time' => $last_checked ); }any truthy
$last_checkedreaching the return is already an array, and falsy values still map toarray(). The new:return $last_checked ? $last_checked : array();preserves the documented
@return arraycontract and just drops a redundant cast. No behavior change for existing stored values or callers relying on an array.classes/models/FrmTableHTMLGenerator.php (1)
97-97: No action required. All constructor call sites pass string literals ('entry'or'shortcode'), confirming the cast removal is safe and not a breaking change. The PHPDoc annotations remain accurate.classes/widgets/FrmShowForm.php (1)
66-66: The (array) cast removal is appropriate.
wp_parse_args()is designed to handle mixed input types and will normalize them to an array. SinceFrmShowFormextends WordPress'sWP_Widgetclass, the widget framework guarantees thatform()receives$instanceas an array from stored widget settings. The PHPDoc already declares the expected type as@param array $instance, making the explicit cast redundant. This change aligns with modern PHP practices and the removal is safe.classes/models/FrmFieldFormHtml.php (1)
32-34: LGTM! PHPDoc accurately reflects the property's union type.The change from
@var arrayto@var array|objectcorrectly documents that the$formproperty can be either type. This is validated by the necessary cast at line 360 ($form = (array) $this->form;), which ensures safe array access for subsequent operations. The default initialization asarray()remains appropriate.classes/models/FrmForm.php (2)
831-839: Docblock type forget_key_by_idnow matches usageAccepting
int|stringin the docblock aligns with the cast to(int)inside and common call patterns (numeric strings); no behavioral change introduced.
1002-1045: Removal of(array)cast aroundFrmDb::get_resultsis safe
FrmDb::get_resultsalready returns an array of rows, so dropping the redundant cast inget_count()preserves behavior while satisfying the “no useless casts” objective.classes/helpers/FrmXMLHelper.php (1)
771-781: Broadened$fielddoc type matches implementation
migrate_field_placeholder()already handles objects via(array) $field, so documentingarray|objectsimply reflects reality and is correct.classes/helpers/FrmEntriesHelper.php (1)
26-34:$fieldsdocblock widened tomixedaligns with casting behaviorSince
setup_new_vars()casts$fieldsto array before iteration, documenting it asmixedis more accurate and doesn’t change runtime behavior.classes/models/FrmDb.php (1)
680-687:save_settingsdoc now matches the(array)castAllowing
array|objectin the docblock is consistent with casting$settingsto array and accurately documents current behavior.classes/controllers/FrmDashboardController.php (1)
470-493:get_youtube_embed_videodocblock matches actual usageDocumenting
$entries_countasint|stringis consistent with the internal(int)casts and common numeric-string inputs; logic and return behavior remain unchanged.classes/helpers/FrmStylesHelper.php (1)
762-769: Docblock forget_base_font_size_scaleupdated to reflect real inputsAllowing
int|stringfor$valuematches how callers pass font sizes and how the function casts to(int)internally; behavior is unaffected.classes/helpers/FrmAppHelper.php (1)
2895-2898: Docblock type now correctly matches actual usage of$fields.Allowing
array|stringin the docblock for$fieldsreflects existing behavior (default''and arrays) and matches the(array) $fieldscast in the implementation, with no runtime change.classes/controllers/FrmFormsController.php (1)
1294-1299: Docblock widening for$valuematches how screen options are passed.Annotating
$valueasint|stringaligns with WordPress screen options behavior and the explicit(int)cast insave_per_page, without changing runtime semantics.classes/models/FrmStyle.php (1)
847-852: Allowingint|stringfor$style_idis consistent with current usage.The docblock now reflects that callers may pass either an int or a string ID, and the method already normalizes via
(int) $style_id, so there is no behavioral impact.classes/models/FrmEntry.php (5)
724-724: PHPDoc type broadening looks correct.The parameter type change from
inttoint|stringaccurately reflects that this method accepts both types at runtime, with the code safely casting tointon Line 730.
1055-1055: PHPDoc type broadening is accurate.The change from
inttoint|stringcorrectly documents that this method accepts both types, with the code safely casting tointon Line 1066 before storing in the array.
1074-1075: PHPDoc update aligns with runtime behavior.The parameter type broadening for
$entry_idfrominttoint|stringis consistent with how the parameter is used—passed to other methods and cast tointwhen needed on Line 1084.
1173-1175: PHPDoc accurately reflects parameter usage.The broadening of
$idfrominttoint|stringcorrectly documents the method's actual behavior—accepting both types and casting tointon Line 1184 when needed for comparison.
1242-1245: PHPDoc changes are consistent and accurate.The parameter type broadening for
$idfrominttoint|stringappropriately documents the method's runtime behavior, with safe casting tointon Line 1260 where needed.
1 participant
This removes an exception for a strict PHPStan rule.