I am using Content Taxonomy with taxonomy T in a content type C. T is also assigned to C in core, but the selection boxes at the top of the node edit form where hidden, because the Content Taxonomy field has the core integration option enabled.

Enabling HS Content Taxonomy or HS Taxonomy causes the boxes to show up again. HS Taxonomy is not enabled for T, only the module is enabled, because HS Content Taxonomy requires it. Am I missing some settings? Is there any workaround?

Comments

wim leers’s picture

Component: Code - Taxonomy » Code - Content Taxonomy

I have no idea. I won't track down this bug (I don't have time for it). Hopefully somebody else will.

bibo’s picture

Status: Active » Patch (to be ported)
StatusFileSize
new613 bytes

This simple patch for "hs_taxonomy.module" should fix the problem. Basically it's calling the form_alter of "content_taxonomy" after it has done it's normal form altering.

I guess setting the module weight of content_taxonomy to be greater than "hs_taxonomy.module" would have worked also.

wim leers’s picture

Status: Patch (to be ported) » Needs work

The weight of content_taxonomy or hs_content_taxonomy?

The solution you proposed may work, but is unacceptable. Changing the module weights sounds much better :)

bibo’s picture

The weight of content_taxonomy or hs_content_taxonomy?

Eventhough the problem concerns the HS+CT integration, I didn't mean altering hs_content_taxonomy, at all.
I did some fast tests, and both my patch, and setting "content_taxonomy" weight to 3, do work.
But I guess altering a 3rd party module's weight is not good solution.

On the other hand, you could set "hs_content_taxonomy" and "hs_taxonomy" a lower weight than content_taxonomy
(which is a default 0), so they run before content_taxonomy... But if their weight is less than 0 they also run before
lots of other modules that probably should run before them (such as taxonomy, forum etc).. or am I mistaken?
There must be a reason why "hs_taxonomy" weight is 2 instead of 0? Altering any weights might mess up
lots of things with a million other modules..

I know you're well aware of all this, but do you know if there's an elegant solution to go around this -
without altering the 3rd party content_taxonomy weight?

The solution you proposed may work, but is unacceptable. Changing the module weights sounds much better :)

I understand running another module's form_alter would usually be a big no-no... but could you reconsider this option
given the following additional information
:

content_taxonomy-package has several modules with various form_alters, but the form_alter of the base module
content_taxonomy only hides the taxonomy fields (and more specifically, only the ones that are handled as CCK-fields).

The full form_alter looks like this:

/**
 * Implementation of hook_form_alter().
 *
 * hides the taxonomy form if there exists a content taxonomy field, which is set to 'hide default taxonomy fields'
 */
function content_taxonomy_form_alter(&$form, $form_state, $form_id) {
  if (isset($form['type']['#value']) && $form['type']['#value'] .'_node_form' == $form_id && isset($form['taxonomy'])) {
    if (!is_array($form['taxonomy'])) {
      // empty taxonomy arrays can cause errors on preview
      if ($form_state['post']['op'] == t('Preview')) {
        $form['taxonomy'] = array('#tree' => TRUE);
        $form['taxonomy']['key'] = array('#type' => 'value', '#value' => '');
        return;
      }
    }
    $_content_taxonomy_vids = array();
    $info = _content_type_info();
    $content_type = $info['content types'][$form['type']['#value']];
    foreach ($content_type['fields'] as $field) {
      if ($field['type'] == 'content_taxonomy' && (!in_array($field['vid'], $_content_taxonomy_vids))) {
        $_content_taxonomy_vids[] = $field['vid'];
      }
    }
    _content_taxonomy_taxonomy_unset($form['taxonomy'], $_content_taxonomy_vids);
    
    //hide empty 'Vocabularies' fieldsets
    $empty_fieldset = TRUE;
    if (is_array($form['taxonomy'])) {
      foreach ($form['taxonomy'] as $key => $value) {
        if (is_array($value) && !empty($value)) {
          $empty_fieldset = FALSE; 
          break;
        }
      }
    }
    if ($empty_fieldset) {
      // creating an empty taxonomy array is causing errors on preview in the taxonomy module
      // so we create an empty value field as placeholder, which is going to prevent the errors
      $form['taxonomy'] = array('#tree' => TRUE);
      $form['taxonomy']['key'] = array('#type' => 'value', '#value' => '');
    }
  }
}

A third solution would be to just copy paste that whole code at the end of hs_taxonomy_form_alter(), instead of my suggested patch:


  if(module_exists('hs_content_taxonomy')){
	content_taxonomy_form_alter($form, $form_state, $form_id);
  }  

I know the form_alter of content_taxonomy might change later, and could cause problems then.. still those few lines looks rather clean compared to the above, right :p?

