From 5546e9235c1a3c75874b176269bbab284021305d Mon Sep 17 00:00:00 2001
From: Peter Gribanov <wcode404@gmail.com>
Date: Fri, 7 Feb 2020 13:25:57 +0300
Subject: [PATCH 1/4] move normalizers and validators to separate methods to
 improve code quality

---
 src/DependencyInjection/Configuration.php | 230 +++++++++++++++-------
 1 file changed, 154 insertions(+), 76 deletions(-)

diff --git a/src/DependencyInjection/Configuration.php b/src/DependencyInjection/Configuration.php
index 049bc6d..8afab38 100644
--- a/src/DependencyInjection/Configuration.php
+++ b/src/DependencyInjection/Configuration.php
@@ -12,6 +12,7 @@
 namespace GpsLab\Bundle\GeoIP2Bundle\DependencyInjection;
 
 use Symfony\Component\Config\Definition\Builder\ArrayNodeDefinition;
+use Symfony\Component\Config\Definition\Builder\NodeDefinition;
 use Symfony\Component\Config\Definition\Builder\TreeBuilder;
 use Symfony\Component\Config\Definition\ConfigurationInterface;
 
@@ -49,7 +50,84 @@ public function getConfigTreeBuilder(): TreeBuilder
             $root_node = $tree_builder->root('gpslab_geoip');
         }
 
-        // normalize default_database from databases
+        $this->normalizeDefaultDatabase($root_node);
+        $this->normalizeRootConfigurationToDefaultDatabase($root_node);
+        $this->validateAvailableDefaultDatabase($root_node);
+        $this->allowGlobalLicense($root_node);
+        $this->allowGlobalLocales($root_node);
+        $this->validateDatabaseLocales($root_node);
+
+        $root_node->fixXmlConfig('locale');
+        $locales = $root_node->children()->arrayNode('locales');
+        $locales->prototype('scalar');
+        $locales
+            ->treatNullLike([])
+            ->defaultValue(['en']);
+
+        $root_node->children()->scalarNode('license');
+
+        $default_database = $root_node->children()->scalarNode('default_database');
+        $default_database->defaultValue('default');
+
+        $root_node->fixXmlConfig('database');
+        $root_node->append($this->getDatabaseNode());
+
+        return $tree_builder;
+    }
+
+    /**
+     * @return ArrayNodeDefinition
+     */
+    private function getDatabaseNode(): ArrayNodeDefinition
+    {
+        $tree_builder = new TreeBuilder('databases');
+
+        if (method_exists($tree_builder, 'getRootNode')) {
+            // Symfony 4.2 +
+            $root_node = $tree_builder->getRootNode();
+        } else {
+            // Symfony 4.1 and below
+            $root_node = $tree_builder->root('databases');
+        }
+
+        $root_node->useAttributeAsKey('name');
+
+        /** @var ArrayNodeDefinition $database_node */
+        $database_node = $root_node->prototype('array');
+
+        $this->normalizeUrl($database_node);
+        $this->normalizePath($database_node);
+
+        $url = $database_node->children()->scalarNode('url');
+        $url->isRequired();
+
+        $this->validateURL($url);
+
+        $path = $database_node->children()->scalarNode('path');
+        $path->isRequired();
+
+        $database_node->fixXmlConfig('locale');
+        $locales = $database_node->children()->arrayNode('locales');
+        $locales->prototype('scalar');
+        $locales
+            ->treatNullLike([])
+            ->requiresAtLeastOneElement()
+            ->defaultValue(['en']);
+
+        $database_node->children()->scalarNode('license');
+
+        $database_node->children()->scalarNode('edition');
+
+        return $root_node;
+    }
+
+    /**
+     * Normalize default_database from databases.
+     *
+     * @param NodeDefinition $root_node
+     */
+    private function normalizeDefaultDatabase(NodeDefinition $root_node): void
+    {
         $root_node
             ->beforeNormalization()
             ->ifTrue(static function ($v): bool {
@@ -65,8 +143,15 @@ public function getConfigTreeBuilder(): TreeBuilder
 
                 return $v;
             });
+    }
 
