Omit page caching and object caching tests for Site Health if on WP>=6.1 by thelovekesh · Pull Request #7645 · ampproject/amp-wp

thelovekesh · 2023-10-25T09:02:31Z

Summary

Fixes #7643

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).

github-actions · 2023-10-25T09:07:04Z

Plugin builds for 57d4c0b are ready 🛎️!

Download development build
Download production build

thelovekesh · 2023-10-25T15:54:49Z

src/Admin/SiteHealth.php

+		if ( version_compare( get_bloginfo( 'version' ), '6.1', '>=' ) ) {
+			/** @deprecated 2.4.3 Use `site_status_good_response_time_threshold` instead. */
+			return (int) apply_filters_deprecated(
+				'amp_page_cache_good_response_time_threshold',
+				[ $threshold ],
+				'2.4.3',
+				'site_status_good_response_time_threshold'
+			);
+		} else {
+			/**
+			 * Filters the threshold below which a response time is considered good.
+			 *
+			 * @since 2.2.1
+			 * @param int $threshold Threshold in milliseconds.
+			 */
+			return (int) apply_filters( 'amp_page_cache_good_response_time_threshold', 600 );


Not sure if this is the best approach to deprecate amp_page_cache_good_response_time_threshold but we need to deprecate it for WP >= 6.1.

…WP < 6.1

… WP 6.1+

westonruter

Very thorough considerations here!

src/Admin/SiteHealth.php

westonruter · 2023-10-25T16:05:45Z

src/Admin/SiteHealth.php

-		return (int) apply_filters( 'amp_page_cache_good_response_time_threshold', 600 );
+	public function get_good_response_time_threshold( $threshold = 600 ) {
+		if ( version_compare( get_bloginfo( 'version' ), '6.1', '>=' ) ) {
+			/** @deprecated 2.4.3 Use `site_status_good_response_time_threshold` instead. */


I think we could just add the @deprecated tag to the phpdoc below, and this line can be replaced with just:

Suggested change

/** @deprecated 2.4.3 Use `site_status_good_response_time_threshold` instead. */

/** This filter is documented in src/Admin/SiteHealth.php */

See:

amp-wp/includes/admin/functions.php

Line 105 in d9727b6

/** This filter is documented in includes/settings/class-amp-customizer-design-settings.php */

amp-wp/includes/amp-helper-functions.php

Lines 706 to 718 in d9727b6

/**

* Filters whether to show the amphtml link on the frontend.

*

* This is deprecated since the name was wrong and the use case is not clear. To remove this from being printed,

* instead of using the filter you can rather do:

*

* add_action( 'template_redirect', static function () {

* remove_action( 'wp_head', 'amp_add_amphtml_link' );

* } );

*

* @since 0.2

* @deprecated Remove amp_add_amphtml_link() call on wp_head action instead.

*/

Could this deprecation tag lead to confusion if the user is on WP < 6.1? Because in that case site_status_good_response_time_threshold won't be available.

We don't want people to use it anymore though, as they should be updating WordPress to a current version. In any case, they won't get the deprecated notice because of the if check.

In any case, I highly doubt this filter is being used by many sites. So I don't think there will be any confusion as nobody will even notice the tag.

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

src/Admin/SiteHealth.php

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

thelovekesh force-pushed the remove/page-cache-site-health-test branch 2 times, most recently from 9c3d0a5 to 737d1da Compare October 25, 2023 15:52

westonruter added this to the v2.4.3 milestone Oct 25, 2023

thelovekesh commented Oct 25, 2023

View reviewed changes

thelovekesh requested a review from westonruter October 25, 2023 15:55

thelovekesh added 4 commits October 25, 2023 21:26

Update SiteHealth service to disable page and object cache tests for …

f5d9af7

…WP < 6.1

Deprecate amp_page_cache_good_response_time_threshold hook

8a157fe

Only deprecate amp_page_cache_good_response_time_threshold hook for…

ee43d6a

… WP 6.1+

Update test cases for SiteHealth service

3c3ad0d

thelovekesh force-pushed the remove/page-cache-site-health-test branch from 737d1da to 3c3ad0d Compare October 25, 2023 15:56

westonruter reviewed Oct 25, 2023

View reviewed changes

thelovekesh and others added 2 commits October 25, 2023 21:38

Add plugin name with version in deprecated filter notice

340710b

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

Update docblock for amp_page_cache_good_response_time_threshold hook

b18e290

westonruter reviewed Oct 25, 2023

View reviewed changes

src/Admin/SiteHealth.php Outdated Show resolved Hide resolved

Fix hook callback

57d4c0b

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

westonruter approved these changes Oct 25, 2023

View reviewed changes

westonruter merged commit c1332cc into develop Oct 25, 2023

westonruter deleted the remove/page-cache-site-health-test branch October 25, 2023 17:22

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Omit page caching and object caching tests for Site Health if on WP>=6.1#7645

Omit page caching and object caching tests for Site Health if on WP>=6.1#7645
westonruter merged 7 commits intodevelopfrom
remove/page-cache-site-health-test

thelovekesh commented Oct 25, 2023 •

edited

Loading

Uh oh!

github-actions bot commented Oct 25, 2023 •

edited

Loading

Uh oh!

thelovekesh Oct 25, 2023 •

edited

Loading

Uh oh!

westonruter left a comment

Uh oh!

Uh oh!

westonruter Oct 25, 2023

Uh oh!

thelovekesh Oct 25, 2023

Uh oh!

westonruter Oct 25, 2023

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	/** @deprecated 2.4.3 Use `site_status_good_response_time_threshold` instead. */
	/** This filter is documented in src/Admin/SiteHealth.php */

	/**
	* Filters whether to show the amphtml link on the frontend.
	*
	* This is deprecated since the name was wrong and the use case is not clear. To remove this from being printed,
	* instead of using the filter you can rather do:
	*
	* add_action( 'template_redirect', static function () {
	* remove_action( 'wp_head', 'amp_add_amphtml_link' );
	* } );
	*
	* @since 0.2
	* @deprecated Remove amp_add_amphtml_link() call on wp_head action instead.
	*/

Conversation

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

Uh oh!

Summary

Checklist

Uh oh!

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

Uh oh!

Uh oh!

thelovekesh Oct 25, 2023 • 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!

westonruter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

westonruter Oct 25, 2023

Choose a reason for hiding this comment

Uh oh!

thelovekesh Oct 25, 2023

Choose a reason for hiding this comment

Uh oh!

westonruter Oct 25, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

thelovekesh commented Oct 25, 2023 •

edited

Loading

github-actions bot commented Oct 25, 2023 •

edited

Loading

thelovekesh Oct 25, 2023 •

edited

Loading