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

revert: #24 CB-14108: fix incorrect count in config_munge #89

Merged
merged 6 commits into from
Aug 21, 2019

Conversation

erisu
Copy link
Member

@erisu erisu commented Aug 20, 2019

Platforms affected

all

Motivation and Context

Even though the PR had fixed the incorrect count in config_munge, which was an issues for plugin uninstall, it introduced issues for the plugin install. Specifically, if a user had installed a plugin that contains config-file and/or edit-config and in their project's config.xml the user also defined additional config-file and/or edit-config blocks, the config.xml would be used and all of the plugin's config-file and/or edit-config were not used, even if the contents of these blocks were not identical.

Description

This reverts commit ce3801a184d34cc4c3ea08aeecd159227c6ae9e9.

Reported in General/Tooling
fixes: apache/cordova#95
fixes: apache/cordova-cli#389

Reported in Android Platform
fixes: apache/cordova-android#704
fixes: apache/cordova-android#801
fixes: apache/cordova-android#809

Reported in iOS Platform
fixes: apache/cordova-ios#581
fixes: apache/cordova-ios#401

Other Possible Reports
This revert may also fix the following issues (similarity in report) but requires additional validation:

Additional Notes

  • The original issue that the breaking PR resolved will be reintroduced.
  • A new ticket and separate PR should be created to attempt to resolve the original issue with out affect plugin install

Testing

  • npm t
  • Locally modified Cordova using reverted code.

Checklist

  • I've run the tests to see all new and existing tests pass
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)

@project-bot project-bot bot added this to 🐣 New PR / Untriaged in Apache Cordova: Tooling Pull Requests Aug 20, 2019
spec/CordovaError/CordovaError.spec.js Outdated Show resolved Hide resolved
src/ConfigChanges/ConfigFile.js Outdated Show resolved Hide resolved
@erisu erisu requested a review from dpogue August 20, 2019 08:59
Apache Cordova: Tooling Pull Requests automation moved this from 🐣 New PR / Untriaged to ✅ Approved, waiting for Merge Aug 21, 2019
@erisu erisu merged commit d1a10a9 into apache:master Aug 21, 2019
Apache Cordova: Tooling Pull Requests automation moved this from ✅ Approved, waiting for Merge to 🏆 Merged, waiting for Release Aug 21, 2019
@odai-alali
Copy link

Hi,

When will this changes be released? I re-installed cordova-cli and it's dependencies and still getting the same error with config files.

@Mapiac
Copy link

Mapiac commented Sep 25, 2019

correct same issue as @odai-alali

Ant idea on release @erisu @dpogue ?

@brodybits
Copy link
Contributor

Guys please ask this kind of question on the dev mailing list https://cordova.apache.org/contact/

We do not provide active support for closed issues or PRs.

And please keep in mind that Cordova is supported by a bunch of overworked volunteers since we lost backing from a couple major companies.

@dminkovsky
Copy link

dminkovsky commented Oct 30, 2019

I tried testing this locally to address jeduan/cordova-plugin-facebook4#828 by patching the deps of CLI 9.0.0 and iOS 5.0.1 to use 3.2.1 of this repo instead of 3.1.0 (i.e. dminkovsky/cordova-ios@aca209e). Unfortunately, no luck. Is there some other spot where this dependency needs to be adjusted? I used npm ls to ensure that cordova-lib that's used by cordova-cli was using 3.2.1, and likewise with cordova-ios.

@raphinesse
Copy link
Contributor

raphinesse commented Nov 1, 2019

@dminkovsky Strange, what you describe sounds correct to me. You could also try to install cordova@nightly to test this change. Please let us know if this fixes the issue.

@liuxiaoy
Copy link

liuxiaoy commented Dec 6, 2019

I tried by patching the deps of CLI 9.0.0 and android 8.0.0 to use 3.2.1 of this repo instead of 3.1.0. Also no luck.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment