Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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
Comment | File | Size | Author |
---|---|---|---|
#14 | hostmaster.user-interface.1853620-14.patch | 670 bytes | helmo |
#10 | hosting_site.jpg | 60.76 KB | omega8cc |
#8 | hostmaster.user-interface.1853620-7-interdiff.patch | 1.94 KB | helmo |
#7 | hostmaster.user-interface.1853620-7-interdiff.patch | 5.61 KB | helmo |
#5 | hostmaster.user-interface.1853620-5.patch | 5.19 KB | helmo |
Comments
Comment #1
Deciphered CreditAttribution: Deciphered commentedPatch:
- Adds 'db_name' column to 'hosting_site' table.
- Saves 'db_name' to site node on site verify.
Comment #2
Deciphered CreditAttribution: Deciphered commentedForgot to mark for review.
Comment #3
anarcat CreditAttribution: anarcat commentedthis 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!
Comment #4
Steven Jones CreditAttribution: Steven Jones commented@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.
This only seems to be populated on a verify, not when installing a site for the very first time.
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.
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.
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!
Comment #5
helmo CreditAttribution: helmo commentedI've updated the patch, with the suggestions from @Steven Jones.
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.
Comment #6
Deciphered CreditAttribution: Deciphered commentedIf at all possible, can we get an interdiff for this to make the review process a little easier?
Comment #7
helmo CreditAttribution: helmo commentedSure
Ignore this one... It's for a different issue
Comment #8
helmo CreditAttribution: helmo commentedThis should be the correct one... time for some sleep ;)
Comment #9
anarcat CreditAttribution: anarcat commentedAlright 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...
Comment #10
omega8cc CreditAttribution: omega8cc commentedAnyone tested this? I have tried the vanilla 2.x install and while
db_name
is present in thehosting_site
table, it is always empty.Comment #11
helmo CreditAttribution: helmo commentedTrue, that's my point in #5.
I've now opened #1950052: Always add a verify task after site install to discuss this.
Comment #12
Steven Jones CreditAttribution: Steven Jones commented@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.
Comment #13
omega8cc CreditAttribution: omega8cc commented@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.
Comment #14
helmo CreditAttribution: helmo commentedFound it.
One line got into the wrong place, this patch solves it.
Must be my bad, as the patch in #1 was ok.
Comment #15
omega8cc CreditAttribution: omega8cc commentedYep, that does the trick, thanks!
Comment #16
anarcat CreditAttribution: anarcat commentedcommitted and pushed, sorry for the mess.