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

Update MediaInfoCommandRunner.php #107

Merged
merged 2 commits into from
Mar 22, 2020
Merged

Update MediaInfoCommandRunner.php #107

merged 2 commits into from
Mar 22, 2020

Conversation

cklm
Copy link
Contributor

@cklm cklm commented Mar 11, 2020

fixes #106

@mhor
Copy link
Owner

mhor commented Mar 12, 2020

Hi @cklm thanks for your PR, Symfony Process component documentation use this syntax for Windows env var:

// On Windows
$process = new Process('echo "!MESSAGE!"');

// On both Unix-like and Windows
$process->run(null, ['MESSAGE' => 'Something to output']);

Could you try with this syntax , and make sure your fix is compatible accross currently supported php-mediainfo, symfony version (https://github.com/mhor/php-mediainfo/blob/master/composer.json#L14) ?

@cklm
Copy link
Contributor Author

cklm commented Mar 13, 2020

Hi @mhor the difference between the percent-sign and the exclamation-mark is described here:
https://stackoverflow.com/questions/14347038/why-are-my-set-commands-resulting-in-nothing-getting-stored/14347131#14347131

Using the percent-signs seems more compatible to me - the exclamation marks must be enabled first with setlocal EnableDelayedExpansion which must be added additionally in win-xp (nobody should care about xp, but because ;) ).
Best practice would be to use $process->run(null, ['MESSAGE' => 'Something to output']); and give it to symfony-component but that would require more refactoring, since it cannot be set in the constructor.

Could you try with this syntax , and make sure your fix is compatible accross currently supported php-mediainfo, symfony version

Does not make a difference between php- or symfony-versions, since it is executed directly in shell in windows. Would be a problem, when some windows-version changes that, but of today that works in every windows-version.

@cklm
Copy link
Contributor Author

cklm commented Mar 20, 2020

ping @mhor

@mhor mhor merged commit c587d0e into mhor:master Mar 22, 2020
@mhor
Copy link
Owner

mhor commented Mar 22, 2020

Thanks you @cklm, Released as 4.1.3

@cklm cklm deleted the patch-2 branch March 22, 2020 18:02
@cklm
Copy link
Contributor Author

cklm commented Mar 22, 2020

Thank you!

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.

Process does not work on Windows / wrong ENV-definition
2 participants