Re: [PR] Config privacy manifest [cordova-ios]
erisu commented on code in PR #1406: URL: https://github.com/apache/cordova-ios/pull/1406#discussion_r1515674210 ## lib/PlatformConfigParser.js: ## @@ -0,0 +1,37 @@ + +/** +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + +http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, +software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +KIND, either express or implied. See the License for the +specific language governing permissions and limitations +under the License. +*/ + +'use strict'; Review Comment: ```suggestion ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@cordova.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@cordova.apache.org For additional commands, e-mail: issues-h...@cordova.apache.org
Re: [PR] Config privacy manifest [cordova-ios]
erisu commented on code in PR #1406: URL: https://github.com/apache/cordova-ios/pull/1406#discussion_r1515673834 ## lib/PlatformConfigParser.js: ## @@ -0,0 +1,37 @@ + Review Comment: Can you remove the blank like at the top. ```suggestion ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@cordova.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@cordova.apache.org For additional commands, e-mail: issues-h...@cordova.apache.org
Re: [PR] Config privacy manifest [cordova-ios]
erisu commented on PR #1406: URL: https://github.com/apache/cordova-ios/pull/1406#issuecomment-1982217021 > How does this work with plugins? Can a plugin use `` within the `plugin.xml` or will the application's `config.xml` overwrite plugin entries? The PR will solely focus on adding the minimum support for the `privacy-manifest` tag to the `config.xml` for app developers. We aim to ensure that both Config.xml (for App Developers) and Plugin.xml (for Plugin Developers) support this tag and are completed and released before Apple's Privacy Manifest requirement takes effect. However, in the event that we are unable to complete the support for plugin.xml, at least the support for config.xml will be completed and merged with this PR. > I don't want to introduce scope creep but I just want to make sure this was thought about. I think it would be ideal for plugins to be able to declare their privacy entries and the results will get merged into one file for the application. Otherwise it will be a painful process for plugin authors to document and hope end users follows instructions to add the necessary privacy entries. As you mentioned, we're aiming to keep the scope specific for this PR. However, the ultimate goal aligns with your idea of gathering all `privacy-manifest` tag values and merging them into one file for the application. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@cordova.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@cordova.apache.org For additional commands, e-mail: issues-h...@cordova.apache.org
Re: [PR] Config privacy manifest [cordova-ios]
knaito-asial commented on code in PR #1406:
URL: https://github.com/apache/cordova-ios/pull/1406#discussion_r1515376423
##
lib/PlatformConfigParser.js:
##
@@ -0,0 +1,37 @@
+
+/**
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+*/
+
+'use strict';
+
+const ConfigParser = require('cordova-common').ConfigParser;
+
+class PlatformConfigParser extends ConfigParser {
+/** getPrivacyManifest */
+getPrivacyManifest () {
+const platform_manifest =
this.doc.find('./platform[@name="ios"]/privacy-manifest-ios');
+if (platform_manifest != null) {
+return platform_manifest;
+}
+const manifest = this.doc.find('privacy-manifest-ios');
+return manifest;
+}
Review Comment:
I think your suggestion about tag structure and name is great.
Please wait a moment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@cordova.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@cordova.apache.org
For additional commands, e-mail: issues-h...@cordova.apache.org
Re: [PR] Config privacy manifest [cordova-ios]
knaito-asial commented on code in PR #1406:
URL: https://github.com/apache/cordova-ios/pull/1406#discussion_r1515374575
##
lib/PlatformConfigParser.js:
##
@@ -0,0 +1,37 @@
+
+/**
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+*/
+
+'use strict';
+
+const ConfigParser = require('cordova-common').ConfigParser;
+
+class PlatformConfigParser extends ConfigParser {
+/** getPrivacyManifest */
+getPrivacyManifest () {
+const platform_manifest =
this.doc.find('./platform[@name="ios"]/privacy-manifest-ios');
+if (platform_manifest != null) {
+return platform_manifest;
+}
+const manifest = this.doc.find('privacy-manifest-ios');
+return manifest;
+}
Review Comment:
Thanks. In this PR, manifest settings in plugins are not considered yet.
Next step, I want to develop the feature which is collecting each plugin's
manifest if exists and generating application privacy manifest file
according to
the collected plugin's plugin.xml and application's config.xml.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@cordova.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@cordova.apache.org
For additional commands, e-mail: issues-h...@cordova.apache.org
Re: [PR] Config privacy manifest [cordova-ios]
breautek commented on code in PR #1406:
URL: https://github.com/apache/cordova-ios/pull/1406#discussion_r1514982487
##
lib/PlatformConfigParser.js:
##
@@ -0,0 +1,37 @@
+
+/**
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+*/
+
+'use strict';
+
+const ConfigParser = require('cordova-common').ConfigParser;
+
+class PlatformConfigParser extends ConfigParser {
+/** getPrivacyManifest */
+getPrivacyManifest () {
+const platform_manifest =
this.doc.find('./platform[@name="ios"]/privacy-manifest-ios');
+if (platform_manifest != null) {
+return platform_manifest;
+}
+const manifest = this.doc.find('privacy-manifest-ios');
+return manifest;
+}
Review Comment:
Personally I prefer a more structured approach, I don't think we need to
support finding the privacy-manifest node in both root or the ios platform
block. I think enforcing it to be inside the ios platform block is sufficient.
Supporting both is twice the ongoing maintenance work.
Additionally, I think we can rename `privacy-manifest-ios` to
`privacy-manifest`, we don't need the extra `-ios`. This creates for a more
generic tag name to have consistency with other platforms (should they copy
Apple and require a similar feature). Chopping of the `-ios` suffix is a way of
hopefully future-proofing ourselves.
With both of these suggestions, the method will look more like:
```suggestion
getPrivacyManifest () {
return this.doc.find('./platform[@name="ios"]/privacy-manifest');
}
```
Supporting just a platform / privacy-manifest way allows for more straight
forward documentation and helps prevent easy mistakes like someone having a
privacy-manifest in both locations, etc...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@cordova.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@cordova.apache.org
For additional commands, e-mail: issues-h...@cordova.apache.org
Re: [PR] Config privacy manifest [cordova-ios]
breautek commented on PR #1406: URL: https://github.com/apache/cordova-ios/pull/1406#issuecomment-1981537920 > If user does not set tag in config.xml, the cordova template default PrivacyManifest.xcprivacy is used. > >Note: This tag overrides whole PrivacyManifest.xcprivacy file in project each time user execute cordova prepare. How does this work with plugins? Can a plugin use `` within the `plugin.xml` or will the application's `config.xml` overwrite plugin entries? I don't want to introduce scope creep but I just want to make sure this was thought about. I think it would be ideal for plugins to be able to declare their privacy entries and the results will get merged into one file for the application. Otherwise it will be a painful process for plugin authors to document and hope end users follows instructions to add the necessary privacy entries. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@cordova.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@cordova.apache.org For additional commands, e-mail: issues-h...@cordova.apache.org
Re: [PR] Config privacy manifest [cordova-ios]
knaito-asial commented on code in PR #1406:
URL: https://github.com/apache/cordova-ios/pull/1406#discussion_r1514030177
##
lib/prepare.js:
##
@@ -23,6 +23,7 @@ const fs = require('fs-extra');
const path = require('path');
const unorm = require('unorm');
const plist = require('plist');
+const et = require('elementtree');
Review Comment:
Ok. I add elementree in package.json.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@cordova.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@cordova.apache.org
For additional commands, e-mail: issues-h...@cordova.apache.org
Re: [PR] Config privacy manifest [cordova-ios]
knaito-asial commented on code in PR #1406:
URL: https://github.com/apache/cordova-ios/pull/1406#discussion_r1514022948
##
lib/prepare.js:
##
@@ -43,9 +45,16 @@ const CDV_ANY_SIZE_CLASS = 'any';
module.exports.prepare = function (cordovaProject, options) {
const platformJson = PlatformJson.load(this.locations.root, 'ios');
const munger = new PlatformMunger('ios', this.locations.root,
platformJson, new PluginInfoProvider());
-
this._config = updateConfigFile(cordovaProject.projectConfig, munger,
this.locations);
+const parser = new ConfigParseriOS(cordovaProject.projectConfig.path);
Review Comment:
Thanks, I updated it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@cordova.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@cordova.apache.org
For additional commands, e-mail: issues-h...@cordova.apache.org
Re: [PR] Config privacy manifest [cordova-ios]
knaito-asial commented on code in PR #1406: URL: https://github.com/apache/cordova-ios/pull/1406#discussion_r1514022627 ## lib/ConfigParseriOS.js: ## @@ -0,0 +1,37 @@ + +/** +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + +http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, +software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +KIND, either express or implied. See the License for the +specific language governing permissions and limitations +under the License. +*/ + +'use strict'; Review Comment: Thanks. I removed it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@cordova.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@cordova.apache.org For additional commands, e-mail: issues-h...@cordova.apache.org
Re: [PR] Config privacy manifest [cordova-ios]
knaito-asial commented on code in PR #1406:
URL: https://github.com/apache/cordova-ios/pull/1406#discussion_r1514021080
##
lib/ConfigParseriOS.js:
##
@@ -0,0 +1,37 @@
+
+/**
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+*/
+
+'use strict';
+
+const ConfigParser = require('cordova-common').ConfigParser;
+
+class ConfigParseriOS extends ConfigParser {
Review Comment:
Yes, I updated it
##
lib/prepare.js:
##
@@ -35,6 +36,7 @@ const FileUpdater = require('cordova-common').FileUpdater;
const projectFile = require('./projectFile');
const Podfile = require('./Podfile').Podfile;
const check_reqs = require('./check_reqs');
+const ConfigParseriOS = require('./ConfigParseriOS');
Review Comment:
Yes I updated it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@cordova.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@cordova.apache.org
For additional commands, e-mail: issues-h...@cordova.apache.org
Re: [PR] Config privacy manifest [cordova-ios]
knaito-asial commented on code in PR #1406: URL: https://github.com/apache/cordova-ios/pull/1406#discussion_r1514020653 ## lib/ConfigParseriOS.js: ## Review Comment: Ok. I updated it -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@cordova.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@cordova.apache.org For additional commands, e-mail: issues-h...@cordova.apache.org
Re: [PR] Config privacy manifest [cordova-ios]
erisu commented on code in PR #1406:
URL: https://github.com/apache/cordova-ios/pull/1406#discussion_r1514006978
##
lib/prepare.js:
##
@@ -23,6 +23,7 @@ const fs = require('fs-extra');
const path = require('path');
const unorm = require('unorm');
const plist = require('plist');
+const et = require('elementtree');
Review Comment:
`cordova-ios` does not have `elementtree` package installed.
I know this works because the package exists in `cordova-common` but we
should also install it to this package as a dependency.
```
npm install elementtree
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@cordova.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@cordova.apache.org
For additional commands, e-mail: issues-h...@cordova.apache.org
Re: [PR] Config privacy manifest [cordova-ios]
erisu commented on code in PR #1406: URL: https://github.com/apache/cordova-ios/pull/1406#discussion_r1514003876 ## lib/ConfigParseriOS.js: ## @@ -0,0 +1,37 @@ + +/** +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + +http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, +software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +KIND, either express or implied. See the License for the +specific language governing permissions and limitations +under the License. +*/ + +'use strict'; Review Comment: Might be able to remove this line. Most of our files does not declare `'use strict';` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@cordova.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@cordova.apache.org For additional commands, e-mail: issues-h...@cordova.apache.org
Re: [PR] Config privacy manifest [cordova-ios]
erisu commented on code in PR #1406:
URL: https://github.com/apache/cordova-ios/pull/1406#discussion_r1513991589
##
lib/ConfigParseriOS.js:
##
Review Comment:
Rename to `PlatformConfigParser.js`
##
lib/ConfigParseriOS.js:
##
@@ -0,0 +1,37 @@
+
+/**
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+*/
+
+'use strict';
+
+const ConfigParser = require('cordova-common').ConfigParser;
+
+class ConfigParseriOS extends ConfigParser {
Review Comment:
```suggestion
class PlatformConfigParser extends ConfigParser {
```
IMO I think it would be better to make the class name generic.
For example, if Android implement its own extended config parser class then
it would follow identical naming conventions.
Almost as if we created and followed a platform template and would be easy
to follow if we create a new platform in the future.
##
lib/prepare.js:
##
@@ -35,6 +36,7 @@ const FileUpdater = require('cordova-common').FileUpdater;
const projectFile = require('./projectFile');
const Podfile = require('./Podfile').Podfile;
const check_reqs = require('./check_reqs');
+const ConfigParseriOS = require('./ConfigParseriOS');
Review Comment:
If class name changes, update here as well.
##
lib/prepare.js:
##
@@ -43,9 +45,16 @@ const CDV_ANY_SIZE_CLASS = 'any';
module.exports.prepare = function (cordovaProject, options) {
const platformJson = PlatformJson.load(this.locations.root, 'ios');
const munger = new PlatformMunger('ios', this.locations.root,
platformJson, new PluginInfoProvider());
-
this._config = updateConfigFile(cordovaProject.projectConfig, munger,
this.locations);
+const parser = new ConfigParseriOS(cordovaProject.projectConfig.path);
Review Comment:
If class name changes, update here as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@cordova.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@cordova.apache.org
For additional commands, e-mail: issues-h...@cordova.apache.org
