Allow the selections in the dropbox to influence the hierarchy

Fixed one small bug I had missed.

I was able to test out the second patch (dropbox_influence_hierarchy.patch posted on 9/23 18:40). Note that the hierarchical_select.module RC3 version that I'd picked up (1.131 2008/08/27 16:21:44) was somewhat out of sync with the patch baseline that you'd used, but I was able to apply the patch anyhow. The patch didn't break any existing functionality.

The patch was against HEAD indeed. HEAD contains improvements over RC 3.

The testing that I did with a multiple select into the dropbox, did indeed provide me the ability to dynamically change the remaining selection hierarchy (i.e. removal of options based on one or more selections). I was able to remove both top-level hierarchy items as well as deepest level entries.

Great!

I still have to find a way to also remove mid-level hierarchy items (in case there are no items left at the deepest level) since it breaks the enforce_deepest model. E.g. if there are no valid selection options left at level 3 then level 2 becomes the "current" deepest for that particular hierarchy branch and selection is allowed (but doesn't make sense and shouldn't be possible in my case).

Let's elaborate on this.
- Why can't you remove mid-level hierarchy items? It should be possible, since mid-level hierarchy items are also retrieved through the _children() hook, the same hook as for deepest-level hierarchy items.
- Why does it break the enforce_deepest model? enforce_deepest is dynamic: it enforces the deepest level depending on how deep the tree actually goes. So in that sense, the current algorithms should still work on your dynamically changing hierarchy.

Note to self: dropbox-influenced hierarchies can NOT be cached, since items that previously had children, may not have children after being added to the dropbox. That might be causing problems, but only if you're using WebKit, because that's the only browser supporting HTML 5 databases.

Other than that it certainly does the job for me. And it pretty much eliminates 400-500 lines of custom javascript code that I had to create to do a similar selection in a standalone webpage.

Awesome :)

Four questions:
1) what is the reason not to provide the dropbox selection to hook_hierarchical_select_valid_item()? Rather than dealing with "removal" in 2 places in the code, select_valid_item would provide a single point for both root and children items.

valid_item() is only used to validate items in the dropbox, to ensure that the user didn't tamper with the data. Basically it just means "item does exist in the hierarchy". And you really can't do anything more advanced than this, because at this point, you can't pass in the dropbox, because that's the very thing that's being validated!
So in that sense, the user could still hamper with the data now: you could dynamically alter the hierarchy, but if the user would change this data to contain the same item twice, it would still be accepted. Then again, the user would have to change a serialized array. I could add a countermeasure to this by adding hash to guarantee no alterations I guess, or safer: store the selections in the dropbox on the server side. Right now I'm not sure anymore why I didn't do that in the first place, but I'm sure I had a good reason.
The point is: this is a design flaw at this point, but an acceptable one, and an understandable one because I didn't foresee this use case.

2) can you provide a bit more insight into the sequence of the hook_hs_ API calls that are made? I've noticed that sometimes root_select, child_select seem to get called several times for the same information (item level) in a single page rendering, potentially resulting in as many trips to the database. I'm trying to understand where I could or potentially need to optimize.

Use the HEAD version and then use Firebug (Firefox) or the Web Inspector (Safari) to get a basic performance analysis. Typically, you'd want to statically cache anything you get from the database, you should do that in any case, even when writing db queries not for the HS API.
The sequence of hook calls … well … that's pretty much impossible to say. Just look how many settings are available for HS, and know that each of those combinations of settings will result in a different sequence ;)

3) Also are there indeed restrictions with the item names that can be used in the name/value pairs that are passed in the hs API? As I mentioned in a separate email, when I used pure numeric labels for the top level hierarchy items, things started to break. If I prefix the item names with a letter (a-z), then everything is fine. This seems to be a type comparison or array referencing issue, but nonetheless very painful to debug.

Impossible. Taxonomy is also 100% numeric items and works just fine. You just have to make sure that each item in the hierarchy has a UNIQUE identifier. If you repeat the same number for different items, then yes, you will get in trouble. It's also possible that your _lineage() implementation is faulty.

4) this last one may turn into a feature request; would it be possible to change a selection into a "disabled" html option (i.e. a grey'd-out item in a selection list) rather than completely removing/omitting the item from the list? This may be more intuitive to the user since options remain visible, but it becomes clear that another selection changed the actual selectable options. I.e. items don't magically disappear, but visibly become "disabled" as a result of the user's selection.

This would require the ability to disable <option>s within <select>s. This is unfortunately not supported by HTML, so can't be done.
What I could do, is using an <ul> instead of a <select>, but then it would no longer be gracefully degradable. And would require special code for the case when JS is enabled. So … also a no-go. Definitely for the D5 version.

This is going to become a .1 feature.

Also, this patch needs work, because cache systems cannot be used when the dropbox can influence the hierarchy!

As long as you're not using Safari, client-side caching won't even work, so it won't affect you.

The save_lineage setting is independent from whether the dropbox will be passed or not. The only relevant setting is $config['dropbox']['status'], which must be set to 1.

In the docs:

 - save_lineage (optional, defaults to 0)
   Triggers the lineage saving functionality. If enabled, the selection can
   consist of multiple values.

So I'd actually say you never want save_lineage to be enabled, because then you wouldn't be able to make selections like:
- BMW > 6 series
- BMW > 7 series

Because if the first lineage would have been added to the dropbox (BMW > 6 series), then BMW would have been removed from the hierarchy, and therefore the user wouldn't be able to select BMW > 7 series.

Why do you insist that you don't get the dropbox content passed when save_lineage is disabled?

It's not necessary to do a review of the patch, the patch in itself is good. I didn't commit it right away because I didn't want to postpone a final release for HS 3 even further.
And yes, it's sad that I can't be very confident about my code. But truth be told: it's badly engineered. It's been extended *so* many times because *so* many settings have been added that all affect the hierarchy. It needs to be rewritten, OO-style, and unit-tested.

What I would appreciate, is if you could reroll the patch against the DRUPAL-5--3 branch. No worries: it's the same as the old HEAD branch. HEAD will now be used for the Drupal 6 port. Rerolling the patch should be fairly trivial. This patch will go in, since it works, as soon as I've tagged the final release of HS 3. So it'll be included in the first minor release.

Thanks for actively following up on this issue! :)

P.S.: the Safari issue, if it's related to client-side caching, can be fixed by disabling the "Cache in a HTML 5 client-side database" setting at admin/settings/hierarchical_select.

Postponing this due to lack of response.

Tagging for HS4. Included in the HS4 roadmap: http://drupal.org/node/1052670.