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

Some errors + CI fails on Laravel 11.15 #85

Closed
damiencriado opened this issue Jul 13, 2024 · 10 comments
Closed

Some errors + CI fails on Laravel 11.15 #85

damiencriado opened this issue Jul 13, 2024 · 10 comments

Comments

@damiencriado
Copy link

I've encountered an issue with the latest version of Laravel (11.15) during migrations.

Schema::table('my_table', static function (Blueprint $table) {
    $table->decimal('my_column', 13, 4)->default(0);
    // ...
});
Duplicate column: 7 ERROR: column "my_column" of relation "my_table" already exists

The CI now fails on the latest version of Laravel: https://github.com/damiencriado/laravel-postgresql-enhanced/actions/runs/9915497290

Maybe it has something to do with this change: laravel/framework#51373

Please let me know if you need any further information to diagnose this issue.

@hafezdivandari
Copy link
Contributor

hafezdivandari commented Jul 13, 2024

Overridden compileAdd and compileChange methods must reflect the same changes as PostgresGrammar on PR laravel/framework#51373 as I explained here. Sorry for the trouble.

@tpetry
Copy link
Owner

tpetry commented Jul 13, 2024

Fixing this to work with all versions will again be tricky. I'll work on this on Monday.

@tpetry tpetry closed this as completed in ad09702 Jul 15, 2024
@tpetry
Copy link
Owner

tpetry commented Jul 15, 2024

I've fixed the CI issues with the newest Laravel 11.x release in 0.40.1.

But your initial issues of the column already existing should be an issue because of breaking change with Laravel 11.15.x behaviour. The issue should also exist when removing my package. From my understanding you need to reorder the schema changes in your migration.

@hafezdivandari
Copy link
Contributor

hafezdivandari commented Jul 15, 2024

Hi @tpetry, the problem is actually on the overriden compileAdd I think. After 11.15, we are adding columns one by one, so $command->column should be used instead of $blueprint->getAddedColumns().

@tpetry
Copy link
Owner

tpetry commented Jul 15, 2024

$blueprint->getAddedColumns() is just an alias to $command->columns with some extra logic. So it shouldnt be the problem. And others reported the same issue with your PR.

@damiencriado Can you check whether the issue remains/disappears when removing my package?

@hafezdivandari
Copy link
Contributor

hafezdivandari commented Jul 15, 2024

$blueprint->getAddedColumns() is just an alias to $command->columns

$blueprint->getAddedColumns() is an alias for $blueprint->commands (all added columns on blueprint), that should change to new $command->column (single column on the command).

Reproducing: try to add / change 2 columns:

Schema::table('table', static function (Blueprint $table) {
    $table->string('column_one');
    $table->string('column_two');
});

@tpetry
Copy link
Owner

tpetry commented Jul 15, 2024

The example by hafezdivandari shows that indeed the current version is broken because of behavioural bc break in Laravel.

@tpetry tpetry reopened this Jul 15, 2024
@damiencriado
Copy link
Author

damiencriado commented Jul 16, 2024

$blueprint->getAddedColumns() is just an alias to $command->columns with some extra logic. So it shouldnt be the problem. And others reported the same issue with your PR.

@damiencriado Can you check whether the issue remains/disappears when removing my package?

Hi @tpetry ! I still have errors in my CI with 0.40.1 :(

SQLSTATE[42701]: Duplicate column: 7 ERROR:   column "my_column" of relation "my_table" already exists

And yes, the error is only present with the package.

@tpetry
Copy link
Owner

tpetry commented Jul 16, 2024

Yeah, I have now a working test case that is failing only in that specific version. Working on a fix based on the PR from @hafezdivandari.

@tpetry tpetry closed this as completed in 0074e7e Jul 16, 2024
@tpetry
Copy link
Owner

tpetry commented Jul 16, 2024

Its now finally fixed with 0.40.2

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 a pull request may close this issue.

3 participants