Make WordPress Core

Opened 7 months ago

Closed 7 months ago

Last modified 6 months ago

#59489 closed defect (bug) (fixed)

Stale core block style cache updates not working on Windows OS

Reported by: wildworks's profile wildworks Owned by: joemcgill's profile joemcgill
Milestone: 6.3.2 Priority: normal
Severity: normal Version: 6.3.2
Component: Themes Keywords: has-patch fixed-major
Focuses: performance Cc:

Description

In [56528], The old core block style cache that was stored as a full path is now replaced by a new relative path. However, this cache update doesn't seem to be working on Windows.

As far as I've researched, the problem is related to this code: https://github.com/WordPress/wordpress-develop/blob/e3a612a97c4c45acd7613779a46d86860e76cc65/src/wp-includes/blocks/index.php#L59-L76

This code does not correctly replace the old cache with a relative path on Windows OS, as below:

  • If the cache's WordPress version is different from the current WordPress version, try to create a new cache by replacing the full path with a relative path.
  • Use the glob function to find the full path of the stylesheet. All path separators in the returned value array are forward slashes: D:/_local/WordPress/wp6.4/app/public/wp-includes/blocks/archives/editor-rtl.css D:/_local/WordPress/wp6.4/app/public/wp-includes/blocks/archives/editor-rtl.min.css
  • However, the BLOCKS_PATH constant on which the substitution is based includes a backslash in the path separator, as shown below: D:\_local\WordPress\wp6.4\app\public/wp-includes/blocks/
  • This results in a cache that still has the full path.
  • Subsequent style enqueues expect the file path to be relative, so the style will not be registered.

This problem seems to be solved by adding normalization to str_replace() function as shown below:

$files = array_map(
	static function ( $file ) {
		return str_replace( wp_normalize_path( BLOCKS_PATH ), '', $file );
	},
	$files
);

Attachments (2)

patch.diff (522 bytes) - added by wildworks 7 months ago.
tt4.png (116.0 KB) - added by wildworks 7 months ago.
How the Twenty Twenty-Four theme looks on Windows OS when updated to WordPress6.4 Beta1

Download all attachments as: .zip

Change History (32)

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


7 months ago

#2 @wildworks
7 months ago

P.S. This issue was originally reported on Gutenberg: https://github.com/WordPress/gutenberg/issues/54849

#3 @swissspidy
7 months ago

  • Component changed from General to Themes
  • Version set to 6.3.2

@wildworks
7 months ago

#4 @wildworks
7 months ago

  • Keywords has-patch added

#5 @wildworks
7 months ago

To reproduce this issue and to test the patch, WordPress must be hosted on Windows host OS.

#6 @wildworks
7 months ago

I think this issue should be fixed in WordPress 6.4. Otherwise, users developing sites on Windows OS will have old absolute paths cached and will not be able to see the correct block style on the front end.

@wildworks
7 months ago

How the Twenty Twenty-Four theme looks on Windows OS when updated to WordPress6.4 Beta1

#7 @swissspidy
7 months ago

  • Milestone changed from Awaiting Review to 6.4

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


7 months ago

#9 @joemcgill
7 months ago

  • Focuses performance added
  • Keywords needs-testing added
  • Milestone changed from 6.4 to 6.3.2

#10 @joemcgill
7 months ago

#59469 was marked as a duplicate.

#11 @pbiron
7 months ago

Test Report

Environment

OS: Windows 11
PHP: 8.0.29
Apache: 2.4.53
WP: 6.4-beta2

Testing

Prior to applying the patch, I was experiencing the problem that no core block CSS was being loaded when a block-based theme was used. After applying the patch, the core block CSS is being loaded.

Great work @wildworks!

This ticket was mentioned in PR #5409 on WordPress/wordpress-develop by joemcgill.


7 months ago
#12

This PR implements the patch provided by wildworks on the original ticket.

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

This ticket was mentioned in Slack in #core-performance by joemcgill. View the logs.


7 months ago

#14 @joemcgill
7 months ago

  • Keywords needs-unit-tests added
  • Owner set to joemcgill
  • Status changed from new to assigned

Thanks for confirming @pbiron! I'd like to get some unit tests added for this so the problem doesn't re-surface, but that shouldn't block this from landing in time for 6.3.2 and 6.4-beta3 if needed.

