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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 7 commits into from
Oct 25, 2023
Merged

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

merged 7 commits into from
Oct 25, 2023

Conversation

thelovekesh
Copy link
Collaborator

@thelovekesh thelovekesh commented Oct 25, 2023 •
edited
Loading

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
Copy link
Contributor

github-actions bot commented Oct 25, 2023 •
edited
Loading

Plugin builds for 57d4c0b are ready 🛎️!

@thelovekesh 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 westonruter added this to the v2.4.3 milestone Oct 25, 2023
thelovekesh
thelovekesh commented Oct 25, 2023
View reviewed changes
src/Admin/SiteHealth.php Outdated
Comment on lines 357 to 372
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 );
Copy link
Collaborator Author

@thelovekesh thelovekesh Oct 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

@thelovekesh thelovekesh force-pushed the remove/page-cache-site-health-test branch from 737d1da to 3c3ad0d Compare October 25, 2023 15:56
westonruter
westonruter reviewed Oct 25, 2023
View reviewed changes
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.

Very thorough considerations here!

src/Admin/SiteHealth.php Outdated Show resolved Hide resolved
src/Admin/SiteHealth.php Outdated
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. */
Copy link
Member

Choose a reason for hiding this comment

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

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.
*/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

@thelovekesh @westonruter
Fix hook callback
57d4c0b 
Co-authored-by: Weston Ruter <[email protected]>
@westonruter westonruter merged commit c1332cc into develop Oct 25, 2023
31 of 33 checks passed
@westonruter westonruter deleted the remove/page-cache-site-health-test branch October 25, 2023 17:22
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@westonruter westonruter westonruter approved these changes

Assignees
No one assigned
Labels
Changelogged Whether the issue/PR has been added to release notes.
Projects
None yet
Milestone
v2.5.0
Development

Successfully merging this pull request may close these issues.

Omit page caching and object caching tests for Site Health if on WP>=6.1
2 participants
@thelovekesh @westonruter