From 4018bd887a91b9e92fa8015fde22629a1ac947d4 Mon Sep 17 00:00:00 2001 From: gravitypdf-bot Date: Thu, 12 Nov 2026 11:42:12 +1100 Subject: [PATCH] Count a font whose download hit another font's file as missing Site Health, the System Report and the Font Manager went by size alone, so they counted another font's file of a known size at a core font's name as installed, even after a failed download had found it was another font's. Site Health then left out a failure the failed fonts notice listed, and saving a setting didn't schedule the sync. Model_Required_Fonts and the Font Manager's families now leave out the file the downloader's failure record says is another font's. Model_Font_Sources::is_installed(), which only compared sizes, had no callers left and is removed. Refs #2036 Co-Authored-By: Claude Opus 5.5 --- src/Model/Model_Font_Downloader.php | 8 +++---- src/Model/Model_Font_Sources.php | 9 -------- src/Model/Model_Required_Fonts.php | 21 ++++++++++++++++--- .../phpunit/Concerns/RecordsFontFailures.php | 9 ++++++++ .../Test_Controller_System_Report.php | 11 ++++++++++ .../Model/Test_Downloaded_Fonts.php | 7 +++++++ .../Model/Test_Model_Font_Sources.php | 18 ++++++++-------- .../Model/Test_Model_Required_Fonts.php | 21 +++++++++++++++++++ 8 files changed, 79 insertions(+), 25 deletions(-) diff --git a/src/Model/Model_Font_Downloader.php b/src/Model/Model_Font_Downloader.php index 474675b57..4f8c513b5 100644 --- a/src/Model/Model_Font_Downloader.php +++ b/src/Model/Model_Font_Downloader.php @@ -481,8 +481,8 @@ protected function is_unknown_build( string $file ): bool { } /** - * Whether the font's own copy is on disk, as is_installed() but without the file a failed download found was another - * font's (see is_failed_conflict()) + * Whether the font's own copy is on disk: Model_Font_Sources::get_own_copy(), less the other font's file a failed + * download found at its name (see is_failed_conflict()) * * @since 6.19.0 */ @@ -498,7 +498,7 @@ protected function has_own_copy( string $file ): bool { * * @since 6.19.0 */ - protected function is_failed_conflict( string $file, string $own_path ): bool { + public function is_failed_conflict( string $file, string $own_path ): bool { $conflict = $this->get_failures()[ $file ]['conflict'] ?? ''; /* A hashed copy is the font's own whatever is at the name */ @@ -1002,7 +1002,7 @@ protected function get_core_families(): array { foreach ( $files as $file ) { $bytes += (int) $manifest[ $file ]['size']; $path = $this->font_sources->get_own_copy( $file ); - if ( $path !== '' ) { + if ( $path !== '' && ! $this->is_failed_conflict( $file, $path ) ) { $paths[ $file ] = $path; } } diff --git a/src/Model/Model_Font_Sources.php b/src/Model/Model_Font_Sources.php index f028efd89..b5c708338 100644 --- a/src/Model/Model_Font_Sources.php +++ b/src/Model/Model_Font_Sources.php @@ -746,15 +746,6 @@ public function get_manifest_file( string $name ): string { return $entry !== null && $parsed['sha'] === substr( $entry['sha'], 0, 8 ) ? $parsed['file'] : ''; } - /** - * Whether the file's own copy is in the fonts folder (see get_own_copy()) - * - * @since 6.19.0 - */ - public function is_installed( string $file ): bool { - return $this->get_own_copy( $file ) !== ''; - } - /** * Whether the file at a manifest name is another font's going by its size alone: no build the sources know has it * diff --git a/src/Model/Model_Required_Fonts.php b/src/Model/Model_Required_Fonts.php index 5e5f35c08..052ee3517 100644 --- a/src/Model/Model_Required_Fonts.php +++ b/src/Model/Model_Required_Fonts.php @@ -344,7 +344,7 @@ public function pdf_needs_download( array $pdf ): bool { */ public function needs_download( $font ): bool { foreach ( $this->get_font_files( $font ) as $file ) { - if ( ! $this->font_sources->is_installed( $file ) ) { + if ( ! $this->is_installed( $file ) ) { return true; } } @@ -372,7 +372,7 @@ public function get_installed_counts(): array { $files = array_intersect( $required, array_keys( $manifest ) ); $counts[ (string) $id ] = [ - 'installed' => count( array_filter( $files, [ $this->font_sources, 'is_installed' ] ) ), + 'installed' => count( array_filter( $files, [ $this, 'is_installed' ] ) ), 'total' => count( $files ), 'manifest' => count( $manifest ) > 0, ]; @@ -393,12 +393,27 @@ public function get_missing_files(): array { array_filter( $this->get_required_font_files(), function ( $file ) { - return ! $this->font_sources->is_installed( $file ); + return ! $this->is_installed( $file ); } ) ); } + /** + * Whether the file's own copy is in the fonts folder: Model_Font_Sources::get_own_copy(), less the other font's file a + * failed download found at its name + * + * @since 6.19.0 + */ + protected function is_installed( string $file ): bool { + $own_path = $this->font_sources->get_own_copy( $file ); + + /* The registered downloader, as it's built after this model */ + $downloader = \GPDFAPI::get_mvc_class( 'Model_Font_Downloader' ); + + return $own_path !== '' && ! ( $downloader instanceof Model_Font_Downloader && $downloader->is_failed_conflict( $file, $own_path ) ); + } + /** * A font key as mPDF stores it: trimmed and lower case, or empty for a non-string * diff --git a/tests/phpunit/Concerns/RecordsFontFailures.php b/tests/phpunit/Concerns/RecordsFontFailures.php index 4454c1d40..fe02061bf 100644 --- a/tests/phpunit/Concerns/RecordsFontFailures.php +++ b/tests/phpunit/Concerns/RecordsFontFailures.php @@ -30,6 +30,15 @@ protected function failure( int $gave_up = 0, ?int $retry_at = null ): array { ]; } + /** + * A failure that gave up with another font's file at the font's name, as the downloader stamps it + * + * @return array{retry_at: int, gave_up: int, dismissed: bool, conflict: string} + */ + protected function conflict_failure( string $path ): array { + return array_merge( $this->failure( time() ), [ 'conflict' => filesize( $path ) . ':' . filemtime( $path ) ] ); + } + /** * Store the record where the downloader keeps it, on the main network, and have the shared downloader read it afresh * diff --git a/tests/phpunit/integration/Controller/Test_Controller_System_Report.php b/tests/phpunit/integration/Controller/Test_Controller_System_Report.php index 3c8d5b8c4..3450c6b2d 100644 --- a/tests/phpunit/integration/Controller/Test_Controller_System_Report.php +++ b/tests/phpunit/integration/Controller/Test_Controller_System_Report.php @@ -743,6 +743,17 @@ public function test_the_core_font_health_only_scans_the_forms_when_a_file_could $this->assertSame( [ 'Garuda.ttf' ], $report->get_core_font_health()['missing'] ); } + public function test_the_core_fonts_site_health_test_is_critical_for_a_file_that_gave_up_on_another_fonts_file_of_a_known_size() { + $this->put_same_size_font_at( 'DejaVuSerifCondensed.ttf', 'DejaVuSerifCondensed.ttf' ); + $this->use_report_downloader( [ 'DejaVuSerifCondensed.ttf' ] ); + $this->set_font_failures( [ 'DejaVuSerifCondensed.ttf' => $this->conflict_failure( $this->font_path( 'DejaVuSerifCondensed.ttf' ) ) ] ); + + $result = $this->run_core_fonts_test(); + + $this->assertSame( 'critical', $result['status'] ); + $this->assertStringContainsString( 'DejaVuSerifCondensed.ttf', $result['description'] ); + } + public function test_the_core_fonts_site_health_test_is_critical_for_a_file_that_gave_up() { $this->use_report_downloader( [ 'Garuda.ttf', 'FreeSans.ttf' ] ); $this->set_font_failures( [ 'Garuda.ttf' => $this->failure( time() ) ] ); diff --git a/tests/phpunit/integration/Model/Test_Downloaded_Fonts.php b/tests/phpunit/integration/Model/Test_Downloaded_Fonts.php index 840ef3240..73f5483ab 100644 --- a/tests/phpunit/integration/Model/Test_Downloaded_Fonts.php +++ b/tests/phpunit/integration/Model/Test_Downloaded_Fonts.php @@ -390,6 +390,13 @@ public function test_delete_keeps_another_fonts_file_of_a_known_size_at_the_name $this->assertCount( 1, $this->log_messages( 'Kept another font\'s file at a font file\'s name' ) ); } + public function test_a_font_whose_download_gave_up_on_another_fonts_file_is_available_to_install() { + $this->put_same_size_font_at( 'DejaVuSerifCondensed.ttf', 'DejaVuSerifCondensed.ttf' ); + $this->set_font_failures( [ 'DejaVuSerifCondensed.ttf' => $this->conflict_failure( $this->font_path( 'DejaVuSerifCondensed.ttf' ) ) ] ); + + $this->assertSame( 'available', $this->core_font_row( 'dejavuserifcondensed' )['type'] ); + } + public function test_a_family_sharing_a_name_with_a_record_lists_and_deletes_only_its_hashed_copy() { $sources = GPDFAPI::get_mvc_class( 'Model_Font_Sources' ); $hashed = $sources->get_hashed_name( 'DejaVuSerifCondensed.ttf' ); diff --git a/tests/phpunit/integration/Model/Test_Model_Font_Sources.php b/tests/phpunit/integration/Model/Test_Model_Font_Sources.php index 31280263e..d5506713a 100644 --- a/tests/phpunit/integration/Model/Test_Model_Font_Sources.php +++ b/tests/phpunit/integration/Model/Test_Model_Font_Sources.php @@ -1066,7 +1066,7 @@ public function test_a_family_uses_the_hashed_name_of_a_file_a_font_manager_reco $this->assertTrue( $this->model->is_record_file( 'DejaVuSans.ttf' ) ); $this->assertArrayNotHasKey( $hashed, $this->model->get_manifest(), 'The hashed name is no manifest file of its own' ); $this->assertSame( 'DejaVuSans.ttf', $this->model->get_manifest_file( $hashed ) ); - $this->assertFalse( $this->model->is_installed( 'DejaVuSans.ttf' ), 'The record\'s file is not the family\'s copy' ); + $this->assertSame( '', $this->model->get_own_copy( 'DejaVuSans.ttf' ), 'The record\'s file is not the family\'s copy' ); } public function test_every_record_file_at_a_family_name_is_a_collision() { @@ -1094,11 +1094,11 @@ public function test_the_own_copy_is_the_hashed_copy_else_the_file_at_the_name_u $this->put_other_font_at( $file ); $this->assertSame( '', $this->model->get_own_copy( $file ), 'Another font\'s file at the name' ); - $this->assertFalse( $this->model->is_installed( $file ) ); + $this->assertSame( '', $this->model->get_own_copy( $file ) ); $this->copy_fixture( $file, $hashed ); $this->assertSame( $this->font_path( $hashed ), $this->model->get_own_copy( $file ) ); - $this->assertTrue( $this->model->is_installed( $file ) ); + $this->assertNotSame( '', $this->model->get_own_copy( $file ) ); } public function test_a_hashed_name_stands_for_its_manifest_file_only_for_the_current_release() { @@ -1161,7 +1161,7 @@ public function test_a_renamed_file_is_not_renamed_again() { /* With no other name to take, the file at the name is the one used */ $this->copy_fixture( 'DejaVuSans.ttf', $renamed ); - $this->assertTrue( $model->is_installed( $renamed ) ); + $this->assertNotSame( '', $model->get_own_copy( $renamed ) ); } public function test_two_sources_using_one_name_for_the_same_file_share_it() { @@ -1205,17 +1205,17 @@ public function test_the_renamed_source_downloads_to_its_hashed_name() { public function test_a_file_is_another_fonts_when_its_size_is_no_known_build() { $model = $this->get_model_with_manifest( [ 'Mine.ttf' => $this->manifest_entry( 'DejaVuSans.ttf', 'https://example.com/Mine.ttf' ) ] ); - $this->assertFalse( $model->is_installed( 'Mine.ttf' ), 'Missing' ); + $this->assertSame( '', $model->get_own_copy( 'Mine.ttf' ), 'Missing' ); $this->copy_fixture( 'DejaVuSans.ttf', 'Mine.ttf' ); - $this->assertTrue( $model->is_installed( 'Mine.ttf' ) ); + $this->assertNotSame( '', $model->get_own_copy( 'Mine.ttf' ) ); $this->copy_fixture( 'Chewy.ttf', 'Mine.ttf' ); - $this->assertFalse( $model->is_installed( 'Mine.ttf' ) ); + $this->assertSame( '', $model->get_own_copy( 'Mine.ttf' ) ); $this->assertFileExists( $this->font_dir . 'Mine.ttf', 'The file at its name' ); $this->copy_fixture( 'DejaVuSans.ttf', $model->get_hashed_name( 'Mine.ttf' ) ); - $this->assertTrue( $model->is_installed( 'Mine.ttf' ) ); + $this->assertNotSame( '', $model->get_own_copy( 'Mine.ttf' ) ); $this->assertSame( $this->font_dir . $model->get_hashed_name( 'Mine.ttf' ), $model->get_own_copy( 'Mine.ttf' ) ); } @@ -1225,7 +1225,7 @@ public function test_an_older_build_of_a_file_has_a_known_size() { $this->copy_fixture( 'Chewy.ttf', 'Mine.ttf' ); - $this->assertTrue( $model->is_installed( 'Mine.ttf' ) ); + $this->assertNotSame( '', $model->get_own_copy( 'Mine.ttf' ) ); $this->assertCount( 2, $model->get_known_builds( 'Mine.ttf' ) ); } diff --git a/tests/phpunit/integration/Model/Test_Model_Required_Fonts.php b/tests/phpunit/integration/Model/Test_Model_Required_Fonts.php index 8af1f2f52..c9467616d 100644 --- a/tests/phpunit/integration/Model/Test_Model_Required_Fonts.php +++ b/tests/phpunit/integration/Model/Test_Model_Required_Fonts.php @@ -5,6 +5,7 @@ namespace GFPDF\Model; use GFPDF\Tests\Concerns\CopiesFontFixtures; +use GFPDF\Tests\Concerns\RecordsFontFailures; use GFPDF\Tests\Concerns\StubsFontSources; use GFPDF\Tests\Concerns\SwitchesNetwork; use GFPDF\Tests\Integration\TestCase; @@ -20,6 +21,7 @@ class Test_Model_Required_Fonts extends TestCase { use CopiesFontFixtures; + use RecordsFontFailures; use StubsFontSources; use SwitchesNetwork; @@ -40,6 +42,7 @@ public function set_up(): void { public function tear_down(): void { $this->remove_font_fixtures(); + $this->delete_font_failures(); GPDFAPI::get_options_class()->delete_option( 'default_font' ); /* A subsite leaves these behind */ @@ -290,6 +293,24 @@ public function test_another_fonts_file_at_a_name_is_not_an_installed_file() { $this->assertSame( 1, $this->required->get_installed_counts()['core']['installed'] ); } + public function test_another_fonts_file_a_failed_download_found_is_not_installed() { + GPDFAPI::get_options_class()->update_option( 'default_font', 'dejavuserifcondensed' ); + $this->put_same_size_font_at( self::FONT, self::FONT ); + $path = $this->font_path( self::FONT ); + $this->set_font_failures( [ self::FONT => $this->conflict_failure( $path ) ] ); + + $this->assertContains( self::FONT, $this->required->get_missing_files() ); + $this->assertSame( 0, $this->required->get_installed_counts()['core']['installed'] ); + $this->assertTrue( $this->required->needs_download( 'dejavuserifcondensed' ) ); + + /* A file copied over it after the failure is installed again */ + touch( $path, filemtime( $path ) + 10 ); + clearstatcache(); + + $this->assertNotContains( self::FONT, $this->required->get_missing_files() ); + $this->assertSame( 1, $this->required->get_installed_counts()['core']['installed'] ); + } + public function test_a_family_sharing_a_name_with_a_record_requires_its_manifest_file() { GPDFAPI::get_options_class()->update_option( 'default_font', 'dejavuserifcondensed' ); $this->put_other_font_at( self::FONT );