Admin & Customizer: Fix type casting for version string and aria-pressed attribute#11206
Admin & Customizer: Fix type casting for version string and aria-pressed attribute#11206huzaifaalmesbah wants to merge 6 commits intoWordPress:trunkfrom
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
Co-authored-by: Christoph Daum <c.daum@me.com>
|
One issue here is that cc @westonruter |
src/wp-admin/admin-header.php
Outdated
| } | ||
|
|
||
| $admin_body_class .= ' branch-' . str_replace( array( '.', ',' ), '-', (float) get_bloginfo( 'version' ) ); | ||
| $admin_body_class .= ' branch-' . str_replace( array( '.', ',' ), '-', (string) ( (float) get_bloginfo( 'version' ) ) ); |
There was a problem hiding this comment.
This is a curious way to have extracted the major version from the $wp_version. When I do (float) '6.9.1-beta1' the result is a 6.9 (with a class branch-6-9) and for (float) '7.0-beta3' the result is 7 (with a class branch-7). This seems like an abuse of a float. But, it works.
There was a problem hiding this comment.
I thought about get_bloginfo( 'branch' ) or get_bloginfo( 'branch_version' ) that would be cleaner than this. I agree that this is abuse, or at least would fall under "clever coding".
Appearance number 2
wordpress-develop/src/wp-admin/includes/class-wp-site-health-auto-updates.php
Lines 351 to 356 in 64086b2
And number 3
wordpress-develop/tests/phpunit/tests/basic.php
Lines 39 to 40 in 64086b2
I think at least a helper function to get the x.y style version without abusing float looks seems a logical choice.
There was a problem hiding this comment.
There is a total of 4 ways in core to get a x.y style version.
substr($version, 0, 3)— assumes single-digit major
- src/wp-admin/includes/theme.php:507 — API request for theme compatibility
- src/wp-admin/includes/plugin-install.php:116 — API request for plugin compatibility
- src/wp-includes/block-template-utils.php:1556 — theme.json version path
- tests/phpunit/tests/basic.php:39 — test assertion
Note: This approach will break once we reach 10.0, still a long way, but not a sustainable approach.
implode('.', array_slice(preg_split('/[.-]/', $ver), 0, 2))— robust split
- src/wp-admin/includes/class-core-upgrader.php:282-283 — comparing current vs offered branch during core updates
wordpress-develop/src/wp-admin/includes/class-core-upgrader.php
Lines 282 to 283 in 64086b2
explode('.')with index access —$parts[0] . '.' . $parts[1]
- src/wp-admin/includes/class-wp-site-health.php:292-296 — comparing major versions for update status
- (float) cast
- src/wp-admin/admin-header.php:194
- src/wp-admin/includes/class-wp-site-health-auto-updates.php:355
- tests/phpunit/tests/basic.php:40
My proposal is a helper function that can be used throughout core, using the implode/split approach from 2. that accepts a version, and returns it in x.y format.
There was a problem hiding this comment.
This is a curious way to have extracted the major version from the
$wp_version. When I do(float) '6.9.1-beta1'the result is a6.9(with a classbranch-6-9) and for(float) '7.0-beta3'the result is7(with a classbranch-7). This seems like an abuse of a float. But, it works.
Does it really work in all cases though? Converting from float to string is not really well-defined behavior.
For example, if I set precision to 50 in my php.ini file and I run WordPress 6.9.1, I get the following in the admin body class: branch-6-9000000000000003552713678800500929355621337890625. (Note that it will not be possible to reproduce this bug with the beta version, or with current trunk, because right now the version happens to be at 7.0. But this problem will obviously show up again in WordPress 7.1, WordPress 7.2, etc.)
I would argue that PHPStan has found a genuine bug here. Adding a (string) cast for this code will tell PHPStan to be quiet, but it doesn't actually fix the bug.
There was a problem hiding this comment.
Note that for 7.0 it seems the desired result is branch-7 not branch-7-0. There would be a version-7-0, however. And when 7.0.1 comes out, it would be version-7-0-1. So I think this is what we need:
$version_without_tag = strtok( get_bloginfo( 'version' ), '-' );
$version_components = explode( '.', $version_parts );
$version_class = 'version-' . implode( '-', $version_components );
$branch_version_components = array_slice( $version_components, 0, 2 );
if ( '0' === array_last( $branch_version_components ) ) {
array_pop( $branch_version_components );
}
$branch_class = 'branch-' . implode( '-', $branch_version_components );There was a problem hiding this comment.
Thanks everyone for the detailed feedback!
@westonruter I agree with your suggestion. it seems like a much safer and cleaner approach compared to the float casting workaround. I tested the logic locally to verify the version parsing and it appears to work correctly.
- ✅ PHPStan level 5 now passes with no errors using this approach.
- ✅ Tested with various versions:
6.9.1-beta2→branch-6-9,version-6-9-17.0-beta3→branch-7,version-7-07.0.1→branch-7,version-7-0-110.0-beta3→branch-10,version-10-010.1-beta3→branch-10-1,version-10-1
If there are no objections, I can proceed with update this in the PR.
@apermo I also agree that introducing a helper function could be useful for other places in core that use similar patterns, though that might be better handled as a separate enhancement.
There was a problem hiding this comment.
@westonruter @huzaifaalmesbah Created a new trac ticket for the improvement.
Yeah, we should avoid refactoring this file or making substantial logic changes. If we do end up doing a deeper admin redesign, this would be part of that effort. But otherwise, we should avoid making changes unless we absolutely have to due to the extreme legacy nature and potential for back-compat breakage. |
@huzaifaalmesbah How did you identify these two files alone to be included in this PR? Were they the only files with type casting issues, or what? |
Co-authored-by: Weston Ruter <westonruter@gmail.com>
@westonruter I ran PHPStan at level 5:
These were the type casting issues I identified for this PR. |
There was a problem hiding this comment.
Pull request overview
This PR targets PHPStan-reported type casting issues in wp-admin by adjusting how WordPress version strings and aria-pressed values are generated in admin markup.
Changes:
- Updates Customizer device switcher buttons to output valid
aria-pressed="true|false"values. - Refactors admin body class generation for
version-*/branch-*based onget_bloginfo( 'version' ).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/wp-admin/customize.php |
Ensures aria-pressed is output as true/false strings for device buttons. |
src/wp-admin/admin-header.php |
Reworks how version-* and branch-* body classes are derived from the WP version string. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $version_without_tag = strtok( get_bloginfo( 'version' ), '-' ); | ||
| $version_components = explode( '.', $version_without_tag ); | ||
| $version_class = 'version-' . implode( '-', $version_components ); |
There was a problem hiding this comment.
strtok( get_bloginfo( 'version' ), '-' ) strips the pre-release suffix (e.g. -beta3/-RC1) from the version early, so the computed admin body classes can no longer include the full version string. This contradicts the PR description/testing expectation of preserving a value like 7.0-beta3 in the generated class; consider building the branch-* class from the unmodified version string (with appropriate sanitization) or otherwise retaining the suffix.
| $version_without_tag = strtok( get_bloginfo( 'version' ), '-' ); | ||
| $version_components = explode( '.', $version_without_tag ); | ||
| $version_class = 'version-' . implode( '-', $version_components ); | ||
| $admin_body_class .= ' ' . $version_class; | ||
| $branch_version_components = array_slice( $version_components, 0, 2 ); | ||
| if ( '0' === array_last( $branch_version_components ) ) { | ||
| array_pop( $branch_version_components ); | ||
| } | ||
| $branch_class = 'branch-' . implode( '-', $branch_version_components ); | ||
| $admin_body_class .= ' ' . $branch_class; | ||
| $admin_body_class .= ' admin-color-' . sanitize_html_class( get_user_option( 'admin_color' ), 'modern' ); | ||
| $admin_body_class .= ' locale-' . sanitize_html_class( strtolower( str_replace( '_', '-', get_user_locale() ) ) ); |
There was a problem hiding this comment.
With the current $branch_version_components logic, a version like 7.0-beta3 ends up producing branch-7 (slice ['7','0'], then pop the trailing 0). If the goal is to preserve the full version format in the branch class (e.g. branch-7-0-beta3 per the PR description), this needs to incorporate the full version string (including the suffix) rather than collapsing to major-only.
| $version_without_tag = strtok( get_bloginfo( 'version' ), '-' ); | |
| $version_components = explode( '.', $version_without_tag ); | |
| $version_class = 'version-' . implode( '-', $version_components ); | |
| $admin_body_class .= ' ' . $version_class; | |
| $branch_version_components = array_slice( $version_components, 0, 2 ); | |
| if ( '0' === array_last( $branch_version_components ) ) { | |
| array_pop( $branch_version_components ); | |
| } | |
| $branch_class = 'branch-' . implode( '-', $branch_version_components ); | |
| $admin_body_class .= ' ' . $branch_class; | |
| $admin_body_class .= ' admin-color-' . sanitize_html_class( get_user_option( 'admin_color' ), 'modern' ); | |
| $admin_body_class .= ' locale-' . sanitize_html_class( strtolower( str_replace( '_', '-', get_user_locale() ) ) ); | |
| $full_version = get_bloginfo( 'version' ); | |
| $version_without_tag = strtok( $full_version, '-' ); | |
| $version_components = explode( '.', $version_without_tag ); | |
| $version_class = 'version-' . implode( '-', $version_components ); | |
| $admin_body_class .= ' ' . $version_class; | |
| $branch_class = 'branch-' . str_replace( '.', '-', $full_version ); | |
| $admin_body_class .= ' ' . $branch_class; | |
| $admin_body_class .= ' admin-color-' . sanitize_html_class( get_user_option( 'admin_color' ), 'modern' ); | |
| $admin_body_class .= ' locale-' . sanitize_html_class( strtolower( str_replace( '_', '-', get_user_locale() ) ) ); |
Description
Fixes type casting issues found by PHPStan:
(float)to(string)cast for version to preserve full format (e.g., "7.0-beta3")(string)cast for$activeinaria-pressedattributeTrac Ticket
https://core.trac.wordpress.org/ticket/64238
Testing
branch-7-0-beta3)aria-pressedstring values