-
Notifications
You must be signed in to change notification settings - Fork 113
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migration generation via cli #34
Conversation
protected function getMigrationName($name = null) | ||
{ | ||
if (empty($name)) { | ||
return $this->error('Choose a migration name to bake in underscore format'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Phenix uses a camel case format for migrations names in the command line, we should follow that decision
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name of the class is UpperCamelCase though.
I really like the direction this is going |
@@ -121,7 +121,7 @@ public function bake($filename) | |||
|
|||
$this->Template->set($data); | |||
|
|||
$out = $this->Template->generate('Migrations.config/migration'); | |||
$out = $this->Template->generate('Migrations.config/initial'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Migrations.config/snapshot
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I thought I fixed this...
I too like the direction. Nice work @josegonzalez 👍 |
cd41be4
to
5239d34
Compare
* @return void | ||
*/ | ||
public function main($name = null) | ||
public function main() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove that? I thought the passed param here in 3.x is a little bit nicer than running through $this->args via array_shift, effectively modifying $this->args in the process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you specify arguments in the ConsoleOptionParser, you cannot use arbitrary args (which are necessary for rails-style migration generation).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, but then $this->args[0] would still be better IMO to not modify the args.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to update the PR 💃
567f07b
to
a1cefbd
Compare
$validTypes = array_values($collection->filter(function ($_, $constant) { | ||
$_; | ||
return substr($constant, 0, strlen('PHINX_TYPE_')) === 'PHINX_TYPE_'; | ||
})->toArray()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
toArray(false) Will do save you the call to array_values()
a1cefbd
to
808e034
Compare
962f9aa
to
76f260a
Compare
I'm having a bit of troubling getting fixtures to run. I'm going to push my current code in the hopes that someone can figure out why fixtures aren't being inserted into the db... |
@josegonzalez I'll try it some time today or tomorrow and share what I find. |
3cffdd5
to
7afdf08
Compare
I ended up splitting out the snapshot generation and generation from the cli into two different tasks. I might still consolidate them to keep the interface the same, but I like how it came out :) |
105f26d
to
1b1e42b
Compare
Also extend the SimpleBakeTask, reducing the amount of code needed to generate a given migration file.
The current version of phinx automatically includes a primaryKey field, which would break snapshot application
Due to how the CakePHP PostgresSchema class treats length on integer-like fields, we cannot depend upon the output to compare correctly when using postgres as a datasource. Refs cakephp/cakephp#5901
1b1e42b
to
b334a85
Compare
- php: hhvm-nightly | ||
- env: DB=pgsql db_dsn='postgres://postgres@127.0.0.1/cakephp_test' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why what?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, thats related to a bug in the schema generation in CakePHP. See this commit for more details.
This looks great! 👍 |
'limit' => 11, | ||
], | ||
], | ||
], $this->columnParser->parseFields(['id:primary'])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some troll is going to want :
in their column names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That troll can write his own migrations sans the cli-based generator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
Looks great 👍 |
This is my first pass at making migration baking a little easier to use.
--snapshot
optionmigration.ctp
tosnapshot.ctp
MigrationHelper
class to handle data output for migration filesThis PR handles the signed/unsigned bug, and adds support for more phinx-supported attributes.
TODO:
MigrationHelper
class