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

[9.x] Add support for native column modifying #45281

Closed
wants to merge 19 commits into from
Closed
Show file tree
Hide file tree
Changes from 18 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 16 additions & 9 deletions src/Illuminate/Database/Schema/Blueprint.php
Expand Up @@ -129,6 +129,15 @@ public function toSql(Connection $connection, Grammar $grammar)
$this->ensureCommandsAreValid($connection);

foreach ($this->commands as $command) {
if (in_array($command->name, $grammar->getFluentCommands())
&& (! $command->column->change && ! isset($command->column->{lcfirst($command->name)})
|| ($command->column->change && ! $connection->usingNativeSchemaOperations()))) {
// Doctrine DBAL handles fluent commands on change. Also, there is no need to
// compile them when creating or adding a column when its value is not set
// but we should always compile them when using native column modifying.
continue;
}

$method = 'compile'.ucfirst($command->name);

if (method_exists($grammar, $method) || $grammar::hasMacro($method)) {
Expand Down Expand Up @@ -188,6 +197,10 @@ protected function commandsNamed(array $names)
*/
protected function addImpliedCommands(Grammar $grammar)
{
if ($this->hasAutoIncrementColumn() && ! $this->creating()) {
array_unshift($this->commands, $this->createCommand('autoIncrementStartingValues'));
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please explain this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's to handle auto-increment starting value using from modifier, before this PR from was only supported when adding a column and creating a table. I removed this from compileAdd and add it here to handle both add and change. now it is supported to use from modifier with change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it's much cleaner to handle auto-increment starting value as a fluent command for MySQL and PostgreSQL. but I didn't want to change a lot of code at once and make it hard to review.

if (count($this->getAddedColumns()) > 0 && ! $this->creating()) {
array_unshift($this->commands, $this->createCommand('add'));
}
Expand Down Expand Up @@ -243,13 +256,7 @@ public function addFluentCommands(Grammar $grammar)
{
foreach ($this->columns as $column) {
foreach ($grammar->getFluentCommands() as $commandName) {
$attributeName = lcfirst($commandName);

if (! isset($column->{$attributeName})) {
continue;
}

$value = $column->{$attributeName};
$value = $column->{lcfirst($commandName)};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please explain the reason for this change. Seems breaking.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not breaking, many tests would fail if it was. I removed this if condition from here and added to L132-L140 of this file. Because unfortunately this method doesn't have $connection argument. I didn't want to change the signature of this method (I'm not quit sure if passing the $connection to this method and changing it's signature is a breaking change so I didn't do that). Please let me know if the comment on L135-L137 that explains why this change is needed is clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TLDR; it's removed from here and added (with more conditions) to its parent method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's for handling comment on PostgreSQL and default on SQL Server`.


$this->addCommand(
$commandName, compact('value', 'column')
Expand Down Expand Up @@ -1794,7 +1801,7 @@ public function getChangedColumns()
*/
public function hasAutoIncrementColumn()
{
return ! is_null(collect($this->getAddedColumns())->first(function ($column) {
return ! is_null(collect($this->columns)->first(function ($column) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why no longer using getAddedColumns?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It now supports both added and modified columns (looking for from modifier on all columns). $this->columns = $this->getAddedColumns() + $this->getChangedColumns()

return $column->autoIncrement === true;
}));
}
Expand All @@ -1810,7 +1817,7 @@ public function autoIncrementingStartingValues()
return [];
}

return collect($this->getAddedColumns())->mapWithKeys(function ($column) {
return collect($this->columns)->mapWithKeys(function ($column) {
return $column->autoIncrement === true
? [$column->name => $column->get('startingValue', $column->get('from'))]
: [$column->name => null];
Expand Down
4 changes: 2 additions & 2 deletions src/Illuminate/Database/Schema/Grammars/Grammar.php
Expand Up @@ -77,7 +77,7 @@ public function compileRenameColumn(Blueprint $blueprint, Fluent $command, Conne
* @param \Illuminate\Database\Schema\Blueprint $blueprint
* @param \Illuminate\Support\Fluent $command
* @param \Illuminate\Database\Connection $connection
* @return array
* @return array|string
*
* @throws \RuntimeException
*/
Expand Down Expand Up @@ -155,7 +155,7 @@ public function compileForeign(Blueprint $blueprint, Fluent $command)
}

/**
* Compile the blueprint's column definitions.
* Compile the blueprint's added column definitions.
*
* @param \Illuminate\Database\Schema\Blueprint $blueprint
* @return array
Expand Down
78 changes: 63 additions & 15 deletions src/Illuminate/Database/Schema/Grammars/MySqlGrammar.php
Expand Up @@ -3,6 +3,7 @@
namespace Illuminate\Database\Schema\Grammars;

use Illuminate\Database\Connection;
use Illuminate\Database\Query\Expression;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Fluent;
use RuntimeException;
Expand All @@ -16,7 +17,7 @@ class MySqlGrammar extends Grammar
*/
protected $modifiers = [
'Unsigned', 'Charset', 'Collate', 'VirtualAs', 'StoredAs', 'Nullable', 'Invisible',
'Srid', 'Default', 'Increment', 'Comment', 'After', 'First',
'Srid', 'Default', 'OnUpdate', 'Increment', 'Comment', 'After', 'First',
];

/**
Expand Down Expand Up @@ -178,16 +179,13 @@ protected function compileCreateEngine($sql, Connection $connection, Blueprint $
*
* @param \Illuminate\Database\Schema\Blueprint $blueprint
* @param \Illuminate\Support\Fluent $command
* @return array
* @return string
*/
public function compileAdd(Blueprint $blueprint, Fluent $command)
{
$columns = $this->prefixArray('add', $this->getColumns($blueprint));

return array_values(array_merge(
['alter table '.$this->wrapTable($blueprint).' '.implode(', ', $columns)],
$this->compileAutoIncrementStartingValues($blueprint)
));
return 'alter table '.$this->wrapTable($blueprint).' '.implode(', ', $columns);
}

/**
Expand All @@ -200,7 +198,7 @@ public function compileAutoIncrementStartingValues(Blueprint $blueprint)
{
return collect($blueprint->autoIncrementingStartingValues())->map(function ($value, $column) use ($blueprint) {
return 'alter table '.$this->wrapTable($blueprint->getTable()).' auto_increment = '.$value;
})->all();
})->values()->all();
}

/**
Expand All @@ -222,6 +220,34 @@ public function compileRenameColumn(Blueprint $blueprint, Fluent $command, Conne
: parent::compileRenameColumn($blueprint, $command, $connection);
}

/**
* Compile a change column command into a series of SQL statements.
*
* @param \Illuminate\Database\Schema\Blueprint $blueprint
* @param \Illuminate\Support\Fluent $command
* @param \Illuminate\Database\Connection $connection
* @return array|string
*
* @throws \RuntimeException
*/
public function compileChange(Blueprint $blueprint, Fluent $command, Connection $connection)
{
if ($connection->usingNativeSchemaOperations()) {
$columns = [];

foreach ($blueprint->getChangedColumns() as $column) {
$sql = $this->wrap($column).' '.$this->getType($column);

$columns[] = $this->addModifiers($sql, $blueprint, $column);
}

return 'alter table '.$this->wrapTable($blueprint).' '
.implode(', ', $this->prefixArray('modify', $columns));
}

return parent::compileChange($blueprint, $command, $connection);
}

/**
* Compile a primary key command.
*
Expand Down Expand Up @@ -758,13 +784,17 @@ protected function typeDate(Fluent $column)
*/
protected function typeDateTime(Fluent $column)
{
$columnType = $column->precision ? "datetime($column->precision)" : 'datetime';

$current = $column->precision ? "CURRENT_TIMESTAMP($column->precision)" : 'CURRENT_TIMESTAMP';

$columnType = $column->useCurrent ? "$columnType default $current" : $columnType;
if ($column->useCurrent) {
$column->default(new Expression($current));
}

if ($column->useCurrentOnUpdate) {
$column->onUpdate(new Expression($current));
}

return $column->useCurrentOnUpdate ? "$columnType on update $current" : $columnType;
return $column->precision ? "datetime($column->precision)" : 'datetime';
}

/**
Expand Down Expand Up @@ -808,13 +838,17 @@ protected function typeTimeTz(Fluent $column)
*/
protected function typeTimestamp(Fluent $column)
{
$columnType = $column->precision ? "timestamp($column->precision)" : 'timestamp';

$current = $column->precision ? "CURRENT_TIMESTAMP($column->precision)" : 'CURRENT_TIMESTAMP';

$columnType = $column->useCurrent ? "$columnType default $current" : $columnType;
if ($column->useCurrent) {
$column->default(new Expression($current));
}

if ($column->useCurrentOnUpdate) {
$column->onUpdate(new Expression($current));
}

return $column->useCurrentOnUpdate ? "$columnType on update $current" : $columnType;
return $column->precision ? "timestamp($column->precision)" : 'timestamp';
}

/**
Expand Down Expand Up @@ -1175,6 +1209,20 @@ protected function modifyComment(Blueprint $blueprint, Fluent $column)
}
}

/**
* Get the SQL for an "on update" column modifier.
*
* @param \Illuminate\Database\Schema\Blueprint $blueprint
* @param \Illuminate\Support\Fluent $column
* @return string|null
*/
protected function modifyOnUpdate(Blueprint $blueprint, Fluent $column)
{
if (! is_null($column->onUpdate)) {
return ' on update '.$column->onUpdate;
}
}

/**
* Get the SQL for a SRID column modifier.
*
Expand Down