Skip to content

Commit ec446c3

Browse files
authored
Always call the thumbnail URL completion handler (#26047)
Creating the thumbnail symlink could throw inside a discarded task, dropping the completion so the editor placeholder never appeared. The symlink also lived in tmp, so the thumbnail path saved on the media pointed at a missing file and reopened drafts lost their placeholder. Return a copy of the cached thumbnail in the media cache directory instead, and report a failure with a nil URL.
1 parent d569a17 commit ec446c3

2 files changed

Lines changed: 73 additions & 11 deletions

File tree

‎Tests/KeystoneTests/Tests/Models/MediaImageServiceTests.swift‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -198,6 +198,62 @@ class MediaImageServiceTests: CoreDataTestCase {
198198
XCTAssertEqual(cachedThumbnail.size.height, expectedSize.height, accuracy: 1.5)
199199
}
200200

201+
// MARK: - Thumbnail URL
202+
203+
func testThatSavedThumbnailURLPointsAtAFile() throws {
204+
// GIVEN
205+
let media = try makeLocalImageMedia()
206+
207+
// WHEN the URL is saved the way `MediaImportService` saves it
208+
let thumbnailURL = try XCTUnwrap(getThumbnailURL(for: media))
209+
media.absoluteThumbnailLocalURL = thumbnailURL
210+
211+
// THEN it has an extension, and the saved path points at a file
212+
XCTAssertEqual(thumbnailURL.pathExtension, "jpeg")
213+
let savedURL = try XCTUnwrap(media.absoluteThumbnailLocalURL)
214+
XCTAssertTrue(FileManager.default.fileExists(atPath: savedURL.path))
215+
}
216+
217+
func testThatThumbnailURLIsReturnedForRepeatedCalls() throws {
218+
// GIVEN
219+
let media = try makeLocalImageMedia()
220+
221+
// WHEN the same media is requested twice
222+
let firstURL = try XCTUnwrap(getThumbnailURL(for: media))
223+
let secondURL = try XCTUnwrap(getThumbnailURL(for: media))
224+
225+
// THEN both calls return a file
226+
XCTAssertTrue(FileManager.default.fileExists(atPath: firstURL.path))
227+
XCTAssertTrue(FileManager.default.fileExists(atPath: secondURL.path))
228+
}
229+
230+
private func makeLocalImageMedia() throws -> Media {
231+
let media = Media(context: mainContext)
232+
media.blog = makeEmptyBlog()
233+
media.mediaType = .image
234+
media.width = 1024
235+
media.height = 680
236+
media.absoluteLocalURL = try makeLocalURL(forResource: "test-image", fileExtension: "jpg")
237+
try mainContext.save()
238+
return media
239+
}
240+
241+
/// Waits with a timeout, so a dropped completion fails the test instead of
242+
/// hanging the test run.
243+
private func getThumbnailURL(for media: Media) -> URL? {
244+
let expectation = expectation(description: "thumbnail URL")
245+
var thumbnailURL: URL?
246+
sut.getThumbnailURL(for: media) {
247+
thumbnailURL = $0
248+
expectation.fulfill()
249+
}
250+
wait(for: [expectation], timeout: 10)
251+
if let thumbnailURL {
252+
addTeardownBlock { try? FileManager.default.removeItem(at: thumbnailURL) }
253+
}
254+
return thumbnailURL
255+
}
256+
201257
// MARK: - Target Size
202258

203259
func testThatLandscapeImageIsResizedToFillTargetSize() {

‎WordPress/Classes/Services/MediaImageService.swift‎

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -222,18 +222,24 @@ final class MediaImageService {
222222
return completion(nil)
223223
}
224224

225-
// Fixes the following React Native issue https://github.com/facebook/react-native/issues/42234
226-
// where it'll fail to display local images with no path extension.
225+
// React Native won't display local files without an extension
226+
// (https://github.com/facebook/react-native/issues/42234), so return
227+
// a copy of the cached thumbnail under the export's original name.
228+
// The copy stays in the media cache directory, where
229+
// `Media.absoluteThumbnailLocalURL` resolves saved file names.
230+
// On APFS the copy is a clone that shares storage with the cached file.
227231
//
228-
// This workaround creates a temporary symlink that has a file extension.
229-
let tempDirectoryURL = URL.Helpers.temporaryDirectory(named: "org.automattic.MediaImageServiceSymlinks")
230-
try? FileManager.default.createDirectory(at: tempDirectoryURL, withIntermediateDirectories: true)
231-
232-
let symlink = tempDirectoryURL
233-
.appendingPathComponent(export.url.lastPathComponent)
234-
try FileManager.default.createSymbolicLink(at: symlink, withDestinationURL: thumbnailURL)
235-
236-
completion(symlink)
232+
// The export file still exists when it couldn't be moved to the cache
233+
// because a cached thumbnail already existed.
234+
do {
235+
if !FileManager.default.fileExists(atPath: export.url.path) {
236+
try FileManager.default.copyItem(at: thumbnailURL, to: export.url)
237+
}
238+
completion(export.url)
239+
} catch {
240+
Loggers.app.error("Failed to copy the thumbnail: \(error)")
241+
completion(nil)
242+
}
237243
}
238244
}
239245

0 commit comments

Comments
 (0)