From d52f43b86381e7dd3101343fdc0fa97aa3cbf7f1 Mon Sep 17 00:00:00 2001 From: Chris Schwerdt Date: Tue, 22 Nov 2016 16:16:54 -0700 Subject: [PATCH] Fix issue with nested configuration keys The ConfigurationFactory used the Collection class for retrieving configuration keys, but the Collection class does not support nested key names. This caused the retrieval of the schema.filter key to always return the default value, which would never filter any tables. - Switched to using the Repository class because it properly supports nested keys. - Fixed the mock value for the schema configuration in the ConfigurationFactoryTests. It's a nested array, not a flattened array. - Fixed setFilterSchemaAssetsExpression() expectations in unit tests as Mockery::with() detects regular expressions and uses them when performing argument matching. This was causing false negatives when asserting method call arguments matched expected values. Added Mockery::mustBe() to use strict equality checks. --- composer.json | 2 +- src/Configuration/ConfigurationFactory.php | 14 +++++++------- tests/Configuration/ConfigurationFactoryTest.php | 15 +++++++++------ 3 files changed, 17 insertions(+), 14 deletions(-) diff --git a/composer.json b/composer.json index f905e8a..8b1c30c 100644 --- a/composer.json +++ b/composer.json @@ -19,9 +19,9 @@ "require": { "php": ">=5.5.9", "doctrine/migrations": "~1.1", + "illuminate/config": "~5.1", "illuminate/contracts": "~5.1", "illuminate/console": "~5.1", - "illuminate/support": "~5.1", "laravel-doctrine/orm": "1.0.*|1.1.*|1.2.*" }, "require-dev": { diff --git a/src/Configuration/ConfigurationFactory.php b/src/Configuration/ConfigurationFactory.php index 1df58e8..a52aa21 100644 --- a/src/Configuration/ConfigurationFactory.php +++ b/src/Configuration/ConfigurationFactory.php @@ -3,15 +3,15 @@ namespace LaravelDoctrine\Migrations\Configuration; use Doctrine\DBAL\Connection; -use Illuminate\Contracts\Config\Repository; +use Illuminate\Config\Repository; +use Illuminate\Contracts\Config\Repository as ConfigRepository; use Illuminate\Contracts\Container\Container; -use Illuminate\Support\Collection; use LaravelDoctrine\Migrations\Naming\DefaultNamingStrategy; class ConfigurationFactory { /** - * @var Repository + * @var ConfigRepository */ protected $config; @@ -21,10 +21,10 @@ class ConfigurationFactory protected $container; /** - * @param Repository $config + * @param ConfigRepository $config * @param Container $container */ - public function __construct(Repository $config, Container $container) + public function __construct(ConfigRepository $config, Container $container) { $this->config = $config; $this->container = $container; @@ -39,9 +39,9 @@ public function __construct(Repository $config, Container $container) public function make(Connection $connection, $name = null) { if ($name && $this->config->has('migrations.' . $name)) { - $config = new Collection($this->config->get('migrations.' . $name, [])); + $config = new Repository($this->config->get('migrations.' . $name, [])); } else { - $config = new Collection($this->config->get('migrations.default', [])); + $config = new Repository($this->config->get('migrations.default', [])); } $configuration = new Configuration($connection); diff --git a/tests/Configuration/ConfigurationFactoryTest.php b/tests/Configuration/ConfigurationFactoryTest.php index a18f080..bb34b92 100644 --- a/tests/Configuration/ConfigurationFactoryTest.php +++ b/tests/Configuration/ConfigurationFactoryTest.php @@ -59,13 +59,13 @@ public function test_can_make_configuration() 'name' => 'Doctrine Migrations', 'namespace' => 'Database\\Migrations', 'table' => 'migrations', - 'schema.filter' => '/^(?).*$/', + 'schema' => ['filter' => '/^(?).*$/'], 'directory' => database_path('migrations'), 'naming_strategy' => DefaultNamingStrategy::class, ]) ; - $this->configuration->shouldReceive('setFilterSchemaAssetsExpression')->with('/^(?).*$/')->once(); + $this->configuration->shouldReceive('setFilterSchemaAssetsExpression')->with(m::mustBe('/^(?).*$/'))->once(); $this->container->shouldReceive('make') ->with(DefaultNamingStrategy::class) @@ -99,13 +99,16 @@ public function test_can_make_configuration_for_custom_entity_manager() 'name' => 'Migrations', 'namespace' => 'Database\\Migrations\\Custom', 'table' => 'migrations', - 'schema.filter' => '/^(?).*$/', + 'schema' => ['filter' => '/^(?!^(custom)$).*$/'], 'directory' => database_path('migrations/custom'), 'naming_strategy' => DefaultNamingStrategy::class, ]) ; - $this->configuration->shouldReceive('setFilterSchemaAssetsExpression')->with('/^(?).*$/')->once(); + $this->configuration->shouldReceive('setFilterSchemaAssetsExpression') + ->with(m::mustBe('/^(?!^(custom)$).*$/')) + ->once() + ; $this->container->shouldReceive('make') ->with(DefaultNamingStrategy::class) ->once() @@ -138,13 +141,13 @@ public function test_returns_default_configuration_if_not_defined() 'name' => 'Doctrine Migrations', 'namespace' => 'Database\\Migrations', 'table' => 'migrations', - 'schema.filter' => '/^(?).*$/', + 'schema' => ['filter' => '/^(?).*$/'], 'directory' => database_path('migrations'), 'naming_strategy' => DefaultNamingStrategy::class, ]) ; - $this->configuration->shouldReceive('setFilterSchemaAssetsExpression')->with('/^(?).*$/')->once(); + $this->configuration->shouldReceive('setFilterSchemaAssetsExpression')->with(m::mustBe('/^(?).*$/'))->once(); $this->container->shouldReceive('make') ->with(DefaultNamingStrategy::class) ->once()