Tiny refactoring

Fixed a bug with rule configuration Added UnknownRuleException Better Tests
frickelbruder · Sep 13, 2016 · 6befa5e · 6befa5e
1 parent 6357ae6
commit 6befa5e
Show file tree

Hide file tree

Showing 7 changed files with 129 additions and 43 deletions.
diff --git a/src/Configuration/Configuration.php b/src/Configuration/Configuration.php
@@ -1,6 +1,7 @@
 <?php
 namespace Frickelbruder\KickOff\Configuration;
 
+use Frickelbruder\KickOff\Configuration\Exceptions\UnknownRuleException;
 use Frickelbruder\KickOff\Rules\RequiresHeaderInterface;
 use Frickelbruder\KickOff\Rules\RuleInterface;
 use Frickelbruder\KickOff\Yaml\Yaml;
@@ -32,15 +33,16 @@ public function getSections() {
     }
 
     public function build($filename) {
-        $config = $this->buildConfig( $filename );
+        $config = $this->readConfigs( $filename );
         $this->prepareConfiguredItems( $config );
         $this->buildSections( $config );
     }
 
-    private function buildConfig($filename) {
+    private function readConfigs($filename) {
         $config = $this->yaml->fromFile( $filename );
         $defaultRulesConfig = $this->yaml->fromFile( __DIR__ . '/../config/Rules.yml' );
         $config = $this->mergeUserConfigWithDefaults( $config, $defaultRulesConfig );
+
         return $config;
     }
 
@@ -78,9 +80,9 @@ private function enrichTarget(TargetUrl $targetUrl, $config) {
                 $targetUrl->$key = $config[ $key ];
             }
         }
-        if(isset($config['headers'])) {
-            foreach($config['headers'] as $header) {
-                $targetUrl->addHeader($header[0], $header[1]);
+        if( isset( $config['headers'] ) ) {
+            foreach( $config['headers'] as $header ) {
+                $targetUrl->addHeader( $header[0], $header[1] );
             }
         }
     }
@@ -92,6 +94,7 @@ private function enrichTarget(TargetUrl $targetUrl, $config) {
      */
     private function generateRules($config) {
         $ruleBuilder = new RuleBuilder();
+
         return $ruleBuilder->buildRules( $config );
     }
 
@@ -116,19 +119,41 @@ private function getRulesForSection($sectionConfig, $mainConfig) {
         $result = array();
         foreach( $sectionConfig['rules'] as $name ) {
             $plainName = $name;
+            $configuration = array();
             if( is_array( $name ) ) {
                 list( $plainName, $configData ) = each( $name );
-                foreach( $configData as $variableBlock ) {
-                    $rule['configuration'][] = array( 'set', $variableBlock );
-                }
+                $configuration = $this->addConfigurationToRuleConfig( $configData );
             }
-            $rule = $mainConfig['Rules'][ $plainName ];
+            $rule = $this->fetchRuleBase( $plainName, $mainConfig );
+            $rule['configuration'] = $configuration;
             $result[ $plainName ] = $rule;
         }
 
         return $this->generateRules( $result );
     }
 
+    private function fetchRuleBase($name, $config) {
+        if( !isset( $config['Rules'][ $name ] ) ) {
+            throw new UnknownRuleException( 'Rule "' . $name . "' not known.'" );
+        }
+
+        return $config['Rules'][ $name ];
+    }
+
+    /**
+     * @param $configData
+     *
+     * @return array
+     */
+    private function addConfigurationToRuleConfig($configData) {
+        $configuration = array();
+        foreach( $configData as $variableBlock ) {
+            $configuration[] = array( 'set', $variableBlock );
+        }
+
+        return $configuration;
+    }
+
     private function getSectionTargetUrl($config, $rules) {
         $target = clone $this->defaultTargetUrl;
 
@@ -153,15 +178,38 @@ private function addRuleRequirementsToConfig($config, array $rules) {
                 continue;
             }
             $headers = $rule->getRequiredHeaders();
-            foreach( $headers as $header ) {
-                if( empty( $config['config'] ) ) {
-                    $config['config'] = array();
-                }
-                if( empty( $config['config']['headers'] ) ) {
-                    $config['config']['headers'] = array();
-                }
-                $config['config']['headers'][] = $header;
-            }
+            $config = $this->addRequiredHeadersToConfig( $config, $headers );
+        }
+
+        return $config;
+    }
+
+    /**
+     * @param array $config
+     *
+     * @return array
+     */
+    private function ensureConfigHeader($config) {
+        if( empty( $config['config'] ) ) {
+            $config['config'] = array();
+        }
+        if( empty( $config['config']['headers'] ) ) {
+            $config['config']['headers'] = array();
+        }
+
+        return $config;
+    }
+
+    /**
+     * @param array $config
+     * @param array $headers
+     *
+     * @return array
+     */
+    private function addRequiredHeadersToConfig($config, $headers) {
+        foreach( $headers as $header ) {
+            $config = $this->ensureConfigHeader( $config );
+            $config['config']['headers'][] = $header;
         }
 
         return $config;

diff --git a/src/Configuration/Exceptions/UnknownRuleException.php b/src/Configuration/Exceptions/UnknownRuleException.php
@@ -0,0 +1,6 @@
+<?php
+namespace Frickelbruder\KickOff\Configuration\Exceptions;
+
+class UnknownRuleException extends \Exception {
+
+}
diff --git a/src/Rules/HttpHeaderRespondsToIfModifiedSince.php b/src/Rules/HttpHeaderRespondsToIfModifiedSince.php
@@ -14,10 +14,10 @@ public function validate() {
             return false;
         }
         if($status != 304) {
-            $this->errorMessage = "The resource responded with an unexpected status code '" . $status . "' 'If-Modified-Since' was set.";
+            $this->errorMessage = "The resource responded with an unexpected status code '" . $status . "' to 'If-Modified-Since'.";
             return false;
         }
-        return $status == 304;
+        return true;
     }
 
 

diff --git a/src/Rules/OgPropertyPresent.php b/src/Rules/OgPropertyPresent.php
@@ -16,26 +16,35 @@ public function validate() {
             $body = $this->httpResponse->getBody();
             $xml = $this->getResponseBodyAsXml( $body );
 
-            foreach( $this->requiredProperties as $propertyName ) {
-                $propertyItemValue = $xml->xpath( '/html/head/meta[@property="og:' . $propertyName . '"]/ @content' );
-                if( !is_array( $propertyItemValue ) || empty( $propertyItemValue ) ) {
-                    $this->errorMessage = 'The open graph property "' . $propertyName . '" was not found on the site.';
+            return $this->checkRequiredProperties( $xml );
 
-                    return false;
-                }
-                $length = mb_strlen( $propertyItemValue[0]['content'], 'UTF-8' );
-
-                if( $length == 0 ) {
-                    $this->errorMessage = 'The open graph property "' . $propertyName . '" was found but is empty.';
-
-                    return false;
-                }
-            }
-
-            return true;
         } catch(\Exception $e) {
             $this->errorMessage = $e->getMessage();
         }
         return false;
     }
