Add type comments#2604
Conversation
WalkthroughWidespread PHPDoc additions across controllers, helpers, models, fields, and view components; removal of deprecated FrmInboxController::dismiss_all_button; added methods FrmAppHelper::post_edit_link and FrmSettings::update_setting; two method signatures simplified (FrmSettingsController::settings_cta, FrmFormsController::update_form_builder_fields); adjusted a hook registration and added a phpstan.neon ignore. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
phpcs.xmlis excluded by!**/*.xml
📒 Files selected for processing (17)
classes/controllers/FrmStylesController.php(2 hunks)classes/helpers/FrmAppHelper.php(5 hunks)classes/models/FrmFieldValueSelector.php(1 hunks)classes/models/FrmForm.php(2 hunks)classes/models/fields/FrmFieldEmail.php(1 hunks)classes/models/fields/FrmFieldType.php(8 hunks)classes/models/fields/FrmFieldUserID.php(2 hunks)classes/views/styles/components/FrmAlignStyleComponent.php(1 hunks)classes/views/styles/components/FrmBackgroundImageStyleComponent.php(1 hunks)classes/views/styles/components/FrmColorpickerStyleComponent.php(1 hunks)classes/views/styles/components/FrmDirectionStyleComponent.php(1 hunks)classes/views/styles/components/FrmDropdownStyleComponent.php(1 hunks)classes/views/styles/components/FrmFieldShapeStyleComponent.php(1 hunks)classes/views/styles/components/FrmPrimaryColorStyleComponent.php(1 hunks)classes/views/styles/components/FrmTextToggleStyleComponent.php(1 hunks)stripe/controllers/FrmTransLiteAppController.php(1 hunks)stripe/models/FrmTransLiteDb.php(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Inspections
classes/models/FrmForm.php
[error] 1-1: PHP-CS-Fixer reported 1 issue: phpdoc_types_order would fix in FrmForm.php. Exit code 8 from fixer (dry-run).
⏰ 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: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (30)
classes/views/styles/components/FrmPrimaryColorStyleComponent.php (1)
20-23: LGTM!The parameter documentation is accurate and enhances code clarity for this constructor.
classes/views/styles/components/FrmBackgroundImageStyleComponent.php (1)
19-21: LGTM!The parameter documentation is accurate and consistent with the other style components in this PR.
classes/views/styles/components/FrmDropdownStyleComponent.php (1)
20-23: LGTM!The parameter documentation is accurate and follows the established pattern.
classes/views/styles/components/FrmAlignStyleComponent.php (1)
20-23: LGTM!The parameter documentation is accurate and maintains consistency across style components.
classes/models/FrmForm.php (2)
229-236: LGTM!The parameter documentation for the
updatemethod is clear and accurate.
73-81: The rewritten review comment is complete and ready. The verification has been finished. Please note that the sandbox environment does not have the php-cs-fixer binary available, but the issue has been confirmed through inspection of the source code and project configuration.stripe/controllers/FrmTransLiteAppController.php (1)
9-13: LGTM!The method documentation clearly describes the purpose and parameter, with the
mixedtype appropriately covering the false default and other possible version values.classes/views/styles/components/FrmColorpickerStyleComponent.php (1)
20-24: LGTM!The parameter documentation is accurate and consistent with the other style component updates.
classes/views/styles/components/FrmFieldShapeStyleComponent.php (1)
20-23: LGTM!The PHPDoc annotations accurately document the constructor parameters with clear descriptions.
classes/models/fields/FrmFieldType.php (8)
439-442: LGTM!The parameter documentation for
show_optionsaccurately describes the method's arguments.
481-482: LGTM!Clear documentation added for the
show_field_optionsmethod parameters.
602-603: LGTM!Appropriate parameter documentation for
show_single_option.
620-622: LGTM!The parameter and return type documentation for
has_field_choicesis accurate.
632-634: LGTM!The parameter documentation for
displayed_field_typecorrectly describes the field argument.
731-732: LGTM!Good documentation noting that
$default_valueis passed by reference, which is important for developers to understand.
909-910: LGTM!Clear parameter documentation added for
get_field_scripts_hook.
1437-1438: LGTM!Correctly documents that
$valueis passed by reference, which is important information for implementers.classes/views/styles/components/FrmDirectionStyleComponent.php (1)
20-23: LGTM!The constructor parameter documentation is accurate and follows consistent patterns across similar style component classes.
classes/models/FrmFieldValueSelector.php (1)
89-90: LGTM!The constructor parameter documentation clearly describes the expected arguments with appropriate type hints.
classes/models/fields/FrmFieldEmail.php (1)
60-60: LGTM!The parameter documentation correctly notes that the email value is passed by reference, which is important information for developers using this method.
classes/views/styles/components/FrmTextToggleStyleComponent.php (1)
21-24: LGTM!Constructor parameter documentation is clear and consistent with the pattern used across other style components.
classes/controllers/FrmStylesController.php (2)
1199-1200: LGTM!The enhanced parameter descriptions for
get_style_valprovide better clarity on what each parameter represents.
1242-1243: LGTM!The added parameter documentation for
do_accordion_sectionsclearly describes the expected screen and context arguments.classes/models/fields/FrmFieldUserID.php (2)
57-58: LGTM!The parameter documentation for
get_field_valueaccurately describes the arguments passed to the method.
136-137: LGTM!The parameter documentation correctly notes that the value is passed by reference, which is essential information for understanding the method's behavior.
classes/helpers/FrmAppHelper.php (5)
912-913: LGTM! Clear documentation for pass-by-reference parameter.The added parameter documentation accurately describes that
$valueis passed by reference and can be of mixed type, which matches the function implementation that handles both arrays and other types.
933-933: LGTM! Clear documentation for pass-by-reference parameter.The added parameter documentation accurately describes that
$stringis passed by reference, which is helpful for understanding the function's behavior.
977-978: LGTM! Well-documented parameter descriptions.The expanded parameter documentation clearly describes both parameters. The note that
$allowedcan accept 'all' for defaults is particularly helpful and matches the implementation.
1645-1646: LGTM! Clear and accurate parameter documentation.The added parameter documentation accurately describes both parameters. The
object|nulltype annotation correctly reflects the nullable parameter with its default value.
2972-2974: LGTM! Enhanced parameter documentation with clear descriptions.The refined parameter documentation accurately describes all three parameters. The type annotations correctly reflect that
$toand$levelscan accept multiple types, and the description for$levelshelpfully explains it can be either a number of units or a specific unit name.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
classes/models/fields/FrmFieldType.php (1)
1742-1750: Optionally clarify pass‑by‑reference insanitize_valuePHPDoc for consistencyThis method also mutates
$valueby reference. For consistency with nearby hooks, you could mirror the “passed by reference” note in the@paramline./** * @since 4.0.04 * - * @param array|string $value + * @param array|string $value Value passed by reference. * @return void */ public function sanitize_value( &$value ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
classes/models/FrmForm.php(2 hunks)classes/models/fields/FrmFieldEmail.php(1 hunks)classes/models/fields/FrmFieldName.php(1 hunks)classes/models/fields/FrmFieldNumber.php(1 hunks)classes/models/fields/FrmFieldPhone.php(1 hunks)classes/models/fields/FrmFieldType.php(9 hunks)classes/models/fields/FrmFieldUrl.php(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- classes/models/fields/FrmFieldPhone.php
- classes/models/fields/FrmFieldName.php
🚧 Files skipped from review as they are similar to previous changes (2)
- classes/models/FrmForm.php
- classes/models/fields/FrmFieldEmail.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). (1)
- GitHub Check: Cypress
🔇 Additional comments (9)
classes/models/fields/FrmFieldUrl.php (1)
109-109: LGTM! Documentation improvement aligns with codebase patterns.The
array|stringtype annotation is appropriate, as theprepare_display_valuemethod at line 91 iterates over(array) $value, confirming the field can handle both single URLs and arrays of URLs.classes/models/fields/FrmFieldNumber.php (1)
178-178: LGTM! PHPDoc enhancement improves code documentation.The
array|stringtype annotation documents the parameter's accepted types. This is consistent with the broader pattern of type comment additions across field classes mentioned in the PR summary.classes/models/fields/FrmFieldType.php (7)
437-444: Docblock forshow_optionscorrectly documents all three array parameters
$field,$display, and$valuesare all treated as arrays in the body, so the types and brief descriptions here are accurate.
479-485:show_field_options$argsdescription matches how it’s usedNoting that
$argsincludesfield,display, andvaluesaligns with the keys accessed in this method and the included template.
600-607:show_single_option$argssummary is compatible with current usageDescribing
$argsas including field configuration is consistent with the method only needing$args['field']to delegate toFrmFieldsHelper::show_single_option().
620-635: Types forhas_field_choices()anddisplayed_field_type()align with implementationAccepting
array|objectfor$fieldand returning an array fromdisplayed_field_type()matches the way field types are probed and filtered here.
729-735:default_value_to_stringdoc now correctly calls out by-reference behaviorThe
mixedtype plus “Default value passed by reference” note matches the logic that mutates$default_valuein place based on whether it’s an array or scalar.
909-915:get_field_scripts_hook$argsdescription is appropriately scopedTreating
$argsas generic field rendering arguments fits its current usage (access toformand optionalparent_form_id) without over-specifying structure.
1435-1441:maybe_trim_excess_valuesdoc correctly highlights reference semanticsThe
mixedtype and “Value passed by reference” indication match this hook’s contract for subclasses that may mutate$value.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
square/helpers/FrmSquareLiteConnectHelper.php (1)
388-392:$keyannotation inget_square_connect_option_nameis incomplete.This method is also called with
'merchant_location_id'and'merchant_currency', so the current list of possible$keyvalues is misleading.You could either generalize or expand the list. For example:
- * @param string $key 'merchant_id', 'client_password', 'server_password'. + * @param string $key Option key, such as 'merchant_id', 'merchant_location_id', + * 'merchant_currency', 'client_password', or 'server_password'.
🧹 Nitpick comments (3)
square/helpers/FrmSquareLiteAppHelper.php (1)
55-55: Doc summary still references Stripe instead of SquareMinor naming inconsistency: the comment says "Add education about Stripe fees." in a Square helper. Consider updating to “Square fees” for clarity.
classes/helpers/FrmStylesHelper.php (2)
259-264: Consider documenting the return type forhex2rgbaThe new
@paramtypes for$hexand$alook correct and align with how they’re used. For completeness with the rest of the file, you might also add@return stringhere.
716-721: Minor naming clarity forget_base_font_size_scaledocThe types and parameter order in the docblock match the function signature. You might consider renaming the
$valuedescription to “Base font size” (to mirror$base_font_sizeat the call site) for slightly clearer intent, but it’s already correct as-is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
classes/helpers/FrmEntriesHelper.php(4 hunks)classes/helpers/FrmFieldsHelper.php(1 hunks)classes/helpers/FrmStylesHelper.php(10 hunks)classes/models/FrmMigrate.php(2 hunks)square/helpers/FrmSquareLiteAppHelper.php(1 hunks)square/helpers/FrmSquareLiteConnectHelper.php(4 hunks)stripe/helpers/FrmStrpLiteAppHelper.php(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- stripe/helpers/FrmStrpLiteAppHelper.php
- classes/helpers/FrmFieldsHelper.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). (3)
- GitHub Check: Cypress
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
🔇 Additional comments (17)
square/helpers/FrmSquareLiteAppHelper.php (1)
57-59: PHPDoc types and descriptions match implementationThe new
@paramannotations correctly reflect the method signature and how$mediumand$gatewayare used (stringandarray|false|stringrespectively). No functional risk introduced.classes/helpers/FrmEntriesHelper.php (4)
417-419: LGTM! Improved parameter documentation.The enhanced PHPDoc correctly documents the by-reference parameters and provides clearer descriptions for each parameter.
458-460: LGTM! Consistent documentation improvement.The PHPDoc accurately documents the by-reference parameters, maintaining consistency with similar methods in the class.
645-647: LGTM! Accurate return type documentation.The
@return voidannotation correctly reflects that this method outputs HTML directly without returning a value.
681-684: LGTM! Clear and accurate documentation.The PHPDoc provides helpful descriptions for parameters and correctly documents the array return type.
classes/models/FrmMigrate.php (1)
579-580: LGTM!The PHPDoc additions accurately document the parameter (correctly noting it's passed by reference) and the void return type.
square/helpers/FrmSquareLiteConnectHelper.php (3)
276-278: Docblock matches parameter type and behavior formaybe_fix_wpml_url.
$urlis always treated as a string and returned as such; the new description is accurate and improves clarity.
372-375: Mode annotation forget_location_id_option_nameis accurate and consistent.The doc now clearly states the allowed
$modevalues ('auto','live','test'), matching usage patterns elsewhere in this helper.
548-552: Enhancedget_location_iddocblock correctly reflects signature and return type.The
$forceand$modedescriptions match how the method is implemented, andfalse|stringis accurate for the return value.classes/helpers/FrmStylesHelper.php (8)
135-143: Docblock foricon_key_to_classmatches implementationThe new
@paramannotations (int|string key, string icon/type) and@return stringaccurately describe current usage and behavior.
421-423:fill_hexby-ref parameter doc looks accurateThe
@param string $colorplus “passed by reference” comment and@return voidcorrectly matchfill_hex( &$color )and its behavior.
432-435:get_css_varsparam/return docs align with usageDocumenting
$varsas an array of CSS variable keys and@return arraymatches the implementation (apply_filters+array_unique).
443-447:output_varsdocs correctly describe settings/defaults/varsThe new annotations for
$settings,$defaults, and$vars, plus@return void, are consistent with howoutput_varsis called and how it only echoes CSS.
675-678:update_base_font_sizedefaults doc is consistentAdding
@param array $defaults Default style settings.matches the actual second argument and its use when scaling font sizes.
732-736:prepare_color_outputby-ref and flag docs look goodThe docs now clearly state
$settingsis passed by reference and$allow_transparentcontrols transparent overrides, with@return void, all matching the implementation.
784-788:get_color_outputparam docs match behaviorDescribing
$defaultas the fallback color and$coloras a by-ref value that may be normalized or prefixed is consistent with how this helper is used.
832-835: Return type fordescription_margin_for_screensizeis accurateThe
false|stringunion in the doc correctly reflects that the function either returns a breakpoint string like'800px'orfalsewhen no adjustment is needed.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
stripe/helpers/FrmStrpLiteConnectHelper.php (1)
534-536: LGTM! Documentation correctly added.The PHPDoc correctly documents the parameter and return types for the
strip_lang_from_urlmethod. The types match the implementation.Optionally, the parameter description could be slightly more descriptive:
- * @param string $url URL. + * @param string $url URL to strip language parameter from.classes/models/FrmAddon.php (2)
467-475: Optional: narrowobjecttype for$version_info
$version_infois always a decoded object (e.g.,stdClass) when passed intoclear_old_plugin_version, and may be set tofalseby reference.If you want tighter static analysis, consider documenting this as a union and a concrete class:
- * @param object $version_info The version info for the plugin. + * @param \stdClass|false $version_info The version info object; may be set to false when expired.This is just for tooling clarity; behavior is already correct.
486-494: Optional: mirror union type for$version_infoinmaybe_use_beta_url
maybe_use_beta_urlcurrently receives an object, but given how$version_infoflows throughprepare_update_details, you might want to mirror the more precise union suggested forclear_old_plugin_version:- * @param object $version_info The version info for the plugin. + * @param \stdClass $version_info The version info for the plugin.Here the parameter is never
falseat call time, so\stdClasswould be a bit clearer than bareobject. Existing behavior is fine either way.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
classes/controllers/FrmAppController.php(1 hunks)classes/controllers/FrmDashboardController.php(1 hunks)classes/controllers/FrmInboxController.php(0 hunks)classes/factories/FrmFieldFactory.php(1 hunks)classes/helpers/FrmCSVExportHelper.php(1 hunks)classes/helpers/FrmDashboardHelper.php(1 hunks)classes/helpers/FrmEntriesHelper.php(6 hunks)classes/helpers/FrmEntriesListHelper.php(1 hunks)classes/helpers/FrmFormsListHelper.php(1 hunks)classes/helpers/FrmListHelper.php(3 hunks)classes/models/FrmAddon.php(7 hunks)classes/models/FrmDb.php(10 hunks)classes/models/FrmEntryFormatter.php(1 hunks)classes/models/FrmEntryValidate.php(3 hunks)classes/models/FrmField.php(6 hunks)classes/models/FrmForm.php(11 hunks)classes/models/FrmFormApi.php(3 hunks)classes/models/FrmMigrate.php(2 hunks)classes/models/FrmOnSubmitAction.php(1 hunks)classes/models/FrmPersonalData.php(2 hunks)classes/models/FrmReviews.php(1 hunks)classes/models/FrmSettings.php(2 hunks)classes/models/FrmSolution.php(7 hunks)square/helpers/FrmSquareLiteConnectHelper.php(5 hunks)stripe/helpers/FrmStrpLiteConnectHelper.php(1 hunks)stripe/helpers/FrmTransLiteListHelper.php(1 hunks)
💤 Files with no reviewable changes (1)
- classes/controllers/FrmInboxController.php
✅ Files skipped from review due to trivial changes (9)
- classes/helpers/FrmListHelper.php
- classes/models/FrmEntryFormatter.php
- classes/models/FrmField.php
- classes/models/FrmSolution.php
- classes/models/FrmPersonalData.php
- classes/models/FrmMigrate.php
- classes/models/FrmReviews.php
- classes/controllers/FrmDashboardController.php
- classes/models/FrmFormApi.php
🚧 Files skipped from review as they are similar to previous changes (1)
- square/helpers/FrmSquareLiteConnectHelper.php
🧰 Additional context used
🧬 Code graph analysis (1)
classes/models/FrmAddon.php (1)
classes/controllers/FrmAddonsController.php (1)
get_addon(697-707)
🪛 GitHub Actions: Inspections
classes/models/FrmForm.php
[warning] 1-1: phpdoc_types_order: PHPDoc types order should be corrected by php-cs-fixer.
🪛 GitHub Actions: Psalm Code Analysis
classes/models/FrmForm.php
[error] 715-715: InvalidParamDefault: Default value type '' for argument 1 of method FrmForm::scheduled_delete does not match the given type int (see https://psalm.dev/062)
🪛 GitHub Check: Psalm
classes/models/FrmForm.php
[failure] 715-715: InvalidParamDefault
classes/models/FrmForm.php:715:12: InvalidParamDefault: Default value type '' for argument 1 of method FrmForm::scheduled_delete does not match the given type int (see https://psalm.dev/062)
⏰ 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 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
🔇 Additional comments (38)
classes/controllers/FrmAppController.php (1)
48-52: PHPDoc types foradd_admin_classcorrectly reflect usage
$classesis used and returned as a string of CSS classes throughout the method, so@param stringand@return stringare accurate and improve static analysis without changing behavior. Looks good.classes/helpers/FrmDashboardHelper.php (1)
363-364: LGTM! Documentation addition is correct and consistent.The added PHPDoc annotations accurately describe the method signature and align with the documentation pattern used throughout the class.
classes/helpers/FrmFormsListHelper.php (1)
154-155: LGTM! Documentation improvement aligns with PR objective.The
@param string $whichannotation correctly documents the parameter type, which is confirmed by the string comparison on line 159.stripe/helpers/FrmTransLiteListHelper.php (1)
455-458: LGTM! Documentation improvement.The addition of the
@paramannotation completes the PHPDoc block for theget_amount_columnmethod, bringing it in line with similar methods in the class and improving PHPCS compliance.classes/factories/FrmFieldFactory.php (1)
122-124: LGTM! Documentation improvement is accurate.The added PHPDoc annotations correctly document the method parameters and return type. Using
@return mixedis appropriately defensive given thatapply_filtersat line 130 can return any type depending on registered filter callbacks.classes/models/FrmDb.php (7)
359-362: LGTM!PHPDoc annotations accurately document the parameters and void return type for
maybe_remove_prefix. The "passed by reference" note is helpful for understanding the method's behavior.
455-457: LGTM!Documentation correctly describes the by-reference array parameter and void return type for
esc_query_args.
526-528: LGTM!Accurate documentation for
esc_order_bywith proper indication of the by-reference parameter.
567-570: LGTM!PHPDoc correctly documents both parameters and the string return type for
prepare_array_values.
669-669: LGTM!The
@param int $timeannotation correctly documents the cache expiration parameter.
698-703: LGTM!Complete and accurate PHPDoc block for
set_cachewith all parameters properly typed.
717-720: LGTM!All remaining PHPDoc additions for cache-related methods (
add_key_to_group_cache,get_group_cached_keys,delete_cache_and_transient,cache_delete_group) accurately reflect their signatures and return types.Also applies to: 730-732, 746-748, 761-761
classes/helpers/FrmEntriesListHelper.php (1)
218-226: search_box PHPDoc matches method signature and parent expectationsThe new docblock correctly documents both parameters as strings and the return type as void, aligning with the
search_box( $text, $input_id )signature and the intended no-op implementation here.classes/helpers/FrmEntriesHelper.php (6)
354-365: Docblock for set_parent_field_posted_value accurately reflects usageThe new annotations (
object $field,mixed $value,array $args,@return void) match how the method uses these parameters and the fact that it only mutates$_POSTwithout returning a value.
405-413: get_posted_meta PHPDoc is consistent with implementationDocumenting
$field_idasint|string,$argsasarray, and the return type asmixedaligns with the way the method reads from$_POST['item_meta']and can return either scalars or arrays (or an empty string).
423-432: maybe_set_other_validation parameter docs match by-reference behaviorThe updated PHPDoc correctly marks
$fieldas an object and$value/$argsas array|string/array passed by reference, which mirrors the function signature and how those variables are mutated for “Other” option handling.
464-473: set_other_repeating_vals PHPDoc correctly describes repeating “Other” handlingThe new
@paramannotations for$field,$value, and$args(including by-reference semantics in the descriptions) are accurate given the function’s role in adjusting values for repeating sections.
654-660: actions_dropdown annotations are accurate for attributes arrayDocumenting
$attsas anarrayand the method as@return voidmatches the implementation, which expects an attributes array (includingentryand optionalid) and only echoes markup.
690-697: get_action_links PHPDoc matches parameter types and return shapeThe docblock correctly notes
$idas an int,$entryas array|object (it’s treated as an object withform_id), and@return arrayfor the action definitions passed through thefrm_entry_actions_dropdownfilter.classes/models/FrmOnSubmitAction.php (1)
42-44: LGTM!The PHPDoc annotations correctly document the method parameters with appropriate types.
classes/helpers/FrmCSVExportHelper.php (1)
660-664: LGTM!The PHPDoc block appropriately documents the private static method's parameters and return type.
classes/models/FrmEntryValidate.php (3)
323-325: LGTM!The PHPDoc correctly documents the method parameters.
596-596: LGTM!Parameter documentation added appropriately.
627-629: LGTM!The PHPDoc block properly documents the parameters and return type for this private method.
classes/models/FrmSettings.php (3)
360-361: LGTM!Parameter documentation added appropriately.
449-450: LGTM!Parameter type annotation correctly added.
485-496: Based on my verification, I can now provide the rewritten review comment:
The primary concern about
sanitize_value()return value is incorrect; the implementation is sound.
FrmAppHelper::sanitize_value()accepts$valueas a reference parameter (&$value), modifying it in place rather than returning a sanitized value. The code at lines 492–493 correctly calls this function without capturing a return value. This pattern is used consistently throughout the codebase.The secondary observation about database persistence is accurate:
update_setting()modifies the property in memory only. Callers must explicitly invokestore()to persist changes to the database, as shown inFrmOnboardingWizardController::ajax_consent_tracking(). This is by design.classes/models/FrmForm.php (6)
73-79: LGTM!The PHPDoc clearly documents all parameters for the
duplicate()method.
229-234: LGTM!Parameter documentation appropriately added.
318-319: LGTM!The PHPDoc correctly documents the method parameters.
508-511: LGTM!Parameter documentation properly added, including the by-reference indicator for
$field.
620-622: LGTM!Parameter documentation correctly added to both methods.
Also applies to: 671-673
748-750: LGTM!The PHPDoc annotations appropriately document the method parameters and return types.
Also applies to: 877-880, 997-998, 1212-1214
classes/models/FrmAddon.php (5)
338-347: Docblock formaybe_show_license_messagematches hook usageThe new
@param string $fileand@param array $pluginannotations, plus@return void, line up with howafter_plugin_row_*passes arguments and how this method is used. No issues here.
357-361: Docblock forshow_license_messageis accurateParameter and return types (
string $file,array $plugin,voidreturn) correctly describe the method’s usage and side‑effect‑only behavior.
443-450: Return type forget_api_infois consistent with usageAnnotating
$licenseasstringand the return value asarraymatches how$addonis consumed (array access and(object)casting). This looks correct.
637-643:activate_license_for_pluginannotations look goodTyping both parameters as
stringand the return asarraymatches how the method is called fromactivate()and whatactivate_license()returns. No functional concerns.
649-652:activate_licensedocblock matches implementation
$licenseasstringand thearrayreturn type align with the shape produced byget_license_status()and the response structure consumed by callers. Looks correct.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
classes/controllers/FrmUsageController.php (1)
55-62: Docblock additions are correct and consistent with the method signatureThe new
@param array $schedulesand@return arrayannotations correctly describe the current signature and usage ofadd_schedulesand align with the rest of the file’s style.If you want to go a step further (purely optional), you could document the expected shape of
$schedulesusing WordPress-style array docs, e.g.:/** * @param array $schedules { * @type int $interval Interval in seconds. * @type string $display Human-readable label. * } * @return array */But as-is, this is already an improvement and perfectly acceptable.
classes/views/frm-form-actions/email_action.php (1)
22-27: Documentation addition looks good.The PHPDoc correctly documents the method parameters and return type. The generic
objecttype for$form_actionis valid, though specifying the actual class type (if known) would provide better IDE support and clarity.If the specific class type for
$form_actionis known, consider updating:/** - * @param object $form_action + * @param FrmFormAction $form_action * @param array $args * * @return void */classes/controllers/FrmXMLController.php (2)
156-178: Docblock foroverride_urlmatches usage; optional tighteningThe new
@param array $form/@param string $urlannotations line up with howoverride_url()is called frominstall_template()and how$urlis built. If you want to help static analysis a bit more, you could narrow these in the future to something likearray<string,mixed>for$formand document that$urlis passed by reference (e.g.string &$urlin the description), but this is not required for this PR.
582-599:prepare_types_arrayparam type is fine; consider aligning with callerThe new
@param array $typeis consistent with the waygenerate_xml()calls this method (where$typeis documented asstring[]). Since this method is private and only used there, you could optionally tighten this to@param string[] $typeto better reflect the actual values, but the currentarrayannotation is acceptable.classes/models/FrmUsage.php (1)
481-487: Consider updatingmaybe_json()return type for accuracy
@param mixed $valueis correct, but the function may return a non-string scalar when$valueis not an array. To keep the PHPDoc accurate, consider widening the return type:/** * @since 3.06.04 * * @param mixed $value Value. * - * @return string + * @return mixed */ private function maybe_json( $value ) {This keeps behavior unchanged while making the type comment precise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
classes/controllers/FrmSettingsController.php(1 hunks)classes/controllers/FrmUsageController.php(1 hunks)classes/controllers/FrmXMLController.php(3 hunks)classes/helpers/FrmAppHelper.php(23 hunks)classes/helpers/FrmFieldsHelper.php(9 hunks)classes/helpers/FrmFormMigratorsHelper.php(1 hunks)classes/helpers/FrmOnSubmitHelper.php(1 hunks)classes/models/FrmCreateFile.php(4 hunks)classes/models/FrmEntry.php(3 hunks)classes/models/FrmForm.php(11 hunks)classes/models/FrmUsage.php(4 hunks)classes/models/fields/FrmFieldCaptcha.php(1 hunks)classes/models/fields/FrmFieldHTML.php(1 hunks)classes/models/fields/FrmFieldType.php(11 hunks)classes/views/frm-form-actions/email_action.php(1 hunks)formidable.php(2 hunks)stripe/controllers/FrmTransLiteListsController.php(1 hunks)stripe/models/FrmStrpLiteAuth.php(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- stripe/models/FrmStrpLiteAuth.php
- classes/helpers/FrmOnSubmitHelper.php
- classes/models/fields/FrmFieldCaptcha.php
- classes/helpers/FrmFormMigratorsHelper.php
- stripe/controllers/FrmTransLiteListsController.php
- classes/models/FrmEntry.php
🚧 Files skipped from review as they are similar to previous changes (3)
- classes/helpers/FrmFieldsHelper.php
- classes/models/fields/FrmFieldType.php
- classes/helpers/FrmAppHelper.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). (3)
- GitHub Check: Cypress
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
🔇 Additional comments (24)
formidable.php (2)
61-66: Accurate @param annotation forfrm_forms_autoloader
@param string $class_namecorrectly reflects how the autoloader is invoked and will help IDEs/static analysis without affecting runtime. Looks good.
75-85: Clear, correct parameter docs forfrm_class_autoloaderThe added
@param string $class_nameand@param string $filepathmatch the function’s usage (class name and base directory), and the existing@since/@returnremain consistent. Good incremental improvement to type clarity.classes/models/FrmCreateFile.php (4)
32-33: LGTM! Documentation accurately describes the parameter.The
@param array $attsannotation correctly reflects that the method expects an associative array.
67-68: LGTM! Documentation accurately describes the parameter.The
@param string $file_contentannotation is consistent with how the parameter is used in string operations.
105-106: LGTM! Documentation accurately describes the return type.The
@return stringannotation correctly reflects that this method returns the file contents as a string.
120-123: LGTM! Documentation accurately describes the parameter and return type.Both the
@param string $fileand@return stringannotations correctly reflect the method signature and behavior.classes/models/fields/FrmFieldHTML.php (1)
44-51: Docblock forafter_replace_html_shortcodesmatches implementation.The added
@paramand@returnannotations accurately describe the current usage ($argsas array,$htmlas string, returning string) and don’t affect runtime behavior. This aligns well with the PR’s goal of improving type documentation.classes/controllers/FrmXMLController.php (1)
657-727: Expandedcsvdocblock accurately reflects parameter behaviorThe added param types for
csv()(false|int|string $form_id,string $search,string $fid) match the defaults and how these values are read from request parameters. The docblock now gives a clear contract for callers and looks good as-is.classes/controllers/FrmSettingsController.php (2)
421-422: LGTM! Docblock improvement aligns with PR objectives.The
@return voidannotation improves documentation quality and accurately reflects the function's return type.
424-424: The parameter removal is not a breaking change; the original review comment is incorrect.The
$viewparameter was never actually passed at runtime. The hook is called viado_action( 'frm_after_settings_tabs' )with no additional parameters (verified inclasses/views/frm-settings/form.php:63), and WordPress only passes parameters when they're included in thedo_action()call. The method is registered as a callback via string reference only, with no direct calls in the codebase. Removing the unused parameter is safe and improves code clarity.Likely an incorrect or invalid review comment.
classes/models/FrmUsage.php (3)
46-52: Docblock foruuid()is accurate and consistent
@param bool $regeneratematches how the flag is used (simple truthy check) and aligns with the existing default value. Looks good.
389-395:form_field_count()parameter annotation matches usage
@param int|string $form_idreflects how IDs can be passed around in WP, and@return intmatches theFrmDb::get_countresult. No issues here.
410-416:form_action_count()parameter annotation is appropriateSame pattern as
form_field_count():int|stringfor$form_idandintreturn type both align with usage and thecount()call.classes/models/FrmForm.php (11)
73-79: PHPDoc forduplicate()is precise and helpful.Parameter types and descriptions for
$id,$template,$copy_keys, and$blog_idcorrectly reflect how they’re used and clarify cross‑site behavior.
229-234:update()docblock correctly documents parameters.The summary and types for
$id,$values, and$create_linkmatch the implementation and make the method’s intent clearer.
318-319:update_fields()parameter types look accurate.Documenting
$idas a form ID and$valuesas the form values array aligns with actual usage in the method.
509-511:get_settings_page_html()doc clarifies internal usage.Adding parameter types and noting that
$fieldis passed by reference makes this internal helper safer to call and refactor.
620-620:trash()now documents accepting both int and string IDs.Allowing
int|stringin the docblock matches how$idis passed intogetOne()and keeps static analysis happy when IDs may originate as strings.
671-671:destroy()docblock correctly reflects ID flexibility.The
int|stringtype is consistent withgetOne()usage and the normalization to$form->idinside the method.
715-715:scheduled_delete()union type resolves the previous default mismatch.Documenting
$delete_timestampasint|stringmatches the''default and the later numeric comparison, addressing the earlier static‑analysis complaint without changing behavior.
748-748:getName()docblock correctly allows ID or key.Typing
$idasint|stringand clarifying “Form ID or key” matches the branching onis_numeric( $id )in the implementation.
877-879:getAll()parameter types now match the flexible API.The
array|string$whereandint|string$limitaccurately describe the two call patterns (structured conditions vs raw SQL fragments / numeric limits).
997-998:validate()input description aligns with current filter behavior.Typing
$valuesas an array of form values and documenting the returned errors array is consistent with thefrm_validate_formfilter usage.
1213-1214:get_option()doc documents the expected$attsshape.Specifying
$attsas an array containingform,option, and optionaldefaultclarifies the contract without over‑constraining the type.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
classes/models/fields/FrmFieldNumber.php (1)
170-179: Fix the return type or logic to match the documented intent.The
@return floatdocumentation doesn't match the actual implementation. When$valueis already numeric (e.g., a numeric string like"123"), line 174's condition is false, so the value is returned as-is without casting to float. This contradicts both the return type documentation and the comment stating it should "force the value to be numeric."Choose one of the following fixes:
Option 1 (Recommended): Always cast to float to match the documented return type:
public function set_value_before_save( $value ) { - if ( ! is_numeric( $value ) ) { - $value = (float) $value; - } - - return $value; + return (float) $value; }Option 2: Update return type to reflect actual behavior:
/** * Force the value to be numeric before it's saved in the DB * * @param array|string $value - * @return float + * @return float|string|int */classes/helpers/FrmXMLHelper.php (1)
434-446: Broaden$this_formparameter type inimport_xml_fieldsdocblock
$this_formcan befalsewhenmaybe_get_form()doesn’t find a matching form, and the code branches on its emptiness. The docblock currently advertises onlyobject, which is misleading for static analysis and readers.Consider updating the annotation to reflect the actual usage, e.g.:
- * @param object $this_form + * @param object|false $this_form(or a more specific union like
FrmForm|falseif you prefer).
🧹 Nitpick comments (6)
classes/models/FrmSpamCheckWPDisallowedWords.php (1)
57-62: Docblock types look correct; consider brief param descriptions.The
@param stringtypes match how these values are built and forwarded to the WP spam APIs, so the annotation is accurate. If you want to further improve readability/IDE help, you could optionally add a short description per parameter (e.g.,@param string $author Comment author name.), but this is non-blocking.classes/controllers/FrmAddonsController.php (1)
749-754:prepare_addonsparam type is correct; consider narrowing array shape (optional)
@param array $addonsmatches usage. If you want to aid static analysis later, you could refine this to something likearray<int|string, array<string,mixed>> $addons, but it’s not required for this PR.classes/controllers/FrmFormActionsController.php (4)
118-126: Docblock formaybe_add_action_to_groupis accurate; consider tighter array typingThe new
@param array $action_controls/@param array $groupsand@return voidlines match the implementation. If you want stronger static analysis, you could narrow these to something likeFrmFormAction[]for$action_controlsand a structured array type (e.g.array<string, array{actions?: string[]}>) for$groups, but what you have is already correct for this PR’s goal.
474-480:fields_to_valuesdocblock is correct but could name the concrete return typeThe new docblock correctly documents
$form_idasint|stringand$valuesas an array passed by reference, and@return objectis technically true. For better tooling support, you might consider using the concrete type (e.g.@return FrmForm) orFrmForm|null/FrmForm|falseifFrmForm::getOne()can fail.
569-576:trigger_create_actionsPHPDoc aligns with implementationThe added annotations for
$entry_id,$form_id(int|string),$args(array), and@return voidare consistent with how this method is used and how it forwards data intotrigger_actionsand the filter. If any callers ever pass an entry object instead of an ID, you may want to extend the param type to includeFrmEntry, but as-is this matches current intent.
760-768: Condition in_register_actionslooks inverted; worth a quick re-checkUnrelated to this PR, but while here: in
Frm_Form_Action_Factory::_register_actions(), you build$keys = array_keys( $this->actions );and then only call_register()when! isset( $this->actions[ $key ] ). Since$keycomes from the existing keys,issetwill always be true, so_register()is never called. That seems opposite to the comment “don’t register new action if old action with the same id is already registered”. It might be intentional/legacy, but it’s worth double‑checking whether this should beif ( isset( $this->actions[ $key ] ) )instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
classes/controllers/FrmAddonsController.php(6 hunks)classes/controllers/FrmFieldsController.php(1 hunks)classes/controllers/FrmFormActionsController.php(3 hunks)classes/controllers/FrmFormsController.php(11 hunks)classes/controllers/FrmSMTPController.php(2 hunks)classes/helpers/FrmAppHelper.php(30 hunks)classes/helpers/FrmFieldsHelper.php(9 hunks)classes/helpers/FrmFormsHelper.php(6 hunks)classes/helpers/FrmXMLHelper.php(14 hunks)classes/models/FrmEntry.php(4 hunks)classes/models/FrmFormAction.php(5 hunks)classes/models/FrmFormMigrator.php(2 hunks)classes/models/FrmSpamCheckWPDisallowedWords.php(1 hunks)classes/models/FrmStyle.php(1 hunks)classes/models/fields/FrmFieldCheckbox.php(2 hunks)classes/models/fields/FrmFieldDefault.php(2 hunks)classes/models/fields/FrmFieldNumber.php(3 hunks)classes/models/fields/FrmFieldRadio.php(2 hunks)classes/models/fields/FrmFieldSelect.php(1 hunks)classes/models/fields/FrmFieldType.php(15 hunks)classes/models/fields/FrmFieldUrl.php(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- classes/controllers/FrmFormsController.php
- classes/controllers/FrmSMTPController.php
🚧 Files skipped from review as they are similar to previous changes (4)
- classes/helpers/FrmFieldsHelper.php
- classes/models/fields/FrmFieldUrl.php
- classes/helpers/FrmAppHelper.php
- classes/models/FrmEntry.php
🧰 Additional context used
🪛 GitHub Actions: Psalm Code Analysis
classes/models/FrmFormAction.php
[error] 351-351: Psalm: AssignmentToVoid: Cannot assign $post_id to type void (see https://psalm.dev/121)
⏰ 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 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
🔇 Additional comments (35)
classes/models/FrmStyle.php (1)
69-73: Docblock update forduplicate()looks correctThe new
@param int|string $iddescription matches how IDs are used elsewhere (including the constructor) and adds helpful context without changing behavior.classes/models/fields/FrmFieldDefault.php (2)
30-31: LGTM! Documentation improvement.The added
@param string $nameannotation correctly documents the parameter type, which is consistent with the default value''in the method signature.
52-53: LGTM! Documentation improvement.The added
@param array $shortcode_attsannotation correctly completes the parameter documentation for this method, following the existing PHPDoc conventions.classes/controllers/FrmAddonsController.php (5)
513-519: Docblock forcheck_updatematches behavior
@param mixed $transientand@return objectare consistent with how$transientis normalized and returned from this method.
815-821:is_plugin_activeparam annotations look accurateBoth
$file_nameand$slugare used as strings, so the new docblock types are appropriate.
839-846:prepare_addon_linkparam type matches usage
$linkis always treated and returned as a string; the new@param string $linkis correct.
859-869:set_addon_statusparam annotation is consistent
$addonis an associative array mutated in-place;@param array $addonaligns with current usage.
1500-1509:get_addon_required_planparam/return types correctly documented
$addon_idcan be an int or string array key, and the method always returns a string (empty or required plan), so the new docblock matches behavior.classes/models/FrmFormMigrator.php (2)
162-165: LGTM! Clear documentation for import_form method.The PHPDoc accurately documents the parameter type and return type for this method.
216-221: LGTM! Accurate documentation for prepare_fields method.The PHPDoc correctly documents both array parameters and the void return type. The by-reference parameter is also properly indicated in the method signature.
classes/helpers/FrmFormsHelper.php (6)
309-312: LGTM! Documentation is accurate.The PHPDoc correctly documents the parameter type and return type for this method.
363-368: LGTM! Documentation is accurate.The PHPDoc correctly documents all parameter types and the return type for this method.
802-808: LGTM! Documentation is accurate.The PHPDoc correctly documents all parameter types. The function modifies parameters by reference and doesn't return a value, which is correctly reflected by the absence of a
@returntag.
910-912: LGTM! Documentation is accurate.The PHPDoc correctly documents the flexible parameter type and the return type, which matches the implementation.
230-234: LGTM! Documentation is accurate.The PHPDoc correctly documents the parameter types and return type for this method.
551-554: LGTM! Documentation is accurate.The PHPDoc correctly documents the parameter types and void return type for this method.
classes/controllers/FrmFieldsController.php (1)
703-705: LGTM! Documentation improvement adds clarity.The added
@param array|object $fieldannotation accurately reflects that this method accepts both array and object representations of field data, which is consistent with the usage ofFrmField::get_option()and similar patterns elsewhere in the codebase (e.g., line 794).classes/models/fields/FrmFieldNumber.php (2)
52-54: LGTM! Type comments are accurate.The added parameter type documentation correctly reflects the method signature and usage.
184-184: LGTM! Type comment is appropriate.The parameter type documentation correctly reflects that the method accepts and sanitizes array or string values via reference.
classes/models/fields/FrmFieldCheckbox.php (2)
62-71: Docblock fordisplayed_field_typecorrectly reflects flexible$fieldtypeAnnotating
$fieldasarray|objectmatches how field data is typically passed around and brings this method in line with similar field classes, without changing behavior.
88-96:show_priority_field_choicesannotations align with actual usageThe new
@param array $argsand@return voidaccurately describe the method signature and side-effect-only behavior (including a view), improving static analysis without impacting runtime.classes/models/fields/FrmFieldSelect.php (1)
52-61:displayed_field_type$fieldparam annotation is consistent and safeAdding
@param array|object $fieldaligns this select field with the other field types and matches the loose input shape actually accepted, with no behavioral change.classes/models/fields/FrmFieldRadio.php (2)
53-62:displayed_field_typedocblock matches usage across field typesDocumenting
$fieldasarray|objectis appropriate for this loosely-typed input and keeps the radio implementation consistent with checkbox and select fields.
93-100:show_priority_field_choicesannotations accurately describe the signatureThe
@param array $argsand@return voidtags align with the method’s actual signature and side-effect-only behavior (view include), and bring it in line with the checkbox implementation.classes/models/fields/FrmFieldType.php (5)
439-442: LGTM! Documentation improvement enhances code clarity.The added PHPDoc comments properly document the method parameters with accurate type annotations.
481-482: LGTM! Consistent parameter documentation.The PHPDoc additions across these methods provide clear parameter and return type information that aligns with the implementations.
Also applies to: 602-603, 620-622, 632-634
732-733: LGTM! Clear documentation for method signatures.The PHPDoc additions effectively document parameter types and behaviors, particularly the reference parameter notation which helps developers understand side effects.
Also applies to: 911-911, 974-979
1167-1170: LGTM! Thorough parameter and return type documentation.The PHPDoc additions correctly document parameter types, including reference parameters, and return types that match the method implementations.
Also applies to: 1179-1181, 1280-1285, 1299-1302
1360-1365: LGTM! Comprehensive documentation coverage.The final set of PHPDoc additions completes the documentation effort with accurate type information and clear descriptions, improving IDE support and code maintainability.
Also applies to: 1467-1468, 1774-1776
classes/controllers/FrmFormActionsController.php (1)
462-468:should_show_log_messagePHPDoc matches usageThe added
@param string $action_typeand@return boolaccurately reflect how this helper is used and what it returns. No further changes needed here.classes/models/FrmFormAction.php (1)
182-187: PHPDoc additions for constructors and callbacks look consistentThe new
@paramand@returnannotations for the PHP4-style constructor (FrmFormAction),prepare_new,duplicate_form_actions, andupdate_callbackmatch how these methods are actually used (including thefalse|int|stringunion for$form_idand thearray|nullreturn forupdate_callback). These should help static analysis without changing behavior.Also applies to: 270-273, 313-317, 420-427
classes/helpers/FrmXMLHelper.php (4)
358-363: Docblock fordelete_removed_fieldsmatches implementation
$form_fieldsis treated purely as an array and the method has no return value, so@param arrayand@return voidare accurate.
560-565: Field default/required helper docblocks are consistent with usageFor
set_default_value,maybe_add_required,maybe_update_in_section_variable, andmaybe_update_get_values_form_setting, the new@param array(and@param int $in_section) plus@return voidannotations accurately match the function signatures and how the parameters are used.Also applies to: 589-596, 603-610, 653-660
673-680: Field migration helper annotations look correctThe updated docblocks for
run_field_migrations,migrate_placeholders, andmigrate_field_placeholdercorrectly document array parameters and the return value (arrayformigrate_field_placeholder,voidfor the others). These line up with the implementations and improve static tooling.Also applies to: 686-691, 704-713
754-762: Form/field/style helper docblocks align with behavior
create_imported_fieldandmaybe_update_field_idsnow explicitly returnvoid, which matches their side-effect-only implementations.update_custom_style_setting_on_importandupdate_custom_style_setting_after_importcorrectly annotate$formasarrayand$form_idasint|stringwithvoidreturn.clear_forms_style_caches’s@param array $imported_formsand@return voidalso match the implementation.These annotations look accurate and help clarify intent.
Also applies to: 811-818, 852-860, 892-900, 1048-1053
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 (2)
classes/helpers/FrmAppHelper.php (1)
1975-1989: Critical XSS vulnerability in post edit link.The
post_edit_linkmethod constructs an HTML link but fails to properly escape the post title. Whileself::truncate()strips HTML tags (viawp_strip_all_tags()), it doesn't escape HTML entities. This means a post title likeTest & <script>would becomeTest & <script>after tag stripping, but the&and other special characters remain unescaped in the HTML output, potentially allowing XSS attacks through carefully crafted post titles.Apply this diff to fix the vulnerability:
public static function post_edit_link( $post_id ) { $post = get_post( $post_id ); if ( $post ) { $post_url = admin_url( 'post.php?post=' . $post_id . '&action=edit' ); - return '<a href="' . esc_url( $post_url ) . '">' . self::truncate( $post->post_title, 50 ) . '</a>'; + return '<a href="' . esc_url( $post_url ) . '">' . esc_html( self::truncate( $post->post_title, 50 ) ) . '</a>'; } return ''; }classes/models/FrmFormAction.php (1)
340-361: Complete the @return type annotation.The
maybe_create_action()method can return eitherintorWP_Errorsince it returns the result ofsave_settings()(line 354) orduplicate_one()(line 358), both of which returnint|WP_Error. However, the docblock at line 348 only declares@return int.Update the return type annotation:
/** * Check if imported action should be created or updated. * * @since 2.0 * * @param array $action * @param array $forms * - * @return int $post_id + * @return int|WP_Error $post_id */
🧹 Nitpick comments (4)
classes/models/FrmFieldFormHtml.php (1)
168-184: Docblock formaybe_replace_description_shortcodematches signature; consider clarifying the flag meaningThe added
@param bool $wp_processedand@return voidcorrectly describe the method. For extra clarity, you might briefly document when$wp_processedis expected to be true (e.g., “true afterprocess_wp_shortcodes()has run, false before”) since the flag controls subtle behavior for HTML vs non-HTML fields.classes/models/FrmFormAction.php (2)
665-672: Consider broadening the type annotation for consistency.Line 665 declares
@param int $form_id, but other methods in this class useint|stringfor form ID parameters (e.g.,duplicate_form_actions()at line 319,update_callback()at line 432). The underlyingget_action_for_form()method also accepts form IDs asint|string.For consistency across the codebase, consider:
- * @param int $form_id + * @param int|string $form_id * @param string $type
732-734: Consider usingint|stringfor form_id parameter.The annotation declares
@param int $form_id, but this method is called with form IDs that may be eitherintorstring(e.g., from line 575 viaself::action_args( $form_id, $limit )). Other methods in this class useint|stringfor form ID parameters for consistency.- * @param int $form_id + * @param int|string $form_id * @param int $limitclasses/controllers/FrmFormsController.php (1)
1664-1700: Improved docs foradvanced_helpers()Annotating
$attsasarrayand the return asarrayis consistent with calls viacompact( 'fields', 'form_id' )and the structure of$advanced_helpers. If desired, you could further narrow this to a shaped array, but not required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
classes/controllers/FrmEntriesController.php(12 hunks)classes/controllers/FrmFormActionsController.php(6 hunks)classes/controllers/FrmFormsController.php(12 hunks)classes/controllers/FrmHooksController.php(1 hunks)classes/helpers/FrmAppHelper.php(31 hunks)classes/helpers/FrmEmailSummaryHelper.php(1 hunks)classes/helpers/FrmEntriesListHelper.php(3 hunks)classes/helpers/FrmFormActionsHelper.php(1 hunks)classes/helpers/FrmFormsHelper.php(12 hunks)classes/helpers/FrmXMLHelper.php(32 hunks)classes/models/FrmEntryMeta.php(3 hunks)classes/models/FrmField.php(8 hunks)classes/models/FrmFieldFormHtml.php(2 hunks)classes/models/FrmFormAction.php(13 hunks)classes/models/FrmSolution.php(8 hunks)classes/views/styles/components/FrmSliderStyleComponent.php(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- classes/models/FrmEntryMeta.php
- classes/views/styles/components/FrmSliderStyleComponent.php
- classes/helpers/FrmEmailSummaryHelper.php
- classes/controllers/FrmEntriesController.php
🚧 Files skipped from review as they are similar to previous changes (4)
- classes/models/FrmField.php
- classes/helpers/FrmFormsHelper.php
- classes/models/FrmSolution.php
- classes/helpers/FrmXMLHelper.php
🧰 Additional context used
🪛 PHPMD (2.15.0)
classes/helpers/FrmEntriesListHelper.php
224-224: Avoid unused parameters such as '$text'. (undefined)
(UnusedFormalParameter)
224-224: Avoid unused parameters such as '$input_id'. (undefined)
(UnusedFormalParameter)
⏰ 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: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (33)
classes/models/FrmFieldFormHtml.php (1)
208-235: Docblock foradd_element_idcorrectly types parameters and returnThe new
@param string $param,@param string $id, and@return voidannotations align with how the method is used (description/errorsections and generated IDs) and with its void behavior. No further changes needed.classes/helpers/FrmEntriesListHelper.php (2)
219-226: LGTM!The PHPDoc comments properly document the method signature. The unused parameters are expected for this stub method that's implemented in the pro version.
311-315: LGTM!The PHPDoc comments properly document both parameters and the return type. The documentation is clear and accurate.
classes/helpers/FrmFormActionsHelper.php (1)
11-14: LGTM! Type annotations align with method signature.The added parameter annotations correctly document the types and match the method's signature and default values.
classes/models/FrmFormAction.php (11)
365-368: LGTM! Return type correctly fixed.The
@returnannotation was updated fromvoidtoint|WP_Erroras requested in the previous review, correctly reflecting thatduplicate_one()returns the result ofsave_settings().
184-186: LGTM! Constructor wrapper annotations are correct.The added parameter annotations for the PHP4-style constructor wrapper match the method signature and align with the main
__construct()method.
209-212: LGTM! Type annotations are accurate.The annotations correctly document that
get_group()accepts an array parameter and returns an array element from the$groupscollection.
241-241: LGTM! Parameter annotation added.The
@param string $post_fieldannotation correctly documents the second parameter with its default value of'post_content'.
276-277: LGTM! Union type correctly documents optional parameter.The
false|int|stringunion type accurately reflects that$form_iddefaults tofalsebut can accept integer or string form IDs.
319-321: LGTM! Parameter annotations match usage.Both form ID parameters are correctly typed as
int|stringto handle the various form ID formats used throughout the codebase.
431-434: LGTM! Return type accurately reflects conditional returns.The
array|nullreturn type correctly documents that the method returns an array of action IDs on success ornullwhen returning early.
647-649: LGTM! Type annotations are precise.The annotations correctly specify that
$action_idis an integer and the method returns eitherfalseor aFrmFormActioninstance.
675-680: LGTM! Complete PHPDoc block added.The annotations accurately document all parameters and the return type, with the
false|int|stringunion type properly reflecting the optional form ID parameter.
754-755: LGTM! WP_Post type annotations are correct.The method receives a
WP_Postobject, modifies its properties, and returns the modified object, which is accurately reflected in the annotations.
783-785: LGTM! Parameter annotations match method signature.The union type
false|int|stringfor$form_idandstringfor$typecorrectly document the method's parameters and their defaults.classes/controllers/FrmFormActionsController.php (7)
122-125: LGTM! Annotations correctly document the method.The parameter and return type annotations accurately reflect the method signature and behavior, including the array parameters and void return.
231-231: LGTM! Parameter annotation added.The
@param array $action_controlscorrectly documents the array parameter used in the foreach loop.
330-334: LGTM! Method documentation is accurate.The annotations correctly specify that
list_actions()accepts a form object and values array, and returns void.
470-473: LGTM! Type annotations match the implementation.The
@param stringand@return boolannotations accurately document the method's signature and return value fromin_array().
480-485: LGTM! Complete PHPDoc block is well-formed.The annotations correctly document the form ID as
int|string, the array parameter passed by reference, and the object return type fromFrmForm::getOne().
575-581: LGTM! Parameter types are appropriate.The
int|stringunion types for$entry_idand$form_idcorrectly accommodate the various ID formats used throughout the codebase.
600-606: LGTM! Complex parameter types are well-documented.The annotations accurately capture that
$formcan be an int ID, string ID, or object, and$entrycan be an int or string ID. This reflects the method's flexible signature for handling various input formats.classes/controllers/FrmFormsController.php (10)
75-89: Docblock forprevent_divi_conflictmatches usageThe added
@param array $shortcodesand@return arrayannotations correctly describe how the filter is used and what it returns.
201-205: Typed PHPDoc foredit()reflects current usageDeclaring
@param array|false $valuesand@return voidaligns with howroute()passes$varsand howedit()behaves; no functional impact.
281-285:update()param doc now constrained to array
@param array $valuesmatches all in-file call sites (includingcreate()androute()), which always pass arrays, so this tighter type hint is accurate.
632-646: Clarified PHPDoc forpreview_page_title()Adding
@param string $titleand@return stringmatches the filter signature and implementation and helps static analysis.
1703-1732: Return type doc forget_advanced_shortcodes()The new
@return arraycommunicates intent and matches the associative array returned; no behavior change.
1734-1752: Return type doc foruser_shortcodes()The added
@return arrayannotation matches the$optionsstructure and downstream usage viaapply_filters( 'frm_user_shortcodes', $options ).
1757-1793: PHPDoc tightened forget_shortcode_helpers()and its filterDocumenting
@param bool $settings_taband@return arrayplus the filter param docs ($entry_shortcodes,$settings_tab) aligns with current usage inmb_tags_box()and existing filter behavior.
1429-1437: Docblock forform_publish_button()The new
@param array $atts/@return voiddoc matches how$attsis used ($atts['values']) and the view include; this is accurate and non-breaking.
2151-2159: Docblock forno_reports()Adding
@param array $valuesis consistent with theroute()call (self::no_reports( $vars )) even though it’s currently unused, and@return voidmatches behavior.
1336-1355: Filter hook argument mismatch creates silent data loss; verify intended behaviorTwo related changes require clarification:
update_form_builder_fieldsnow accepts only$fields, but the filter hook at line 1318 passes two arguments:apply_filters( 'frm_fields_in_form_builder', $fields, compact( 'form' ) ). The hook registration (FrmHooksController.php:154) lacks a thirdaccepted_argsparameter, so it defaults to 1. This causes the second argument to be silently dropped for any filter callback. While no direct two-argument calls exist in the codebase, the filter hook mismatch means thecompact( 'form' )context is never received by this callback or any other callback on that filter. If that context is needed, either addaccepted_args => 2to the filter registration or remove the second argument fromapply_filters.
maybe_update_form_builder_message( &$message )correctly mapsmessage=form_duplicatedto a friendly notice inget_edit_vars()(line 1325). The logic is sound. Consider addingif ( '' === $message )guard to avoid overriding an explicit caller-provided message, though this is not critical in the current flow.classes/controllers/FrmHooksController.php (1)
150-156: Filter registration updated to match new callback signatureDropping the explicit
(10, 2)onadd_filter( 'frm_fields_in_form_builder', 'FrmFormsController::update_form_builder_fields' )correctly aligns the hook with the new one-parameterupdate_form_builder_fields( $fields ). Other listeners onfrm_fields_in_form_buildercan still opt into the second argument by registering with$accepted_args = 2, so this should not affect third‑party filters.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
classes/models/fields/FrmFieldType.php (1)
910-917: Front‑end rendering and HTML helper docblocks look correctFor
get_field_scripts_hook,after_replace_html_shortcodes,add_extra_html_atts,add_min_max,show_single_hidden,get_select_box,select_tag, andload_field_scripts, the added@paramand@returntags match the current method signatures and how arguments/results are used (arrays of args, HTML fragments as strings, no returns where declaredvoid). These will aid static analysis without affecting behavior. One tiny nit is that the$fieldparam descriptions saying “Field settings array” (while typedarray|object) could be relaxed to “Field settings” for strict consistency, but that’s purely cosmetic.Also applies to: 974-982, 1162-1174, 1176-1201, 1280-1295, 1297-1323, 1325-1338, 1364-1372
classes/helpers/FrmFormsHelper.php (1)
1118-1157: Consider narrowing$form_iddoc type to scalar IDsThe method treats
$form_idas an identifier that’s concatenated into URLs and passed to helpers expecting an ID, while$formis the place where an object is normalized viaFrmForm::getOne. To avoid implying that passing an object as$form_idis supported, consider updating the PHPDoc to:@param int|string $form_id @param mixed $form @return arrayThis better reflects current usage patterns and reduces the chance of someone passing an object where a scalar ID is expected.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
classes/controllers/FrmEntriesController.php(14 hunks)classes/controllers/FrmFormsController.php(19 hunks)classes/helpers/FrmAppHelper.php(32 hunks)classes/helpers/FrmFormsHelper.php(13 hunks)classes/models/FrmFormAction.php(15 hunks)classes/models/FrmFormMigrator.php(6 hunks)classes/models/fields/FrmFieldType.php(16 hunks)phpstan.neon(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- classes/models/FrmFormAction.php
- classes/controllers/FrmFormsController.php
- classes/helpers/FrmAppHelper.php
🧰 Additional context used
🧬 Code graph analysis (1)
classes/controllers/FrmEntriesController.php (1)
classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-4786)ips_saved(347-350)
⏰ 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: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (27)
phpstan.neon (1)
137-139: <function_calls>
#!/bin/bashCheck phpstan.neon configuration
echo "=== phpstan.neon content around line 137-139 ==="
sed -n '130,150p' phpstan.neonecho -e "\n=== Count of ignoreErrors in phpstan.neon ==="
rg -c 'message:' phpstan.neonecho -e "\n=== Check if FrmFormAction has other suppressions ==="
rg -n 'FrmFormAction' phpstan.neonecho -e "\n=== Check phpstan.neon header for configuration ==="
head -30 phpstan.neon
</function_calls># Look for WordPress stubs or type definitions echo -e "\n=== Check composer.json for phpstan/wordpress versions ===" jq '.require | keys[]' composer.json 2>/dev/null | grep -i phpstan echo -e "\n=== Check if type casts exist elsewhere in codebase ===" rg -n '\(WP_Post\)' --type=php | head -20 </parameter> </invoke> </invoke> </function_calls> </blockquote></details> <details> <summary>classes/controllers/FrmEntriesController.php (16)</summary><blockquote> `134-138`: **LGTM!** The PHPDoc accurately documents the parameter types and return type for this screen options filter method. --- `217-222`: **LGTM!** The PHPDoc correctly documents the method parameters including the pass-by-reference `$columns` array, and the void return type. --- `242-247`: **LGTM!** The PHPDoc is consistent with similar methods and accurately documents the parameters and return type. --- `286-291`: **LGTM!** The PHPDoc accurately documents this conditional column addition method. --- `298-306`: **LGTM!** The PHPDoc correctly documents this filter callback with appropriate parameter types. --- `326-332`: **LGTM!** The PHPDoc accurately documents this metadata update hook callback. --- `384-387`: **LGTM!** The PHPDoc correctly documents the parameter and return type for this utility method. --- `397-400`: **LGTM!** The PHPDoc correctly documents this utility method that generates column keys. --- `412-418`: **LGTM!** The PHPDoc accurately documents this filter callback for the per-page option. --- `502-506`: **LGTM!** The PHPDoc correctly documents this method that filters hidden columns for a specific form. --- `598-602`: **LGTM!** The PHPDoc accurately documents this method that calculates and stores the trash deletion time. --- `616-618`: **LGTM!** The PHPDoc correctly documents this entry display method. --- `762-770`: **LGTM!** The PHPDoc accurately documents this hook callback that deletes entries before redirecting. --- `781-781`: **LGTM!** The addition of the `@return void` completes the PHPDoc for this method. --- `787-794`: **LGTM!** The PHPDoc accurately documents this private entry deletion method. --- `809-812`: **LGTM!** The PHPDoc accurately documents this private method that unlinks entries from posts. </blockquote></details> <details> <summary>classes/models/FrmFormMigrator.php (2)</summary><blockquote> `162-166`: **LGTM! Accurate PHPDoc documentation.** The parameter and return types are correctly documented for the `import_form` method. --- `428-433`: **LGTM! Accurate PHPDoc documentation.** The parameter and return types are correctly documented for the `form_creation_error_response` method. </blockquote></details> <details> <summary>classes/models/fields/FrmFieldType.php (2)</summary><blockquote> `437-446`: **Builder/options docblocks match actual usage** The added `@param`/`@return` annotations for `show_options`, `show_field_options`, `show_single_option`, `has_field_choices`, and `displayed_field_type` correctly reflect how these methods are invoked and what they return (arrays and booleans keyed off field type). Nothing here changes runtime behavior, and the types look accurate for the current implementations. Also applies to: 479-510, 600-607, 616-626, 631-646 --- `730-746`: **Value-normalization helper docs are consistent with signatures** The new/expanded docblocks on `default_value_to_string`, `maybe_trim_excess_values`, and `sanitize_value` align with the pass‑by‑reference signatures and the value shapes actually handled (`mixed` / `array|string`). The “passed by reference” descriptions are helpful for readers and tools; no issues from a type or behavior standpoint. Also applies to: 1470-1476, 1776-1788 </blockquote></details> <details> <summary>classes/helpers/FrmFormsHelper.php (6)</summary><blockquote> `307-382`: **Docblocks for `setup_new_vars` and `setup_edit_vars` accurately describe types** The new `@param`/`@return` annotations for these two helpers match how `$values`, `$record`, and `$post_values` are used and the fact they always return an array. No behavioral change and the documentation is consistent with the implementation. --- `799-827`: **`maybe_create_end_section` PHPDoc matches implementation** The new parameter annotations (`bool`, `int`, `array`, `object`, `string`) line up with how each argument is used, and omitting a `@return` tag is appropriate for this void helper. --- `909-948`: **`get_form_style_class` union types and nullable return are correct** Allowing `array|bool|int|object|string` for `$form` and documenting `string|null` for the return value matches all current code paths in this method. --- `1226-1334`: **Trash/delete helper PHPDocs line up with behavior** The added annotations for `delete_trash_link`, `format_link_html`, `delete_trash_info`, and `delete_trash_links` correctly describe the integer ID, string status/length, array structures, and return types (string or array). This clarifies usage without altering behavior. --- `1374-1430`: **Layout and status helper PHPDocs are accurate** The new `@return array` for `grid_classes` and the detailed annotations for `style_class_label` and `status_nice_name` accurately reflect the existing implementations and will make consuming these helpers clearer. --- `1582-1600`: **`show_plan_required` parameter and void return docs match usage** The added PHPDoc for `$requires` and `$link` as strings, and `@return void`, matches the control flow and echo-based behavior of this helper. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
classes/models/FrmFormAction.php (3)
351-368: Align maybe_create_action() return type with actual return
maybe_create_action()returns$this->save_settings()or$this->duplicate_one(), both of which areint|WP_Error, but the docblock currently says@return int $post_id. This can mislead consumers and static tools.Consider updating the return annotation:
- * @return int $post_id + * @return int|WP_Error $post_id
657-674: Correct get_single_action_type() return type (currently misleading)
get_single_action_type()returnsfalsewhen$typeis empty, otherwise it returns the result of$action_control->get_single_action($action_id), which isobject|null(WP_Post|null), notFrmFormAction.The current
@return bool|FrmFormActionis inaccurate.Recommend updating to something along these lines:
- * @return bool|FrmFormAction + * @return false|WP_Post|null(or
false|object|nullif you prefer not to tie it toWP_Post).
687-741: Clarify get_all() return type – can be array or single action
get_all()returns:
array()when no actions,- an array of actions when
$limit > 1,- a single action object when
$limit === 1(afterreset($settings)).The new
@return arrayannotation is therefore incomplete.Consider a more accurate return type such as:
- * @return array + * @return array|object(or a more specific union if you want to model the
limit === 1case).
♻️ Duplicate comments (4)
classes/models/FrmFormMigrator.php (4)
216-221: Document the pass-by-reference parameter.The
$formparameter is passed by reference (line 222) but the PHPDoc doesn't indicate this. Since this PR focuses on adding type comments, the documentation should accurately reflect that this parameter is mutated.
293-295: Document the pass-by-reference parameter.The
$fieldsparameter is passed by reference (line 297) but the PHPDoc doesn't indicate this. In a documentation-focused PR, it's important to accurately reflect that this parameter is mutated.
331-335: Document the pass-by-reference parameters.Both
$fieldsand$orderare passed by reference (line 337) but the PHPDoc doesn't indicate this. Both parameters are mutated by this method.
350-356: Document the pass-by-reference parameter.The
$fieldsparameter is passed by reference (line 358) but the PHPDoc doesn't indicate this. This parameter is directly modified viaarray_splice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
classes/controllers/FrmFormsController.php(24 hunks)classes/models/FrmFormAction.php(16 hunks)classes/models/FrmFormMigrator.php(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- classes/controllers/FrmFormsController.php
🧰 Additional context used
🪛 GitHub Actions: Psalm Code Analysis
classes/models/FrmFormAction.php
[error] 281-281: InvalidDocblock: Found duplicated @param or prefixed @param tag in docblock for FrmFormAction::prepare_new (see https://psalm.dev/008)
⏰ 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: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (16)
classes/models/FrmFormMigrator.php (3)
162-165: LGTM!The PHPDoc correctly documents the parameter and return type for the
import_formmethod.
428-431: LGTM!The PHPDoc correctly documents the parameter and return type for the
form_creation_error_responsemethod.
457-459: LGTM!The PHPDoc correctly documents the parameter types (including the appropriate union type for
$form_id) and return type for thecreate_emailsmethod.classes/models/FrmFormAction.php (13)
182-190: Constructor PHPDoc additions look correctThe added param docs for
$id_base,$name,$action_options, and$control_optionsmatch the signature and are clear.
207-215: get_group() docblock matches usageThe new
@param array $action_optionsand@return arrayannotations correctly reflect the method’s contract.
235-245: get_field_name() param annotation is accurateAdding
@param string $post_fieldaligns with the default'post_content'and helps static analysis.
311-322: create() return type docblock is consistent
create()ultimately returnsFrmDb::save_settings()which yieldsint|WP_Error; the new@return int|WP_Erroris correct and helpful.
324-344: duplicate_form_actions() docblock looks goodThe
@param int|string $form_id/$old_idand@return voidannotations match the implementation (no return value).
370-403: duplicate_one() docblock now correctly reflects return typeThe updated
@return int|WP_Errormatches the call tosave_settings()and resolves the earlier mismatch with callers.
433-514: update_callback() annotations match behavior
@param int|string $form_idand@return array|nullcorrectly describe that the method either returns an array of IDs or early-returnsnull.
549-563: get_single_action() docblock is reasonableThe new
@param int $idand@return object|nullmirror theget_post()+prepare_action()flow and are acceptable (even if the concrete type isWP_Post).
676-685: form_has_action_type() change is cleanRenaming to
$actionsand using! empty( $actions )with@return boolkeeps behavior intact and improves clarity.
743-763: action_args() docblock matches usage
@param int $form_id,@param int $limit, and@return arrayalign with the query args being built.
765-792: prepare_action() PHPDoc is accurateTyping the parameter and return as
WP_Postis consistent withget_post()usage and the fact that you’re mutating and returning the same post object.
794-818: destroy() param types look correct
@param false|int|string $form_idand@param string $typedescribe the inputs well, and@return voidmatches the implementation.
865-912: migrate_to_2() docblock improvements are correctThe added
@param object $form,@param string $update, and@return int|WP_Errorlines match the logic (migration may create/update an action and return the resulting post ID or WP_Error).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
classes/models/FrmFormAction.php (2)
345-367: Include WP_Error in maybe_create_action() @return type
maybe_create_action()returns$post_idcoming from:
save_settings( $action ), which returnsint|WP_Error, andduplicate_one(), which now correctly documents and returnsint|WP_Error.So the current docblock:
* @return int $post_idis too narrow. To keep type information accurate and avoid confusing static analysis, it should reflect the union:
- * @return int $post_id + * @return int|WP_Error
duplicate_one()’s new@return int|WP_Errorannotation is correct and consistent withsave_settings().Also applies to: 369-402
793-811: migrate_to_2() @return annotation doesn’t match possible array return
migrate_to_2()is documented as:* @return int|WP_Errorbut in practice:
$post_id = get_posts( [ ..., 'numberposts' => 1 ] ); if ( empty( $post_id ) ) { $post_id = $this->save_settings( $action ); // int|WP_Error } // ... return $post_id;When an action already exists,
$post_idis the array returned byget_posts(), not an int or WP_Error. So actual return type isint|WP_Error|array.To reconcile this you have two options:
Normalize to an ID (recommended if the intended contract is “ID or error”):
$post_id = get_posts(array('name' => $action->post_name,'post_type' => FrmFormActionsController::$action_post_type,'post_status' => $action->post_status,'numberposts' => 1,));if ( empty( $post_id ) ) {// create action now$post_id = $this->save_settings( $action );}
$posts = get_posts(array('name' => $action->post_name,'post_type' => FrmFormActionsController::$action_post_type,'post_status' => $action->post_status,'numberposts' => 1,));if ( empty( $posts ) ) {// create action now$post_id = $this->save_settings( $action );} else {$post_id = $posts[0]->ID;}This keeps the `@return int|WP_Error` contract true.
- Or, if callers genuinely expect the full post array, widen the docblock to include
arrayand update callers accordingly.Given this PR’s focus on type comments, I’d lean towards normalizing to an ID if that matches historical expectations.
Also applies to: 864-911
🧹 Nitpick comments (3)
classes/models/FrmFormAction.php (2)
432-513: Optional: tighten update_callback() return type to describe element types
update_callback()is annotated as@return array|null, but$action_idsis built from:
$new_instance['ID'](ints), andsave_settings( $instance )(int|WP_Error).If you want more precise static analysis, consider clarifying the element type, e.g.:
* @return array<int|WP_Error>|null(or similar generic notation your tools prefer). Current behavior is correct; this is purely a type-precision improvement.
548-562: Optional: align get_single_action() return type with WP_Post
get_single_action()ultimately returns the result ofget_post()passed throughprepare_action(), so it’s effectivelyWP_Post|nullin normal usage. Right now it’s documented as@return object|null.If you want stronger typing and consistency with
prepare_action()’s@param WP_Post/@return WP_Post, you could update the docblock to:- * @return object|null + * @return WP_Post|nullBehavior doesn’t change either way; this just improves clarity.
Also applies to: 764-791
classes/controllers/FrmFormsController.php (1)
3320-3332: Script defer helper and related docs – consider a more explicitstrposcheck
- The new PHPDocs for
defer_script_loading( $tag, $handle ),footer_js( $location ),maybe_minimize_form( $atts, &$content ), andis_minification_on( $atts )all match current usage.- In
defer_script_loading(), the stricter handle check ('captcha-api' === $handle) is good. The condition! strpos( $tag, 'defer' )works for typical<script>tags, but it relies onstrpos()never returning0. For future‑proofing, it would be more idiomatic (and robust) to use:if ( 'captcha-api' === $handle && false === strpos( $tag, 'defer' ) ) { $tag = str_replace( ' src', ' defer="defer" async="async" src', $tag ); }This avoids the edge case where
'defer'might appear at position 0 and be treated as “not found”.Also applies to: 3334-3350, 3352-3365, 3367-3375
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
classes/controllers/FrmFormsController.php(25 hunks)classes/models/FrmFormAction.php(16 hunks)tests/cypress/e2e/Forms/formPageDataValidation.cy.js(1 hunks)tests/cypress/e2e/admin-a11y.cy.js(1 hunks)
⏰ 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 trunk
- GitHub Check: PHP 8 tests in WP trunk
🔇 Additional comments (16)
tests/cypress/e2e/Forms/formPageDataValidation.cy.js (1)
44-44: LGTM!The additional expected heading text appropriately extends test coverage for promotional banner variations.
tests/cypress/e2e/admin-a11y.cy.js (1)
33-34: Based on my verification, I've gathered important information about this change:Key Findings:
axe-core 'region' rule verified: The rule checks that all page content (except skip links) is contained within landmark elements (HTML5 landmarks like
<header>,<nav>,<main>,<footer>or ARIA landmark roles). This is a best-practice check for screen reader navigation.Scope of impact: The 'region' rule is now disabled across 9+ admin page tests that use
baselineRules(including dashboard, form list, entries, import/export, global settings, add-ons, SMTP, and trash pages).No tracking found: I found no TODOs, FIXMEs, GitHub issues, or documentation comments in the codebase indicating that this accessibility gap is being tracked as technical debt. The rule is simply silently disabled without an apparent remediation plan.
Consistency: The 'region' rule also appears disabled in
form-preview-a11y.cy.jswith the same pattern.
Confirm the landmark region violations are documented or tracked as technical debt.
Disabling the
regionrule allows content to live outside landmark regions without detection, affecting screen reader navigation across multiple admin pages. No tracking mechanism is visible in the codebase for remediating these issues. Consider creating a GitHub issue to formally document this accessibility debt before merging.classes/models/FrmFormAction.php (3)
182-190: Constructor and helper PHPDoc additions look consistentThe added/expanded annotations for the legacy constructor,
get_group(), andget_field_name()correctly reflect the signatures and current usage; no inconsistencies spotted.Also applies to: 207-213, 235-245
275-321: prepare_new() and create() PHPDoc now accurately match behavior
prepare_new()’s@param false|int|string $form_idand@return objectalign with how$form_idis used and the(object) $form_actionreturn.create()’s@param int|string $form_idand@return int|WP_Errorare consistent with delegating tosave_settings().Nothing else to adjust here.
323-343: duplicate_form_actions() / form_has_action_type() docs and minor rename are sound
duplicate_form_actions()’s param types for$form_idand$old_idmatch how they’re used asmenu_ordervalues.form_has_action_type()’s new PHPDoc and the rename to$actionsalign with the actual call toget_action_for_form()and its boolean result check.All good in this area.
Also applies to: 675-684
classes/controllers/FrmFormsController.php (11)
75-89: Divi conflict filter PHPDoc matches implementationThe new
@param array $shortcodesand@return arrayannotations onprevent_divi_conflict()correctly describe how the function is used (pushes'formidable'into the array and returns it). No behavior change here.
200-211: Type docs foredit()andupdate()align with usageDocumenting
edit( $values = false )asarray|falseandupdate( $values = array() )asarraymatches how these methods are called (either with an array from routing/JSON or falling back to$_POST). The annotations look accurate and should help static analysis without changing behavior.Also applies to: 281-327
632-646:preview_page_title()PHPDoc is accurateThe added
@param string $title/@return stringdocs match the filter signature and implementation ($titleis always returned as a string). No issues here.
1428-1437:form_publish_button()param/return annotations look correctThe new
@param array $attsand@return voidtags match the current usage ($atts['values']is read and no value is returned). This is consistent and non‑breaking.
1920-1973: Bulk actions and view/report/admin‑bar helpers: docs match behavior
process_bulk_form_actions( array $errors ) : arraymatches how the method is used and what it returns.no_views( $values = array() )andno_reports( $values = array() )now document the$valuesargument even though they currently ignore it, which is fine for filters/actions.add_forms_to_admin_bar( $actions )is correctly described as taking an array and returning void.These PHPDoc additions look accurate and don’t affect runtime.
Also applies to: 2136-2146, 2153-2161, 2210-2249
2440-2451: Saved‑errors and entry‑lookup docs are a good fit
get_saved_errors( $form, $params ) : arraynow documents$formasobjectand$paramsasarray, which matches the usage ($form->idandFrmForm::get_params()).just_created_entry( $form_id ) : intmatches how the function is used (array key into$frm_vars['created_entries'], returning an entry ID or0).These clarifications should help static analysis without changing behavior.
Also applies to: 2467-2476
2720-2751: On‑submit actions and page‑load helper docs reflect actual usage
run_on_submit_actions( array $args )is now clearly documented and matches the internal contract used inget_form_contents().load_page_after_submit( array $args )’s PHPDoc matches how$argsis constructed and consumed (expects keys likesuccess_opt,form,entry_id).No behavioral changes here; the type comments look correct.
Also applies to: 2823-2854
3060-3087: Success‑message / post‑submit helpers: array param docs look rightThe added/expanded PHPDocs for:
show_message_after_save( array $atts )show_form_after_submit( array $args )fill_atts_for_form_display( array &$args )show_lone_success_message( array $atts )all accurately describe how these methods are being passed associative arrays and return no value. The by‑reference usage of
$argsinfill_atts_for_form_display()is consistent with the docs and call sites.Also applies to: 3090-3127, 3161-3189, 3191-3212
3493-3502: Deprecatedcreate()PHPDoc is now explicit and consistentAdding
@param array $valuesand@return voidtocreate()matches what the method does (proxy toupdate()after_deprecated_function()). This is a nice clarity/typing improvement without affecting behavior.
1252-1355: Hook registration is correctly configured – no changes neededThe
frm_fields_in_form_builderfilter is registered inFrmHooksController.php:154without an explicit$accepted_argsparameter:add_filter( 'frm_fields_in_form_builder', 'FrmFormsController::update_form_builder_fields' );WordPress defaults
$accepted_argsto 1 when omitted. Sinceupdate_form_builder_fields()accepts only one parameter ($fields), WordPress will correctly pass only the first argument to the callback, making this safe and compatible with the two-argumentapply_filters()call at line 1318.The PHPDoc suggestion remains optional but reasonable for tooling clarity.
1664-1700: I'll help you verify the review comment about the$settings_tabtype consistency. Let me start by examining theFrmAppHelper::is_admin_page()method to confirm its return type.
<function_calls>
#!/bin/bashSearch for FrmAppHelper class and is_admin_page method definition
rg -n "function is_admin_page" -C5 --type php
</function_calls>I'll search for the
is_admin_pagemethod definition to check its return type:
<function_calls>
#!/bin/bashFind FrmAppHelper file and is_admin_page method
find . -type f -name "*.php" | xargs grep -l "class FrmAppHelper"
</function_calls>Let me take a broader approach to locate this class:
<function_calls>
#!/bin/bashSearch for is_admin_page method definition
rg -n "function is_admin_page" -A10 --type php
</function_calls>Now let me find where
get_shortcode_helpers()callsis_admin_page():
<function_calls>
#!/bin/bashSearch for get_shortcode_helpers method
rg -n "function get_shortcode_helpers" -A20 --type php
</function_calls>Let me also search for the specific file mentioned in the review and examine it:
<function_calls>
#!/bin/bashRead the FrmFormsController.php file around the mentioned lines
sed -n '1754,1793p' classes/controllers/FrmFormsController.php
</function_calls>Let me check if the file exists and examine more context:
<function_calls>
#!/bin/bashFind FrmFormsController.php
find . -name "FrmFormsController.php" -type f
</function_calls><function_calls>
#!/bin/bashList the repository structure to understand organization
ls -la
</function_calls><function_calls>
#!/bin/bashGet list of PHP files related to controllers
find . -path "/controllers/.php" -type f | head -20
</function_calls>Let me search more broadly for these methods:
<function_calls>
#!/bin/bashSearch for get_shortcode_helpers in all PHP files
rg "get_shortcode_helpers" -n --type php -C3
</function_calls><function_calls>
#!/bin/bashSearch for is_admin_page calls to understand how it's used
rg "is_admin_page" -n --type php -C3
</function_calls>Let me check the git repository information:
<function_calls>
#!/bin/bashCheck git status and repository info
git log --oneline -1
git branch -a
</function_calls><function_calls>
#!/bin/bashList all PHP files in the repo
git ls-files "*.php" | head -30
</function_calls><function_calls>
#!/bin/bashSearch more specifically for FrmAppHelper
git ls-files "*.php" | xargs grep -l "class FrmAppHelper" 2>/dev/null
</function_calls><function_calls>
#!/bin/bashGet comprehensive list of all files to understand repo structure
git ls-files | grep -E "(Helper|Controller)" | head -20
</function_calls>Let me try a different approach - search for any mention of these function/class names:
<function_calls>
#!/bin/bashSearch in all tracked files
git ls-files | xargs grep -l "is_admin_page" 2>/dev/null | head -10
</function_calls><function_calls>
#!/bin/bashTry to understand the repository structure
git ls-files | head -50
</function_calls>Let me try Python to help search through the repository more effectively:
<function_calls>
#!/bin/bash
python3 << 'EOF'
import subprocess
import osGet all PHP files
result = subprocess.run(['git', 'ls-files', '*.php'], capture_output=True, text=True)
files = result.stdout.strip().split('\n')print(f"Total PHP files: {len(files)}")
print("\nFirst 20 PHP files:")
for f in files[:20]:
print(f)
EOF
</function_calls><function_calls>
#!/bin/bashCheck if there are any files at all
ls -la | head -20
pwd
</function_calls><function_calls>
#!/bin/bashList git tracked files count
git ls-files | wc -l
git ls-files | head -50
</function_calls>Based on my attempts to locate the codebase files, it appears the repository may not have been cloned into the sandbox environment, or the files are not accessible. Let me verify the current state and provide guidance:
<function_calls>
#!/bin/bashCheck current directory and git status
git status
</function_calls>Given that I'm unable to access the repository files to perform the verification, I need to gather information differently. Let me search for documentation or type information about the
is_admin_page()method:
<function_calls>
FrmAppHelper is_admin_page return type PHP
</function_calls>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
classes/helpers/FrmAppHelper.php (1)
3016-3044: Regex inget_formatted_timeis invalid and should be fixedWhile this hunk only updates PHPDoc, the nearby regex:
if ( preg_match( '/^\d{1-2}\/\d{1-2}\/\d{4}$/', $date ) && self::pro_is_installed() ) {uses
{1-2}which is not a valid quantifier in PCRE. This will trigger a compilation warning and causepreg_matchto always fail.It looks like the intention was
{1,2}(“1 or 2 digits”). Suggest:- if ( preg_match( '/^\d{1-2}\/\d{1-2}\/\d{4}$/', $date ) && self::pro_is_installed() ) { + if ( preg_match( '/^\d{1,2}\/\d{1,2}\/\d{4}$/', $date ) && self::pro_is_installed() ) {Since this can emit PHP warnings at runtime, it’s worth fixing even though it predates the doc change.
🧹 Nitpick comments (3)
classes/helpers/FrmAppHelper.php (3)
2006-2020: Consider using core helpers and escaping forpost_edit_linkNew
post_edit_link( $post_id )is generally fine, but a couple of small improvements would make it more robust:
- Core already provides
get_edit_post_link( $post_id ), which also respects permissions; using it would align behavior with WP expectations.- Even though
truncate()strips tags, wrapping the title inesc_html()would make the intent explicit and guard against any future change totruncate().For example:
- $post = get_post( $post_id ); - if ( $post ) { - $post_url = admin_url( 'post.php?post=' . $post_id . '&action=edit' ); - - return '<a href="' . esc_url( $post_url ) . '">' . self::truncate( $post->post_title, 50 ) . '</a>'; - } + $post = get_post( $post_id ); + if ( $post ) { + $post_url = get_edit_post_link( $post ); + + if ( ! $post_url ) { + return ''; + } + + return '<a href="' . esc_url( $post_url ) . '">' . esc_html( self::truncate( $post->post_title, 50 ) ) . '</a>'; + }
2476-2482: Docstring forreplace_quotesis misleadingThe summary says “Replace quotes with their HTML entities”, but the implementation does the opposite: it replaces several HTML entities for curly/prime quotes with straight ASCII
"'"characters.Consider rewording to avoid confusion, e.g.:
/** * Normalize HTML-encoded quote entities to straight ASCII quotes. * * @param string $val * @return string */
3141-3177:time_formatandconvert_timedocs: return type mismatch
time_format()’s docblock matches behavior (intreturn afterfloor()).convert_time()is annotated as returningint, but it may return a float (months and years use fractional day constants, and the division itself is not cast).For accuracy, consider updating:
/** * @return float */ private static function convert_time( $from, $to ) {This avoids misleading static analysis and future readers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
classes/helpers/FrmAppHelper.php(32 hunks)classes/models/FrmFormAction.php(16 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- classes/models/FrmFormAction.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). (1)
- GitHub Check: Cypress
🔇 Additional comments (21)
classes/helpers/FrmAppHelper.php (21)
943-1116: Sanitization helpers PHPDoc updates look consistent with behaviorThe added/expanded docs around
decode_specialchars,decode_amp,kses, andallowed_htmlcorrectly describe by‑reference parameters and value types; signatures and behavior remain unchanged and coherent with existing usage.
1287-1315: Query var helper type hints are reasonableDocumenting
$valueinget_query_var()asarray|stringand$paramasstringmatches the loose behavior (early-return if already populated, otherwise WP_Query lookup). No functional concerns here.
1623-1683:show_search_boxPHPDoc matches implementationThe new
@param array $atts/@return voidannotations align with how$attsis merged with defaults and only echoed. No functional changes; behavior still looks good.
1686-1696:trigger_hook_loaddocblock clarificationThe new parameter docs for
$typeand optional$objectmatch existing usage and the filter/action signatures. No behavior change; implementation remains straightforward.
1965-2004: Page dropdown helper docs match usageThe updated docs for
prep_page_dropdown_params()andpreformat_selection_args()(deprecated params + mixed$args) accurately describe how old call styles are normalized into the current array format. No changes needed.
2287-2310: Permission helper docblocks align with current behaviorThe added parameter docs for
permission_check()($show_message) andpermission_nonce_error()($nonce_name,$nonce) correctly describe the existing control flow (optional nonce validation + capability check). No implementation issues spotted.
2332-2357: Selection helpers: docs and behavior look goodNew docblocks for
checked()andcheck_selected()correctly capture the accepted types (array|string) and return types. The trimming +htmlspecialchars_decodeviarecursive_function_map()makes sense for robust comparisons against$current.
2404-2440:array_flatten/safe_implodedocs match behaviorThe clarified docs (array +
$keysmode, returning array/string) correctly reflect whatarray_flatten()andsafe_implode()do. Logic remains unchanged and still guards against “Array to string conversion” notices.
2465-2474:use_wpautoptype hints are accurateAnnotating
$contentasmixedand the return asmixedis appropriate given the earlyis_string()guard. No behavior change; OK as is.
2494-2515:script_versiondocblock matches implementationThe new
@paramand@return int|stringannotations align with how$defaultis passed through and how$query->veris extracted. Logic still correctly defends against missing$wp_scriptsor unregistered handles.
2999-3013:mb_functionhelper implementation and docs look solidThe new
mb_function( array $function_names, array $args )correctly prefers the multibyte function when available and falls back to the non‑mb equivalent. Call sites intruncate()pass compatible pairs (mb_strlen/strlen,mb_substr/substr,mb_split/explode), so signatures line up.No changes needed.
3066-3077:get_localized_datedocs align with behaviorThe added params/return annotations (
string $date_format,string $date,stringreturn) match the implementation that converts from GMT and delegates todate_i18n(). No further comments.
3083-3136:human_time_diffparameter docs and behavior are consistentDocumenting
$from/$toas seconds and$levelsasint|stringmatches usage (Unix timestamps wrapped viaDateTime('@...'), with$levelseither depth or a specific unit). The logic for unit selection and filtering viafrm_time_ago_levelsstill looks correct.
3193-3211:get_unitdocblock is accurateThe
@return int|stringannotation fits the implementation (either returns the original unit string, numeric, or1as a fallback). No behavior concerns.
3286-3339:json_to_arraydocblock and behaviorThe addition of
@param array $json_vars/@return arraycorrectly captures how this converts serialized form-style name/value pairs into a nested array. The implementation’s use ofadd_value_to_array()and nested switches is unchanged; no obvious correctness issues.
3341-3355:add_value_to_arraydocs match its current semanticsThe new annotations for
$valand$varsreflect existing behavior: this helper only sets the first occurrence for a given key (unless$nameis empty, in which case it appends). That behavior is consistent with howjson_to_array()uses it.
3390-3408:select_current_pagedocblock updates look goodDocumenting
$page/$current_pageasint|stringand$actionasarraymatches how they’re compared and how the optional action filtering works. No change in logic; still a simple echo‑side effect helper.
3570-3595:format_form_dataannotations are accurateThe updated doc (
@param array $formby reference,@return void) aligns with this method mutating$formin place based on name/value pairs. Implementation remains unchanged and still useshttp_build_query/parse_strcorrectly.
4139-4151:get_images_dropdown_attsdocblock is clear and correctNew docs for
$optionand$argsmatch usage: it composes the image HTML, extra classes, and custom attributes into a compact array consumed by the images dropdown view. No issues.
4379-4393:get_landing_page_upgrade_data_paramstype docs align with usageAdding
@param string $mediumandarrayreturn type matches how$mediumis dropped into the params array and howget_upgrade_data_params()is called. Everything stays backward‑compatible.
3853-3875:min_pro_version_noticeparameter docThe new
@param string $min_versionannotation correctly reflects current calls and the version comparison againstFrmProDb::$plug_version. Implementation is otherwise unchanged and still guards against non‑Pro installs.
1 participant
This PR adds missing
@paramcomments for every function.And the PHPCS rule is no longer ignored 🎉.