Integrate with Site Health by kienstra · Pull Request #3886 · ampproject/amp-wp

kienstra · 2019-12-05T17:27:22Z

Summary

Integrates with Site Health in the 'Status' and 'Info' tabs.

Status tab

Info (debug) tab

Fixes #2199

Checklist

My pull request is addressing an open issue (please create one otherwise).
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).

Adds tests for persistent object cache and curl_multi_* functions.

Including the mode and the templates used. Also, add 'pcl' to the required extensions via a filter.

In the debugging section, add the content types, like 'post' and 'page'.

kienstra · 2019-12-05T17:33:14Z

I still need to consider #2199 (comment) and #2199 (comment).

westonruter · 2019-12-08T23:55:30Z

src/Admin/SiteHealth.php

+		return array_merge(
+			$extensions,
+			[
+				'spl' => [
+					'extension' => 'spl',
+					'function'  => 'spl_autoload_register',
+					'required'  => true,
+				],
+			]
+		);


Since spl is required, the AMP plugin won't even initialize without it, right?

amp-wp/amp.php

Lines 72 to 74 in e6f9dc3

'spl' => array(

'functions' => array( 'spl_autoload_register' ),

),

So is spl relevant here? The test would never fail.

What is relevant are tests for the suggest packages:

amp-wp/composer.json

Lines 29 to 34 in e6f9dc3

"suggest": {

"ext-intl": "Enables use of idn_to_utf8() to convert punycode domains to UTF-8 for use with an AMP Cache.",

"ext-json": "Provides native implementation of json_encode()/json_decode().",

"ext-mbstring": "Used by PHP-CSS-Parser when working with stylesheets.",

"ext-zip": "Enables the use of ZipArchive to export AMP Stories."

},

Thanks, that's a good point. Tomorrow might be the earliest I could make that change, if that's alright.

src/Admin/SiteHealth.php

This seems to be left over from something earlier.

As Weston pointed out, if the spl extension isn't present, the plugin won't even load. So check for the modules in the suggest value of composer.json: https://github.com/ampproject/amp-wp/blob/e6f9dc3587954e40c0c21bc66ccfa88f99aaa1b3/composer.json#L29-L34

Remove post type support for all posts that aren't in the dataProvider.

Simply add the post types to an array, instead of repeating the function.

It shouldn't be necessary to escape simple strings, also update DocBlocks.

It might instead check for an error on running idn_to_utf8(), like: Deprecated: idn_to_utf8(): INTL_IDNA_VARIANT_2003 is deprecated in /home/tipsonlifeandlov/public_html/wp-content/plugins/amp/includes/class-amp-http.php on line 208

westonruter · 2019-12-17T22:42:09Z

tests/php/test-class-site-health.php

+			$this->instance->persistent_object_cache()
+		);
+
+		$GLOBALS['_wp_using_ext_object_cache'] = true;


This should be unset in tearDown, yeah?

Ah, right. 8d19165 does that.

westonruter · 2019-12-17T22:45:58Z

src/Admin/SiteHealth.php

+			],
+			'description' => esc_html__( 'The AMP plugin performs at its best when persistent object cache is enabled.', 'amp' ),
+			'actions'     => '',
+			'test'        => 'persistent_object_cache',


Let's prefix all tests with amp_

Good idea, 60970ce does that.

westonruter · 2019-12-17T22:47:11Z

src/Admin/SiteHealth.php

+				'color' => $is_proper_version ? 'green' : 'orange',
+			],
+			'description' => sprintf(
+				esc_html__( 'The version of ICU can affect how the intl extension runs. The minimum recommended version of ICU is %s.', 'amp' ),


Missing /* translators: ... */ comment in the preceding line.

Good point, 3e95814 adds the translators comment.

westonruter · 2019-12-17T22:51:34Z

includes/admin/functions.php

+function amp_bootstrap_admin() {
 	$admin_pointers = new AMP_Admin_Pointers();
 	$admin_pointers->init();
+
+	$post_meta_box = new AMP_Post_Meta_Box();
+	$post_meta_box->init();
+
+	$site_health = new SiteHealth();
+	$site_health->init();
 }


General note: it seems somewhat strange that these instances are completely inaccessible since the variables are scoped in the function. This is probably a wider issue that should be addressed later, as it's worse to pollute the global namespace with more variables. /cc @schlessera

Yeah, that's a good point. It'd be good to have some access to them.

This led to a failed Travis build, from a PHPCS error.

Thanks to Weston's suggesiton, this should help prevent conflicts with other plugins' tests.

As Weston brought up, there's a need to unset this.

Thanks to Brandon Kraft's comment, it looks like this version has teh INTL_IDNA_VARIANTE_UTS46 constant: #1439 (comment) And as long as that constant is defined, it will be passed to idn_to_utf8(): https://github.com/ampproject/amp-wp/blob/39336b78be7a4ba8b3a09a6f43d9f66e3c201e05/includes/class-amp-http.php#L233 ...and that won't trigger this deprecation warning: https://github.com/php/php-src/pull/2355/files#diff-24931a05f9e656989a55c44cf8a72f9eR294

kienstra · 2019-12-18T20:26:55Z

src/Admin/SiteHealth.php

+	 */
+	public function icu_version() {
+		$icu_version       = defined( 'INTL_ICU_VERSION' ) ? (float) INTL_ICU_VERSION : null;
+		$minimum_version   = 4.6;


Thanks to Brandon Kraft's comment, it looks like ICU version 4.6 introduced the INTL_IDNA_VARIANTE_UTS46 constant.

And as long as that constant is defined, it will be passed to idn_to_utf8():

amp-wp/includes/class-amp-http.php

Line 233 in 39336b7

$domain = idn_to_utf8( $domain, IDNA_DEFAULT, defined( 'INTL_IDNA_VARIANT_UTS46' ) ? INTL_IDNA_VARIANT_UTS46 : INTL_IDNA_VARIANT_2003 );

...and that won't trigger the deprecation warning mentioned here.

Also, I tested this locally with PHP 7.2:

$ wp shell $ wp> idn_to_utf8( 'foobar', IDNA_DEFAULT, INTL_IDNA_VARIANT_UTS46 ); => string(6) "foobar" # As expected, no deprecation warning $ wp> idn_to_utf8( 'foobar', IDNA_DEFAULT, INTL_IDNA_VARIANT_2003 ); PHP Deprecated: idn_to_utf8(): INTL_IDNA_VARIANT_2003 is deprecated in phar:///usr/local/bin/wp/vendor/wp-cli/shell-command/src/WP_CLI/Shell/REPL.php(46) : eval()'d code on line 1 PHP Stack trace: PHP 1. {main}() /usr/local/bin/wp:0 ....

So I think the minimum ICU version of 4.6 should be good.

If this is in Reader mode, it doesn't apply, so explain that.

Similar to how the other tests here work, so users can always find out more about it, with the link.

This is how the rest of the sprintf() calls in this PR are when they only have one interoplated string.

There is probably a more complex way to make this work, but %s seems simpler.

This is in the new src/ directory, under a namespace.

As Alain mentioned, it's easy to later change from final to non-final.

It looks like this needs to have a language in it, so pass it to a translation function.

I should have updated this earlier for the new URL for the curl documentation.

kienstra · 2019-12-19T17:28:20Z

src/Admin/SiteHealth.php

+
+		// Add the supported templates, like 'is_author', if not in 'Reader' mode.
+		if ( AMP_Theme_Support::READER_MODE_SLUG !== AMP_Theme_Support::get_support_mode() ) {
+			$supported_templates = array_merge(


These are not translated, as this is debugging information that we might get on a WordPress.org support topic. It could be confusing to see these in different languages.

kienstra · 2019-12-19T17:29:22Z

Ready For Another Round Of Review When Available

Hi @westonruter,
See you in a bit 😄

When you have a chance, this should be ready for another round of review.

In deference to other plugins, as there are other AMP plugins.

kienstra added 3 commits November 26, 2019 22:12

Beging the Site Health integration

c7cabc5

Adds tests for persistent object cache and curl_multi_* functions.

Add debugging information for the AMP plugin

92a04d2

Including the mode and the templates used. Also, add 'pcl' to the required extensions via a filter.

Add the content types to the 'Templates enabled' information

8d7fd3f

In the debugging section, add the content types, like 'post' and 'page'.

googlebot added the cla: yes Signed the Google CLA label Dec 5, 2019

westonruter reviewed Dec 8, 2019

View reviewed changes

kienstra added 9 commits December 9, 2019 11:33

Remove unnecessary add_theme_support() call

b943f7e

This seems to be left over from something earlier.

Merge branch 'develop' into add/2199-site-health

990ef7e

Address failed PHPUnit tests

bca3dc1

Remove post type support for all posts that aren't in the dataProvider.

Use a for loop instead of 3 calls of the same function

2e459d6

Simply add the post types to an array, instead of repeating the function.

Remove esc_url() wrappers from plain string URLs, update DocBlocks

2a8c88b

It shouldn't be necessary to escape simple strings, also update DocBlocks.

Merge branch 'develop' into add/2199-site-health

54745fb

Merge branch 'develop' into add/2199-site-health

08e5ec1

westonruter requested changes Dec 17, 2019

View reviewed changes

kienstra added 6 commits December 18, 2019 00:24

Add a translators comment above an esc_html__() call

3e95814

This led to a failed Travis build, from a PHPCS error.

Prefix the Site Health tests with 'amp_'

60970ce

Thanks to Weston's suggesiton, this should help prevent conflicts with other plugins' tests.

Unset global value in tearDown() method

8d19165

As Weston brought up, there's a need to unset this.

Merge branch 'develop' into add/2199-site-health

98db052

Merge branch 'develop' into add/2199-site-health

c06c68c

kienstra commented Dec 18, 2019

View reviewed changes

kienstra added 7 commits December 18, 2019 15:02

Add debugging information for 'Serve all templates as AMP?'

27f7372

If this is in Reader mode, it doesn't apply, so explain that.

Always display the extra information about persistent object caching

54783a6

Similar to how the other tests here work, so users can always find out more about it, with the link.

Change %s to %d for a float, and change %1$d to %d

097c57d

This is how the rest of the sprintf() calls in this PR are when they only have one interoplated string.

In sprintf(), change %d back to %s, as %s does not allow multiple digits

4febeb6

There is probably a more complex way to make this work, but %s seems simpler.

Update the package from AMP to Amp\AmpWP

8c160d5

This is in the new src/ directory, under a namespace.

Make the SiteHealth class final

57ed521

As Alain mentioned, it's easy to later change from final to non-final.

Correct the link for the curl documentation

cf97965

It looks like this needs to have a language in it, so pass it to a translation function.

Correct failed unit test: test_curl_multi_functions()

8184e80

I should have updated this earlier for the new URL for the curl documentation.

kienstra changed the title ~~[WIP] Integrate with Site Health~~ Integrate with Site Health Dec 19, 2019

kienstra commented Dec 19, 2019

View reviewed changes

kienstra and others added 7 commits December 19, 2019 11:38

Merge branch 'develop' into add/2199-site-health

5c8d4bb

Change debug information key from 'amp' to 'amp_wp'

74e0326

In deference to other plugins, as there are other AMP plugins.

Represent versions as strings; improve escaping

9fc127b

Remove periods from test labels

d0e0a20

Improve cURL multi messages

c3dde83

Improve translated strings: de-duplicate and simplify

af21727

Make tests more DRY by not copying translation strings

d1c4d05

westonruter approved these changes Dec 19, 2019

View reviewed changes

westonruter added this to the v1.5 milestone Dec 19, 2019

westonruter merged commit 22056fb into develop Dec 19, 2019

westonruter deleted the add/2199-site-health branch December 19, 2019 23:54

westonruter added the Changelogged label Mar 23, 2020

	'spl' => array(
	'functions' => array( 'spl_autoload_register' ),
	),

	"suggest": {
	"ext-intl": "Enables use of idn_to_utf8() to convert punycode domains to UTF-8 for use with an AMP Cache.",
	"ext-json": "Provides native implementation of json_encode()/json_decode().",
	"ext-mbstring": "Used by PHP-CSS-Parser when working with stylesheets.",
	"ext-zip": "Enables the use of ZipArchive to export AMP Stories."
	},

Conversation

kienstra commented Dec 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Status tab

Info (debug) tab

Checklist

Uh oh!

kienstra commented Dec 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

kienstra Dec 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kienstra Dec 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kienstra Dec 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

kienstra commented Dec 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kienstra commented Dec 5, 2019 •

edited

Loading

kienstra Dec 18, 2019 •

edited

Loading

kienstra Dec 18, 2019 •

edited

Loading

kienstra Dec 18, 2019 •

edited

Loading