bibo’s picture

Status: Needs work » Needs review
StatusFileSize
new2.29 KB

Ok, so how about this patch?

Code-wise is pretty much the same, except I just duplicated code from content_taxonomy_form_alter() to be run
in hs_taxonomy.module:s form_alter (after the normal altering). The only downside should be code duplication.

The patch is made against the latest dev version (2009-Oct-19).

TripleEmcoder’s picture

What is the core issue here? Is it HS that makes the core taxonomy fields reappear (why? could it perhaps detect that content_taxonomy is active?) or is it content_taxonomy that does not recognize core taxonomy fields altered by HS as, well, core taxonomy fields (why? wouldn't it still be better to add support to recognize them there than to duplicate code?)?

bibo’s picture

What is the core issue here? Is it HS that makes the core taxonomy fields reappear

In short, yes. Or rather, its not the core fields reappearing, but the overridden HS values.

(why? could it perhaps detect that content_taxonomy is active?)

If you look at the patch this line does exactly that:

   if(module_exists('hs_content_taxonomy')){

The new code is only executed if hs_content_taxonomy is installed and enabled. And since hs_content_taxonomy depends on content_taxonomy (and hierarchical_select and hs_taxonomy), those
have to be enabled as well.

or is it content_taxonomy that does not recognize core taxonomy fields altered by HS as, well, core taxonomy fields (why? wouldn't it still be better to add support to recognize them there than to duplicate code?)?

Content taxonomy can't recognize things set by HS, since those will be only added after content_taxonomy has run its code. That is because hierachy_select is run after content_taxonomy, and also hs_taxonomy (weight ) is run later. hs_taxonomy is also even later than hs_content_taxonomy. Setting content_taxonomy module height higher would make it run later and then automatically hide the taxonomy fields set by hierachy_select. However, this could mess up pretty much anything, and is imo not a solution.

The code duplication from content_taxonomy_form_alter() is not that much code, and it simply works.

Altering module weigths can get very tricky. For example, check this install script of hierarchical_select:

/**
 * Implementation of hook_install().
 */
function hierarchical_select_install() {
  // Ensure the Hierarchical Select module runs after the Taxonomy and Content
  // Taxonomy modules! This should not be necessary to do, but apparently some
  // idiot module developer is changing the weight of the Taxonomy module...
  $weight = db_result(db_query("SELECT weight FROM {system} WHERE name = 'taxonomy'"));
  $weight = max($weight, db_result(db_query("SELECT weight FROM {system} WHERE name = 'content_taxonomy'")));
  // Also ensure the Hierarchical Select module runs after the i18ntaxonomy
  // module.
  $weight = max($weight, db_result(db_query("SELECT weight FROM {system} WHERE name = 'i18ntaxonomy'")));
  // Also ensure the Hierarchical Select module runs after the og_vocab module.
  $weight = max($weight, db_result(db_query("SELECT weight FROM {system} WHERE name = 'og_vocab'")));
  // If none of these modules was already enabled, the weight will still be
  // incorrect. Therefore, let's make the minimum weight of Hierarchical
  // Select 15.
  $weight = max($weight, 15);

  // Set the weight one higher than the highest weight we've encountered, so
  // that Hierarchical Select will run after it.
  $weight++;

  db_query("UPDATE {system} SET weight = %d WHERE name  = '%s'", $weight, 'hierarchical_select');
}

memfis, could you please try my patch and report if it works for you too, so we maybe get this committed to the module?

hefox’s picture

Experiencing the same issue

My views are going to be ...picky but here's a few opinions on this issue (not the patch specifically, the issues leading to it):

In some respects, a parent module should not know about a sub module.

Also, the hs_taxonomy module changes things that people wanting to use hs_content_taxonomy might not want changed, like parent addition on term edit pages, so having it as a sub module, meh!

Duplicate code is generally a bad idea (ie fix something somewhere, forget it elsewhere.. eek!), but making a general shared function is cool:>.

Probably a bad idea to change unrelated items; I'm weary about how hs_taxonomy blows up the taxonomy form element and re-does it; that is somewhat the issue here I think? If instead it only blew up the taxonomy for the vocabularies it was enabled for and checked the elements were in the forum still, the problem would be gone right?

current workflow:
1) remake taxonomy (which also is problematic for other modules trying to effect taxonomy element, like modules that also change one of the input elements. Makes the weight issue so more messy?)
2) add in some hs changes
instead
1) all hs content_taxonomy code in it's own module only effecting itself.
2) add in some hs changes to relevant form elements based on vocabulary
3) check if fieldset should be removed from taxonomy element.