#15 @pbiron
7 months ago

I also now done a search thru core code looking for other places where not normalizing BLOCKS_PATH would cause a problem in Windows. The only other place I can find is https://core.trac.wordpress.org/browser/trunk/src/wp-includes/blocks/index.php?rev=56559#L92 and it's already normalized there.

Last edited 7 months ago by pbiron (previous) (diff)

#16 follow-up: @joemcgill
7 months ago

Thanks @pbiron. I did the same search and came to the same conclusion as you. It may be useful to look into normalizing the BLOCKS_PATH value when it's created, rather than throughout this code, but is not necessary to fix this bug. Can you test the updates I've made to the original patch in PR #5409 and confirm it's still working?

joemcgill commented on PR #5409:


7 months ago
#17

Thanks @felixarntz. I do think that running the test suite on a windows OS would be the best way to uncover this type of issue. I've followed up with @desrosj and may open another ticket to address that issue. Otherwise, we'd need to refactor this function in a way that we could test it with mocked paths.

#18 in reply to: ↑ 16 @pbiron
7 months ago

Replying to joemcgill:

It may be useful to look into normalizing the BLOCKS_PATH value when it's created, rather than throughout this code, but is not necessary to fix this bug.

I was thinking the same thing, but didn't want to rush into anything like that for fear of breaking something else.

Can you test the updates I've made to the original patch in PR #5409 and confirm it's still working?

I commented on the PR that moving the normalization outside the loop still works...no need to normalize the constant ~500 times ;-)

#19 @joemcgill
7 months ago

  • Keywords commit added; needs-testing needs-unit-tests removed

Given feedback from @pbiron and @flixos90. I'm setting this as ready for commit and will handle getting this in trunk and the 6.3 branch prior to 6.3.2.

Discussion about testing on Windows will need to happen in a separate ticket.

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


7 months ago

#21 @joemcgill
7 months ago

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

In 56785:

Themes: Fix core block style paths on Windows.

This is a follow-up to [56528], which normalizes the BLOCKS_PATH for Windows prior to making paths relative for caches during the registration process. Prior to this change, incorrect file paths would lead to broken styles for core blocks on Windows.

Props wildworks, pbiron, flixos90, joemcgill.
Fixes #59489. See #59111.

#22 @joemcgill
7 months ago

  • Keywords fixed-major added; commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening to backport to the 6.3 branch.

#24 @spacedmonkey
7 months ago

@joemcgill Could we not have reused the variable in the line above?

// Normalize BLOCKS_PATH prior to substitution for Windows environments.
$normalized_blocks_path = wp_normalize_path( BLOCKS_PATH );
$files = glob( wp_normalize_path( $normalized_blocks_path . '**/**.css' ) );

#25 @joemcgill
7 months ago

@spacedmonkey I considered whether we should really normalize the BLOCK_PATH value when it's created so it's used throughout, but decided against it since other constants like WP_INC are not following that pattern. There's not really any advantage to using the normalized blocks path in the line above, since we would still need to normalize the '**/ **.css' part of that glob pattern. I just wanted to make sure we weren't normalizing the same value inside the array_map() loop. I think any future optimizations to this process could happen in new tickets at this point.

#26 @audrasjb
7 months ago

Fine. I'm going to backport this to 6.3 right now.

#27 @audrasjb
7 months ago

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

In 56789:

Themes: Fix core block style paths on Windows.

This is a follow-up to [56528], which normalizes the BLOCKS_PATH for Windows prior to making paths relative for caches during the registration process. Prior to
this change, incorrect file paths would lead to broken styles for core blocks on Windows.

Props wildworks, pbiron, flixos90, joemcgill.
Merges [56785] to the 6.3 branch.
Fixes #59489. See #59111.

#28 @wildworks
7 months ago

Thank you to everyone who worked on this issue.

As a Windows user myself, I understand that differences in path delimiters can cause problems and confusion.

I don't think there are any other major WIndows OS-specific issues in the core so far, but this Gutenberg issue, for example, suggests an issue with normalizing path separators.

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


7 months ago

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


6 months ago

Note: See TracTickets for help on using tickets.