cakephp / phinx

PHP Database Migrations for Everyone
https://phinx.org
MIT License
4.46k stars 890 forks source link

Feature Request: custom data types #753

Open bradynpoulsen opened 8 years ago

bradynpoulsen commented 8 years ago

I think it would be really useful to have a way to add a custom column name data type when using addColumn(). This would then make it so it could be aliased to an actual data type.

For example with MySQL,

without custom type

$table->addColumn('id', 'binary', ['limit' => 16]); // uuid v4

with custom type

$table->addColumn('id', 'uuid');
bradynpoulsen commented 8 years ago

Here is a possible flow:

twoixter commented 8 years ago

@bradynpoulsen This is actually something I was thinking about implementing. This semantic is very much like a User Defined Type or a Data Domain. I was skeptical at first because each database has a concept of UDT or Domain (E.G: Check constraints and UDT in SQL Server) and AFAIK, don't supported in migrations. My concern was that if implemented, it would give the impression to extend the user defined type to the DB Adapter.

This is my suggestion for the flow. If interested, I might create a fork and start developing:

Define a new "domain" for our data types in the phinx.yml configuration

# Domain defined in the config phinx.ml
# This would start a "domain" for our data types
domain:
    phone:    # New user defined data type name
        type: string   # Based on this base phinx data type
        options:       # With these options.
            length: 9
            null: true
    short_status:
        type: enum
        values: pending, started, finished

The intention is to use those as new data types, and they'd just use the values for type and options.

    // Use our user defined types from config
    $table->addColumn("phone_number", "phone");
    $table->addColumn("task_status", "short_status");

Since those are really just aliases for the type and options, we could just add new options to override:

    // Use our user defined types from config
    $table->addColumn("mobile_number", "phone", array("length" => 12, "null" => false));

Let me know what do you think.

bradynpoulsen commented 8 years ago

That would certainly acheive the goals of my example in a much more isolated location instead of having to include a bunch of driver classes

dereuromark commented 6 years ago

Are you able to provide a patch with your suggested changes as PR?

twoixter commented 6 years ago

Hi @dereuromark ! Not yet. It was just a suggestion at the time, but never worked on it.

I'm willing to fork and do a PR if it's still relevant! (This issue dates back to Jan, 2016).

dereuromark commented 6 years ago

You can give it a try and see if that is still relevant. And then a PR would be most welcome!

twoixter commented 6 years ago

Hi @dereuromark ! I just sent PR #1320 for this feature. I'm still fixing some stickler-ci and phpunit version issues for Travis, but you can start looking at it.

dereuromark commented 4 years ago

I tried myself today (as per https://github.com/cakephp/cakephp/pull/11297/files#diff-26ba5244f4728ab88ec0f493f66576e2R37 )

$table->addColumn('uuid', 'binaryuuid', [
    'default' => null,
    'null' => false,
]);

and got

InvalidArgumentException: An invalid column type "binaryuuid" was specified for column "uuid".

dereuromark commented 4 years ago

This is now solved for non primary key columns for sure using e.g.

$table->addColumn('uuid', 'binaryuuid', [
    'default' => null,
    'null' => false,
]);

For primary key ones we have to further investigate and add test cases.

geoidesic commented 4 years ago

Doesn't seem to work for me. I get InvalidArgumentException: An invalid column type "binaryuuid" was specified for column "id"

robmorgan/phinx                       dev-0.next 7b3ca6c
dereuromark commented 4 years ago

Make sure to use master (as 0.next is behind) But either way, we need to figure out then why it is throwing this error here.

What DB type are you using? We only enabled this so far for a few DBs.

geoidesic commented 4 years ago

MariaDB (MySQL)

geoidesic commented 4 years ago

Master branch doesn't work for me:

Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - cakephp/migrations 3.0.0-beta2 requires robmorgan/phinx 0.next-dev -> satisfiable by robmorgan/phinx[dev-0.next].
    - cakephp/migrations 3.0.0-beta2 requires robmorgan/phinx 0.next-dev -> satisfiable by robmorgan/phinx[dev-0.next].
    - cakephp/migrations 3.0.0-beta2 requires robmorgan/phinx 0.next-dev -> satisfiable by robmorgan/phinx[dev-0.next].
    - Can only install one of: robmorgan/phinx[dev-0.next, 0.12.3].
    - Can only install one of: robmorgan/phinx[0.12.3, dev-0.next].
    - Can only install one of: robmorgan/phinx[0.12.3, dev-0.next].
    - Installation request for robmorgan/phinx ^0.12.3 -> satisfiable by robmorgan/phinx[0.12.3].
    - Installation request for cakephp/migrations (locked at 3.0.0-beta2, required as ^3.0) -> satisfiable by cakephp/migrations[3.0.0-beta2].
dereuromark commented 4 years ago

you are using an outdated version here. we have no betas anymore. make sure you upgraded your migrations plugin properly.

dereuromark commented 4 years ago

By the way: This works for me in a migration file for MySQL

        $options = [
            'id' => false,
            'primary_key' => 'id',
        ];

        $table = $this->table('uuid_users', $options);
        $table->addColumn('id', 'binaryuuid', [
            'null' => false,
        ]);

        $table->addColumn('name', 'string', [
            'default' => null,
            'limit' => 100,
            'null' => false,
        ]);

        $table->addColumn('additional_uuid', 'binaryuuid', [
            'default' => null,
            'null' => true,
        ]);

        $table->save();
geoidesic commented 4 years ago

I've tested this now on latest Phinx (0.12.3), CakePHP (4.1) and Migrations plugin (3.0.0), on a newly created Cake project. On PHP 7.3.9 and 7.4.1 with MySQL 5.7.26. The migration works but I get this error whenever trying to fill a binaryuuid within a Seed by using Text::uuid.

  SQLSTATE[22001]: String data, right truncated: 1406 Data too long for column 'alternate_uuid' at row 1

As far as I can tell this means that the uuid is not being converted to binary, although the datatype's code shows that this should happen automatically: https://api.cakephp.org/3.8/source-class-Cake.Database.Type.BinaryUuidType.html#154-167

geoidesic commented 4 years ago

Setting aside the binaryuuid type for a moment, It doesn't seem to me that the toDatabase method of the Type is called. I've tried putting a die statement in there and the seeder carries on merrily regardless, so it's never reached. In fact when working with uuid data types, even if I delete the UuidType.php file from vendor/cakephp/src/Database/Type it still seems to generate my seeded tables.

geoidesic commented 4 years ago

Getting back to binaryuuid it does seem to me that the conversion is not being called, because when I do this, the seeder finally generates a record with (what is presumably) a valid binaryuuid pk:

<?php

use Migrations\AbstractSeed;
use Cake\Utility\Text;

/**
 * Uuid seed.
 */
class AppDataSeeder extends AbstractSeed
{
    private function convert($string)
    {
        $string = str_replace('-', '', $string);
        return pack('H*', $string);
    }

    public function run()
    {
        $data = [
            [
                'id' => $this->convert(Text::uuid()),
                'name' => 'individual',
                'additional_uuid' => Text::uuid(),
            ],
        ];

        $table = $this->table('uuid_users');
        $table->insert($data)->save();
    }
}
dereuromark commented 4 years ago

@lorenzo Isn't the idea of using the ORM wrapper here, that it automatically uses the built in CakePHP type class for this (BinaryUuid)? Which would trigger this conversation? At least for me in the CakePHP + Migrations plugin context this works as expected.

geoidesic commented 4 years ago

Also ORM validation seems to break. Looks like the default expects id fields to be integers, even when the Type is uuid. [Actually nvm this – Seems to be due to me not having re-baked my Table objects]