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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix run-script not setting Path correctly on Windows #10700

Merged
merged 2 commits into from Apr 6, 2022
Merged

Fix run-script not setting Path correctly on Windows #10700

merged 2 commits into from Apr 6, 2022

Conversation

xPaw
Copy link
Contributor

@xPaw xPaw commented Apr 3, 2022

Fixes #10699.

getenv is case-insensitive as far as I understand it, which makes it not work correctly.

Not entirely sure why it works on 2.2, because the same code should be there.

It was changed here: bd4d624#diff-4ec8bee1b3bdbc03e2097675b4586f3bb92ff70afff4e6ee5e4cc25847816e97L569

@@ -571,7 +571,7 @@ protected function popEvent(): ?string
private function ensureBinDirIsInPath(): void
{
$pathEnv = 'PATH';
if (false === Platform::getEnv('PATH') && false !== Platform::getEnv('Path')) {
Copy link
Contributor

@herndlm herndlm Apr 3, 2022

Choose a reason for hiding this comment

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

I'm pretty sure the Platform::getEnv helper is used for a good reason and should not be removed :)

Copy link
Contributor Author

@xPaw xPaw Apr 3, 2022

Choose a reason for hiding this comment

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

Platform::getEnv will fallback to getenv which is not case sensitive, making this check moot.

Copy link
Contributor

@GrahamCampbell GrahamCampbell Apr 4, 2022

Choose a reason for hiding this comment

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

No, that's still not right. Env variables may exist in $_SERVER only, and not in putenv.

Copy link
Contributor Author

@xPaw xPaw Apr 4, 2022

Choose a reason for hiding this comment

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

Well, this is the code that was used before it was changed to getEnv. I don't really think getenv matters here because on Windows, Path (but not PATH) should exist in $_SERVER. All we're doing is just getting the right capitalization.

Regardless, if you have a better idea on how to fix scripts not running on Windows in 2.3, do suggest it.

@xPaw
Copy link
Contributor Author

@xPaw xPaw commented Apr 4, 2022

I did a bisect on main branch, and it stops working with this commit: d2ede37 which makes me think symfony/process (v2.8.52 => v5.4.0) upgrade is what actually made it stop working (disregarding the fact that PATH/Path capitalization is still broken that this PR attempts to fix back).

While that commit is checked out, downgrading back to v2.8.52 makes it work again, upgrading to v3.4.47 breaks. Testing it further, v3.4.2 appears to be the exact version when it broke.

@Seldaek
Copy link
Member

@Seldaek Seldaek commented Apr 6, 2022

Thanks, I believe the fix makes sense. Probably could be checked even further by using getenv() without args which returns the whole env then we can do a case sensitive check in that array I assume. If you want to improve further..

But I'll merge this now as it is definitely a fix already.

@Seldaek Seldaek merged commit c259879 into composer:main Apr 6, 2022
16 of 17 checks passed
@Seldaek Seldaek added this to the 2.3 milestone Apr 6, 2022
@Seldaek Seldaek added the Bug label Apr 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants