2.14.0 SQL server incompatible queries

[jlsjonas]

This is a bug.

Prerequisites

• Are you running the latest version? (upgrading to)
• Are you reporting to the correct repository?
  (enso is made of many specialized packages: https://github.com/laravel-enso)
• Did you check the documentation?
• Did you perform a cursory search?

Description

the new enso:upgrade contains queries not compatible with SQL Server

Steps to Reproduce

1. execute enso:upgrade during 2.14.0 upgrade process

Expected behavior

upgrade to execute

Actual behavior

λ php artisan enso:upgrade
The upgrade process has started
Updating data_imports table

   Illuminate\Database\QueryException  : SQLSTATE[42000]: [Microsoft][ODBC Driver 13 for SQL Server][SQL Server]Incorrect syntax near 'MODIFY'. (SQL: ALTER TABLE data_imports MODIFY status TINYINT NOT NULL)

  at C:\Users\EREKEJO1\PhpstormProjects\WebTools\vendor\laravel\framework\src\Illuminate\Database\Connection.php:664
    660|         // If an exception occurs when attempting to run a query, we'll format the error
    661|         // message to include the bindings with SQL, which will make this exception a
    662|         // lot more helpful to the developer instead of just the database's errors.
    663|         catch (Exception $e) {
  > 664|             throw new QueryException(
    665|                 $query, $this->prepareBindings($bindings), $e
    666|             );
    667|         }
    668|

  Exception trace:

  1   Doctrine\DBAL\Driver\PDOException::("SQLSTATE[42000]: [Microsoft][ODBC Driver 13 for SQL Server][SQL Server]Incorrect syntax near 'MODIFY'.")
      C:\Users\EREKEJO1\PhpstormProjects\WebTools\vendor\doctrine\dbal\lib\Doctrine\DBAL\Driver\PDOStatement.php:119

  2   PDOException::("SQLSTATE[42000]: [Microsoft][ODBC Driver 13 for SQL Server][SQL Server]Incorrect syntax near 'MODIFY'.")
      C:\Users\EREKEJO1\PhpstormProjects\WebTools\vendor\doctrine\dbal\lib\Doctrine\DBAL\Driver\PDOStatement.php:117

  Please use the argument -v to see more details.

docs: https://docs.microsoft.com/en-us/sql/t-sql/statements/alter-table-transact-sql?view=sql-server-2017#alter_column

[jlsjonas]

Note that since Laravel 5 you can simply append ->change() to make a change instead of new column.

i.e. on line 65/66, adding $table->tinyInteger('status')->change(); should do the same as the custom statement

(reference)

[aocneanu]

not really

[jlsjonas]

Due to long-lasting laravel/framework#8840 as they both blame each other (a doctrine member claims tinyint is barely supported, although everything but postgres supports it)
Perhaps use the better supported smallIntegrer instead? (https://laravel.com/docs/5.7/migrations#modifying-columns)

Alternatively, perhaps we should consider implementing a custom type for tinyint example?

As an intermediate solution (source), use the following snippet instead of the original \DB::statement: requires connection definition, careful when using multiple connections

$connection = config('database.default');

        $driver = config("database.connections.{$connection}.driver");

        switch($driver)
        {
            case 'sqlsrv':
                \DB::statement('ALTER TABLE data_imports ALTER COLUMN status TINYINT NOT NULL');
                break;

            case 'mysql':
                \DB::statement('ALTER TABLE data_imports MODIFY status TINYINT NOT NULL');
                //mysql syntax
                break;

            case 'sqlite':
                //sqlite syntax
                break;
            default:
                throw new \Exception("Driver not supported: $driver");
            break;
        }

[aocneanu]

Let's simply ignore it and in the future when we will need changes on unsupported types where we will add raw queries we'll try to write a note in the changelog / upgrade steps.

[jlsjonas]

👍
I would suggest keeping track of laravel/framework#8840 for a little while and implement the custom mapping type for tinyint in Enso if Laravel doesn't (as I believe it's quite a common & powerful datatype & shouldn't require much effort)

[aocneanu]

We have several apps with multiple connections: mysql1, mysql2 etc. (more representative names)

[jlsjonas]

I'm sorry but I'm not sure how that's related?
As far as I can see the type definition happens on the framework level, not connection level

[aocneanu]

I think I read between the lines, you're right

[jlsjonas]

The workaround works on a connection basis, that's probably what caused the mix-up. added a notice above it to clarify that.