spotorm / spot2

Spot v2.x DataMapper built on top of Doctrine's Database Abstraction Layer
http://phpdatamapper.com
BSD 3-Clause "New" or "Revised" License
601 stars 101 forks source link

Use "default" as default value if given #229

Closed Arzaroth closed 7 years ago

Arzaroth commented 7 years ago

A very simple PR addressing issue #215 I also had to adapt unit test for email validation, because of Valitron changes

vlucas commented 7 years ago

I think this is a good solution as well. While this won't work the other way with DB migrations, it does make sense this way and I think will serve to avoid some confusion.

tuupola commented 7 years ago

There should be only one test of feature per pull request ie this should be two pull requests. Also the new default feature needs tests.

Arzaroth commented 7 years ago

I rebased on upstream which merged #217 so this PR is solely for the proposed minor tweak. A very basic test was added as well.

tuupola commented 7 years ago

What happens if I have Entity with DateTime() value?

public static function fields()
{
    return [
        "id" => ["type" => "integer", "unsigned" => true, "primary" => true, "autoincrement" => true],
        "created_at"   => ["type" => "datetime", "value" => new \DateTime()]
    ];
}
Arzaroth commented 7 years ago

Not sure what you mean. It will behave the exact same way as before.

FlipEverything commented 7 years ago

I thinked about @tuupola's question and did not understand it either. Maybe we are missing something @Arzaroth...

tuupola commented 7 years ago

I was thinking it backwards. I ment what happens if I have entity with default value of DateTime? I have not tested it, probably unsupported even now.

public static function fields()
{
    return [
        "id" => ["type" => "integer", "unsigned" => true, "primary" => true, "autoincrement" => true],
        "created_at"   => ["type" => "datetime", "default" => new \DateTime()]
    ];
}
FlipEverything commented 7 years ago

Now that's a great question. I think it'll throw an exception during migration. It doesn't make any sense to me to use it that way. I think with DateTime() you should be okay with the value property.

nebulousGirl commented 7 years ago

Using a DateTime as a default is not ideal because each time a migration is run a new default is defined in the database. value is a better choice when a DateTime field needs a default.

FlipEverything commented 7 years ago

Thank you!