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

Table names with db prefix not included in migration snapshots #193

Closed
wants to merge 1 commit into from

Conversation

makallio85
Copy link

Table name can be prefixed with database name. If prefix is present, ensure it matches with current connection database name.

…, ensure it matches with current connection database name.
@makallio85 makallio85 changed the title Table names with connection prefix not included in migration snapshots Table names with db prefix not included in migration snapshots Feb 5, 2016
@@ -246,7 +246,15 @@ public function fetchTableName($className, $pluginName = null)
$tables[] = $table->associations()->get($key)->_junctionTableName();
}
}
$tables[] = $table->table();
$t = $table->table();
$splitted = array_reverse(explode('.', $t));
Copy link
Member

Choose a reason for hiding this comment

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

Are you assuming that the plugin name is the prefix?

Copy link
Member

Choose a reason for hiding this comment

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

I think @makallio85 wants to cover the case where you define your table name as schema.table.

Copy link
Author

Choose a reason for hiding this comment

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

@HavokInspiration You are correct.

@HavokInspiration HavokInspiration added this to the 1.5.6 milestone Feb 6, 2016
@HavokInspiration
Copy link
Member

I'm ok with the improvement but we need tests to make sure it works as expected for all db vendors (and avoid future regressions).
I can take care of that part if you want.

Any thoughts on this @josegonzalez ?

@makallio85
Copy link
Author

@HavokInspiration Yes please ;)

@HavokInspiration
Copy link
Member

Closing in favor of #195

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

Successfully merging this pull request may close these issues.

None yet

3 participants