PHP 8.2 Compatibility, Strict Types & Docblock Cleanup#659
PHP 8.2 Compatibility, Strict Types & Docblock Cleanup#659bhavz-10 wants to merge 8 commits intotheme-elementary-v2from
Conversation
| @@ -29,7 +31,7 @@ protected function __construct() { | |||
| * | |||
| * @return void | |||
There was a problem hiding this comment.
Should we remove @return void from the code now? Only keep those for the places where we need to document what the function is returning
There was a problem hiding this comment.
I thought about that but then felt that if we do not add void then that would then be ignored for all the functions which have void as a return type
Update style.css to remove @ tags and make the Description more descriptive. Make other changes based on return types and description.
There was a problem hiding this comment.
Pull request overview
This PR updates the theme toward PHP 8.2 compatibility by introducing strict typing and modernizing docblocks/configuration to match the rtCamp\Theme\Elementary namespace and updated platform requirements.
Changes:
- Added
declare( strict_types = 1 );and native return/parameter types across updated PHP files (theme + test scaffolding). - Updated
@packagetags tortCamp\Theme\Elementaryand refreshed some docblocks/signatures. - Aligned project metadata/config with PHP 8.2 and WP 6.5 (theme headers + PHPCS config).
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/php/TestCase.php | Updates package tag and enables strict types for the base test case. |
| tests/php/inc/MainTest.php | Adds strict types and return types for PHPUnit tests; docblock adjustments. |
| tests/bootstrap.php | Enables strict types and adds a void return type to the bootstrap helper. |
| style.css | Adds Requires PHP: 8.2 / Requires at least: 6.5 and modernizes the theme header block. |
| phpcs.xml.dist | Updates minimum_supported_wp_version to 6.5 and PHPCompatibility testVersion to 8.2-. |
| inc/Modules/BlockExtensions/MediaTextInteractive.php | Adds strict types, updates package tag, and types hook callbacks/return values. |
| inc/Main.php | Adds strict types, updates package tag, and adds void return types to public methods. |
| inc/helpers/custom-functions.php | Adds strict types and updates package tag (file is still a placeholder). |
| inc/Framework/Traits/Singleton.php | Adds strict types, updates package tag, and types get_instance() as static. |
| inc/Framework/Traits/AutoloaderTrait.php | Adds strict types and adds return types to trait methods. |
| inc/Framework/Traits/AssetLoaderTrait.php | Adds strict types and introduces type declarations to asset registration/version helpers. |
| inc/Core/Assets.php | Adds strict types and types public methods and render-block filter callback. |
| inc/Autoloader.php | Adds strict types and types autoloader methods/returns. |
| functions.php | Updates @package tag to the new namespace root. |
Comments suppressed due to low confidence (2)
inc/Framework/Traits/AssetLoaderTrait.php:117
get_file_version()is declared to returnstring|bool|null, but it returnsfilemtime()(anint) when$veris empty. This will cause a runtimeTypeErrorwhen the file exists. Update the return type (and/or cast/normalize the returned value) so it matches the actual possible return values.
private function get_file_version( string $file, string|bool|null $ver = false ): string|bool|null {
if ( ! empty( $ver ) ) {
return $ver;
}
$file_path = sprintf( '%s/%s', ELEMENTARY_THEME_BUILD_DIR, $file );
return file_exists( $file_path ) ? filemtime( $file_path ) : false;
}
inc/Framework/Traits/AssetLoaderTrait.php:43
register_script()acceptsstring|bool $filebut passes$fileintoget_asset_meta(string $file, ...). Withstrict_types=1, callingregister_script()withfalsewill throw aTypeError. Also, the$verparameter is currently unused (not forwarded intoget_asset_meta()/ version resolution). Align the parameter types and ensure$veris applied or remove it if it’s not supported.
private function register_script( string $handle, string|bool $file, array $deps = [], string|bool|null $ver = false, bool $in_footer = true ): bool {
$file_path = sprintf( '%s/%s', ELEMENTARY_THEME_BUILD_DIR, $file );
if ( ! \file_exists( $file_path ) ) {
return false;
}
$src = sprintf( ELEMENTARY_THEME_BUILD_URI . '/%s', $file );
$asset_meta = $this->get_asset_meta( $file, $deps );
return wp_register_script( $handle, $src, $asset_meta['dependencies'], $asset_meta['version'], $in_footer );
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * Test if class Main exists. | ||
| * | ||
| * @return void | ||
| * | ||
| * @since 1.0.0 | ||
| */ |
There was a problem hiding this comment.
Docblocks contain blank lines with trailing whitespace (e.g. * ). This is likely to trigger PHPCS whitespace sniffs and should be changed to a plain * line (no extra spaces).
| /** | ||
| * Setup hooks. | ||
| * | ||
| * @return void | ||
| * |
There was a problem hiding this comment.
Docblocks include blank lines with trailing whitespace (e.g. * on otherwise empty lines). This is likely to fail PHPCS whitespace sniffs; remove the trailing spaces so blank docblock lines are just *.
| /** | ||
| * Enqueue block specific assets. | ||
| * | ||
| * @param string $markup Markup of the block. | ||
| * @param array $block Array with block information. | ||
| * @param array $block Array with block information. | ||
| * | ||
| * @return string Updated markup. |
There was a problem hiding this comment.
There is trailing whitespace on an otherwise blank docblock line (* ). Removing the extra space avoids PHPCS whitespace violations.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (3)
inc/Framework/Traits/AssetLoaderTrait.php:43
register_script()allowsstring|bool $filebut immediately passes$fileintoget_asset_meta(string $file, ...), which will throw aTypeErroriffalseis ever provided. Also, the$verparameter onregister_script()is currently ignored (not forwarded), so callers can’t override the asset version. Consider either restricting$filetostring(recommended, since it’s used as a path) or updating the downstream methods to handlefalse, and pass$verthrough toget_asset_meta()/get_file_version()as intended.
private function register_script( string $handle, string|bool $file, array $deps = [], string|bool|null $ver = false, bool $in_footer = true ): bool {
$file_path = sprintf( '%s/%s', ELEMENTARY_THEME_BUILD_DIR, $file );
if ( ! \file_exists( $file_path ) ) {
return false;
}
$src = sprintf( ELEMENTARY_THEME_BUILD_URI . '/%s', $file );
$asset_meta = $this->get_asset_meta( $file, $deps );
return wp_register_script( $handle, $src, $asset_meta['dependencies'], $asset_meta['version'], $in_footer );
inc/Framework/Traits/AssetLoaderTrait.php:71
- Same issues as
register_script():register_style()acceptsstring|bool $filebut passes it toget_asset_meta(string $file, ...), which willTypeErroriffalseis provided. Additionally, the$verparameter is not forwarded, so version overrides are ignored. Align the parameter types with actual usage (preferstring $file) and pass$verthrough toget_asset_meta().
private function register_style( string $handle, string|bool $file, array $deps = [], string|bool|null $ver = false, string $media = 'all' ): bool {
$file_path = sprintf( '%s/%s', ELEMENTARY_THEME_BUILD_DIR, $file );
if ( ! \file_exists( $file_path ) ) {
return false;
}
$src = sprintf( ELEMENTARY_THEME_BUILD_URI . '/%s', $file );
$asset_meta = $this->get_asset_meta( $file, $deps );
return wp_register_style( $handle, $src, $asset_meta['dependencies'], $asset_meta['version'], $media );
inc/Framework/Traits/AssetLoaderTrait.php:116
get_file_version()declares a return type ofstring|bool|null, but it returnsfilemtime()(anint) when the file exists. Under strict types this will raise aTypeError. Update the return type (and docblock) to includeint(or cast thefilemtime()result tostringif that’s what callers expect).
private function get_file_version( string $file, string|bool|null $ver = false ): string|bool|null {
if ( ! empty( $ver ) ) {
return $ver;
}
$file_path = sprintf( '%s/%s', ELEMENTARY_THEME_BUILD_DIR, $file );
return file_exists( $file_path ) ? filemtime( $file_path ) : false;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
inc/Core/Assets.php
Outdated
| * @param string $markup Markup of the block. | ||
| * @param array $block Array with block information. | ||
| * @param array $block Array with block information. | ||
| * |
There was a problem hiding this comment.
There’s trailing whitespace on the blank docblock line (* ) which is typically flagged by PHPCS (WordPress/VIP standards). Remove the extra space to avoid lint failures.
| * | |
| * |
| */ | ||
| public function enqueue_block_specific_assets( $markup, $block ) { | ||
| public function enqueue_block_specific_assets( string $markup, array $block ): string { | ||
| if ( is_array( $block ) && ! empty( $block['blockName'] ) && 'core/navigation' === $block['blockName'] ) { |
There was a problem hiding this comment.
enqueue_block_specific_assets() now type-hints $block as array, so is_array( $block ) is redundant and can be removed to simplify the condition.
| if ( is_array( $block ) && ! empty( $block['blockName'] ) && 'core/navigation' === $block['blockName'] ) { | |
| if ( ! empty( $block['blockName'] ) && 'core/navigation' === $block['blockName'] ) { |
There was a problem hiding this comment.
Going to resolve all such issues with phpstan PR
Description
PHP 8.2 Compatibility, Strict Types & Docblock Cleanup
Technical Details
In the Issue: #657
Checklist
Screenshots
To-do
Fixes/Covers issue
Fixes #657