HS Taxonomy interaction with other modules (content_taxonomy, unhides taxonomy fields)

memfis - October 11, 2009 - 14:12
Project:Hierarchical Select
Version:6.x-3.x-dev
Component:Code - Taxonomy
Category:bug report
Priority:normal
Assigned:Unassigned
Status:needs review
Description

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?

#1

Wim Leers - October 11, 2009 - 15:01
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.

#2

bibo - October 14, 2009 - 17:32
Status:active» patch (to be ported)

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.

AttachmentSize
hs_taxonomy_hide_taxonomy.patch 613 bytes

#3

Wim Leers - October 14, 2009 - 22:17
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 :)

#4

bibo - October 15, 2009 - 00:58

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:

<?php
/**
* 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:

<?php
 
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?

#5

bibo - October 21, 2009 - 21:14
Status:needs work» needs review

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

AttachmentSize
hs_taxonomy.module_fix_content_taxonomy_ui.patch 2.29 KB

#6

memfis - October 27, 2009 - 13:44

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

#7

bibo - October 27, 2009 - 15:15

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:

<?php

  
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:

<?php
/**
* 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?

#8

hefox - October 27, 2009 - 18:33

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.

#9

bibo - October 28, 2009 - 02:12

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

<?php
 
// Update the module weight to 2 so hs_taxonomy runs after the forum module.
  // See <a href="http://drupal.org/node/475784.
" title="http://drupal.org/node/475784.
" rel="nofollow">http://drupal.org/node/475784.
</a>  $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":

<?php
/**
* 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).

AttachmentSize
hs_taxonomy_re-hide-vocabularies.module.txt 28.4 KB

#10

hefox - October 28, 2009 - 02:21

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 **** :>

AttachmentSize
alternate_hs_content_taxonomy.patch 6.56 KB

#11

memfis - October 28, 2009 - 16:49

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.

#12

memfis - October 28, 2009 - 17:00
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.

#13

hefox - October 28, 2009 - 17:54

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.

#14

hefox - October 28, 2009 - 18:17
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.)

#15

hefox - October 28, 2009 - 18:23

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

AttachmentSize
alternate_hs_content_taxonomy.patch 6.57 KB

#16

bibo - October 29, 2009 - 10:01

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

#17

hefox - October 29, 2009 - 12:42

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.

#18

hefox - October 29, 2009 - 12:45

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.

#19

memfis - October 29, 2009 - 14:19

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.

#20

hefox - October 29, 2009 - 15:05

<?php
 
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

<?php
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 :(.

#21

benone - October 30, 2009 - 12:53

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

#22

benone - October 30, 2009 - 12:58

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 ?

#23

hefox - October 30, 2009 - 13:19

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

#24

benone - October 30, 2009 - 13:25

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.

#25

hefox - October 30, 2009 - 14:05

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

AttachmentSize
alternate_hs_content_taxonomy.patch 6.43 KB

#26

hefox - October 30, 2009 - 14:06

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

#27

benone - October 30, 2009 - 14:10

Ok, I will wait. Thanks.

#28

Wim Leers - October 30, 2009 - 16:46
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.

#29

hefox - October 30, 2009 - 18:00

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

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

#30

Wim Leers - October 30, 2009 - 23:32

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.

#31

Wim Leers - October 31, 2009 - 02:13
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?

#32

hefox - October 31, 2009 - 04:23

(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

#33

Wim Leers - October 31, 2009 - 09:29

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)? :)

#34

hefox - October 31, 2009 - 14:15

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: Honour taxonomy_override_selector, 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).

AttachmentSize
hs_taxonomy_weigt_options.patch 1.5 KB

#35

memfis - October 31, 2009 - 16:00

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?

#36

hefox - October 31, 2009 - 17:10

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 :

<?php
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.

AttachmentSize
hs_taxonomy_weigt_options.patch 1.99 KB

#37

Summit - November 11, 2009 - 09:35

Subscribing, greetings, Martijn

 
 

Drupal is a registered trademark of Dries Buytaert.