Make WordPress Core

Opened 4 months ago

Closed 2 weeks ago

Last modified 2 weeks ago

#59704 closed enhancement (fixed)

Bundled themes: harden fonts URL code for PHP 8.1

Reported by: jordesign's profile jordesign Owned by: audrasjb's profile audrasjb
Milestone: 6.5 Priority: normal
Severity: normal Version:
Component: Bundled Theme Keywords: has-patch has-testing-info
Focuses: Cc:

Description (last modified by sabernhardt)

If twentyfifteen_fonts_url is overwritten and no fonts are returned then line 184 of functions.php creates a warning as the subject of str_replace does not exist.

Attachments (1)

child-themes-null-fonts.zip (932.1 KB) - added by sabernhardt 2 weeks ago.
child themes that replace the font URL, either returning null or using an empty bracket instead of returning a string

Download all attachments as: .zip

Change History (16)

#1 @jordesign
4 months ago

Last edited 2 weeks ago by sabernhardt (previous) (diff)

#2 @SergeyBiryukov
4 months ago

Hi there, welcome back to WordPress Trac! Thanks for the ticket.

Just noting that a similar fragment also exists in a few other themes:

$font_stylesheet = str_replace(
	array( get_template_directory_uri() . '/', get_stylesheet_directory_uri() . '/' ),
	'',
	twentyfifteen_fonts_url()
);
  • Twenty Thirteen
  • Twenty Fifteen
  • Twenty Sixteen
  • Twenty Seventeen

If twentyfifteen_fonts_url is overwritten and no fonts are returned then line 184 of functions.php creates a warning as the subject of str_replace does not exist.

It is worth noting that twentyfifteen_fonts_url() is documented to always return a string. It appears that the original reporter redeclared the function, but their implementation returns null instead of a string, causing a notice:

Deprecated: str_replace(): Passing null to parameter #3 ($subject) of type array|string is deprecated 

If they correct the customized function to always return a string, as the original does, that should resolve the issue.

#3 @sabernhardt
4 months ago

  • Description modified (diff)
  • Summary changed from TwentyFifteen: refactor twentyfifteen_fonts_url code for php 8.1 to Twenty Fifteen: harden twentyfifteen_fonts_url code for php 8.1
  • Type changed from defect (bug) to enhancement

I'm changing this to an enhancement because it should work with the documented type.

However, it could be worth fixing type errors, as I had done it wrong with Twenty Fifteen and/or Twenty Sixteen before working on the patches in #55985.

Last edited 4 months ago by sabernhardt (previous) (diff)

#4 @sabernhardt
4 weeks ago

A change similar to the patch on #45880 could avoid the error when the font stylesheet function does not return a string as expected, and it should be better with empty strings too.

		$editor_styles = array( 'css/editor-style.css', 'genericons/genericons.css' );
		$fonts_url     = twentyfifteen_fonts_url();
		if ( $fonts_url ) {
			$editor_styles[] = str_replace(
				array( get_template_directory_uri() . '/', get_stylesheet_directory_uri() . '/' ),
				'',
				$fonts_url
			);
		}
		add_editor_style( $editor_styles );

#5 @sabernhardt
4 weeks ago

Or for a smaller change, casting it as a string seems to prevent the error in PHP 8.2.

		$font_stylesheet = str_replace(
			array( get_template_directory_uri() . '/', get_stylesheet_directory_uri() . '/' ),
			'',
			(string) twentyfifteen_fonts_url()
		);

(The type cast is used for strpos() in twentyfifteen_scripts() and twentyfifteen_block_editor_styles(), and those functions did not cause an error when I overrode the fonts URL function with an empty bracket function twentyfifteen_fonts_url() {} in a child theme.)

Last edited 4 weeks ago by sabernhardt (previous) (diff)

This ticket was mentioned in PR #5995 on WordPress/wordpress-develop by @sabernhardt.


4 weeks ago
#6

  • Keywords has-patch added

Avoid PHP type errors with str_replace() when removing directory from font functions.

  • twentythirteen_fonts_url()
  • twentyfourteen_font_url()
  • twentyfifteen_fonts_url()
  • twentysixteen_fonts_url()
  • twentyseventeen_fonts_url()

Trac 59704

#7 @sabernhardt
4 weeks ago

  • Milestone changed from Awaiting Review to 6.5
  • Summary changed from Twenty Fifteen: harden twentyfifteen_fonts_url code for php 8.1 to Bundled themes: harden fonts URL code for PHP 8.1

