Skip to content

Conversation

@mwr
Copy link
Contributor

@mwr mwr commented Feb 17, 2016

Right now the sub process command uses php to invoke sub-processes.
That may not be the correct php binary to use.
Like if you have multiple php binaries available.

This pull request uses the PHP_BINARY Constant to use the same php binary the parent process was triggered with

@tkn98
Copy link
Contributor

tkn98 commented Feb 17, 2016

+1 from my end :)

@davidalger: Can you please restart the failed travis build?

@davidalger
Copy link
Member

@mwr Thanks for your contribution! Looks good in my opinion. :) Please accept the license CLA by clicking the Details link next to it in the list of pending checks below. Once you do that, this will be ready for the Magento team to review and process internally.

@tkn98 done. appreciate the heads-up

@davidalger davidalger added the CS label Feb 18, 2016
@mwr
Copy link
Contributor Author

mwr commented Feb 18, 2016

@davidalger CLA accepted.

@tkn98
Copy link
Contributor

tkn98 commented Feb 19, 2016

@davidalger: We're using PHP_BINARY with success in Magerun since some time. So I can say from practice it's working well. And in M2 you don't have any compatibility hurdles b/c of the PHP versions (in Magerun we also support older versions for M1).

@davidalger
Copy link
Member

@mwr Thanks. It's now up to the Magento team to review and process accordingly.

@tkn98 Personally, I'd like to see it merged as well, but that's unfortunately not my call to make, so we'll have to wait and see :)

@mazhalai
Copy link
Contributor

Some devs use windows as their local dev environment, and PHP_BINARY has issues on Windows OS. Can you confirm if this works on windows?

@tkn98
Copy link
Contributor

tkn98 commented Feb 26, 2016

@mazhalai: Windows is not part of the Magento2 system-requirements. Could you elaborate on these PHP_BINARY issues on the Windows OS?

@mazhalai
Copy link
Contributor

@tkn98 yes, windows is not part of system-requirements, however many devs(also our internal devs) still use it locally (only options are mac and windows, neither are supported according to system-requirements), which also includes running tests locally. I don't recall exact issues, but there were some issues on certain WAMP flavors, maybe we can check again.

@mazhalai mazhalai added PS and removed CS labels Feb 26, 2016
@mazhalai mazhalai self-assigned this Feb 26, 2016
@tkn98
Copy link
Contributor

tkn98 commented Feb 29, 2016

@mazhalai: Yes please do so if you require Windows and it's not part. Otherwise it would be a pity to degrade the system for the many, many other users that are using the software on base of the system-requirements. I perhaps should add that I do not know about any problems with that constant on the Windows OS. Just saying that I don't want to talk away any issues, I just do not see them.

@mazhalai
Copy link
Contributor

Internal ticket MAGETWO-49929.

@mazhalai mazhalai added Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Progress: accept labels Feb 29, 2016
@magento-team magento-team merged commit 113c330 into magento:develop Mar 8, 2016
magento-team pushed a commit that referenced this pull request Mar 8, 2016
magento-team pushed a commit that referenced this pull request Mar 8, 2016
@mazhalai
Copy link
Contributor

mazhalai commented Mar 8, 2016

@mwr @tkn98 Thank you for your contribution, your PR has been merged.

magento-team pushed a commit that referenced this pull request Nov 28, 2018
[Plankton] CHANGELOG.md for 2.1.16 Release
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Progress: accept

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants