[9.x] Add support for native column modifying#45281
[9.x] Add support for native column modifying#45281hafezdivandari wants to merge 19 commits intolaravel:9.xfrom hafezdivandari:9.x
Conversation
It's it a pretty big breaking change sense i would assume that just changing the coumn type would keep any other attributes as it currently does when using doctrine? |
|
@morloderex It won't be a breaking change because it only applies to those who doesn't use DBAL currently or has called |
@hafezdivandari okay, but what would happen if I already have doctrine installed and then I decide to drop it from my project. Isn't it a breaking change in that regard or would that be considered unexpected behavior? |
|
@morloderex it's not a breaking change nor an unexpected behavior in my opinion, you would expect a change when you add/drop a package. if you want to see an exception when DBAL is not available, like before this PR, you may call |
| } | ||
|
|
||
| $value = $column->{$attributeName}; | ||
| $value = $column->{lcfirst($commandName)}; |
There was a problem hiding this comment.
Please explain the reason for this change. Seems breaking.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
TLDR; it's removed from here and added (with more conditions) to its parent method.
There was a problem hiding this comment.
It's for handling comment on PostgreSQL and default on SQL Server`.
| if ($this->hasAutoIncrementColumn() && ! $this->creating()) { | ||
| array_unshift($this->commands, $this->createCommand('autoIncrementStartingValues')); | ||
| } | ||
|
|
There was a problem hiding this comment.
Please explain this change.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| public function hasAutoIncrementColumn() | ||
| { | ||
| return ! is_null(collect($this->getAddedColumns())->first(function ($column) { | ||
| return ! is_null(collect($this->columns)->first(function ($column) { |
There was a problem hiding this comment.
Why no longer using getAddedColumns?
There was a problem hiding this comment.
It now supports both added and modified columns (looking for from modifier on all columns). $this->columns = $this->getAddedColumns() + $this->getChangedColumns()
| $column->default(new Expression('CURRENT_TIMESTAMP')); | ||
| } | ||
|
|
||
| return 'datetime'; |
There was a problem hiding this comment.
Please explain need for this change.
There was a problem hiding this comment.
to be consistent with other Grammars, I changed SQLite too to use the same syntax for handling useCurrent.
There was a problem hiding this comment.
there are 2 different statements to change column type and default value on PostgreSQL and SQL server.
|
@taylorotwell I think it is much cleaner to send this PR to master instead, the |
|
Sure, you can send a single PR to master. |
3 participants
Moved to 10.x