I'm willing to try making patches, but I'm not up on some of the issues that lead to the current issue ... Or alternitly bibo's patch, if it works then go with that :>. (Haven't tried it, I did some simple changes since I don't have forms that both have hs_content_taxonomy field and taxonomy).

Sorry if I sounded mean/picky/etc.

bibo’s picture

My views are going to be ...picky but here's a few opinions on this issue (not the patch specifically, the issues leading to it):

In some respects, a parent module should not know about a sub module.

I'm not sure what you mean with parent and submodules. As I see in Drupal there is no clear module hierarchy (except in this specific module name, hehe), there are just dependencies and order of execution (order being alphabetic and module weights). I guess you ment hs_taxonomy.module as sub, since it depends on "hierarchical_select" and "taxonomy" . Or hs_content_taxonomy that depends on the same + hs_taxonomy and content_taxonomy.

Hmm. Since all the hs-modules are in the same project (and package), imo they should know about each other. And most big projects support/integrate with each other also... but this isn't the problem, and a bit offtopic, isn't it?

Duplicate code is generally a bad idea (ie fix something somewhere, forget it elsewhere.. eek!), but making a general shared function is cool:>.

Yeah, and that function would be content_taxonomy_form_alter(), as I suggested in my first patch. That version would limit to patch to a few lines (or only one if you want). But it's not exactly shared (or normally not supposed to be used by other modules). The function just happens to be doing exatcly what is needed, but since it is out of this module's control, and it might change, it is "unaccaptable" (or so I understood). Thus duplicating code, it's not that many lines anyway.

Probably a bad idea to change unrelated items; I'm weary about how hs_taxonomy blows up the taxonomy form element and re-does it; that is somewhat the issue here I think? If instead it only blew up the taxonomy for the vocabularies it was enabled for and checked the elements were in the forum still, the problem would be gone right?

Since HS is actually ment to alter the form UI of the (content) taxonomies, I believe the correct way is to use the form API the way it does it. Not that I have gone through all the code.. But the logic isn't likely to change (not for this contrib module anyway :p). The module execution order is important tho, since the last one gets to override what ever it wants to.

current workflow:
1) remake taxonomy (which also is problematic for other modules trying to effect taxonomy element, like modules that also change one of the input elements. Makes the weight issue so more messy?)
2) add in some hs changes
instead
1) all hs content_taxonomy code in it's own module only effecting itself.
2) add in some hs changes to relevant form elements based on vocabulary
3) check if fieldset should be removed from taxonomy element.

Remake taxonomy is a typo, right? Since changing the core taxonomy is not possible. Not for Drupal 6 and not even for 7 since it's in code freeze allready :d. Ah I guess I again just didn't understand you.. or you didnt mean the module, but just the $node->taxonomy?

But I understood the latter 3 steps. If you mean that "hs_content_taxonomy" would include this code instead of "hs_taxonomy", I just realized that might also be possible. So far I thought "hs_content" weight is 2 so it runs after "hs_taxonomy_taxonomy" (weight 0), and thus also "hs_content_taxonomy" weight shouldn't be altered. But I just read hs_taxonomy_install():

  // Update the module weight to 2 so hs_taxonomy runs after the forum module.
  // See http://drupal.org/node/475784.
  $ret[] = update_sql("UPDATE {system} SET weight = 2 WHERE name = 'hs_taxonomy'");

Since the weight is because of "forum"-module, I assumed wrong.

So, to change this code the more logical module (and the one set as "component"), this would work: add the patch changes from the end of "hs_taxonomy_form_alter()" to the end of hs_content_taxonomy_form_alter()-function. And also add a new file: "hs_content_taxonomy.install":

/**
 * Implementation of hook_install().
 */
function hs_content_taxonomy_install() {
  $ret = array();

  // Update the module weight to 3 so hs_content_taxonomy runs after the hs_taxonomy module and can hide fields
  // See drupal.org/node/601500
  $ret[] = update_sql("UPDATE {system} SET weight = 3 WHERE name = 'hs_content_taxonomy'");

  return $ret;
}

I won't make a patch for that tho, unless someone wants it. Wouldnt change the result in any way.

I'm willing to try making patches, but I'm not up on some of the issues that lead to the current issue ... Or alternitly bibo's patch, if it works then go with that :>. (Haven't tried it, I did some simple changes since I don't have forms that both have hs_content_taxonomy field and taxonomy).

If you don't have both in use for the same nodetype, this problem wouldn't concern you at all, right?

Would you (or just about anyone) please just try my patch - I know it works fine and causes no problems. The code just wont get any attention (=wont get committed) until someone else reports back about it working or not.

To save a bit time, I attached the patched version of the hs_taxonomy-module (for dev 2009-Oct-19-version).

hefox’s picture

