Add array_unique call to prevent loading the same file multiple times.

[wgevaert]

The composer json example I showed in an issue on this project runs into a problem where it loads the same file multiple times.

This patch will not generate such an autoload file, by ensuring these files are unique.

Example setup:

composer.json:

{
    "name": "test/test",
    "require": {
        "wikimedia/composer-merge-plugin": "@dev"
    },
    "config": {
        "allow-plugins": {
            "wikimedia/composer-merge-plugin": true,
            "composer/installers": true
        }
    },
    "extra": {
        "merge-plugin": {
            "include": [
                "composer.local.json"
            ],
            "recursive": true,
            "merge-dev": false
        }
    }
}

composer.local.json:

{
  "require": {
    "mediawiki/user-functions": "^2.7"
  },
  "extra": {
    "merge-plugin": {
      "include": [
        "extensions/*/composer.json"
      ]
    }
  }
}

Problem without this patch:

The file vendor/composer/autoload_files.php that is generated looks like

<?php

// autoload_files.php @generated by Composer

$vendorDir = dirname(__DIR__);
$baseDir = dirname($vendorDir);

return array(
    '7bb4d66942ef8c1728c8f742a0d5639d' => $baseDir . '/extensions/UserFunctions/UserFunctions.php',
    '3661fc4b5b3581d0c7a3aa2f63bb491b' => $baseDir . '/extensions/UserFunctions/UserFunctions.php',
);

which results in an error as described in this issue.

Result after this patch:

The file vendor/composer/autoload_files.php looks like

<?php

// autoload_files.php @generated by Composer

$vendorDir = dirname(__DIR__);
$baseDir = dirname($vendorDir);

return array(
    '7bb4d66942ef8c1728c8f742a0d5639d' => $baseDir . '/extensions/UserFunctions/UserFunctions.php',
);

[physikerwelt]

@Seldaek, what do you think about this PR? It works for me locally, but I am a bit concerned about using a custom composer version in production.

[wgevaert]

@Seldaek, what do you think about this PR? It works for me locally, but I am a bit concerned about using a custom composer version in production.

I believe they do not want this pull request to be merged, as seen in this reply.

The recommended solution (for now) is NOT to use the following in your composer.local.json:

{
  "require": {
    "...": "..."
  },
  "extra": {
    "merge-plugin": {
      "include": [
        "extensions/*/composer.json"
      ]
    }
  }

But instead, use something like this:

{
  "require": {
    "...": "..."
  },
  "extra": {
    "merge-plugin": {
      "include": [
        "extensions/ExtensionOne/composer.json"
        "extensions/ExtensionTwo/composer.json"
        "extensions/ExtensionThree/composer.json"
      ]
    }
  }

Where you only include those composer.json files that are not automatically parsed already by composer.

See also this issue for the merge plugin and this comment on an issue related to this on SemanticMediaWiki.

[physikerwelt]

Is there also a patch for the merge-plugin? For now, I am using my own composer alternative based on this patch.

https://github.com/MaRDI4NFDI/docker-composer/pkgs/container/docker-composer

In general, I think it is a good idea to ensure that there are no duplicates in the autoload file. However, my feeling is that this implementation might miss some edge cases. Thus I would see it as an advantage if composer maintainers would have a look.

[wgevaert]

Is there also a patch for the merge-plugin?

Yes, I made this pull request some time ago, which I believe fixes this particular issue.

In general, I think it is a good idea to ensure that there are no duplicates in the autoload file.

Composer already tries very hard to make sure there are no duplicates, see this comment:
Every file has as the key the value md5( package/name:path/to/file ), so the only way a file is included twice is when two different packages attempt to include the same file. This is a very very weird situation: Usually, composer puts every package in the folder vendor/package/name/, and thus only md5(package/name:path/to/file) should include that file, and not md5(other/package:path/to/file). It is very weird if two packages try to include the same file.

I hope the patch for the merge plugin fixes your issues.

[physikerwelt]

Composer already tries very hard to make sure there are no duplicates, see this comment: Every file has as the key the value md5( package/name:path/to/file ), so the only way a file is included twice is when two different packages attempt to include the same file. This is a very very weird situation: Usually, composer puts every package in the folder vendor/package/name/, and thus only md5(package/name:path/to/file) should include that file, and not md5(other/package:path/to/file). It is very weird if two packages try to include the same file.

I would have said, try harder, which is what this PR does;-) However, either way is fine with me.

I hope the patch for the merge plugin fixes your issues.

It does.

So, if I understand you right, this PR can be closed?

[wgevaert]

So, if I understand you right, this PR can be closed?

I still believe either this PR or the merge-plugin pr should be merged, so I'll leave this open until one of them is merged.

I still think this PR adds some extra security for very little performance loss, so I don't think I want to close it just yet, although it seems quite clear that this will not be merged.

[physikerwelt]

I think the merge fix looks good and will be merged soon (maybe WMF wants to be conservative and add a feature flag).

The only issue I see with this pr (#11109 )is that it silently fixes problems that occurred somewhere else. Maybe adding a warning on duplicate values would resolve the concerns of the package maintainers. I hope after the summer break, we will get some feedback.

[physikerwelt]

There is a mix of "multiple" and "twice" in the message.

Files have been requested multiple times for inclusion. This might indicate a problem in the configuration. Affected files:

[Seldaek]

I'd rather not merge this because this is a misconfiguration which should not happen, and if a plugin decides to break things the fix and deduplication should be done in that plugin IMO.

[physikerwelt]

I can understand this way if thinking. Instead of a warning one could also error out. However, I think generating autoload files that can not work should be avoided and discovered as early as possible.

[Seldaek]

That depends really, many files which are meant for files autoloading support being included many times without problem by doing conditional defining of functions for example. Also if we error out it would probably prevent a plugin from resolving that situation after the autoload dump is complete.

[physikerwelt]

I understand there are situations when no problems occur if one requires the same file twice. I would guess that this is, however, not the majority of packages. However, I am primarily a simple PHP user who found it difficult and time-consuming to trace back the problem through different layers way down to composer itself. If anything can be done to simplify it for future people who have the same problem, I would appreciate it, even if I better understand now that composer does not feel guilty:-) Users appreciate robust products a lot.

[stof]

@Seldaek maybe composer could detect that case and display a warning message (not an exception stopping everything). this way, the user would be aware that they have an unexpected state (which might be due to a bad plugin)

[stof]

should we really output the PHP code in the warnings ?

[Seldaek]

I was also wondering about this.. but I think at least it is clear what dir this is, otherwise you'd just see "./foo" and we don't have the reference to the package or anything.. So I find this is maybe clearer.

[Seldaek]

Alright merged now as a simple warning, to help catch this early. Thanks everyone