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

Prevent using unavailable databases #334

Merged
merged 14 commits into from
Apr 25, 2024
Merged

Conversation

driesvints
Copy link
Member

@driesvints driesvints commented Apr 19, 2024

This PR will make sure that unavailable database options are filtered out of the database list. In some rare situations, Sqlite isn't available and people fail their installations because they press continue on it as the default. This PR will filter out Sqlite when it's not available as a PDO driver and will move to the next in line as the default. Similar, SqlServer is filtered out on macOS, etc.

Pending shivammathur/setup-php#835

@jessarcher
Copy link
Member

I wonder whether it would be better to display the databases with something like (Missing extension) on the end. The default could be set to the first available.

If they are hidden, people may not know that Laravel even supports them. This also allows people to deliberately choose a missing extension during setup, knowing that they would just need to install it afterwards.

If someone has just set up a new machine and forgotten to install their favourite extension, I think it would be annoying if they got all the way through the setup and then had to either cancel or choose an extension they didn't plan on using and then manually change it afterwards.

@driesvints
Copy link
Member Author

I wonder whether it would be better to display the databases with something like (Missing extension) on the end. The default could be set to the first available.

@jessarcher haha I was thinking the same thing actually and really doubting between the current and that approach. I'll update the PR.

I think it would be annoying if they got all the way through the setup and then had to either cancel or choose an extension they didn't plan on using and then manually change it afterwards.

They still have to do that though if they reach the above select menu with the "missing extension" labels. They'll only notice it then and will have to cancel the process.

@ChazyTheBest
Copy link

@driesvints I know this is out of the scope of this PR but looking at how it's been ignored how about migrations failing for databases other than sqlite? The way I see it the installer should ask for auth details for the other databases or simply disable migrations except for sqlite. The installer is currently broken when selecting migrations with databases other than sqlite and I don't know why it's been like this for such a long time. I'd open a PR but I've seen one closed without any comment whatsoever. Not that it's a big deal, just saying.

@driesvints
Copy link
Member Author

@ChazyTheBest not sure what you mean but we have no knowledge of migrations being broken with the installer atm. If you have a specific problem please open an issue and reference the related issue you mention.

@driesvints
Copy link
Member Author

@jessarcher I pushed a change that now implements the "Missing PDO extension" label. I didn't change the default database option to the first one available but kept things as they were. I do believe we need that because if people now install with no interactivity and a database is automatically selected that's not available they'll run into the validation I've put in place. If you want, could you maybe look into that?

Screenshot 2024-04-23 at 14 51 16

src/NewCommand.php Outdated Show resolved Hide resolved
src/NewCommand.php Outdated Show resolved Hide resolved
src/NewCommand.php Outdated Show resolved Hide resolved
driesvints and others added 4 commits April 25, 2024 11:22
@driesvints
Copy link
Member Author

driesvints commented Apr 25, 2024

This is the latest that we landed on: we'll add a label to the database engines which are missing their PDO extension.

Screenshot 2024-04-25 at 11 40 40

@driesvints driesvints marked this pull request as ready for review April 25, 2024 09:42
@taylorotwell taylorotwell merged commit b8587d0 into master Apr 25, 2024
11 checks passed
@taylorotwell taylorotwell deleted the unavailable-databases branch April 25, 2024 14:46
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.

4 participants