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

wgevaert · 2022-10-11T14:56:38Z

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',
);

Originally I wrote an arrow function and I backported it in the wrong way

physikerwelt · 2023-07-23T19:14:00Z

@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 · 2023-07-24T12:36:21Z

@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 · 2023-07-24T12:54:50Z

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 · 2023-07-24T14:42:14Z

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 · 2023-07-24T15:18:23Z

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 · 2023-07-25T12:34:28Z

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 · 2023-07-26T06:38:26Z

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.

Note that in most cases, array_diff_assoc is not called, so should not increase complexity in standard setup

physikerwelt · 2023-07-26T12:48:10Z

src/Composer/Autoload/AutoloadGenerator.php

+            $dupedFiles = array_diff_assoc( $oldFiles, $files );
+            $this->io->writeError(
+                '<warning>'
+                . 'Some files are included multiple times, not including these twice in autoloader: '


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 · 2023-08-30T08:26:36Z

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 · 2023-08-30T09:57:00Z

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 · 2023-08-30T11:32:55Z

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 · 2023-09-08T09:24:53Z

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 · 2023-09-08T09:31:47Z

@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 · 2023-09-13T11:54:26Z

tests/Composer/Test/Autoload/AutoloadGeneratorTest.php

+        $this->generator->dump($this->config, $this->repository, $package, $this->im, 'composer', false, 'FilesWarning');
+        self::assertFileContentEquals(__DIR__.'/Fixtures/autoload_files_duplicates.php', $this->vendorDir.'/composer/autoload_files.php');
+        $expected = '<warning>The following "files" autoload rules are included multiple times, this may cause issues and should be resolved:</warning>'.PHP_EOL.
+            '<warning> - $baseDir . \'/foo.php\'</warning>'.PHP_EOL;


should we really output the PHP code in the warnings ?

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 · 2023-09-13T12:11:19Z

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

Wout Gevaert added 3 commits October 11, 2022 16:39

Add array_unique call

6db588e

Include 'use' in lambda functionndefinition

43d39ad

Originally I wrote an arrow function and I backported it in the wrong way

Fix typo: getpathCode -> getPathCode

2703e48

lucamauri mentioned this pull request Apr 28, 2023

Error "Cannot redeclare smwfContLang()" on Composer and Update SemanticMediaWiki/SemanticMediaWiki#5479

Open

wgevaert mentioned this pull request Jul 26, 2023

Don't include composer files already required by root package wikimedia/composer-merge-plugin#251

Open

Add warning if some files are not included.

8a3799f

Note that in most cases, array_diff_assoc is not called, so should not increase complexity in standard setup

physikerwelt reviewed Jul 26, 2023

View reviewed changes

Improve wording of warning message

7ef3908

Seldaek added this to the 2.6 milestone Sep 13, 2023

Seldaek added 2 commits September 13, 2023 11:25

Only warn about duplicate files

bd337b5

Add test for the new warning

830bbc9

Seldaek force-pushed the main branch from 6d68f98 to 830bbc9 Compare September 13, 2023 11:51

stof reviewed Sep 13, 2023

View reviewed changes

Seldaek merged commit e2f5afd into composer:main Sep 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

wgevaert commented Oct 11, 2022 •

edited

Loading

physikerwelt commented Jul 23, 2023

wgevaert commented Jul 24, 2023

physikerwelt commented Jul 24, 2023

wgevaert commented Jul 24, 2023

physikerwelt commented Jul 24, 2023

wgevaert commented Jul 25, 2023

physikerwelt commented Jul 26, 2023 •

edited

Loading

physikerwelt Jul 26, 2023

Seldaek commented Aug 30, 2023

physikerwelt commented Aug 30, 2023

Seldaek commented Aug 30, 2023

physikerwelt commented Sep 8, 2023

stof commented Sep 8, 2023

stof Sep 13, 2023

Seldaek Sep 13, 2023

Seldaek commented Sep 13, 2023

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

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

Conversation

wgevaert commented Oct 11, 2022 • edited Loading

Example setup:

Problem without this patch:

Result after this patch:

physikerwelt commented Jul 23, 2023

wgevaert commented Jul 24, 2023

physikerwelt commented Jul 24, 2023

wgevaert commented Jul 24, 2023

physikerwelt commented Jul 24, 2023

wgevaert commented Jul 25, 2023

physikerwelt commented Jul 26, 2023 • edited Loading

physikerwelt Jul 26, 2023

Choose a reason for hiding this comment

Seldaek commented Aug 30, 2023

physikerwelt commented Aug 30, 2023

Seldaek commented Aug 30, 2023

physikerwelt commented Sep 8, 2023

stof commented Sep 8, 2023

stof Sep 13, 2023

Choose a reason for hiding this comment

Seldaek Sep 13, 2023

Choose a reason for hiding this comment

Seldaek commented Sep 13, 2023

wgevaert commented Oct 11, 2022 •

edited

Loading

physikerwelt commented Jul 26, 2023 •

edited

Loading