From 1f2b54c38fed6e24166c8db624a06835a4c150d2 Mon Sep 17 00:00:00 2001 From: gravitypdf-bot Date: Thu, 5 Nov 2026 14:47:55 +1100 Subject: [PATCH] Keep font files another site uses when deleting or editing a custom font On multisite the fonts folder is network-wide but each site keeps its own custom_fonts records, and sites can point at the same file (records from before 6.18.0's unique upload names, and premium templates' add_pdf_font() copies). Deleting a font, or replacing or removing one of its styles, unlinked the file without checking the other sites, breaking their PDFs. This has been the case since 6.0. Model_Custom_Fonts::get_files_used_by_other_sites() lists the basenames every other site's records use. It reads each site's gfpdf_settings straight from that site's options table rather than switching blogs, skips corrupt values, and is memoised per request. It returns nothing on single site and runs no queries there. Controller_Custom_Fonts' delete_item() and update_item() now remove files through one method that keeps any file on that list (logged at debug level), and skips the lookup when there is nothing to delete. Matching by basename errs towards keeping a file: the worst case is an orphan, never a broken PDF. Model_Font_Sources::remove_record() can then always go through GPDFAPI::delete_pdf_font(), dropping the multisite branch that only removed the record and left every file behind. Adoption now also removes a file on multisite once no other site uses it. The style keys move to Model_Custom_Fonts::FONT_STYLES so the controller, font sources and the lookup share one list. Co-Authored-By: Claude Opus 5.5 --- src/Controller/Controller_Custom_Fonts.php | 39 ++++++-- src/Model/Model_Custom_Fonts.php | 76 +++++++++++++++ src/Model/Model_Font_Sources.php | 36 +++---- tests/phpunit/Concerns/CopiesFontFixtures.php | 6 +- .../Test_Controller_Custom_Fonts.php | 95 +++++++++++++++++++ .../Model/Test_Model_Custom_Fonts.php | 60 ++++++++++++ .../Model/Test_Model_Font_Sources.php | 37 +++----- 7 files changed, 294 insertions(+), 55 deletions(-) diff --git a/src/Controller/Controller_Custom_Fonts.php b/src/Controller/Controller_Custom_Fonts.php index 7943b3dcd..0aa997f0f 100644 --- a/src/Controller/Controller_Custom_Fonts.php +++ b/src/Controller/Controller_Custom_Fonts.php @@ -85,7 +85,7 @@ class Controller_Custom_Fonts extends Helper_Abstract_Controller { * @var string[] List of the standard font keys used when saving settings * @since 6.0 */ - protected $font_keys = [ 'regular', 'italics', 'bold', 'bolditalics' ]; + protected $font_keys = Model_Custom_Fonts::FONT_STYLES; public function __construct( Model_Custom_Fonts $model, LoggerInterface $log, Helper_Abstract_Form $gform, string $font_dir_path, string $filesystem = 'GFPDF_Vendor\\GravityPdf\\Upload\\Storage\\FileSystem', string $file = 'GFPDF_Vendor\\GravityPdf\\Upload\\File' ) { $this->model = $model; @@ -428,7 +428,7 @@ public function update_item( WP_REST_Request $request ) { } /* Not one an upload was just saved under, which happens when the old file had gone missing */ - array_map( [ $this, 'delete_font_file' ], array_diff( $unused_files, $uncommitted_files ) ); + $this->delete_unused_font_files( array_diff( $unused_files, $uncommitted_files ) ); $uncommitted_files = []; @@ -554,15 +554,16 @@ public function delete_item( WP_REST_Request $request ) { } /* Delete TTF files from disk */ - $font = $this->model->get_font_by_id( $id ); + $font = $this->model->get_font_by_id( $id ); + $files = []; foreach ( $this->font_keys as $font_id ) { - if ( empty( $font[ $font_id ] ) ) { - continue; + if ( ! empty( $font[ $font_id ] ) ) { + $files[] = basename( $font[ $font_id ] ); } - - $this->delete_font_file( basename( $font[ $font_id ] ) ); } + $this->delete_unused_font_files( $files ); + /* Update DB */ if ( ! $this->model->delete_font( $id ) ) { throw new GravityPdfDatabaseUpdateException(); @@ -739,6 +740,30 @@ protected function get_upload_error_message( File $file, string $error_code = Er return __( 'The upload is not a valid TTF file', 'gravity-pdf' ); } + /** + * Delete font files, keeping any another site's fonts use (the fonts folder is network-wide) + * + * @param string[] $files The filenames + * + * @since 6.19.0 + */ + protected function delete_unused_font_files( array $files ): void { + if ( $files === [] ) { + return; + } + + $shared = $this->model->get_files_used_by_other_sites(); + + foreach ( $files as $file ) { + if ( in_array( $file, $shared, true ) ) { + $this->log->debug( 'Kept a font file another site uses', [ 'file' => $file ] ); + continue; + } + + $this->delete_font_file( $file ); + } + } + /** * @param string $file The filename of the font to be deleted * diff --git a/src/Model/Model_Custom_Fonts.php b/src/Model/Model_Custom_Fonts.php index debe2b553..713004a7f 100644 --- a/src/Model/Model_Custom_Fonts.php +++ b/src/Model/Model_Custom_Fonts.php @@ -28,12 +28,24 @@ */ class Model_Custom_Fonts extends Helper_Abstract_Model { + /** + * @var string[] The keys a font record stores its files under + * @since 6.19.0 + */ + public const FONT_STYLES = [ 'regular', 'italics', 'bold', 'bolditalics' ]; + /** * @var Helper_Abstract_Options * @since 6.0 */ protected $options; + /** + * @var array The files other sites' fonts use, keyed by the current site ID + * @since 6.19.0 + */ + protected $files_used_by_other_sites = []; + public function __construct( Helper_Abstract_Options $options ) { $this->options = $options; } @@ -158,6 +170,70 @@ public function delete_font( string $id ): bool { return $this->options->update_option( 'custom_fonts', $fonts ); } + /** + * The filenames the other sites' fonts use, read from each site's options table to avoid switch_to_blog() + * + * @return string[] + * + * @since 6.19.0 + */ + public function get_files_used_by_other_sites(): array { + global $wpdb; + + if ( ! is_multisite() ) { + return []; + } + + $current_site = get_current_blog_id(); + if ( isset( $this->files_used_by_other_sites[ $current_site ] ) ) { + return $this->files_used_by_other_sites[ $current_site ]; + } + + $site_ids = get_sites( + [ + 'fields' => 'ids', + 'number' => 0, + 'site__not_in' => [ $current_site ], + ] + ); + + $files = []; + + foreach ( $site_ids as $site_id ) { + $prefix = $wpdb->get_blog_prefix( $site_id ); + $settings = maybe_unserialize( $wpdb->get_var( "SELECT option_value FROM {$prefix}options WHERE option_name = 'gfpdf_settings' LIMIT 1" ) ); //phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery, WordPress.DB.DirectDatabaseQuery.NoCaching, WordPress.DB.PreparedSQL.InterpolatedNotPrepared + + if ( ! is_array( $settings ) || ! is_array( $settings['custom_fonts'] ?? null ) ) { + continue; + } + + foreach ( $settings['custom_fonts'] as $font ) { + if ( ! is_array( $font ) ) { + continue; + } + + foreach ( self::FONT_STYLES as $style ) { + if ( is_string( $font[ $style ] ?? null ) && $font[ $style ] !== '' ) { + $files[] = basename( $font[ $style ] ); + } + } + } + } + + $this->files_used_by_other_sites[ $current_site ] = array_values( array_unique( $files ) ); + + return $this->files_used_by_other_sites[ $current_site ]; + } + + /** + * Forget the other sites' font files read this request + * + * @since 6.19.0 + */ + public function flush(): void { + $this->files_used_by_other_sites = []; + } + /** * Checks if the ID already exists and, if so, suffixes the ID with a string until unique * diff --git a/src/Model/Model_Font_Sources.php b/src/Model/Model_Font_Sources.php index d8d4c01f6..624852743 100644 --- a/src/Model/Model_Font_Sources.php +++ b/src/Model/Model_Font_Sources.php @@ -4,7 +4,6 @@ namespace GFPDF\Model; -use GFPDF\Helper\Fonts\FlushCache; use GFPDF\Helper\Helper_Abstract_Model; use GFPDF\Helper\Helper_Data; use GFPDF\Statics\Git_Blob; @@ -668,7 +667,7 @@ protected function get_record_file_names(): array { $names = []; foreach ( $records as $record ) { - foreach ( [ 'regular', 'italics', 'bold', 'bolditalics' ] as $style ) { + foreach ( Model_Custom_Fonts::FONT_STYLES as $style ) { $file = is_string( $record[ $style ] ?? null ) ? basename( $record[ $style ] ) : ''; if ( isset( $family_files[ $file ] ) ) { $names[ $file ] = $file; @@ -1032,39 +1031,30 @@ public function adopt_records( string $source_id ): array { } /** - * Remove a custom font record, and outside Multisite its files too: another site's record may use the shared fonts - * folder. Logs and returns false on failure + * Remove a custom font record and the files no other site uses. Logs and returns false on failure * * @since 6.19.0 */ protected function remove_record( string $id ): bool { - $removed = false; - $error = ''; + $error = ''; try { - if ( ! is_multisite() ) { - $removed = GPDFAPI::delete_pdf_font( $id ) === true; - } elseif ( $this->custom_fonts->delete_font( $id ) ) { - FlushCache::flush( $id ); - $removed = true; + if ( GPDFAPI::delete_pdf_font( $id ) === true ) { + return true; } } catch ( Throwable $e ) { $error = $e->getMessage(); } - if ( ! $removed ) { - $this->log->error( - 'Could not remove a custom font record replaced by a font source', - [ - 'font' => $id, - 'exception' => $error, - ] - ); - - return false; - } + $this->log->error( + 'Could not remove a custom font record replaced by a font source', + [ + 'font' => $id, + 'exception' => $error, + ] + ); - return true; + return false; } /** diff --git a/tests/phpunit/Concerns/CopiesFontFixtures.php b/tests/phpunit/Concerns/CopiesFontFixtures.php index 0a587afad..602e3bbdc 100644 --- a/tests/phpunit/Concerns/CopiesFontFixtures.php +++ b/tests/phpunit/Concerns/CopiesFontFixtures.php @@ -5,7 +5,6 @@ namespace GFPDF\Tests\Concerns; use GFPDF\Helper\Fonts\FlushCache; -use GFPDF\Model\Model_Custom_Fonts; use GFPDF\Model\Model_Font_Sources; use GFPDF\Statics\Git_Blob; use GPDFAPI; @@ -98,8 +97,8 @@ protected function add_record( string $id, array $files ): void { /** * A model whose bundled source accepts the fixture as a legacy Arimo copy */ - protected function get_model_with_legacy( string $fixture, ?Model_Custom_Fonts $custom_fonts = null ): Model_Font_Sources { - $model = new class( GPDFAPI::get_data_class(), $custom_fonts ?? GPDFAPI::get_mvc_class( 'Model_Custom_Fonts' ), GPDFAPI::get_log_class() ) extends Model_Font_Sources { + protected function get_model_with_legacy( string $fixture ): Model_Font_Sources { + $model = new class( GPDFAPI::get_data_class(), GPDFAPI::get_mvc_class( 'Model_Custom_Fonts' ), GPDFAPI::get_log_class() ) extends Model_Font_Sources { /** @var array */ public $legacy = []; @@ -294,6 +293,7 @@ protected function remove_font_fixtures(): void { array_map( [ FlushCache::class, 'flush' ], array_unique( $this->recorded_fonts ) ); GPDFAPI::get_mvc_class( 'Model_Font_Sources' )->flush(); GPDFAPI::get_mvc_class( 'Model_Required_Fonts' )->flush(); + GPDFAPI::get_mvc_class( 'Model_Custom_Fonts' )->flush(); /* A download or its fallback caches metrics under every font key the render asked for, substitutes included */ if ( $this->requested !== [] ) { diff --git a/tests/phpunit/integration/Controller/Test_Controller_Custom_Fonts.php b/tests/phpunit/integration/Controller/Test_Controller_Custom_Fonts.php index 1c80a1ca0..64e1d2002 100644 --- a/tests/phpunit/integration/Controller/Test_Controller_Custom_Fonts.php +++ b/tests/phpunit/integration/Controller/Test_Controller_Custom_Fonts.php @@ -883,6 +883,101 @@ public function test_delete_item_with_font_reference_gone_success() { $this->assertSame( 200, $response->get_status() ); } + /** + * @group ms-required + */ + public function test_deleting_a_font_keeps_the_files_another_site_uses() { + global $gfpdf; + + $this->skipWithoutMultisite(); + + [ $shared, $own, $other_site ] = $this->share_a_file_with_another_site(); + + $this->assertNull( $this->controller->delete_item( $this->delete_request( 'font' ) ) ); + + $this->assertSame( [], $this->model->get_custom_fonts() ); + $this->assertFileExists( $shared ); + $this->assertFileDoesNotExist( $own ); + $this->assertArrayHasKey( 'other', get_blog_option( $other_site, 'gfpdf_settings' )['custom_fonts'] ); + + /* The last site using the file removes it */ + switch_to_blog( $other_site ); + $gfpdf->options->set_plugin_settings(); + $response = $this->controller->delete_item( $this->delete_request( 'other' ) ); + restore_current_blog(); + $gfpdf->options->set_plugin_settings(); + + $this->assertNull( $response ); + $this->assertFileDoesNotExist( $shared ); + } + + /** + * @group ms-required + */ + public function test_update_item_keeps_a_replaced_file_another_site_uses() { + $this->skipWithoutMultisite(); + + [ $shared, $own, $other_site ] = $this->share_a_file_with_another_site(); + + $request = new WP_REST_Request( 'POST' ); + $request->set_body_params( + [ + 'id' => 'font', + 'bold' => '', + ] + ); + $this->set_font_files( $request, [ 'regular' => [ 'DejaVuSans.ttf', self::$test_fonts[0] ] ] ); + + $updated = $this->controller->update_item( $request ); + + $this->assertNotSame( $shared, $updated['regular'] ); + $this->assertSame( '', $updated['bold'] ); + $this->assertFileExists( $shared ); + $this->assertFileDoesNotExist( $own ); + } + + /** + * Save a font whose regular file another site's font also uses, and whose bold file is only its own + * + * @return array{0: string, 1: string, 2: int} The shared file, the font's own file and the other site's ID + */ + protected function share_a_file_with_another_site(): array { + $shared = $this->copy_font( 0, 'Shared.ttf' ); + $own = $this->copy_font( 1, 'Own.ttf' ); + $this->model->add_font( $this->font_record( 'font', $shared, $own ) ); + + $other_site = self::factory()->blog->create(); + update_blog_option( $other_site, 'gfpdf_settings', [ 'custom_fonts' => [ 'other' => $this->font_record( 'other', $shared ) ] ] ); + + return [ $shared, $own, $other_site ]; + } + + protected function copy_font( int $fixture, string $name ): string { + copy( self::$test_fonts[ $fixture ], $this->tmp_font_location . $name ); + + return $this->tmp_font_location . $name; + } + + protected function font_record( string $id, string $regular, string $bold = '' ): array { + return [ + 'id' => $id, + 'font_name' => $id, + 'regular' => $regular, + 'italics' => '', + 'bold' => $bold, + 'bolditalics' => '', + 'useOTL' => 0x00, + 'useKashida' => 0, + ]; + } + + protected function delete_request( string $id ): WP_REST_Request { + $request = new WP_REST_Request( 'DELETE' ); + $request->set_param( 'id', $id ); + + return $request; + } + public function test_delete_item_invalid_font_id() { wp_set_current_user( $this->admin_user ); diff --git a/tests/phpunit/integration/Model/Test_Model_Custom_Fonts.php b/tests/phpunit/integration/Model/Test_Model_Custom_Fonts.php index 8c6aa2e52..3504d7231 100644 --- a/tests/phpunit/integration/Model/Test_Model_Custom_Fonts.php +++ b/tests/phpunit/integration/Model/Test_Model_Custom_Fonts.php @@ -198,4 +198,64 @@ public function data_has_unique_font_id(): array { [ true, 'dejavusans1' ], ]; } + + /** + * @group ms-excluded + */ + public function test_a_single_site_reads_no_other_sites_fonts() { + global $wpdb; + + $this->skipWithMultisite(); + + $queries = $wpdb->num_queries; + + $this->assertSame( [], $this->model->get_files_used_by_other_sites() ); + $this->assertSame( $queries, $wpdb->num_queries ); + } + + /** + * @group ms-required + */ + public function test_get_files_used_by_other_sites() { + global $wpdb; + + $this->skipWithoutMultisite(); + + $this->model->add_font( + [ + 'id' => 'mine', + 'regular' => '/fonts/Mine.ttf', + ] + ); + + $fonts = [ + 'one' => [ + 'regular' => '/fonts/One.ttf', + 'bolditalics' => '/fonts/Shared.ttf', + ], + 'two' => [ + 'regular' => '', + 'italics' => '/fonts/Shared.ttf', + 'bold' => [ 'Array.ttf' ], + ], + 'corrupt' => 'font', + ]; + + update_blog_option( self::factory()->blog->create(), 'gfpdf_settings', [ 'custom_fonts' => $fonts ] ); + update_blog_option( self::factory()->blog->create(), 'gfpdf_settings', [ 'custom_fonts' => 'fonts' ] ); + update_blog_option( self::factory()->blog->create(), 'gfpdf_settings', 'settings' ); + self::factory()->blog->create(); + + /* A truncated serialized value */ + $truncated = self::factory()->blog->create(); + update_blog_option( $truncated, 'gfpdf_settings', [ 'custom_fonts' => [] ] ); + $wpdb->update( $wpdb->get_blog_prefix( $truncated ) . 'options', [ 'option_value' => 'a:1:{s:12:"custom_fonts";a:1:{' ], [ 'option_name' => 'gfpdf_settings' ] ); + + $this->assertSame( [ 'One.ttf', 'Shared.ttf' ], $this->model->get_files_used_by_other_sites() ); + + /* Read once a request */ + $queries = $wpdb->num_queries; + $this->model->get_files_used_by_other_sites(); + $this->assertSame( $queries, $wpdb->num_queries ); + } } diff --git a/tests/phpunit/integration/Model/Test_Model_Font_Sources.php b/tests/phpunit/integration/Model/Test_Model_Font_Sources.php index 8cac6882a..dd20ff3a2 100644 --- a/tests/phpunit/integration/Model/Test_Model_Font_Sources.php +++ b/tests/phpunit/integration/Model/Test_Model_Font_Sources.php @@ -4,7 +4,6 @@ namespace GFPDF\Model; -use GFPDF\Exceptions\GravityPdfIdException; use GFPDF\Tests\Concerns\CopiesFontFixtures; use GFPDF\Tests\Concerns\Seamed_Font_Downloader; use GFPDF\Tests\Concerns\UnregistersMvcClasses; @@ -427,10 +426,8 @@ public function test_adopt_records_removes_a_record_matching_the_manifest() { $this->assertSame( [ 'dejavusanscondensed' ], $this->model->adopt_records( 'core' ) ); $this->assertSame( [], $this->custom_fonts->get_custom_fonts() ); - - /* Multisite keeps the files in the shared fonts folder */ - $this->assertSame( is_multisite(), is_file( $this->font_dir . 'DejaVuSansCondensed85274.ttf' ) ); - $this->assertSame( is_multisite(), is_file( $this->font_dir . 'DejaVuSans-Bold.ttf' ) ); + $this->assertFileDoesNotExist( $this->font_dir . 'DejaVuSansCondensed85274.ttf' ); + $this->assertFileDoesNotExist( $this->font_dir . 'DejaVuSans-Bold.ttf' ); } public function test_adopt_records_removes_a_record_of_the_core_font_installers_files() { @@ -486,7 +483,7 @@ public function test_adopt_records_removes_a_record_matching_legacy_hashes() { $this->assertSame( [ 'arimo' ], $model->adopt_records( 'bundled' ) ); $this->assertSame( [], $this->custom_fonts->get_custom_fonts() ); - $this->assertSame( is_multisite(), is_file( $this->font_dir . 'Arimo-Regular85274.ttf' ) ); + $this->assertFileDoesNotExist( $this->font_dir . 'Arimo-Regular85274.ttf' ); } public function test_adopt_records_keeps_an_edited_record() { @@ -933,10 +930,6 @@ public function test_adopt_records_logs_a_record_it_could_not_remove() { } public function test_adopt_records_keeps_the_record_when_the_custom_fonts_controller_is_unhooked() { - if ( is_multisite() ) { - $this->markTestSkipped( 'Multisite removes the record through the model' ); - } - $model = $this->get_model_with_legacy( 'Chewy.ttf' ); $this->copy_fixture( 'Chewy.ttf', 'Arimo-Regular.ttf' ); $this->add_record( 'arimo', [ 'regular' => 'Arimo-Regular.ttf' ] ); @@ -952,22 +945,22 @@ function () use ( $model ) { $this->assertFileExists( $this->font_dir . 'Arimo-Regular.ttf' ); } - public function test_adopt_records_logs_a_record_removed_elsewhere_on_multisite() { - if ( ! is_multisite() ) { - $this->markTestSkipped( 'Single site removes the record through the API' ); - } - - $custom_fonts = new class( GPDFAPI::get_options_class() ) extends Model_Custom_Fonts { - public function delete_font( string $id ): bool { - throw new GravityPdfIdException(); - } - }; + /** + * @group ms-required + */ + public function test_adopt_records_keeps_a_file_another_site_uses() { + $this->skipWithoutMultisite(); - $model = $this->get_model_with_legacy( 'Chewy.ttf', $custom_fonts ); + $model = $this->get_model_with_legacy( 'Chewy.ttf' ); $this->copy_fixture( 'Chewy.ttf', 'Arimo-Regular.ttf' ); $this->add_record( 'arimo', [ 'regular' => 'Arimo-Regular.ttf' ] ); - $this->assert_record_kept( $model->adopt_records( 'bundled' ) ); + $other_site = self::factory()->blog->create(); + update_blog_option( $other_site, 'gfpdf_settings', [ 'custom_fonts' => $this->custom_fonts->get_custom_fonts() ] ); + + $this->assertSame( [ 'arimo' ], $model->adopt_records( 'bundled' ) ); + $this->assertSame( [], $this->custom_fonts->get_custom_fonts() ); + $this->assertFileExists( $this->font_dir . 'Arimo-Regular.ttf' ); } public function test_adopt_records_moves_on_after_an_error_removing_a_record() {