Removed Unsigned Integer Support in SQL Server (and unsupported platforms)

[scalp]

Hi,

In my opinion PR-207 didn't remove full support in SQL Server of UNSIGNED integer.

Like ISSUE-1695 for SQLite, Schema\Comparator return differences for each column with "unsigned => true" attribute.

As i see, UNSIGNED integer is not available from neither SQL Server, PostgreSQL, SQLite, DB2, ...

I tried to see in which way it would be possible to fix it :

1. i think the best way would be to remove "unsigned" from defaults attributes and switch it to Schema\Column\PlatformOptions or a newer property like PlatformAttributes. The problem is that it is a lot of work and will cause BC breaks.
2. another way, would be to add a method supportsUnsignedInteger in Platforms\AbstractPlatform and test it in Schema\SchemaDiff_toSql method to don't add SQL lines if unsupported. But i've an unresolved problem with this option, if (int) type change (small int to int for example) or nullable property.
3. last way was to inject Platform in Schema\Comparator and test support (like previous point with $platform->supportsUnsignedInteger) in Schema\Comparator\diffColumn method.

What do you think about theses solutions ? I can make a PR for solution 2 or 3. First way is too complicated for me and maybe a little bit unreasonable.

[deeky666]

@scalp I'm not sure if I get the issue here. Where is SQL Server platform even using unsigned? Please provide us some information on how to reproduce the actual issue. Thanks!

[scalp]

Hi @deeky666, it's the same issue as ISSUE-1695 but for SQLServer.

Requests are finely generated without UNSIGNED attribute by DBAL but Schema\Comparator method always detect differences on syncing methods.

It's because Schema\SQLServerSchemaManager->createSchema return a Schema\Schema without UNSIGNED attribute for concerned columns (as SQLServer doesn't support it) whereas Tool\SchemaTool->getSchemaFromMetadata return a Schema\Schema with UNSIGNED attribute. So when Schema\Comparator->diffColumn check differences, it returns an unsigned changed properties for each concerned column.

Impact is schema and database are never consider as sync for exemple by command like "doctrine:schema:update" whereas they are. For instance, it's a problem when you use third libraries which defines unsigned => true in schema.

[deeky666]

@scalp got it now. Thanks for reporting. Unfortunately the comparator isn't platform aware so vendor specific comparator issues have to be dealt with (and is already done) in the platform's getAlTerTableSQL() method when generating SQL for the column diffs. The issue should be fixed there.

[scalp]

I understand comparator isn't platform aware.

But why in your opinion this should be fixed in Platforms\SQLServerPlatform->getAlterTableSQL instead of modify these lines of _Schema\SchemaDiff->toSql. For instance with a new platform method like $platform->supportsUnsignedInteger and a check on each changedColumns if ColumnDiff->changedProperties contains only unsigned then don't execute getAlterTableSQL ?

[deeky666]

@scalp the problem with that approach is, that it doesn't fix getAlterTableSQL() itself which can be used directly without SchemaDiff. Also the root problem is not solved by adding additional supports*() method to the platform as there are many other different vendor specific semantics for table alteration that cannot be abstracted easily. For example some platform disallow altering from specific column types to others (each vendor having their own constraints).
Those vendor specific constraints are already dealt with in getAlterTableSQL() methods as it is the only place where to fix these things properly. For 2.x we won't do any big changes to that approach.

[bartfeenstra]

We ran into this problem last week, as we started migrating a project from MariaDB to SQL Server. I agree with @deeky666 that Comparator is platform-unaware and that's not where we should address this. However, while is the platform that builds the SQL, for schema validation it's SchemaTool building the schema that includes unsigned definitions in the first place.

[morozov]

As of #4746, schema comparison is schema-aware. Could somebody verify if this issue is still relevant?

[bartfeenstra]

I changed jobs a few weeks ago, and I am no longer able to test this. Sorry for the inconvenience!

[morozov]

Then I'll close it given that the issue doesn't contain reproduction steps.

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.