Skip to content

[5.6] Better enumeration columns support - #22109

Merged
taylorotwell merged 1 commit into
laravel:masterfrom
paulofreitas:full-enum-support
Nov 22, 2017
Merged

[5.6] Better enumeration columns support#22109
taylorotwell merged 1 commit into
laravel:masterfrom
paulofreitas:full-enum-support

Conversation

@paulofreitas

@paulofreitas paulofreitas commented Nov 17, 2017

Copy link
Copy Markdown
Contributor

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

sisve commented Nov 17, 2017

Copy link
Copy Markdown
Contributor

... 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
Copy Markdown
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

paulofreitas commented Nov 17, 2017

Copy link
Copy Markdown
Contributor Author

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

I do prefer to move this to 5.6.

@taylorotwell

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