Look into skipping audit of composer operations in package manager

I would imagine we will never show the results of the audit directly in the composer UI, it would be an explicit operation, so we should probably look into passing --no-audit both inside and outside of tests, although this could be two separate issues.

Well here is at least an attempt. I don't know the package manager codebase intimately, but this seems to be a common code path for the tests at least.

I also verified that the output before adding the flag included the string "No security vulnerability advisories found." (which comes from when composer is doing audits) and after I added it, it no longer was part of the output.

The result for me for running just one specific test was no actual gain in speed, but that was a relatively simple and fast kernel test. Let's rather see what the full run says

Hm this makes me a bit unsure if I am reading this correctly. Or even how consistent timings are across test runs or test runners. So I hope you will forgive me if I am completely misunderstanding and off track here... But...

https://git.drupalcode.org/issue/drupal-3491268/-/pipelines/357608 first full pipeline with fix. 7 mins 29 secs

https://git.drupalcode.org/issue/drupal-3491268/-/pipelines/357625 second run. Fix reverted for comparison. 9 mins 43 secs.

https://git.drupalcode.org/issue/drupal-3491268/-/pipelines/357678 third run, fix added back. 4 mins 45 secs.

Sounds kind of promising?

So the way I compare run times on gitlab is to click into the individual job, and look at the run-tests.sh summary.

e.g.

Run 1 https://git.drupalcode.org/issue/drupal-3491268/-/jobs/3571254

Drupal\Tests\package_manager\Kernel\PhpTufValidatorTest       13 passes  216s  

Run 2
https://git.drupalcode.org/issue/drupal-3491268/-/jobs/3571532

Drupal\Tests\package_manager\Kernel\PhpTufValidatorTest       13 passes  129s  

Run 3 https://git.drupalcode.org/issue/drupal-3491268/-/jobs/3572029

Drupal\Tests\package_manager\Kernel\PhpTufValidatorTest       13 passes  149s   

However the times are so variable on gitlab at the moment that this is very inconclusive. Also possible it looks different for a different package_manager test.

I think running one of the package_manager tests with a lot of methods/data providers locally before/after is probably a better bet for comparison at the moment.

Ah, makes much more sense. Thanks!

I'll produce some comparisons locally instead then ✌️

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies.

First want to say I am so glad Package Manager is in core so we can get more eyes on it to make improvements.

Theoretically we might even want this to be this way in the actual package manager module?

Just want to say that we did the audit in package manager because we were a bit paranoid that our test setup can only test so much and there might be many many differences among real composer projects. So we thought it safest to use audit to hopefully at least ensure the projects are the way composer expects or would want things set up.

I think running composer operations on live site, even though we do it in sandbox first, is potentially dangerous so I think being paranoid and trying to ensure setting up things the *right* way, as much as possible, is warranted.

That's a very valid reason. However, I want to offer another view at it.

I think running composer operations on live site, even though we do it in sandbox first, is potentially dangerous so I think being paranoid and trying to ensure setting up things the *right* way, as much as possible, is warranted.

Ideally one would of course not want to run composer on a live site. But we know that will be the case already for many, plus using package manager is in practice exactly that. But personally, I would for sure turn off auditing on a live site. The auditing should occur other places than on prod while building your site. But yeah, that ideal scenario is not what we are catering with package manager for sure :)

In addition, I would argue it's less robust, especially in the package manager setting. Doing a require with audit takes longer time, and increases the chance of different forms of timeout. In addition it opens up for the scenario where the composer require (or other commands) could exit with exit code 1, even if the install was succesful. In a programmatic way, this will for sure create confusion. Here is an example where I am faking an exception, to illustrate the state of it:

$ composer require psr/log                                                                                                        
./composer.json has been updated
Running composer update psr/log
Loading composer repositories with package information
Updating dependencies
Lock file operations: 1 install, 0 updates, 0 removals
  - Locking psr/log (3.0.2)
Writing lock file
Installing dependencies from lock file (including require-dev)
Package operations: 1 install, 0 updates, 0 removals
  - Installing psr/log (3.0.2): Extracting archive
Generating autoload files

In Installer.php line 430:
                  
  some exception  
                  

require [--dev] [--dry-run] [--prefer-source] [--prefer-dist] [--prefer-install PREFER-INSTALL] [--fixed] [--no-suggest] [--no-progress] [--no-update] [--no-install] [--no-audit] [--audit-format AUDIT-FORMAT] [--update-no-dev] [-w|--update-with-dependencies] [-W|--update-with-all-dependencies] [--with-dependencies] [--with-all-dependencies] [--ignore-platform-req IGNORE-PLATFORM-REQ] [--ignore-platform-reqs] [--prefer-stable] [--prefer-lowest] [-m|--minimal-changes] [--sort-packages] [-o|--optimize-autoloader] [-a|--classmap-authoritative] [--apcu-autoloader] [--apcu-autoloader-prefix APCU-AUTOLOADER-PREFIX] [--] [<packages>...]

$ echo $?
1
$ ls vendor
autoload.php  composer  psr

So I would argue using no audit is more robust, and more "right way". My personal opinion and 2 cents though :D

Still have not had time to produce any good results on test runs, but it seems the gain is small but consistent. Will try to post some results at some point :)

Doing a require with audit takes longer time, and increases the chance of different forms of timeout.

We also have additional overhead (quite a lot at the moment, hopefully it will improve) from PHP-TUF, so the chances of timeouts are non-zero and agreed that the theoretical chance of a timeout seems more likely than the theoretical chance of composer audit finding a mistake.