Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ticket/53566 #2313

Open
wants to merge 12 commits into
base: trunk
Choose a base branch
from
Open

Ticket/53566 #2313

wants to merge 12 commits into from

Conversation

Neychok
Copy link

@Neychok Neychok commented Feb 11, 2022

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

@desrosj
Copy link
Contributor

desrosj commented Oct 25, 2022

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

@Neychok
Copy link
Author

Neychok commented Oct 27, 2022

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

@@ -114,12 +114,13 @@ function register_block_script_handle( $metadata, $field_name, $index = 0 ) {
substr_replace( $script_path, '.asset.php', - strlen( '.js' ) )
)
);
if ( ! file_exists( $script_asset_path ) ) {
if ( false === $script_asset_path ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also commented on the ticket, but 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch @desrosj

Changed in 7db1cba

_doing_it_wrong(
__FUNCTION__,
sprintf(
/* translators: 1: Field name, 2: Block name. */
__( 'The asset file for the "%1$s" defined in "%2$s" block definition is missing.' ),
__( 'The asset file (%1$s) for the "%2$s" defined in "%3$s" block definition is missing.' ),
dirname( $metadata['file'] ) . '/' . substr_replace( $script_path, '.asset.php', - strlen( '.js' ) ),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we avoid having to do this twice? A third variable in addition to $script_handle, and $script_asset_path could store the result of this line before passing to wp_normalize_path( realpath() )above and then to the_doing_it_wrong()`.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. 749efcf stores the result in a variable for reuse.

@@ -114,12 +114,13 @@ function register_block_script_handle( $metadata, $field_name, $index = 0 ) {
substr_replace( $script_path, '.asset.php', - strlen( '.js' ) )
)
);
if ( ! file_exists( $script_asset_path ) ) {
if ( false === $script_asset_path ) {
_doing_it_wrong(
__FUNCTION__,
sprintf(
/* translators: 1: Field name, 2: Block name. */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The translator comment needs to be updated to correctly describe all three placeholders.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 9a48f68

@simongomes
Copy link

Working as expected, no fatal error occured.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants