add fix for mariadb virtual column creation

[oskarkregar]

When using virtual column definition with Literal class, migration fails with SQL syntax error on MariaDB as NOT NULL or NULL are not allowed at the end of column definition. Below is the link to an issue.

Fixes https://github.com/cakephp/phinx/issues/2157

[ndm2]

I'm not sure if it's overly reasonable to completely break the null option for all literal type usages with MariaDB, just because of this one quirks.

Maybe the check should rather look for SQL that is incompatible with [NOT] NULL, or maybe it should be possible to explicitly suppress generating [NOT] NULL?

In any case, this should have tests. Since existing tests didn't break, there seems to already be missing coverage.

[MasterOdin]

We don't currently test against mariadb, so we'd need to wire that up before we could have a good test for this. MySQL does happily allow null and not null for virtual columns so I wouldn't expect our existing tests to be able to cover this.

[oskarkregar]

It could then be fixed with option to explicitly suppress null and not null as @ndm2 suggested. It is not needed on most MySQL and MariaDB column creation (except on TIMESTAMP type).

[kyojin007]

Is there an argument to say that adding options to a Literal column type is unnecessary? By definition we are manually specifying the column and allowing options just adds complexity.

Then we might do;

        if (!$column->getType() instanceof Literal) {
            $def .= $column->getEncoding() ? ' CHARACTER SET ' . $column->getEncoding() : '';
            $def .= $column->getCollation() ? ' COLLATE ' . $column->getCollation() : '';
            $def .= !$column->isSigned() && isset($this->signedColumnTypes[$column->getType()]) ? ' unsigned' : '';
            $def .= $column->isNull() ? ' NULL' : ' NOT NULL';
        }

[dereuromark]

@ndm2 So you think we shouldn't merge this?

[ndm2]

@dereuromark Not in this state, no. IMHO there should either be an option to exclude [not] null generation, or there should be a clear way to define the whole column creation SQL via a Literal object. I would also second @MasterOdin that this should have an actual test on MariaDB.