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.

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

Detect available object caching and show recommended plugin in admin dashboard. #6493

Merged
merged 13 commits into from
Sep 10, 2021

Conversation

dhaval-parekh
Copy link
Collaborator

@dhaval-parekh dhaval-parekh commented Jul 28, 2021

Summary

Fixes #5780

image

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

@dhaval-parekh dhaval-parekh self-assigned this Aug 3, 2021
@dhaval-parekh dhaval-parekh marked this pull request as ready for review August 5, 2021 12:47
@github-actions
Copy link
Contributor

github-actions bot commented Aug 5, 2021

Plugin builds for f198357 are ready 🛎️!

@dhaval-parekh dhaval-parekh removed the request for review from westonruter August 10, 2021 07:28
Copy link
Contributor

@pierlon pierlon left a comment

Choose a reason for hiding this comment

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

I made a few comments but I like where this is heading 😄

src/Admin/SiteHealth.php Outdated Show resolved Hide resolved
/*
* Try to connect redis server.
*/
$redis_server = [
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that W3 Total Cache only checks if the Redis class exists.

Any reason to go further and attempt to ping the Redis server? It seems like a brittle way of determining if there is indeed a Redis server as there may be many things that could cause it not to respond such as a firewall or iptables rule, for instance. It may also come off as hostile to some persons as it's attempting to make a connection that the user did not deliberately agree to.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@pierlon As I understood, We do need to check services with connecting instead of checking if a class of extension exists. And if some services are not able to connect then it's better not to recommend anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, but attempting to connect and fail would do more harm than good here and is more likely to produce more false positives. We should be only checking if a user's environment is capable of using Redis cache, not if we can connect to a Redis server.

*/
public function is_site_has_memcached() {

if ( ! ( class_exists( 'Memcache' ) || class_exists( 'Memcached' ) ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

W3 Total Cache also checks for these

return false;
}

$has_object_cache = false;
Copy link
Contributor

@pierlon pierlon Aug 17, 2021

Choose a reason for hiding this comment

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

Similar to my comment above, I don't think it our place to attempt to connect to an external service. Checking if the Memcache or Memcached class exists seems to be a good enough heuristic measure.

src/Admin/SiteHealth.php Outdated Show resolved Hide resolved
$available_cache = $this->check_available_cache();

/* translators: plugin recommendation markup */
$plugin_placeholder = _x( 'We recommend to use %s plugin.', 'plugin recommendation markup', 'amp' );
Copy link
Contributor

Choose a reason for hiding this comment

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

We may want to not recommend any specific plugin, but instead point them to documentation that may guide them how to configure object caching for their site. As mentioned in the issue by @westonruter:

Instead of mentioning the specific plugins in the Site Health info recommendation, the list of backend-specific plugins can be in a documentation page on amp-wp.org. This documentation page can pull together the various pieces of information about object caching spread out across WordPress.org, including:

Ultimately, the user should ask their host for the recommended way to enable object caching first before they do it themselves. So our recommendation should be to first contact the host, and then continue with our guidance to try the plugins we've identified which may work with their site.

See support topics about object caching:

Anything we can do to make it easier for users to enable object caching the better.

Copy link
Member

Choose a reason for hiding this comment

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

What about linking off to search results for object caching plugins using a given backend? For example:

And then we can include with that a link to our object caching guide so they can see which plugins specifically we've tested: https://amp-wp.org/documentation/getting-started/amp-site-setup/persistent-object-caching/#i-need-help-setting-up-my-persistent-object-cache

At the very lest we can just say: “It looks like you have X available on your server.” where X is the backend service and then they will provide them with a starting point for which plugins to look for.

@dhaval-parekh dhaval-parekh force-pushed the performance/5780-object-cache-guide branch 2 times, most recently from fd132ed to 271e75c Compare August 20, 2021 09:02
@dhaval-parekh dhaval-parekh force-pushed the performance/5780-object-cache-guide branch from 271e75c to ceca448 Compare August 26, 2021 06:04
Copy link
Contributor

@pierlon pierlon left a comment

Choose a reason for hiding this comment

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

Apart from attempting to externally connect to Redis and Memcache server instances, this looks OK.

@dhaval-parekh dhaval-parekh force-pushed the performance/5780-object-cache-guide branch from ceca448 to 852a5a4 Compare September 6, 2021 08:24
@westonruter westonruter added this to the v2.2 milestone Sep 10, 2021
…ce/5780-object-cache-guide

* 'develop' of github.com:ampproject/amp-wp: (43 commits)
  Make placeholder dependency on video ID more explicit
  Fix adding start to placeholder link; improve coverage
  Let placeholder link point to YouTube permalink and not iframe src
  Prevent generating closing IMG tags
  Use coversDefaultClass
  Remove condition which cannot be reached since video ID is always provided
  Opt to pass-through iframe for unrecognized YouTube URL rather than convert to link
  Remove unreachable code
  Give more explicit array phpdoc type
  Fix issues identified by phpstan
  Try installing phpstan via shivammathur/setup-php
  Try downloading a specific version of phpstan instead of latest
  Fix yml syntax error
  Try removing phpstan before composer install
  Print phpstan version when running on GHA
  Revert "Revert phpstan ignores"
  Try adding phpstan version to composer cache key
  Revert phpstan ignores
  update unit test cases
  Sanitize attribute name Use original YouTube URL as placeholder when sanitizing raw embed Fix phpcs alignment warning Use constant instead of member variable Let Document::fromNode() obtain the ownerDocument Deduplicate iframe_props into constant
  ...
@westonruter
Copy link
Member

@dhaval-parekh I've made some improvements to the test. It now shows multiple persistent object caching services when they are available and then links to the Add Plugins screen in the admin:

image

Could you please add some test coverage?

@westonruter
Copy link
Member

(Without rebasing please 😊)

@westonruter westonruter merged commit f819294 into develop Sep 10, 2021
@westonruter westonruter deleted the performance/5780-object-cache-guide branch September 10, 2021 16:13
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Detect available object caching backend(s) to guide users toward persistent object caching plugins
3 participants