StatusFileSize
new6.56 KB

Hm, here's my not very tested alternate patch.

The most important part in it is I get rid of the override taxonomy selector. If one was completely overriding every taxonomy selector, that'd seem like a great thing to do, however what it ended up doing (from what I could see) is essently repeating taxonomy_form_alter.

Why I saw this as an issue
1) Repeating code makes fox a sad panda. (not that fox doesn't tend to do that, but fox is a hippo sometime, and not even a hungry hungry hippo at the moment, but a sleepy, sleepy hippo). This is problamic for when the code gets updated; vocabulary->help was updated in core (run through a special parser) but this code wasn't updated (not yelling at maintainer for that! just saying repeating code leads to this type of stuff!).

2) Bulldozers other module's code, for example i8(whatever)taxonomy, the taxonomy translate module(which basically does similar to this module on repeating code from what I could see based on lullubot api's current api of the code, again, sad panda :( ), translates help, descriptions, etc. based on settings. I see that in hs it tries to componsate for this by re-doing the translation, but it misses translating the fields I think. Then, it misses any other module it doesn't know about changes, like content_taxonomy, ie this issue).

So, if it remains as is it'll likely have to keep changing to support any other module that alters $form['taxonomy'], or some delicate balance of weighting, and also has to be kept updating if core changes again (unlikely). (can't think of any contrib modules atm. For example site specific modules people [well, i at least] use to change stuff like the fieldset name would need to be weighted above hs_taxonomy.).

So this patch gets rid of overriding taxonomy selector and instead goes through $form['taxonomy'] (still needs to be above taxonomy weight wise) to look for vocabularies to transform. (With this, idealy content_taxonomy runs before in this case, so it doesn't process anything that content_taxonomy will remove, and that is likely +1 for performance). Instead of reloading the vocabulary, it uses the form element information to set it's information, so if a module has done changes it'll be kept (needs to run after i8(whatever)taxomy also so it does it's changes because i8(whatever)taxonomy overrides the taxonomy[$vid] form also :/)

Looking at it, I suspect I'll be making a feature request about making optional overriding term edit since it's too much shared to seperate hs_content_taxonomy and hs_taxonomy, but I'll see how this issue goes.

Again, haven't tested much yet and not with i8(whatever)taxonomy, but logically it seems likely to work.

BTW, this is probably a Code -- Content Taxonomy issue, but I dislike changing other people's issues. I believe Code -- Content taxonomy is for the hs_content_taxonomy module, where the issue here is resulting form the hs_taxonomy module. (Though the issue is effecting the operations of the content_taxonomy module).

Again, sorry if I come out as picky or mean or tired as **** :>

TripleEmcoder’s picture

What is the core issue here? Is it HS that makes the core taxonomy fields reappear

In short, yes. Or rather, its not the core fields reappearing, but the overridden HS values.

So, why does HS blindly override these fields instead of looking at the $form it gets and only overriding the fields that are still there after whatever module has run before HS (like content_taxonomy).

I think this is the approach proposed by hefox, am I right?

or is it content_taxonomy that does not recognize core taxonomy fields altered by HS as, well, core taxonomy fields (why? wouldn't it still be better to add support to recognize them there than to duplicate code?)?

Content taxonomy can't recognize things set by HS, since those will be only added after content_taxonomy has run its code. That is because hierachy_select is run after content_taxonomy, and also hs_taxonomy (weight ) is run later. hs_taxonomy is also even later than hs_content_taxonomy. Setting content_taxonomy module height higher would make it run later and then automatically hide the taxonomy fields set by hierachy_select. However, this could mess up pretty much anything, and is imo not a solution.

I am definately not suggesting messing up with module weights. What I meant was something like detecting if content_taxonomy is active as a module or perhaps better an API in content_taxonomy that would give a green light for HS to override?

But it seems that this would be almost the same as detecting active fields from $form, only with less impact on interdependencies.

TripleEmcoder’s picture

Component: Code - Content Taxonomy » Code - Taxonomy

Hefox, I see you deleted a lot of code from HS Taxonomy in your patch ;) That amount scares me a bit, but as far as I can tell these are good changes. I didn't know that HS Taxonomy actually rebuilds the taxonomy part of the form from scratch. Your approach is much better, in my opinion it's actually the only correct and logical one. Are there any historical reasons for the current approach?

I agree that we should move this issue to Code - Taxonomony. Someone with better understanding (than me) of HS Taxonomy code should review this patch. I'll be happy to test once we agree on the approach.

hefox’s picture

Oh wow, I took over 10 minutes writing my last post (based on that bibo posted during the time I was writing so I never noticed their reply).

