Skip to content

Conversation

@lucacri
Copy link

@lucacri lucacri commented Feb 7, 2024

Changes In Code

I added a check to see if the statement is ending with a semicolon. If not, it'll add it so that the .sql file generated is going to work also from the cockroach sql command

@lucacri lucacri requested a review from peterfox as a code owner February 7, 2024 20:47

if ($statements !== []) {
$file .= PHP_EOL . $statements[0]['query'];
$file .= PHP_EOL . $statements[0]['query'] .(ends_with(trim($statements[0]['query']), ';') ? '' : ';');
Copy link
Collaborator

Choose a reason for hiding this comment

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

ends_with will never work as the helper isn't installed in all applications with Laravel.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
$file .= PHP_EOL . $statements[0]['query'] .(ends_with(trim($statements[0]['query']), ';') ? '' : ';');
$file .= PHP_EOL . Str::finish($statements[0]['query'], ';');

And import use Illuminate\Support\Str;

Copy link
Collaborator

@peterfox peterfox left a comment

Choose a reason for hiding this comment

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

I'm not sure how this didn't come up in development. Could you share an example file output with the problem and what version of Laravel/CockroachDB you're using?

lucacri and others added 2 commits April 24, 2024 14:01
Add illuminate/contracts ^12.0 constraint to enable compatibility with
Laravel 12.x framework while maintaining backward compatibility with
Laravel 9, 10, and 11.

This preserves the custom semicolon fix for schema dumps (commit a94c0d4)
which is not present in upstream ylsideas/cockroachdb-laravel.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
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.

2 participants