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 rename/drop column commands #45258

Merged
merged 12 commits into from Dec 12, 2022
Merged

[9.x] Add support for native rename/drop column commands #45258

merged 12 commits into from Dec 12, 2022

Conversation

hafezdivandari
Copy link
Contributor

@hafezdivandari hafezdivandari commented Dec 10, 2022

Related to #45254

Currently it is required to install doctrine/dbal to be able to rename a column on all databases and drop a column on SQLite.
But all databases now have native support for renaming and dropping columns on their latest versions:

Database Supported version on Laravel Native rename column support Native drop column support
MariaDB 10.3+ 10.5.2+ (Docs)
MySQL 5.7+ 8.0+ (Docs)
PostgreSQL 10.0+ ✓ (Docs)
SQLite 3.8.8+ 3.25.0+ (Docs) (Release logs) 3.35.0+ (Docs) (Release logs)
SQL Server 2017+ ✓ (Docs)

This PR adds support for native rename column command on all databases and drop column command on SQLite without any breaking changes.

This means if a user has already installed doctrine/dbal, they see no change, but for those users with a database version that supports native rename/drop column commands (as listed on the above table) there is no need to install doctrine/dbal anymore.

But what if I have already installed doctrine/dbal and still want to use the native rename/drop column commands?
This PR adds a new static method to Schema facade, Schema::useNativeSchemaOperationsIfPossible(). For example if you have already installed doctrine/dbal library but you don't want to use Doctrine for renaming/dropping columns you may call Schema::useNativeSchemaOperationsIfPossible() method within the boot method of your App\Providers\AppServiceProvider class (we already have similar methods like this one) or maybe inside your migration files directly.

Finally, this PR causes Laravel to support native rename/drop column commands and sees the usage of doctrine\dbal as a workaround for users that prefer Doctrine or have an old DB version.

PS1: I'll send a PR to add/edit Laravel docs if it merged (laravel/docs#8396).
PS2: I'll send a PR with the same approach for changing/modifying columns if this one merged (#45487).

@hafezdivandari hafezdivandari marked this pull request as ready for review December 11, 2022 04:32
@hafezdivandari hafezdivandari changed the title [9.x] Add rename column command [9.x] Add support for native rename/drop column commands Dec 11, 2022
@driesvints
Copy link
Member

@hafezdivandari I like this PR but I wouldn't make it as complicated as to introduce Schema::useDoctrineToRenameColumns or Schema::useDoctrineToDropColumns. I'd make the behavior so it detects if DBAL is installed and use that and if not that it uses the native column renaming and dropping. In a future major version when we can drop support for older database engines we can drop DBAL for this entirely.

@hafezdivandari
Copy link
Contributor Author

hafezdivandari commented Dec 12, 2022

Thank you @driesvints, You are right, I agree that it's complicated now, but I think just checking if DBAL is installed or not is not enough, we would need some kind of config for that. do you have any suggestion?

