Missing swizzle for application:openURL:options in ios

[guylando]

The swizzling is done to the deprecated openUrl version application:openURL:sourceApplication:annotation: and not the new one application:openURL:options and the new one is overridden without swizzling which causes bugs.
https://github.com/jeduan/cordova-plugin-facebook4/blob/master/src/ios/FacebookConnectPlugin.m#L789

What happens is that the new application:openURL:options is called and it calls the CDVAppDelegate implementation of the old application:openURL:sourceApplication:annotation: and doesnt let any other plugins the chance to get their application:openURL:options called.

Note also that cordova-ios 5 changed old openUrl with new one which probably will break this plugin, see: apache/cordova-ios#476 (comment)

So the solution is that there is no need to remove the current swizzling, just need to add a new swizzling for application:openURL:options like it is done here: https://github.com/nrikiji/twitter-connect-plugin/blob/master/src/ios/AppDelegate%2BTwitterConnect.m and stop overriding application:openURL:options without swizzling

Bugs caused by this:
#166
ionic-team/ionic-plugin-deeplinks#102

[peterpeterparker]

Feel free to provide a PR, I would be happy to merge it

[guylando]

@peterpeterparker Can you merge? #751
Important note: people using cordova-ios below 5 should copy the code in the following comment into their CordovaLib/Classes/Public/CDVAppDelegate.m file:
apache/cordova-ios#476 (comment)

I tested it and using this fix it finally called the openUrl of other plugins which wasn't working before.

[peterpeterparker]

@guylando I'm happy to merge it, but first could you describe what I should test precisely and/or provide a sample app to do so?

Also what do you mean with "people using cordova-ios below 5 should copy the code in the following comment into their CordovaLib/Classes/Public/CDVAppDelegate.m" ? couldn't we develop something which does "Cordova >= 5 do that else do that"?

[guylando]

Without this PR, other social login plugins openUrl was not called and in general deep linking got broken see: ionic-team/ionic-plugin-deeplinks#102
So you can add this plugin and another plugin which has some openUrl implementation and see that its not called without this PR.

People with cordova-ios below 5 dont have application:openURL:options in CDVAppDelegate.m

The CDVAppDelegate.m file is not part of this plugin, its part of cordova-ios template and the modification I described does not break anything and without it people will having missing logic anyway.

[guylando]

Also this will solve #166

[peterpeterparker]

Thx for the information. Have you got maybe one test app so I could give a try?

About Cordova-ios below 5, understood, if it doesn't break it's good. Should we add something in the README about that particularity? Could you extend the PR with it?

[guylando]

Unfortunately I don't have any test app other than my company full big app where I tested this (which is not open source).

Regarding cordova-ios (not cordova!) below 5 I think it depends on whether the app is run on a new ios which calls the new version of openUrl or old one. If its being run on older ios then old version is called and nothing changes. If its run on newer versions then as part of upgrading this plugin version the programmer should also do the change I described to insure that nothing breaks. However right now what happens on new ios is that instead of hooking openUrl, this plugin OVERRIDES openUrl. This means if some other plugin overrides openUrl as well then there is a raise condition and only one of them will work. That is what swizzling=hooking is supposed to solve, to seamlessly add logic without overriding anything. How do I modify a file in an existing PR?

[peterpeterparker]

Mmmh then I'll have to try out and build an test app, might take a bit then

Ok thx for the explanation. Just to be clear, the change (cf.: "If its run on newer versions then as part of upgrading this plugin version the programmer should also do the change") is only needed if we don't merge the PR right? as soon as we have merged the PR no one will have to do stuffs manually? correct?

If you can't modify the PR you could submit a new one, that would be ok and cool too?

P.S.: In any case, I'll not have time today to look deeper and stuffs. Will try in the next days, can't be sure but will try

[guylando]

After the PR: the plugin works with cordova-ios 5 without any modifications required and the plugin stops breaking other plugins but since that additional change is in cordova-ios template file which is not part of this plugin, it will be required to be modified manually (copy pasting the code from that link).

Before the PR: this plugin breaks other plugins and does not work with cordova-ios 5.

You still want me to add the required modification in the readme?

[peterpeterparker]

Well now I understand that the "manually modification" would be a must and therefore we should think about a better approach, we can't I think ask everyone to do manually change in cordova-ios

Any idea how we could improve that? Other way to solve the problem?

[peterpeterparker]

Like at least finding a hook and documenting it?

Also in the PR regarding Readme you write that the plugin doesn't work with Cordova-ios v5 which is wrong, I mean ok that particular deep linking case doesn't work but the other features are fine, plz improve the text, thx a lot

[guylando]

I don't see anything written in the readme about the plugin not working with cordova-ios 5.
I don't have any more time to put into this. This requires first of all debugging and checking what happens right now without modifications and then maybe in runtime check and choose which method to call.
Using my fork my code works well right now.

[peterpeterparker]

You are right are read it to fast, my bad sorry

Ok then when you've got time or when someone as time, ping me

[t2wu]

I think the Swizzling in this plugin is failing. Cordova's own CDVAppDelegate's application:openURL:options: is not getting called.

Maybe avoid swizzling altogether. cordova-plugin-googleplus is using notification instead:

[[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(handleOpenURL:) name:CDVPluginHandleOpenURLNotification object:nil];
[[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(handleOpenURLWithAppSourceAndAnnotation:) name:CDVPluginHandleOpenURLWithAppSourceAndAnnotationNotification object:nil];

[t2wu]

Or, just comment out the method under:

// This method is a duplicate of the other openURL method below, except using the newer iOS (9) API.

- (BOOL)application:(UIApplication *)application openURL:(NSURL *)url options:(NSDictionary<NSString *,id> *)options

I believe It's completely superfluous.

[guylando]

@t2wu I think I described in the first message here in the paragraph starting with "So the solution.." the solution to "Cordova's own CDVAppDelegate's application:openURL:options: is not getting called"

[t2wu]

Thanks guylando I'll take a look at it if something goes wrong. Obviously this is open source, pure volunteering effort, so there is not anyone's responsibility. But this is so major, a pull request would be nice to be accepted.

[guylando]

@t2wu old one not merged: #751

[peterpeterparker]

The PR #751 has now been merged and I released a new version of the plugin v5.0.0

@guylando thx a lot and sorry for the delay. I'm not really using that much the plugin anymore that's why it's always difficult to find time.