Skip to content
This repository has been archived by the owner on Oct 24, 2022. It is now read-only.

Fix for #599 #601

Closed
wants to merge 4 commits into from
Closed

Fix for #599 #601

wants to merge 4 commits into from

Conversation

amritk
Copy link

@amritk amritk commented Dec 9, 2017

This is my attempted fix for the new android 7.0.0 update. Not sure if this works for all cases but its working for me.

@fredgalvao
Copy link
Collaborator

fredgalvao commented Dec 9, 2017

@amritk Thanks for helping!

So, looking at the push plugin + docs + blog post, and even though I haven't really tested it myself recently or talked to @macdonst specifically about it, I think there's something not matching or still unclear.

You see, at the plugin.xml file over at the push plugin (which was recently updated with the same goal as this, which is to support cordova-android@7.0) we didn't have to change the target on the AndroidManifest.xml config-file tag to the "full path" with target="app/src/main/AndroidManifest.xml".
Another confusing point is that the v7 announcement post doesn't really touch the config-file tag per se, only saying that the edit-config tag needs changing to be compatible.
The last confusing point is that the docs on both of them still use the old simple ="AndroidManifest.xml" attribute value.

So, my point is (and @macdonst could help us on this one):

  • based on the disagreement between the blog post and the docs, does edit-config really need changing to work with cordova-android@7.0?
  • based on everything, does config-file also need changing to work with cordova-android@7.0?
  • do all the options still work for compatibility? (this would invalidate a major sentence on the blog post about it not being possible to support all versions)

We could summon the boss Joe Bowser in here if needed, but I think we can test and solve this ourselves.

@amritk
Copy link
Author

amritk commented Dec 10, 2017

Yea I wasn't 100% sure if I was doing it right. But I did get it working for my own case. Definitely needs to be checked by someone more knowledgeable.

@macdonst
Copy link

@amritk @fredgalvao yeah, the new project structure for Android Studio 3.0.0 compatibility released in cordova-android 7.0.0 is a bit of a pain but it lines us up with Google going forward. Any plugin that relies on paths to config files will need to be updated and a new version released.

This is one of the reasons why it is cordova-android 7.0.0. When major version numbers change then you can expect some breaking changes.

My suggestion would be for the maintainer to pull in this change, bump the version to 2.0.0 and add and engine section to plugin.xml & package.json so that if you are running cordova-android 7 or higher it installs version 2.0.0 but if cordova-android is less then 7 it installs the 1.x stream.

@fredgalvao
Copy link
Collaborator

@macdonst
I understand all of those points, and they weren't my main point in here.

My question is regarding the actual need to change the config-file and/or edit-config tags. We have evidence that it works without changing it (phonegap-plugin-push) and that it works when changed (facebook4, this PR). I'm okay with stuff changing and breaking, I'm just trying to understand the actual changes involved, because there is no global agreement as of right now.

@macdonst
Copy link

@fredgalvao in this case there is a mapping in cordova-android for AndroidManifest.xml and I just checked with Joe and apparently there is one for res/values as well. So the change is probably not needed but I am having trouble building it as the Facebook SDK includes the ZXing library that sets min sdk to 15 when cordova-android 7 sets it to 16 so everything is screwed up right now.

@fredgalvao
Copy link
Collaborator

I was about to bet there to be a specific mapping for AndroidManifest.xml, glad to know it's now explained, thanks!

I'll have to test the build on my app with this update, but it'll require a lot of upgrades on my part, so it might take a while. Regardless, facebook-sdk asking for a minSdk=15 shouldn't be an issue, it's just a "minimum", isn't it? It shouldn't conflict, but then again, I'll have to test it myself and see what you're going through.

Thanks for clarifying anyway, @macdonst.

@amritk I'd say now that it's preferable to leave the old raw AndroidManifest.xml value and see if it works on both versions using the custom inner mappings. If that works, it's the best option, as it'll work on all versions we want to support. If that doesn't work, then I'll merge this and we can ask help to do a release on npm.

@amritk
Copy link
Author

amritk commented Dec 12, 2017

@fredgalvao cool yea if you can get it working on both then that would be ideal

@peterpeterparker
Copy link
Collaborator

peterpeterparker commented Dec 21, 2017

@fredgalvao @amritk guyz, had you had time to work on this compatibility with cordova-Android 7.0.0? could I help with some tests or something? or should it be merged?

@amritk
Copy link
Author

amritk commented Dec 21, 2017

I'm just using the pull request, working fine for me at the moment in production.

@fredgalvao
Copy link
Collaborator

@peterpeterparker I am waiting on a few releases on the cordova world to be able to fully test this and all the new stuff on platforms on my project, so it's a "not yet" for me.

@peterpeterparker
Copy link
Collaborator

@amritk @fredgalvao thx for your feedback, appreciated!

today I noticed that I wasn't able to update any plugins of the last cordova release (https://cordova.apache.org/news/2017/12/20/plugins-release.html) because I wasn't able to upgrade yet to cordova-android@7.0.0, that's why I asked to know the status of this PR

if you need any beta tester, just ping me, I would like to help there

@rfreis
Copy link

rfreis commented Jan 16, 2018

