merged branch Tobion/requirementscheck (PR #5187)

fabpot · fabpot · commit 2cf50b78017b · 2012-08-30T16:40:08.000+02:00
Commits ------- 3363832 extended phpdoc of ConfigurableRequirementsInterface 5f64503 [Routing] added test for disabled requirements check 4225869 [Routing] allow disabling the requirements check on URL generation Discussion ---------- [Routing] allow disabling the requirements check on URL generation This adds the possibility to disable the requirements check (`strict_requirements = null`) on URL generation. To sum up, here the possibilities: - `strict_requirements = true`: throw exception for mismatching requirements (most useful in development environment). - `strict_requirements = false`: don't throw exception but return null as URL for mismatching requirements and log it (useful when you cannot control all params because they come from third party libs or something but don't want to have a 404 in production environment. it logs the mismatch so you can review it). - `strict_requirements = null`: Return the URL with the given parameters without checking the requirements at all. When generating an URL you should either trust your params or you validated them beforehand because otherwise it would break your link anyway (just as with strict_requirements=false). So in production environment you should know that params allways pass the requirements. Thus you could disable the check on each URL generation for performance reasons. If you have 300 links on a page and each URL at least one param you safe 300 unneeded `preg_match` calls. I tested the performance in one of my projects. The rendering time of a single template that contains ~300 links with several params was reduced from avg. 46ms to avg. 42ms. That are 8.7% reduction in the twig layer where the links are created on each request. So this option combines the improved usability of strict_requirements=false with an additional increased performance. --------------------------------------------------------------------------- by fabpot at 2012-08-30T13:40:38Z Can you put the comment about all the possibilities you have mentioned here in the phpdoc for future reference? Thanks. --------------------------------------------------------------------------- by Tobion at 2012-08-30T13:49:25Z In `ConfigurableRequirementsInterface` or which phpdoc would you like to have it? Because `ConfigurableRequirementsInterface` already has it explained. But I can extend its description if thats what you mean. --------------------------------------------------------------------------- by fabpot at 2012-08-30T13:50:40Z The comment in the PR is more explicit and more detailed than the one in the interface. So, yes, basically, it would be great if you can move all the information in the interface phpdoc. --------------------------------------------------------------------------- by Tobion at 2012-08-30T14:35:59Z Done.
diff --git a/src/Symfony/Component/Routing/Generator/ConfigurableRequirementsInterface.php b/src/Symfony/Component/Routing/Generator/ConfigurableRequirementsInterface.php
@@ -12,25 +12,44 @@
 namespace Symfony\Component\Routing\Generator;
 
 /**
- * ConfigurableRequirementsInterface must be implemented by URL generators in order
- * to be able to configure whether an exception should be generated when the
- * parameters do not match the requirements.
+ * ConfigurableRequirementsInterface must be implemented by URL generators that
+ * can be configured whether an exception should be generated when the parameters
+ * do not match the requirements. It is also possible to disable the requirements
+ * check for URL generation completely.
+ *
+ * The possible configurations and use-cases:
+ * - setStrictRequirements(true): Throw an exception for mismatching requirements. This
+ *   is mostly useful in development environment.
+ * - setStrictRequirements(false): Don't throw an exception but return null as URL for
+ *   mismatching requirements and log the problem. Useful when you cannot control all
+ *   params because they come from third party libs but don't want to have a 404 in
+ *   production environment. It should log the mismatch so one can review it.
+ * - setStrictRequirements(null): Return the URL with the given parameters without
+ *   checking the requirements at all. When generating an URL you should either trust
+ *   your params or you validated them beforehand because otherwise it would break your
+ *   link anyway. So in production environment you should know that params always pass
+ *   the requirements. Thus this option allows to disable the check on URL generation for
+ *   performance reasons (saving a preg_match for each requirement every time a URL is
+ *   generated).
  *
  * @author Fabien Potencier <fabien@symfony.com>
+ * @author Tobias Schultze <http://tobion.de>
  */
 interface ConfigurableRequirementsInterface
 {
     /**
      * Enables or disables the exception on incorrect parameters.
+     * Passing null will deactivate the requirements check completely.
      *
-     * @param Boolean $enabled
+     * @param Boolean|null $enabled
      */
     public function setStrictRequirements($enabled);
 
     /**
-     * Gets the strict check of incorrect parameters.
+     * Returns whether to throw an exception on incorrect parameters.
+     * Null means the requirements check is deactivated completely.
      *
-     * @return Boolean
+     * @return Boolean|null
      */
     public function isStrictRequirements();
 }
diff --git a/src/Symfony/Component/Routing/Generator/UrlGenerator.php b/src/Symfony/Component/Routing/Generator/UrlGenerator.php
@@ -100,7 +100,7 @@ public function getContext()
      */
     public function setStrictRequirements($enabled)
     {
-        $this->strictRequirements = (Boolean) $enabled;
+        $this->strictRequirements = null === $enabled ? null : (Boolean) $enabled;
     }
 
     /**
@@ -146,7 +146,7 @@ protected function doGenerate($variables, $defaults, $requirements, $tokens, $pa
             if ('variable' === $token[0]) {
                 if (!$optional || !array_key_exists($token[3], $defaults) || (string) $mergedParams[$token[3]] !== (string) $defaults[$token[3]]) {
                     // check requirement
-                    if (!preg_match('#^'.$token[2].'$#', $mergedParams[$token[3]])) {
+                    if (null !== $this->strictRequirements && !preg_match('#^'.$token[2].'$#', $mergedParams[$token[3]])) {
                         $message = sprintf('Parameter "%s" for route "%s" must match "%s" ("%s" given).', $token[3], $name, $token[2], $mergedParams[$token[3]]);
                         if ($this->strictRequirements) {
                             throw new InvalidParameterException($message);
diff --git a/src/Symfony/Component/Routing/Tests/Generator/UrlGeneratorTest.php b/src/Symfony/Component/Routing/Tests/Generator/UrlGeneratorTest.php
@@ -207,6 +207,14 @@ public function testGenerateForRouteWithInvalidOptionalParameterNonStrictWithLog
         $this->assertNull($generator->generate('test', array('foo' => 'bar'), true));
     }
 
+    public function testGenerateForRouteWithInvalidParameterButDisabledRequirementsCheck()
+    {
+        $routes = $this->getRoutes('test', new Route('/testing/{foo}', array('foo' => '1'), array('foo' => 'd+')));
+        $generator = $this->getGenerator($routes);
+        $generator->setStrictRequirements(null);
+        $this->assertSame('/app.php/testing/bar', $generator->generate('test', array('foo' => 'bar')));
+    }
+
     /**
      * @expectedException Symfony\Component\Routing\Exception\InvalidParameterException
      */
