Feature/package for packagist#62
Conversation
There was a problem hiding this comment.
Code Review
This pull request focuses on improving code quality, type safety, and static analysis across the codebase. It introduces strict parameter and return type declarations, adds comprehensive PHPStan annotations, and implements defensive checks (such as is_array, is_scalar, and is_wp_error) to prevent runtime errors. Additionally, quality check commands were documented in the README, and several features were refactored for better readability. Feedback on these changes highlights a potential PHP warning in SaltusAdminCols.php if get_post_status_object() returns null, as well as a minor indentation inconsistency in Core.php.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive type-hinting, return type declarations, and defensive checks across the codebase to achieve a clean PHPStan level 7 run. Key changes include tightening property and method signatures in core services, models, and features, as well as handling potential WordPress errors and type mismatches. The review feedback identifies two important issues: a potential PHP 8.x TypeError in UpdateMenuDragAndDrop.php when parsing the order parameter, and a potential undefined index notice in WalkerTaxonomyDropdown.php if the taxonomy argument is missing.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/gemini review :( |
There was a problem hiding this comment.
Code Review
This pull request focuses on tightening type declarations, adding return types, and improving static analysis compliance across the codebase to achieve a clean PHPStan level 7 run. Key feedback points out several critical issues: an undefined variable $hook_name in Core.php that will cause a crash on initialization, a missing authorization check in the AJAX callback within UpdateMenuDragAndDrop.php, and inconsistent usage of raw model names instead of the newly introduced sanitized registration name in PostType.php, Taxonomy.php, and ModelFactory.php. Additionally, get_registration_name() should be made public, and GenericContainer.php needs to be updated to properly pass and handle registered dependencies.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request focuses on tightening types, adding PHPStan type annotations, and improving static analysis compliance across the codebase to reach PHPStan level 7. Key feedback includes addressing a bug in GenericContainer.php where constructor dependencies are ignored during instantiation, fixing a security vulnerability in UpdateMenuDragAndDrop.php due to a missing authorization check on the AJAX callback, and resolving an issue in Taxonomy.php where string-based associations are not properly registered.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request focuses on tightening types, adding property and return type declarations, and updating PHPDoc annotations across the codebase to achieve a clean PHPStan level 7 run. It also updates development dependencies and documents quality checks in the README. The review feedback highlights opportunities to improve usability and code quality, specifically by falling back to plain text instead of rendering empty links when term links are invalid, and by removing unnecessary HTML-escaping and localization from developer-facing exception messages.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
When get_edit_term_link returns null or a WP_Error, the previous code rendered an empty href attribute (<a href="">), which is confusing for screen readers and provides no usable navigation. Now falls back to rendering just the escaped term name without a link, matching the behavior used when the user lacks edit_terms capability.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request focuses on tightening types, adding PHPStan type annotations, and improving overall code quality across the repository to achieve a clean PHPStan level 7 run. Key changes include adding strict type declarations, return types, and parameter types across core files, features, and infrastructure classes, as well as upgrading dev dependencies. The review feedback identifies a critical security vulnerability where the AJAX handler in UpdateMenuDragAndDrop.php lacks a user capability check, allowing unauthorized users to modify the menu order. Additionally, it is recommended to cast $post->menu_order to an integer in SaltusDragAndDrop.php to prevent potential SQL injection or malformed queries.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Cast $post->menu_order to (int) in previous_post_where() and next_post_where() to prevent SQL injection or malformed queries from unexpected property data.
|
/gemini review 🤞 |
There was a problem hiding this comment.
Code Review
This pull request refactors the codebase to achieve a clean PHPStan level 7 run by tightening types, adding return types, and updating annotations across various services, models, and features. It also introduces a get_registration_name() method in BaseModel to sanitize model names for WordPress registration. The reviewer recommends validating WordPress length limits (20 characters for post types and 32 for taxonomies) within this new method to prevent silent registration failures.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…name() WordPress imposes strict length limits on registered keys: 20 characters for post types and 32 characters for taxonomies. The get_registration_name() method now validates these limits based on the model type to prevent silent registration failures or database errors.
|
/gemini review 🤞 |
There was a problem hiding this comment.
Code Review
This pull request focuses on tightening type safety, adding type hints, and improving static analysis compliance across the codebase, alongside updating development dependencies. Key changes include adding parameter and return types, introducing defensive type checks, and adding a sanitized model name resolver for WordPress APIs. The review feedback highlights a critical signature compatibility issue in WalkerTaxonomyDropdown::start_el that would cause a PHP Fatal Error, as well as opportunities to clean up an exception message in Core.php and correct an instance call of a static method in ModelFactory.php.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| */ | ||
| $hook_name = static::HOOK_PREFIX . static::SERVICES_FILTER; | ||
| if ( empty( $hook_name ) ) { | ||
| throw new \InvalidArgumentException( esc_html__( 'Hook name cannot be empty', 'saltus' ) ); |
There was a problem hiding this comment.
Using esc_html__ inside an InvalidArgumentException is an anti-pattern. Exception messages are developer-facing and logged in system logs or shown in CLI environments, where HTML escaping is unnecessary and can make the messages harder to read (e.g., converting quotes to HTML entities). Since this is a developer-facing error, a plain English string is preferred.
throw new \\InvalidArgumentException( 'Hook name cannot be empty' );| // make sure $args is an array | ||
| $args = is_array( $args ) ? $args : []; | ||
| $service = $this->app->get( $normalized_feature_name ); | ||
| $service_imp = $service->make( $cpt->get_registration_name(), $this->project, $args ); |
There was a problem hiding this comment.
The make method is defined as a static method in the Assembly interface. Calling a static method as an instance method (using ->) is a code smell and can be flagged by static analysis tools. It is cleaner and more standard to call it statically using the :: operator.
$service_imp = $service::make( $cpt->get_registration_name(), $this->project, $args );There was a problem hiding this comment.
ohh i missed this one!
The parent class Walker::start_el does not declare a return type. Adding : void in the child causes a PHP fatal error at runtime due to incompatible signature.
The Assembly interface declares make() as a static method. Calling it via -> works but is a code smell flagged by static analysis. Use :: for clarity.
Exception messages are developer-facing and logged in system logs or CLI environments. HTML escaping with esc_html__ or esc_html makes them harder to read and is unnecessary.
Add more checks and type safety