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

Maria DB before 10.5.2 rename nullable column fails #51171

Closed
dav245 opened this issue Apr 22, 2024 · 5 comments · Fixed by #51177
Closed

Maria DB before 10.5.2 rename nullable column fails #51171

dav245 opened this issue Apr 22, 2024 · 5 comments · Fixed by #51177

Comments

@dav245
Copy link

dav245 commented Apr 22, 2024

Laravel Version

11

PHP Version

8.3

Database Driver & Version

Maria before 10.5.2 (10.3.39 in our case)

Description

While trying to rename nullable numeric column, migration fails. It tries to gather data from existing schema as seen in Illuminate\Database\Schema\Grammars@343 (function compileLegacyRenameColumn). On line 357 it checks if it has default timestamp, which in our case had not. So $column['default'] was used as is. But schema returned default value as 'NULL'. Since it is string it tried to set default value as 'NULL' for numeric column thus crashing.

    protected function compileLegacyRenameColumn(Blueprint $blueprint, Fluent $command, Connection $connection)
    {
/*343*/ $column = collect($connection->getSchemaBuilder()->getColumns($blueprint->getTable()))
            ->firstWhere('name', $command->from);

        $modifiers = $this->addModifiers($column['type'], $blueprint, new ColumnDefinition([
            'change' => true,
            'type' => match ($column['type_name']) {
                'bigint' => 'bigInteger',
                'int' => 'integer',
                'mediumint' => 'mediumInteger',
                'smallint' => 'smallInteger',
                'tinyint' => 'tinyInteger',
                default => $column['type_name'],
            },
/*357*/     'nullable' => $column['nullable'],
            'default' => $column['default'] && str_starts_with(strtolower($column['default']), 'current_timestamp') 
                ? new Expression($column['default'])
                : $column['default'],
            'autoIncrement' => $column['auto_increment'],
            'collation' => $column['collation'],
            'comment' => $column['comment'],
            'virtualAs' => ! is_null($column['generation']) && $column['generation']['type'] === 'virtual'
                ? $column['generation']['expression'] : null,
            'storedAs' => ! is_null($column['generation']) && $column['generation']['type'] === 'stored'
                ? $column['generation']['expression'] : null,
        ]));

        return sprintf('alter table %s change %s %s %s',
            $this->wrapTable($blueprint),
            $this->wrap($command->from),
            $this->wrap($command->to),
            $modifiers
        );
    }

Our solution was to patch
'default' => $column['default'] && str_starts_with(strtolower($column['default']), 'current_timestamp')
to
'default' => (($column['default'] && str_starts_with(strtolower($column['default']), 'current_timestamp')) || ($column['default'] === 'NULL'))

Afterwards migration succeeded.

I honestly don't know if the problem lies in schema or in unpatched check. So this is as much as i can do.

Steps To Reproduce

Rename numeric nullable column in maria before version 10.5.2

@driesvints
Copy link
Member

Are you using the dedicated mariadb connection in Laravel v11? https://github.com/laravel/laravel/blob/11.x/config/database.php#L62

@dav245
Copy link
Author

dav245 commented Apr 22, 2024

Yes, forgot to mention that. Sorry

@driesvints
Copy link
Member

@hafezdivandari do you reckon this could be an easy fix?

@hafezdivandari
Copy link
Contributor

@driesvints sure, I'll send a PR.

@dav245
Copy link
Author

dav245 commented Apr 22, 2024

Hats off. That was quick. Thank you all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants