Add and fix return and param types where missing#1732
Conversation
passing callable directly is only still working due to back compat but it should be an array of args
📝 WalkthroughWalkthroughThis PR standardizes code documentation and hygiene across the codebase by adding ~50 missing ChangesDocumentation and code hygiene improvements
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request focuses on improving type safety and documentation across the codebase by adding return types to docblocks, modernizing register_setting calls, and ensuring correct type casting. Key improvements are suggested for includes/options-page.php: first, the edac_sanitize_accessibility_policy_page function should support numeric page IDs by checking if the input is numeric before applying esc_url(); second, the sanitization functions for position and prompt should return their respective default values instead of an empty string when encountering invalid inputs to prevent bypassing default option fallbacks in the settings UI.
| if ( $page ) { | ||
| return esc_url( $page ); | ||
| } | ||
| return ''; |
There was a problem hiding this comment.
The edac_sanitize_accessibility_policy_page function sanitizes the input using esc_url(). However, the accessibility policy page option can also be stored as a numeric page ID (as seen in edac_accessibility_policy_page_cb(), which checks is_numeric( $policy_page )). Running esc_url() on a numeric string like '42' will return an empty string, thereby breaking the option when a page ID is selected.
To support both numeric page IDs and URLs, check if the input is numeric and sanitize accordingly using absint() and esc_url_raw().
if ( $page ) {
return is_numeric( $page ) ? absint( $page ) : esc_url_raw( $page );
}
return '';| if ( in_array( $position, [ 'before', 'after', 'none' ], true ) ) { | ||
| return $position; | ||
| } | ||
| return ''; |
There was a problem hiding this comment.
Returning an empty string '' when the position is invalid will cause an empty string to be saved in the database. This bypasses the default value fallback of get_option() and results in none of the radio buttons being selected in the settings UI.
Instead of returning '', it is safer to return the default value 'after' to ensure the UI remains consistent.
if ( in_array( $position, [ 'before', 'after', 'none' ], true ) ) {
return $position;
}
return 'after';| if ( in_array( $prompt, [ 'when required', 'always', 'none' ], true ) ) { | ||
| return $prompt; | ||
| } | ||
| return ''; |
There was a problem hiding this comment.
Returning an empty string '' when the prompt is invalid will cause an empty string to be saved in the database. This bypasses the default value fallback of get_option() and results in none of the radio buttons being selected in the settings UI.
Instead of returning '', it is safer to return the default value 'when required' to ensure the UI remains consistent.
if ( in_array( $prompt, [ 'when required', 'always', 'none' ], true ) ) {
return $prompt;
}
return 'when required';| * @param array $filter [post_types, rule_types, rule_slugs]. | ||
| * @param integer $record_limit Max number of records we'll query. | ||
| * @param string $flags Flag used to determine how ignored issues sould be handled. | ||
| * @param int $flags Flag used to determine how ignored issues sould be handled. |
| * Run the fix for adding the comment and search form labels. | ||
| * | ||
| * @return void |
| * Run the fix for adding the comment and search form labels. | ||
| * | ||
| * @return void |
| * Run the fix for adding the comment and search form labels. | ||
| * | ||
| * @return void |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
includes/helper-functions.php (1)
434-436:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep
edac_get_issue_density()return type consistently float.With
@return floatand the cast on Line 449, the early exit on Line 435 should return a float literal too to avoid mixed return types.Suggested fix
if ( $element_count < 1 || $content_length < 1 ) { - return 0; + return 0.0; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@includes/helper-functions.php` around lines 434 - 436, The early return in edac_get_issue_density() returns integer 0 causing mixed return types; change that early exit to return a float literal (e.g., 0.0) so the function always returns float (keep the existing float cast at the final return intact).admin/class-helpers.php (2)
65-89:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the
@returntype forHelpers::format_percentage().
The unit tests expect numeric inputs to always return a string containing the%, and non-numeric inputs to return the value unchanged (e.g.,'N/A'), so the current@return string|int|floatdocblock doesn’t match the actual behavior—document the return asstring(or explicitly note that non-numeric inputs are returned as-is).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@admin/class-helpers.php` around lines 65 - 89, Update the PHPDoc for Helpers::format_percentage() to reflect actual behavior: change the `@return` from "string|int|float" to "string" (and note in the docblock that non-numeric inputs are returned unchanged), so that numeric inputs always document returning a percent string (e.g., "12.34%") while non-numeric inputs are returned as-is.
40-60:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix
@returnPHPDoc forHelpers::format_number()to match implementation
format_number()returns the original$numberunchanged when it’s not numeric (return $number;), but the docblock says@return string|false. Update the@returnannotation to include the non-numeric passthrough type(s) (or change the function to only accept numeric inputs).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@admin/class-helpers.php` around lines 40 - 60, The PHPDoc for Helpers::format_number() is incorrect: the function can return the original non-numeric $number as-is or a formatted string (and NumberFormatter::format() can return false on failure). Update the `@return` annotation for format_number() to reflect the actual possible types (e.g. string|int|float|false|mixed) so it documents both formatted string/false and the passthrough non-numeric value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@admin/class-helpers.php`:
- Around line 65-89: Update the PHPDoc for Helpers::format_percentage() to
reflect actual behavior: change the `@return` from "string|int|float" to "string"
(and note in the docblock that non-numeric inputs are returned unchanged), so
that numeric inputs always document returning a percent string (e.g., "12.34%")
while non-numeric inputs are returned as-is.
- Around line 40-60: The PHPDoc for Helpers::format_number() is incorrect: the
function can return the original non-numeric $number as-is or a formatted string
(and NumberFormatter::format() can return false on failure). Update the `@return`
annotation for format_number() to reflect the actual possible types (e.g.
string|int|float|false|mixed) so it documents both formatted string/false and
the passthrough non-numeric value.
In `@includes/helper-functions.php`:
- Around line 434-436: The early return in edac_get_issue_density() returns
integer 0 causing mixed return types; change that early exit to return a float
literal (e.g., 0.0) so the function always returns float (keep the existing
float cast at the final return intact).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 08d33c14-1c6e-4a64-89f9-911b345c0379
📒 Files selected for processing (34)
admin/AdminPage/FixesPage.phpadmin/AdminPage/FixesSettingType/Checkbox.phpadmin/AdminPage/FixesSettingType/Text.phpadmin/AdminPage/PageInterface.phpadmin/class-activation-redirect.phpadmin/class-admin-footer-text.phpadmin/class-enqueue-admin.phpadmin/class-frontend-highlight.phpadmin/class-helpers.phpadmin/class-issues-query.phpadmin/class-orphaned-issues-cleanup.phpadmin/class-settings.phpadmin/class-update-database.phpadmin/class-widgets.phpadmin/opt-in/class-email-opt-in.phpincludes/classes/Fixes/Fix/AddFileSizeAndTypeToLinkedFilesFix.phpincludes/classes/Fixes/Fix/AddLabelToUnlabelledFormFieldsFix.phpincludes/classes/Fixes/Fix/AddNewWindowWarningFix.phpincludes/classes/Fixes/Fix/BlockPDFUploadsFix.phpincludes/classes/Fixes/Fix/CommentSearchLabelFix.phpincludes/classes/Fixes/Fix/MetaViewportScalableFix.phpincludes/classes/Fixes/Fix/PreventLinksOpeningNewWindowFix.phpincludes/classes/Fixes/Fix/SkipLinkFix.phpincludes/classes/Fixes/FixInterface.phpincludes/classes/Fixes/FixesManager.phpincludes/classes/MyDot/Connector.phpincludes/classes/class-accessibility-statement.phpincludes/classes/class-enqueue-frontend.phpincludes/classes/class-lazyload-filter.phpincludes/classes/class-rest-api.phpincludes/classes/class-simplified-summary.phpincludes/classes/class-summary-generator.phpincludes/helper-functions.phpincludes/options-page.php
This pull request primarily improves code documentation and type safety throughout the codebase. The most significant changes are the addition of explicit
@returnannotations in PHPDoc blocks, updates to parameter and return types for better accuracy, and minor improvements to script enqueuing. These changes enhance code readability, maintainability, and developer experience, especially when using IDEs or static analysis tools.Documentation and Type Hinting Improvements:
Added explicit
@returnannotations (such as@return void,@return array,@return bool,@return string) to PHPDoc blocks for many public methods and functions across multiple files, clarifying expected return types and improving code documentation. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15] [16] [17] [18] [19] [20] [21] [22] [23] [24] [25]Improved parameter and return type documentation in helper and query classes, such as changing parameter types from
inttoint|floatand specifying accurate return types (e.g.,string|false,string|int|float). [1] [2] [3] [4]Script and Style Enqueuing:
wp_enqueue_scriptandwp_enqueue_stylein several places to use an empty array ([]) instead offalsefor the dependencies argument, aligning with best practices and preventing potential warnings. [1] [2]Bug Fixes and Minor Adjustments:
edac_get_upcoming_meetups_htmlis a string instead of an integer.These changes are mostly non-functional but are important for long-term maintainability and code quality.
Fixes: #393
Summary by CodeRabbit
Bug Fixes
Refactor