[Issue] Prevent `db_schema.xml` declarations with `comment=""` from breaking zero downtime deployments

[m2-assistant]

This issue is automatically created based on existing pull request: #40242: Prevent db_schema.xml declarations with comment="" from breaking zero downtime deployments

---

Many magento deployment pipelines achieve zero downtime deployments by checking if database migrations need to be executed with setup:db:status and only putting up maintenance mode if there are changes needing to be made.

This process can be seen in deployerphp/deployer here.

The issue

The way setup:db:status works is by

1. Reading all of the db_schema.xml into an array
2. Reading all of the data from the database into an array
3. Comparing the results, non zero exit code if changes are detected

The problem we frequently see is that third party modules inadvertently make a small mistake which breaks zero downtime deployments, they say comment="" instead of omitting entirely.

Given that the mistake is small and basically a NOOP it has a disproportionately negative impact. You can see this is not an infrequent error, many extensions make this mistake https://github.com/search?q=path%3A**%2Fdb_schema.xml+%22comment%3D%5C%22%5C%22%22&type=code

The issue is if you define a db_schema.xml with comment="" then the system will always be flagged as needing to make changes. When a table is created there is no concept of a null versus empty string comment, the comment is either there or it is not and this disparity breaks the comparison.

Why this happens

db_schema.xml	Database COMMENT section	Result
<column comment="commented"/>	COMMENT 'commented'	✅ db schema and database both agree the comment exists
<column />		✅ db schema and database both agree the comment does not exist and is null
<column comment=""/>		⚠️ db schema believe the comment should be an empty string, the database believes the comment does not exist

The data is read from the here database, defaulting the comment to NULL if it is not existing

magento2/lib/internal/Magento/Framework/Setup/Declaration/Schema/Db/MySQL/DbSchemaReader.php

Lines 97 to 119 in 98f028a

	public function readColumns($tableName, $resource)
	{
	$columns = [];
	$adapter = $this->resourceConnection->getConnection($resource);
	$dbName = $this->resourceConnection->getSchemaName($resource);
	$stmt = $adapter->select()
	->from(
	'information_schema.COLUMNS',
	[
	'name' => 'COLUMN_NAME',
	'default' => 'COLUMN_DEFAULT',
	'type' => 'DATA_TYPE',
	'nullable' => new Expression('IF(IS_NULLABLE="YES", true, false)'),
	'definition' => 'COLUMN_TYPE',
	'extra' => 'EXTRA',
	'comment' => new Expression('IF(COLUMN_COMMENT="", NULL, COLUMN_COMMENT)'),
	'charset' => 'CHARACTER_SET_NAME',
	'collation' => 'COLLATION_NAME'
	]
	)
	->where('TABLE_SCHEMA = ?', $dbName)
	->where('TABLE_NAME = ?', $tableName)
	->order('ORDINAL_POSITION ASC');

Potential fix

Given that the error here is never in the databases side of things, I believe it makes most sense to correct this during the reading and handling of db_schema.xml. We already have an area of code which is doing casting and manipulation of elements, so stripping out empty comment declarations (which will never make it to the database anyway) seems safe to me.

The fix and manual testing

In the past I have used this script I wrote for debugging what is breaking setup:db:status. You can copy that and place it in your magento root as setup-db-status.php

We can force in some example db_schema.xml changes by editing vendor/magento/module-catalog/etc/db_schema.xml and placing these at the bottom of the file. These changes cover all cases to do with comments

• Properly declared
• Not existing
• Declared as empty string

    <table name="some_dummy_table123" resource="default" engine="innodb" comment="">
        <column xsi:type="text" name="some_dummy_field_with_empty_string_comment" comment=""/>
        <column xsi:type="text" name="some_dummy_field_with_full_comment"  comment="commented"/>
        <column xsi:type="text" name="some_dummy_field_with_no_comment_declaration" />
    </table>

    <table name="some_dummy_table456" resource="default" engine="innodb" comment="we have a comment">
        <column xsi:type="text" name="some_dummy_field_with_empty_string_comment" comment=""/>
        <column xsi:type="text" name="some_dummy_field_with_full_comment"  comment="commented"/>
        <column xsi:type="text" name="some_dummy_field_with_no_comment_declaration" />
    </table>

    <table name="some_dummy_table789" resource="default" engine="innodb" >
        <column xsi:type="text" name="some_dummy_field_with_empty_string_comment" comment=""/>
        <column xsi:type="text" name="some_dummy_field_with_full_comment"  comment="commented"/>
        <column xsi:type="text" name="some_dummy_field_with_no_comment_declaration" />
    </table>

</schema>

After running setup:upgrade we can see that we still have changes being flagged.

$ php bin/magento set:up # output redacted for brevity
$ php bin/magento setup:db:status
Declarative Schema is not up to date
Run 'setup:upgrade' to update your DB schema and data.

And the deeper analysis using the debug script above

$ php setup-db-status.php
modify_table                  some_dummy_table123
modify_column                 some_dummy_table123 some_dummy_field_with_empty_string_comment
old: {"type":"text","nullable":true,"comment":null}
new: {"type":"text","nullable":true,"comment":""}

modify_column                 some_dummy_table456 some_dummy_field_with_empty_string_comment
old: {"type":"text","nullable":true,"comment":null}
new: {"type":"text","nullable":true,"comment":""}

modify_column                 some_dummy_table789 some_dummy_field_with_empty_string_comment
old: {"type":"text","nullable":true,"comment":null}
new: {"type":"text","nullable":true,"comment":""}

You can see that it believes the "old" value in the database is a null comment, while the "new" value from the schema is an empty string.

When you apply the fix from this PR we get the following output, which will allow for more robust zero downtime deployments

$ php bin/magento setup:db:status
All modules are up to date.

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
• All automated tests passed successfully (all builds are green)