Use separated function to resolve command and related arguments #11497
Conversation
| /** | ||
| * Finds The PHP executable arguments. | ||
| * | ||
| * @return string|false The PHP executable path or false if it cannot be found |
There was a problem hiding this comment.
The type and description are wrong ;)
|
Doesn't this have to be merged into the 2.3 branch since the SensioDistributionBundle can be used with older Symfony versions too? |
| @@ -64,4 +66,19 @@ public function find() | |||
|
|
|||
| return $this->executableFinder->find('php', false, $dirs); | |||
| } | |||
|
|
|||
| /** | |||
| * Finds The PHP executable arguments. | |||
There was a problem hiding this comment.
ok :) just a copy / paste of the previous comment line
|
@xabbuh I took care not to cause BC break by adding a param to find() function |
|
👍 |
|
@JJK801 But the |
| { | ||
| // HHVM support | ||
| if (defined('HHVM_VERSION')) { | ||
| return (false !== ($hhvm = getenv('PHP_BINARY')) ? $hhvm : PHP_BINARY).' --php'; | ||
| return (false !== ($hhvm = getenv('PHP_BINARY')) ? $hhvm : PHP_BINARY) . ($includeArgs ? ' '.$this->findArguments() : '' ); |
|
this should be covered by a test IMO |
|
@stof done ;) |
|
@fabpot is it ok to merge it ? |
|
@JJK801 👍 This looks good to me know. However, can you rebase your PR on current master to remove the merge commit? Thanks. |
|
This should be merged in 2.3 to be able to resolve sensiolabs/SensioDistributionBundle#150. |
|
ping @symfony/deciders |
|
I guess under normal circumstances we wouldnt merge something like this into 2.3. but I guess it can be seen as a bug fix, rather than a new feature. also alternative language implementation compatibility issue are sort of a new category of issue to deal with. |
|
@fabpot done, rebased and squashed |
|
@lsmith77 So, is it a +1 from you? |
|
yes .. +1 |
|
👍 |
|
Thank you @JJK801. |
…ments (JJK801) This PR was submitted for the master branch but it was merged into the 2.3 branch instead (closes #11497). Discussion ---------- Use separated function to resolve command and related arguments Hi, This PR split command and related arguments resolution into two distinct functions. It will help to solve the HHVM issue sensiolabs/SensioDistributionBundle#150 . Thanks, Jérémy | Q | A | ------------- | --- | Bug fix? | yes | New feature? | yes | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | ~ | License | MIT | Doc PR | ~ Commits ------- ee75af0 Use separated function to resolve command and related arguments
|
@fabpot my pleasure |
Hi,
This PR split command and related arguments resolution into two distinct functions.
It will help to solve the HHVM issue sensiolabs/SensioDistributionBundle#150 .
Thanks,
Jérémy