[go: nahoru, domu]
Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

[google_sign_in] Fix deprecated API usage issue by upgrading CocoaPod to 5.0 #2127

Merged
merged 10 commits into from
Oct 29, 2019
Merged

[google_sign_in] Fix deprecated API usage issue by upgrading CocoaPod to 5.0 #2127

merged 10 commits into from
Oct 29, 2019

Conversation

collinjackson
Copy link
Contributor
@collinjackson collinjackson commented Sep 30, 2019 •
edited
Loading

Description

Fixes deprecated API issue when uploading to App Store. ("Apple will stop accepting submissions of apps that use UIWebView APIs")

This requires updating the plugin to deal with some breaking API changes.

Fixes flutter/flutter#39470

Related Issues

firebase/flutterfire#62

amirh
amirh approved these changes Sep 30, 2019
View reviewed changes
Copy link
Contributor
@amirh amirh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSLGTM

@collinjackson collinjackson changed the title Fix Deprecated API Usage issue Fix deprecated API usage issue Sep 30, 2019
@collinjackson collinjackson changed the title Fix deprecated API usage issue [google_sign_in] Fix deprecated API usage issue by upgrading Google Sign-In CocoaPod to 5.0 Sep 30, 2019
@collinjackson collinjackson changed the title [google_sign_in] Fix deprecated API usage issue by upgrading Google Sign-In CocoaPod to 5.0 [google_sign_in] Fix deprecated API usage issue by upgrading CocoaPod to 5.0 Sep 30, 2019
jmagman
jmagman suggested changes Oct 1, 2019
View reviewed changes
packages/google_sign_in/ios/Classes/GoogleSignInPlugin.m Outdated
[registrar addApplicationDelegate:instance];
[registrar addMethodCallDelegate:instance channel:channel];
}

- (instancetype)init {
- (instancetype)initWithController:(id)controller {
Copy link
Member
@jmagman jmagman Oct 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes the designated initializer from -init to -initWithController:

GoogleSignInPlugin.h can indicate this:

#import <Flutter/Flutter.h>

NS_ASSUME_NONNULL_BEGIN

@interface FLTGoogleSignInPlugin : NSObject <FlutterPlugin>

- (instancetype)initWithController:(id)controller NS_DESIGNATED_INITIALIZER;
- (instancetype)init NS_UNAVAILABLE;

@end

NS_ASSUME_NONNULL_END

Also, it looks like this should be a UIViewController instead of an id (meaning a type check on [registar.messenger]).

packages/google_sign_in/ios/Classes/GoogleSignInPlugin.m Outdated
self = [super init];
if (self) {
[GIDSignIn sharedInstance].delegate = self;
[GIDSignIn sharedInstance].uiDelegate = self;
[GIDSignIn sharedInstance].presentingViewController = controller;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That header says:

// The view controller used to present |SFSafariViewContoller| on iOS 9 and 10.
@property(nonatomic, weak) UIViewController *presentingViewController;

Does that mean this is only applicable for iOS 9 and 10?

@matthew-carroll
Copy link
Contributor

@gaaclarke is probably the right person to make a call on the API/usage.

gaaclarke
gaaclarke reviewed Oct 1, 2019
View reviewed changes
packages/google_sign_in/ios/Classes/GoogleSignInPlugin.m Outdated Show resolved Hide resolved
packages/google_sign_in/ios/Classes/GoogleSignInPlugin.m Outdated
@@ -40,16 +40,16 @@ + (void)registerWithRegistrar:(NSObject<FlutterPluginRegistrar> *)registrar {
FlutterMethodChannel *channel =
[FlutterMethodChannel methodChannelWithName:@"plugins.flutter.io/google_sign_in"
binaryMessenger:[registrar messenger]];
FLTGoogleSignInPlugin *instance = [[FLTGoogleSignInPlugin alloc] init];
FLTGoogleSignInPlugin *instance = [[FLTGoogleSignInPlugin alloc] initWithController:[registrar messenger]];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initWithController:[registrar messenger]]

This is going to be a problem for you. You are ignoring types and forcing a binary messenger to be a view controller? Once you fix that, you'll find that the binary messenger isn't really a view controller anymore.

Copy link
Contributor Author
@collinjackson collinjackson Oct 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I do something like this?

    UIViewController *controller = [UIApplication sharedApplication].keyWindow.rootViewController;	
    [GIDSignIn sharedInstance].presentingViewController = controller;

Or should the app developer manually pass their FlutterViewController to the plugin? I'm not sure what the best approach is here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if grabbing the rootViewController here is going to work for you since the rootViewController might not still be the same rootViewController when GIDSignIn actually wants to present something. Also, I'm not sure what it would do if the rootViewController was a UINavigationController and its topmost view is a FlutterViewController, would it display correctly? You might have to try that out.

You might want to move the setter to the handle method call, is it possible GIDSignIn will want to display something without a method having triggered it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, i don't know if sending the FlutterViewController will work since I don't think there is a mechanism to do that?

Copy link
Contributor Author
@collinjackson collinjackson Oct 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that there probably isn't a mechanism to that.

I'm fine with grabbing the rootViewController at the time that the sign-in happens if you think that's a reasonable approach. It seems like the sign-in method is the only time that this gets used.

Copy link
Contributor Author
@collinjackson collinjackson Oct 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the approach as described, PTAL

@jmagman jmagman mentioned this pull request Oct 1, 2019
Closed
@cyanglaz
Copy link
Contributor
cyanglaz commented Oct 3, 2019

This might help with getting the top most view controller.
https://github.com/flutter/plugins/blob/master/packages/url_launcher/url_launcher/ios/Classes/UrlLauncherPlugin.m#L156-L182

@lookfirst
Copy link

🏓

Knupper
Knupper reviewed Oct 17, 2019
View reviewed changes
packages/google_sign_in/pubspec.yaml Outdated
@@ -3,7 +3,7 @@ description: Flutter plugin for Google Sign-In, a secure authentication system
for signing in with a Google account on Android and iOS.
author: Flutter Team <flutter-dev@googlegroups.com>
homepage: https://github.com/flutter/plugins/tree/master/packages/google_sign_in
version: 4.0.9
version: 4.0.10
Copy link
@Knupper Knupper Oct 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we not update the version to 5.x.x as well to indicate wich GoogleSignIn Version is used inside?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally we don't update the Dart and wrapped SDK versions in lockstep. In this case, we're not making a breaking change to the semantics of the Dart plugin, so I don't think a major version increment is needed.

@collinjackson
Copy link
Contributor Author

This might help with getting the top most view controller.
https://github.com/flutter/plugins/blob/master/packages/url_launcher/url_launcher/ios/Classes/UrlLauncherPlugin.m#L156-L182

Thanks, this was useful. Can you take a look at the updated PR and let me know if this looks right?

jmagman
jmagman approved these changes Oct 23, 2019
View reviewed changes
Copy link
Member
@jmagman jmagman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@collinjackson collinjackson added the submit queue The Flutter team is in the process of landing this PR. label Oct 24, 2019
@PerLycke PerLycke mentioned this pull request Oct 28, 2019
Closed
@collinjackson
Copy link
Contributor Author

I tested this by hand by creating my own GoogleService-Info.plist following the instructions in the README and it worked. We should be testing this on CI but our infrastructure isn't there on iOS yet.

I filed flutter/flutter#43680 for addressing automated testing of this plugin.

@collinjackson collinjackson merged commit 4c793a3 into flutter:master Oct 29, 2019
@collinjackson collinjackson deleted the google_sign_in branch October 29, 2019 23:29
mormih pushed a commit to mormih/plugins that referenced this pull request Nov 17, 2019
@collinjackson @mormih
[google_sign_in] Fix deprecated API usage issue by upgrading CocoaPod…
3947eba 
… to 5.0 (flutter#2127)

* Fix Deprecated API Usage issue
* Update to 5.0 compatibility
@zmeggyesi
Copy link

Am I understanding it right that the deprecation warning should go away now that this branch was merged?

I tried upgrading the firebase_auth plugin to 0.15.0, but the warning remains - perhaps I need to load a new version of Flutter as well (currently 1.9.1+6)?

@lookfirst
Copy link 

/flutter/.pub-cache/hosted/pub.dartlang.org/google_sign_in-4.0.14/ios/Classes/GoogleSignInPlugin.m:138:13: warning: unused variable 'sourceApplication' [-Wunused-variable]
      NSString *sourceApplication = options[UIApplicationOpenURLOptionsSourceApplicationKey];
                ^
/flutter/.pub-cache/hosted/pub.dartlang.org/google_sign_in-4.0.14/ios/Classes/GoogleSignInPlugin.m:138:41: warning: 'UIApplicationOpenURLOptionsSourceApplicationKey' is only available on iOS 9.0 or newer [-Wunguarded-availability]
      NSString *sourceApplication = options[UIApplicationOpenURLOptionsSourceApplicationKey];
                                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    In module 'UIKit' imported fromios/Pods/Target Support Files/google_sign_in/google_sign_in-prefix.pch:2:
    /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS13.2.sdk/System/Library/Frameworks/UIKit.framework/Headers/UIApplication.h:518:51: note: 'UIApplicationOpenURLOptionsSourceApplicationKey' has been marked as being introduced in iOS 9.0 here, but the deployment target is iOS 8.0.0
    UIKIT_EXTERN UIApplicationOpenURLOptionsKey const UIApplicationOpenURLOptionsSourceApplicationKey NS_SWIFT_NAME(sourceApplication) API_AVAILABLE(ios(9.0));   // value is an NSString containing the bundle ID of the originating application; non-nil if the originating application and this application share the same team identifier
                                                      ^
/flutter/.pub-cache/hosted/pub.dartlang.org/google_sign_in-4.0.14/ios/Classes/GoogleSignInPlugin.m:138:41: note: enclose 'UIApplicationOpenURLOptionsSourceApplicationKey' in an @available check to silence this warning
      NSString *sourceApplication = options[UIApplicationOpenURLOptionsSourceApplicationKey];
                                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    2 warnings generated.

@PerLycke
Copy link
Contributor

@zmeggyesi Do you have any other plugin making use of UIWebView? Regarding google_sign_in it should not use it after this merge so might be another plugin causing it for you

@zmeggyesi
Copy link

@PerLycke I don't think I should. Is there a one-liner way to list such dependencies (I should think not, but I thought I'd ask)?

In any case, these are all that I have in my Pubspec:

dependencies:
  shared_preferences: 0.5.3+4
  protobuf: 1.0.0
  google_maps_flutter: 0.5.21+7
  incrementally_loading_listview: 0.2.0+2
  location: 2.3.5
  firebase_crashlytics: 0.1.1+2
  firebase_core: 0.4.0+9
  firebase_auth: 0.15.0
  firebase_admob: 0.9.0+7
  firebase_remote_config: 0.2.0+7
  google_sign_in: 4.0.7
  http: 0.12.0+2
  logging: 0.11.3+2
  uuid: 2.0.2
  admob_flutter:
    git:
      url: https://github.com/zmeggyesi/admob_flutter.git
      ref: targeting
  equatable:
  provider:
  flutter:
    sdk: flutter

  cupertino_icons: ^0.1.2

admob_flutter might be suspect, but I checked my fork, and couldn't see any references there either.

@jayjun jayjun mentioned this pull request Dec 5, 2019
Merged
sungmin-park pushed a commit to sungmin-park/flutter-plugins that referenced this pull request Dec 17, 2019
@collinjackson
[google_sign_in] Fix deprecated API usage issue by upgrading CocoaPod…
429fe61 
… to 5.0 (flutter#2127)

* Fix Deprecated API Usage issue
* Update to 5.0 compatibility
@jmagman
Copy link
Member
jmagman commented Apr 27, 2020

This is a merged PR. If you are experiencing a bug, please file a new GitHub issue so the right team can take a look. Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Knupper Knupper Knupper left review comments

@gaaclarke gaaclarke gaaclarke left review comments

@jmagman jmagman jmagman approved these changes

@amirh amirh amirh approved these changes

@cyanglaz cyanglaz Awaiting requested review from cyanglaz

@mehmetf mehmetf Awaiting requested review from mehmetf

@matthew-carroll matthew-carroll Awaiting requested review from matthew-carroll

Assignees
No one assigned
Labels
cla: yes submit queue The Flutter team is in the process of landing this PR.
Projects
None yet
Milestone
No milestone