Skip to content

Introduce ComposerHelper for composer related config operations#1384

Merged
ondrejmirtes merged 1 commit intophpstan:1.7.xfrom
canvural:composer-config
Jun 2, 2022
Merged

Introduce ComposerHelper for composer related config operations#1384
ondrejmirtes merged 1 commit intophpstan:1.7.xfrom
canvural:composer-config

Conversation

@canvural
Copy link
Copy Markdown
Contributor

@canvural canvural commented Jun 1, 2022

This PR is mostly for #1355

It extracts a helper class to determine Composer vendor-dir

@ondrejmirtes
Copy link
Copy Markdown
Member

Otherwise - awesome, I didn't even know about the COMPOSER env variable.

Also - I believe there are few more hardcoded places for vendor in bin/phpstan. You can set PhpStorm to override the default file type so that it's highlighted as PHP too!

@canvural
Copy link
Copy Markdown
Contributor Author

canvural commented Jun 2, 2022

Also - I believe there are few more hardcoded places for vendor in bin/phpstan. You can set PhpStorm to override the default file type so that it's highlighted as PHP too!

I guess this one is fine. It's always vendor. I changed the other occurrence.

@ondrejmirtes ondrejmirtes force-pushed the composer-config branch 2 times, most recently from 6f11f57 to c83c1e9 Compare June 2, 2022 11:08
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Alright, some more ideas. There's an inconsistency - getComposerJsonPath returns an absolute path, but getVendorDir returns a relative path. Both should return absolute paths.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also, I don't like the optional , ?array $composerConfig = null) parameter. In that case the function does something completely different. I'd like two different functions - one that you call if you have the JSON array, another one if you don't.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, I refactored it little bit. Then it became clear that I don't need two methods. First getComposerConfig should be called and if returns null that means composer.json is not found or it's not a valid json file, so getVendorDirFromComposerConfig shouldn't be called.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There might be garbage in this setting. We should normalize that by doing trim($dir, '/').

bin/phpstan Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This can return null which means it's gonna get concatenated lower leading to double // in the path...

if ($composer !== null) {
$vendorDirectory = ComposerHelper::getVendorDirFromComposerConfig(getcwd(), $composer);
} else {
$vendorDirectory = getcwd() . '/' . 'vendor';
Copy link
Copy Markdown
Contributor Author

@canvural canvural Jun 2, 2022

Choose a reason for hiding this comment

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

This still uses the hardcoded vendor But this was the only way I could think of. Anyway, this case should be fairly rare.

@ondrejmirtes ondrejmirtes merged commit 6ee8810 into phpstan:1.7.x Jun 2, 2022
@ondrejmirtes
Copy link
Copy Markdown
Member

Thank you very much!

@canvural canvural deleted the composer-config branch June 2, 2022 15:42
@ondrejmirtes
Copy link
Copy Markdown
Member

Did some additional improvements: 39bbdb9...1232186

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants