Skip to content

indexes for mysql schema dump#1

Merged
basilfx merged 3 commits into
basilfx:0.7.x-devfrom
jwh315:0.7.x-dev
Nov 12, 2016
Merged

indexes for mysql schema dump#1
basilfx merged 3 commits into
basilfx:0.7.x-devfrom
jwh315:0.7.x-dev

Conversation

@jwh315

@jwh315 jwh315 commented Oct 30, 2016

Copy link
Copy Markdown

I ran across your schema dump/load feature for phinx a few weeks ago. I really love your implementation but really needed indexes. I was able to get the indexes put into the mysql adapter pretty easily.

I have a pretty decent size dev database (over 700 tables) at my company, that has an array of ridiculous design inconsistencies from years of development. The indexing seems to be recreating tables 100% from their original state, so I am fairly confident that I have covered most edge cases related to indexing and phinx.

Not sure if you would be interested in merging this branch into yours, or if you would need more from my end before doing so.

Either way, thanks for the effort putting this schema dumper together, it has saved me several hours worth of work.

@basilfx

basilfx commented Nov 2, 2016

Copy link
Copy Markdown
Owner

Sounds like a great addition! Let me take a look at it.

const PHINX_TYPE_CHAR = 'char';
const PHINX_TYPE_TEXT = 'text';
const PHINX_TYPE_INTEGER = 'integer';
const PHINX_TYPE_DOUBLE = 'double';

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think this would be better in a separate PR.

Comment thread src/Phinx/Db/Adapter/MysqlAdapter.php Outdated
case static::PHINX_TYPE_FLOAT:
return array('name' => 'float');
break;
case static::PHINX_TYPE_DOUBLE:

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think this would be better in a separate PR.

Comment thread src/Phinx/Db/Adapter/MysqlAdapter.php Outdated
* @return array
*/
protected function getIndexes($tableName)
public function getIndexes($tableName, $lowercase = true)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

What's the added benefit of lowercase?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

this method was originally lowercasing all index column names (as it is currently). I initially thought I needed to have case sensitive column names for mysql on some operating systems. That turned out to be incorrect thinking on my part. I have removed the extra param

@basilfx

basilfx commented Nov 10, 2016

Copy link
Copy Markdown
Owner

Can you also rebase (interactively) to fixup 2d8ebb3 in 8904d29?

Jacob Holmes added 2 commits November 10, 2016 21:34
-fix some primary key issues which were cropping up on tables where a column name was id but was not the pk
-remove lowercase param from index function, indexes are not case sensitive in mysql
-account for tables that do not have primary keys when dumping a schema
@jwh315

jwh315 commented Nov 11, 2016

Copy link
Copy Markdown
Author

I removed the DOUBLE column type and fixed the commit history to remove the superfluous commit. I also noticed another issue that needed to be addressed for tables that do not have primary keys, that has been fixed in the most recent commit.

@basilfx basilfx merged commit c4d182d into basilfx:0.7.x-dev Nov 12, 2016
@basilfx

basilfx commented Nov 12, 2016

Copy link
Copy Markdown
Owner

Looks good to me, thanks!

@basilfx

basilfx commented Nov 12, 2016

Copy link
Copy Markdown
Owner

I made a few improvements to the code:

  • There was still a reference to double in the PDOAdapter
  • Undefined index check
  • Optimized template

Other than that, it works great :-)

<?php endforeach; ?>

<?php foreach($table->getIndexes() as $name => $index): ?>
<? if ($name !== 'PRIMARY'): ?>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

<?php if ($name !== 'PRIMARY'): ?>

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Fixed, thanks!

@basilfx

basilfx commented Nov 18, 2016

Copy link
Copy Markdown
Owner

@jwh315 I have fixed some more:

  • 84cc9b7 reverts another change that should not be part of this fork.
  • 926918f column names should not be converted to lower case, just interpret them as returned by MySQL's SHOW INDEXES.
  • 322eedf fixes a problem with the code generation: in buildIndexString the $name parameter caused issues. In SQLite, the name of an index cannot match a column name. I remove this parameter, but added it back as an option of the Index class.

basilfx pushed a commit that referenced this pull request Feb 15, 2017
* Add index capability to the schema:dump feature
* Set index limits where applicable. Fix some primary key issues which were cropping up on tables where a column name was id but was not the pk.
* Account for tables that do not have primary keys when dumping a schema
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants