Skip to content

Fix migrations when database does not exist #412

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

Closed
wants to merge 1 commit into from

Conversation

miagg
Copy link
Contributor

@miagg miagg commented Nov 14, 2024

My previous PR #408 fixed native:migrate:fresh but only when database already existed. 😢
When database is missing, it does not migrate at all.

It seems that the Artisan::call is not needed at all during the database creation since the parent handler will migrate the database anyway.

Please check if I'm right before merging. It did solve the problem for me but I wouldn't want to break something.

Sorry for the confusion.

@simonhamp
Copy link
Member

When database is missing, it does not migrate at all.

That makes sense to me - it can't migrate a database that doesn't exist

Does Laravel's own migrate:fresh create the database?

the parent handler will migrate the database anyway

Which parent handler?

@miagg
Copy link
Contributor Author

miagg commented Nov 15, 2024

Which parent handler?

@simonhamp, The DB is migrated after the rewriteDatabase() call on both commands. It seems totally unneeded here. Also, this call will migrate the original database and not the appdata database. Maybe @mpociot can confirm this. I'm not familiar with the codebase, I just wanted to pinpoint the error and the solution that worked for me instead of openning an issue.

return parent::handle();

return parent::handle();

@simonhamp
Copy link
Member

simonhamp commented Nov 15, 2024

Thanks for that extra context. You're right that it's potentially unnecessary - in the context of running the commands - but from what I can tell it shouldn't cause any issues with those - the parent::handle() attempt would be a no-op ("Nothing to migrate").

It is still needed when the developer boots the app without running native:migrate.

this call will migrate the original database and not the appdata database

We're not concerned about that here. The appdata database gets migrated automatically when the NATIVEPHP_APP_VERSION changes.

When database is missing, it (native:migrate:fresh) does not migrate at all.

Does Laravel's migrate:fresh create a database when it doesn't exist?

Although the code for Laravel's FreshCommand itself seems to suggest that it could create the database, if the database file that it's trying to set up doesn't exist in the first place, it errors:

Screenshot 2024-11-15 at 10 41 33

I think this is probably the correct behaviour. In any case, this would be something to take up with the Laravel project.

So I don't think we need to change anything here on our side.


If you wanted to, you could remove the unnecessary (and potentially recursive) logic that executes when running the migration commands, I think it's a case of moving these lines out of rewriteDatabase and into their own method in the NativeServiceProvider that only gets executed when APP_DEBUG=true:

if (! file_exists($databasePath)) {
    touch($databasePath);

    Artisan::call('native:migrate');
}

On second thoughts, don't attempt this. It's not that straightforward.

@simonhamp
Copy link
Member

I've tested this thoroughly now - and created #413 off the back of that.

So I'm confident that this is all working as intended and that this PR isn't necessary.

Thanks

@simonhamp simonhamp closed this Nov 15, 2024
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