Make WordPress Core

Opened 8 months ago

Closed 6 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's profile lgladdy Owned by: audrasjb's profile 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)

59018.diff (1.8 KB) - added by azaozz 8 months ago.

Download all attachments as: .zip

Change History (34)

This ticket was mentioned in Slack in #core-editor by lgladdy. View the logs.


8 months ago

#2 @lgladdy
8 months ago

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? That way, it will accurately check the provided paths are in the exact folder, rather than just a folder starting with the same name?

Last edited 8 months ago by lgladdy (previous) (diff)

#3 @masteradhoc
8 months ago

Following. thanks @lgladdy for debugging our issue!

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


8 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

#5 @audrasjb
8 months ago

  • Milestone changed from Awaiting Review to 6.3.1

Moving for 6.3.1 consideration.

#6 @masteradhoc
8 months ago

@lgladdy Thanks for the PR. i just tested on one of the sites having trouble - it fixes the issue

#7 @swissspidy
8 months ago

  • Keywords needs-unit-tests added

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


8 months ago

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


8 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:


8 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.


8 months ago

#13 @masteradhoc
8 months ago

  • Keywords needs-testing added

@swissspidy commented on PR #4983:


8 months ago
#14

There are still some coding standards errors being reported now

@rajinsharwar commented on PR #4983:


8 months ago
#15

Fixed those @swissspidy :)

#16 @audrasjb
8 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.


8 months ago

#18 @masteradhoc
8 months ago

Adding further testing instruction based on feedback of @audrasjb and @azaozz

Setup of our site is as following:

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": ["home", "events"],
  "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.

Version 0, edited 8 months ago by masteradhoc (next)

#19 follow-up: @masteradhoc
8 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 @azaozz
8 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.

Last edited 8 months ago by azaozz (previous) (diff)

#21 @masteradhoc
8 months ago

pinging @audrasjb / @guillaumeturpin / @jacknotman who worked on #57566 to check this regression.
Any idea to the comments if azaozz?

#22 @azaozz
8 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.

Last edited 8 months ago by azaozz (previous) (diff)

@azaozz
8 months ago

#23 @azaozz
8 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.


8 months ago

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


8 months ago

#26 @masteradhoc
8 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 @lgladdy
8 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 @audrasjb
7 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.

#29 @azaozz
7 months ago

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

In 56527:

Editor: Fix loading of assets in blocks in child themes where the directory name starts with the parent theme's directory name. Example: twentyseventeen and twentyseventeen-child.

Props: lgladdy, masteradhoc, audrasjb, rajinsharwar, azaozz.
Fixes: #59018.

#30 @azaozz
7 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopen for 6.3.2.

#31 @azaozz
7 months ago

  • Keywords fixed-major added

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


7 months ago

#33 @audrasjb
6 months ago

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

In 56776:

Editor: Fix loading of assets in blocks in child themes where the directory name starts with the parent theme's directory name.

Props lgladdy, masteradhoc, audrasjb, rajinsharwar, azaozz.
Merges [56527] to the 6.3 branch.
Fixes #59018.

Note: See TracTickets for help on using tickets.