cakephp / migrations

CakePHP database migrations plugin
Other
137 stars 116 forks source link

Add support for specifying a nullable field #203

Closed josegonzalez closed 8 years ago

josegonzalez commented 8 years ago

Currently a user has to go in and manually update a migration if they want to allow the field to be nulled out, which is a bit annoying. It would be nice to be able to specify a nullable field, similar to how we do size.

Does anyone have any ideas around syntax for this?

HavokInspiration commented 8 years ago

What about [null] next to the field type ?

email:string[null]
email:string[null]:unique
email:string[120][null]:unique:EMAIL_INDEX
josegonzalez commented 8 years ago

What about text fields? They don't have a length, so a dumb user might do:

content:text[null]

And we would maybe not have any idea what they are talking about.

HavokInspiration commented 8 years ago

It might be doable with a sophisticated enough regex.

HavokInspiration commented 8 years ago

Something like this one :

^(\w*)(?::(\w*\[?(\d+|\bnull\b)\]?(\[?\bnull\b\]?)?))?(?::(\w*))?(?::(\w*))?

See those tests :

https://regex101.com/r/oK0eS0/2 https://regex101.com/r/oK0eS0/3 https://regex101.com/r/oK0eS0/4

josegonzalez commented 8 years ago

Alright cool, if you think this works, +1. I am not crazy about the syntax, but it's better than nothing.

What about default values for a field? Ideas on that?

HavokInspiration commented 8 years ago

Mmm, if we throw in default values, we need a more explicit syntax than we already have. Something more descriptive, closer to an array.

Something like :

email:string:[null=true:default="derp":length=128]:unique:EMAIL_UNIQUE

This way we can have whatever properties we need and adding them should not be that much hassle once the syntax is set. I however have no idea if that can be implemented and be correctly parsed. This would need some serious testing.

What do you think ? Do you have another idea ?

josegonzalez commented 8 years ago

What about:

email:string[null]:unique:EMAIL_UNIQUE
email:string[LENGTH]:unique:EMAIL_UNIQUE
email:string[default=derp]:unique:EMAIL_UNIQUE

?

HavokInspiration commented 8 years ago

There would be no way to specify all of that in one statement ?

josegonzalez commented 8 years ago

I guess you could, but ideally we also support simpler methods

lorenzo commented 8 years ago

A quick way of supporting nullable is doing something like this:

email:string? or email:?string

That emulates the nullable type hints of some languages, like hacklang

HavokInspiration commented 8 years ago

@lorenzo I like the idea.

nitincoded commented 8 years ago

I've worked on a quick patch that served my need recently. Being a former C# and Java developer, my thought aligned with @lorenzo 's suggested syntax

Example:

bin/cake bake migration CreatePerson name:string description:string? age:integer?

I've put up my fork on GitHub at nitincoded/migrations

I would love to be able to bring the changes into cakephp/migrations and I'm currently working on building the CI test cases.

lorenzo commented 8 years ago

great news @nitincoded !

dereuromark commented 8 years ago

You are probably referring to https://github.com/cakephp/migrations/compare/master...nitincoded:patch-1 - you can directly link to your changes as diff this way to make it easier to communicate what you did.

nitincoded commented 8 years ago

@dereuromark : Thank you for the heads up. This is my first go at contributing to a project on GitHub.

The diff of the change is at: https://github.com/nitincoded/migrations/commit/bd453eee3817a6c55d1c4f56c68088041fff84b3

I also added a test for nullable columns to the PHPUnit test case, and submitted a pull request bearing the same name as this GitHub issue.

HavokInspiration commented 8 years ago

PR #249 opened to add the feature.