diff --git a/src/Model/Model_Font_Downloader.php b/src/Model/Model_Font_Downloader.php index cd59be24a..8849fff72 100644 --- a/src/Model/Model_Font_Downloader.php +++ b/src/Model/Model_Font_Downloader.php @@ -310,13 +310,15 @@ public function download( string $file, bool $verify = false ): bool { return false; } - if ( $this->font_sources->has_hashed_copy( $file ) ) { - return true; - } - $font_path = $this->font_sources->get_plain_path( $file ); $conflict = is_file( $font_path ); - if ( $conflict && ! $this->is_conflict( $file, $verify && $this->is_auto_install_enabled( $file ) ) ) { + + $own_copy = $this->font_sources->has_hashed_copy( $file ) || ( $conflict && ! $this->is_conflict( $file, $verify && $this->is_auto_install_enabled( $file ) ) ); + + /* Copied in by hand or saved by another request, so any failure on record is stale */ + if ( $own_copy ) { + $this->clear_failure( $file ); + return true; } @@ -403,15 +405,6 @@ protected function hash_blob( string $path, int $size ): string { return Git_Blob::sha1( $path, $size ); } - /** - * Whether the file's own copy is on disk, hashing the file at its name to be sure it's the manifest's - * - * @since 6.19.0 - */ - protected function is_resolved( string $file ): bool { - return $this->font_sources->has_hashed_copy( $file ) || ( is_file( $this->font_sources->get_plain_path( $file ) ) && ! $this->is_conflict( $file, true ) ); - } - /** * Delete the unmodified files (fonts and licences) the Core Font installer downloaded that aren't kept. A file whose * download is turned off stays, as it couldn't come back. Only files of the installer's size are hashed. @@ -1231,8 +1224,7 @@ function ( $file ) use ( $gave_up ) { } /** - * The files whose one retry is due, and when the next one still to come is (or 0). A due file already on disk, - * copied in by hand, no longer needs its retry. + * The files whose one retry is due, and when the next one still to come is (or 0) * * @return array{0: string[], 1: int} * @@ -1250,8 +1242,6 @@ protected function get_retries(): array { if ( $failure['retry_at'] > time() ) { $next = $next === 0 ? $failure['retry_at'] : min( $next, $failure['retry_at'] ); - } elseif ( $this->is_resolved( $file ) ) { - $this->clear_failure( $file ); } else { $due[] = $file; } @@ -1340,8 +1330,16 @@ protected function save( string $file, string $font_path, array $entry ): bool { } if ( ! $this->move( $download_path, $font_path ) ) { + /* The right font is there anyway, as when another request saved it first */ + if ( Git_Blob::matches( $font_path, $entry['size'], $entry['sha'] ) ) { + wp_delete_file( $download_path ); + $this->log->notice( 'Font already in place after a failed move', [ 'name' => $file ] ); + + return true; + } + /* A move across filesystems copies, so a failure can leave a partial font behind */ - if ( is_file( $font_path ) && ! Git_Blob::matches( $font_path, $entry['size'], $entry['sha'] ) ) { + if ( is_file( $font_path ) ) { wp_delete_file( $font_path ); } diff --git a/tests/phpunit/integration/Model/Test_Model_Font_Downloader.php b/tests/phpunit/integration/Model/Test_Model_Font_Downloader.php index 24427c663..5dfa13cca 100644 --- a/tests/phpunit/integration/Model/Test_Model_Font_Downloader.php +++ b/tests/phpunit/integration/Model/Test_Model_Font_Downloader.php @@ -236,8 +236,51 @@ public function test_a_failed_move_keeps_a_valid_font_another_request_saved() { $downloader = $this->new_downloader( file_get_contents( $this->font_fixture( self::FONT ) ) ); $this->serve_font_downloads(); - $this->assertFalse( $downloader->download( self::FONT ) ); + $this->assertTrue( $downloader->download( self::FONT ) ); $this->assertFileEquals( $this->font_fixture( self::FONT ), $this->font_path( self::FONT ) ); + $this->assertNoStrayTmpFiles(); + $this->assertEmpty( $this->get_font_failures() ); + $this->assertCount( 1, $this->log_messages( 'Font already in place after a failed move' ) ); + } + + public function test_a_font_on_disk_clears_its_stale_failure() { + $this->set_font_failures( [ self::FONT => $this->failure( time() ) ] ); + $this->set_back_off( self::FONT ); + $this->copy_fixture( self::FONT ); + $this->serve_font_downloads(); + + $this->assertTrue( $this->downloader->download( self::FONT ) ); + + $this->assertSame( [], $this->requested ); + $this->assertEmpty( $this->get_font_failures() ); + $this->assertFalse( $this->get_back_off( self::FONT ) ); + } + + public function test_a_failed_move_to_a_hashed_copy_another_request_saved_is_not_a_failure() { + $this->put_other_font_at( self::FONT ); + $hashed = GPDFAPI::get_mvc_class( 'Model_Font_Sources' )->get_hashed_name( self::FONT ); + $downloader = $this->new_downloader( file_get_contents( $this->font_fixture( self::FONT ) ) ); + $this->serve_font_downloads(); + + $this->assertTrue( $downloader->download( self::FONT ) ); + $this->assertFileEquals( $this->font_fixture( self::FONT ), $this->font_path( $hashed ) ); + $this->assertFileEquals( $this->font_fixture( 'Chewy.ttf' ), $this->font_path( self::FONT ), 'The other font is left alone' ); + $this->assertNoStrayTmpFiles(); + $this->assertEmpty( $this->get_font_failures() ); + $this->assertCount( 1, $this->log_messages( 'Font already in place after a failed move' ) ); + } + + public function test_a_hashed_copy_on_disk_clears_its_stale_failure() { + $hashed = GPDFAPI::get_mvc_class( 'Model_Font_Sources' )->get_hashed_name( self::FONT ); + $this->set_font_failures( [ self::FONT => $this->failure( time() ) ] ); + $this->put_other_font_at( self::FONT ); + $this->copy_fixture( self::FONT, $hashed ); + $this->serve_font_downloads(); + + $this->assertTrue( $this->downloader->download( self::FONT ) ); + + $this->assertSame( [], $this->requested ); + $this->assertEmpty( $this->get_font_failures() ); } /**