Change table engine for MySQL databases, issue #41 #989

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
2 participants
@MunGell

MunGell commented Jan 27, 2012

Small functions to allow developers choose table engines. Work with
MySQL and MySQLi drivers (all other ignore it).

The functions are tested, but I will be glad if you perform some more tests on them.
Documentation was "vandalised" by me, hope this will help. :)

Change table engine for MySQL database, issue #41
Small functions to allow developers choose table engines. Works with
MySQL and MySQLi drivers (other ignore it).
@narfbg

This comment has been minimized.

Show comment Hide comment
@narfbg

narfbg Jan 27, 2012

Contributor

Why not just add logic for it in _create_table() and _alter_table()?

Contributor

narfbg commented Jan 27, 2012

Why not just add logic for it in _create_table() and _alter_table()?

@MunGell

This comment has been minimized.

Show comment Hide comment
@MunGell

MunGell Jan 27, 2012

It is in _create_table().

_alter_table() function does a bit different tasks (related to content).

There is _rename_table() function, which also uses ALTER TABLE statement and is allocated into separate function.
Thus, I decided to separate table engine definition also.

MunGell commented Jan 27, 2012

It is in _create_table().

_alter_table() function does a bit different tasks (related to content).

There is _rename_table() function, which also uses ALTER TABLE statement and is allocated into separate function.
Thus, I decided to separate table engine definition also.

@narfbg

View changes

system/database/drivers/mysqli/mysqli_forge.php
+ */
+ function _set_table_engine($table_name, $engine)
+ {
+ $sql = 'ALTER TABLE '.$this->db->_protect_identifiers($table_name).' ENGINE '.$engine;

This comment has been minimized.

Show comment Hide comment
@narfbg

narfbg Jan 27, 2012

Contributor

_protect_identifiers() is supposed to be a protected method, but currently it's public because all the drivers use it for DB forge. There's a protect_identifiers() method (no underscore) that I believe exist solely for this purpose - can we use that instead, please? :)

Also, on a side note - you can directly return that string. There's no point in assigning it and then returning a variable.

@narfbg

narfbg Jan 27, 2012

Contributor

_protect_identifiers() is supposed to be a protected method, but currently it's public because all the drivers use it for DB forge. There's a protect_identifiers() method (no underscore) that I believe exist solely for this purpose - can we use that instead, please? :)

Also, on a side note - you can directly return that string. There's no point in assigning it and then returning a variable.

This comment has been minimized.

Show comment Hide comment
@MunGell

MunGell Jan 27, 2012

I used the same coding style as in all other functions in this file. Of course, I can return string directly without additional variable. :) By the way, it might be resonable to do the same with _rename_table() function.

To be honest, protect_identifiers() as well as _protect_identifiers() defined as private ( *@access private ) in annotations and underscored version of the function widely used in the file. So, should I write my functions in different way?

@MunGell

MunGell Jan 27, 2012

I used the same coding style as in all other functions in this file. Of course, I can return string directly without additional variable. :) By the way, it might be resonable to do the same with _rename_table() function.

To be honest, protect_identifiers() as well as _protect_identifiers() defined as private ( *@access private ) in annotations and underscored version of the function widely used in the file. So, should I write my functions in different way?

This comment has been minimized.

Show comment Hide comment
@narfbg

narfbg Jan 27, 2012

Contributor

Well, they are described as private(and those descriptions are often not correct), but since there's no real definition, the default is public. Those descriptions are currently being removed from all over the place and each of my pending pull requests remove them and put actual definitions instead.

Usually, methods that are prefixed by an underscore are supposed to be private or protected, but protected should be used so that the classes can be easily extendable.

Btw, the mysql driver version that you have has been updated a few hours ago and you should merge the changes first. You'll see that it's a bit different in there. :)

@narfbg

narfbg Jan 27, 2012

Contributor

Well, they are described as private(and those descriptions are often not correct), but since there's no real definition, the default is public. Those descriptions are currently being removed from all over the place and each of my pending pull requests remove them and put actual definitions instead.

Usually, methods that are prefixed by an underscore are supposed to be private or protected, but protected should be used so that the classes can be easily extendable.

Btw, the mysql driver version that you have has been updated a few hours ago and you should merge the changes first. You'll see that it's a bit different in there. :)

@narfbg

This comment has been minimized.

Show comment Hide comment
@narfbg

narfbg Jan 27, 2012

Contributor

@MunGell

Yes, but almost all databases support renaming tables (and some of them have different syntax, not using ALTER TABLE), while this is a MySQL only feature.
We shouldn't pollute the base classes with methods and properties that are not relevant for at least a few of the drivers. And while you can't avoid adding another parameter to create_table(), you can simply use the $fields parameter in _alter_table() and add an if ($alter_type === 'ENGINE') inside it.

Contributor

narfbg commented Jan 27, 2012

@MunGell

Yes, but almost all databases support renaming tables (and some of them have different syntax, not using ALTER TABLE), while this is a MySQL only feature.
We shouldn't pollute the base classes with methods and properties that are not relevant for at least a few of the drivers. And while you can't avoid adding another parameter to create_table(), you can simply use the $fields parameter in _alter_table() and add an if ($alter_type === 'ENGINE') inside it.

@MunGell

This comment has been minimized.

Show comment Hide comment
@MunGell

MunGell Jan 27, 2012

@narfbg good point, thanks.

MunGell commented Jan 27, 2012

@narfbg good point, thanks.

MunGell added some commits Jan 27, 2012

MySQL driver was changed in accordance with suggestions
Waiting for new version of MySQLi driver to change it also.
MySQLi driver was changed in accordance with suggestions
Now it is ready for tests and possibly for merge.
@MunGell

This comment has been minimized.

Show comment Hide comment
@MunGell

MunGell Jan 27, 2012

@narfbg your code was merged, some changes done. Take a look ;)

MunGell commented Jan 27, 2012

@narfbg your code was merged, some changes done. Take a look ;)

@narfbg

This comment has been minimized.

Show comment Hide comment
@narfbg

narfbg Jan 27, 2012

Contributor

Um, while manually copying changes technically should work ... it's not the proper way to do that and makes it really hard to review. See this URL for some info on how to do it correctly: http://help.github.com/fork-a-repo/ :)

Contributor

narfbg commented Jan 27, 2012

Um, while manually copying changes technically should work ... it's not the proper way to do that and makes it really hard to review. See this URL for some info on how to do it correctly: http://help.github.com/fork-a-repo/ :)

@MunGell

This comment has been minimized.

Show comment Hide comment
@MunGell

MunGell Jan 27, 2012

All copyings have been done particularly in accordance with that manual. :)
I just manually solved a couple of conflicts. Sorry about that.

MunGell commented Jan 27, 2012

All copyings have been done particularly in accordance with that manual. :)
I just manually solved a couple of conflicts. Sorry about that.

@narfbg

This comment has been minimized.

Show comment Hide comment
@narfbg

narfbg Jan 2, 2014

Contributor

This is way too old and didn't have the proper approach anyway. #2776 is way better.

Contributor

narfbg commented Jan 2, 2014

This is way too old and didn't have the proper approach anyway. #2776 is way better.

@narfbg narfbg closed this Jan 2, 2014

narfbg added a commit that referenced this pull request Jan 20, 2014

@narfbg

This comment has been minimized.

Show comment Hide comment
@narfbg

narfbg Jan 20, 2014

Contributor

Anybody who wants this feature, please test the above commit from the feature/dbforge_table_attributes branch.

Contributor

narfbg commented Jan 20, 2014

Anybody who wants this feature, please test the above commit from the feature/dbforge_table_attributes branch.

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