Skip to content

Conversation

@adamsilverstein
Copy link
Member

Summary

Fixes errors raised when running composer phpstan

Relevant technical choices

@github-actions
Copy link

github-actions bot commented Oct 28, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: adamsilverstein <[email protected]>
Co-authored-by: westonruter <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@westonruter
Copy link
Member

I can indeed see the following errors after doing composer update:

------ ------------------------------------------------------------------------------------------------------ 
  Line   plugins/embed-optimizer/hooks.php                                                                     
 ------ ------------------------------------------------------------------------------------------------------ 
  185    Parameter #1 $function_name of function wp_trigger_error expects callable-string, '{closure}' given.  
         🪪  argument.type                                                                                     
 ------ ------------------------------------------------------------------------------------------------------ 

 ------ ------------------------------------------------------------------------------------------------- 
  Line   plugins/optimization-detective/class-od-html-tag-processor.php                                   
 ------ ------------------------------------------------------------------------------------------------- 
  633    Parameter #1 $function_name of function wp_trigger_error expects callable-string, string given.  
         🪪  argument.type                                                                                
 ------ ------------------------------------------------------------------------------------------------- 

 ------ ----------------------------------------------------------------------------------------------- 
  Line   plugins/optimization-detective/storage/class-od-url-metrics-post-type.php                      
 ------ ----------------------------------------------------------------------------------------------- 
  122    Parameter #3 $error_level of function wp_trigger_error expects 256|512|1024|16384, int given.  
         🪪  argument.type                                                                              
 ------ ----------------------------------------------------------------------------------------------- 

Comment on lines 185 to 186
wp_trigger_error( __FUNCTION__, esc_html( $message ) );
$function_name = __FUNCTION__;
$trigger_error = static function ( string $message ) use ( &$function_name ): void {
wp_trigger_error( $function_name, esc_html( $message ) );
Copy link
Member

Choose a reason for hiding this comment

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

Good catch. This was wrong!

Copy link
Member Author

Choose a reason for hiding this comment

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

right, PHPSTan caught that - __FUNCTION__ is not the same inside a closure.

@westonruter westonruter added the [Type] Bug An existing feature is broken label Oct 28, 2024
Comment on lines 121 to 127
$trigger_error = static function ( string $message, int $error_level = E_USER_NOTICE ) use ( $this_function ): void {
// Default to E_USER_NOTICE.
if ( ! in_array( $error_level, array( E_USER_NOTICE, E_USER_WARNING, E_USER_ERROR, E_USER_DEPRECATED ), true ) ) {
$error_level = E_USER_NOTICE;
}
wp_trigger_error( $this_function, esc_html( $message ), $error_level );
};
Copy link
Member

@westonruter westonruter Oct 28, 2024

Choose a reason for hiding this comment

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

It's too bad that this doesn't seem to work:

/**
 * Triggers error.
 *
 * @param string $message     The message explaining the error.
 * @param int    $error_level Optional. The designated error type for this error.
 *
 * @phpstan-param E_USER_DEPRECATED|E_USER_ERROR|E_USER_NOTICE|E_USER_NOTICE $error_level
 */
$trigger_error = static function ( string $message, int $error_level = E_USER_NOTICE ) use ( $this_function ): void {
	if ( ! in_array( $error_level, array( E_USER_ERROR, E_USER_NOTICE, E_USER_NOTICE, E_USER_DEPRECATED ), true ) ) {
		throw new InvalidArgumentException( 'Invalid error level provided.' );
	}
	wp_trigger_error( $this_function, esc_html( $message ), $error_level );
};

If I do $trigger_error( 'foo', 181293 ) it doesn't trigger any PHPStan static analysis error.

Copy link
Member Author

Choose a reason for hiding this comment

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

But you do get a "InvalidArgumentException", right?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I mean more this part:

@phpstan-param E_USER_DEPRECATED|E_USER_ERROR|E_USER_NOTICE|E_USER_NOTICE $error_level

I'm not getting any static analysis issues when passing an integer other than those.

@westonruter
Copy link
Member

Unit tests are failing in trunk for the webp-uploads plugin, which is unrelated to this PR. However, they'll need to be fixed either in this PR or another.

@westonruter
Copy link
Member

Unit tests are failing in trunk for the webp-uploads plugin, which is unrelated to this PR. However, they'll need to be fixed either in this PR or another.

I've opened #1634 to address this.

@westonruter westonruter merged commit ce2880d into WordPress:trunk Nov 5, 2024
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Type] Bug An existing feature is broken

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants