Inaccurate error thrown when site is attempted to be created on platform that isn't verified

Attached is a patch against Hosting HEAD that doesn't allow sites to be created on unverified platforms (at least through the standard Add Site node/add/site frontend interface).

The existing site form gets the list of available platforms via _hosting_get_platforms(), which gets its data from the node table (which doesn't know anything about whether a platform is actually verified or not).

I copied this function to make _hosting_get_verified_platforms(), which does basically the same thing but does a join on hosting_platforms table to only return rows where the verified field is greater than 0.

Substituting this function in favour of _hosting_get_platforms() in the site form only shows thus 'verified' platforms (or, shows no Platforms if the results are not greater than 1.. which is existing functionality that makes sense) . So in this sense it isn't 'preventing' sites from being created on this platform, but it's not giving the option to.

I decided not to modify the existing _hosting_get_platforms() because it's used elsewhere, plus even if it wasn't, it'd be a useful function to have for any future work that might want to just get a list of all platforms.

As a result I think this isn't a 'Provision' bug as it doesn't get this far, it's in Hosting. I've modified the issue metadata to suit (sorry if I'm overstepping my mark doing so)

Cheers (sorry digging up these old issues, I felt like doing some 'housecleaning' :) )

1/ hidding not verified platforms is annoying. It can be solved by just putting some extra in the field description, as "Unverified patforms are not listed".

Not as annoying as trying to provision a site on an unverified platform and getting an error :)
It is trivial to add some wording to that effect. But remember you can always see the list of all Platforms even in the block that provides it.

2/ Most important: how will it behaves when trying to create a site in the first platform and that has not been verified yet?

Isn't the verification of the first platform, the first thing that happens post-install? Even before any existing sites are imported.

If this isn't the case, then to me that is a separate bug!

I should also add, that remember that even prior to this patch, if there is *only* one platform created, the entire 'Platform' field in the form is hidden, and has always been this way at least since I've been using Aegir.

I was reminded of this after editing the Description for this field, and then realised that this extra wording will still be hidden if two platforms exist and only the first platform is verified :) so to me there is little point in adding the wording. If it's seen as significant, use the attached patch.

jonhattan has pointed out over IRC that if no platforms are verified ('verified = 0' on all platforms in hosting_platform), the install profile options (if applicable) are still shown on the Add Site form, and that this'd still break new site creations.

This is kind of based on the logic that people are trying to create sites prior to the initial platform being verified, and I can't imagine that being done realistically.

On the other hand it's occured to me another issue: what happens if a platform that was verified, becomes un-verified (maybe someone adds a new install profile to the platform, runs another Verify task to pick it up, and it's poorly coded and breaks the platform on a re-verify), does it have its 'verified' field set back to 0 ? If not, my patch may assume a false positive. I guess this needs more work.

Attached is a reroll, I offer two different versions.

Common changes

Both patches do the following:

1) Throw an error in hook_validate() of the site form if the platform is not valid

2) Edit hosting_site_hosting_verify_task_rollback so that if a verify fails of a site, set verification to '-1' instead of 0 (which translates to Never, which isn't really true). Also add a clause of the same sort but for task->ref->type 'platform' as opposed to just 'site'

3) Edit hosting_format_interval() to account for the '-1' parameter above, so that in a View of a platform or a site that is currently not verified, but has been verified in the past, 'Last verification failed' is shown instead of 'Never'

Differences in the two options

Option one does 1) above differently: it uses the _hosting_get_verified_platforms() I created in the original patch to get the list of verified platforms, and it doesn't show platforms that are not verified, in the Site form. So any platform chosen by the user on form submit should already be valid (unless they have hit submit wildly despite no verified platforms existing yet: see below to see that we do still check this in hook_validate anyway just in case)

In Option 1 as a result, there is also now some extra sanity checking in there to not expose things like the install profiles etc on error, that jonhattan picked up on in #11.

Option 1 also throws an immediate form_set_error on the rendering of the Site form (before submit, when just visiting /node/add/site), telling the user they won't be able to create the site if there are no verified platforms, in an effort to stop them then and there (it later goes into hook_validate and sets a weightier check to see if the user has gone on anyway and submitted the form: if they have, and no verified platforms exist, that is the same error again that is thrown back, as opposed to bothering to run all the other checks like 'is this is a new client' and so on)

Then I rewrote this whole idea which became Option 2, as I wasn't sure that throwing such blanket errors at the user was good.. so here we have Option 2, which instead of hiding the unverified platforms and throwing an error if there are none, still displays all platforms that *exist*, but uses hook_validate and a new function I called '_hosting_platform_is_valid($platform)' to check if the platform specific to the user's choice in the form is valid or not, and if not, *then* throw an error in the more customary way.

To me Option 2 is a bit more graceful, lightweight and consistent with how Drupal tends to operate (using just hook_validate to deal with whether the choice is ok or not).. Option 1 takes things a bit more further by actually *hiding* unverified platforms altogether. I leave both options here for discussion, testing and choice for the developers!

Thanks jonhattan for ideas, testing and advice while I worked on this.

Crap, you're right, I didn't even realise I'd put 'platform' specific conditions into the site/ stuff. That was ugly, thanks for picking it up.

Jonhattan, I dunno if I agree with your point 2/ : about leaving new platforms as verified = '0' as opposed to setting them to -1.

The reason being that it's already like that, let me illustrate in a scenario:

1) User adds a platform
2) Task gets added to the scheduling queue. At this point it has already been added to the hosting_platform table with a 'verified' value of 0. This makes sense, it's never been verified. At this point the View shows Verified: Never. Good.

3) Verification fails when the task is executed. It is set to -1

4) The status of the platform is now 'Last verification failed'. To me this makes sense: the last verification *did* fail, and since a verification was attempted, it's both true that it's never been 'verified', and that the last verification attempt failed. But at this point, to me, it is more important to know the verification failed than whether it's the first time or not.

Arguments welcome! :)