I wanted to submit both options but had trouble with adding to the array. So I just made a patch for the simpler changes.

#8 @poena
3 weeks ago

  • Keywords needs-testing needs-testing-info added

@sabernhardt
2 weeks ago

child themes that replace the font URL, either returning null or using an empty bracket instead of returning a string

#9 @sabernhardt
2 weeks ago

  • Keywords has-testing-info added; needs-testing-info removed

Testing:

  1. Check the PHP version to make sure it is at least 8.1, and verify that WP_DEBUG is true.
  2. Install the current versions of Twenty Thirteen, Twenty Fourteen, Twenty Fifteen, Twenty Sixteen and Twenty Seventeen.
  3. Add the child themes from the zipped file.
  4. Activate one of the child themes. This should show an error about the str_replace function and possibly another about headers already sent.
  5. Activate Twenty Seventeen or another parent theme (one that does not cause an error), and refresh the Themes page if it shows a 'headers already sent' error.
  6. Continue activating the other child themes to confirm the errors, and activate an errorless theme in between each.
  7. Activate Twenty Seventeen.
  8. Apply the patch (PR 5995).
  9. Activate each of the child themes to confirm that they do not show errors anymore.
  10. If you visit the site or open the post editor, the font should be a system font.

#10 @huzaifaalmesbah
2 weeks ago

Test Report

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

Environment

  • WordPress: 6.5-alpha-56966-src
  • PHP: 8.2.15
  • Server: nginx/1.25.3
  • Database: mysqli (Server: 8.0.36 / Client: mysqlnd 8.2.15)
  • Browser: Chrome 121.0.0.0
  • OS: macOS
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.0.1

Actual Results

✅ Issue resolved with patch.

Screenshots

Before Apply Patch After Apply Patch ✅
https://i.ibb.co/ZKzqVHH/Screenshot-2024-02-11-at-8-46-06-AM.png https://i.ibb.co/RSr7M0T/Screenshot-2024-02-11-at-8-45-33-AM.png

#11 @shailu25
2 weeks ago

Test Report:

Tested Patch: https://github.com/WordPress/wordpress-develop/pull/5995

Testing Environment:

OS: Windows
Web Server: Nginx
PHP: 8.1.23
WordPress: 6.4.3
Browser: Chrome
Theme: Twenty Thirteen, Twenty Fourteen, Twenty Fifteen, Twenty Sixteen, Twenty Seventeen
Active Plugins: None

Actual Results:

  • Font Issue is Resolved with patch.✅

Screenshots:

Twenty Thirteen:
Before Patch: https://prnt.sc/QqepYU_IYheF
After Patch: https://prnt.sc/jW6B7Rq6h5pJ

Twenty Fourteen:
Before Patch: https://prnt.sc/aC9G0_-hyScF
After Patch: https://prnt.sc/wymyC7gWpaNU

Twenty Fifteen:
Before Patch: https://prnt.sc/jxqdfGUuWdTC
After Patch: https://prnt.sc/4O8s2xCpXQ0z

Twenty Sixteen:
Before Patch: https://prnt.sc/3xDAvQ_ufKJV
After Patch: https://prnt.sc/g5tVjwTNGX56

Twenty Seventeen:
Before Patch: https://prnt.sc/Sjtx1-ECbqMK
After Patch: https://prnt.sc/7ct4rkLdZZsQ

#12 @audrasjb
2 weeks ago

  • Keywords needs-testing removed
  • Owner set to audrasjb
  • Status changed from new to reviewing

Ok it's looking great. Self assigning for final review and commit.

#13 @audrasjb
2 weeks ago

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

In 57601:

Bundled Themes: Cast font URL functions to string for add_editor_style().

This changeset ensures the result of the font URL functions is a string before using it in add_editor_style(), to avoid PHP warnings on child themes. This similarily updates Twenty Thirteen, Twenty Fourteen, Twenty Fifteen, Twenty Sixteen, and Twenty Seventeen.

Props jordesign, SergeyBiryukov, sabernhardt, huzaifaalmesbah, shailu25.
Fixes #59704.

#15 @audrasjb
2 weeks ago

Note: I manually added the missed props for @shawfactor using the Core Props tool.

Note: See TracTickets for help on using tickets.