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

[desrosj]

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.

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

[neychok]

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.

[desrosj]

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.

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

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

[mahbubshovan]

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.

[ironprogrammer]

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.

[neychok]

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

[mahbubshovan]

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

[robinwpdeveloper]

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.

[desrosj]

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.

[hellofromTonya]

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.

[ironprogrammer]

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:
  • test_trac_53566 (​test mu-plugin)

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.

Working as expected, no fatal error occured.

[simongomes02]

Working as expected, no fatal error occured.

[mukesh27]

This ticket was discussed in the bug scrub.

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

Additional props: @costdev

[audrasjb]

Self assigning for final testing and, hopefully, commit.

[audrasjb]

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.