Anyhow, bibo my later post might be more clear. Not going to answer everything cause I'm lazy and probably explained most in the last post, but this issue was effecting anyone that uses content_taxonomy weighted below hs_taxonomy since hs_taxonomy is what was creating $form['taxonomy'] (so content_taxonomy trying to remove it failed since it's added back in).

To memfis.. yes.. I did delete a lot of code; however most of the code I deleted was duplicate code directly from the taxonomy module :P.

It's in the README (also with some what I'd consider scary advice to hack core). As far as I can read on why they did it that way, it's related to performance issues. D6 provides that variable (override taxonomy whatever) for modules to be able to completely take over taxonomy form elements, which is a cool idea but when used like here causes issues with other modules expecting to be able to edit $form['taxonomy'] at a certain time and expect their edits to stay.

hefox’s picture

Title: HS (Content) Taxonomy support unhides core taxonomy selection boxes » HS Taxonomy interaction with other modules (content_taxonomy, unhides taxonomy fields)

Tried changing the title to reflect (my) current understanding of the issue.

Mostly just noting my patch needs an update run (to reset override taxonomy back to false.)

hefox’s picture

StatusFileSize
new6.57 KB

swt, and here's a patch that doesn't cause foreach loop warning; forgot to return the array() at the end of it.

bibo’s picture

Hefox, your patch is pretty big (compared to the 3 new lines required for my patch to work). Unfortunately I'm working too close to a deadline to test or work with that. I'll just do this fast post.

Does your patch actually address the fieldset unhiding (that is: fix it) or not? If not, you should probably make a new issue. It's great if you can fix for example translation issues (which I haven't noticed yet, but I will also need those fixed).

hefox’s picture

Yes, it fixes the fieldset issue.

This whole issue is that hs_taxonomy was remaking the taxonomy array (the fieldset). My method only edits on valid entries in the taxonomy array, not adding or removing anything, so it honors whether content_taxonomy has removed some entries.

Though, I just realized there's one issue with free tagging vocabularies (they don't have the options array, oops).; I'll look into that tonight if I have time.

hefox’s picture

Just realized that shouldn't be an issue; people should just be advised not to have their hierarchical select vocabularies set to tags also, since it's a pointless setting and it makes things more complicated.

for example, taxonomy translation, if the taxonomy is tags it won't get translated, etc.

TripleEmcoder’s picture

How does that event work originally, I mean HS + tags taxonomy? I suppose that a feature being pointless is not good enough an argument to brake something in a patch (or not fix entirely).

I should manage to test the patch tonight.

hefox’s picture

  if ($form_state['values']['hierarchical_select']['status']) {
    form_set_value($form['tags'], 0, $form_state);
  }

Ooo, hs_taxonomy is already setting tags to 0 when it's enabled =)! so hs_taxonomy already took care of that.

So the part in my patch that goes over the $form['taxonomy']['tags'] should be removed.
ie

 if (is_array($form['taxonomy']['tags'])) {

All of that part, though atm it is not hurting anything, it's not needed code.

Which my alterations to hs_content_taxonomy in a different issue, making further alterations to the patch is quite.. annoying atm, so I'd be much happy if someone could make an updated patch u.u. If not I'll make it :(.

benone’s picture

Hi, I did not install any patch from here.
What I did:
Taxonomy > Edit vocabulary > Content types: UNCHECK ALL

And now I have only CCK HS Taconomy field in my form.
In this field settings page there is still option - Save values additionally to the core taxonomy system (into the 'term_node' table).
So I CHECKED it.

Is the the choosen category saved also in core taxonomy table, if in Vocabulary settings there is no Content type choosen ?

Thanks

benone’s picture

I am answering to my question.

YES. In core taxonomy the category I choosed is saved. I checked the table in phpmyadmin.

So my question is - what for is Content type choosing in Vocabulary settings ?

hefox’s picture

The core taxonomy module handles basic adding taxonomy to node forms; the per content type settings determines what content types that taxonomy will show up on.

Content taxonomy is an add on module, which expands on taxonomy, but is not necessary. It makes taxonomy a cck field with more flexibility. It hides the field that core taxonomy adds in. As you mentioned, taxonomy does not actual need the content types settings when using content taxonomy. Personally I use them anyway as description so I can easily remember which taxonomy is on which content type.

Un-checking all content type settings for taxonomy that are having troubles is a good temporary solution, but being that many people have those settings it's should be addressed. Also, in my opinion, there's certain other issues my patch is addressing with interaction with other modules (ie, translation and using other taxonomy overrides.)

benone’s picture

Right. Temporary solution. Anyway, I will try to install your patch to have it done in a proper way.
Could you point me a link to the patch ? I don't know which one from here is the last , working version.
Many thanks for your explanation.

hefox’s picture

StatusFileSize
new6.43 KB

Newest patch

While editing out the tags for each I also made some other changes; instead of creating a new forum element I use a old one and just change the relevant settings with the intent to save any other changes a module may want to add.

benone, you may unfamiliar with testing patches, etc., you may want to wait till the maintainer weights on my patch . My patch is sort of a big change; or rather, it removes a lot of code and does thing a bit differently then the original module. But if you want to test it, test away :).

hefox’s picture

(Oh right, and if anyone does test it, remember to run update 2 under update.php for hs taxonomy).

benone’s picture

Ok, I will wait. Thanks.

wim leers’s picture

Status: Needs review » Reviewed & tested by the community

I read through all of the comments above. At #10, things started to go wrong, unfortunately. hefox had noticed the use of taxonomy_override_selector and assumed this was an ugly core hack. I agree that it looks like it, and in a sense it *is* a core hack. However, it's an officially supported feature of core, but unfortunately an undocumented one.

However, you can find this section in the README:

Maximum scalability
-------------------
hs_taxonomy takes advantage of the taxonomy_override_selector variable to
improve scalability: the whole tree is no longer loaded by Drupal core.
While the hs_book, hs_menu and hs_subscriptions modules override existing form
items, those form items are *still* being generated. This can cause scalability
issues.
If you want to fix this, you *will* have to modify the original modules (so
that includes Drupal core modules). Simply move the changes from the
hook_form_alter() implementations into the corresponding form definitions.

I fully agree that the code is ugly, but there is just no other way. catch (and I think also pwolanin) opted for this method at the end of the D6 cycle to at least make it *possible* to make this more scalable without hacking core, at the cost of copy/pasting a ton of code.

So this approach will absolutely not go in, for scalability reasons.

Next is the module weight approach, which also seems to have problems.

Finally, there is the initial approach suggested by bibo (the patch in #2). While it's not very elegant, it's the most elegant solution of all and gets the job done well.
I'd like your feedback before I commit the patch in #2.

P.S.: hs_content_taxonomy depends on hs_taxonomy because it reuses hs_taxonomy's implementation of the HS API. I.e. it reuses it exactly to prevent code duplication.

hefox’s picture

Patch #2 will work with some modifications; it should be module_exists('content_taxonomy') instead of module_exists('hs_content_taxonomy'); the issue is not due to hs_content_taxonomy :P.

As stated in one of my comments, I did read the README and understand why it was done as it was done, but see it as problamatic. (Also, I realized why the hs_taxonomy was required, as I may have said.).

The issue I have with patch #2 is that it is content_taxonomy support specific; other module that interacts with the $form['taxonomy'] array and is weighted before hs_taxonomy it's changes will be lost, which may lead to more issues and frustrations later on.

For example.i18ntaxonomy I noticed there was some calls to fix label
ie. from hs_taxonomy

 if (module_exists('i18ntaxonomy')) {
 	$title = check_plain(tt("taxonomy:vocabulary:$vid:name", $vocabulary->name));
 	$description = ($help) ? $help : tt("taxonomy:vocabulary:$vid:help", $vocabulary->help);
 }

However i18ntaxonomy also translates the terms of the vocabulary. As far as I can tell, (I do not use i18ntaxonomy but looked it up when looking over the code; I spent a bit looking into this...) hs_taxonomy does not address this since it ignores the current $form['taxonomy'].

Upping the weight if i18ntaxonomy would not work since it also remakes the taxonomy array.

Another example; I know a lot use little sub modules to edit information here and there. For example if I wanted to change the taxonomy description per content type, I'd do it in a site module and expect it would work, then get quite confused when it didn't work since I wouldn't know hs_taxonomy was doing this (Readme? Who reads those! Or rather, who remembers what they've read months later when trying to debug something seemingly unrelated.).

Then there's also the issue with hs_taxonomy did not get updated when core taxonomy got patched in the duplicate code, which is generally the issue with duplicate code.

Long story short: Patch 2 works, but (in my opinion) there is an issue with flexibility with other modules that it does not address. Is the scalability issue worth it?

Also, how much is saved by doing it that way?

I'm probably missing something; Most of the information is reused; no additional database calls, just a foreach loop and some edits to the array. (Could even get rid of the function calling, by putting all the changes in the foreach loop).

wim leers’s picture

Long story short: Patch 2 works, but (in my opinion) there is an issue with flexibility with other modules that it does not address. Is the scalability issue worth it?

Yes. It can make a world of difference on sites with multiple thousands of terms.

Because over the taxonomy_override_selector variable, *only* HS generates a form item. In Drupal 5, or without that variable, Drupal core would first generate a normal select with as many terms as there are in the vocabulary. Say 10,000. Clearly, that costs a lot of time to generate. Then HS comes along and simply *deletes* the select. I.e. all the work core did was pointless, completely in vain. That's why it's absolutely necessary to keep on using taxonomy_override_selector.

wim leers’s picture

Status: Reviewed & tested by the community » Needs review

Another thought: changing the module weight was necessary due to #475784: #options set by forum module, this causes a validation error. What you could do, is change the weight back to 1 and then unset #options from within HS itself, at the end of the #process callback. Would that solve all problems?

hefox’s picture

(oh the forum module... only site I've used it on I ended up uninstalling it after a week. It also has other issues like not checking if body is set for the content_type ='( )

(Whatever way, should also do the changes introduced to taxonomy module .. in taxonomy_form_alter? back in.. 6.12? 6.13?. I believe an xss_ something call around vocabulary help or something
http://cvs.drupal.org/viewvc/drupal/drupal/modules/taxonomy/taxonomy.mod...
).

Do you mean set the weight back to 0?

The i18ntaxonomy module is still problematic to, since it's overriding the taxonomy array also IF taxonomy array is set. However, I think the answer is more likely to be in patching that module than this since:
It's is producing those scabilities issues that are the reason for taxonomy_override x2 I think; it's not using the variable from what I can see based on the install hook), so alone or with hs_taxonomy, it's going through the terms (again).
Simpliest way is to ask it to honour that variable and not run when it's set; an until then, weight it before hs_taxonomy.

A perhaps better solution would be to ask it to stop overriding the forum with values it does not need to change; ie #type.

The advantage to that is that could remove some of the i18ntaxonomy support from the module since i8ntaxonomy would be taken care of (other than translating terms, which I finally noticed you are doing -- sorry for not noticing that) like vocabulary names. Also, if instead they used the existing #options it'd deal with the scability issue more I believe.

So the patch would be:
Weight hs_taxonomy to run around the time taxonomy would run; my guess is -1. taxonomy and content_taxonomy are both weighted 0; taxonomy runs first due to the asc weight, asc filename in module_list (just looked that up). So -1 would run it before content_taxonomy and other modules.
Reset the options. Resetting options sounds like a good idea anyway in case other modules create similiar changes:).
Deal with i18ntaxonomy somehow.
filter $vocabulary->help

wim leers’s picture

Do you mean set the weight back to 0?

Yes.

The i18ntaxonomy module is still problematic to, since it's overriding the taxonomy array also IF taxonomy array is set. However, I think the answer is more likely to be in patching that module than this since:
It's is producing those scabilities issues that are the reason for taxonomy_override x2 I think; it's not using the variable from what I can see based on the install hook), so alone or with hs_taxonomy, it's going through the terms (again).
Simpliest way is to ask it to honour that variable and not run when it's set; an until then, weight it before hs_taxonomy.

A perhaps better solution would be to ask it to stop overriding the forum with values it does not need to change; ie #type.

The advantage to that is that could remove some of the i18ntaxonomy support from the module since i8ntaxonomy would be taken care of (other than translating terms, which I finally noticed you are doing -- sorry for not noticing that) like vocabulary names. Also, if instead they used the existing #options it'd deal with the scability issue more I believe.

Interesting reasoning, I hadn't considered that. That'd be nice. But it's unlikely that it will happen. If you'd get those patches in though, I'd be very grateful :)

So the patch would be:
Weight hs_taxonomy to run around the time taxonomy would run; my guess is -1. taxonomy and content_taxonomy are both weighted 0; taxonomy runs first due to the asc weight, asc filename in module_list (just looked that up). So -1 would run it before content_taxonomy and other modules.
Reset the options. Resetting options sounds like a good idea anyway in case other modules create similiar changes:).
Deal with i18ntaxonomy somehow.
filter $vocabulary->help

Right! It should be -1, not 0.
Thanks for looking into this! :) Care to roll the necessary patch(es)? :)

hefox’s picture

StatusFileSize
new1.5 KB

Bah! I8ntaxonomy needs a 5 weight due to it's interaction with views in the latest dev (which would override hs_taxonomy at weight 2 anyway).

I've filed an issue with a patch to check empty(taxonomy override selector) here #619726: Ensure normal (non-HS) taxonomy form items get translated via i18n, and that's module's devolpment is fairly active so hopeful that (or a better patch) will get in.

Attached patch moves the weight back to -1 I believe (unless I fail at basic copy, past, edit 2 to -1 ) and unsets the #options at the end of hierarchical_select_process. Tested briefly with forum and content_taxonomy, and both seem to be working.

(another module that could be tested is comment alter taxonomy since they also remove the vocabulary from the node edit form, but I don't use that module).

TripleEmcoder’s picture

Wow, now I got lost in this discussion ;)

