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

Fix notice error in php unit tests. #5320

Closed
wants to merge 2 commits into from

Conversation

spacedmonkey
Copy link
Member

@spacedmonkey spacedmonkey commented Sep 26, 2023

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


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@spacedmonkey spacedmonkey changed the title No real path when register core block scripts Fix notice error in php unit tests. Sep 26, 2023
@@ -181,7 +181,7 @@ function register_block_script_handle( $metadata, $field_name, $index = 0 ) {
$script_uri = get_block_asset_url( $script_path_norm );

$script_args = array();
if ( 'viewScript' === $field_name ) {
if ( 'viewScript' === $field_name && $script_uri ) {
Copy link
Member

Choose a reason for hiding this comment

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

This works too. But it's just that it's not ideal from a code coverage perspective, since the in normal execution the condition would be truthy. It's just that during PHPUnit tests that it is false. So this is essentially adding a condition like ! $is_running_unit_tests

Copy link
Member Author

Choose a reason for hiding this comment

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

@westonruter This is a valid case to catch outside of unit tests. If you pass an empty thing as a path to script, this should result false. For example, if you using a unbuilt version of plugin for development this might happen.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@spacedmonkey spacedmonkey self-assigned this Sep 27, 2023
@spacedmonkey spacedmonkey marked this pull request as ready for review September 27, 2023 08:55
Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

I don't feel strongly about my comment. If it's good with another reviewer, it's good for me.

@spacedmonkey
Copy link
Member Author

Committed in 345fb48

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