Since hosting_get_profiles() returns only the profiles that have status=1 and the hostmaster/hostslave profiles have kludges in verify to be "disabled" (status = 0), we cannot even *edit* the hostmaster site (which would be useful to change ports, add SSL or things like that).

        // Kludge to hide the hostmaster and hostslave profiles
        if (($type == 'profile')
          && in_array($short_name, array('hostslave', 'hostmaster'))) {
          $package->status = 0;
        }
        else {
          $package->status = 1;
        }

In turn, hosting_get_profiles() is this:

function hosting_get_profiles($platform = NULL) {
  $profiles = array();
  if (!is_null($platform)) {
    $instances = hosting_package_instances_load(array(
      'rid' => $platform, 'package_type' => 'profile',
      'n.status' => 1));

Basically, I think that hosting_get_profiles() *should* return the hostmaster profiles, but other access control mechanisms should disable access to it. It *is* a profile, and a valid one, for *one* site (or many, once we start self-hosting). But it's understandable that it needs to be controled.

So i'm not proposing to ditch the status right away. I would suggest however to replace it with proper access control, a bit like the work mig5 is doing on platforms.

CommentFileSizeAuthor
#6 profile.png30.89 KBmig5
#6 599758.patch3.04 KBmig5
#2 599758.patch2.19 KBmig5

Comments

Anonymous’s picture

Anonymous (not verified) commented
Status: Active » Needs review

I have a fix for this.

Please clone or add my git repo as a remote

git://git.mig5.net/drupal/profiles/hostmaster/

The branch is 599758_edit_main_aegir_site

Or just cherry pick this (I need to clean up my dirty git):

http://git.mig5.net/?p=profiles/hostmaster/.git;a=commitdiff;h=abbb298a8...

Anonymous’s picture

Anonymous (not verified) commented
StatusFileSize
new 599758.patch2.19 KB

Reverted hosting_get_profiles to still check if node's status = 1, even though it doesn't matter anymore because the logic is done in hosting_site_form(). However, we may want to check on the status of a profile package down the track (so it's just in case).

So I now add a hook_update to set status = 1 for any hostmaster or hostslave package in this commit and also added a check for whether a profile is hostslave and not just hostmaster (for ancient systems) in this commit

Overall patch for the lazy attached

ac’s picture

ac
Primary language English
Location Perth
commented

works against DRUPAL-6--0-4-ALPHA3

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Fixed

Thanks! Fixed in git.

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Needs work

So as usual, my attempt here caused a nice new regressive bug that prevented sites being created on platforms that only had one profile.

 function _hosting_site_form_profile($platform = NULL) {
   $profiles = hosting_get_profiles($platform);
   foreach($profiles as $id => $name) {
    if (in_array($name, array('hostmaster', 'hostslave'))) {
       unset($profiles[$id]);
     }
   }
 ...

Somehow this caused platforms with, say, only the 'default' profile, to get nuked somehow?? even though I could var_dump it and see it.. on submission of the site form, hook_validate threw 'Please fill in a valid profile'. Provisioning a site on the main aegir platform didn't, i.e it nuked the hostmaster profile but left 'default' hidden in the form.

Tried to find a fix but I don't understand why it isn't working.

So, reverted yet another regression on my part. Sorry everyone.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs work » Needs review
StatusFileSize
new 599758.patch3.04 KB
new profile.png30.89 KB

If I var_dump the form when there's only one profile on that platform, it shows the default profile..

If I submit the form and var_dump in hook_validate, $node->profile is null !! and hence the exception

So if I add a #value to the form when the number of profiles is <= 1, it works.. can someone explain this to me?

Attached is a patch where it's working..

The annoying thing is when there is *more* than 1 profile and neither of them are hostmaster or hostslave, and the default_value is key($profiles) in both versions of the profile form, the default platform is no longer the one selected by default.. I don't understand this either, probably did something wrong. screenshot of that attached. I thought it might've been alphabetical or something, but then i cloned my mig5_net profile to zmig5_net and it was still mig5_net selected by default!

At least the form submits now with this patch. But I really wish I understood the rest above, and why #value is needed where it wasn't before, just because of doing an unset in the array

Anonymous’s picture

Anonymous (not verified) commented

Too spooked to commit to HEAD now. So there's a branch on g.a.o, 599758_edit_main_site

anarcat’s picture

anarcat commented 
--- a/modules/hosting/package/hosting_package.install
+++ b/modules/hosting/package/hosting_package.install
@@ -331,3 +331,13 @@ function hosting_package_update_6001() {
   return $ret;
 }
 
+// Set the hostmaster or hostslave profile packages to status = 1 in the node table per #599758
+function hosting_package_update_6002() {
+  $ret = array();
+  $result = db_query("SELECT h.nid FROM {hosting_package} h JOIN {node} n ON n.nid = h.nid 
+                      WHERE h.short_name = 'hostmaster' OR h.short_name = 'hostslave';");
+  while ($package = db_fetch_array($result)) {
+    db_query("UPDATE {node} SET status = %d WHERE nid = %d", 1, $package['nid']);
+  }
+  return $ret;
+}

Not sure why this is necessary, but you shouldn't loop on a select, but UPDATE on a WHERE instead... like this:

UPDATE {hosting_package} h JOIN {node} n ON n.nid = h.nid SET status = %d WHERE h.short_name = 'hostmaster' OR h.short_name = 'hostslave'

... or something like that.

+++ b/modules/hosting/site/hosting_site.module
+++ b/modules/hosting/site/hosting_site.module
@@ -415,7 +415,6 @@ function hosting_site_validate($node, &$form) {
   if (!hosting_domain_allowed($url, (array) $node)) {
     form_set_error('title', t("The domain name you have specified is already in use."));
   }
-
   if (!array_key_exists($node->profile, hosting_get_profiles($node->platform))) {
     form_set_error('profile', t('Please fill in a valid profile'));
   }

keep whitespace changes out of regular commits please

+++ b/modules/hosting/site/hosting_site.module
@@ -889,7 +893,7 @@ function _hosting_site_form_profile($platform = NULL) {
   else {
-    $form['profile'] = array('#type' => 'hidden', '#default_value' => key($profiles), '#attributes' => array('class' => "hosting-site-form-profile-options"));
+    $form['profile'] = array('#type' => 'hidden', '#value' => key($profiles), '#default_value' => key($profiles), '#attributes' => array('class' => "hosting-site-form-profile-options"));
   }

i think you just need #value here and that,s the key fix here: that's why things broke when there was only one profile... this code probably never worked!

I'm on crack. Are you, too?

Anonymous’s picture

Anonymous (not verified) commented 
UPDATE {hosting_package} h JOIN {node} n ON n.nid = h.nid SET status = %d WHERE h.short_name = 'hostmaster' OR h.short_name = 'hostslave'

I have no idea why I didn't do it so simple like that. I think *I* am the one on crack.

The reason the hook_update is necessary is to set status = 1 for those profiles since the logic for 'ignoring' those profiles is now done in the hosting_site_form().

I'll reroll soon.

Anonymous’s picture

Anonymous (not verified) commented

After that patch above, 'Please select a valid install profile' error is not seen when:

1 x main aegir platform with default + hostmaster profiles

or

1 x main aegir platform with default + hostmaster profiles
1 x drupal-6.14 platform with just default profile

But *is* seen with:

1 x main aegir platform with default + hostmaster profiles
1 x drupal-6.14 platform with default + some other profile
1 x another drupal-6.14 platform with just default profile

when the third platform there, with only default, is chosen as the platform for a new site.

I have no idea.

Anonymous’s picture

Anonymous (not verified) commented

I've pushed some changes to that branch on git.aegirproject.org, fixing the hook_update and removing the whitespace.

The profile section of hosting_site_form actually works now too, in all cases that I can find, except that it's messy and there's some duplication, because I seem to have to account twice for the potential for the number of profiles to be no more than 1 (once if there is already only one profile, and again if there were more than one but unsetting the hostmaster/hostslave options has dropped it down to one).

Tried other more sensible combinations of this but was having no luck!

And I suppose it needs a check for the edge case of a platform with *only* the hostmaster or hostslave profile, and unsetting it dropping it to 0.. although the hook_validate might be enough to take care of that anyway.

Kind of at a wall with this one, sorry.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Fixed

I think I've finally fixed this stupid bug

tema’s picture

tema commented

It's working for me. Many thanks!

Status: Fixed » Closed (fixed)
Issue tags: -self-hosting

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

Issue tags: +self-hosting

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.

  • Commit bc6d30e on 6.x-2.x, 7.x-3.x, dev-ssl-ip-allocation-refactor, dev-sni, dev-helmo-3.x by mig5: 
    #599758 - allow the main aegir site to be edited (perform profile =...
  • Commit cff562d on 6.x-2.x, 7.x-3.x, dev-ssl-ip-allocation-refactor, dev-sni, dev-helmo-3.x by mig5:
    mig5 does not belong here. reverting 'fix' for #599758
  • Commit 38564d6 on 6.x-2.x, 7.x-3.x, dev-ssl-ip-allocation-refactor, dev-sni, dev-helmo-3.x by mig5:
    Fix for #599758 - allow main aegir site node to be editable.

  • Commit bc6d30e on 6.x-2.x, 7.x-3.x, dev-ssl-ip-allocation-refactor, dev-sni, dev-helmo-3.x by mig5: 
    #599758 - allow the main aegir site to be edited (perform profile =...
  • Commit cff562d on 6.x-2.x, 7.x-3.x, dev-ssl-ip-allocation-refactor, dev-sni, dev-helmo-3.x by mig5:
    mig5 does not belong here. reverting 'fix' for #599758
  • Commit 38564d6 on 6.x-2.x, 7.x-3.x, dev-ssl-ip-allocation-refactor, dev-sni, dev-helmo-3.x by mig5:
    Fix for #599758 - allow main aegir site node to be editable.