Taxonomy Multiple Hierarchy doesn't display children for all its parent

can you check out the fix I outline here: http://drupal.org/node/23884

http://cvs.drupal.org/viewcvs/drupal/drupal/modules/taxonomy.module?r1=1...

I guess we should reevaluate the last two chunks of this commit.

When I tested this, the problem only occured in the add term interface. Once you go back to the view tab, it displays correctly. I think it's a problem with how the select box is rendered. Obviously it's storing the information properly in the database because it displays correctly on the view tab. It should be fixed anyway.

Any fixes yet? Not being able to add multiple child terms seems like pretty a major bug. I'm running into this issue, and it's really starting to bug me.

While it appears that this bug is fixed in the latest CVS (and at some point before now: 2006-03-21 @~ 07:00 GMT-0800) in the creation and viewing of vocabulary terms, it still shows up in the creation and editing of posts. When choosing a term in a category, only the first choice under the multiply selected parents is shown. Also, it does not matter whether the vocabulary is Multiple select or not.

See carefully the attached image file:
1. When adding terms in a vocabulary, the parents does not list the terms correctly (eg. C#)
2. As you can see C# as two parents!
3. When editing a term it is *NOT* possible to change their parents (this is a huge flaw)
4. When adding a new content page that uses this vocabulary, it is not listed all terms hierarchy.

very confusing, this bug report is. But as I try to create and use a multi hiearchy I see the problems. I am fixing. Patch soon.

I would suspect that this is a bug in 4.7 as this looks like a form API bug to me. With $options being an array, options which have the same value can't appear more than once. The fix, however, is not difficult (kudos to Adrian for the microtime() index trick).

To replicate bugs, I created two terms called Parent1 and Parent2 in a multiple hiearchy vocab. Then added a child under both. Appeared only under the first (no surprise). After the patch it appeared under both.

Appeared -- but appeared where? In the taxonomy selector box for add term and 'categories' on node/add

Ajk reports that edit still does not work. Bother! said Pooh and submitted the fix.

It works now.

Was having this issue (not being able to edit parent terms for hierarcy taxonomies) but this patch fixed it rightup. Thanks much!

patching file includes/form.inc
Hunk #1 succeeded at 890 (offset 21 lines).
patching file modules/taxonomy/taxonomy.module
Hunk #2 FAILED at 1310.
1 out of 2 hunks FAILED -- saving rejects to file modules/taxonomy/taxonomy.module.rej

That hunk is already committed. Rerolled which removes the offset from form.inc as wel..

Why use microtime() instead of a simple numerically indexed array?

The object parsing should be documented in form.inc.

The use of objects is a bit weird. The closest parallel I can think of is 'data' keys in various themeable functions, but that wouldn't work here. Maybe using an array for the extra element would work instead? Maybe an object is best, but I'm not sure.

This patch is in the 'controversial but we have nothing better' category. I disagree with the need to comment in form.inc , this needs to be documented in fapi reference at #options which will happen if this gets in. I removed microtime().

Sorry, but why don't we have anything better? I don't understand.

And please do add a comment in form.inc explaining this.

I know you want lots of comments in form.inc and we often oblige but this simply does not belong there -- noone will even search for the comment there, as they need to know what to use in #options so they will look at the #options document. This function is actually an inside only one. I know, I know, it should be _form_select_value then.

Why we have no better? Well, look, #options is an associate array. We need to deal with the problem that sometimes the keys can repeat. And the value can't be an array, that's reserved for optgroups. So, how else do we solve this? I have asked Adrian and he has no better idea too. This is a bit... interesting :) but I would not call that bad. It's a very edge case anyways and the handling code is extremely small.

Yes, this needs to be documented in contrib's formapi section. Please do so soon since I just committed this to HEAD.

Kick ass!!! Nice one chx.

The form_select_hyper_multiple_2.patch from comment #19 introduces a validation bug with taxonomy (and likely any other select using objects in #options). The function form_flatten_options doesn't flatten the options correctly. The attached patch fixes this and allows the form to be submitted correctly. Please review and apply.

mmmm okay. I was thinking on this and I thought "hey if anyone tampers something which matches the microtime he is exremely lucky and it's not a tid anyways..." but then i was told to remove microtime and forgot to rethink the keys.

Committed to HEAD.

So do we want microtime(), or does this fix everything?

Uh oh. Derek tells me that if someone wants to manipulate the new taxonomy form then he is stuck. A little helper is badly needed here. Yes this is a new API function for Drupal 5.x but it simply was left out from the patch above and if you want to manipulate taxonomy form, you will need this.

this is great. it works as advertised, and saves me the trouble of duplicating a similar helper method in both cvs.module and project_release.module (both of which need something like this -- see http://drupal.org/node/105558 and http://drupal.org/node/101150#comment-168755).

new patch that strips out some extra newlines that snuck into chx's version. otherwise, identical code.

thanks!
-derek

p.s. the original bug was critical, this pain in the ass isn't, but i'd still love to see it get in before 5.0 final. ;)

minor typos in the doxygen... 1 sec.

Patch in #32 adds a working, convenient little helper function that will be needed by all modules manipulating #options. Therefor, my +1 for committing to 5.x.

Yes, this API function is badly needed. Committed to HEAD.

changing title back for posterity.

also, FYI: i'm adding a blurb to the update docs about this (and the new method) right now. it'll be at http://drupal.org/node/64279#options once it's done (in a few minutes).

thanks, all!
-derek

the more i think about this (and the act of documenting it) i'm now a little worried about how the code currently stands.

a) the whole point of this thread is that you want to support N select labels with the same key. as currently in CVS, this helper just returns you the *first* one it finds, which isn't necessarily what you want.

