Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Timestamp is not a valid Doctrine column type #35498

Closed
TomHAnderson opened this issue Dec 5, 2020 · 16 comments
Closed

Timestamp is not a valid Doctrine column type #35498

TomHAnderson opened this issue Dec 5, 2020 · 16 comments

Comments

@TomHAnderson
Copy link
Contributor

TomHAnderson commented Dec 5, 2020

  • Laravel Version: 8.17.0
  • PHP Version: 7.3.25
  • Database Driver & Version: mysql / 10.2.36-MariaDB

Preface

I created the below issue from my own research before finding this duplicate issue:

This bug report is a duplicate of: #16526

and I'd like to address the resolution of that issue here. As a member of the Doctrine team I can assure you this is not an issue in Doctrine. It is a design decision just as not supporting enum is a design decision. But Laravel uses timestamp column types throughout and timestamp is a standard in Laravel. So just because Doctrine chooses not to support this directly is not reason enough for Laravel not to take the small steps necessary to support Laraval's own design decisions and implement the column in the DBAL library Laravel is integrating with. I'm open to writing the PR for this.

Description:

Attempting to change a column from a datetime to a timestamp results in the following exception:

Unknown column type "timestamp" requested. Any Doctrine type that you use has to be registered with \Doctrine\DBAL\Types\Type::addType(). You can get a list of all the known types with \Doctrine\DBAL\Types\Type::getTypesMap(). If this error occurs during database introspection then you might have forgotten to register all database types for a Doctrine Type. Use AbstractPlatform#registerDoctrineTypeMapping() or have your custom types implement Type#getMappedDatabaseTypes(). If the type name is empty you might have a problem with the cache or forgot some mapping information.

Steps To Reproduce:

Run this migration against an empty database:

    public function up()
    {
        Schema::create('user_to_role', function (Blueprint $table) {
            $table->bigInteger('role_id')->unsigned();
            $table->bigInteger('user_id')->unsigned();
            $table->primary(['role_id','user_id']);
        });

        Schema::table('user_to_role', function (Blueprint $table) {
            $table->dateTime('created_at')->after('user_id');
        });

        Schema::table('user_to_role', function (Blueprint $table) {
            $table->timestamp('created_at')->after('user_id')->change();
        });
    }

Discussion

Laravel uses the mysql timestamp field type throughout the framework. To modify columns Laravel uses the Doctrine DBAL library. Doctrine has a strict set of data types and uses the more generic datetime instead of the MySQL specific timestamp.

The problem reported is the same class of problem where Doctrine does not support the enum datatype: https://www.doctrine-project.org/projects/doctrine-orm/en/2.7/cookbook/mysql-enums.html

This library creates a proper timestamp column type for DBAL: https://github.com/marktopper/doctrine-dbal-timestamp-type and it includes a Laravel 5 service provider which suggests to me that this problem has been around for some time.

I do not believe it should be necessary to install a 3rd party package in order to change a column to the widely used and supported timestamp data type.

See also:

Similar issue with enum:
#35096

Documentation where changing to enum is not supported says nothing about timestamp
https://github.com/laravel/docs/blame/8.x/migrations.md#L403

@taylorotwell
Copy link
Member

So... what are you suggesting we do?

@TomHAnderson
Copy link
Contributor Author

TomHAnderson commented Dec 6, 2020

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;
    }
    
    ...
}

@TomHAnderson
Copy link
Contributor Author

My second idea is to create a small Laravel branded repo which requires doctrine/dbal and includes a ServiceProvider, configuration, and TimestampType class. To use this a user would need to

  1. require the repo
  2. add the config
  3. add the service provider

This is an alternate solution and not my preferred one.

@TomHAnderson
Copy link
Contributor Author

Using either of these methods opens the possibility to rename an enum column.

@driesvints
Copy link
Member

Hey @TomHAnderson. Thanks for the very thorough analysis of the problem and the solutions you proposed. I think your first solution is the most applicable one as it's indeed unfeasible to create a new package just for this.

I'm wondering why we need to add a new config option? Can't we directly add these types in the framework? What's the main benefit of letting the user control these as a config option? (Maybe I missed this from the above)

@TomHAnderson
Copy link
Contributor Author

@driesvints I haven't analyzed the problem so throughly as to know all the issues users have with field types. I do know that enum is an issue for many and of course the issue at hand with timestamp. By including a configuration it allows users to specify their own field types.

However to build beyond the lines drawn by Laravel is breaking with convention. My approach to programming is to let the programmer draw their own lines via configuration. And this is the root cause of your question, I believe.

Should the decision be made to not include the configuration then I would feel an obligation to include enum support. But proper enum support limits the values to those of the enum. The easy enum solution is to map enum to string and this works for the vast majority of cases. But it isn't exact: but it works. So, by including a configuration section it puts off the decision of including enum support. Because enum is not a design decision of Laravel I don't feel obligated to support it the same way I feel about timestamp. So if no configuration is provided then users would be locked out of defining enum for themselves.

So for the same reason that Doctrine provides a full page of instructions about enum so too could Laravel create a similar page if the user has the ability to map it themselves.

@driesvints
Copy link
Member

@TomHAnderson let's try with the configuration and work from there? It's easier to review a PR and look at actual code :)

@TomHAnderson
Copy link
Contributor Author

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

@TomHAnderson
Copy link
Contributor Author

@driesvints do you want these PR on the current 8.x branch or master?

I don't see a harm in putting them into 8.x and if a user wants to use them they can add the configuration to database.php.

@driesvints
Copy link
Member

8.x is fine!

@driesvints
Copy link
Member

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

@driesvints
Copy link
Member

I'm gonna re-open this again for a sec to discuss one more thing.

I have to say Tom, there's something I've been thinking about after looking at your example from above some more. Does it actually make sense to do this part on a MSSQL database to begin with?

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

You know up front that the timestamp column type isn't available in MSSQL so why do the change to begin with? Given that we're purely now setting up the related PR to make sure the transition is smoothly on MSSQL I'm not sure it's needed at all? Would love to have your input on that one.

@driesvints driesvints reopened this Dec 14, 2020
@TomHAnderson
Copy link
Contributor Author

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.

@driesvints
Copy link
Member

Okay so I see a use case here for something like a package that ships with migrations but decided to change the datetime type for a column to a timestamp type. In that scenario you don't know what database engine the consumer uses. That definitely makes sense.

@TomHAnderson
Copy link
Contributor Author

TomHAnderson commented Dec 15, 2020

I'm glad you're seeing the benefit to a Laravel branded repository for alter migrations (original comment #35498 (comment))

I have an even better reason to create this repository. doctrine/dbal recently released v3.0. There are namespace changes to the drivers (at a minimum) which break Laravel's implementation with it. v2.x still works fine. The problem is by asking a user to simply require doctrine/dbal they will get a version that does not work because the version is not constrained by composer.json.

I think Laravel should desire to implement v3.0 but I don't see a pressing need so I would [still] like a Laravel branded repo for alter migrations which would provide the necessary version constraint.

@driesvints
Copy link
Member

@TomHAnderson no I think you misunderstood me. I highlighted my comment above as a practical use case for the PR you've sent in now, not as a potential solution to the current problem. Your current PR is fine.

The problem is by asking a user to simply require doctrine/dbal they will get a version that does not work because the version is not constrained by composer.json.

We highlight which Doctrine DBAL versions are compatible in the "suggest" of our composer.json. Atm we support both 2.x and 3.x: https://github.com/laravel/framework/blob/8.x/composer.json#L132

Although undecided at this time, Laravel 9 which is coming out end of february/beginning of march will probably only support DBAL 3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants