FIX use composer runtime api

[lekoala]

Description

VersionProvider has to store in cache the composer lock file when it could simply use the composer runtime api to get the same information
Also fixes return types for array, fix irrelevant ?? checks (that are done on false, they only apply to null values)

Issues

#11134

Pull request checklist

• The target branch is correct
  • See picking the right version
• All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
  • Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
• The commit messages follow our commit message guidelines
• The PR follows our contribution guidelines
• Code changes follow our coding conventions
• This change is covered with tests (or tests aren't necessary for this change)
• Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
• CI is green

[kinglozzer]

Some thoughts:

• Test failures :(
• We should look to remove the code that manually parses composer.lock. We already implicitly require composer-runtime-api: ^2 (via silverstripe/vendor-plugin), so we know that the runtime API will be available
• We should explicitly require composer-runtime-api in composer.json
• It’d be worth testing the performance overhead of the runtime API (it looks like it’ll be very minimal) and seeing if we can remove our caching. There’s nothing inherently brittle about the caching we have in place, but less code to maintain is less code to maintain 😄

[lekoala]

seems harder than expected, i only did some quick tests on my local project and it seemed to work at first glance but obviously the return values are not the ones expected in the tests.
it was a bit worried to add a composer.json entry in a patch release so that's why i didn't do it and kept the existing code for the same reason, since it's a protected method, it could technically be overwritten.

[GuySartorelli]

This is mostly an echo of KingLozzer's review:

This will at a very minimum need the broken tests updated match the new functionality. We don't need as much test coverage with the composer runtime API since we can trust it to give us the correct versions, but we do need some (non-failing) test coverage.

I agree that we should aim to remove the composer.lock related functionality. Please:

• deprecate the getComposerLock() method
• add composer-runtime-api as an explicit dependency (we can do this in minor releases, which this should be targetting)
• Remove the if (class_exists(\Composer\InstalledVersions::class)) { conditional statement since this will always evaluate as true, and remove the code that won't be reached

Please target the 5 branch and add a changelog entry for this change.

[GuySartorelli]

Also please don't tick boxes in the checkbox that aren't correct. i.e. "CI is green" was ticked but CI is very obviously not green 😅

[GuySartorelli]

i only did some quick tests on my local project and it seemed to work at first glance but obviously the return values are not the ones expected in the tests.

The current tests are basically feeding in dummy composer.lock files (or content, haven't actually looked at them this is from memory), which obviously won't affect the composer runtime API.
I think we can ignore what the actual versions are for the tests now and just trust that composer isn't lying - so just check that our method outputs a SEMVER compliant string using composer/semver

[lekoala]

i only did some quick tests on my local project and it seemed to work at first glance but obviously the return values are not the ones expected in the tests.

The current tests are basically feeding in dummy composer.lock files (or content, haven't actually looked at them this is from memory), which obviously won't affect the composer runtime API. I think we can ignore what the actual versions are for the tests now and just trust that composer isn't lying - so just check that our method outputs a SEMVER compliant string using composer/semver

yes, that's exactly why it fails
i've updated the test it's only very basic for now but it should pass
the fixtures could be removed if we are happy with this

[lekoala]

• It’d be worth testing the performance overhead of the runtime API (it looks like it’ll be very minimal) and seeing if we can remove our caching. There’s nothing inherently brittle about the caching we have in place, but less code to maintain is less code to maintain 😄

from what i see, it prevents loading the installed.php file which is a 2k line array. Not sure what is faster, instantiating a cache interface then get/set value from it or read a 2k array that is going to be opcached anyway.

[GuySartorelli]

Tests are passing and works well locally. Just needs a changelog entry, but that's obviously a separate PR to silverstripe/developer-docs
@kinglozzer do you want any further changes? I agree that removing the caching would be ideal but it's also not really a problem if it stays.

[kinglozzer]

One change, otherwise good to go I think. Thanks @lekoala

[lekoala]

i've also deleted the fixtures

[GuySartorelli]

Fantastic, thank you!
Please don't forget to open a PR to silverstripe/developer-docs to add a quick note to the 5.2.0 changelog.