Added ability to specify a table engine for MySQL tables and potentially - other attributes. #2776

Closed
wants to merge 1 commit into
from

Projects

None yet

2 participants

@Minras
Minras commented Dec 17, 2013

Possibly other attributes can be specified using the _build_create_table_attributes() method.

@narfbg
Collaborator
narfbg commented Dec 18, 2013

Looks pretty good by design, especially as opposed to other attempts to implement this. Does it have to be named _build_create_table_attributes() though? Sure, it has to be descriptive and this is only internal, so we don't really have to care about saving time for end users, but it still feels unnecessarily long and not consistent with the rest of the forge methods. How about i.e. _table_attributes()?

There's some other stuff that needs to be polished, but that's mostly routine:

  • Changelog entry and documentation update.
  • Spacing. We use tabs only, you seem to be mixing both and even the diff here on github looks oddly aligned.
  • You're patching mysqli_driver.php, instead of mysqli_forge.php.
  • pdo/mysql also needs to have this and I'm not sure about CUBRID (it pretty much aims to be a MySQL clone).
  • Not sure about this {@inheritdoc} thing. I must admit this is the first time I see it and it probably makes sense, but we've never used it and that's probably for a reason.
  • No need to call parent::<method>() if the parent does nothing and other kinds of micro-optimizations - these I can do afterwards.
@narfbg narfbg added a commit that referenced this pull request Jan 20, 2014
@narfbg narfbg Add support for optional table attributes to CI_DB_forge::create_table()
Supersedes PRs #989, #2776
Related issue: #41
27f798b
@narfbg
Collaborator
narfbg commented Jan 20, 2014

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

@narfbg narfbg closed this Jan 21, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment