added possibility to configure commands by passing an array #87

Closed
wants to merge 7 commits into
from

Conversation

Projects
None yet
6 participants

zolex commented Sep 5, 2012

It's now possible to configure a migration command by passing a configuration array.

zolex commented Sep 5, 2012

also see related pull request: doctrine/DoctrineORMModule#118

Ocramius referenced this pull request in doctrine/DoctrineORMModule Sep 5, 2012

Merged

Added possibility to configure command using the module config files #118

@Ocramius Ocramius and 1 other commented on an outdated diff Sep 5, 2012

.../Migrations/Tools/Console/Command/AbstractCommand.php
@@ -45,6 +47,17 @@
*/
private $configuration;
+ /**
+ * @var array
+ */
+ private $arrayConfig;
+
+ public function __construct($name = null, $config = null) {
@Ocramius

Ocramius Sep 5, 2012

Owner

I'd avoid passing the config as a constructor parameter. After all it is not a hard dependency.

@zolex

zolex Sep 5, 2012

this is why the parameter has a default dependency. but i can change it to use a setter

@Ocramius

Ocramius Sep 5, 2012

Owner

Yes, indeed, but it is kinda entropic IMO :)

@Ocramius Ocramius and 1 other commented on an outdated diff Sep 5, 2012

.../Migrations/Tools/Console/Command/AbstractCommand.php
@@ -101,7 +114,10 @@ protected function getMigrationConfiguration(InputInterface $input, OutputInterf
throw new \InvalidArgumentException('You have to specify a --db-configuration file or pass a Database Connection as a dependency to the Migrations.');
}
- if ($input->getOption('configuration')) {
+ if ($this->arrayConfig) {
@Ocramius

Ocramius Sep 5, 2012

Owner

Also here, no need to do any transformation. I'd just expect an ArrayConfiguration to be passed in instead of doing this inside the command itself

@zolex

zolex Sep 5, 2012

also xml and yaml configuration are created inside the command, because they have dependencies on the conenction and the outputwriter.

@Ocramius

Ocramius Sep 5, 2012

Owner

I see... Too bad they cannot be removed now :\

@stof stof and 1 other commented on an outdated diff Sep 5, 2012

.../DBAL/Migrations/Configuration/ArrayConfiguration.php
+
+ if (isset($options['table_name'])) {
+
+ $this->setMigrationsTableName($options['table_name']);
+ }
+
+ if (isset($options['directory'])) {
+
+ $this->setMigrationsDirectory($options['directory']);
+ }
+
+ if (isset($options['directories'])) {
+
+ if (!is_array($options['directories'])) {
+
+ throw new \Exception('directories config must be an array');
@stof

stof Sep 5, 2012

Member

you should not use the base exception class but the exception of the package

@zolex

zolex Sep 5, 2012

yes, just was for a quick test and forgot to change it

zolex commented Sep 9, 2012

what about merging it?

@Ocramius Ocramius commented on the diff Sep 9, 2012

lib/Doctrine/DBAL/Migrations/MigrationException.php
@@ -63,4 +63,14 @@ public static function configurationFileAlreadyLoaded()
@Ocramius

Ocramius Sep 9, 2012

Owner

Can you restore the file mode?

Owner

Ocramius commented Sep 9, 2012

For me this is 👍

Ocramius referenced this pull request in doctrine/DoctrineORMModule Dec 6, 2012

Closed

Manually merge #118 since it was merged too early and reverted #134

zolex commented Jan 14, 2013

any progress in merging here?

Owner

jwage commented Jan 14, 2013

This PR needs to be cleaned before it can be merged.

Member

mikeSimonson commented Feb 14, 2015

@zolex Can you rebase your PR and add a test ?

Thanks

mikeSimonson added the 3.0 label Feb 14, 2015

@mikeSimonson mikeSimonson modified the milestone: 1.1 Feb 15, 2015

kdubuc commented Jun 30, 2015

any news on this ?

@mikeSimonson mikeSimonson commented on the diff Jun 30, 2015

lib/Doctrine/DBAL/Migrations/MigrationException.php
@@ -63,4 +63,14 @@ public static function configurationFileAlreadyLoaded()
{
return new self(sprintf('Migrations configuration file already loaded'), 8);
}
+
+ public static function directoriesConfigInvalid()
+ {
+ return new self('Migraion directories config must be an array', 9);
@mikeSimonson

mikeSimonson Jun 30, 2015

Member

Migration instead of Migraion

Member

mikeSimonson commented Jun 30, 2015

To be merged the part in the abstractCommand needs to be merged into the ConfigurationHelper and it needs tests.

mikeSimonson reopened this Jun 30, 2015

Member

mikeSimonson commented Dec 15, 2015

Another PR was merged with the feature cleaned up from this PR.

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment