beberlei / zf-doctrine

A Zend Framework 1.x and Doctrine 1.2 Integration - UNMAINTAINED
101 stars 21 forks source link

Fix for zf generate-migration command #21

Open wjgilmore opened 14 years ago

wjgilmore commented 14 years ago

Hi Ben,

I've been having great success using zf-doctrine, however it definitely seems as if the migration feature is broken. A glance at DoctrineProvider.php indicates that this was due to a misnamed variable ($migrationsPath instead of $migratePath) and a misplaced call to $this->_initDoctrineResource(). I've fixed these issues within this doctrineproviderfix branch, and a migration is now being created (and the errors have disappeared).

Unfortunately it is unclear what exactly this migration file (which is successfully generated within the migrations directory is supposed to look like. A look at a generated migration file (generated with my patch in place) shows empty up() and empty() down methods which clearly seems incorrect. Nonetheless, the two changes found in this patch strike me as a step in the right direction towards fixing this problem.

Could you confirm and let me know? Happy to keep helping you out with this project if you'd like me to.

Jason

wjgilmore commented 14 years ago

Hi Ben, After further review I believe the migration class is indeed being created as you intended with the attached patch. I think my earlier confusion regarding what the migration file is supposed to look like stems from the confusing CLI command signature:

zf generate-migration doctrine class-name d-from-database m-from-models

Apparently "d-from-database" and "m-from-models" are actually just perhaps naming suggestions for helping developers to visually determine the purpose of a particular migration just by looking at the migration filename? Is that correct? If so while I understand your goal, I think it would be far less confusing if you changed the signature to simply this, leaving the matter of naming preference to the developer (perhaps mention the naming recommendation in the documentation):

zf generate-migration doctrine migration-name

kaptenpeter commented 13 years ago

You should probably run it like this zf generate-migration doctrine --d-from-database

To create a migration class from database. It would not be empty. I will run a test to verify.

kaptenpeter commented 13 years ago

Yes it works fine, the class (with the database calls) is created nicely.

ghost commented 13 years ago

I noticed some weird behavior with the zf generate-migration command: I wanted to create a migration set so I used --m-from-database to get a diff to my existing schema.yml. However the up/down methods wanted to drop/create the whole tables instead of addColumen/removeColumn. I traced thru the generateMigration code, which calls Doctrine_Core::generateMigrationsFromDiff(..) , my yml against models created from my db, however Doctrine/Migration/Diff.php::_generateModels seems to fail to create the temp files and therefore the migration diffs against an empty 'from' schema. I didnt had the time to fix the zf generateMigration function so I added my own public function generateMigrationDiff() { $this->_initDoctrineResource(); $migrationsPath = $this->_getMigrationsDirectoryPath(); $yamlSchemaPath = $this->_getYamlDirectoryPath(); Doctrine_Core::generateMigrationsFromDiff($migrationsPath, $yamlSchemaPath.'/schema_old.yml', $yamlSchemaPath.'/schema.yml'); } which perfectly does what it should do, but I have to create a second temporary schema_old.yml to generate the diff set

kaptenpeter commented 13 years ago

I think this is unavoidable, because the way that we name the classes vs tables. Going straight from the database you have no way of mapping tables to classnames.

Example:

schema.yml Name_Model_Test: table: test

Here we can map to table name.

Database table: test

No way to map back to the classname.

So if I understand this correctly the only way of producing a correct diff is to do as you did, that is to keep two versions of the yaml file.

I'm trying the same approach actually because writing the migration classes manually seems like too much work.

If anyone has any ideas as to how you can map from database tables to classname I'd like to hear it.

Edit: Would it be able to map back to classname if the table is named accordingly? Like table: name_model_test It's not the best looking solution but could work in some cases.

philicious commented 13 years ago

no you got me wrong. _generateModels usually works. My tables are named like the classes (name_model_test). Therefore I CAN generate models from the database. But when doing generate-migration --d-from-database _generateModels fails to create the temporary model files! I think its a problem with the paths to them being messed up. But didnt have the time to trace on so I added ad quickfix that works on 2 yaml files..

kaptenpeter commented 13 years ago

Oh. Perhaps I was a bit too fast to apply my own findings to your problems.

My temporarye files are created alright, so it should be possible to get it to work with your setup.

Anyway I don't have time to work on it right now, but maybe later down the road, if noone else steps up to do it.

philicious commented 13 years ago

so for you, when you do a --d-from-database it produces a correct diff with addColumn etc of the changes and not a whole dropTable/addTable ?

(btw I am encountering that bug on a Mac)

kaptenpeter commented 13 years ago

I will have to check.. But it will be a few days until I get round to i.