Skip to content
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

Use schema config for migrations table #555

Closed

Conversation

aik099
Copy link

@aik099 aik099 commented Oct 2, 2017

When creating migration_versions table use same table options (e.g. charset, collate and engine), that are used during migration diff generation.

Solves the problem, where doctrine.dbal.default_table_options setting wasn't respected by https://github.com/doctrine/DoctrineMigrationsBundle.

$table = new Table($this->migrationsTableName, $columns);
$schema = new Schema([], [], $schemaManager->createSchemaConfig());
$table = $schema->createTable($this->migrationsTableName);
$table->addColumn($this->migrationsColumnName, 'string', ['length' => 255]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you keep the getType here ?

Copy link
Author

@aik099 aik099 Oct 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I change string into Type::getType('string') I'm getting an exception back, because \Doctrine\DBAL\Schema\Table::addColumn method expects type as string and internally it's doing the same thing: https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Schema/Table.php#L321

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@mikeSimonson
Copy link
Contributor

@aik099 Thanks for the PR

];
$table = new Table($this->migrationsTableName, $columns);
$schema = new Schema([], [], $schemaManager->createSchemaConfig());
$table = $schema->createTable($this->migrationsTableName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The build is failling because there is a space missing before the equal to align it with the one from the line above.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@aik099 aik099 force-pushed the use-schema-config-for-migrations-table branch from 0042d68 to 88b78d1 Compare October 2, 2017 13:44
Copy link
Member

@lcobucci lcobucci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aik099 can you please add a functional test for this fix? Otherwise it's kind of hard ensure that we won't break this again in the future.

@lcobucci
Copy link
Member

lcobucci commented Oct 2, 2017

@aik099 Patch LGTM btw 👍

@aik099
Copy link
Author

aik099 commented Oct 2, 2017

@lcobucci , any example of such test?

@aik099
Copy link
Author

aik099 commented Oct 2, 2017

Almost all commands use (and create if missing) migrations table and I've changed code, used to create it.

@lcobucci
Copy link
Member

lcobucci commented Oct 2, 2017

Almost all commands use (and create if missing) migrations table and I've changed code, used to create it.

Sure, but no automated test cover that specific scenario. So it means that you're not breaking existing code but you're not adding tests that ensure that in the future no one will break what you've done. Got it?

jwage
jwage previously approved these changes Oct 2, 2017
@jwage jwage dismissed their stale review October 2, 2017 20:26

Accident

@aik099
Copy link
Author

aik099 commented Oct 3, 2017

Sure, but no automated test cover that specific scenario.

Yes, none of the tests likely check actual structure of the table being created after the migrate:status command was executed. To test it I'm afraid, that real MySQL database is needed because charset & collation stuff might not manifest itself in say SQLLite database.

@mikeSimonson
Copy link
Contributor

I thinking to a test where you would pass a schema where you changed the charset and you verify that after execution it's that charset that was used for the new code. I will try that tomorrow.

@aik099
Copy link
Author

aik099 commented Oct 5, 2017

@mikeSimonson , did it work, I mean idea about new test?

@mikeSimonson
Copy link
Contributor

@aik099 I taught that you where going to take a look at it. If you can't, just tell me and I will try.

@aik099
Copy link
Author

aik099 commented Oct 31, 2017

Yeah, I can't, because I haven't found any tests that are using MySQL or similar database engine where we can actually dump database structure and verify that it was using correct collation, charset and type (InnoDB or MyISAM).

@jwage
Copy link
Member

jwage commented May 16, 2018

Since master has changed so much since this PR was originally created, I continued the work for this in #679

@jwage jwage closed this May 16, 2018
@jwage jwage added this to the 2.0 milestone May 16, 2018
@jwage jwage removed the Needs Tests label May 16, 2018
@jwage jwage self-assigned this May 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants