Make WordPress Core

Opened 2 years ago

Closed 16 months ago

Last modified 16 months ago

#54645 closed defect (bug) (fixed)

Allow numeric theme name

Reported by: alvastar's profile alvastar Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.2 Priority: normal
Severity: normal Version: 5.7
Component: Themes Keywords: has-patch has-unit-tests has-testing-info
Focuses: administration, template, multisite Cc:

Description

What steps should be taken to consistently reproduce the problem?

  • Create a theme with a numeric name or rename an existing one.
  • Activate the theme for your site.

The theme will not be displayed as active.

Also, when using the Gutenberg editor, calls to the add_theme_support function will not work.

Possibly the problem is how PHP is converting the keys of the array:

Strings containing valid decimal ints, unless the number is preceded by a + sign, will be cast to the int type. E.g. the key "8" will actually be stored under 8. On the other hand "08" will not be cast, as it isn't a valid decimal integer.

Suggested solution to the problem:

File in wp-icnludes/rest-api/endpoints/class-wp-rest-themes-controller.php replace this

/**
 * Helper function to compare two themes.
 *
 * @since 5.7.0
 *
 * @param WP_Theme $theme_a First theme to compare.
 * @param WP_Theme $theme_b Second theme to compare.
 * @return bool
 */
protected function is_same_theme( $theme_a, $theme_b ) {
        return $theme_a->get_stylesheet() === $theme_b->get_stylesheet();
}

with this

/**
 * Helper function to compare two themes.
 *
 * @since 5.7.0
 *
 * @param WP_Theme $theme_a First theme to compare.
 * @param WP_Theme $theme_b Second theme to compare.
 * @return bool
 */
protected function is_same_theme( $theme_a, $theme_b ) {
        return (string)$theme_a->get_stylesheet() === (string)$theme_b->get_stylesheet();
}

Attachments (2)

Screenshot before fix.png (230.9 KB) - added by jakariaistauk 18 months ago.
Screenshot before fix the issue
Screenshot after fix.png (252.0 KB) - added by jakariaistauk 18 months ago.
Screenshot after fix the issue

Download all attachments as: .zip

Change History (23)

#1 @spacedmonkey
2 years ago

  • Keywords needs-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to Future Release
  • Owner set to spacedmonkey
  • Status changed from new to assigned
  • Version changed from 5.8.2 to 5.7

#2 @SergeyBiryukov
2 years ago

#54666 was marked as a duplicate.

This ticket was mentioned in PR #2758 on WordPress/wordpress-develop by enricobattocchi.


2 years ago
#3

  • Keywords has-patch added; needs-patch removed

Casts theme dir name to string when creating a WP_Theme object, allowing to have numerical dirnames for themes.

Trac ticket: https://core.trac.wordpress.org/ticket/54645

#4 follow-up: @lopo
2 years ago

  • Keywords needs-patch needs-testing added; needs-unit-tests has-patch removed

To clarify: having a numerical name (in style.css) doesn't seem to create problems, while having a numerical folder name does.

The patch fixes this by making sure that we cast the theme dirname to a string when creating the WP_Theme object when assigning to the stylesheet property in the WP_Theme contructor.

To test, rename e.g. wp-content/themes/twentytwentyone to wp-content/themes/2021.

Without the patch:

  • setting the theme as "Active" doesn't result in being displayed as such in Appearance > Themes
  • editing a post in the block editor shows e.g. no section to set featured image in the sidebar

With this patch:

  • setting the theme as "Active" results in being displayed as such in Appearance > Themes
  • editing a post in the block editor shows the supported features e.g. the featured image

I'm removing the needs-unit-tests keyword since there's currently no default theme with a numerical dirname to be retrieved by a call to wp_get_themes() Unit test added after change of approach.

Last edited 2 years ago by lopo (previous) (diff)

#5 @lopo
2 years ago

  • Keywords has-patch added; needs-patch removed

#6 in reply to: ↑ 4 ; follow-up: @SergeyBiryukov
2 years ago

  • Keywords needs-unit-tests added

Replying to lopo:

I'm removing the needs-unit-tests keyword since there's currently no default theme with a numerical dirname to be retrieved by a call to wp_get_themes().

Thanks for the patch! I think this change would still benefit from unit tests to make sure the fix works as expected and there are no regressions at a later point.

Last edited 2 years ago by SergeyBiryukov (previous) (diff)

#7 in reply to: ↑ 6 @lopo
2 years ago

Replying to SergeyBiryukov:

Replying to lopo:

I'm removing the needs-unit-tests keyword since there's currently no default theme with a numerical dirname to be retrieved by a call to wp_get_themes().

Thanks for the patch! I think this change would still benefit from unit tests to make sure the fix works as expected and there are no regressions at a later point.

Would love to, @SergeyBiryukov, but I'm not sure how it can be done without adding a theme to the default ones?
I changed the approach by moving the cast inside the WP_Theme constructor so I could add a unit test for that.

Last edited 2 years ago by lopo (previous) (diff)

#8 @lopo
2 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

#9 @lopo
2 years ago

  • Component changed from REST API to Themes
  • Focuses rest-api removed

#10 @SergeyBiryukov
21 months ago

  • Milestone changed from Future Release to 6.1

#11 @hugodevos
21 months ago

Test Report

This report validates that the indicated patch addresses the issue.

Patch tested: REPLACE_WITH_PATCH_URL

Environment

  • OS: macOS 12.3.1
  • Web Server: Nginx
  • PHP: 7.4.29
  • WordPress: 6.1-alpha-53344-src
  • Browser: Chrome 105.0.5195.102 (Official Build) (x86_64)
  • Theme: Twenty Twenty-Two (and Twenty Twenty-One)
  • Active Plugins:

Actual Results

  • ✅ Issue resolved with patch.
  • All worked as expected

Additional Notes

  • Any additional details worth mention.
  • Also tested with other interesting characters such as $$$ for the filename or 2021.2021 and worked as expected.

#12 @Ankit K Gupta
21 months ago

  • Keywords needs-testing removed

Hello All, I also got a chance to test this issue with some other theme like, Numeric, Alphanumeric, etc and PR https://github.com/WordPress/wordpress-develop/pull/2758 fixed the issue.

Test Report

Env

Steps to test

  1. Rename the theme name from style.css and folder name like 'wp-content/themes/2022' or 'wp-content/themes/AB123'
  2. Go to Appearance > Themes
  3. Activate your theme which has a name set in step-1

Test result

A theme that has names like 'ABC2022' / '2022' / 'Twenty Twenty Two', etc is appearing as active (if activated) with available option 'Customize'.

Screencast example:

Before Fix -

https://i.imgur.com/Gpc9Anm.jpg

After Fix by PR2758 -

https://i.imgur.com/IUMZA6k.jpg

Last edited 21 months ago by Ankit K Gupta (previous) (diff)

#13 @spacedmonkey
21 months ago

  • Milestone changed from 6.1 to 6.2

Sadly didn't get to this ticket in 6.1 release. Punting to 6.2

#14 @jakariaistauk
18 months ago

Test Report

This report validates that the indicated patch addresses the issue.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/2758

Environment

  • OS: macOS 13.0
  • Web Server: Nginx
  • PHP: 7.4.30
  • WordPress: 6.2-alpha-54642-src
  • Browser: Google Chrome 108.0.5359.124, Mozilla Firefox 108.0.1(64bit)
  • Theme: Twenty Ten, Twenty Twenty-Three

Actual Results

  • ✅Issue resolved with patch.

@jakariaistauk
18 months ago

Screenshot before fix the issue

@jakariaistauk
18 months ago

Screenshot after fix the issue

This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.


16 months ago

#16 @mukesh27
16 months ago

  • Keywords has-testing-info added

@alvastar thanks for the ticket!

This ticket was discussed during the recent bug scrub. It looks like it's unlikely that work will be done on this during the 6.2 cycle.

According to the manual test report, the ticket is operational. 

@spacedmonkey Is this still possible to land in 6.2, or should it be moved to Future Release for now?

Last edited 16 months ago by mukesh27 (previous) (diff)

This ticket was mentioned in Slack in #core by costdev. View the logs.


16 months ago

#18 @spacedmonkey
16 months ago

  • Milestone changed from 6.2 to Future Release

Bumping to future release.

#19 @SergeyBiryukov
16 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 55426:

Themes: Account for a numeric theme directory in WP_Theme::__construct().

This ensures that if a theme with a numeric directory name is active, it is correctly identified as such, and that theme support features work as expected.

Follow-up to [20029], [49925].

Props lopo, alvastar, winterpsv, hugodevos, ankit-k-gupta, jakariaistauk, mukesh27, spacedmonkey, SergeyBiryukov.
Fixes #54645.

#20 @SergeyBiryukov
16 months ago

  • Milestone changed from Future Release to 6.2

@SergeyBiryukov commented on PR #2758:


16 months ago
#21

Thanks for the PR! Merged in r55426.

Note: See TracTickets for help on using tickets.