How is it going guys ?
Have you worked this PR already ? I'm still having the same issue here...

@peterpeterparker
Copy link
Collaborator

@rfreis in the meantime, to solve the issue, you could use the workaround described by @ChrisHSandN in #599 it works just fine

@rfreis
Copy link

rfreis commented Jan 16, 2018

@peterpeterparker thanks for reply.
Compilation is working now though it seems not to into ionic's-pro package build

@peterpeterparker
Copy link
Collaborator

@rfreis can't help you there unfortunately, I don't use pro

@rfreis
Copy link

rfreis commented Jan 16, 2018

@peterpeterparker no problem buddy. Thank you very much for your reply!

@fredgalvao
Copy link
Collaborator

@amritk @peterpeterparker @rfreis FYI I'll be migrating/upgrading platforms on my project soon (this week/weekend) and I'll then have time to play with this a bit. We might have a merge then, if all goes well and if I can clarify the doubts I mentioned before. We'll still need help from someone with permission to publish to npm after that.

@peterpeterparker
Copy link
Collaborator

@fredgalvao thx in advance for all the effort, kudos

@kevin-lot
Copy link

config-file seems to correct automaticaly paths only with 'AndroidManifest.xml', files ended with 'config.xml' files ended with 'strings.xml' or files matched with "res/xml'.

This PR modifies "res/values/facebookconnect.xml" too and I think the building issue comes from here.

https://github.com/apache/cordova-common/blob/fe21952b492dd9f90b7de07c256064626cdb8d5b/src/ConfigChanges/ConfigFile.js#L191

Am I wrong ?

@fredgalvao
Copy link
Collaborator

@kevin-lot You seem very much correct. It's close to confirmed that we won't need the change on the AndroidManifest.xml tag (but we could leave it in to be explicit). However, I couldn't find a mapping for anything too generic regarding res/values, so I'd keep the change for res/values/facebookconnect.xml as it is.

As Simon mentioned, we'll need to update plugin.xml on the engines tag too, to specify that it's aimed at cordova-android@7.0.0, and bump the plugin version to 2.0.0 to make it clear.

What do you think, @amritk ?

@peterpeterparker
Copy link
Collaborator

@fredgalvao I've to be honest, I don't get all the subtlety of this PR, but just wanted to ask, do you think we should merge it/include it in next version (in the same time as Fb SDK/iOS 11 PR #634) or it's not mergeable?

@fredgalvao
Copy link
Collaborator

Considering what @kevin-lot found with #621, and with the final simpler solution suggested by @ChrisHSandN here I'd say we're safer going on that route instead. It's simpler, delegates configuration to user > cordova > app instead of user > cordova > plugin > cordova > app, and reduces code to be maintained. The only downside being a higher cordova version requirement, which is acceptable imho.

It's independent from the SDK issues, but we should aim to get them both if we want a pristine version working with modern libs.

@fredgalvao
Copy link
Collaborator

If we all agree on that, we can close a bunch of issues with a single release.

@peterpeterparker
Copy link
Collaborator

@fredgalvao I'm agree with you, I like @ChrisHSandN solution 👍

I have now merged the iOS11 branch into master. Could you take care of merging and/or closing the PRs related to this cordova-android improvements? I would feel more comfortable to let you do it, you know way better than me this topic

Afterwards I suggest that we bump up the plugin version to v2.0.0, agree? If so, I could do it and open a CHANGELOG to document it

@fredgalvao
Copy link
Collaborator

@peterpeterparker Agreed on all topics. It'll be important for the one writing the breaking changes and updating plugin.xml to review cordova and cordova-android version requirements, as I'm sure we'll have to bump some of them (thus my agreement on the major bump). I will take some time tonight or tomorrow to do some chore on this repo and clean it up a bit.

@peterpeterparker
Copy link
Collaborator

@fredgalvao awesome, thx in advance for everything and for your time!!!

@peterpeterparker
Copy link
Collaborator

@fredgalvao any news from your side?
do you think you will have time in the next days/weeks?
or you rather like me to go on and release a version 1.10 only with the iOS SDK?

no pressure at all, just a question

thx in advance for your feedback

@quedicesebas
Copy link

Please!

@peterpeterparker
Copy link
Collaborator

I have release v1.10.0 with out this improvement, I hope it's ok for you @fredgalvao

I have added a note in the Android Guide about how to config the config.xml in order to be compatible with cordova-android >= 7

Copy link

@GuillaumeRemyCSI GuillaumeRemyCSI left a comment

Choose a reason for hiding this comment

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

This PR solves the issue with this plugin even for cordova-android v7.1.0

feliperfmarques added a commit to feliperfmarques/cordova-plugin-facebook4 that referenced this pull request Sep 5, 2018
rsevil added a commit to rsevil/cordova-plugin-facebook4 that referenced this pull request Sep 17, 2018
@amritk
Copy link
Author

amritk commented Dec 13, 2018

Just merged in the latest changes from upstream. Still seems to be working for me. I did have to re-add the platform though.

@peterpeterparker
Copy link
Collaborator

I gonna close this PR as I merged #716 which solves the same issue

Note: I merged the other PR as it offers the backwards compatibility with cordova-android < v7

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants