FIX use composer runtime api

lekoala commented 4 months ago

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

https://github.com/silverstripe/silverstripe-framework/issues/11134

Pull request checklist

[x] The target branch is correct
- See picking the right version
[x] 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.
[x] The commit messages follow our commit message guidelines
[x] The PR follows our contribution guidelines
[x] Code changes follow our coding conventions
[x] 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
[x] CI is green

lekoala commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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.

lekoala commented 4 months ago

i've also deleted the fixtures

silverstripe / silverstripe-framework