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

adding TYPE_ENUM to Phalcon\Db\Column #11368

Merged
merged 3 commits into from Oct 21, 2018

Conversation

Projects
10 participants
@mattvb91
Copy link
Contributor

mattvb91 commented Feb 4, 2016

PR to devtools will follow

@mattvb91 mattvb91 force-pushed the mattvb91:enum_type branch from 3277e47 to 8427e0b Feb 4, 2016

@sergeyklay

This comment has been minimized.

Copy link
Member

sergeyklay commented Feb 4, 2016

Hi @mattvb91
What about other databases?
If someone moves from MySQL to Postgresql what would happen with those enum types.

In my opinion this PR this is not completed.

We have to support that type virtually for the other database engines supported.

@mattvb91

This comment has been minimized.

Copy link
Contributor Author

mattvb91 commented Feb 4, 2016

@sergeyklay do you mean just defaulting it to something like varchar for all the other adapters?

@sergeyklay

This comment has been minimized.

Copy link
Member

sergeyklay commented Feb 4, 2016

As I said above we have to support that type for the other database. For example we can transparent convert from Myslq's enum to Postgres' varchar. I'm not saying what you need to do exactly. I only express my opinion. Perhaps you'll propose a better idea

@mattvb91

This comment has been minimized.

Copy link
Contributor Author

mattvb91 commented Feb 4, 2016

@sergeyklay all the adapters default to either varchar or text if a column type isn't supported which in this case would still be ok as the result would still allow insertion of the data into the column.

@sergeyklay

This comment has been minimized.

Copy link
Member

sergeyklay commented Feb 4, 2016

And back?

@mattvb91

This comment has been minimized.

Copy link
Contributor Author

mattvb91 commented Feb 4, 2016

Im not sure if im missing something in your question but if porting data back INTO mysql from a data source not supporting enum it would also default the column to varchar.

@Dublerq

This comment has been minimized.

Copy link

Dublerq commented Mar 10, 2016

Please consider merging this pull request to 2.x. We're waiting for enums since 2014

@sergeyklay

This comment has been minimized.

Copy link
Member

sergeyklay commented Mar 10, 2016

@mattvb91 Could you please rebase?

if preg_match(sizePattern, columnType, matches) {

if definition["type"] == Column::TYPE_ENUM {
let definition["size"] = substr(columnType, 5, -1);

This comment has been minimized.

@sergeyklay

sergeyklay Mar 10, 2016

Member

@mattvb91 Could you please explain this a bit more?

This comment has been minimized.

@mattvb91

mattvb91 Mar 11, 2016

Author Contributor

@sergeyklay this is where we were trying to use a regex but its easier to simply get the contents out of enum(CONTENTS) by doing a substr select.

So essentially if you have ENUM('val1', 'val2', 'val3') then definition["size"] = "'val1', 'val2', 'val3'"

Does that make more sense?

@sergeyklay

This comment has been minimized.

Copy link
Member

sergeyklay commented Jun 24, 2016

@sergeyklay

This comment has been minimized.

Copy link
Member

sergeyklay commented Jun 24, 2016

@dcosta-ptc

This comment has been minimized.

Copy link

dcosta-ptc commented Aug 16, 2016

Any news regarding this topic?

@sergeyklay sergeyklay added this to the 3.1.0 milestone Aug 16, 2016

@sergeyklay sergeyklay self-assigned this Aug 16, 2016

@sergeyklay

This comment has been minimized.

Copy link
Member

sergeyklay commented Aug 16, 2016

@dcosta-ptc Will be part of 3.1.x

@Dublerq

This comment has been minimized.

Copy link

Dublerq commented Aug 16, 2016

I need to find some time to configure zephir and learn how to test/contribute. Looks like I'll need to speed up needed features development myself because waiting more than half a year to get a simple feature or fix is a bit too long.

@sergeyklay

This comment has been minimized.

Copy link
Member

sergeyklay commented Aug 16, 2016

@Dublerq sounds fine!

@ruudboon ruudboon referenced this pull request Oct 5, 2016

Closed

Added Column::TYPE_TIME #12293

@mattvb91 mattvb91 force-pushed the mattvb91:enum_type branch from 8427e0b to b07146c Oct 8, 2016

@mattvb91 mattvb91 changed the base branch from 2.1.x to 3.0.x Oct 8, 2016

@mattvb91 mattvb91 changed the base branch from 3.0.x to master Oct 8, 2016

@mattvb91 mattvb91 force-pushed the mattvb91:enum_type branch from b07146c to e3acbcf Oct 8, 2016

@mattvb91 mattvb91 changed the base branch from master to 3.0.x Oct 8, 2016

@sergeyklay sergeyklay changed the base branch from 3.0.x to 3.1.x Oct 15, 2016

@sergeyklay sergeyklay modified the milestones: 4.0.0, 3.1.0 Jan 4, 2017

@sergeyklay sergeyklay force-pushed the phalcon:3.1.x branch 9 times, most recently from 4d3c73f to 6ef079c Apr 3, 2017

@sjinks sjinks closed this Apr 26, 2017

@sjinks sjinks reopened this Apr 26, 2017

@denkurbatov

This comment has been minimized.

Copy link

denkurbatov commented Jun 16, 2017

And what now with ENUM ? I use Phalcon 3.1.2 and have problems with ENUM migrations.

@mattvb91 mattvb91 force-pushed the mattvb91:enum_type branch from e3acbcf to 1bba50d Aug 5, 2017

@mattvb91 mattvb91 changed the base branch from 3.1.x to 4.0.x Aug 5, 2017

@mattvb91 mattvb91 force-pushed the mattvb91:enum_type branch from 1bba50d to 6847fcc Aug 5, 2017

@mattvb91

This comment has been minimized.

Copy link
Contributor Author

mattvb91 commented Aug 5, 2017

@sergeyklay @dcosta-ptc @Dublerq sorry guys im not sure why im only now receiving notifications about this... I've rebased the branch and repushed. Looks like your builds are failing?

Surprised this hadn't been merged 2 years ago as its a pretty minor change from code side.

@sergeyklay

This comment has been minimized.

Copy link
Member

sergeyklay commented Aug 6, 2017

@mattvb91 I'll take a look a bit later. Thank you for contributing.

@sergeyklay sergeyklay force-pushed the phalcon:4.0.x branch from 4a7aa3c to 44ce3c6 Aug 13, 2017

@jesugmz

This comment has been minimized.

Copy link

jesugmz commented Sep 6, 2017

3.2.1 and problems too. Any plan when it could be?

@aat2703

This comment has been minimized.

Copy link

aat2703 commented Sep 6, 2017

4.0.0 according to Sergey... Not usable without ENUM type for me... sadly... Migrations are pretty rough atm...

@ruudboon

This comment has been minimized.

Copy link
Member

ruudboon commented Sep 7, 2017

Hoping that this and some other types like TIME will get in at 4.0

@niden

This comment has been minimized.

Copy link
Member

niden commented Oct 19, 2018

@mattvb91 Could you be so kind as to resolve the conflict with the changelog and have a look at the tests? It appears there is a syntax error somewhere. After that I can merge this.

I will need to add more tests on this one

@niden niden merged commit 6847fcc into phalcon:4.0.x Oct 21, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@niden

This comment has been minimized.

Copy link
Member

niden commented Oct 21, 2018

Thank you!

@niden niden added this to Done in 4.0 Release Dec 7, 2018

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