[db]: Remove not supported column comments for SQLite

[susnux]

• Resolves Add lastUpdated property to Form forms#1479 (comment)

Summary

Some times column comments are used, e.g. to make clear an integer is used as a timestamp.
For SQLite column comments are not supported and migration that use column comments will not work (see linked comment above for an example).
Somehow it works when you have a clean install, then all migrations pass, but when executing single migrations they will fail.

This solves the problem by simply removing column comments when using SQLite.

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• Tests (unit, integration, api and/or acceptance) are included
• Screenshots before/after for front-end changes
• Documentation (manuals or wiki) has been updated or is not required
• Backports requested where applicable (ex: critical bugfixes)

[kesselb]

Sounds weird.

Comments are supported by doctrine and sqlite: doctrine/dbal#819

[susnux]

Sounds weird.

Comments are supported by doctrine and sqlite: doctrine/dbal#819

The documentation does not list SQLite as supported:
https://www.doctrine-project.org/projects/doctrine-dbal/en/3.2/reference/schema-representation.html#common-options

Also SQL comments are not supported by SQLite (only inline comments within the schema).

How you can reproduce the error:

1. Setup a nextcloud server using SQLite (e.g. docker image)
2. Install and enable the forms app (version 3.1.0)
3. Update forms to nextcloud/forms@3181423
4. Try to run migrations and see error.

Or any other app you like and try to run a migration containing

$table->addColumn('foo', Types::INTEGER, [
	'notnull' => false,
	'default' => 0,
	'comment' => 'unix-timestamp',
]);

[kesselb]

Hi,

I created a bug report for doctrine/dbal: doctrine/dbal#5934.

Also SQL comments are not supported by SQLite (only inline comments within the schema).

'comment' => 'unix-timestamp', that's an inline comment within the schema ;)

Some times column comments are used, e.g. to make clear an integer is used as a timestamp.

I don't like the idea of having code in Nextcloud to remove comments on SQLite.

My recommendation is to remove the comments from the migrations because a comment in the code is a better place to document that a column should store a certain type of data.

[kesselb]

Seems valid, should work, still I don't like it ;)

[jotoeri]

Looks good to me. 👍

I'd rather have the comment at least when using mysql,... (and e.g. phpmyadmin), than not having it at all. To have code-comments is independently of course a good thing.

• One could perhaps here add a comment to link to the dbal-issue. So once resolved one could re-check, if the comment-removal is still necessary.

[jotoeri]

/backport to stable25

[jotoeri]

/backport to stable24

[nickvergessen]

Can we add a test to https://github.com/nextcloud/server/blob/master/tests/lib/DB/MigratorTest.php
So that it is ensured that SQLite keeps working?

[susnux]

Can we add a test so that it is ensured that SQLite keeps working?

@nickvergessen done :)