Prevent db_schema.xml declarations with comment="" from breaking zero downtime deployments

[convenient]

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)

[m2-assistant]

Hi @convenient. Thank you for your contribution!
Here are some useful tips on how you can test your changes using Magento test environment.
❗ Automated tests can be triggered manually with an appropriate comment:

• @magento run all tests - run or re-run all required tests against the PR changes
• @magento run <test-build(s)> - run or re-run specific test build(s)
  For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names.

Allowed build names are:

1. Database Compare
2. Functional Tests CE
3. Functional Tests EE
4. Functional Tests B2B
5. Integration Tests
6. Magento Health Index
7. Sample Data Tests CE
8. Sample Data Tests EE
9. Sample Data Tests B2B
10. Static Tests
11. Unit Tests
12. WebAPI Tests
13. Semantic Version Checker

You can find more information about the builds here
ℹ️ Run only required test builds during development. Run all test builds before sending your pull request for review.

---

For more details, review the Code Contributions documentation.
Join Magento Community Engineering Slack and ask your questions in #github channel.

[convenient]

@magento run all tests

[furan917]

Excellent addition, I love it.

You get test failures for B2B which are nothing to do with you. (I hate that BTW)
And you get linting failure, 1 is yours, ones from ghosts past.

There was 1 failure:

1) Magento\Test\Php\LiveCodeTest::testCodeStyle
PHP Code Sniffer detected 1 violation(s): 

FILE: /var/www/html/lib/internal/Magento/Framework/Setup/Declaration/Schema/Dto/ElementFactory.php
--------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
--------------------------------------------------------------------------------------------------
 26 | WARNING | Class properties must have type declaration using @var tag.
 77 | WARNING | There must be exactly one blank line before tags
--------------------------------------------------------------------------------------------------



Failed asserting that 1 matches expected 0.

/var/www/html/dev/tests/static/testsuite/Magento/Test/Php/LiveCodeTest.php:257

FAILURES!
Tests: 6, Assertions: 5, Failures: 1, Skipped: 1.

A linebreak at the @see should cover that and the B2B failures are out of your hands.

[convenient]

@magento run all tests

[convenient]

@magento run all tests

[convenient]

@magento run Unit Tests, Functional Tests EE, Functional Tests B2B

[convenient]

@magento run Functional Tests B2B

[convenient]

@magento run Functional Tests B2B, Sample Data Tests CE, Sample Data Tests EE, Sample Data Tests B2B