I submitted a but with the same title for 4.6.

I just installed the cvs version and it doesn't handle multiple parent/hierarchy correctly either.

Multiple hierarchy is doesn't correctly display the children term for all its parent terms, but only display the first children term below the first parent it finds.

Effectively, multiple parent linking doesn't work and the child below the lowest tid is displayed...

ex.

Should be...

Languages
--Object Oriented
----Java
----C++
----Smalltalk
--Procedural
----C++
----C

Bug...

Languages
--Object Oriented
----Java
----C++
----Smalltalk
--Procedural
----C
####Missing "C++" ####

Comments

samo’s picture

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

John Hwang’s picture

samo,

your post addresses another issue where nodes within a taxonomy term is not displayed when the taxonomy/term/# is called... I'm not sure if your patch is correct but it looks like the parameters are not right and your code fixes that.

This bug is related to a bug in _taxonomy_term_select(), i believe, where the options[] is not contructed correctly. I print_r($tree) right before $options is constructed but I haven't figured out how the code in 4.6+ gets it wrong... I'm comparing 4.5's taxonomy.module and 4.6's taxonomy.module to figure out what's wrong.

Who wrote the refactored code in 4.6 for _taxonomy_term_select()? Some help would be appreciated...

killes@www.drop.org’s picture

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

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

John Hwang’s picture

I'm bumping this one up again... This still doesn't work in 4.7/CVS

this is has not been fixed til now... Could someone please fix this?

Multiple select just doesn't work.

jo1ene’s picture

Priority:Critical» Normal

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.

bigbman’s picture

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.

oadaeh’s picture

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.

magico’s picture

Priority:Normal» Critical
StatusFileSize
new81.19 KB

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.

chx’s picture

Assigned:Unassigned» chx

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.

chx’s picture

StatusFileSize
new1.07 KB

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).

chx’s picture

Status:Active» Needs review

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.

chx’s picture

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

chx’s picture

StatusFileSize
new1.42 KB

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

AjK’s picture

Status:Needs review» Reviewed & tested by the community

It works now.

rszrama’s picture

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

drumm’s picture

Status:Reviewed & tested by the community» Needs work

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

chx’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new1.17 KB

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

drumm’s picture

Status:Reviewed & tested by the community» Needs work

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.

chx’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new1.16 KB

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().

Dries’s picture

Status:Reviewed & tested by the community» Needs work

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

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

chx’s picture

Status:Needs work» Reviewed & tested by the community

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.

drumm’s picture

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

drumm’s picture

Status:Reviewed & tested by the community» Fixed
RobRoy’s picture

Kick ass!!! Nice one chx.

vhmauery’s picture

Status:Fixed» Needs review
StatusFileSize
new553 bytes

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.

chx’s picture

Status:Needs review» Reviewed & tested by the community

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.

drumm’s picture

Status:Reviewed & tested by the community» Fixed

Committed to HEAD.

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

Anonymous’s picture

Status:Fixed» Closed (fixed)
chx’s picture

Title:Taxonomy Multiple Hierarchy doesn't display children for all its parent» Taxonomy form become (almost) immutable
Version:x.y.z» 5.x-dev
Component:taxonomy.module» forms system
Status:Closed (fixed)» Needs review
StatusFileSize
new872 bytes

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.

dww’s picture

Priority:Critical» Normal
Status:Needs review» Reviewed & tested by the community
StatusFileSize
new1.01 KB

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. ;)

dww’s picture

Assigned:chx» dww
Status:Reviewed & tested by the community» Needs work

minor typos in the doxygen... 1 sec.

dww’s picture

Status:Needs work» Needs review
StatusFileSize
new1.03 KB
Heine’s picture

Status:Needs review» Reviewed & tested by the community

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.

Steven’s picture

Status:Reviewed & tested by the community» Fixed

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

dww’s picture

Title:Taxonomy form become (almost) immutable» Taxonomy Multiple Hierarchy doesn't display children for all its parent
Priority:Normal» Critical

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

dww’s picture

Title:Taxonomy Multiple Hierarchy doesn't display children for all its parent» form_get_option_key() isn't really right
Priority:Critical» Normal
Status:Fixed» Needs review
StatusFileSize
new2.08 KB

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. ;)

dww’s picture

StatusFileSize
new2.08 KB

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

chx’s picture

Status:Needs review» Reviewed & tested by the community

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

Dries’s picture

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.

Dries’s picture

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".)

drumm’s picture

Status:Reviewed & tested by the community» Needs work
dww’s picture

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

dww’s picture

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

dww’s picture

Status:Needs work» Needs review

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

thanks,
-derek

Dries’s picture

I'd go with form_get_options($key).

dww’s picture

Title:form_get_option_key() isn't really right» rename form_get_option_key() and fix its behavior
StatusFileSize
new2.75 KB

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

dww’s picture

StatusFileSize
new2.76 KB

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.

chx’s picture

Status:Needs review» Reviewed & tested by the community

I ran out of problems with the patch :)

dww’s picture

StatusFileSize
new2.77 KB

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

Dries’s picture

Status:Reviewed & tested by the community» Fixed

Committed to CVS HEAD. Thanks.

dww’s picture

Title:rename form_get_option_key() and fix its behavior» Taxonomy Multiple Hierarchy doesn't display children for all its parent
Assigned:dww» Unassigned
Priority:Normal» Critical

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

Anonymous’s picture

Status:Fixed» Closed (fixed)
firstov’s picture

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!

ebrophy’s picture

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