I understand that you agreed on a solution involving module weight correction plus some changes in i18n_taxonomy? Could please sum up the current situation and point to the patches that need to be installed to get all this in shape (I counted 2)?

I am using all of the mentioned modules on my site, i.e. the HS suite, forum, content_taxonomy and i18n_taxonomy. Will it work?

hefox’s picture

StatusFileSize
new1.99 KB

Theoretically it'll work, but I'm using neither i18ntaxonomy nor forum, though I tested briefly for forum. I'd love for you to test it since you are using those! XD

1) The patch for i18ntaxonomy is for Upgrade to the dev verison if i18ntaxonomy (or guess where the line would go in current verison; it's fairly simple)
2) If you applied my previous patch, undo it via this sql statement :

update variable  set value="b:1;" where name="taxonomy_override_selector";

3) Upgrade latest dev of hierarhical select.
4) Apply the patch
5) Run update.php for hierarchical select; update 2.
That should be it.

New patch, haven't tested it, but quickly added in the $description filter_xss_admin on vocabulary help.

summit’s picture

Subscribing, greetings, Martijn

mrfelton’s picture

patch at #36 fixes a conflict with the cck_taxonomy_subset module. Thanks.

wim leers’s picture

Thanks mrfelton!

I'd like one more confirmation :)

TripleEmcoder’s picture

Ok, I finally got around to testing this patch. Sorry for taking so long. It does solve the issue and doesn't seem to brake anything (as far a I can tell for now). But the patch for i18n_taxonomy produces a syntax error :D Easy to fix though.

hefox’s picture

memfis, could you redo the patch for il8n_taxonomy, or post in the ie8n taxonomy issue the patch is from on what the quick fix is (bump the issue, yay \o/)? I don't have a testing enviroment set up to quickly do it and bit busy atm.

TripleEmcoder’s picture

Ok, so just be sure I captured the intent. What you want to check there is that taxonomy_override_selector is FALSE or not set, right? Can you look below?

Broken:

-        && ($node = $form['#node']) && isset($form['taxonomy']) ) {
+        && ($node = $form['#node']) && isset($form['taxonomy'] && empty(variable_get('taxonomy_override_selector',FALSE))) ) {

Fixed:

-        && ($node = $form['#node']) && isset($form['taxonomy']) ) {
+        && ($node = $form['#node']) && isset($form['taxonomy']) && variable_get('taxonomy_override_selector', FALSE) == FALSE) {

Is this what you meant? I honestly don't know what this does, just tried to figure out from code a semantically equivalent but syntactically correct statement ;)

Bilmar’s picture

subscribing

hefox’s picture

http://drupal.org/node/619726#comment-2373804 Updated i18n taxonomy patch, though manually so hopefully it's fine. It'd be very useful if those who have reviewed it say in that thread that they have reviewed it since right now it's just me in that thread.

Sigh, forgot can't do empty with functions... again.
I did ! instead of == FALSE; is there any advantage to == false over not (!)?

baaj’s picture

Sweet! applied both patches (#36,#44) and it now works
Much appreciated hefox et al

wim leers’s picture

Can you please combine them into a single patch so it's easier to test and review? Thanks.

TripleEmcoder’s picture

@hefox: they are the same thing, I just used == FALSE because it was easier to read the intent with the default parameter also set to FALSE.

YK85’s picture

subscribing

Alex Andrascu’s picture

+1

wim leers’s picture

Status: Needs review » Fixed

[rant]Wow, just read through the entire issue again (but skipped some parts, otherwise it'd take *way* too long) and this issue further decreases my faith in Drupal's Forms API and Drupal in general. This is a truly ridiculous edge case and should never occur in the first place.[/rant]

I've added my vote of confidence at #619726: Ensure normal (non-HS) taxonomy form items get translated via i18n, which will hopefully help in getting that patch committed sooner rather than later.

I've committed the patch in #36, with some changes. See http://drupal.org/cvs?commit=335120

Thanks for your hard work and persistence, bibo and of course most of all, hefox! :)

klonos’s picture

Wow Wim! You're on fire!!...

I see you've committed a whole lot of patches and code that needs testing. When are we to see a new dev release available? Say 12 hours or so?

Note to self: Need to get some rest and be ready for some testing once a (dev)release is out.

wim leers’s picture

I think the dev snapshots refresh every 4 hours. You can ensure you've got all patches by comparing the date and time at which the snapshot was generated with the date and time of the last commit.

klonos’s picture

wim leers’s picture

See my reply :)

In any case, you can just download the dev snapshot from http://drupal.org/project/hierarchical_select!

Or even better: get it directly from CVS!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.