Support for comments on table in all databases

[moufmouf]

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]

Commit fb9d2b3 adds support for table comments in Sqlite.

Those databases still need some love:

• Oracle
• SQL Server
• DB2

[moufmouf]

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]

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

[moufmouf]

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]

@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.

[morozov]

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?

[moufmouf]

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:

dbal/lib/Doctrine/DBAL/Platforms/AbstractPlatform.php

Lines 1639 to 1656 in 4ea7c11

	* @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.

[moufmouf]

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".

[morozov]

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).

[moufmouf]

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?

[moufmouf]

@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]

@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]

Any more feedback on this?
Ping @morozov

[Ocramius]

LGTM 👍

[moufmouf]

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]

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

[moufmouf]

@morozov Rebase done.

[moufmouf]

@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]

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]

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

#3512 (comment) is resolved.

[morozov]

🚢