fix: resolve application log filenames against their directory - #26010
Open
jkmassel wants to merge 1 commit into
Open
fix: resolve application log filenames against their directory#26010jkmassel wants to merge 1 commit into
jkmassel wants to merge 1 commit into
Conversation
readFiles(in:) passed contentsOfDirectory(atPath:) results straight to ApplicationLog(filePath:). Those results are bare filenames, not paths, so attributesOfItem(atPath:) could not find them and the call threw on the first entry. No shipping conformer calls this default implementation today, so the failure was latent.
Contributor
|
| App Name | Jetpack | |
| Configuration | Release-Alpha | |
| Build Number | 34434 | |
| Version | PR #26010 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 007e265 | |
| Installation URL | 0u9hdo1go8mq8 |
Contributor
|
| App Name | WordPress | |
| Configuration | Release-Alpha | |
| Build Number | 34434 | |
| Version | PR #26010 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 007e265 | |
| Installation URL | 30sq96o00fo80 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.


Fixes a bug where
ApplicationLogDataProvider.readFiles(in:)throws on the first entry in any non-empty directory.Found while sweeping for the
URL.path()shape from #26005. Unrelated to that bug, so it's on its own.Summary
contentsOfDirectory(atPath:)returns bare filenames, not paths.ApplicationLog(filePath:), which callsattributesOfItem(atPath:)— so the lookup resolved against the process's working directory rather than the directory that was passed in, and threw.WpLogDataProvidergets its logs from CocoaLumberjack'ssortedLogFileInfos, which are already full paths.Root Cause
Modules/Sources/Support/SupportDataProvider.swift
The closure parameter is named
filePath, butcontentsOfDirectory(atPath:)documents its return as "the names of the items" —a.log, not/…/logs/a.log.ApplicationLog(filePath:)isinit?and throwing, and the missing-file case is the throwing one, so this doesn't degrade to an empty result — it propagates out of thecompactMapand fails the whole call.Fix
Resolve each name against
directorybefore handing it toApplicationLog, and rename the closure parameter tofilenameso the next reader isn't told it's a path.Test plan
ApplicationLogDataProviderTests— five cases: reads every log in a directory, resolves each path against that directory, handles a filename with a space, round-trips the contents through the defaultreadApplicationLog, and returns nothing for an empty directory.WordPressTest/ApplicationLogDataProviderTests— 5/5 pass on an iOS 26.4 simulator.The
Supportmodule has no test target of its own, so the tests live inKeystoneTests, which already imports it. Adding a target for one default implementation looked disproportionate; happy to add one if you'd rather.