-
Notifications
You must be signed in to change notification settings - Fork 87
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
PostgreSQL query fix #51
base: master
Are you sure you want to change the base?
Conversation
Thanks. I'd like to leverage Doctrine. I believe it's already a dependency of the project and handles all this "what type of DB" is it logic for free. I'm going to leave this open for a bit. In the meantime, anyone landing here can feel free to pull this branch. |
|
||
$type = DB::select(DB::raw('SHOW COLUMNS FROM ' . $table . ' WHERE Field = "' . $name . '"'))[0]->Type; | ||
|
||
if (env('DB_HOST') == 'postgres') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that's a good idea to check the database host to determine whether it's PostgreSQL. This check should be for pgsql driver:
if (env('DB_CONNECTION') === 'pgsql') {
What if the user has custom connection names? I think the best way would be to add a option to the command, to pass the desired connection name if not default: protected $name = 'generate:model-factory {--connection=?}'; Than, it should query for the connection driver if the option is set: $connection = $this->option('connection');
$dbDriver = config("database.connections.$connection.driver");
if ($dbDriver === 'pgsql') { Something like that, give or take. |
Even in case of custom connection name that probably still will be a default connection. Something like that should work fine: if (DB::connection()->getDriverName() === 'pgsql') { |
Any plan on merging this? |
If the application is connected to an postgres DB then the query that existed wouldn't suffice, because postgres has a different syntax. I added a simple if clause to check if the laravel application is connected to an postgres DB, if so the query will be executed with proper postgres syntax else the existing query will be executed. This will remove the SQL error as well as the class not recognized error when running the php artisan command for applications that have postgres DB.