Stage::validatePackageNames() should not use the Composer API

Assigning to Wim to review and hopefully commit this so we can finally get rid of ComposerUtility.

This blocks 📌 Remove our runtime dependency on composer/composer: remove ComposerUtility Fixed , tagging accordingly.

The code looks great! 👏

/opt/homebrew/bin/php /private/var/folders/x1/mlh998q15c70vs7_y8s1mgqm0000gp/T/ide-phpunit.php --configuration /Users/wim.leers/core/core/phpunit.xml --filter "/(Drupal\\Tests\\package_manager\\Kernel\\StageTest::testValidateRequirements)( .*)?$/" --test-suffix StageTest.php /Users/wim.leers/core/modules/contrib/automatic_updates/package_manager/tests/src/Kernel
Testing started at 9:41 AM ...
PHPUnit 9.5.28 by Sebastian Bergmann and contributors.

Testing /Users/wim.leers/core/modules/contrib/automatic_updates/package_manager/tests/src/Kernel
................................................................. 65 / 66 ( 98%)
.                                                                 66 / 66 (100%)

Time: 02:32.646, Memory: 10.00 MB

OK (66 tests, 198 assertions)
Process finished with exit code 0

👆 \Drupal\Tests\package_manager\Kernel\StageTest::testValidateRequirements() test now takes 2.5 minutes to run, that's not acceptable … converting this to a unit test.

Will merge as soon as this comes back green — which it will!

I did not change anything materially:

1. I moved the regexes to private constants
2. I made the method static
3. I moved the test from a kernel test to a unit test (which requires reflection, but ProcessFactoryTest already did that) to avoid this making the test suite >10% slower 😅

After searching for 30 minutes, I found d.o's "Merge" button 😳

Nice.

I have no objections to your changes, apart from calling self on a protected static method (if it's protected, we should use static:: invocation). But that's not a problem for now and easily fixed later.

Fun fact: the test coverage previously was a unit test. I too was disappointed I had to convert it to a kernel test. But hey, sometimes you need reflection, and this is one of those times. Later we can re-evaluate how much of this test coverage is actually needed (vs. how much we can trust Composer to take care of for us) and trim it down.

About a year ago I would have agreed with you about calling it using static::. But not anymore, because a static function just means it cannot access $this. But invoking a method using static::methodName() is only needed for Late Static Binding, which is only relevant when dealing with inheritance. That's not applicable here.

So self::methodName() is the closest equivalent to $this->methodName() that does not try to pass $this into the invoked method!

Pretty interesting, eh? 🤓

I know static:: is for late static bindings, but this is a protected method, which means that inheritance is relevant. Even if it were private, I'd have actually called for static, since that would allow us to easily open it up in the future if we wanted to.

Nonetheless, it's not a big deal; if we're lucky we'll be able to remove this method entirely anyway.

Ah … well … this is only protected to allow subclasses to override ::require(), nobody should be overriding ::validateRequirements()!

Then we should probably make it final, if we end up keeping it. :)

Good point!

Automatically closed - issue fixed for 2 weeks with no activity.