[FrameworkBundle] Fixed server:start --router relative path issue #14124 #14129

abulford · 2015-03-31T09:01:48Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#14124
License	MIT
Doc PR	-

This ensures that a relative path can be used to specify the router script, and that if the router script cannot be found an error is reported and the command fails.

The file path for the router script is now determined and checked prior to the child process being forked, which seems cleaner. The same change could be made in ServerRunCommand.php, or possibly in the ServerCommand base class; to keep the scope small, though, this wasn't done as part of this bug fix.

xabbuh · 2015-03-31T14:21:45Z

src/Symfony/Bundle/FrameworkBundle/Command/ServerStartCommand.php

+                ->locateResource(sprintf('@FrameworkBundle/Resources/config/router_%s.php', $env))
+            ;
+        } else {
+            if (!file_exists($router)) {


Please merge both lines into a single elseif.

By the way, will this work if the user used a relative path?

OK, will merge to elseif.

Yep, I've tried this with a relative path and it seems to work fine. This was actually the main point of this change, and resembles how file checking is done in ServerRunCommand.php.

@xabbuh - I've changed to elseif in 898f97a.

symfony#14129 * Changed `if` nested in `else` to `elseif` * Modified `determineRouterScript` function prototype so that just the `router` argument is passed, rather than the whole `InputInterface`

xabbuh · 2015-04-01T19:02:15Z

src/Symfony/Bundle/FrameworkBundle/Command/ServerStartCommand.php

@@ -177,25 +182,44 @@ private function isOtherServerProcessRunning($address)
    }

    /**
+     * Determine the absolute file path for the router script based on user input and the environment.


This sentence needs to be changed as the user input isn't passed here.

OK, I've updated the comment to be more specific in 4be3757.

…fony#14124 This ensures that a relative path can be used to specify the router script, and that if the router script cannot be found an error is reported and the command fails. The file path for the router script is now determined and checked prior to the child process being forked, which seems cleaner. The same change could be made in ServerRunCommand.php, or possibly in the ServerCommand base class; to keep the scope small, though, this wasn't done as part of this bug fix.

symfony#14129 * Changed `if` nested in `else` to `elseif` * Modified `determineRouterScript` function prototype so that just the `router` argument is passed, rather than the whole `InputInterface`

symfony#14129 * Tweaked `determineRouterScript` function comment to better describe what the function is doing * Removed the usage of `file_exists` and instead made use of the `realpath` result to determine if the specified router file exists

aitboudad · 2015-04-02T05:58:07Z

src/Symfony/Bundle/FrameworkBundle/Command/ServerStartCommand.php

@@ -129,7 +134,7 @@ protected function execute(InputInterface $input, OutputInterface $output)
            return 1;
        }

-        if (null === $process = $this->createServerProcess($output, $address, $documentRoot, $input->getOption('router'), $env, null)) {
+        if (null === $process = $this->createServerProcess($output, $address, $documentRoot, $router, null)) {


you can avoid explicitly set null to the timeout argument as it's the default value.

Agreed, the null there isn't necessary. I was tempted to remove it but didn't want to meddle with the code any more than I needed to. @xabbuh - as the author of this class, would you like me to remove it?

I don't see anything wrong with you removing this as it is really useless.

// before $this->createServerProcess($output, $address, $documentRoot, $router, null) // after $this->createServerProcess($output, $address, $documentRoot, $router)

Fair enough, I was just wary of changing more than was necessary - I'm pretty new to GitHub so wasn't sure if I would be committing a faux pas!
All done in 139191f.

Thank you. Could also completely remove the $timeout parameter from the createServerProcess() method? It's not used anymore.

Could do, though the null would still need passing to the Process constructor, otherwise the timeout will be set to 60 seconds:

private function createServerProcess(OutputInterface $output, $address, $documentRoot, $router) { ... ... return new Process('exec '.$script, $documentRoot, null, null, null); }

It doesn't seem to do any harm leaving the option there for future use, but then, you're right, it's currently not being used to doesn't need to be there. Let me know if you'd prefer for it to be removed and, if so, I'll take it out.

Yes please :)

OK, done in a8c29c4.

…ymfony#14129 * Removed unnecessary `null` from `createServerProcess` call

…ony#14129 * Removed unused `timeout` argument from `createServerProcess` function

xabbuh · 2015-04-06T08:21:45Z

👍

abulford · 2015-04-10T06:47:21Z

@xabbuh - Is there anything else I need to do in order for this PR to be accepted?

xabbuh · 2015-04-10T07:02:38Z

@abulford Looks good to me.

ping @symfony/deciders

willdurand · 2015-04-13T14:48:39Z

👍

Tobion · 2015-04-13T15:28:59Z

👍

fabpot · 2015-04-13T15:34:05Z

Thank you @abulford.

…h issue #14124 (abulford) This PR was squashed before being merged into the 2.6 branch (closes #14129). Discussion ---------- [FrameworkBundle] Fixed server:start --router relative path issue #14124 | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #14124 | License | MIT | Doc PR | - This ensures that a relative path can be used to specify the router script, and that if the router script cannot be found an error is reported and the command fails. The file path for the router script is now determined and checked prior to the child process being forked, which seems cleaner. The same change could be made in ServerRunCommand.php, or possibly in the ServerCommand base class; to keep the scope small, though, this wasn't done as part of this bug fix. Commits ------- 0c1b9ba [FrameworkBundle] Fixed server:start --router relative path issue #14124

xabbuh reviewed Mar 31, 2015
View reviewed changes

abulford force-pushed the ticket_14124 branch from bc6237c to 898f97a Compare April 1, 2015 05:59

xabbuh reviewed Apr 1, 2015
View reviewed changes

abulford added 3 commits April 2, 2015 06:44

[FrameworkBundle] Implemented @xabbuh's suggestions for improvements to

9abad5b

symfony#14129 * Changed `if` nested in `else` to `elseif` * Modified `determineRouterScript` function prototype so that just the `router` argument is passed, rather than the whole `InputInterface`

abulford force-pushed the ticket_14124 branch from 898f97a to 4be3757 Compare April 2, 2015 05:45

aitboudad reviewed Apr 2, 2015
View reviewed changes

abulford added 2 commits April 3, 2015 18:36

[FrameworkBundle] Implemented @aitboudad's suggested improvement to s…

139191f

…ymfony#14129 * Removed unnecessary `null` from `createServerProcess` call

[FrameworkBundle] Implemented @xabbuh's suggested improvement to symf…

a8c29c4

…ony#14129 * Removed unused `timeout` argument from `createServerProcess` function

fabpot closed this Apr 13, 2015

willdurand mentioned this pull request Apr 13, 2015

[FrameworkBundle] Bundled router files have hardcoded front controller names #14339

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FrameworkBundle] Fixed server:start --router relative path issue #14124 #14129

[FrameworkBundle] Fixed server:start --router relative path issue #14124 #14129

abulford commented Mar 31, 2015

xabbuh Mar 31, 2015

xabbuh Mar 31, 2015

abulford Mar 31, 2015

abulford Apr 1, 2015

xabbuh Apr 1, 2015

abulford Apr 2, 2015

aitboudad Apr 2, 2015

abulford Apr 2, 2015

xabbuh Apr 2, 2015

aitboudad Apr 2, 2015

abulford Apr 3, 2015

xabbuh Apr 3, 2015

abulford Apr 4, 2015

xabbuh Apr 4, 2015

abulford Apr 4, 2015

xabbuh commented Apr 6, 2015

abulford commented Apr 10, 2015

xabbuh commented Apr 10, 2015

willdurand commented Apr 13, 2015

Tobion commented Apr 13, 2015

fabpot commented Apr 13, 2015

[FrameworkBundle] Fixed server:start --router relative path issue #14124 #14129

[FrameworkBundle] Fixed server:start --router relative path issue #14124 #14129

Conversation

abulford commented Mar 31, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xabbuh commented Apr 6, 2015

abulford commented Apr 10, 2015

xabbuh commented Apr 10, 2015

willdurand commented Apr 13, 2015

Tobion commented Apr 13, 2015

fabpot commented Apr 13, 2015