Make WordPress Core

Opened 3 years ago

Closed 12 months ago

#53566 closed defect (bug) (fixed)

Pass the asset path to the `_doing_it_wrong()` notice in `register_block_script_handle()`

Reported by: desrosj's profile desrosj Owned by: audrasjb's profile audrasjb
Milestone: 6.2 Priority: normal
Severity: normal Version:
Component: Script Loader Keywords: good-first-bug has-patch commit
Focuses: Cc:

Description

The _doing_it_wrong() notice in register_block_script_handle() provides the metadata field name and block name when the requested asset does not exist, but not the path of where the asset is expected to be. Adding the value passed to realpath() could be helpful to debug why the file is considered non-existent.

Also, the conditional surrounding the _doing_it_wrong() notice is a ! file_exists() check. However, the path used to check for the file is passed through realpath() before, which will be false if the file does not exist. The file_exists() check could be removed in favor of a false !== $script_asset_path check.

Related #53565.

Change History (24)

#1 @desrosj
3 years ago

  • Summary changed from Include the asset path to the `_doing_it_wrong()` notice in `register_block_script_handle()` to Pass the asset path to the `_doing_it_wrong()` notice in `register_block_script_handle()`

This ticket was mentioned in PR #2313 on WordPress/wordpress-develop by Neychok.


2 years ago
#2

  • Keywords has-patch added

Added asset file path in _doing_it_wrong() notice in register_block_script_handle()

Changed the ! file_exists() check to ! file_exists( $script_asset_path as suggested by desrosj

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

#3 @neychok
2 years ago

Hey @desrosj, I did the changes you suggested. I wasn't sure about how to structure the notice text but hopefully this does the job.

#4 @JeffPaul
17 months ago

  • Keywords needs-testing added

#5 @desrosj
17 months ago

  • Milestone changed from Awaiting Review to 6.2

Thanks @neychok! I've added this to the 6.2 milestone to take a look. 6.1 RC is in a few days, so I'd rather not force this one in.

@desrosj commented on PR #2313:


16 months ago
#6

Sorry this took so long to get to @Neychok! Could you refresh this PR against the current state of trunk?

@neychok commented on PR #2313:


16 months ago
#7

@desrosj, just refreshed it. Thank you for looking into this!

#8 follow-up: @mahbubshovan
13 months ago

I see in the GitHub PR some checks were not successful.

It would be great if someone added testing instructions so that I could test and submit a report.

This ticket was mentioned in Slack in #core-test by robinwpdeveloper. View the logs.


13 months ago

#10 in reply to: ↑ 8 @ironprogrammer
13 months ago

  • Keywords changes-requested added

Thank you for the progress so far, @neychok! This PR needs some work, and could use a refresh against trunk. Hopefully we can see some traction on this before the coming beta 🤞🏻.

As @desrosj pointed out, $script_asset_path is false when the asset file is missing, so changes are needed to capture the missing asset path to provide to _doing_it_wrong().


I might suggest the following test instructions and acceptance criteria for this ticket (and happy to update the provided test plugin if these change):

Testing Instructions

Steps to Reproduce or Test

  1. In your test site, ensure that WP_DEBUG and WP_DEBUG_LOG are enabled.
  2. Install the test plugin located here on your test site (gist includes instructions).
  3. In a browser, open or refresh the homepage on your test site.
  4. Inspect the logged PHP notice that begins with PHP Notice: Function register_block_script_handle was called <strong>incorrectly</strong>.

Expected Results

When reproducing existing behavior:

  • ❌ The PHP notice does not contain the expected missing path:

The asset file for the "script" defined in "unit-tests/test-block" block definition is missing.

When testing a patch to validate it works as expected:

  • ✅ The PHP notice should contain the expected path to the missing .asset.php file derived from $metadata['script']:

The asset file "blocks/notice/missing-asset.asset.php" for the "script" defined in "unit-tests/test-block" block definition is missing.

#11 @neychok
13 months ago

@ironprogrammer, just refreshed my PR and I also saw some crucial mistakes that I corrected.

#12 @mahbubshovan
13 months ago

After following the instructions, I have found the below scenario:

@ironprogrammer After applying the patch, I see fatal errors.

Before - https://d.pr/i/Rv4d7Q
After - https://d.pr/i/dv47eo

#13 @robinwpdeveloper
13 months ago

Found fatal error.
I think we need help from @ironprogrammer here.
https://d.pr/i/A3GZFU

I have tried with mu-plugins and as a separate plugin too.

#14 @desrosj
13 months ago

I've done some testing on this and left a review on the pull request. Mentioning some of the feedback here for reference as well.

It appears my initial suggestion to use a false === $script_asset_path comparison was incorrect. While realpath() returns explicit false when a file does not exist, wp_normalize_path() will return an empty string when passed false. Seems an empty() check would be more appropriate.

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


12 months ago

#16 @hellofromTonya
12 months ago

  • Keywords changes-requested removed

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

The patch has been updated with @desrosj feedback including using empty() to resolve the fatal error @robinwpdeveloper and @mahbubshovan caught (thank you!).

@robinwpdeveloper @ironprogrammer this is ready for another round of testing, if you have time.

@desrosj your feedback has been updated in the PR. If you have time, please re-review.

#17 @ironprogrammer
12 months ago

Thanks, all, for the PR reviews and update!

Test Report

Patch tested: https://github.com/WordPress/wordpress-develop/pull/2313 since 749efcf 👍🏻

Environment

  • Hardware: MacBook Pro Apple M1 Pro
  • OS: macOS 12.6.3
  • Browser: Safari 16.3
  • Server: nginx/1.23.3
  • PHP: 7.4.33
  • WordPress: 6.2-alpha-54642-src
  • CLI: WP-CLI 2.7.1
  • Theme: twentytwentythree v1.0
  • Active Plugins:

Actual Results

  • ✅ The _doing_it_wrong() notice is updated with the absolute path to the missing asset file. (Path shown is truncated for privacy.)

The asset file (../wp-content/mu-plugins/blocks/notice/missing-asset.asset.php) for the "script" defined in "unit-tests/test-block" block definition is missing.

  • ✅ Previous fatal error in patched register_block_script_handle() has been resolved.

@simongomes02 commented on PR #2313:


12 months ago
#18

Working as expected, no fatal error occured.

#19 @simongomes02
12 months ago

Working as expected, no fatal error occured.

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


12 months ago

#21 @mukesh27
12 months ago

  • Keywords commit added; needs-testing removed

This ticket was discussed in the bug scrub.

Confirm that PR working fine in manual testing. Moving to commit consideration.

Additional props: @costdev

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


12 months ago

#23 @audrasjb
12 months ago

  • Owner set to audrasjb
  • Status changed from new to reviewing

Self assigning for final testing and, hopefully, commit.

#24 @audrasjb
12 months ago

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

In 55446:

Script Loader: Pass the asset path to the _doing_it_wrong() notice in register_block_script_handle().

This changeset ensures the file path is correctly passed in the output from _doing_it_wrong() notice in register_block_script_handle().

Props desrosj, neychok, mahbubshovan, ironprogrammer, robinwpdeveloper, hellofromTonya, simongomes02, mukesh27, costdev.
Fixes #53566.

Note: See TracTickets for help on using tickets.