-
Notifications
You must be signed in to change notification settings - Fork 10.7k
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
[11.x] SQLite: Add support for strict tables #51413
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may add public $strict = false;
property to the Blueprint
class instead.
But What's the point of implementing strict
mode when most of the current column types are not compatible and won't work with strict mode enabled? According to docs:
The column datatype must be one of following:
* INT
* INTEGER
* REAL
* TEXT
* BLOB
* ANY
No other datatype names are allowed, though new types might be added in future releases of SQLite.
$blueprint->temporary ? 'create temporary' : 'create', | ||
$this->wrapTable($blueprint), | ||
implode(', ', $this->getColumns($blueprint)), | ||
$this->addForeignKeys($this->getCommandsByName($blueprint, 'foreign')), | ||
$this->addPrimaryKeys($this->getCommandByName($blueprint, 'primary')) | ||
$this->addPrimaryKeys($this->getCommandByName($blueprint, 'primary')), | ||
$this->getCommandByName($blueprint, 'strict') ? ' strict' : '', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$this->getCommandByName($blueprint, 'strict') ? ' strict' : '', | |
$blueprint->strict ? ' strict' : '', |
*/ | ||
public function strict() | ||
{ | ||
return $this->addCommand('strict'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return $this->addCommand('strict'); | |
$this->strict = true; |
@hafezdivandari r.e. the column datatypes – creating a So if you try to use |
@ryangjchandler I mean most of the current types are considered invalid, even the query on your added example test will throw an exception! If we want to implement strict mode, we should use strict types only, please check: framework/src/Illuminate/Database/Schema/Grammars/SQLiteGrammar.php Lines 597 to 959 in d4ce2a5
|
Thanks for your pull request to Laravel! Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include. If possible, please consider releasing your code as a package so that the community can still take advantage of your contributions! If you feel absolutely certain that this code corrects a bug in the framework, please "@" mention me in a follow-up comment with further explanation so that GitHub will send me a notification of your response. |
This pull request adds support for strict tables in SQLite.
I think this is a worthy addition to the framework, but I'm not entirely sure I've gone the right way about doing it.
a) It feels weird having a method specific to this one SQLite feature on the
Blueprint
class.b) Should this be something that you call inside of the
Schema::create()
callback?I considered adding a specific
createStrict()
method, but that doesn't feel like a "Laravel-esque" thing to do?I also thought about making
Schema::create(...)->strict()
, but that would be a bit of a refactor since theSchema::create()
method returnsvoid
atm.I'm not sure. Some opinions and thoughts would be much appreciated.