GitHub user omefire opened a pull request:
https://github.com/apache/cordova-lib/pull/174
CB-8577 BugFix 'cordova plugin add <plugin> --save' should properly save variables into config.xml to enable restoring later on.
CB-8577 BugFix 'cordova plugin add <plugin> --save' should properly save variables into config.xml to enable restoring later on.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/MSOpenTech/cordova-lib CB-8577
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-lib/pull/174.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #174
----
commit 59b1a6bbbe89120281581cb4c9ced7f487c4e8fd
Author: Omar Mefire <om...@microsoft.com>
Date: 2015-02-28T04:30:27Z
CB-8577 BugFix 'cordova plugin add <plugin> --save' should properly save variables into config.xml to enable restoring later on.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
Github user omefire commented on the pull request:
https://github.com/apache/cordova-lib/pull/174#issuecomment-76532251
@gorkem , I disagree with your statement that it does not affect functionality. As it currently stands, the code doesn't work for plugins that have variables. it fails on restoring !
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
Github user gorkem commented on the pull request:
https://github.com/apache/cordova-lib/pull/174#issuecomment-76639381
Ahhh. I see what you mean now. I think we should fix this on the Config.getFeature(id) so that the existing information on config.xml files are not discarded.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
Github user asfgit closed the pull request at:
https://github.com/apache/cordova-lib/pull/174
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
Github user gorkem commented on the pull request:
https://github.com/apache/cordova-lib/pull/174#issuecomment-76528103
Correct, let's do the change together with the <plugin> change. My plan is to have some sort of migration logic with that as well to help old friends. At this moment, this does not affect the functionality a bit. It is just the syntax used on config.xml. I personally do not see any value on changing from param -> variable, and I do not think this was discussed and agreed to be variable.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
Github user omefire commented on the pull request:
https://github.com/apache/cordova-lib/pull/174#issuecomment-76512869
What do you mean ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
Github user gorkem commented on the pull request:
https://github.com/apache/cordova-lib/pull/174#issuecomment-76510698
If we change this now it will just make the migration from <feature> to <plugin> logic more complex.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
Github user gorkem commented on the pull request:
https://github.com/apache/cordova-lib/pull/174#issuecomment-76643012
I agree, but this is temporary. Special params are going away with the plugin tag. And ConfigParser's current implementation already does a special treatment of FEATURE_SPECIAL_PARAMS.
I have a commit that I am testing that fixes the getFeature(id) which fixes the tests that depend on the variable too.
In case interested, this is how it all started https://github.com/apache/cordova-lib/pull/63
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
Github user omefire commented on the pull request:
https://github.com/apache/cordova-lib/pull/174#issuecomment-76532764
It currently doesn't work in 100% of the cases but I do agree we can postpone the fix.
I don't really care what the name of the element is : '<variable>' or something else. As long as it works, I'm fine with it.
Although it is NOT written anywhere that '<variable>' is the name to be used, the code is currently saying a different story(long before this change). In fact, "ConfigParser.getFeature(id)" does assume that variables are under the '<variable>' element name. That's why changing things to '<variable>' fixes this issue.
Like I said, I'm fine if instead you want to modify the name to something else.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
Github user omefire commented on the pull request:
https://github.com/apache/cordova-lib/pull/174#issuecomment-76533060
It currently doesn't work in 100% of the cases but I do agree we can postpone the fix.
It doesn't really matter what the name of the element is : '<variable>' or something else. As long as it works, I'm fine with it.
Although it is NOT written anywhere that '<variable>' is the name to be used, the code is currently saying a different story(long before this change). In fact, "ConfigParser.getFeature(id)" does assume that variables are under the '<variable>' element name. That's why changing things to '<variable>' fixes this issue.
Like I said, I'm fine if instead you want to modify the name to something else.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
Github user omefire commented on the pull request:
https://github.com/apache/cordova-lib/pull/174#issuecomment-76663106
@gorkem, the change you've committed to fix this issue ( https://github.com/apache/cordova-lib/commit/4c0fec7d30df238cfc4c834a4235b2c8fcf330ad ) introduces another bug : the variables list now contains all the `<param>` such as 'id', 'url', 'installPath', which is not correct !
This is exactly the kind of subtle bugs I was afraid would sneak in if we used the approach you're currently using to fix this issue.
Related PR, with alternative fix : https://github.com/apache/cordova-lib/pull/174
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
Github user omefire commented on the pull request:
https://github.com/apache/cordova-lib/pull/174#issuecomment-76641530
Fixing it in Config.getFeature(id) would mean that we would have to differentiate between `<param>` types : variables and non-variables. I think that would be unecessarily complex, require special logic and create more subtle bugs. So, I'm in favor on fixing the syntax itself. It also has the benefit of conveying its goal clearer.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
Github user omefire commented on the pull request:
https://github.com/apache/cordova-lib/pull/174#issuecomment-76641117
Fixing it in Config.getFeature(id) would mean that we would have to differentiate between `<param>` types : variables and non-variables. I think that could be unecessarily complex, require special logic and create more subtle bugs. So, I'm in favor on fixing the syntax itself.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org