So, having patched this form in #937490: Fix Platform access control to deal with profile > platform paradigm, I'm a bit concerned about what we are doing in an ajax callback.

I see this primarily as a usability issue. First of all, on our Aegir deployment at Koumbit, with it's multitude of platforms, we have a significant lag (7-10 seconds!) between loading the form and getting the ajax callback to format the options. This leads to confusion for first-time users, and frustration for those of us waiting around for the from to load up.

I've managed (with anarcat's help) to lighten the load by reducing the number of queries I introduced to this function with the attached patch.

Now I would like to rework the hosting_site form function to limit the options available when the form is built, before js even turns on. I think we can do this independantly of reworking the platform access control into something sensible, if we provide some clear function calls that do the work of checking the client's access permissions. In this case, those functions are:

* hosting_site_available_options
* _hosting_get_allowed_platforms (Deprecated in #725952: implement node-level access permissions for platforms)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

adrian’s picture

to be fair. it should be using node_access

Anonymous’s picture

I welcome any improvement here - I find the site form to be one of our weakest points (granted, I don't understand javascript so am not helping to make it better)

I was also musing about some of the idiosyncracies of the hosting_site_available_options and hosting_site_form stuff here #952700: Site form, duplication, and platform access control (saga continues)

Awkwardly I moved your 'additionally show platforms that have NO access control set' into the _hosting_site_available_platforms function, but that actually managed to make it slower, as the query to do that in there was another ugly JOIN, so I didn't bother committing it til we can think it all through in a better way..

sfyn’s picture

Fair. I'm still trying to figure out a solution - +1 for node_access, but I am not sure how to use that to determine what to display in the form - by firing the hooks?

sfyn’s picture

OK so here's my hitlist:

[snip]

Nevermind, I am just going to call _available_options in the form constructor and use it to assign options to the form for now.

sfyn’s picture

Ok, here's my first attempt at patching this.

All this does is change the output of _available_options and then make hosting_site_form and the js use the new data.

adrian’s picture

i'm very unlikely to commit something like this before beta.

previously, i was pre-populating the javascript options array -

http://git.aegirproject.org/?p=hostmaster.git;a=commitdiff;h=12e95d3f452...

I'm not 100% sure why i did this, but i think part of the reason was directly due to the profile first policy.
It was also causing problems with failed submissions.

For the life of me, i could also not determine why the pre_render was being called twice, and that was why i had to have the
static hack in there.

sfyn’s picture

I see what was going on - I forgot to set the right value in the form validation function. Here is an updated patch, that will at least allow correct form submissions to go through.

It's not great, I know. As I said, I am going to take a swing at rewriting the js entirely (probably this week actually)

But for now, it works, people won't see options they don't have access to while the pre-render loads, and it still respects the profiles first policy.

PS I know I typoed the patch name with the wrong nid, sorry but there are no takebacks with files posted to comments.

sfyn’s picture

Status: Active » Needs review
anarcat’s picture

Status: Needs review » Fixed

this is good, i think. while I agree that we should use node_access for this stuff (I was amazed that we didn't) the current situation is quite horribly slow so I believe this patch to be a good thing.

we can open another issue for the idea of using node access here.

in the meantime, i committed the two patches suggested here.

omega8cc’s picture

Category: task » bug
Priority: Normal » Critical
Status: Fixed » Active

Changes introduced in this commit: http://git.aegirproject.org/?p=hostmaster.git;a=commitdiff;h=9d388fa788b... breaks the form completely.

To reproduce:

1. Try to use recent d7-beta2 platform

or

2. Try to enable "Hide platforms with non-default profiles" option.

When you click on any d7-beta2 install profile, you will not be able to choose d7 platform and you need to reload the form in the browser to be able to choose any non-d7 install profile, or no platform can be choosed (unless you have 2 platforms using the same install profile name, like d6 and d5). All other platforms will not be available.

With "Hide platforms with non-default profiles" option enabled all platforms are not available.

Not sure how to rewrite it, so I had to revert this commit in my github clone.

sfyn’s picture

Interesting.

What is "Hide platforms with non-default profiles" supposed to do, exactly? I assume it would allow the Standard and Minimal platforms from d7?

omega8cc’s picture

The "Hide platforms with non-default profiles" option when enabled makes the form far more user friendly, since it allows to automatically hide all platforms using any non-default install profile when the default (Drupal) install profile is clicked in the form. It helps to avoid confusion, because people will no longer try to use default install profile to deploy the site in the platform using its own profile, like OA, MN etc.

But the form is now broken not only with this option enabled in admin/hosting/settings, it is broken also with this option disabled, but when you are using d7 platform.

adrian’s picture

yeah. i was pretty clear about being against this unless it was done properly.

there's a HELL of a lot that ties into that functionality, including all of the validation regarding sites, which then includes things like determining migrate/clone targets and so forth.

adrian’s picture

i also dont like the new deeply nested definitions (form/js).

if they dont match exactly, the form will not validate, hence the entire point is that they should always be the same, hence array.

anarcat’s picture

Status: Active » Needs work

I see. I don't think we tested the D7 case properly here, since it's not a common deployment for us.

I must say that the form was very very slow here and this fixed the problem for us, so while I agree this can be improved, I do believe the performance problelms need to be fixed before release.

Adrian, being "against it" doesn't bring much here: we needed a stopgap measure and while D7 is broken now, I feel it should be easily fixed. Nobody opposes fixing this the right way (ie. node_access) and I believe that sfyn is going to actually try to do that within the next weeks, but I certainly hope nobody opposes making the site form render in less than 10 seconds, which is what we've done here.

Feel free to revert the change and commit a better fix if there's such a thing waiting somewhere..

adrian’s picture

i am reverting this change until it has been properly discussed

[master 5d477b2] Revert "Modified the site form so that options are limited before being delivered to the client"
2 files changed, 20 insertions(+), 18 deletions(-)

sfyn’s picture

> If they dont match exactly, the form will not validate, hence the entire point is that they should always be the same, hence array.

I would really appreciate some concrete suggestions for the "right" way to implement this patch. Is there one?

Also, what is "properly discussed"? In what venue would you like to do that, if not here?

anarcat’s picture

Issue tags: +optimization

@adrian - do you mean #725952: implement node-level access permissions for platforms when you talk about this being "done properly"? I would also love to discuss this, but so far things have been mostly going one way.

Since I felt there were other over-arching issues with the site form, I have opened a meta-issue about this: #967888: meta: refactor site form.

sfyn’s picture

So I have reviewed my patch today and I see what I was doing wrong, or actually two errors I was making that could cause bugs:

1. I was checking platforms and profiles only for the selected client
2. I was using the platforms for the selected drupal profile as the one to check for platforms - on an all D6 deployment that caused no problems, because the default Drupal profile was in every platform. Obviously this breaks down when you are doing D7 stuff as well.

But I still want to find a way to render this form before it is delivered.

Another option would be to do it poll.module style, rebuilding the entire form in the ajax callback and then reloading it when a new choice is made. That way we can safely limit options on load, knowing the form will be rebuilt for each profile. Of course the overhead of that could be staggering.

So I give up for today. This will make a nice project over the next few days.

Anonymous’s picture

The site form is a really crucial part of Aegir and so is a combination of complicated/sensitive to change!

I sympathise with Koumbit and their need for the stopgap, as well as Adrian's desire to see this done right before it goes in (and potentially causes regressions).

But unless I'm forgetting something, we're developing on git and we have feature branches for heavy work like this. dev-services anyone? :)

For this reason I'd propose future work on this form be done in a separate branch. That will allow Koumbit to keep working on this efficiently while isolating it from the master branch while it fluctuates like this. (theoretically there should be nothing wrong with working on it in master, but unfortunately we have too many people using the bleeding edge perhaps).

I'm speaking for Koumbit there in that I presume working on a different branch and continue to merge in/rebase from master if needed in the meantime is not too big of an issue, it may be.

Going back to the 'feature branch' method would also be more efficient than dropping patches around, especially for testers to keep up to date with continuous changes.

Of course, for it to fluctuate, we need eyes on this and more testing, so I'd also like to propose we have some sort of dev chat either on the aegir-dev mailing list to deal with the timezone issues, or find a suitable time to all be on IRC and nut it out.

omega8cc’s picture

While I agree it is a good idea/practice to develop/introduce that kind of work/changes in a separate branch, I'm also almost sure it would go unnoticed if it would not be committed to the HEAD. It was introduced in HEAD, I cloned it to my github repo and then it failed during standard testing with more than 10 different platforms. So in fact it helped with debugging! :)

Anonymous’s picture

It wouldn't go unnoticed if everyone was actually cooperating with each other and testing it.

Sending stuff to the master branch just to see if it causes regression is *not* a development practice I *want* to be involved in (yes, ironically I say that despite my high rate of migressions). I am surprised you are lighthearted about it breaking for you in your mirror - if it had been more serious I am sure you would've preferred it to have been tested in a feature branch first :)

But this is what branches are for per our development workflow, I suggest those that are here to develop and test, do so *together* on a feature branch and leave the master branch pristine until it's been proven to work without regressions. At least to avoid DrupalDrama. But it's only my advice, do whatever you (everyone) like :)

omega8cc’s picture

I fully agree, I just wanted to find a bright side of that incident :)

adrian’s picture

i clearly voiced that i do not wish to see that patch committed and it was committed anyway.

the only thing that is _NEEDED_ to 'preload' the form is to have the settings passed to the front end on page load already, instead of doing a callback first.
this is how it worked before, and i sent you the link to the commit that removed it.

the problem with limiting what actually is generated vis a vis the markup / form structure, is that no matter how you try and get around it, the original form array is what is going to be validated against.

hence, all possible options need to be present, but you can hide them. Previously we hid them by replacing the markup only, but this new approach opts to rather toggle the markup hidden on the ui level. this is orders of magnitude simpler and results in a far more clear line of code. you pass it the current state of the object (when it changes) and it returns the valid options.

previously there were separate callbacks for each of the different fields that then returned markup specifically for those fields, and you had to build the arguments and the queries and the javascript calling them around the exact semantics of the fields including their relationship to each other. now that is not necessary.
It was also not possible to plug into it. So the SSL module couldn't go and remove platforms that dont support ssl from the listing.

additionally you had to re-do all that logic again for validation on the backend, and we had an entire class of errors where the front end available options and the backend options didnt match.

The entire point of the new site form is that the front end and backend available options are at all times exactly the same, and therefor splitting them into ['form'] and ['js'] is the antithesis of what the code is meant to do.

