Prioritise project installer paths as first takes precedence

Any chance that you can create an issue MR in a project where you have this so that we can test/debug easily? We can create a test case for it but having a real one would be even better.

Just working on this now. To test, I have added the following in the test input composer.json

  "extra": {
        "installer-paths": {
          "web/core": {
            "0": "type:my-own-override"
          }
        }
    }

Without the change, the output composer.json shows

   "extra": {
        "installer-paths": {
            "web/core": [
                "type:drupal-core",
                "type:my-own-override"
            ],
            "web/libraries/{$name}": [
                "type:drupal-library"
            ],
...
 

With the change, we get the first two in the reverse order

  "extra": {
        "installer-paths": {
            "web/core": [
                "type:my-own-override",
                "type:drupal-core"
            ],
            "web/libraries/{$name}": [
                "type:drupal-library"
            ],
  ..

I have pushed the change, but can easily alter if this is not what is needed.

I echo @fjgarlin's words, regarding opening an issue in the project which needs this if that is possible. But it may not be as straight-forward as testing in a pipeline.

Maybe this is a better example. The project defines:

 "extra": {
        "installer-paths": {
          "web/my/path": {
            "0": "type:drupal-module"
          }
        }
    }

Before, we got

"extra": {
        "installer-paths": {
            "web/core": [
                "type:drupal-core"
            ],
            "web/libraries/{$name}": [
                "type:drupal-library"
            ],
            "web/modules/contrib/{$name}": [
                "type:drupal-module"
            ],
...
           "web/my/path": [
                "type:drupal-module"
            ]
        },

but with this change we get

 "extra": {
        "installer-paths": {
            "web/my/path": [
                "type:drupal-module"
            ],
            "web/core": [
                "type:drupal-core"
            ],
            "web/libraries/{$name}": [
                "type:drupal-library"
            ],
            "web/modules/contrib/{$name}": [
                "type:drupal-module"
            ],
...

The change in the MR looks good and simple.

However, this would be the second exception that we need to add, and we don't know if more might be needed.

I wonder if we should try to do the "merge_deep" call with the arrays in the reverse order. Won't that try to prioritize module first always, and then suggested defaults? If that's the case, we won't need to add exceptions.

But I don't know what'd be the side effects of doing it that way, so let's test that here. Alternavitely, we could merge this and try the above suggestion in a follow-up.

Yes I was wondering about that too. In principle it seems better to allow the project to override anything, not just the few exceptions we have discovered already. I may ask @moshe as he was probably involved in setting it up this way in the first instance, and there may be a design reason why it was done that way.

I'd be fine with you merging MR384 now, as we know that improvement is needed. But keep this issue open and I will make a second MR to investigate the full deep_merge.

Ok, let's do that. I'll merge the existing MR for now but keep this issue open.

@andrewbelcher - you can use this fix already if you're using "main" as reference (https://project.pages.drupalcode.org/gitlab_templates/info/templates-ver...).

Otherwise, this will go in the next release.

I've just been looking a merge_deep and merge_deep_array and I think the merge takes the later value and overwrites any already merged value with that key. So the order is correct for the top-level items, and if we switched it there would be incorrect values, say for top-level keys like 'description'. The problem we are trying to solve in the above change is not really a merge when the values overright, but simply an ordering of the values in the array 'installer-paths'. I don't know enough about the installer-paths array to see if what I've chnaged above is actually what @andrewbelcher wanted. But it has done no harm, so no need for any revert.

I've adde a --verbose flag and also a --suffix to allow easy repeated re-running locally, so we can see the outcome of changes. Will push that tomorrow.

The new MR does not change any existing functionality, but it does add --verbose and --debug options, and --input and --suffix options to allow easier local re-running without messing up the projects real composer.json and not overwriting the input.

The characteristic of placing the project's values after the default is due to the way merge_deep_array is written. The keys for the first array define the order, whereas the values of the second array take precedence and overwrite the first array. This means that by default, the output order is not preserved from what it is in the projects file. See attached file compare of projects original on the left, vs output on the right.

However, I found a nice simple and neat way to preserve the input order of the array. deep_merge can take multiple arrays, so instead of merge_deep($json_default, $json_project) we just repeat the project array at the start - merge_deep($json_project, $json_default, $json_project); and then use the $preserve_integer_keys = TRUE parameter to avoid duplicating arrays that do not have a named text key. The result is the much cleaner version as attached. This may also help to keep the projects values ahead of the defaults, thus removing the need for the two exceptions. But I will need to check that next.

There may be a downside to using $preserve_integer_keys = TRUE if we ever added new numeric (non-text) array values to the $json_default array. But we could document that, and make sure its avoided.

From #15

This may also help to keep the projects values ahead of the defaults, thus removing the need for the two exceptions.

As expected, the change to repeat the project's json array at the start of the merge does in fact solve both of those ordering overrides. So we can delete those two lines.

This looks promising and it’d be great to remove those two exceptions in favor of this new approach.

Let’s test it and see if everything is still good.

It’d be great to find the issue for which we needed to add the first exception and see if still works with this approach.