+
+    /**
+     * @param \SimpleXMLElement $body
+     *
+     * @return bool
+     */
+    private function checkRequiredProperties($body) {
+        foreach( $this->requiredProperties as $propertyName ) {
+            $propertyItemValue = $body->xpath( '/html/head/meta[@property="og:' . $propertyName . '"]/ @content' );
+            if( !is_array( $propertyItemValue ) || empty( $propertyItemValue ) ) {
+                $this->errorMessage = 'The open graph property "' . $propertyName . '" was not found on the site.';
+
+                return false;
+            }
+            $length = mb_strlen( $propertyItemValue[0]['content'], 'UTF-8' );
+
+            if( $length == 0 ) {
+                $this->errorMessage = 'The open graph property "' . $propertyName . '" was found but is empty.';
+
+                return false;
+            }
+        }
+        return true;
+    }
 }
diff --git a/src/Rules/RuleBase.php b/src/Rules/RuleBase.php
@@ -60,14 +60,18 @@ protected function findHeader($headerName, $normalize = true) {
         $headers = $this->httpResponse->getHeaders();
         foreach($headers as $key => $header) {
             if(strtolower($key) == $loweredHeaderName) {
-                if($normalize && is_array($header) ) {
-                    return implode("\n", $header);
-                }
-                return $header;
+                return $this->getNormalizedHeaderItem($header, $normalize);
             }
         }
         throw new HeaderNotFoundException('The HTTP header "' . $headerName. '" is missing.');
     }
 
+    private function getNormalizedHeaderItem($header, $normalize = true) {
+        if($normalize && is_array($header) ) {
+            return implode("\n", $header);
+        }
+        return $header;
+    }
+
 
 }
diff --git a/tests/Configuration/ConfigurationTest.php b/tests/Configuration/ConfigurationTest.php
@@ -64,11 +64,13 @@ public function testBaseSectionRules() {
         $secondSection = $sections['second'];
         $rules = $secondSection->getRules();
 
-        $this->assertCount(1, $rules, 'Not matching rules count');
+        $this->assertCount(2, $rules, 'Not matching rules count');
 
         $this->assertArrayHasKey('HttpHeaderExposeLanguage', $rules);
+        $this->assertArrayHasKey('HttpRequestTime', $rules);
 
         $this->assertInstanceOf('\Frickelbruder\KickOff\Rules\HttpHeaderNotPresent', $rules['HttpHeaderExposeLanguage']);
+        $this->assertInstanceOf('\Frickelbruder\KickOff\Rules\HttpRequestTime', $rules['HttpRequestTime']);
     }
 
     public function testConfigurationOfRule() {
@@ -78,7 +80,23 @@ public function testConfigurationOfRule() {
         $rules = $mainSection->getRules();
 
         $configuredRule = $rules['HttpRequestTime'];
-        $configuredRule->maxTransferTime = 22500;
+        $this->assertEquals(22500, $configuredRule->maxTransferTime);
+    }
+
+    public function testConfigurationOfRuleDoesNotAffectSameRuleInOtherSection() {
+        $sections = $this->configuration->getSections();
+
+        $mainSection = $sections['main'];
+        $rules = $mainSection->getRules();
+
+        $configuredRule = $rules['HttpRequestTime'];
+        $this->assertEquals(22500, $configuredRule->maxTransferTime);
+
+        $secondSection = $sections['second'];
+        $rules = $secondSection->getRules();
+
+        $configuredRule = $rules['HttpRequestTime'];
+        $this->assertEquals(1000, $configuredRule->maxTransferTime);
     }
 
     public function testConfigurationTargetUrlHasAddedHeaders() {
@@ -94,4 +112,5 @@ public function testConfigurationTargetUrlHasAddedHeaders() {
         $this->assertArrayHasKey('Accept-Encoding', $targetUrl->headers);
     }
 
+
 }
diff --git a/tests/Configuration/files/config.yml b/tests/Configuration/files/config.yml
@@ -18,7 +18,7 @@ Sections:
             port: 8080
         rules:
             - HttpHeaderExposeLanguage
-
+            - HttpRequestTime
 
 Rules:
     HttpHeaderXSSProtection: