From c84beaed6e0960df3e5ef985b3e9979015801c7c Mon Sep 17 00:00:00 2001 From: Mohammad Jangda Date: Tue, 9 Feb 2021 19:20:42 -0500 Subject: [PATCH 1/5] Restrict Unpublished Files: PURGE attachment URLs When a post is updated, we should purge all children attachments. This allows us to propagate changes more quickly to the edges, and also allows us to cache public files for much longer which has performance benefits. --- files/acl/acl.php | 2 + files/acl/restrict-unpublished-files.php | 41 ++++++++++++- .../acl/test-restrict-unpublished-files.php | 57 +++++++++++++++++++ 3 files changed, 99 insertions(+), 1 deletion(-) diff --git a/files/acl/acl.php b/files/acl/acl.php index 598f27b55b..77325f6869 100644 --- a/files/acl/acl.php +++ b/files/acl/acl.php @@ -39,6 +39,8 @@ function maybe_load_restrictions() { require_once( __DIR__ . '/restrict-unpublished-files.php' ); add_filter( 'vip_files_acl_file_visibility', __NAMESPACE__ . '\Restrict_Unpublished_Files\check_file_visibility', 10, 2 ); + // Purge attachments for posts for better cacheability + add_filter( 'wpcom_vip_cache_purge_urls', __NAMESPACE__ . '\Restrict_Unpublished_Files\purge_attachments_for_post', 10, 2 ); } } diff --git a/files/acl/restrict-unpublished-files.php b/files/acl/restrict-unpublished-files.php index 8575da3c4b..7b35e03e2b 100644 --- a/files/acl/restrict-unpublished-files.php +++ b/files/acl/restrict-unpublished-files.php @@ -12,7 +12,6 @@ const CACHE_GROUP = 'vip-files-acl'; - /** * Given a path determine whether the file is private or public * @@ -105,3 +104,43 @@ function get_attachment_id_from_file_path( $path ) { return $attachment_id; } + +/** + * PURGE all attachments for a given post. + * + * This is to allow us to propagate visibility changes ASAP to the edges + * and also to allow us to cache public files for much longer (which comes + * some performance benefits). + * + * @param array urls Array of URLs to be purged + * @param int post_id The ID of the post for which we're purging URLs + * + * @return array An array of URLs to be purged + */ +function purge_attachments_for_post( $urls, $post_id ) { + $post = get_post( $post_id ); + + if ( empty( $post ) ) { + return $urls; + } + + $attachment_ids = get_posts( [ + 'post_parent' => $post->ID, + 'post_type' => 'attachment', + 'posts_per_page' => 100, // Set a reasonably high limit (instead of -1 as default) + 'orderby' => 'ID', // For performance (instead of `date` as default) + 'order' => 'ASC', + 'fields' => 'ids', + 'suppress_filters' => false, + ] ); + + if ( empty( $attachment_ids ) ) { + return $urls; + } + + foreach ( $attachment_ids as $attachment_id ) { + $urls[] = wp_get_attachment_url( $attachment_id ); + } + + return $urls; +} \ No newline at end of file diff --git a/tests/files/acl/test-restrict-unpublished-files.php b/tests/files/acl/test-restrict-unpublished-files.php index 6d88ec60a8..6aa317c5b1 100644 --- a/tests/files/acl/test-restrict-unpublished-files.php +++ b/tests/files/acl/test-restrict-unpublished-files.php @@ -222,5 +222,62 @@ public function test__get_attachment_id_from_file_path__attachment_multiple_resu private function strip_wpcontent_uploads( $path ) { return substr( $path, strlen( '/wp-content/uploads/' ) ); } + + public function test__purge_attachments_for_post__invalid_post() { + // Input and output are the same; no change + $input_urls = [ + 'https://example.com', + ]; + $expected_urls = [ + 'https://example.com', + ]; + + // Set a highly unlikely post ID + $test_post_id = PHP_INT_MAX; + + $actual_urls = purge_attachments_for_post( $input_urls, $test_post_id ); + + $this->assertEquals( $expected_urls, $actual_urls ); + } + + public function test__purge_attachments_for_post__post_with_no_attachments() { + // Input and output are the same; no change + $input_urls = [ + 'https://example.com', + ]; + $expected_urls = [ + 'https://example.com', + ]; + + // No attachments for post + $test_post_id = $this->factory->post->create(); + + $actual_urls = purge_attachments_for_post( $input_urls, $test_post_id ); + + $this->assertEquals( $expected_urls, $actual_urls ); + } + + public function test__purge_attachments_for_post__post_with_some_attachments() { + $input_urls = [ + 'https://example.com', + ]; + + $test_post_id = $this->factory->post->create(); + $attachment_id_1 = $this->factory->attachment->create_upload_object( self::TEST_IMAGE_PATH, $test_post_id ); + $attachment_id_2 = $this->factory->attachment->create_upload_object( self::TEST_IMAGE_PATH, $test_post_id ); + $attachment_id_3 = $this->factory->attachment->create_upload_object( self::TEST_IMAGE_PATH, $test_post_id ); + + // Output should include new attachment URLs + $expected_urls = [ + 'https://example.com', + wp_get_attachment_url( $attachment_id_1 ), + wp_get_attachment_url( $attachment_id_2 ), + wp_get_attachment_url( $attachment_id_3 ), + ]; + + $actual_urls = purge_attachments_for_post( $input_urls, $test_post_id ); + + $this->assertEquals( $expected_urls, $actual_urls ); + } } From ab51ec074f4d80f164efad8842b0477a9ee26ff8 Mon Sep 17 00:00:00 2001 From: Mohammad Jangda Date: Tue, 9 Feb 2021 19:22:47 -0500 Subject: [PATCH 2/5] Add tests to verify attachment purge is enabled --- files/acl/restrict-unpublished-files.php | 12 ++++++------ tests/files/acl/test-acl.php | 7 +++++-- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/files/acl/restrict-unpublished-files.php b/files/acl/restrict-unpublished-files.php index 7b35e03e2b..9dac6283bb 100644 --- a/files/acl/restrict-unpublished-files.php +++ b/files/acl/restrict-unpublished-files.php @@ -120,13 +120,13 @@ function get_attachment_id_from_file_path( $path ) { function purge_attachments_for_post( $urls, $post_id ) { $post = get_post( $post_id ); - if ( empty( $post ) ) { - return $urls; + if ( empty( $post ) ) { + return $urls; } $attachment_ids = get_posts( [ - 'post_parent' => $post->ID, - 'post_type' => 'attachment', + 'post_parent' => $post->ID, + 'post_type' => 'attachment', 'posts_per_page' => 100, // Set a reasonably high limit (instead of -1 as default) 'orderby' => 'ID', // For performance (instead of `date` as default) 'order' => 'ASC', @@ -142,5 +142,5 @@ function purge_attachments_for_post( $urls, $post_id ) { $urls[] = wp_get_attachment_url( $attachment_id ); } - return $urls; -} \ No newline at end of file + return $urls; +} diff --git a/tests/files/acl/test-acl.php b/tests/files/acl/test-acl.php index 0f43e24b87..27e9f7cfd4 100644 --- a/tests/files/acl/test-acl.php +++ b/tests/files/acl/test-acl.php @@ -40,7 +40,8 @@ public function test__maybe_load_restrictions__constant_and_restrict_all_option( maybe_load_restrictions(); - $this->assertEquals( 10, has_filter( 'vip_files_acl_file_visibility', 'Automattic\VIP\Files\Acl\Restrict_All_Files\check_file_visibility' ), 'File visibility filter has no callbacks attached' ); + $this->assertTrue( function_exists( 'Automattic\VIP\Files\Acl\Restrict_All_Files\check_file_visibility' ), 'Restrict_All_Files\check_file_visibility() is not defined; restrict-all-files.php may not have been require-d correctly' ); + $this->assertEquals( 10, has_filter( 'vip_files_acl_file_visibility', 'Automattic\VIP\Files\Acl\Restrict_All_Files\check_file_visibility' ), 'vip_files_acl_file_visibility filter does not have the correct callback attached' ); } /** @@ -53,7 +54,9 @@ public function test__maybe_load_restrictions__constant_and_restrict_unpublished maybe_load_restrictions(); - $this->assertEquals( 10, has_filter( 'vip_files_acl_file_visibility', 'Automattic\VIP\Files\Acl\Restrict_Unpublished_Files\check_file_visibility' ), 'File visibility filter has no callbacks attached' ); + $this->assertTrue( function_exists( 'Automattic\VIP\Files\Acl\Restrict_Unpublished_Files\check_file_visibility' ), 'Restrict_Unpublished_Files\check_file_visibility() is not defined; restrict-unpublished-files.php may not have been require-d correctly' ); + $this->assertEquals( 10, has_filter( 'vip_files_acl_file_visibility', 'Automattic\VIP\Files\Acl\Restrict_Unpublished_Files\check_file_visibility' ), 'vip_files_acl_file_visibility filter does not have correct callback attached' ); + $this->assertEquals( 10, has_filter( 'wpcom_vip_cache_purge_urls', 'Automattic\VIP\Files\Acl\Restrict_Unpublished_Files\purge_attachments_for_post' ), 'wpcom_vip_cache_purge_urls filter does not have correct callback attached' ); } /** From 3965e10a5bdddc05778a3bb867529573628514ee Mon Sep 17 00:00:00 2001 From: Mohammad Jangda Date: Wed, 10 Feb 2021 15:35:37 -0500 Subject: [PATCH 3/5] Higher posts_per_page for attachment purges Since some galleries can have a pretty large number of attachments. We do want to keep an upper limit though to avoid inundating the purge system and also to avoid OOM issues. Any higher than 250 and we likely have a problem. --- files/acl/restrict-unpublished-files.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/files/acl/restrict-unpublished-files.php b/files/acl/restrict-unpublished-files.php index 9dac6283bb..bf7a412daf 100644 --- a/files/acl/restrict-unpublished-files.php +++ b/files/acl/restrict-unpublished-files.php @@ -127,7 +127,7 @@ function purge_attachments_for_post( $urls, $post_id ) { $attachment_ids = get_posts( [ 'post_parent' => $post->ID, 'post_type' => 'attachment', - 'posts_per_page' => 100, // Set a reasonably high limit (instead of -1 as default) + 'posts_per_page' => 250, // Set a reasonably high limit (instead of -1 as default) 'orderby' => 'ID', // For performance (instead of `date` as default) 'order' => 'ASC', 'fields' => 'ids', From e9dfdeb5b3905fe4c4386683e1e5563e461460fc Mon Sep 17 00:00:00 2001 From: Mohammad Jangda Date: Wed, 10 Feb 2021 23:05:01 -0500 Subject: [PATCH 4/5] Don't suppress_filters The query isn't going to be run frequently enough to benefit from caching. Caching would actually be counter productive here since we want fresh data about what attachments a post has. Co-authored-by: Rinat K --- files/acl/restrict-unpublished-files.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/files/acl/restrict-unpublished-files.php b/files/acl/restrict-unpublished-files.php index bf7a412daf..e1306049c2 100644 --- a/files/acl/restrict-unpublished-files.php +++ b/files/acl/restrict-unpublished-files.php @@ -131,7 +131,7 @@ function purge_attachments_for_post( $urls, $post_id ) { 'orderby' => 'ID', // For performance (instead of `date` as default) 'order' => 'ASC', 'fields' => 'ids', - 'suppress_filters' => false, + 'suppress_filters' => true, ] ); if ( empty( $attachment_ids ) ) { From e8a177ceaf5693f4f24a5bbed6ad8ad64019a2ee Mon Sep 17 00:00:00 2001 From: Rinat Khaziev Date: Thu, 11 Feb 2021 11:43:54 -0500 Subject: [PATCH 5/5] Silence the linter --- files/acl/restrict-unpublished-files.php | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/files/acl/restrict-unpublished-files.php b/files/acl/restrict-unpublished-files.php index e1306049c2..e14bc622c4 100644 --- a/files/acl/restrict-unpublished-files.php +++ b/files/acl/restrict-unpublished-files.php @@ -127,10 +127,11 @@ function purge_attachments_for_post( $urls, $post_id ) { $attachment_ids = get_posts( [ 'post_parent' => $post->ID, 'post_type' => 'attachment', - 'posts_per_page' => 250, // Set a reasonably high limit (instead of -1 as default) - 'orderby' => 'ID', // For performance (instead of `date` as default) - 'order' => 'ASC', - 'fields' => 'ids', + 'posts_per_page' => 250, // Set a reasonably high limit (instead of -1 as default) + 'orderby' => 'ID', // For performance (instead of `date` as default) + 'order' => 'ASC', + 'fields' => 'ids', + // phpcs:ignore WordPressVIPMinimum.Performance.WPQueryParams.SuppressFiltersTrue 'suppress_filters' => true, ] );