i reiterate however :

THE ONLY THING NECESSARY TO PRELOAD THE OPTIONS FOR THE FIRST PAGE LOAD IS TO POPULATE THE SETTINGS JS ON FORM GENERATION.

sfyn’s picture

> this is how it worked before, and i sent you the link to the commit that removed it.

(This link is in #6, btw - http://git.aegirproject.org/?p=hostmaster.git;a=commitdiff;h=12e95d3f452...)

Adrian, your comment on this commit indicates that there was some problem with fields being visible on failed submits with this code. Did you every go further into that?

adrian’s picture

Generating the pre-loaded array was not taking into account the POSTed values. Usually that's not a problem since you are trying to set up an initial state of the fields. However, when the validation fails, you are displayed the form a second time, and the default state then overrides the actual current state.

where i was generating the preloaded array was in a pre_render callback on the form object, which for some reason was being called twice on every page load, and also didnt have access to the POST without peaking into the globals (which shouldnt happen).

The problem is that the preloading needs to happen in 2 different places, once when displaying the form the first time, and in a different place when the form has been submitted and the post values are available. the latter values should override the earlier values. If the latter one is going to happen, the earlier one probably shouldnt be happening.

Perhaps the answer is using a combination of #validate and #pre_render to generate the needed array, but to do the actual pushing to the UI in a #post_render.

So in summary :
Where the preloaded settings are generated, the post values werent available. meaning it would override with the defaults every time.
Not using the pre-loading meant that on loading the page it would then make the ajax request, that resulted in the same behaviour as the post request.

I should also mention, that before this work came along, the site form also didnt preload the fields. It showed the whole page then made 3 sequentual AHAH
callbacks to retrieve the filtered markup to display for each element (platform -> profile -> language).

dergachev’s picture

Just glanced over the relevant code for a while, after chatting with anarcat.

I notice that hosting_site_available_options() is called for each ajax callback request, and in it, each profile is loaded and validated, and for each profile, each platform is loaded and validated. If performance is an issue, it might make sense to load and validate only the selected profile, and only the platforms that are relevant for this profile.

If the total amount of profile-platform combinations aren't large, we can also consider storing the whole mapping in Drupal.settings, and doing away with the Ajax callbacks altogether.

anarcat’s picture

Priority: Critical » Major

Since nobody seems able to fix this in a timely manner, I'm going to push this beyond our beloved 1.0 release. Set back to critical if you object.

Thanks evolvingweb for the review, by the way!

anarcat’s picture

Title: Set hosting site form options in the form constructor, before we deliver it to the client » Optimize the site form to deal with huge number of platforms and packages
Category: bug » task
anarcat’s picture

Title: Optimize the site form to deal with huge number of platforms and packages » site can be very slow with huge number of platforms and packages
Category: task » bug
ergonlogic’s picture

Version: 6.x-0.4-alpha3 » 7.x-3.x-dev
Dane Powell’s picture

Title: site can be very slow with huge number of platforms and packages » Site form can be very slow with huge number of platforms and packages

Renamed issue for clarity.

Would the number of deleted platforms also play a role in this? I only have 15 platforms on my Aegir development server, but 800 deleted platforms and 500,000 package instances, and the node/add/site form is annoyingly laggy.

It looks like hosting_get_profile_platforms() probably returns packages for deleted platforms as well, which would slow things down. As a workaround, I may write a module to remove deleted platforms (and associated package instances) from the database. This would shave 100MB off the db, and maybe improve the site form?

sfyn’s picture

Assigned: sfyn » Unassigned
chertzog’s picture

I posted a patch in #2061509: List of platforms is no longer updated to match install profile clicked at node/add/site that ajaxifies the site form profile/platform fields. This should help this i believe.

ergonlogic’s picture

Issue summary: View changes

Related to this, we should have js.module available now. So AJAX callbacks could potentially avoid a full bootstrap, and thus speed up slow refreshes,