-        // normalize databases root configuration to default_database
+    /**
+     * Normalize databases root configuration to default_database
+     *
+     * @param NodeDefinition $root_node
+     */
+    private function normalizeRootConfigurationToDefaultDatabase(NodeDefinition $root_node): void
+    {
         $root_node
             ->beforeNormalization()
             ->ifTrue(static function ($v): bool {
@@ -86,8 +171,15 @@ public function getConfigTreeBuilder(): TreeBuilder
 
                 return $v;
             });
+    }
 
-        // default_database should be exists in databases
+    /**
+     * Validate that the default_database exists in the list of databases.
+     *
+     * @param NodeDefinition $root_node
+     */
+    private function validateAvailableDefaultDatabase(NodeDefinition $root_node): void
+    {
         $root_node
             ->validate()
                 ->ifTrue(static function ($v): bool {
@@ -103,8 +195,16 @@ public function getConfigTreeBuilder(): TreeBuilder
 
                     throw new \InvalidArgumentException(sprintf('Undefined default database "%s". Available "%s" databases.', $v['default_database'], $databases));
                 });
+    }
 
-        // add license to databases config if not exists (allow use a global license for all databases)
+    /**
+     * Add a license option to the databases configuration if it does not exist.
+     * Allow use a global license for all databases.
+     *
+     * @param NodeDefinition $root_node
+     */
+    private function allowGlobalLicense(NodeDefinition $root_node): void
+    {
         $root_node
             ->beforeNormalization()
             ->ifTrue(static function ($v): bool {
@@ -123,8 +223,16 @@ public function getConfigTreeBuilder(): TreeBuilder
 
                 return $v;
             });
+    }
 
-        // add locales to databases config if not exists (allow use a global locales for all databases)
+    /**
+     * Add a locales option to the databases configuration if it does not exist.
+     * Allow use a global locales for all databases.
+     *
+     * @param NodeDefinition $root_node
+     */
+    private function allowGlobalLocales(NodeDefinition $root_node): void
+    {
         $root_node
             ->beforeNormalization()
             ->ifTrue(static function ($v): bool {
@@ -143,66 +251,41 @@ public function getConfigTreeBuilder(): TreeBuilder
 
                 return $v;
             });
+    }
 
