Add "executable" option to server:run console command #23721

tomasfejfar · 2017-07-30T16:18:14Z

Q	A
Branch?	3.4
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	there are no relevant tests for WebServerBundle
Fixed tickets	#22101
License	MIT
Doc PR	will be added when finalized

The PR adds the "executable" option to server:run command to allow running server with custom executable (as in bin\console server:run --executable="php -c /path/to/ini")

I'm having a hard time testing this. The webserver bundle does not have any tests. I tried to add the symfony repo locally to my other project to test it, but I got:

Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - Can only install one of: symfony/symfony[dev-custom-server-executable, v3.3.3].
    - don't install symfony/web-server-bundle v3.3.3|don't install symfony/symfony dev-dev-custom-server-executable
    - Installation request for symfony/symfony dev-dev-custom-server-executable -> satisfiable by symfony/symfony[dev-dev-custom-server-executable].
    - Installation request for symfony/web-server-bundle 3.3.3 -> satisfiable by symfony/web-server-bundle[v3.3.3], symfony/symfony[v3.3.3].

Any pointers welcome.

tomasfejfar · 2017-07-30T16:19:54Z

src/Symfony/Bundle/WebServerBundle/WebServerConfig.php

@@ -69,6 +70,8 @@ public function __construct($documentRoot, $env, $address = null, $router = null
        if (!ctype_digit($this->port)) {
            throw new \InvalidArgumentException(sprintf('Port "%s" is not valid.', $this->port));
        }
+
+        $this->executable = $executable;


I welcome any suggestions on how to validate this. Using is_executable would only work for full paths. And this can be anything from xdebug (from my original issue) to /usr/bin/php7_custom_build -c /custom/php.ini.

For now the command would just fail on the higher layer (in WebServer::run)

chalasr · 2017-07-31T09:14:15Z

src/Symfony/Bundle/WebServerBundle/WebServer.php

+            $finder = new PhpExecutableFinder();
+            if (false === $executable = $finder->find()) {
+                throw new \RuntimeException('Unable to find the PHP executable.');
+            }


I'd move this logic to WebServerConfig constructor now that it holds an executable.

tomasfejfar · 2017-07-31T19:06:47Z

src/Symfony/Bundle/WebServerBundle/Command/ServerRunCommand.php

+                $env,
+                $input->getArgument('addressport'),
+                $input->getOption('router'),
+                $input->getOption('executable')


Is in your oppinion "executable" correct name for what it holds? (e.g.: php or php -c /path/to/ini or /usr/bin/custom_php). Other options I considered were "command" (although that clashes with Symfony command) or "binary" (which is usually the file itself, not the whole command).

I believe executable and binary in this scenario are synonyms. Bash calls it command, in my mind it makes a bit more sense (even though the command will be the whole string that the Process will run).

tomasfejfar · 2017-07-31T20:19:10Z

I added a test for WebServerConfig class. In first commit the test is added for the existing implementation. Then a change commit adds the "executable" parameter and tests that it works as expected.

I still couldn't properly test whether the code works as a whole (see first message) and would very much welcome any suggestions on how to do it.

fabpot · 2017-08-01T05:55:20Z

Instead of adding an option for an edge case, I propose to be able to configure the executable via an env var.

fernandocarletti · 2017-08-01T05:55:54Z

I was working on the very same issue, but I came with a different approach: 3.4...xurumelous:feature/cli-settings-server

I did not change the $binary variable , only added support for php settings definition: php bin/console server:run localhost:8001 -c 'xdebug.remote_port=9001' -c 'xdebug.idekey=SAMPLE'. It is a bit more limited, since a custom php.ini cannot be set.

tomasfejfar · 2017-08-02T21:30:35Z

@fabpot that makes sense. I'll try to update the code asap.

@xurumelous yes, I thought about that as well. It's actually what most people use when debugging CLI apps. But it would not work for my case. If you look at #22101 - my exact use cases is xdebug wrapper for the php binary. What's great about it is that you have centralized configuration and all you need to know is that you need to call xdebug instead of php. I always had a hard time describing someone how to pass the params - which is why I created the wrapper. It's setup once and forget. Also you may have multiple PHP versions and want to run it in a specific one (i.e. test something in 7.0 even though 7.1 is your default).

fernandocarletti · 2017-08-02T21:44:36Z

@tomasfejfar Yes, it makes sense. Are you making the change for the env variable? If you need help you can count on me. I really need to be able to set xdebug.remote_port per project. Right now I'm basically doing it manually, getting the command the server:run would run and running it myself.

tomasfejfar · 2017-08-02T22:46:04Z

This implementation should allow you to set the executable to php -dxdebug.remote_port=123

If you could do the change for the env variable and send a PR from your fork to this branch, that would be great. I'll probably not have time for it this week.

Did you manage to test your changes somehow? How did you approach it? I've coded the whole thing without ever running it because I couldn't get it to work (as noted in the first post) :(

tomasfejfar · 2017-08-02T22:49:37Z

@fabpot what about setting the executable as a parameter and pulling it from the DI? I would prefer changing parameters.yml to fiddling with ENV variables. Also it could have specific settings for different envs (dev/prod).

fernandocarletti · 2017-08-02T22:53:45Z

@tomasfejfar I believe I'll have time for this later today or tomorrow night.

I tested in the vendor folder. I ran composer with --prefer-source option, so it cloned the repository for each dependency. I worked on this in the vendor folder, when it was ready, I patched my fork with the changes I made (git diff >> /tmp/patch then patch -p1 < /tmp/patch in my fork). I'm not very used to Symfony development, but that was my workaround to test it before running.

I thought you have tested it, so I did not mention one issue before, I'll comment on the line I'm talking about.

fernandocarletti · 2017-08-02T22:57:03Z

src/Symfony/Bundle/WebServerBundle/WebServer.php


-        $process = new Process(array($binary, '-S', $config->getAddress(), $config->getRouter()));
+        $process = new Process(array($executable, '-S', $config->getAddress(), $config->getRouter()));


I believe we should "explode" $executable string. Depending on how the Process class is escaping the array, the first one will be the command name, see the example:

$ 'echo -e test' bash: echo -e test: command not found

If you pass a whole string as the command, it will be interpreted as its name. I did not test it, but I believe it will do the same in the end.

Oh, yes... definitely. Thanks.

This is not as simple as it seems. Consider following executable: php -dmemory_limit=16M -dzend_extension="C:\Program Files\some_extension.dll". If I were to split this with spaces, it would break around the Program Files part. And parsing CLI arguments is way over the scope of this PR IMHO. I've googled for some ready-made solutions and everything is in the ballpark of hundred lines of code.

I tried to repurpose \Symfony\Component\Console\Input\StringInput for this, but it feels very hacky and I needed to add new public method to it.

See 50fda23

Tested with php index.php s:r --docroot web\ --executable "php -dmemory_limit=512M -dinclude_path=\"c:\Program Files\""

Any suggestions welcome

tomasfejfar · 2017-08-07T20:53:22Z

I kept the various commits to allow easy revert if some of them are not OK.

Tested on Windows:

php index.php s:r --docroot web\
SET SYMFONY_SERVER_EXECUTABLE=xdebug&&php index.php s:r --do
6D40
croot web\
SET SYMFONY_SERVER_EXECUTABLE=php -dmemory_limit=512M -dinclude_path="c:\\Program Files"&& php index.php s:r --docroot web\

Works as expected.

All green. Ready for review.

tomasfejfar · 2017-08-15T12:09:29Z

Can I do anything else to move this forward?

tomasfejfar · 2017-08-21T15:37:37Z

Any update on this?

fabpot · 2017-08-22T20:50:50Z

src/Symfony/Bundle/WebServerBundle/WebServer.php

+        $executable = $config->getExecutable();
+
+        // we need to separate the executable from the rest of the string as StringInput won't handle it
+        $firstPartOfCommand = explode(' ', $executable)[0];


What about a path with a space in it?

fabpot · 2017-08-22T20:51:29Z

src/Symfony/Bundle/WebServerBundle/WebServerConfig.php

+        $executable = null;
+
+        $envExecutable = getenv('SYMFONY_SERVER_EXECUTABLE');
+        if ($envExecutable !== false) {


should be false !== $envExecutable

fabpot · 2017-08-22T20:51:41Z

src/Symfony/Bundle/WebServerBundle/WebServerConfig.php

+            $executable = $envExecutable;
+        }
+
+        if ($executable === null) {


Yoda-style as well

fabpot · 2017-08-22T20:53:05Z

src/Symfony/Component/Console/Input/ArgvInput.php

@@ -177,12 +177,12 @@ private function parseArgument($token)
            $arg = $this->definition->getArgument($c);
            $this->arguments[$arg->getName()] = $arg->isArray() ? array($token) : $token;

-        // if last argument isArray(), append token to last argument
+            // if last argument isArray(), append token to last argument


should be reverted (fabbot false positive)

fabpot · 2017-08-22T20:53:09Z

src/Symfony/Component/Console/Input/ArgvInput.php

        } elseif ($this->definition->hasArgument($c - 1) && $this->definition->getArgument($c - 1)->isArray()) {
            $arg = $this->definition->getArgument($c - 1);
            $this->arguments[$arg->getName()][] = $token;

-        // unexpected argument
+            // unexpected argument


should be reverted (fabbot false positive)

nicolas-grekas

@tomasfejfar: the current logic is broken, because it implicitly mixes escaped and unescaped command line arguments (this previous comment hints it.) Instead, what about not allowing command line args in the new env var? That would require you to create a small shell wrapper to add them there, thus make everything else much simpler.
If you really want to push args in the env var, the logic needs to consider that:

the env var is already shell-safe and doesn't need further escaping
the added args need to be escaped
the concatenated string needs to be prefixed by "exec" on Linux, to not break signalling the child process.

Escaping should be done in a similar way as done here:
https://github.com/symfony/symfony/blob/3.4/src/Symfony/Component/Process/ProcessBuilder.php#L270-L273

nicolas-grekas · 2017-09-26T13:55:08Z

what about not allowing command line args in the new env var?

Oh btw, if you follow this path, what about putting that env var in PhpExecutableFinder?

nicolas-grekas · 2017-10-08T15:53:30Z

Moving to 4.1. Rebase on master might be needed, where PHP 7.1 features can be used btw.

@tomasfejfar

…_BINARY` env var (nicolas-grekas) This PR was merged into the 4.1-dev branch. Discussion ---------- [Process] Make `PhpExecutableFinder` look for the `PHP_BINARY` env var | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #22101 | License | MIT | Doc PR | - I think this is enough to fix the linked issue and thus replace #23721. ping @tomasfejfar FYI Commits ------- 4bd01f2 [Process] Make `PhpExecutableFinder` look for the `PHP_BINARY` env var

carsonbot added Status: Needs Review Feature labels Jul 30, 2017

tomasfejfar commented Jul 30, 2017

View reviewed changes

chalasr reviewed Jul 31, 2017

View reviewed changes

chalasr added this to the 3.4 milestone Jul 31, 2017

chalasr added the WebServerBundle label Jul 31, 2017

tomasfejfar commented Jul 31, 2017

View reviewed changes

tomasfejfar force-pushed the custom-server-executable branch from 85061fb to cde9568 Compare July 31, 2017 20:16

tomasfejfar force-pushed the custom-server-executable branch 4 times, most recently from be96c77 to 011d1db Compare July 31, 2017 20:31

fernandocarletti reviewed Aug 2, 2017

View reviewed changes

tomasfejfar force-pushed the custom-server-executable branch 3 times, most recently from a990d3c to 8970e9f Compare August 7, 2017 20:43

fabpot requested changes Aug 22, 2017

View reviewed changes

nicolas-grekas requested changes Sep 26, 2017

View reviewed changes

nicolas-grekas modified the milestones: 3.4, 4.1 Oct 8, 2017

tomasfejfar force-pushed the custom-server-executable branch from 8970e9f to f14fde3 Compare November 3, 2017 22:34

tomasfejfar added 7 commits November 3, 2017 23:35

Add tests to WebServerConfig

65dcd7a

Add "executable" option to server:run console command

9d4f729

Move executable finding fallback to WebServerConfig

2feb064

Tests for "executable" option

6c8aa40

Proof of concept - executable with params

c051b18

Pull executable from ENV

426b30a

Revert CLI param and set using ENV

b5ff2ba

tomasfejfar force-pushed the custom-server-executable branch from f14fde3 to b5ff2ba Compare November 3, 2017 22:35

nicolas-grekas mentioned this pull request Dec 29, 2017

[Process] Make PhpExecutableFinder look for the PHP_BINARY env var #25629

Merged

fabpot closed this Dec 31, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Add "executable" option to server:run console command #23721

Add "executable" option to server:run console command #23721

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!


		$process = new Process(array($binary, '-S', $config->getAddress(), $config->getRouter()));
		$process = new Process(array($executable, '-S', $config->getAddress(), $config->getRouter()));

Uh oh!

Add "executable" option to server:run console command #23721

Add "executable" option to server:run console command #23721

Uh oh!

Conversation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!