Extract SFHFKeychainUtils to dedicated Swift package #24192
Merged
Conversation
This file contains 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
All those Swift files compile without importing UIKit because `SFHFKeychainUtils.h` did that for them and they got access to it via the bridging header. Next, we'll move SFHFKeychainUtils into the modules, and UIKit will no longer be available through it. Side note: UIKit should not have been available via SFHFKeychainUtils in the first place. On top of that, SFHFKeychainUtils does not need UIKit either.
mokagio
changed the title
Generated by 🚫 Danger |
mokagio
commented
Mar 12, 2025
| #import <UIKit/UIKit.h> | ||
| #import <Foundation/Foundation.h> |
There was a problem hiding this comment.
UIKit was not needed.
Also, you'll see many import UIKit in the diff. All those Swift files compiled before without importing UIKit because SFHFKeychainUtils.h did that for them and they got access to it via the bridging header.
I find it quite interesting that a seldom visited file had such an impact on unrelate files that did not even use its functionality.
mokagio
commented
Mar 12, 2025
| @@ -1,6 +1,7 @@ | |||
| // TODO: Port to yet to create SFHFKeychainUtils test package | |||
There was a problem hiding this comment.
Waiting for feedback on this PR. Then, if the setup remains the same, I'll open an issue to remember to do this.
|
| App Name |  WordPress Alpha |
|
| Configuration | Release-Alpha | |
| Build Number | pr24192-45a98f7 | |
| Version | 25.8 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 45a98f7 | |
| App Center Build | WPiOS - One-Offs #11643 |
We need it in a dedicated package because the Objective-C implementation does not use ARC. Therefore, we need to use the `-fno-objc-arc` compiler setting. But such a setting should be applied sparingly so instead of blanket using it in an existing Objective-C package like WordPressSharedObjC, we use it surgically just for this file.
mokagio
force-pushed
the
mokagio/SFHFKeychainUtils-wordpressshared
branch
from
95580ff to
45a98f7
Compare
|
| App Name |  Jetpack Alpha |
|
| Configuration | Release-Alpha | |
| Build Number | pr24192-45a98f7 | |
| Version | 25.8 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 45a98f7 | |
| App Center Build | jetpack-installable-builds #10672 |
mokagio
requested review from
crazytonyli and
kean
and removed request for
crazytonyli
|
And I need this too for my work on app extensions – merging 🙇 |
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.
Problem Some of the files we are moving out of the WordPress/Jetpack target to WordPressData access the keychain via
SFHFKeychainUtils, but we cannot move out the utility itself because other files unrelated to storage use it, too.Solution Move
SFHFKeychainUtilsto a shared location.Given the object is implemented in Objective-C, I initially tried to move it to WordPressSharedObjC, but this failed because
SFHFKeychainUtilsis so old it does not use ARC.I decided not to go down the rabbit hole of rewriting it to support ARC (maybe I should have) and instead moved it into a dedicated package with the
-fno-objc-arccompiler flag.Testing
I run the WordPress app on my iPhone and smoke tested it checking all tabs.
Part of #24165