-        // validate database locales
+    /**
+     * Validate database locales.
+     *
+     * @param NodeDefinition $root_node
+     */
+    private function validateDatabaseLocales(NodeDefinition $root_node): void
+    {
         $root_node
             ->validate()
-                ->ifTrue(static function ($v): bool {
-                    return
-                        is_array($v) &&
-                        array_key_exists('databases', $v) &&
-                        is_array($v['databases']);
-                })
-                ->then(static function (array $v): array {
-                    foreach ($v['databases'] as $name => $database) {
-                        if (!array_key_exists('locales', $database) || empty($database['locales'])) {
-                            throw new \InvalidArgumentException(sprintf('The list of locales should not be empty in databases "%s".', $name));
-                        }
+            ->ifTrue(static function ($v): bool {
+                return
+                    is_array($v) &&
+                    array_key_exists('databases', $v) &&
+                    is_array($v['databases']);
+            })
+            ->then(static function (array $v): array {
+                foreach ($v['databases'] as $name => $database) {
+                    if (!array_key_exists('locales', $database) || empty($database['locales'])) {
+                        throw new \InvalidArgumentException(sprintf('The list of locales should not be empty in databases "%s".', $name));
                     }
+                }
 
-                    return $v;
-                });
-
-        $root_node->fixXmlConfig('locale');
-        $locales = $root_node->children()->arrayNode('locales');
-        $locales->prototype('scalar');
-        $locales
-            ->treatNullLike([])
-            ->defaultValue(['en']);
-
-        $root_node->children()->scalarNode('license');
-
-        $default_database = $root_node->children()->scalarNode('default_database');
-        $default_database->defaultValue('default');
-
-        $root_node->fixXmlConfig('database');
-        $root_node->append($this->getDatabaseNode());
-
-        return $tree_builder;
+                return $v;
+            });
     }
 
     /**
-     * @return ArrayNodeDefinition
+     * Normalize url option from license key and edition id.
+     *
+     * @param NodeDefinition $database_node
      */
-    private function getDatabaseNode(): ArrayNodeDefinition
+    private function normalizeUrl(NodeDefinition $database_node): void
     {
-        $tree_builder = new TreeBuilder('databases');
-
-        if (method_exists($tree_builder, 'getRootNode')) {
-            // Symfony 4.2 +
-            $root_node = $tree_builder->getRootNode();
-        } else {
-            // Symfony 4.1 and below
-            $root_node = $tree_builder->root('databases');
-        }
-
-        /** @var ArrayNodeDefinition $database_node */
-        $database_node = $root_node
-            ->requiresAtLeastOneElement()
-            ->useAttributeAsKey('name')
-            ->prototype('array');
-
-        // normalize url from license and edition
         $database_node
             ->beforeNormalization()
             ->ifTrue(static function ($v): bool {
@@ -217,8 +300,15 @@ private function getDatabaseNode(): ArrayNodeDefinition
 
                 return $v;
             });
+    }
 
-        // normalize path from edition
+    /**
+     * Normalize path option from edition id.
+     *
+     * @param NodeDefinition $database_node
+     */
+    private function normalizePath(NodeDefinition $database_node): void
+    {
         $database_node
             ->beforeNormalization()
             ->ifTrue(static function ($v): bool {
@@ -229,10 +319,15 @@ private function getDatabaseNode(): ArrayNodeDefinition
 
                 return $v;
             });
+    }
 
-        $url = $database_node->children()->scalarNode('url');
-        $url->isRequired();
-        // url must be a valid URL
+    /**
+     * The url option must be a valid URL.
+     *
+     * @param NodeDefinition $url
+     */
+    private function validateURL(NodeDefinition $url): void
+    {
         $url
             ->validate()
             ->ifTrue(static function ($v): bool {
@@ -241,22 +336,5 @@ private function getDatabaseNode(): ArrayNodeDefinition
             ->then(static function (string $v): array {
                 throw new \InvalidArgumentException(sprintf('URL "%s" must be valid.', $v));
             });
-
-        $path = $database_node->children()->scalarNode('path');
-        $path->isRequired();
-
-        $database_node->fixXmlConfig('locale');
-        $locales = $database_node->children()->arrayNode('locales');
-        $locales->prototype('scalar');
-        $locales
-            ->treatNullLike([])
-            ->requiresAtLeastOneElement()
-            ->defaultValue(['en']);
-
-        $database_node->children()->scalarNode('license');
-
-        $database_node->children()->scalarNode('edition');
-
-        return $root_node;
     }
 }

From 4e5a19a4e1a329b888bbceb7d092616e0af47d7d Mon Sep 17 00:00:00 2001
From: Peter Gribanov <wcode404@gmail.com>
Date: Fri, 7 Feb 2020 13:28:17 +0300
Subject: [PATCH 2/4] fix CS error

---
 src/DependencyInjection/Configuration.php | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/DependencyInjection/Configuration.php b/src/DependencyInjection/Configuration.php
index 8afab38..4e5c68e 100644
--- a/src/DependencyInjection/Configuration.php
+++ b/src/DependencyInjection/Configuration.php
@@ -146,7 +146,7 @@ private function normalizeDefaultDatabase(NodeDefinition $root_node): void
     }
 
     /**
-     * Normalize databases root configuration to default_database
+     * Normalize databases root configuration to default_database.
      *
      * @param NodeDefinition $root_node
      */

From e795442a0b25ddf037ad4bfb9dbae454b7bb9183 Mon Sep 17 00:00:00 2001
From: Peter Gribanov <wcode404@gmail.com>
Date: Fri, 7 Feb 2020 14:16:27 +0300
Subject: [PATCH 3/4] optimize condition in
 Configuration::validateDatabaseLocales()

---
 src/DependencyInjection/Configuration.php | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/src/DependencyInjection/Configuration.php b/src/DependencyInjection/Configuration.php
index 4e5c68e..121d7fa 100644
--- a/src/DependencyInjection/Configuration.php
+++ b/src/DependencyInjection/Configuration.php
@@ -263,14 +263,11 @@ private function validateDatabaseLocales(NodeDefinition $root_node): void
         $root_node
             ->validate()
             ->ifTrue(static function ($v): bool {
-                return
-                    is_array($v) &&
-                    array_key_exists('databases', $v) &&
-                    is_array($v['databases']);
+                return is_array($v) && array_key_exists('databases', $v) && is_array($v['databases']);
             })
             ->then(static function (array $v): array {
                 foreach ($v['databases'] as $name => $database) {
-                    if (!array_key_exists('locales', $database) || empty($database['locales'])) {
+                    if (empty($database['locales'])) {
                         throw new \InvalidArgumentException(sprintf('The list of locales should not be empty in databases "%s".', $name));
                     }
                 }

From 2cd2ae7a62e36fcaa9a453358a65c1ed21e30634 Mon Sep 17 00:00:00 2001
From: Peter Gribanov <wcode404@gmail.com>
Date: Fri, 7 Feb 2020 14:27:24 +0300
Subject: [PATCH 4/4] fix unit tests

---
 src/DependencyInjection/Configuration.php     |  3 +-
 .../DependencyInjection/ConfigurationTest.php | 28 ++++++++++++-------
 2 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/src/DependencyInjection/Configuration.php b/src/DependencyInjection/Configuration.php
index 121d7fa..b90b7cb 100644
--- a/src/DependencyInjection/Configuration.php
+++ b/src/DependencyInjection/Configuration.php
@@ -135,7 +135,8 @@ private function normalizeDefaultDatabase(NodeDefinition $root_node): void
                     is_array($v) &&
                     !array_key_exists('default_database', $v) &&
                     array_key_exists('databases', $v) &&
-                    is_array($v['databases']);
+                    is_array($v['databases']) &&
+                    $v['databases'];
             })
             ->then(static function (array $v): array {
                 $keys = array_keys($v['databases']);
diff --git a/tests/DependencyInjection/ConfigurationTest.php b/tests/DependencyInjection/ConfigurationTest.php
index f8e84e3..763f551 100644
--- a/tests/DependencyInjection/ConfigurationTest.php
+++ b/tests/DependencyInjection/ConfigurationTest.php
@@ -29,16 +29,6 @@ public function getBadConfigs(): array
     {
         $return = [];
         foreach (['/tmp/var/cache', null] as $cache_dir) {
-            $return[] = [$cache_dir, [
-                'gpslab_geoip' => [
-                    'databases' => null,
-                ],
-            ]];
-            $return[] = [$cache_dir, [
-                'gpslab_geoip' => [
-                    'databases' => [],
-                ],
-            ]];
             $return[] = [$cache_dir, [
                 'gpslab_geoip' => [
                     'license' => 'LICENSE',
@@ -151,6 +141,24 @@ public function getConfigs(): array
                 'default_database' => 'default',
                 'databases' => [],
             ]];
+            $return[] = [$cache_dir, [
+                'gpslab_geoip' => [
+                    'databases' => null,
+                ],
+            ], [
+                'databases' => [],
+                'locales' => ['en'],
+                'default_database' => 'default',
+            ]];
+            $return[] = [$cache_dir, [
+                'gpslab_geoip' => [
+                    'databases' => [],
+                ],
+            ], [
+                'databases' => [],
+                'locales' => ['en'],
+                'default_database' => 'default',
+            ]];
             $return[] = [$cache_dir, [
                 'gpslab_geoip' => [
                     'license' => 'LICENSE',