merged branch Seldaek/commands (PR #1470)

Commits
-------

d675c28 [FrameworkBundle] Use Router instead of RouterInterface
ae7ae8d [FrameworkBundle] Moved router_listener from web to router.xml since it depends on the router
35a9023 [FrameworkBundle] Added isEnabled to Router commands, fixes #1467
536d979 [Console] Added Command::isEnabled method that defines whether to add the command or not

Discussion
----------

[2.1] [Console] Added Command::isEnabled method

This addresses #1467.

The idea is to allow commands to evaluate whether they can run or not, since they are automatically registered.

- It's useful for the two router:* commands since they're optional (router can be disabled), but part of the FrameworkBundle that is not really optional.
- It could be useful for third party code as well.
- It's BC.
- aa95bb0d395810b29a3e654673e130736d9d1080 should address the issue in #1467, while the other commits just make sure the command is not registered at all if the router isn't standard.

One issue remains though:

- A few other services like twig helpers get the `ròuter` injected, this means that if there is really **no** router service defined, there is still an error. I'm not sure how to fix those beyond adding `on-invalid="null"` but I'm not sure if that's desirable. I guess we could argue that the router is a big candidate for replacement/suppression, and as such it should be truly optional, but if we do it I don't know where it'll lead. I don't want to end up in a situation where half the dependencies are optional to support every possible combination. @fabpot wdyt?

---------------------------------------------------------------------------

by kriswallsmith at 2011/06/28 16:19:46 -0700

I'd rather see us not register a command instead of register and then disable it. Can we do the same thing you've done here in the bundle's registerCommands() method?

---------------------------------------------------------------------------

by Seldaek at 2011/06/28 16:51:36 -0700

Note that it's never really registered. During the registration it's checked and skipped if not enabled.

However, doing it as you suggest means overriding/copy-pasting all the code from the core Bundle class, which I don't like so much. It also means adding code specific to those two commands in a somewhat unrelated place, which I also don't like.

I'm not saying the current solution is perfect, but from the alternatives I considered, it's the best I have found.

---------------------------------------------------------------------------

by stof at 2011/09/04 04:58:04 -0700

@Seldaek your branch conflicts with master. could you rebase it ?

@fabpot what do you think about this PR ?

---------------------------------------------------------------------------

by Seldaek at 2011/09/04 08:39:05 -0700

Rebased

Author: fabpot

Command/RouterApacheDumperCommand.php

@@ -17,6 +17,7 @@
 use Symfony\Component\Console\Output\OutputInterface;
 use Symfony\Component\Console\Output\Output;
 use Symfony\Component\Routing\Matcher\Dumper\ApacheMatcherDumper;
+use Symfony\Component\Routing\Router;
 
 /**
  * RouterApacheDumperCommand.
@@ -25,6 +26,21 @@
  */
 class RouterApacheDumperCommand extends ContainerAwareCommand
 {
+    /**
+     * {@inheritDoc}
+     */
+    public function isEnabled()
+    {
+        if (!$this->getContainer()->has('router')) {
+            return false;
+        }
+        $router = $this->getContainer()->get('router');
+        if (!$router instanceof Router) {
+            return false;
+        }
+        return parent::isEnabled();
+    }
+
     /**
      * @see Command
      */

Command/RouterDebugCommand.php

@@ -17,6 +17,7 @@
 use Symfony\Component\Console\Output\OutputInterface;
 use Symfony\Component\Console\Output\Output;
 use Symfony\Component\Routing\Matcher\Dumper\ApacheMatcherDumper;
+use Symfony\Component\Routing\Router;
 
 /**
  * A console command for retrieving information about routes
@@ -25,6 +26,21 @@
  */
 class RouterDebugCommand extends ContainerAwareCommand
 {
+    /**
+     * {@inheritDoc}
+     */
+    public function isEnabled()
+    {
+        if (!$this->getContainer()->has('router')) {
+            return false;
+        }
+        $router = $this->getContainer()->get('router');
+        if (!$router instanceof Router) {
+            return false;
+        }
+        return parent::isEnabled();
+    }
+
     /**
      * @see Command
      */

Resources/config/routing.xml

@@ -69,5 +69,15 @@
             <tag name="kernel.cache_warmer" />
             <argument type="service" id="router" />
         </service>
+
+        <service id="router_listener" class="%router_listener.class%">
+            <tag name="kernel.event_listener" event="kernel.request" method="onEarlyKernelRequest" priority="255" />
+            <tag name="kernel.event_listener" event="kernel.request" method="onKernelRequest" />
+            <tag name="monolog.logger" channel="request" />
+            <argument type="service" id="router" />
+            <argument>%request_listener.http_port%</argument>
+            <argument>%request_listener.https_port%</argument>
+            <argument type="service" id="logger" on-invalid="ignore" />
+        </service>
     </services>
 </container>

Resources/config/web.xml

@@ -24,16 +24,6 @@
             <argument type="service" id="logger" on-invalid="ignore" />
         </service>
 
-        <service id="router_listener" class="%router_listener.class%">
-            <tag name="kernel.event_listener" event="kernel.request" method="onEarlyKernelRequest" priority="255" />
-            <tag name="kernel.event_listener" event="kernel.request" method="onKernelRequest" />
-            <tag name="monolog.logger" channel="request" />
-            <argument type="service" id="router" />
-            <argument>%request_listener.http_port%</argument>
-            <argument>%request_listener.https_port%</argument>
-            <argument type="service" id="logger" on-invalid="ignore" />
-        </service>
-
         <service id="response_listener" class="%response_listener.class%">
             <tag name="kernel.event_listener" event="kernel.response" method="onKernelResponse" />
             <argument>%kernel.charset%</argument>