Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Stop requiring composer itself #988

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

xolf
Copy link
Contributor

@xolf xolf commented Apr 22, 2024

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 composer/composer#11940

馃殽 Resolves ISSUE_ID
#987

馃О Changes

The readme/metrics requires Composer itself. As mentioned by the maintainers of Composer, see composer/composer#11940 (comment), 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.

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 composer/composer#11940
@xolf xolf changed the title Stop requiring composer itself fix: Stop requiring composer itself Apr 22, 2024
@Seldaek
Copy link

Seldaek commented Apr 22, 2024

Checking again for Composer usage in this repo now that I'm on a computer.. it seems Composer\Factory is used here

$this->cache_dir = Factory::createConfig()->get('cache-dir');

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.

Copy link
Member

@erunion erunion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we (un)fortunately use the cache_dir that Composer uses for caching data about your project so we don't have to make that API call for every call we make to our Metrics service. It's been a while since I've worked on this SDK, and used Composer, but if there's another way to access that cache directory without having to use a Composer API for it I'd love to move to that as I agree, and have never liked requiring a direct dependency on Composer for that.

@Seldaek
Copy link

Seldaek commented Apr 23, 2024

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 erunion added the area:php Issues related to our PHP SDK label Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:php Issues related to our PHP SDK
Development

Successfully merging this pull request may close these issues.

None yet

3 participants