Fix PHPStan errors by thelovekesh · Pull Request #7477 · ampproject/amp-wp

thelovekesh · 2023-03-01T11:05:36Z

Summary

Add treatPhpDocTypesAsCertain: false to PHPStan config so that PHPStan doesn't take doc types as certain.

Checklist

My code is tested and passes existing tests.
My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

- This will stop PHPStan to consider docblock type defination as certain

CLAassistant · 2023-03-01T11:09:58Z

All committers have signed the CLA.

github-actions · 2023-03-01T11:16:07Z

Plugin builds for 140aeb7 are ready 🛎️!

Download development build
Download production build

thelovekesh · 2023-03-01T16:41:52Z

@westonruter It seems there are a few bugs in the latest PHPStan version so I have started a discussion which later converted into an issue phpstan/phpstan#8980.

…ansientCaching::process()`

…::extract_by_filename_or_filesystem()`

…t_instance()` - On AMP Support page it will create a new instance of `WP_Site_Health` anyway. So better to remove dependency on `get_instance()` method.

…t_errors`

…rl` in `AMP_Validated_URL_Post_Type::print_admin_notice()`

…AMP_Validation_Error_Taxonomy::handle_validation_error_update()`

thelovekesh · 2023-03-01T16:55:11Z

src/Support/SupportData.php

 		if ( class_exists( 'WP_Site_Health' ) ) {
-			$site_health     = method_exists( 'WP_Site_Health', 'get_instance' ) ? WP_Site_Health::get_instance() : new WP_Site_Health();
-			$loopback_status = $site_health->can_perform_loopback();
+			$loopback_status = ( new WP_Site_Health() )->can_perform_loopback();


Even though this is wrongly flagged by PHPStan as an error but I think it's in better shape now. On the support page, there will be no instance of WP_Site_Health and it will be freshly instantiated anyway.

So IMO it's better to instantiate and read the required data since we will not be using this class instance anywhere else.

Yes, this makes sense. And WP_Site_Health::get_instance() is the singleton static getter, so it'll instantiate it if it doesn't already exist, and only one copy will ever only ever get constructed.

Nevermind, my comment is saying the opposite.

The WP_Site_Health::get_instance() method was introduced in WordPress 5.4 via WordPress/wordpress-develop@637b6f5. So that's why we have the method_exists check.

The only potential problem here is that the constructor for the class has side effects. But as you say, this class wouldn't have been instantiated on the support page anyway.

westonruter · 2023-03-01T22:22:53Z

includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php


 		// Remove element if it has illegal CDATA.
-		if ( ! empty( $cdata ) && $node instanceof DOMElement ) {
+		if ( ! empty( $cdata ) ) {


Makes sense since we already know it is a DOMElement.

westonruter · 2023-03-01T22:23:37Z

includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php


 		// Check if element needs amp-bind component.
-		if ( $node instanceof DOMElement && ! in_array( 'amp-bind', $this->script_components, true ) ) {
+		if ( ! in_array( 'amp-bind', $this->script_components, true ) ) {


westonruter · 2023-03-01T22:23:41Z

includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php

 		// Ensure attributes match; if not move up to the next node.
 		foreach ( $parsed_spec_name['attributes'] as $attr_name => $attr_value ) {
-			if ( $node instanceof DOMElement && strtolower( $node->getAttribute( $attr_name ) ) !== $attr_value ) {
+			if ( strtolower( $node->getAttribute( $attr_name ) ) !== $attr_value ) {


westonruter · 2023-03-01T22:24:51Z

includes/validation/class-amp-validation-error-taxonomy.php

 		$updated_count = 0;
 		foreach ( $term_ids as $term_id ) {
-			if ( 'delete' === $action && self::delete_empty_term( $term_id ) ) {
+			if ( self::delete_empty_term( $term_id ) ) {


Good since the function bails above already if 'delete' !== $action

westonruter · 2023-03-01T22:24:58Z

includes/validation/class-amp-validation-error-taxonomy.php


 		$term_ids_count = count( $term_ids );
-		if ( 'edit.php' === $pagenow && 'delete' === $action && 1 === $updated_count ) {
+		if ( 'edit.php' === $pagenow && 1 === $updated_count ) {


Good since the function bails above already if 'delete' !== $action

westonruter · 2023-03-01T22:27:16Z

src/Support/SupportData.php

 		if ( class_exists( 'WP_Site_Health' ) ) {
-			$site_health     = method_exists( 'WP_Site_Health', 'get_instance' ) ? WP_Site_Health::get_instance() : new WP_Site_Health();
-			$loopback_status = $site_health->can_perform_loopback();
+			$loopback_status = ( new WP_Site_Health() )->can_perform_loopback();


Yes, this makes sense. And WP_Site_Health::get_instance() is the singleton static getter, so it'll instantiate it if it doesn't already exist, and only one copy will ever only ever get constructed.

westonruter · 2023-03-01T22:28:44Z

includes/amp-helper-functions.php

 		}
 	} elseif ( ! (
 		$queried_object instanceof WP_Post &&
-		$wp_query instanceof WP_Query &&


Redundant indeed since above:

if ( ! $wp_query instanceof WP_Query || ! did_action( 'wp' ) ) { return;

westonruter · 2023-03-01T22:30:13Z

includes/utils/class-amp-image-dimension-extractor.php

 			}

-			if ( ( empty( $image_size ) || ! is_array( $image_size ) ) && file_exists( $image_file ) ) {
+			if ( ( empty( $image_size ) ) && file_exists( $image_file ) ) {


Redundant indeed since we check if it is an array above. Nevertheless, this can be further cleaned up a tiny bit more to remove the extra parentheses:

Suggested change

if ( ( empty( $image_size ) ) && file_exists( $image_file ) ) {

if ( empty( $image_size ) && file_exists( $image_file ) ) {

westonruter · 2023-03-01T22:31:31Z

includes/validation/class-amp-validated-url-post-type.php

 		// Add notice for PHP fatal error during validation request.
 		$post = get_post();
-		if ( $post instanceof WP_Post && 'post' === get_current_screen()->base && self::POST_TYPE_SLUG === get_current_screen()->post_type ) {
+		if ( $post instanceof WP_Post && 'post' === get_current_screen()->base ) {


Indeed redundant since above:

if ( ! get_current_screen() || self::POST_TYPE_SLUG !== get_current_screen()->post_type ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended return; }

westonruter · 2023-03-01T22:34:46Z

src/Support/SupportData.php

-			if ( ! empty( $error_source_slugs ) && is_array( $error_source_slugs ) ) {
+			if ( ! empty( $error_source_slugs ) ) {


In PHP<8.0, array_values() can return null if it is not passed an array. Nevertheless, we have ensured above that the $error_sources input is an array.

westonruter · 2023-03-01T22:38:04Z

It seems there are a few bugs in the latest PHPStan version so I have started a discussion which later converted into an issue

This is related to these failures, correct?

Co-authored-by: Weston Ruter <westonruter@google.com>

thelovekesh · 2023-03-02T03:59:19Z

This is related to these failures, correct?

Yes. Since we are very specific about these cases we require them as it is, what about ignoring PHPStan on these lines?

…n phpstan baseline instead

…tan-ignore-next-line` comments

thelovekesh · 2023-03-02T05:27:30Z

@westonruter I have made a few changes to how we ignore the PHPStan errors, Previously we were using @phpstan-ignore-next-line comments which are hard to keep track of, and even remove them when things get fixed upstream.

I have added a new phpstan-baseline.php file which will keep track of ignoring required errors with todo. Or we can say a single source to deal with PHPStan errors.

westonruter · 2023-03-02T19:40:41Z

The inclusion of phpstan-baseline.php should eliminate the need for ignoreErrors in the phpstan.neon.dist, correct?

amp-wp/phpstan.neon.dist

Lines 33 to 35 in 963082a

    
           ignoreErrors: 
        
           	# Setting src to false indicates a bundle of dependencies. 
        
           	- '#^Property _WP_Dependency::\$src \(string\) does not accept false\.$#'

Should that be moved to phpstan-baseline.php as well?

phpstan-baseline.php

Co-authored-by: Weston Ruter <westonruter@google.com>

westonruter · 2023-03-02T19:48:44Z

Alternatively, I found that if I run:

phpstan analyse --generate-baseline

It will generate a phpstan-baseline.neon file. The output can then be put straight into the ignoreErrors section of phpstan.neo.dist.

Is there a specific reason to use a PHP file for this?

thelovekesh · 2023-03-02T19:57:46Z

Yes, but I am not sure if we should use the main config file for this as changes can be there as PHPStan introduces new rules.

I choose a PHP file for better readability and comments due to better syntax highlighting.

thelovekesh · 2023-03-20T11:56:53Z

QA Passed ✅

Up until 4337065, the base branch had no PHPStan issues.

Add treatPhpDocTypesAsCertain: false config

ef5233c

- This will stop PHPStan to consider docblock type defination as certain

thelovekesh added 8 commits March 1, 2023 22:14

Remove obsolate phpstan-ignore-next-line comment from `MonitorCssTr…

95bfb25

…ansientCaching::process()`

Remove additional instanceOf DOMElement checks

691b10a

Remove redundant instanceof WP_Query check from amp_is_available()

a695b8b

Remove redundant is_array check from `AMP_Image_Dimension_Extractor…

67888ae

…::extract_by_filename_or_filesystem()`

Refactor to use new WP_Site_Health() instead of `WP_Site_Health::ge…

aa54201

…t_instance()` - On AMP Support page it will create a new instance of `WP_Site_Health` anyway. So better to remove dependency on `get_instance()` method.

Remove additional is_array check from `SupportData::process_raw_pos…

3562537

…t_errors`

Remove additional check to test if current screen is `amp_validated_u…

451d982

…rl` in `AMP_Validated_URL_Post_Type::print_admin_notice()`

Remove redundant checks for validating if $action === 'delete' in `…

983147b

…AMP_Validation_Error_Taxonomy::handle_validation_error_update()`

thelovekesh force-pushed the fix/phpstan branch from 8f3820d to 983147b Compare March 1, 2023 16:49

thelovekesh commented Mar 1, 2023

View reviewed changes

westonruter added this to the v2.4.1 milestone Mar 1, 2023

westonruter reviewed Mar 1, 2023

View reviewed changes

Remove extra parenthesis from if conditional

2324efa

Co-authored-by: Weston Ruter <westonruter@google.com>

thelovekesh added 5 commits March 2, 2023 10:51

Add temp-baseline.php to be git ignored

368eedc

Remove @phpstan-ignore-next-line from code files and include them i…

1acfe9b

…n phpstan baseline instead

Remove @phpstan-ignore-next-line to be appended by amphtml-update.py

64eec4b

Add phpstan-baseline.php to handle PHPStan errors instead of `@phps…

df9202c

…tan-ignore-next-line` comments

Add phpstan-baseline.php as baseline

cfbc3de

Fix linting of phpstan-baseline.php

a5199d0

thelovekesh requested a review from westonruter March 2, 2023 05:29

Update phpstan-baseline.php file doc comment

963082a

westonruter reviewed Mar 2, 2023

View reviewed changes

phpstan-baseline.php Show resolved Hide resolved

westonruter reviewed Mar 2, 2023

View reviewed changes

phpstan-baseline.php Outdated Show resolved Hide resolved

thelovekesh and others added 2 commits March 3, 2023 01:13

Add @codeCoverageIgnore to phpstan-baseline.php

2e2f6a8

Co-authored-by: Weston Ruter <westonruter@google.com>

Fix command to generate PHPStan baseline

83a0e30

Co-authored-by: Weston Ruter <westonruter@google.com>

Move ignoreErrors from phpstan.neon.dist to phpstan-baseline.php

140aeb7

westonruter approved these changes Mar 2, 2023

View reviewed changes

westonruter merged commit b51c19f into develop Mar 2, 2023

westonruter deleted the fix/phpstan branch March 2, 2023 20:27

thelovekesh assigned westonruter and thelovekesh and unassigned westonruter and thelovekesh Mar 17, 2023

westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Mar 22, 2023

	if ( ( empty( $image_size ) ) && file_exists( $image_file ) ) {
	if ( empty( $image_size ) && file_exists( $image_file ) ) {

		if ( ! empty( $error_source_slugs ) && is_array( $error_source_slugs ) ) {
		if ( ! empty( $error_source_slugs ) ) {

Conversation

thelovekesh commented Mar 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

CLAassistant commented Mar 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Mar 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thelovekesh commented Mar 1, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

westonruter commented Mar 1, 2023

Uh oh!

thelovekesh commented Mar 2, 2023

Uh oh!

thelovekesh commented Mar 2, 2023

Uh oh!

westonruter commented Mar 2, 2023

Uh oh!

Uh oh!

Uh oh!

westonruter commented Mar 2, 2023

Uh oh!

thelovekesh commented Mar 2, 2023

Uh oh!

thelovekesh commented Mar 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

thelovekesh commented Mar 1, 2023 •

edited

Loading

CLAassistant commented Mar 1, 2023 •

edited

Loading

github-actions bot commented Mar 1, 2023 •

edited

Loading