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

[5.6] Better enumeration columns support #22109

Merged
merged 1 commit into from
Nov 22, 2017

Conversation

paulofreitas
Copy link
Contributor

@paulofreitas paulofreitas commented Nov 17, 2017

TL;DR

  • Currently we have no true support for enum() columns when using SQLite and SQL Server although they support CHECK constraints like PostgreSQL

  • This PR adds this missing functionality to both SQLite and SQL Server so they can have true enumeration columns support that actually work as enumeration columns and validates their stored values

Proposal

Currently we have true enumeration columns support for MySQL and PostgreSQL. MySQL implementation is using the native non-standard ENUM data type while PostgreSQL implementation is using a VARCHAR data type with a CHECK constraint - PostgreSQL does have a native enumerated data type but they both work and it's easier to implement and sometimes preferable to use constraints. They're well implemented.

Since CHECK constraints are part of SQL-92 standard (ANSI X3.135-1992 and ISO/IEC 9075:1992) and both SQLite and SQL Server does support them, I'm proposing true support for enumerated columns for all database drivers.

Implementation

Both SQLite and SQL Server would be using the same implementation being used in PostgreSQL: a VARCHAR compatible data type with a CHECK constraint. SQL Server would be using NVARCHAR(255) while SQLite just VARCHAR since it does not have support for column length.

As a proof-of-concept that it works, here are the SQL fiddles demonstrating them in action:

Changes

I'm targeting this for 5.5 first just because I'm not sure where it would be desirable since it's a feature that would be applied only to new installations and just adds a non-existing behaviour that matches the usage expectation. If this would be acceptable just for 5.6, let me know and I'll rebase it accordingly ASAP. 😉

Code
  • Illuminate\Database\Grammar
    • Added a new quote() helper method to reuse within concrete grammars.
  • Illuminate\Database\Schema\Grammars\MySqlGrammar
  • Illuminate\Database\Schema\Grammars\PostgresGrammar
    • Changed typeEnum() method to use the new quote() method, minor code readability improvements.
  • Illuminate\Database\Schema\Grammars\SQLiteGrammar
  • Illuminate\Database\Schema\Grammars\SqlServerGrammar
    • Changed typeEnum() method to include a new CHECK constraint.
Tests
  • Illuminate\Tests\Database\DatabaseMySqlSchemaGrammarTest
  • Illuminate\Tests\Database\DatabasePostgresSchemaGrammarTest
  • Illuminate\Tests\Database\DatabaseSQLiteSchemaGrammarTest
  • Illuminate\Tests\Database\DatabaseSqlServerSchemaGrammarTest
    • Small tweaks and changes to make tests pass.

@sisve
Copy link
Contributor

sisve commented Nov 17, 2017

... that would be applied only to new installations ...

What? What is a new installation? Could you explain this scenario?

As I've mentioned previously, people execute old migrations when setting up new homestead environments. Imagine having a new employee or that you just ragequit on your old homestead box and needs to regenerate the database. The generated dev database should still match the production database.

@deleugpn
Copy link
Contributor

@sisve describes a scenario where, on a point release, you composer update and php artisan migrate would get different result than a production environment, therefore breaking change. I've been always on the fence about using enum, but I was quite sad that Laravel was just aliasing that. Would love to see this, but at the same time I emphasize with the fact that migrations is a very delicate area of the framework for breaking changes.

@paulofreitas
Copy link
Contributor Author

paulofreitas commented Nov 17, 2017

I really don't bother pushing it to 5.6 - I'm ready to change the base branch quickly. I'm targeting these proposals for 5.5 first just to avoid having to postpone anything that might be acceptable before. 👍

For any proposal that would be acceptable, just let me know the desired branch to push and I'll make it accordingly ASAP. 😉

@paulofreitas paulofreitas changed the title [5.5] Complete enumeration columns support [5.5] Full enumeration columns support Nov 18, 2017
@taylorotwell
Copy link
Member

I do prefer to move this to 5.6.

@taylorotwell
Copy link
Member

Once the branch is updated to 5.6 I don't think I see any big blockers to this being merged.

@paulofreitas paulofreitas changed the base branch from 5.5 to master November 21, 2017 16:06
@paulofreitas paulofreitas changed the title [5.5] Full enumeration columns support [5.6] Full enumeration columns support Nov 21, 2017
@paulofreitas
Copy link
Contributor Author

@taylorotwell Thanks for guidance, I just completed the branch change. 😉

@taylorotwell taylorotwell merged commit d698976 into laravel:master Nov 22, 2017
@paulofreitas paulofreitas deleted the full-enum-support branch November 22, 2017 17:18
@paulofreitas paulofreitas changed the title [5.6] Full enumeration columns support [5.6] Better enumeration columns support Nov 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants