Failed to install '<plugin>': TypeError: Cannot read properties of undefined (reading 'id') about cordova-common

[GiovanniBattista]

Sorry for the late response, I was on a short vacation.

Unfortunately, my proposed fix didn't work correctly and led to a subsequent error.

Yeah, it also seams to me, that the cleanup upon plugin removal doesn't work properly.
In my case, the resulting data point looks like this:

"/manifest/application": [
{
  "xml": "<application android:networkSecurityConfig=\"@xml/uq_network_security_config\" />",
  "count": 1,
  "mode": "merge",
  "plugin": "uq-cordova-plugin-settings",
  "oldAttrib": {
    "android:hardwareAccelerated": "true",
    "android:icon": "@mipmap/ic_launcher",
    "android:label": "@string/app_name",
    "android:networkSecurityConfig": "@xml/uq_network_security_config",
    "android:supportsRtl": "true"
  }
},

Upon removal, it actually looks like this which produces the error when the plugin is re-added again.

"/manifest/application": []

If I remove this line, then the re-adding works.

It also works, if at least one other plugin adds something to "/manifest/application", i.e. if the array isn't empty when the plugin is re-added again.

[breautek]

@GiovanniBattista @breautek I think the idea of using an xml string to identify the code added to AndroidManifest is not as good as it seems to be, bacause in some cases you can end up adding something twice, for ex in my case, adding a permission tag from one plugin and the same permission with a maxSdkVersion attribute with another, will result on adding the same permission twice which means build failure, I think using a definition object inside parents is better be like below:

{
    tag:"uses-permission",
    attributes:[
        {
            name:"android:name",
            value:"android.permission.READ_EXTERNAL_STORAGE",
            plugins:["plugin_1","plugin_2"],
        },
        {
            name:"android:maxSdkVersion",
            value:"30",
            plugins:["plugin_2"],
        }
    ]
}

and then if you remove a plugin , if the plugin exists in plugins and plugins.length>1, you keep the tag and just remove the plugin from plugins and if the plugins.length==1 then remove from AndroidManifest. I don't know it seems legit, I'm a bit busy these days, but i'll definitely give a shot, I just wrote this comment to someone who can work on it sooner.

I do agree to this and something similar to this is already being done. The codebase calls it munging. But it's not as simple as this structure. This in itself doesn't handle conflicts, and operations needs to support targeting nodes and attributes so they can be manipulated, or deleted, etc.

Personally I think what Cordova has needs to be rewritten, but I haven't found the time to provide a PoC either.

[GiovanniBattista]

I may have spoken too soon. The guard condition in registerConflict only leads to another error further down the road:

Failed to install 'uq-cordova-plugin-settings': TypeError: Cannot read properties of undefined (reading 'charAt')
    at SAXParser.write (D:\CordovaApp\node_modules\sax\lib\sax.js:986:17)
    at XMLParser.feed (D:\CordovaApp\node_modules\elementtree\lib\parsers\sax.js:48:15)
    at ElementTree.parse (D:\CordovaApp\node_modules\elementtree\lib\elementtree.js:271:10)
    at exports.XML (D:\CordovaApp\node_modules\elementtree\lib\elementtree.js:606:13)
    at ConfigFile.prune_child (D:\CordovaApp\node_modules\cordova-common\src\ConfigChanges\ConfigFile.js:135:46)
    at PlatformMunger.apply_file_munge (D:\CordovaApp\node_modules\cordova-common\src\ConfigChanges\ConfigChanges.js:76:43)  
    at PlatformMunger._munge_helper (D:\CordovaApp\node_modules\cordova-common\src\ConfigChanges\ConfigChanges.js:173:18)    
    at PlatformMunger.add_plugin_changes (D:\CordovaApp\node_modules\cordova-common\src\ConfigChanges\ConfigChanges.js:115:18)
    at D:\CordovaApp\node_modules\cordova-common\src\PluginManager.js:120:33
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

It only works if you delete the two empty arrays in platforms/android/android.json:

[breautek]

The entire system that powers the <config-file> and <edit-config> doesn't work very well, and there is a lot of known issues surrounding it.

If you're able to determine a solution, even if it's not perfect then a PR would be appreciated...

Most issues starts to occur when there are more than 1 thing trying to modify the same node, or part of the node tree. Your issue doesn't appear to be that however given that it looks like you have a sample project with your sample plugin.

The issue appears to be more on the fact that on plugin uninstall, it isn't "cleaning" up after itself.

If I recall correctly, when you have a munge operation:

it will add a data point like this:

{
     "xml": "<uses-permission android:name=\"android.permission.WRITE_EXTERNAL_STORAGE\" />",
    "count": 1
},

and it's suppose to increment the count. This I believe is to support 2 plugins potentially trying to make hte same modification. e.g. if you add plugin A and plugin B that has the same edit-config, it should have a count of 2. If you remove plugin B, then the count I think is suppose to decrement. If the count reaches 0, it suppose to signal to remove the modification from the target file. At least I believe that is how it's suppose to work.

Perhaps on plugin add, it's incrementing more than once? If so that might the root cause of your issue.

[PrasannaKumarChalla]

I am having the same issue, is there any update or workaroung for this?.

[AhmedAyachi]

@GiovanniBattista @breautek I think the idea of using an xml string to identify the code added to AndroidManifest is not as good as it seems to be, bacause in some cases you can end up adding something twice, for ex in my case, adding a permission tag from one plugin and the same permission with a maxSdkVersion attribute with another, will result on adding the same permission twice which means build failure, I think using a definition object inside parents is better be like below:

{
    tag:"uses-permission",
    attributes:[
        {
            name:"android:name",
            value:"android.permission.READ_EXTERNAL_STORAGE",
            plugins:["plugin_1","plugin_2"],
        },
        {
            name:"android:maxSdkVersion",
            value:"30",
            plugins:["plugin_2"],
        }
    ]
}

and then if you remove a plugin , if the plugin exists in plugins and plugins.length>1, you keep the tag and just remove the plugin from plugins and if the plugins.length==1 then remove from AndroidManifest.
I don't know it seems legit, I'm a bit busy these days, but i'll definitely give it a shot, I just wrote this comment to someone who can work on it sooner.

[AhmedAyachi]

A conflict is when two definition objects with the same name have two different values, and that will happen a lot when the project is using more and more plugins.
By saying "This in itself doesn't handle conflicts", you mean: when plugin_2 overwrites the value provided by plugin_1 (with the --force flag let's say), when plugin_2 is removed, the value should get back to the one provided by plugin_1 ?
I did not think of that and I totally agree, but what if we change the schema to this :

{
    tag:"uses-permission",
    attributes:[
        {
            name:"android:name",
            plugins:[
                {id:"plugin_1",value:"android.permission.READ_EXTERNAL_STORAGE"},
                {id:"plugin_2",value:"android.permission.READ_EXTERNAL_STORAGE"}
            ],
        },
        {
            name:"android:maxSdkVersion",
            plugins:[
                {id:"plugin_1",value:"29"},
                {id:"plugin_2",value:"30"},
            ],
        }
    ]
}

And in this case, the value of each attribute is determined by the last plugin item .value.
Let's say plugin_2 was removed, the value will fallback automatically to the plugin_1's.
I don't know, I'm just sharing ideas with you, so when I start contributing, I actually have something to work with/on.