Removed the email warning about notifications entitlement #29996

gaaclarke · 2021-11-30T00:42:16Z

This removes explicit references to application:didReceiveRemoteNotification:fetchCompletionHandler: from the engine and adds it at runtime. This will remove the false positive emails users receive about entitlements. If the user is using a plugin that implements -[NSPlugin application:didReceiveRemoteNotification:fetchCompletionHandler:] that should trigger the warning email since that plugin will be linked in and that symbol will show up for the plugin.

This is based off of what firebase does: https://github.com/firebase/firebase-ios-sdk/blob/7eb1747e4b9be195ea9908b9da28b1d0f818afe3/GoogleUtilities/AppDelegateSwizzler/GULAppDelegateSwizzler.m#L98

fixes flutter/flutter#9984

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

gaaclarke · 2021-11-30T00:51:13Z

I haven't tested it by uploading an app yet. The remedy is based on the information linked in the issue. The problematic selector never shows up as a symbol in the engine and is never created with an @selector, it only appears as a string literal. That could potentially still trigger the warning, but that doesn't seem to be the case based on what people have said in the issue and the firebase code.

jmagman

Is what was there before even work?
flutter/flutter#52895
https://github.com/Bathant/PushNotificationFlutterExample/blob/master/ios/Runner/AppDelegate.swift
https://stackoverflow.com/questions/60760123/flutter-didreceiveremotenotification-not-called

shell/platform/darwin/ios/framework/Source/FlutterPluginAppLifeCycleDelegate.mm

jmagman · 2021-11-30T01:34:12Z

shell/platform/darwin/ios/framework/Source/FlutterPluginAppLifeCycleDelegate.mm

 static const SEL selectorsHandledByPlugins[] = {
-    @selector(application:didReceiveRemoteNotification:fetchCompletionHandler:),
+    NSSelectorFromString(kApplicationDidReceiveRemoteNotificationFetchCompletionHandler),
    @selector(application:performFetchWithCompletionHandler:)};


Does application:performFetchWithCompletionHandler: have a similar problem for the fetch background mode?

I don't believe so. It isn't listed in the issue or the pasted in message from the App Store. The documentation for it doesn't list a need for entitlements.

jmagman · 2021-11-30T01:40:28Z

shell/platform/darwin/ios/framework/Headers/FlutterPluginAppLifeCycleDelegate.h

 * Calls all plugins registered for `UIApplicationDelegate` callbacks.
 */
- (void)application:(UIApplication*)application
+- (void)performApplication:(UIApplication*)application


Wouldn't this be a breaking change for plugins that have implemented this method?

What about application:didRegisterForRemoteNotificationsWithDeviceToken? didFailToRegisterForRemoteNotificationsWithError? etc
https://github.com/firebase/firebase-ios-sdk/blob/7eb1747e4b9be195ea9908b9da28b1d0f818afe3/GoogleUtilities/AppDelegateSwizzler/GULAppDelegateSwizzler.m#L111-L112

Wouldn't this be a breaking change for plugins that have implemented this method?

I'm not sure what you have in mind. This is on the FlutterPluginAppLifeCycleDelegate. Do you mean the usage of _cmd? The modification of the class at runtime takes care of that as demonstrated in the test.

application:didRegisterForRemoteNotificationsWithDeviceToken

Yea, I'll do that one too, thanks.

Wouldn't this be a breaking change for plugins that have implemented this method?

If a plugin has application:didReceiveRemoteNotification:fetchCompletionHandler: implemented like this:
https://github.com/blackmenthor/flutter-branch-io/blob/39746a23dcacca266cc8af98efda7687643bed89/ios/Classes/SwiftFlutterBranchIoPlugin.swift#L173-L176

It's not implementing performApplication:didReceiveRemoteNotification:fetchCompletionHandler:, but the implementation has been swapped out in +load. Would their implementation be called? Is there a way we can make sure that delegates that already respond to application:didReceiveRemoteNotification:fetchCompletionHandler: don't break?

Hmm, let me write up some tests for that use case. It's hard to know what will happen without trying it.

I've added application:didRegisterForRemoteNotificationsWithDeviceToken and backfilled tests to FlutterAppDelegate.

Firebase also did didFailToRegisterForRemoteNotificationsWithError I'll do that one too.

