-
-
Notifications
You must be signed in to change notification settings - Fork 7
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
Use default DDEV database #22
Conversation
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 think it would be better to use a separate database, perhaps named xhgui? In general, the actual database should probably only contain project information.
xhgui/xhgui.config.php
Outdated
'dsn' => getenv('XHGUI_PDO_DSN') ?: null, | ||
'user' => getenv('XHGUI_PDO_USER') ?: null, | ||
'pass' => getenv('XHGUI_PDO_PASS') ?: null, | ||
'dsn' => getenv('XHGUI_PDO_DSN') ?: 'mysql:host=db;dbname=xhgui', |
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 like it.
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.
Not sure if it's better to nudge people to update docker-compose.xhgui.yaml
or xhgui.config.php
?
We pass environmental through docker-compose so these are DDEV-prefered fallbacks.
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 recommend having them touch either one. Why would we have them change any file? xhgui's files can create the db if it doesn't exist, and use the special db. The only problem, as you mentioned elsewhere, might be postgres. It doesn't know how to use 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.
While testing, I discovered that ${DDEV_DATABASE_FAMILY}
does not work for postgres.
The original PR (mine) sets DDEV_DATABASE_FAMILY as follows:
- Defaults to
mysql
- Set to
postgres
if database is postgres
This is in line with official connection strings: https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-CONNSTRING
EG. postgresql://user:secret@localhost
However, when using PHP DSN strings, such as xhgui uses, it requires pgsql
Eg. psql:host=db;dbname=xhgui
37d8de7
to
cd50c92
Compare
OK this looks ready to me. It dynamically sets the database and includes functions tests for each.
|
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.
This probably needs removal_actions
in the install.yaml to remove the new database when the add-on is uninstalled.
Thanks for the great work on this!
Manually testing with
|
It's a great improvement, thanks! Tested OK. Looking forward to all the great improvements here. |
It now removes the XHGUI database on removal. Updated the tests for confirmation. |
Oh, sorry. I guess I have to change Drupal configuration to make it work at all. Sometimes these PRs and issues run too long! :) |
Now it works, since I followed the instructions. A future improvement might be to figure out a way to tell people "You haven't done the configuration". Optionally adding the configuration the way ddev-redis does for Drupal would be a thing to consider. |
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.
This works great! Removal works great, except the xhprof_prepend, which is elsewhere.
Future improvements may include coaching people if they haven't done necessary pre-configuration, as you have to do for Drupal, etc.
This PR
PDO does NOT have feature parity with MongoDB, see Compatibility matrix.
Not sure if PDO or MongoDb should be default.
See #16.