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

Make DynamicDatabaseConnection static #898

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

PseudoResonance
Copy link
Contributor

I thought it was odd that this class, which has no internal state or anything, wasn't a static method and required instantiating the class first.
Does this seem reasonable to change, or is there some plan for it?

@Boy132
Copy link
Member

Boy132 commented Jan 9, 2025

We should get rid of DynamicDatabaseConnection completely and update Database to handle it directly.
Ideally using DB::build instead of DB::connection and the config()->set().

@PseudoResonance
Copy link
Contributor Author

We should get rid of DynamicDatabaseConnection completely and update Database to handle it directly. Ideally using DB::build instead of DB::connection and the custom config.

What about any logic for generating the connection config?

This is just an example from my initial commit, but I needed to get a few different default values depending on the driver.
https://github.com/PseudoResonance/pelican-panel/blob/support-postgresql/app/Extensions/DynamicDatabaseConnection.php#L9

@Boy132
Copy link
Member

Boy132 commented Jan 10, 2025

Do we even need to set the charset and collation?
Worst case we just use the default values from the normal config.

@PseudoResonance
Copy link
Contributor Author

PseudoResonance commented Jan 10, 2025

Do we even need to set the charset and collation? Worst case we just use the default values from the normal config.

I have no idea. Postgres almost certainly doesn't need it, but the one thing that is really important is the default database. On postgres, you usually want to test a connection by connecting to postgres, while mariadb and mysql, you connect to mysql to validate it.

Edit: This is kind of not entirely the correct way to do it either, but it's a limitation by Laravel. You should be able to connect to the server with no database specified, but Laravel requires it.
Additionally, on MariaDB/MySQL, the database is only a suggestion, so it should be fine regardless, but on PostgreSQL, the database is required and will fail if it's invalid.

Edit: I realized, I'm already planning on making an enum for the driver types, and the enum will store the default database as a property. So it's not that big of a deal to just get rid of this.

@PseudoResonance
Copy link
Contributor Author

What about something like this?
The defaults and all other database related things are all defined in this enum. Then it can be fetched fairly easily if you need it for DB::build, like in that first example.
I was also thinking perhaps I can just return the whole default options array, and then the desired values can be overwritten. Then the DB::builds can be 100% driver agnostic and will still fetch and use all the driver specific options.

These are just some things I was trying out, I'm not sure what would be best. I tried to move all the defaults to a central place where they can be referenced fairly easily from anywhere else, instead of redefining them all over the place.

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