Okay, so I looked into the use case for subclassing FlutterPluginAppLifeCycleDelegate like in the example provided.
It is using the override annotation so removing application:didReceiveRemoteNotification:fetchCompletionHandler: will be a problem since its removal is necessary in order to fix the problem. I've added a test to show that subclassing and overriding performApplication:didReceiveRemoteNotification:fetchCompletionHandler: behaves as expected. So this plugin will break in the best way possible, at compilation time when the override annotation doesn't make any sense. They'll have to rename their method to have the perform prefix. For objc subclasses this shouldn't be a problem since there is no "override" analog. I don't believe this is technically a breaking change since it doesn't break any tests. I'm not sure if this is different criteria for plugins.

Cross-referencing firebase/firebase-ios-sdk#2835

So this plugin will break in the best way possible, at compilation time when the override annotation doesn't make any sense.

Fortunately I doubt this feature is used by many plugins.

For objc subclasses this shouldn't be a problem since there is no "override" analog.

The problem there is that it wouldn't break, and they wouldn't know they need to rename the method. Could we detect that case in +load and NSAssert? Or just avoid the swapping?

I don't believe this is technically a breaking change since it doesn't break any tests

A remote notification test would be difficult, but if we had one it would break, but point taken.

Added didFailToRegisterForRemoteNotificationsWithError.

The problem there is that it wouldn't break, and they wouldn't know they need to rename the method. Could we detect that case in +load and NSAssert? Or just avoid the swapping?

For objc the hypothetical subclasser wouldn't have to rename it, it would just work if the implementation is like the example provided above. If they are calling [super application:didReceiveRemoteNotification:fetchCompletionHandler:] in the subclass it will create a warning or error depending on their compilation settings but would execute fine.

shell/platform/darwin/ios/framework/Source/FlutterAppDelegate.mm

jmagman

Is what was there before even work? flutter/flutter#52895 https://github.com/Bathant/PushNotificationFlutterExample/blob/master/ios/Runner/AppDelegate.swift https://stackoverflow.com/questions/60760123/flutter-didreceiveremotenotification-not-called

I'd like more validation that this actually works since it seems like what's on main may not actually work? This PR is hopefully fixing the validation error, but if the feature doesn't work at all we should either fix it or maybe delete it instead of all the swizzling.

jmagman · 2021-11-30T17:49:40Z

shell/platform/darwin/ios/framework/Headers/FlutterPluginAppLifeCycleDelegate.h

 * Calls all plugins registered for `UIApplicationDelegate` callbacks.
 */
- (void)application:(UIApplication*)application
+- (void)performApplication:(UIApplication*)application


Wouldn't this be a breaking change for plugins that have implemented this method?

If a plugin has application:didReceiveRemoteNotification:fetchCompletionHandler: implemented like this:
https://github.com/blackmenthor/flutter-branch-io/blob/39746a23dcacca266cc8af98efda7687643bed89/ios/Classes/SwiftFlutterBranchIoPlugin.swift#L173-L176

It's not implementing performApplication:didReceiveRemoteNotification:fetchCompletionHandler:, but the implementation has been swapped out in +load. Would their implementation be called? Is there a way we can make sure that delegates that already respond to application:didReceiveRemoteNotification:fetchCompletionHandler: don't break?

shell/platform/darwin/ios/framework/Source/FlutterAppDelegate.mm

jmagman · 2021-11-30T19:05:04Z

shell/platform/darwin/ios/framework/Headers/FlutterPluginAppLifeCycleDelegate.h

 * Calls all plugins registered for `UIApplicationDelegate` callbacks.
 */
- (void)application:(UIApplication*)application
+- (void)performApplication:(UIApplication*)application


Cross-referencing firebase/firebase-ios-sdk#2835

So this plugin will break in the best way possible, at compilation time when the override annotation doesn't make any sense.

Fortunately I doubt this feature is used by many plugins.

For objc subclasses this shouldn't be a problem since there is no "override" analog.

The problem there is that it wouldn't break, and they wouldn't know they need to rename the method. Could we detect that case in +load and NSAssert? Or just avoid the swapping?

I don't believe this is technically a breaking change since it doesn't break any tests

A remote notification test would be difficult, but if we had one it would break, but point taken.

shell/platform/darwin/ios/framework/Source/FlutterPluginAppLifeCycleDelegate.mm

