-
Notifications
You must be signed in to change notification settings - Fork 11.1k
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] Introduce method Blueprint::rawColumn()
#53496
Conversation
I think |
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.
It can simply be called column
, $table->column('name', 'bit')
.
I also suggest to accept the type
argument as string
and also Closure
. The Closure
may have Connection
as the first argument:
$table->column('name', function ($connection) {
return $connection->isMaria() ? 'mariadb_type' : 'mysql_type';
})
$table->column('name', function ($connection) {
return version_compare($connection->getServerVersion(), '>=', '3.35')
? 'new_type'
: 'old_type';
})
/** | ||
* Create a new custom column on the table. | ||
* | ||
* @param string $column | ||
* @param string $statement | ||
* @return \Illuminate\Database\Schema\ColumnDefinition | ||
*/ | ||
public function custom($column, $statement) | ||
{ | ||
return $this->addColumn('custom', $column, compact('statement')); | ||
} |
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.
The second argument is not a SQL statement, a but SQL type.
/** | |
* Create a new custom column on the table. | |
* | |
* @param string $column | |
* @param string $statement | |
* @return \Illuminate\Database\Schema\ColumnDefinition | |
*/ | |
public function custom($column, $statement) | |
{ | |
return $this->addColumn('custom', $column, compact('statement')); | |
} | |
/** | |
* Create a new custom type column on the table. | |
* | |
* @param string $column | |
* @param string|(\Closure(\Illuminate\Database\Connection): string) $type | |
* @return \Illuminate\Database\Schema\ColumnDefinition | |
*/ | |
public function column(string $column, string|Closure $type) | |
{ | |
return $this->addColumn('custom', $column, ['definition' => $type]); | |
} |
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.
Not sure I feel like the closure has an added value, perhaps Taylor might feel otherwise, still open to that.
Blueprint::custom()
Blueprint::rawColumn()
Not sure that I'm a fan of using the callback as I feel like it doesn't really have added value. The connection is already accessible via Requested changes have been applied, feel free to re-check. |
This PR introduces a new column method when creating/updating table schema.
Currently, when trying to create columns which are not natively supported by any of the grammars, we have to instead use database statements. This creates a rather big inconvenience as we cannot use a single table creation callback, because database statements are fired immediately and therefore cannot be used in
Schema::create(<table>, <callback>)
.Imagine a scenario, where we would like to create a:
posts
id
1
namedlegacy_boolean
with a default value of0
The example code to solve this would likely be as follows:
This PR introduces a new column creation method which supports directly specifiying the column type definition used for creating a column:
This however has one possible drawback - this is not grammar-agnostic and therefore would be specific to the database driver used, even though, by-default, working with columns is grammar-agnostic as the underlying column definitions are compiled by the grammar implementation used by the driver.
On the other hand, the previous solution using database statements is also not grammar-agnostic and therefore we're just moving the drawback from one place to another, meaning, this should not be an issue after all, as we were already facing it elsewhere.