Cache error index counts and URL count in "At a Glance" widget #5343

johnwatkins0 · 2020-09-06T22:35:53Z

Summary

This PR adds transient-based caching to the get_validation_error_count function. The transient data is an associative array with the JSON-encoded get_validation_error_count used as the key. The transient data is deleted when terms are created, deleted, or updated.

Additionally, this PR caches the validated URL count shown in the "At a Glance" admin dashboard widget using the transient used to show the count in the sidebar.

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

github-actions · 2020-09-07T01:53:11Z

Plugin builds for 4fe39ad are ready 🛎️!

Download development build
Download production build

westonruter

The transient will also need to be deleted in \AMP_Validated_URL_Post_Type::handle_validation_error_status_update().

Also when an amp_validated_url post is deleted.

Basically I think wherever we have a delete_transient( AMP_Validated_URL_Post_Type::NEW_VALIDATION_ERROR_URLS_COUNT_TRANSIENT ) we'll need to also add deletion of this other transient.

google-cla · 2020-09-09T00:49:08Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

pierlon · 2020-09-09T00:49:17Z

@googlebot I consent.

johnwatkins0 · 2020-09-09T03:01:46Z

Added transient deletion in more places, and it looks like Pierre helped with the unrelated e2e test failure.

…ount

westonruter · 2020-09-09T04:31:23Z

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

@@ -219,6 +219,7 @@ static function () {
 		$handle_delete = static function ( $post_id ) {
 			if ( static::POST_TYPE_SLUG === get_post_type( $post_id ) ) {
 				delete_transient( static::NEW_VALIDATION_ERROR_URLS_COUNT_TRANSIENT );
+				delete_transient( AMP_Validation_Error_Taxonomy::TRANSIENT_KEY_ERROR_INDEX_COUNTS );


Just saw that the value stored in the TRANSIENT_KEY_ERROR_INDEX_COUNTS is only amp_validation_error taxonomy terms irrespective of amp_validated_url posts (hide_empty is not set to true when calling wp_count_terms()). But someone could pass 'hide_empty' => true when calling \AMP_Validation_Error_Taxonomy::get_validation_error_count(). So I suppose this is safest to include.

westonruter · 2020-09-09T04:39:06Z

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

@@ -1970,6 +1972,7 @@ static function( $result ) {
 			}

 			delete_transient( static::NEW_VALIDATION_ERROR_URLS_COUNT_TRANSIENT );
+			delete_transient( AMP_Validation_Error_Taxonomy::TRANSIENT_KEY_ERROR_INDEX_COUNTS );


I realize this may not be necessary because recheck_post() will call AMP_Validated_URL_Post_Type::store_validation_errors() which will then clear both of these transients.

Yeah, it won't be needed with clearing transient at the edit_amp_validation_error action.

Suggested change

delete_transient( AMP_Validation_Error_Taxonomy::TRANSIENT_KEY_ERROR_INDEX_COUNTS );

westonruter · 2020-09-09T04:46:53Z

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

+		$count = get_transient( static::NEW_VALIDATION_ERROR_URLS_COUNT_TRANSIENT );

-		$query = new WP_Query(
-			[
-				'post_type'              => self::POST_TYPE_SLUG,
-				AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_STATUS_QUERY_VAR => [
-					AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_REJECTED_STATUS,
-					AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_ACCEPTED_STATUS,
-				],
-				'update_post_meta_cache' => false,
-				'update_post_term_cache' => false,
-			]
-		);
+		if ( false === $count ) {
+
+			$query = new WP_Query(
+				[
+					'post_type'              => self::POST_TYPE_SLUG,
+					AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_STATUS_QUERY_VAR => [
+						AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_REJECTED_STATUS,
+						AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_ACCEPTED_STATUS,
+					],
+					'update_post_meta_cache' => false,
+					'update_post_term_cache' => false,
+				]
+			);
+
+			$count = $query->found_posts;
+			set_transient( static::NEW_VALIDATION_ERROR_URLS_COUNT_TRANSIENT, $count, DAY_IN_SECONDS );
+		}


It turns out the logic here is duplicated with the logic in \AMP_Validated_URL_Post_Type::get_validation_error_urls_count(). What we are missing is setting the transient inside this method.

Done in b98b921

westonruter · 2020-09-09T04:48:44Z

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

@@ -957,6 +958,7 @@ public static function store_validation_errors( $validation_errors, $url, $args
 		}

 		delete_transient( static::NEW_VALIDATION_ERROR_URLS_COUNT_TRANSIENT );
+		delete_transient( AMP_Validation_Error_Taxonomy::TRANSIENT_KEY_ERROR_INDEX_COUNTS );


This is correct as terms may be created/updated.

However, it is not needed if we delete the transient at the edit_amp_validation_error action.

Suggested change

delete_transient( AMP_Validation_Error_Taxonomy::TRANSIENT_KEY_ERROR_INDEX_COUNTS );

westonruter · 2020-09-09T04:57:48Z

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

+
+		delete_transient( self::TRANSIENT_KEY_ERROR_INDEX_COUNTS );
+


Instead of deleting the transient here, we can catch more cases by adding a delete_amp_validation_error action above.

Suggested change

delete_transient( self::TRANSIENT_KEY_ERROR_INDEX_COUNTS );

westonruter · 2020-09-09T04:58:05Z

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

@@ -279,6 +286,8 @@ public static function register() {
 		if ( is_admin() ) {
 			self::add_admin_hooks();
 		}
+
+		add_action( 'created_' . self::TAXONOMY_SLUG, [ __CLASS__, 'clear_cached_counts' ] );


Suggested change

add_action( 'created_' . self::TAXONOMY_SLUG, [ __CLASS__, 'clear_cached_counts' ] );

add_action( 'created_' . self::TAXONOMY_SLUG, [ __CLASS__, 'clear_cached_counts' ] );

add_action( 'delete_' . self::TAXONOMY_SLUG, [ __CLASS__, 'clear_cached_counts' ] );

add_action( 'edit_' . self::TAXONOMY_SLUG, [ __CLASS__, 'clear_cached_counts' ] );

Done in 0675a76.

@pierlon @johnwatkins0 Thoughts?

westonruter · 2020-09-09T05:01:38Z

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

@@ -2846,6 +2874,7 @@ public static function handle_validation_error_update( $redirect_to, $action, $t

 		if ( $updated_count ) {
 			delete_transient( AMP_Validated_URL_Post_Type::NEW_VALIDATION_ERROR_URLS_COUNT_TRANSIENT );
+			delete_transient( self::TRANSIENT_KEY_ERROR_INDEX_COUNTS );


This is not needed with adding a delete_amp_validation_error action callback above.

westonruter · 2020-09-09T05:30:02Z

After the changes in this PR, all queries pull from transients:

Before	After

westonruter

But looking for confirmation prior to merging.

johnwatkins0 · 2020-09-09T22:10:29Z

@westonruter I see the same.

westonruter · 2020-09-09T22:17:07Z

So you endorse my additional changes?

johnwatkins0 · 2020-09-09T22:21:30Z

✅ Endorsed. I think we're still deleting the transient more than technically needed, but that's better than less than needed.

pierlon · 2020-09-09T22:22:07Z

Oh wait! Making a quick review.

pierlon · 2020-09-09T22:24:59Z

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

+		add_action( 'edit_' . self::TAXONOMY_SLUG, [ __CLASS__, 'clear_cached_counts' ] );
+		add_action( 'delete_' . self::TAXONOMY_SLUG, [ __CLASS__, 'clear_cached_counts' ] );


Should these instead be edited_ and deleted_? The edit_ and delete_ actions are run before the term is filtered, while the past tense counterpart is run after the term cache has been cleaned.

Note that created_ is used above.

I'm seeing that edit_{$taxonomy} also happens after the edit has been made. It's docblock says:

Fires after a term in a specific taxonomy has been updated, but before the term cache has been cleaned.

I don't see an action for deleted_{$taxonomy}.

Yes your right. Just was wondering why the use of created_{$taxonomy} and not create_{$taxonomy}.

I suppose we could use created_{$taxonomy} and edited_{$taxonomy} instead?

I see that saved_{$taxonomy} is called when both inserting and editing, so that's an alternative.

Oh wait that was introduced in WP 5.5, nevermind.

See 4fe39ad, where edited_{$taxonomy} is instead used.

pierlon · 2020-09-09T22:28:13Z

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

-			$new_validation_error_urls = (int) $new_validation_error_urls;
-		}
-
+		$new_validation_error_urls = static::get_validation_error_urls_count();


Should this instead be self:: (although the class is not marked as final)?

Suggested change

$new_validation_error_urls = static::get_validation_error_urls_count();

$new_validation_error_urls = self::get_validation_error_urls_count();

Just wondering after seeing the alternating uses of static and self within the class.

I think it's in part because we used to support PHP 5.2 in which static:: didn't exist.

Ah OK, that can be attended to at a later time.

… before

Co-authored-by: John Watkins <johnwatkins0@gmail.com> Co-authored-by: Pierre Gordon <pierregordon@protonmail.com>

johnwatkins0 added 3 commits September 6, 2020 12:26

Cache error counts

061af1d

Cache error count in 'At a Glance' widget

1031afc

Delete transient on term updates

3ba723b

google-cla bot added the cla: yes Signed the Google CLA label Sep 6, 2020

johnwatkins0 marked this pull request as ready for review September 7, 2020 01:49

johnwatkins0 self-assigned this Sep 7, 2020

johnwatkins0 requested review from westonruter, schlessera and pierlon September 7, 2020 22:22

westonruter requested changes Sep 8, 2020

View reviewed changes

johnwatkins0 added 2 commits September 8, 2020 07:57

Delete error count transient in more places

f4cdf2b

E2e tests: use neve instead of astra

6c72b93

westonruter added the WS:Core Work stream for Plugin core label Sep 8, 2020

Use non-reader theme

9bb244f

google-cla bot added cla: no Has not signed the Google CLA and removed cla: yes Signed the Google CLA labels Sep 9, 2020

google-cla bot added cla: yes Signed the Google CLA and removed cla: no Has not signed the Google CLA labels Sep 9, 2020

Delete non-reader theme

fb98d8e

pierlon approved these changes Sep 9, 2020

View reviewed changes

westonruter modified the milestones: v2.1, v2.0.2 Sep 9, 2020

johnwatkins0 requested a review from westonruter September 9, 2020 03:01

De-duplicate logic and add transients for get_validation_error_urls_c…

b98b921

…ount

westonruter requested changes Sep 9, 2020

View reviewed changes

westonruter added 2 commits September 8, 2020 22:14

Fix logic inversion error

262a074

Delete transient when editing/deleting amp_validation_error term

0675a76

westonruter approved these changes Sep 9, 2020

View reviewed changes

pierlon reviewed Sep 9, 2020

View reviewed changes

Clear cached counts after the term cache has been cleaned, instead of…

4fe39ad

… before

pierlon approved these changes Sep 9, 2020

View reviewed changes

westonruter merged commit a2c0d44 into develop Sep 10, 2020

westonruter deleted the feature/5271-cache-error-index-counts branch September 10, 2020 00:00

westonruter added a commit that referenced this pull request Sep 10, 2020

Cache error index counts and URL count in "At a Glance" widget (#5343)

5032fa7

Co-authored-by: John Watkins <johnwatkins0@gmail.com> Co-authored-by: Pierre Gordon <pierregordon@protonmail.com>

westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Sep 17, 2020

westonruter mentioned this pull request Jan 12, 2021

Asynchronously populate counts for unreviewed validation errors and validated URLs in admin menu and dashboard widget #5772

Closed

		add_action( 'edit_' . self::TAXONOMY_SLUG, [ __CLASS__, 'clear_cached_counts' ] );
		add_action( 'delete_' . self::TAXONOMY_SLUG, [ __CLASS__, 'clear_cached_counts' ] );

	$new_validation_error_urls = static::get_validation_error_urls_count();
	$new_validation_error_urls = self::get_validation_error_urls_count();

Cache error index counts and URL count in "At a Glance" widget #5343

Cache error index counts and URL count in "At a Glance" widget #5343

Conversation

johnwatkins0 commented Sep 6, 2020 • edited Loading

Summary

Checklist

github-actions bot commented Sep 7, 2020 • edited Loading

westonruter left a comment

Choose a reason for hiding this comment

google-cla bot commented Sep 9, 2020

pierlon commented Sep 9, 2020

johnwatkins0 commented Sep 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Sep 9, 2020

westonruter left a comment

Choose a reason for hiding this comment

johnwatkins0 commented Sep 9, 2020

westonruter commented Sep 9, 2020

johnwatkins0 commented Sep 9, 2020

pierlon commented Sep 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnwatkins0 commented Sep 6, 2020 •

edited

Loading

github-actions bot commented Sep 7, 2020 •

edited

Loading