gaaclarke · 2021-11-30T19:35:42Z

I'd like more validation that this actually works since it seems like what's on main may not actually work? This PR is hopefully fixing the validation error, but if the feature doesn't work at all we should either fix it or maybe delete it instead of all the swizzling.

I've added backfill tests for the FlutterAppDelegate. If it previously worked, the tests should prove it it doesn't deviate from that. If it was broken, this won't fix it, but it will fix the linked issue.

jmagman

If it previously worked, the tests should prove it it doesn't deviate from that. If it was broken, this won't fix it, but it will fix the linked issue.

True, but if it just doesn't work at all then removing this totally may be a better "fix" for the App Store verification issue than claiming to support something and just have it not work at all. We'd have to set up remote notifications to test (and yikes: https://developer.apple.com/documentation/usernotifications/sending_push_notifications_using_command-line_tools), hopefully our users can help us out testing it. 🙂

Thanks for digging into this, LGTM

gaaclarke · 2021-11-30T22:04:36Z

The shell_test failure is unrelated, rebasing to see if that fixes the issue.

…utter/engine#29996)

…9996)" This reverts commit 68f9f3c.

Stratovarius93 · 2021-12-15T15:35:10Z

I must update to Flutter 2.8 to get solution ?

jmagman · 2021-12-15T19:03:49Z

I must update to Flutter 2.8 to get solution ?

No, this PR was reverted, and it's not available on 2.8.

esDotDev · 2021-12-24T08:26:28Z

I'm on the latest master, just submitted, and got the email. Not sure if this is expected or not...

Flutter version 2.9.0-1.0.pre.187

jmagman · 2022-01-04T19:26:27Z

I'm on the latest master, just submitted, and got the email. Not sure if this is expected or not...

This PR was reverted, so this is expected.

flutter-dashboard bot added the platform-ios label Nov 30, 2021

google-cla bot added the cla: yes label Nov 30, 2021

gaaclarke requested a review from jmagman November 30, 2021 00:49

gaaclarke marked this pull request as ready for review November 30, 2021 00:51

gaaclarke mentioned this pull request Nov 30, 2021

Plugins could avoid warnings by installing UIApplicationDelegate methods dynamically flutter/flutter#9984

Open

jmagman reviewed Nov 30, 2021

View reviewed changes

gaaclarke requested a review from jmagman November 30, 2021 19:37

jmagman approved these changes Nov 30, 2021

View reviewed changes

gaaclarke added 7 commits November 30, 2021 14:03

removed the email warning about notifications entitlement

2a76cd2

jenn feedback

a0069f9

added registration method too

94e816f

backfilled app delegate tests

7331f5c

added tests for subclasses

dc31952

added didFailToRegisterForRemoteNotificationsWithError

49a6b0e

removed class calls

3161ff0

gaaclarke force-pushed the notification-warning-email branch from aeb2574 to 3161ff0 Compare November 30, 2021 22:04

gaaclarke merged commit 68f9f3c into flutter:main Nov 30, 2021

engine-flutter-autoroll mentioned this pull request Dec 1, 2021

Roll Engine from f37334353f48 to c2fcadef89ad (6 revisions) flutter/flutter#94442

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 1, 2021

68f9f3c Removed the email warning about notifications entitlement (fl…

721aaf4

…utter/engine#29996)

jmagman added a commit that referenced this pull request Dec 14, 2021

Revert "Removed the email warning about notifications entitlement (#2…

cd5c73d

…9996)" This reverts commit 68f9f3c.

jmagman mentioned this pull request Dec 14, 2021

Revert "Removed the email warning about notifications entitlement" #30330

Merged

Removed the email warning about notifications entitlement #29996

Removed the email warning about notifications entitlement #29996

Uh oh!

Conversation

gaaclarke commented Nov 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

gaaclarke commented Nov 30, 2021

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmagman Nov 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gaaclarke commented Nov 30, 2021

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

gaaclarke commented Nov 30, 2021

Uh oh!

Stratovarius93 commented Dec 15, 2021

Uh oh!

jmagman commented Dec 15, 2021

Uh oh!

esDotDev commented Dec 24, 2021

Uh oh!

jmagman commented Jan 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gaaclarke commented Nov 30, 2021 •

edited

Loading

jmagman Nov 30, 2021 •

edited

Loading