search

readmeio / metrics-sdks

SDKs and integrations for ReadMe's Metrics platform
https://readme.com/metrics
10 stars 23 forks source link

fix: Stop requiring composer itself #988

Closed xolf closed 2 months ago

xolf commented 5 months ago

Requiring composer itself might lead to a difference between the project composer version and the locally installed version. By this no composer actions can be performed any more in case of breaking changes.

See https://github.com/composer/composer/issues/11940

🚥 Resolves ISSUE_ID
https://github.com/readmeio/metrics-sdks/issues/987

🧰 Changes

The readme/metrics requires Composer itself. As mentioned by the maintainers of Composer, see https://github.com/composer/composer/issues/11940#issuecomment-2068728878, this is a bad design and lead to unusable composer installation if the version within the vendor folder differs from the system-wide installation.

I'm not quite sure, what are the reasons to require composer/composer itself, but not other package are requiring composer, so one should be safe to remove.

🧬 QA & Testing

Provide as much information as you can on how to test what you've done.

Seldaek commented 5 months ago

Checking again for Composer usage in this repo now that I'm on a computer.. it seems Composer\Factory is used here https://github.com/readmeio/metrics-sdks/blob/d97b3d0b9e32bba51ce33b572cc776e49406c4cb/packages/php/src/Metrics.php#L260

So this PR will break that, and it should be adjusted/removed.

In a way I kinda see the use case of reusing Composer's cache dir, although I also kinda disagree that other software should dump stuff in there. But I mainly disagree that it's a good idea to pull the entire composer codebase and dependencies into the project just to read a cache-dir value out of the composer.json file. That seems really overkill.

Unfortunately I'm not sure there is a sensible way to expose only Composer\Config in a standalone package as it depends on quite a few other things, the entire composer core is intertwined in many ways that make it hard to extract only some bits of it.

Seldaek commented 5 months ago

Yeah i mean you can read composer.json but if that doesn't set a cache-dir you have to figure out the composer home dir then cache dir, which would mean copying some amount of code from Composer\Factory i guess. I'm not sure what's worse..

erunion commented 2 months ago

Closing this out because we've unfortunately not been able to come up with a better solution to this.