For example we currently force user to install doctrine/dbal to be able to use artisan model:show command (Although I think it's not necessary) but when user agrees to install this package, it would cause different behavior on compiling migration commands.

@driesvints
Copy link
Member

it would cause different behavior on compiling migration commands.

Are you saying the behavior between using DBAL and native commands is different? If so then I think we might better hold off with this entire PR since it would be too much of a breaking change for many users. I don't think that the Schema::useDoctrineToDropColumns etc is the way forward.

@hafezdivandari
Copy link
Contributor Author

@driesvints it would be different behavior (breaking change) if I apply your suggestion into this PR:

I'd make the behavior so it detects if DBAL is installed and use that and if not that it uses the native column renaming and dropping.

What am I saying that is we need a switch/config instead of Schema::useDoctrineToRenameColumns or Schema::useDoctrineToDropColumns. For example I can simplify that to Schema::useDoctrine, is it better you think?

@driesvints
Copy link
Member

I guess I don't understand why that's a problem. If there's a difference in behavior then surely we can't continue with this PR? I expect nothing to change explicitly if someone installs the DBAL package other than it uses DBAL instead of native column rewrites.

@hafezdivandari
Copy link
Contributor Author

hafezdivandari commented Dec 12, 2022

@driesvints This PR, as is, is not a breaking change at all, it only uses native rename/drop if DBAL is not installed and changes nothing else.

I was explaining that it would be a breaking change IF I apply your suggestion and remove Schema::useDoctrineToRenameColumns or Schema::useDoctrineToDropColumns from this PR.

@hafezdivandari
Copy link
Contributor Author

I just merged Schema::useDoctrineToRenameColumns and Schema::useDoctrineToDropColumns into one negated method Schema::preventDoctrineUsageIfPossible.

@taylorotwell
Copy link
Member

taylorotwell commented Dec 12, 2022

@hafezdivandari but I don't think Dries or I understand WHY it is a breaking change to remove those configuration methods.

Just use Doctrine if it is installed and use the native methods if Doctrine is not installed. No configuration methods needed. Why is that a breaking change? Please explain.

@hafezdivandari
Copy link
Contributor Author

@taylorotwell I think I was wrong using the term "braking change" here, we might see that as an "unexpected behavior". Assume this scenario after this PR, for example on MySQL 8.0:

  1. I run migrations to rename a column (it compiles to native rename command as expected).
  2. After a while I install DBAL, for example to be able to use artisan model:show.
  3. I run migrations again (this time it compiles to a different query using DBAL or even an exception e.g. if there are 2 renames)
  4. I have no way to get the previous migration without that config method. Isn't this an unexpected behavior?

I added that config method on schema for user to be able to control these situations regardless of DBAL availability.

I'm sorry for the confusion, please let me know if you prefer this PR without any config method.

@driesvints
Copy link
Member

But the migrations will already have run? You won't run migrations twice.

@hafezdivandari
Copy link
Contributor Author

@driesvints I agree but you once said: #44101 (comment)

Won't this change the behavior for apps who have already run their migrations in production?
Their migrations which are part of their app will still be run in development. If they run them in development after this PR is merged, their development databases will be out if sync with production.

@driesvints
Copy link
Member

That's a good remark. But the main question here remains why a Doctrine rename would be different from a native rename. You say:

I run migrations again (this time it compiles to a different query using DBAL or even an exception e.g. if there are 2 renames)

Why would there be two renames? What exception would occur?

@taylorotwell
Copy link
Member

taylorotwell commented Dec 12, 2022

A Doctrine rename could be "different" than a native one since on SQLite I'm pretty sure Doctrine drops the column and re-adds it with a new name after storing the data in a temporary table?

The final outcome should be the same but the road taken to get there is different. Is that what you mean @hafezdivandari?

@hafezdivandari
Copy link
Contributor Author

hafezdivandari commented Dec 12, 2022

@driesvints @taylorotwell

Why would there be two renames? What exception would occur?

Doctrine can not handle multiple rename/drop, check this tests please but SQLite has no problem with multiple native rename/drop:

public function testItEnsuresDroppingMultipleColumnsIsAvailable()
{
$this->expectException(BadMethodCallException::class);
$this->expectExceptionMessage("SQLite doesn't support multiple calls to dropColumn / renameColumn in a single modification.");
$this->db->connection()->getSchemaBuilder()->table('users', function (Blueprint $table) {
$table->dropColumn('name');
$table->dropColumn('email');
});
}
public function testItEnsuresRenamingMultipleColumnsIsAvailable()
{
$this->expectException(BadMethodCallException::class);
$this->expectExceptionMessage("SQLite doesn't support multiple calls to dropColumn / renameColumn in a single modification.");
$this->db->connection()->getSchemaBuilder()->table('users', function (Blueprint $table) {
$table->renameColumn('name', 'first_name');
$table->renameColumn('name2', 'last_name');
});
}
public function testItEnsuresRenamingAndDroppingMultipleColumnsIsAvailable()
{
$this->expectException(BadMethodCallException::class);
$this->expectExceptionMessage("SQLite doesn't support multiple calls to dropColumn / renameColumn in a single modification.");
$this->db->connection()->getSchemaBuilder()->table('users', function (Blueprint $table) {
$table->dropColumn('name');
$table->renameColumn('name2', 'last_name');
});
}
public function testItEnsuresDroppingForeignKeyIsAvailable()
{
$this->expectException(BadMethodCallException::class);
$this->expectExceptionMessage("SQLite doesn't support dropping foreign keys (you would need to re-create the table).");
$this->db->connection()->getSchemaBuilder()->table('users', function (Blueprint $table) {
$table->dropForeign('something');
});
}

A Doctrine rename could be "different" than a native one since on SQLite I'm pretty sure Doctrine drops the column and re-adds it with a new name after storing the data in a temporary table?

Yes, exactly, and also on MySQL the doctrine renames column using alter table change column instead of alter table rename column that causes this issue: #45254

The final outcome should be the same but the road taken to get there is different. Is that what you mean?

The final outcome also may be different because the way we currently use DBAL is buggy: #45254

Finally is it that bad to use a single config method to control such behavior changes? We are using many config methods on Eloquent Model for example.

@taylorotwell
Copy link
Member

Renamed to useNativeSchemaOperationsIfPossible.

@taylorotwell taylorotwell merged commit 72f6e37 into laravel:9.x Dec 12, 2022
@EdgarSedov
Copy link

EdgarSedov commented Dec 16, 2022

Sorry, could u please elaborate further, what means 10.0- version for Rename column support for postgres? Does that mean what i can rename natively without doctrine only while using postgres version 10.0 and below? Kinda confused here

@hafezdivandari
Copy link
Contributor Author

hafezdivandari commented Dec 16, 2022

@EdgarSedov sorry for confusion, I wanted to declare that the PostgeSQL and SQL server support native renaming even before that version. It was just a simple comparison to current supported DB version on Laravel. It's corrected now.

As stated on the updated Laravel docs on renaming columns you won't need to install DBAL to rename a column on PostgreSQL at all.

@IP-Developer
Copy link

Will this change also fix those long-standing issues with renaming columns when an enum column is present in the table? #1186

@hafezdivandari
Copy link
Contributor Author

@IP-Developer Yes, this change fixes that issue.

Note: As pointed above, don't forget to call Schema::useNativeSchemaOperationsIfPossible() if you have doctrine/dbal in your dependencies.

@IP-Developer
Copy link

@hafezdivandari in this case I guess the same code can be reused in some other places were the enum issue still persists (like db:show command etc.)

@hafezdivandari
Copy link
Contributor Author

@IP-Developer True. I'm working on it, waiting for this to be reviewed #45598 as a prerequisite.

@IP-Developer
Copy link

@hafezdivandari those are good news for the Laravel community :) Thanks for your work

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.

None yet

5 participants