Skip to content

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
Member

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!

)
);
if ( ! file_exists( $script_asset_path ) ) {
if ( false === $script_asset_path ) {
Copy link
Member

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

/* 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
Member

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.

_doing_it_wrong(
__FUNCTION__,
sprintf(
/* translators: 1: Field name, 2: Block name. */
Copy link
Member

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