b) this helper *only* works on #options arrays that have the crazy object stuff (since we're explicitly testing for "isset($object->option[$key])" which normal #option arrays won't have). so the usability of it is rather limited it seems. you have to know in advance that the array you're dealing with has the objects, or go out of your way to test the return value === FALSE and then try to see if the key you're looking for is just a normal, non-object value. :( what i documented at http://drupal.org/node/64279#options is not exactly ideal usage...

after consultations with chx to make sure i'm not crazy, here's a new version of the function. it solves both of the above limitations of the current version. since my code is (so far) the only one making use of this helper, i don't mind (and have already implemented/tested) changing it to use the new interface.

apologies for being hasty in the first place. :( at least i'm documenting all the changes as they come, so no one can complain about that. ;)

some commit to form.inc just broke the old patch. rerolled to ensure no conflicts.

Aye. This is much better than the previous version and actually does what it was intended for.

Love the PHPdoc but I think it could be slightly more explicit as when this helper function is useful. I understand it returns an array of options that match, but in what cases do I want to use this? It is the kind of information that is obvious to you, but not to the person who never saw this function before.

Also, isn't the function name a bit misleading? The name suggest that we return a single key, but in fact, we are returning multiple indices. Odd.

(IMO, it was too late for API additions. The fact that API additions can be problematic is illustrated by this "patch chain".)

i agree it was late/hasty, my apologies for that. all the bugs/problems i found in 5.x core from porting project* have taught me the value of porting early and porting often, so i would have noticed these things earlier and had more time to fix them properly. live and learn...

i'd really like to avoid a long series of dww-rolls-new-patch-and-asks-for-review, chx RTBC's, someone marks "needs work", repeat...

i'm happy to rename the function, but let's just decide on a name, i'll code it up, and we're done. seems like "options" should be in there, since this is potentially returning more than one (Dries's concern), and it's specific to #options elements...

proposals for names:

a) form_options_key()
b) form_options_for_key()
c) form_get_options_key()
d) form_get_options_for_key()
e) form_find_options_key()
f) form_find_options_for_key()

sadly, http://api.drupal.org/api/HEAD/file/developer/topics/forms_api_reference... doesn't actually specify a standard, agreed-upon terminology for these arrays, but it seems like "key" (return value) and "label" (display value) are in common use, and this function is definitely searching by key, not label.

i'd probably vote for (b), but i'd be basically happy with any of them.

i'll also try to make the comment more expressive about why you might want to use this function, that's a good idea.

in my case, it's that project_release.module and cvs.module both need to alter the default taxonomy selector on the release node form for the special vocabulary created by project_release.module (by default called "Project release API compatibility" but on d.o, called "Drupal Core compatibility"). in most cases, the proper value of the "Drupal core compatibility" is already determined by the CVS tag selected on the first page, so we need to search through the array of #options provided by taxonomy.module and only keep the 1 value that's valid, given their choice of CVS tag. in some cases (when adding release nodes pointing to HEAD), we do want to provide a set of choices, but then we still need to alter the array to deal with the fact that some of the taxonomy terms can be disabled by an admin setting (e.g. to remove "4.6.x" from the list of available options, even though the taxonomy term itself shouldn't be deleted)...

i'll condense this down and make it more general for the doxygen.

thanks,
-derek

in case you're thinking of just undoing the change...

it's possible that project* + cvs.module are the only ones that ever try to alter the choices in taxonomy arrays like this. i'd certainly be willing to just move this function into project.module, and share it amongst my modules there, if you just want to back out this patch entirely.

however, i truly suspect others might be trying to do similar things (or trying to alter some other form element with the same crazy object-based #options array). D5 porting progress is certainly being made, but we're far from done, and there will be lots of new modules developed for D5. therefore, this helper will almost certainly be useful to others, which is why i'm still advocating for fixing the version now in there, instead of ripping it out completely. the code itself is fairly small (it's not really bloating core), but big enough that it will suck to replicate everywhere you might need it.

plus, after going through 2 iterations of the interface to the function, having something in core that actually encourages correct usage (by returning an array of index values, not just the first you find) will help prevent buggy code and force the callers to loop over an array instead of taking incorrect short-cuts with single values. they'll at least be forced to consider that there might be multiple indexes returned, so they have to think about what that means and how they want to handle it...

helper functions that not only help to keep code smaller, but also more correct, seem like a very good thing. ;)

thanks,
-derek

ok, it's not the patch that needs review, but comment #42 regarding what the new name of this function should be... ;)

thanks,
-derek

I'd go with form_get_options($key).

after discussion with Dries, new patch:
- renames the function to form_get_options()
- adds more explaination to the doxygen.

chx noticed 2 problems:
1) logic bug: the middle case of else if was too restrictive, so the last case could fire when it shouldn't. we really need the middle one to just test if the choice is an object or not, so the last only fires if it's neither an array nor an object. inside the middle case, we also test if it's the *right* object, and if so, add it to our array of results.
2) typo in the doxygen.

both are now fixed.

I ran out of problems with the patch :)

hehe, except i left a syntax error in there. ;) now, it's *really* RTBC, i promise...

Committed to CVS HEAD. Thanks.

thanks, Dries!
settings this issue's characteristics back to the original bug for posterity...

I'm having very similar problem with Drupal 4.7.4 (w/security patches applied) :
When I list the terms I could see all children terms for multiple parents, however when I'm trying to add a new content, some parents missing their children terms.
Is there a patch for 4.7.4 available?
Not sure if this is a correct place to post this question.
Thanks!

I'm having the same issue with 4.7.6 version. Is there a bug fix out there for this?