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

Added saving column's comment for PostgresAdapter when you add column #1246

Merged
merged 1 commit into from
Dec 20, 2017

Conversation

Rymmugygr
Copy link

No description provided.

@codecov-io
Copy link

codecov-io commented Nov 28, 2017

Codecov Report

Merging #1246 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1246      +/-   ##
==========================================
+ Coverage   72.32%   72.34%   +0.01%     
==========================================
  Files          35       35              
  Lines        5551     5554       +3     
==========================================
+ Hits         4015     4018       +3     
  Misses       1536     1536
Impacted Files Coverage Δ
src/Phinx/Db/Adapter/PostgresAdapter.php 92.7% <100%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 454238f...dd20ad6. Read the comment docs.

$this->quoteTableName($table->getName()),
$this->quoteColumnName($column->getName()),
$this->getColumnSqlDefinition($column)
);

if ($column->getComment()) {
$sql .= $this->getColumnCommentSqlDefinition($column, $table->getName());
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be done in the getColumnSqlDefinition() method, like MysqlAdapter:

if ($column->getComment()) {
$def .= ' COMMENT ' . $this->getConnection()->quote($column->getComment());
}

and SQLiteAdapter:
$def .= $this->getCommentDefinition($column);

Copy link
Author

@Rymmugygr Rymmugygr Dec 19, 2017

Choose a reason for hiding this comment

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

The problem is that PostgreSQL requires additional (separate) request for adding a comment for a column.

For example:

  • MySQL: "ALTER TABLE books ADD COLUMN author int(10) unsigned NOT NULL COMMENT 'a new comment';"
  • PostgreSQL: "ALTER TABLE books ADD COLUMN author int NOT NULL;" + "COMMENT ON COLUMN books.author IS 'a new comment';"

If we add this logic into getColumnSqlDefinition() its behavior will be much different from the same methods in other adapters.

Comments treated this way in other methods, for example, createTable() adds comments only after column definitions:

foreach ($columns as $column) {
$sql .= $this->quoteColumnName($column->getName()) . ' ' . $this->getColumnSqlDefinition($column) . ', ';
// set column comments, if needed
if ($column->getComment()) {
$this->columnsWithComments[] = $column;
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the explanation. You are right.

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.

3 participants