[google_sign_in_ios] Fix conflicting return type (BOOL vs void) in scene:openURLContexts: - #12777
Conversation
…atch FlutterSceneLifeCycleDelegate
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request modifies FLTGoogleSignInPlugin to return a BOOL from handleURLs: and scene:openURLContexts: to resolve a compilation warning. Feedback on the changes suggests replacing the logical OR assignment handled = [self.signIn handleURL:url] || handled; with an explicit if statement to avoid potential short-circuiting bugs if the expression is refactored in the future.
| BOOL handled = NO; | ||
| for (NSURL *url in urls) { | ||
| [self.signIn handleURL:url]; | ||
| handled = [self.signIn handleURL:url] || handled; |
There was a problem hiding this comment.
Relying on the left-hand side of the logical OR (||) operator to guarantee execution of a method with side-effects (handleURL:) is subtle and error-prone. If a future refactoring or automatic code formatter changes this to handled = handled || [self.signIn handleURL:url];, the short-circuiting behavior of || will prevent handleURL: from being called for subsequent URLs once handled becomes YES.
Using an explicit if statement is much safer, clearer, and less prone to accidental regression.
if ([self.signIn handleURL:url]) {
handled = YES;
}There was a problem hiding this comment.
Applied — replaced the short-circuit OR with an explicit if so the side-effecting handleURL: always runs regardless of prior results. Pushed in 5216498.
Fixes a compile warning surfaced when building with Flutter 3.44.8 / Xcode 26 (iOS 26 SDK):
Cause
FLTGoogleSignInPluginconforms to<FlutterSceneLifeCycleDelegate>and implements- (void)scene:openURLContexts:. In the Flutter iOS engine shipped with Flutter 3.44.8,FlutterSceneLifeCycleDelegate.hdeclares the method as:(documented as "@return
YESif this handled one or more of the URLs"), so the plugin'svoidimplementation conflicts with the protocol'sBOOLreturn type.Fix
handleURLs:now returnsBOOL(aggregating theFSIGIDSignIn handleURL:results with OR, mirroring the existing macOShandleOpenURLs:path).scene:openURLContexts:now returnsBOOLand propagateshandleURLs:, matching the protocol and the existingapplication:openURL:options:pattern.Verification
scene:openURLContexts:/handleURLs:occurrence in the package (only the plugin.mand the test-only header expose these; the existing Swift testhandleURLs()still compiles since it ignores the return value).FlutterSceneLifeCycleDelegate.hshipped with Flutter 3.44.8 viaclang -fsyntax-only(exit 0).Detected with Flutter 3.44.8 / Xcode 26 (iOS 26 SDK). Note: PR #12655 is migrating this plugin class to Swift; this change is complementary and scoped to the current Objective-C code.