framework: Timestamp is not a valid Doctrine column type

I have a couple ideas.
Here is what I’m leaning towards because it involves the least amount of change for a user when they have alter migrations. This approach continues the current practice of only including doctrine/dbal when the user needs alter migrations. But it configures the application with existing data and functions therefore allowing the user to just include dbal-and-go instead of adding a new ServiceProvider too (see idea #2)

Idea #1:

In https://github.com/laravel/laravel/blob/8.x/config/database.php add a configuration option

use Illuminate\Database\DBAL\TimestampType;

'dbal' => [
    'types' => [
        'timestamp' => TimestampType::class,
    ],
],

Use this configuration in https://github.com/laravel/framework/blob/8.x/src/Illuminate/Database/DatabaseServiceProvider.php#L39 and add to the boot() function:

$this->addDbalTypes();                       

and in the class add

use Doctrine\DBAL\Types\Type;

protected function addDbalTypes() 
{
    if (class_exists(Type::class)) {
        $types = config('database.dbal.types');
        
        if (! is_array($types)) {
            // No need to throw an exception here?
            return;
        }
    
        foreach ($types as $typeName => $typeDefinition) {
             if (! Type::hasType($typeName) {
                Type::addType($typeName, $typeDefinition);
            }            
        }
    }
}

Finally create a proper Illuminate\Database\DBAL\TimestampType class

use Doctrine\DBAL\DBALException;
use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Types\Type;

class TimestampType extends Type
{
    public function getName()
    {
        return 'timestamp';
    }

    public function getSQLDeclaration(array $fieldDeclaration, AbstractPlatform $platform)
    {
        $name = $platform->getName();

        // See https://www.doctrine-project.org/projects/doctrine-dbal/en/latest/reference/configuration.html
        switch ($name) {
            case 'mysql':
            case 'mysql2':
                return $this->getMySQLPlatformSQLDeclaration($fieldDeclaration);
                break;

            case 'sqlite':
            case 'sqlite3':
                return $this->getSqlitePlatformSQLDeclaration($fieldDeclaration);
                break;
            
            case 'mssql':
                return $this->getMSSQLPlatformSQLDeclaration($fieldDeclaration);
                break;

            case 'pgsql':
            case 'postgres':
            case 'postgresql':
                return $this->getPostgresqlPlatformSQLDeclaration($fieldDeclaration);
                break;
            
            default:
                throw DBALException::notSupported($name);
        }              
    }

    protected function getMySQLPlatformSQLDeclaration(array $fieldDeclaration) 
    {
        $columnType = 'TIMESTAMP';
        
        if ($fieldDeclaration['precision']) {
            $columnType = 'TIMESTAMP(' . $fieldDeclaration['precision'] . ')';
        }
        
        $notNull = $fieldDeclaration['notnull'] ?? false;
        
        if (! $notNull) {       
            return $columnType . ' NULL';
        }
        
        return $columnType;
    }
    
    ...
}

So I don’t come across as crazy, I want to point out that I’m not using MSSQL. It is included because Laravel claims support for it and I’m a completist.

Now, as I said above, I work on the Doctrine team, and in Doctrine metadata for the ORM is managed in a very different way than Laraval’s ORM (and not just because it is a Data Mapper pattern). But for both ORMs metadata can be extracted into an ORM abstraction pattern. And in Laravel, the available metadata for date fields includes datetime and timestamp regardless of the database driver which means that this bit of code is valid for every database Laravel supports including MSSQL

$table->timestamp('created_at')->after('user_id')->change();

It would be crazy to write documentation that says “if you’re using MSSQL you have to modify your migrations” because portable migrations seem to be the very goal of Laravel’s migration code. If a user advertises their product works on MySQL, PostgreSQL, SQLite, and MSSQL then Laravel should support what Laravel says it supports and not force such a user to put in database driver-specific migrations.

I may be short-sighted concerning the availability of unit testing in laravel/framework because I thought PostgreSQL was in the same boat as MSSQL; that being there are no unit tests for PostgreSQL because the docker-composer.yml file for laravel/framework does not include PostgreSQL and https://github.com/laravel/framework/blob/8.x/.github/workflows/tests.yml does not include PostgreSQL.

And to close my own sanity argument I did start this issue because I’m using PostgreSQL.

Going to close this as this is technically an enhancement. Let’s focus on the PR.

Great! I’ll put a PR together soon and reference this issue.