It's not uncommon for a database to not get removed correctly or similar site names to have similar database names, so it would be extremely helpful if the actual sites db_name was listed in the site summary.

Patch in comment #1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Deciphered’s picture

FileSize
5.23 KB

Patch:
- Adds 'db_name' column to 'hosting_site' table.
- Saves 'db_name' to site node on site verify.

Deciphered’s picture

Status: Active » Needs review

Forgot to mark for review.

anarcat’s picture

this seems like a great idea, once it's RTBC i'll happily commit this (or I will test it myself when i find time), thanks!

Steven Jones’s picture

Status: Needs review » Needs work

@Deciphered Thanks so much for the patch, I like it too, here's a brief review, the main issues being the length of the DB column, and that it doesn't work when using just the 'install' task.

+++ b/modules/hosting/site/hosting_site.drush.incundefined
@@ -195,6 +195,7 @@ function hosting_site_post_hosting_verify_task($task, $data) {
+    $task->ref->db_name = $data['self']['db_name'];

This only seems to be populated on a verify, not when installing a site for the very first time.

+++ b/modules/hosting/site/hosting_site.installundefined
@@ -30,6 +30,12 @@ function hosting_site_schema() {
+        'length' => 16,

The length of the column should match the maximum length of a database name according to MySQL, not to Aegir's auto-generated names, so this needs to be 64.

+++ b/modules/hosting/site/hosting_site.installundefined
@@ -314,3 +320,12 @@ function hosting_site_update_6201() {
+  db_add_field($ret, 'hosting_site', 'db_name', array('type' => 'varchar', 'length' => 16, 'not null' => TRUE, 'default' => ''));

This line is too long, can we split the large array onto multiple lines, using a temporary variable if needed please? Also, the db column length needs to be 64 again.

+++ b/modules/hosting/site/hosting_site.nodeapi.incundefined
@@ -75,11 +84,11 @@ function hosting_site_view(&$node, $teaser = false) {
+      '#value' => $node->db_name,

This should run the DB name through check_plain to prevent weird security issues.

This patch should be good to go after those edits, thanks again!

helmo’s picture

Status: Needs work » Needs review
FileSize
5.19 KB

I've updated the patch, with the suggestions from @Steven Jones.

This only seems to be populated on a verify, not when installing a site for the very first time.

This would have to be triggered from provision: drush_provision_drupal_post_provision_install()
I'm reluctant to add add this logic to provision, maybe we should start to trigger a verify task after installing a site.

Deciphered’s picture

If at all possible, can we get an interdiff for this to make the review process a little easier?

helmo’s picture

Sure

Ignore this one... It's for a different issue

helmo’s picture

This should be the correct one... time for some sleep ;)

anarcat’s picture

Status: Needs review » Fixed

Alright thanks everyone, pushed as e7cb9b9.

By the way, I got humbled by Jon Pugh's way of doing hook_update_N() schema changes in #1681904: Ability to configure a url to redirect to in site configuration., this could have been used here for the db_add_field() - but I didn't find it fair to push back the patch again...

omega8cc’s picture

Status: Fixed » Needs work
FileSize
60.76 KB

Anyone tested this? I have tried the vanilla 2.x install and while db_name is present in the hosting_site table, it is always empty.

hs

helmo’s picture

True, that's my point in #5.

I've now opened #1950052: Always add a verify task after site install to discuss this.

Steven Jones’s picture

@anarcat Re your comment in #9 about using hook_schema in hook_update_N functions, that is a seriously bad idea and should never ever be done!
hook_update_N functions need to never ever change. But you change the hook_schema function all the time, so if your hook_update_N function depends on it, then you are changing it by proxy, and bad things will happen when people upgrade.

omega8cc’s picture

@helmo - OK, but my point is - and you can see it in the screenshot above - that it is *not* populated on a verify, not just on an initial install. Or do you mean that chained install+verify would be required to populate it? That seems overcomplicated to me.

helmo’s picture

Status: Needs work » Needs review
FileSize
670 bytes

Found it.
One line got into the wrong place, this patch solves it.

Must be my bad, as the patch in #1 was ok.

omega8cc’s picture

Status: Needs review » Reviewed & tested by the community

Yep, that does the trick, thanks!

anarcat’s picture

Status: Reviewed & tested by the community » Fixed

committed and pushed, sorry for the mess.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

  • Commit e7cb9b9 on 6.x-2.x, 7.x-3.x, dev-588728-views-integration, dev-1403208-new_roles, dev-helmo-3.x by anarcat:
    Issue #1853620 by helmo, Deciphered: Added db_name() to site summary.
    

  • Commit e7cb9b9 on 6.x-2.x, 7.x-3.x, dev-588728-views-integration, dev-1403208-new_roles, dev-helmo-3.x by anarcat:
    Issue #1853620 by helmo, Deciphered: Added db_name() to site summary.