Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CB-12002 - Support LSApplicationQueriesSchemes #269

Closed
wants to merge 3 commits into from

Conversation

dpogue
Copy link
Member

@dpogue dpogue commented Oct 12, 2016

⚠️ Depends on apache/cordova-lib#502

Platforms affected

iOS

What does this PR do?

Adds support for parsing <allow-intent> tags in config.xml and creating a list of allowed schemes to be written as LSApplicationQueriesSchemes in the Info.plist.

For more info about LSApplicationQueriesSchemes, see https://developer.apple.com/library/content/documentation/General/Reference/InfoPlistKeyReference/Articles/LaunchServicesKeys.html#//apple_ref/doc/uid/TP40009250-SW14

What testing has been done on this change?

Added spec test.

Checklist

  • Reported an issue in the JIRA database: CB-12002
  • Commit message follows the format.
  • Added automated test coverage as appropriate for this change.

@shazron
Copy link
Member

shazron commented Oct 12, 2016

The dependent changes will be in the cordova-common 1.5.1 release, and will be integrated as soon as it is released.

@shazron
Copy link
Member

shazron commented Oct 17, 2016

@dpogue Can you rebase? cordova-common 1.5.1 is in the repo now

@jcesarmobile
Copy link
Member

I commented this on the issue

LSApplicationQueriesSchemes is only needed for canOpenURL, but we use openURL without querying if it can be opened first. That opened the apps if they were on the allow-intent (worked on iOS 9 and earlier, but didn't test on iOS 10)

@codecov-io
Copy link

codecov-io commented Oct 18, 2016

Codecov Report

Merging #269 into master will increase coverage by 0.14%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #269      +/-   ##
==========================================
+ Coverage   63.97%   64.12%   +0.15%     
==========================================
  Files          14       14              
  Lines        1674     1681       +7     
  Branches      277      279       +2     
==========================================
+ Hits         1071     1078       +7     
  Misses        603      603
Impacted Files Coverage Δ
bin/templates/scripts/cordova/lib/prepare.js 84.87% <100%> (+0.21%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d44cec6...24a0923. Read the comment docs.

@shazron
Copy link
Member

shazron commented Oct 18, 2016

Yeah looks like openURL does not need the key: https://developer.apple.com/reference/uikit/uiapplication/1622952-canopenurl
(see last paragraph -- it has a typo, UIDefaultLaunchStoryboard should be LSApplicationQueriesSchemes

@dpogue
Copy link
Member Author

dpogue commented Oct 18, 2016

hmm, I think in our case we were trying to use canOpenURL to see if a partner app was able to launch and redirect to the store otherwise. This also came up in the Slack channel recently, and was mentioned in some Github threads on notification plugins trying to launch mailto links.

@shazron
Copy link
Member

shazron commented Oct 18, 2016

So if they use canOpenURL they need this, but if they just use openURL, like our whitelist system, we don't. Therefore I believe our whitelist system with <allow-intent> does not need these keys.

@jcesarmobile
Copy link
Member

We where using canOpenURL on inAppBrowser plugin, but I removed it to fix CB-11178 as we weren't really sending any response to the user if canOpenURL was false.

I don't think it's a good idea to add all the allow-intent schemes into the LSApplicationQueriesSchemes, as not all are needed to be there (as http or https), also, the PR doesn't handle the case of a duplicate scheme, as you can put multiple urls starting by http in allow intent tags and this will add http protocol multiple times.

As we are going to allow to write into the info.plist from the config.xml soon, and we don't use canOpenURL on cordova-ios, I don't think this should be merged.

@shazron
Copy link
Member

shazron commented Aug 8, 2017

eslint errors.

@shazron
Copy link
Member

shazron commented Aug 11, 2017

Please check the "Allow Edits from Maintainers" checkbox on this pull request.

@dpogue
Copy link
Member Author

dpogue commented Aug 11, 2017

It is checked already :\ I can try toggling it off and back on?

@shazron
Copy link
Member

shazron commented Aug 11, 2017

oh? i don't see it, maybe I don't have karma -- weird, I thought we get those already.

@shazron
Copy link
Member

shazron commented Aug 11, 2017

Oh I see "Add more commits by pushing to the allow-intent branch on dpogue/cordova-ios" maybe thats it

@shazron
Copy link
Member

shazron commented Feb 26, 2018

Reviewing PRs for new cordova-ios release. @dpogue @jcesarmobile -- not sure what the status of this is.

@dpogue
Copy link
Member Author

dpogue commented Feb 26, 2018

I believe (but have not confirmed) that it is possible to use <edit-config> to do this now, so this PR would not be strictly necessary

@jcesarmobile
Copy link
Member

I confirm the edit-config tag allows to add the LSApplicationQueriesSchemes, so I'm -1 on merging this

@dpogue dpogue closed this May 5, 2018
@dpogue dpogue deleted the allow-intent branch September 22, 2021 02:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants