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

Use default DDEV database #22

Merged
merged 7 commits into from
May 28, 2024
Merged

Use default DDEV database #22

merged 7 commits into from
May 28, 2024

Conversation

tyler36
Copy link
Collaborator

@tyler36 tyler36 commented Nov 21, 2023

This PR

  • uses the default DDEV database
  • removes Mongo as a dependency.

PDO does NOT have feature parity with MongoDB, see Compatibility matrix.
Not sure if PDO or MongoDb should be default.

See #16.

Copy link
Member

@rfay rfay left a 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.

'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',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it.

Copy link
Collaborator Author

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.

Copy link
Member

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?

Copy link
Collaborator Author

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

@tyler36
Copy link
Collaborator Author

tyler36 commented Apr 26, 2024

OK this looks ready to me.

It dynamically sets the database and includes functions tests for each.

  • mariadb
  • mysql
  • postgres

@tyler36 tyler36 requested a review from rfay April 26, 2024 08:53
Copy link
Member

@rfay rfay left a 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!

config.xhgui.yaml Show resolved Hide resolved
config.xhgui.yaml Show resolved Hide resolved
@rfay
Copy link
Member

rfay commented Apr 27, 2024

Manually testing with

ddev get https://github.com/tyler36/ddev-xhgui/tarball/20231121_use_ddev_database

@rfay
Copy link
Member

rfay commented Apr 27, 2024

It's a great improvement, thanks! Tested OK. Looking forward to all the great improvements here.

@tyler36 tyler36 requested a review from rfay May 27, 2024 23:48
@tyler36
Copy link
Collaborator Author

tyler36 commented May 27, 2024

It now removes the XHGUI database on removal. Updated the tests for confirmation.

@rfay
Copy link
Member

rfay commented May 28, 2024

I took it for a spin, but now nothing seems to happen at all. I installed this PR, did ddev xhprof on, visited a few pages, and still see "Looks like you haven't done any profiling"

select * from watches; and select * from results; both show no rows.

image

@rfay
Copy link
Member

rfay commented May 28, 2024

Oh, sorry. I guess I have to change Drupal configuration to make it work at all.

Sometimes these PRs and issues run too long! :)

@rfay
Copy link
Member

rfay commented May 28, 2024

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.

Copy link
Member

@rfay rfay left a 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.

@tyler36 tyler36 merged commit 40f5ef2 into main May 28, 2024
2 checks passed
@tyler36 tyler36 deleted the 20231121_use_ddev_database branch May 28, 2024 01:44
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