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

Add support for specifying a nullable field #203

Closed
josegonzalez opened this issue Mar 1, 2016 · 16 comments
Closed

Add support for specifying a nullable field #203

josegonzalez opened this issue Mar 1, 2016 · 16 comments
Milestone

Comments

@josegonzalez
Copy link
Member

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
Copy link
Member

What about [null] next to the field type ?

email:string[null]
email:string[null]:unique
email:string[120][null]:unique:EMAIL_INDEX

@josegonzalez
Copy link
Member Author

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
Copy link
Member

It might be doable with a sophisticated enough regex.

@HavokInspiration
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

What about:

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

?

@HavokInspiration
Copy link
Member

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

@josegonzalez
Copy link
Member Author

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

@lorenzo
Copy link
Member

lorenzo commented Mar 3, 2016

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
Copy link
Member

@lorenzo I like the idea.

@nitincoded
Copy link
Contributor

nitincoded commented Jun 22, 2016

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
Copy link
Member

lorenzo commented Jun 22, 2016

great news @nitincoded !

@dereuromark
Copy link
Member

You are probably referring to 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
Copy link
Contributor

@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:
nitincoded@bd453ee

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
Copy link
Member

PR #249 opened to add the feature.

dereuromark added a commit that referenced this issue Jun 23, 2016
Add support for specifying a nullable field #203
nitincoded added a commit to nitincoded/docs that referenced this issue Jun 23, 2016
This feature is being introduced in cakephp/migrations v1.6.3.

Reference: Issue cakephp/migrations#203 / PR cakephp/migrations#249
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

5 participants