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

Support for comments on table in all databases #3512

Merged
merged 1 commit into from Aug 8, 2019

Conversation

@moufmouf
Copy link
Contributor

commented Apr 12, 2019

Q A
Type improvement
BC Break no

Summary

I opened a previous PR to add support for table level comments on PostgreSQL: #3499

This PR is an attempt to add the same feature on all databases (and not only MySQL / PostgreSQL).

This PR is built on top of #3499 that already adds support to PostgreSQL.

Commit c4d83a7 moves the functional test from PostgreSQL to the abstract functional test case. Obviously, this test will fail as long as all databases do not support table comments.

Closes #3499

@moufmouf moufmouf force-pushed the moufmouf:comments_on_table_in_all_db branch from 177c2d3 to fb9d2b3 Apr 12, 2019

@moufmouf

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

Commit fb9d2b3 adds support for table comments in Sqlite.

Those databases still need some love:

  • Oracle
  • SQL Server
  • DB2

@moufmouf moufmouf force-pushed the moufmouf:comments_on_table_in_all_db branch 5 times, most recently from 4fca80f to b55c861 Apr 15, 2019

@moufmouf moufmouf changed the title [WIP] Comments on table in all databases Comments on table in all databases Apr 15, 2019

@moufmouf

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

Current status: thanks to this PR, DB comments are now working on SQLServer and SQlite.
I've "skipped" tests on Oracle and DB2 so this PR can be merged (I can open a PR later for Oracle and DB2)

@moufmouf moufmouf changed the title Comments on table in all databases Support for comments on table in SQL Server and Sqlite Apr 15, 2019

@moufmouf moufmouf force-pushed the moufmouf:comments_on_table_in_all_db branch 5 times, most recently from 58c18cc to b7051fe Apr 15, 2019

@moufmouf moufmouf changed the title Support for comments on table in SQL Server and Sqlite Support for comments on table in SQL Server, Sqlite and Oracle Apr 15, 2019

@moufmouf moufmouf changed the title Support for comments on table in SQL Server, Sqlite and Oracle Support for comments on table in all databases Apr 15, 2019

@moufmouf

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

.... and support in DB2 and Oracle has been added! \o/

@moufmouf

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

Since all databases now support table level comments, I took the liberty to add a Table::getComment and a Table::setComment method.
In order to avoid causing BC breaks with MySQL user that previously relied on the "comment" option, those methods are simply writing a "comment" option.

We can remove the "comment" option in the next major release.

@morozov
Copy link
Member

left a comment

@moufmouf thank you for putting this together. We may need to do a couple of rounds of reviews and changes to work through the newly introduced APIs.

lib/Doctrine/DBAL/Schema/SqliteSchemaManager.php Outdated Show resolved Hide resolved
@@ -1621,6 +1624,17 @@ public function getCreateTableSQL(Table $table, $createFlags = self::CREATE_INDE
return array_merge($sql, $columnSql);
}
public function getCommentOnTableSQL(string $tableName, ?string $comment) : string

This comment has been minimized.

Copy link
@morozov

morozov Apr 16, 2019

Member

A comment is usually part of the table declaration (same as name). Should it really be a new public method or it could be used by CREATE/ALTER table internally?

This comment has been minimized.

Copy link
@moufmouf

moufmouf Apr 16, 2019

Author Contributor

It is a good question. I actually tried to mimic what was done with the comments on columns to be consistent.

There is already a public function getCommentOnColumnSQL() method:

* @param string $tableName
* @param string $columnName
* @param string|null $comment
*
* @return string
*/
public function getCommentOnColumnSQL($tableName, $columnName, $comment)
{
$tableName = new Identifier($tableName);
$columnName = new Identifier($columnName);
return sprintf(
'COMMENT ON COLUMN %s.%s IS %s',
$tableName->getQuotedName($this),
$columnName->getQuotedName($this),
$this->quoteStringLiteral((string) $comment)
);
}

