Opened 15 months ago
Closed 13 months ago
#59018 closed defect (bug) (fixed)
Blocks in child themes with folder names starting with the same as the parent cannot load assets in WP6.3
Reported by: | lgladdy | Owned by: | audrasjb |
---|---|---|---|
Milestone: | 6.3.2 | Priority: | normal |
Severity: | major | Version: | 6.3 |
Component: | Editor | Keywords: | has-unit-tests has-patch has-testing-info commit fixed-major |
Focuses: | Cc: |
Description
A bug has been introduced in WordPress 6.3 for blocks contained inside a child theme, where their path begins with the parent themes path.
For example, if my theme is called twentytwentythreechild
as is a child theme of twentytwentythree
the code in blocks.php's register_block_script_handle and register_block_style_handle to handle child themes assets path will incorrectly change the script to:
/wp-content/themes/twentytwentythree/child/blocks/all-fields-block-json/all-fields-block-in-theme.js
instead of:
/wp-content/themes/twentytwentythreechild/blocks/all-fields-block-json/all-fields-block-in-theme.js
The specific code causing this is the switch on $is_parent_theme_block
, commented as // Get the script path deterministically based on whether or not it was registered in a parent or child theme.
Attachments (1)
Change History (34)
This ticket was mentioned in Slack in #core-editor by lgladdy. View the logs.
15 months ago
This ticket was mentioned in PR #4978 on WordPress/wordpress-develop by @lgladdy.
15 months ago
#4
- Keywords has-patch added
This PR resolves a bug introduced in WordPress 6.3 where blocks in a child theme contained in a path matching the start of the parent theme will not be able to load assets correctly.
Trac ticket: https://core.trac.wordpress.org/ticket/59018
#6
@
15 months ago
@lgladdy Thanks for the PR. i just tested on one of the sites having trouble - it fixes the issue
This ticket was mentioned in Slack in #core by masteradhoc. View the logs.
15 months ago
This ticket was mentioned in PR #4983 on WordPress/wordpress-develop by @rajinsharwar.
15 months ago
#9
- Keywords has-unit-tests added; needs-unit-tests removed
Attempt to add unit tests.
Trac ticket: https://core.trac.wordpress.org/ticket/59018
@rajinsharwar commented on PR #4983:
15 months ago
#10
Made some serious mistakes I guess, will try to fix those tomorrow.
This ticket was mentioned in Slack in #core by masteradhoc. View the logs.
15 months ago
#12
@
15 months ago
Updated the unit tests https://github.com/WordPress/wordpress-develop/pull/4983
@swissspidy commented on PR #4983:
15 months ago
#14
There are still some coding standards errors being reported now
@rajinsharwar commented on PR #4983:
15 months ago
#15
Fixed those @swissspidy :)
#16
@
15 months ago
- Milestone changed from 6.3.1 to 6.3.2
WP 6.3.1 is going to be released in the next few days, so let's move this ticket to 6.3.2 to give it more time to be committed and backported.
This ticket was mentioned in Slack in #core by masteradhoc. View the logs.
15 months ago
#18
@
15 months ago
Adding further testing instruction based on feedback of @audrasjb and @azaozz
Setup of our site is as following:
- WordPress 6.2.2
- GeneratePress Theme (Slug: generatepress) see https://wordpress.org/themes/generatepress/
- GeneratePress ChildTheme (Slug: generatepress_child) download from here: https://docs.generatepress.com/article/using-child-theme/#installing-a-child-theme
- ACF Pro
Now you'll add a block over ACFs PHP Method see https://www.advancedcustomfields.com/resources/blocks/
In functions.php you'll call the block like this
<?php add_action( 'init', 'register_acf_blocks' ); function register_acf_blocks() { register_block_type( __DIR__ . '/inc-child/blocks/demo-v2' ); }
In the respective folder you'll have a block.json that's setup like this: https://www.advancedcustomfields.com/resources/acf-blocks-key-concepts/
{ "name": "acf/board-members", "title": "Board Members", "description": "Board Members", "script": "file:./board-members.js", "category": "formatting", "icon": "admin-comments", "keywords": ["board", "team"], "supports": { "anchor": true }, "acf": { "mode": "preview", "renderTemplate": "board-members.php" }, "align": "full" }
After updating the WordPress Version to 6.3 we noticed that the path where the js file is called is wrong.
WP 6.2.2: /generatepress_child/inc-child/blocks/slider-bottom/slider.js WP 6.3 /generatepress/_child/inc-child/blocks/slider-bottom/slider.js
We renamed our child theme from generatepress_child to childthemetest and the js was called correctly again.
Also if the above fixed is applied the file was called correctly.
#19
follow-up:
↓ 20
@
15 months ago
Further testing and trying to find the regression shows that without this commit it would work: https://github.com/WordPress/wordpress-develop/commit/03ff5a4f225927447b25591b4f18aa68f30b35ed
Affected file: src/wp-includes/blocks.php
Trac Ticket: #57566
#20
in reply to:
↑ 19
@
15 months ago
- Keywords needs-patch added; has-patch removed
Replying to masteradhoc:
Yes seems this is caused by [56183]. If the child theme directory name starts with the parent theme's directory name, these two are both true
(which shouldn't be possible?):
$is_parent_theme_block = str_starts_with( $script_path_norm, $template_path_norm ); $is_child_theme_block = str_starts_with( $script_path_norm, $stylesheet_path_norm );
Testing with a child theme of Twenty Seventeen that's in a twentyseventeen-child
directory,
wp_normalize_path( get_template_directory() ) wp_normalize_path( get_stylesheet_directory() )
return
string(39) "/var/www/wp-content/themes/twentyseventeen" string(45) "/var/www/wp-content/themes/twentyseventeen-child"
where both match str_starts_with( $script_path_norm, ... )
so $is_parent_theme_block
is erroneously set to true. Later on that's used when determining the URL in $script_uri = $is_parent_theme_block ? ...
.
Assuming that $is_parent_theme_block
and $is_child_theme_block
cannot be true at the same time, an "easy fix" may be to test for a child theme block first and if negative, test for a parent theme. However that leaves the edge case where a parent theme's name/directory might contain the child theme's name (the reverse case). The same seems true for the proposed patch with adding a slash.
Seems matching with the longer string first may work, but is somewhat "weak". Ideally this comparison should not be used to determine whether a block is from a parent or child theme.
#21
@
15 months ago
pinging @audrasjb / @guillaumeturpin / @jacknotman who worked on #57566 to check this regression.
Any idea to the comments if azaozz?
#22
@
15 months ago
Looking at this more: seems adding a trailing slash would make it work (somehow) for now. Not a fan of this solution but then there seems to be some "pretty unusual" code there (not really sure what is the intent for this use of substr_replace: substr_replace( $script_path, '.asset.php', - strlen( '.js' ) )
, why the negative index and why do strlen()
of a hard-coded string).
Thinking the trailingslashit()
would be better applied to $template_path_norm
and $stylesheet_path_norm
, not just when comparing the paths. Later on when construction the script/stylesheet URL get_theme_file_uri()
removes slashes from the beginning of the fragment anyways.
#23
@
15 months ago
- Keywords has-patch added; needs-patch removed
In 59018.diff: A variant of https://github.com/WordPress/wordpress-develop/pull/4978. Add trailing slashes to the parent and child themes directories as returned by get_template_directory()
and get_stylesheet_directory()
.
This ticket was mentioned in Slack in #core by azaozz. View the logs.
15 months ago
This ticket was mentioned in Slack in #core by masteradhoc. View the logs.
15 months ago
#26
@
15 months ago
@audrasjb tested as discussed on slack the solution of @azaozz
Here are some steps to replicate:
1) Preparation see https://core.trac.wordpress.org/ticket/59018#comment:18
2) applied azaozz change locally
3) the path seems to be called directly now. no errors appear anymore
After: /generatepress_child/inc-child/blocks/slider-bottom/slider.js Before: /generatepress/_child/inc-child/blocks/slider-bottom/slider.js
LGTM.
#27
@
15 months ago
Also looks good to me. @azaozz's fix location make sense; I just wasn't sure if applying it there in my original patch would have caused any issues in other places!
Confirmed this new patch resolves the issue in all my test cases.
#28
@
14 months ago
- Keywords has-testing-info commit added; needs-testing removed
- Owner set to audrasjb
- Status changed from new to accepted
I tested the patch in various test cases and it seems to fix the issue on my side as well.
#30
@
14 months ago
- Resolution fixed deleted
- Status changed from closed to reopened
Reopen for 6.3.2.
Small correction here, the issue using
str_starts_with
to determine if the block assets are on the child or parent theme to set $is_parent_theme_block and $is_child_theme_block.This happens a few lines earlier in the functions.
I'd suggest the fix here is to add the trailing slash to
$template_path_norm
and$stylesheet_path_norm
for these checks?