If I was to start from zero, I would probably make the method protected, put it in a trait, and "use" this trait directly in the XXXSchemaManager classes. But since there is already a getCommentOnColumnSQL method, I thought it would be better to do something similar.

I'm really open to any change here, if you think there is a better way.

This comment has been minimized.

Copy link
@moufmouf

moufmouf Apr 16, 2019

Author Contributor

It is useful to have it as a separate method because the statement can be different (typically, this method is overloaded for SQLServer). However, you are right that it is not necessary to have this "public". I'm moving this to "protected".

lib/Doctrine/DBAL/Platforms/DB2Platform.php Outdated Show resolved Hide resolved
lib/Doctrine/DBAL/Platforms/SqlitePlatform.php Outdated Show resolved Hide resolved
{
$table = parent::listTableDetails($tableName);
/** @var DB2Platform $platform */

This comment has been minimized.

Copy link
@morozov

morozov Apr 16, 2019

Member

There seems to be some repetition of the code like this: most platforms override listTableDetails(), call the parent and add the comment. Probably, this logic should be part of the parent method itself (most of the platforms support comments).

This comment has been minimized.

Copy link
@moufmouf

moufmouf Apr 16, 2019

Author Contributor

True, but there is a catch.

For most DBMS we want to fetch the comments. For MySQL, we fetch the comments + a bunch of metadata.

What we could do is design a method that returns an array of "options" to be applied to the table. Something like:

    public function listTableDetails($tableName)
    {
        // ....
        $options      = $this->getTableOptions($tableName);
        foreach ($options as $key => $value) {
            $table->addOption($key, $value);
        }
        return $table;
    }

Now, for each XXXSchemaManager, we can provide a protected getTableOptions method that is specific to the schema manager. Would that be ok?

lib/Doctrine/DBAL/Schema/SqliteSchemaManager.php Outdated Show resolved Hide resolved
@moufmouf

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

@morozov thanks a lot for the review. I took in account most of your comments and added some questions/comments where I'm not completely sure what the best solution is.

@morozov

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

@moufmouf sorry, I don't have enough time for this issue right now, so please don't take my absence for ignoring. Unless other core team members help you finish the code changes, I'll try to get back to this as soon as I can.

@moufmouf

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

Any more feedback on this?
Ping @morozov

@Ocramius
Copy link
Member

left a comment

LGTM 👍

@moufmouf

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

Trying to revive this stalled PR (sorry, I got a lot of work lately).
I took into account @Ocramius comments regarding the useless trimming of table comments in Sqlite.

I pulled from master in order to get the latest changes and the build is now failing for a reason that seems unrelated to this PR.
@Ocramius @morozov Do you see any more changes to be made?

@morozov

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

@moufmouf please rebase. All existing issues should be resolved, however MySQL 8 builds may still fail.

@moufmouf moufmouf force-pushed the moufmouf:comments_on_table_in_all_db branch from fcb52fa to d9b9fb2 Aug 5, 2019

@moufmouf

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

@morozov Rebase done.

@moufmouf

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

@morozov @Ocramius I think I handled all comments on this PR.

Is there anything else you want me to do or should I let it rest here until it is merged?

(more specifically, OracleSchemaManager is now rated C by Scrutinizer after I added a new method in it, but I don't really see how to make that rating go away)

@morozov

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

Is there anything else you want me to do or should I let it rest here until it is merged?

I think we're good. Please squash the commits unless you think some of them are relevant fro the history.

@moufmouf moufmouf force-pushed the moufmouf:comments_on_table_in_all_db branch from d9b9fb2 to 62c2a8b Aug 7, 2019

@moufmouf

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

@morozov Squash done.
Thanks a lot for taking the time to review this PR!

@morozov

morozov approved these changes Aug 8, 2019

@morozov morozov merged commit 9ff47e7 into doctrine:master Aug 8, 2019

3 of 4 checks passed

Scrutinizer Analysis: Failure condition met – Tests: passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuousphp Build passed successfully on continuousphp.
Details
@morozov

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

🚢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.