I find NCO is a great tool, but I have multiple vocabularies in my setup and what I need is NCO to span most of them in the analysis.

I simply tried to change the default select box to a "#multiple", to add some implode/explode to the setting variable and to add some IN (:vocabularies) in the queries and I'm just testing the result.

I'm posting this "earlier" during my tests because I'd like to hear if there are reasons for being limited to a single vocabulary.

CommentFileSizeAuthor
#4 taxonomy_nco-multiplevids-1483672-1.patch16.92 KBbago

Comments

russellb’s picture

russellb commented

Hi. We limited NCO to one vocabulary for simplicity, and because the original application only needed to work on one vocabulary. It would be a great upgrade to allow multiple vocabularies, and I don't see any major obstacles. We would welcome patches on this.

russellb’s picture

russellb commented
Status: Active » Postponed (maintainer needs more info)
russellb’s picture

russellb commented
Status: Postponed (maintainer needs more info) » Closed (works as designed)

Closing this for now as nothing heard - bago if you do have code on this just let us know - we will probably revisit this topic in due course in any case.

bago’s picture

bago commented
Status: Closed (works as designed) » Needs review
StatusFileSize
new taxonomy_nco-multiplevids-1483672-1.patch16.92 KB

Hi russelb, sorry to be late but I've been busy.

I just send you the diff for my local patched module (against 1.1 release, not 1.2, I didn't check if it works on 1.2 too).

Changes involved:
in taxonomy_nco_analysis.module I simply changed the variable taxonomy_nco_analysis_vocabulary to be a comma separated list of vids (so to be backward compatible) and changed the query to use "IN" instead of "=".

in taxonomy_nco.module I added support for multiblock and added a taxonomy_nco_block_configure method also supporting multiblock. Multiblock support is the easier way to support multiple instances with different configurations. The multiblock dependency is optional (you just need it if you want multiple blocks with different configurations).

I guess this is not "ready to be applied", but I wanted to share this with you anyway, so you can have a look at it.

bago’s picture

bago commented

I forgot to say that this patch includes much more than what I described in the original issue.
In fact it also adds block configuration to decide what vids include during "querying".

So you can add vid 1,2,3 in analysis and then have a block reading vids 1 and 2, another reading only vid 1 and another on vids 1 and 3, for example.

russellb’s picture

russellb commented

Hi bago, great to see your patch there. I'll take a look.

If we are going to get this committed we will need to get this code integrated with the latest dev. I've recently added support for all entities in the main analysis functions and the batch run. I don't think this should be too much of a problem, the same principles still apply, we're using the taxonomy_entity_index module which gives us a table very much like taxonomy_index, but with taxonomy from all entities.

I'll try to hold up commits to dev. while we look at your patch and see if we can get it included.

russellb’s picture

russellb commented

Looks good.

At line 295 of your patch you have the comment '// TODO taxonomy_nco_all_tids is a killer!' - could you explain what you were thinking of there?

bago’s picture

bago commented

Hi russellb,

I admit I don't remember :-(

I can add to the context that the website I'm using this module has almost 100K terms. So maybe I noticed that the method is "heavy" and I commented it that way (maybe I noticed that the same thing could have been done in a lighter way.. don't remeber and no time right now to have a better look at it, sorry).

russellb’s picture

russellb commented

OK good to hear you're putting some load on the system there in D7.

Very happy to look at any optimisation patches obviously!

So I am keen to proceed with your patch here on multiple vocabularies, do you think you could reroll it against taxonomy_nco 7.x-1.x-dev ? As I said the main difference from 1.1 is the taxonomy_entity_index stuff, but that is all the same really. ..I'm thinking it would be good to refactor some of those query opt-outs at some point but we don't have to do that now.. one thing at a time..

bago’s picture

bago commented

I don't even know what "taxonomy_entity_index" is, so I guess the patch doesn't touch similar things: I altered joins using taxonomy_index with no _entity_ substring so I guess you're talking about something else.

So, in theory it should work against latest dev, but I didn't check that.

russellb’s picture

russellb commented

taxonomy_index only contains data for taxonomy on nodes, for taxonomy on other entities 7.x-1.x-dev now supports a contrib. module taxonomy_entity_index which gives us a table taxonomy_entity_index which is basically the same as taxonomy_index but indexes tax. on all entities. If the module taxonomy_entity_index is not present current dev works basically the same as 1.1

russellb’s picture

russellb commented

http://drupal.org/files/taxonomy_nco-multiplevids-1483672-1.patch fails against taxonomy_nco 7.x-1.x-dev

We're trying git merges.

russellb’s picture

russellb commented

ap has done a git merge on this, and I've done some manual conflict resolution. It looks promising so far. I've pushed my merge to git 7.x-1.x-dev - it is available on the nightly D7 dev download & on git now.

7.x-1.x-dev download:
http://drupal.org/node/1018496

merge commit:
http://drupal.org/commitlog/commit/12120/56e1594bf489e7de6a2926d1c204ea2...

Could you test this new version please bago? I've ported your multiple vids code onto the new entities stuff here as well, so it should work for all entities as well as nodes now.

bago’s picture

bago commented

I just upgraded my website to the latest dev and it seems it works as before (I was on 1.1 + my patch).

Thank you :-) :-)

russellb’s picture

russellb commented

You're welcome. Great news.

I have found one problem I think - am I right in saying that you don't delete the variables you create on uninstall? It looks like there could be any number of variables created for multiblock .. I wondered if a quick query in uninstall to look for all variables with names starting with taxonomy_nco could be a solution. What do you think?

bago’s picture

bago commented

Yes, I forgot about uninstalling. I don't know if there is a best-practice for uninstalling and multiblock support.

E.g: when you remove a multiblock instances multiblock doesn't call the block module owner so we can't know that it has been deleted. Maybe this needs a multiblock feature requests.

Maybe the easiest solution is to do the query for taxonomy_nco%, as you suggest.

russellb’s picture

russellb commented

Hmm .. some criticism of that approach here:
http://drupal.cocomore.com/blog/how-not-delete-drupal-variables-your-mod...

I wonder if you could tidy up by doing something like adding '_multi_' into your multi variables so that we could do a clean uninstall. Then we could do a query for taxonomy_nco_multi% or somesuch, and keep our variable namespace organised.

bago’s picture

bago commented

I'm not sure why you think adding "multi" will make it better. We already are in a "custom" prefix named "taxonomy_nco" so I don't expect other modules to use this prefix anyway.

About the delete I it seems the best way is to run a dbselect to find them and then call the variable_del for each one.

To do that "better" it could be added some logic to only remove variable names with given regex patterns so that we remove only variable we know we created.

I've no dev environment right now, so I just opened an issue to the multiblock list (#1680870) and write something here for you (or my next days). Here are the "multi" variable we use:

taxonomy_nco'.$multi_id.'_include_tags_in_related
taxonomy_nco'.$multi_id.'_minimum_match_intersection
taxonomy_nco'.$multi_id.'_default_minimum_match_nco
taxonomy_nco_block'.$multi_id.'_vocabularies

so something like
if (preg_match('/^taxonomy_nco(_block(_\d+)?_vocabularies|(_\d+)?_(include_tags_in_related|minimum_match_intersection|default_minimum_match_nco))$/',$varname)) variable_del($varname);
in the loop, could do the trick in a safe way.

PS: and maybe a loop on the $conf keys is better than a db select...

russellb’s picture

russellb commented

I found an error in the merge:

Undefined variable: vocabularies in taxonomy_nco_analysis_tids_on_nodes_updated_between()
(line 513 of taxonomy_nco_analysis.module)

It's in the ongoing updates part of the analysis module.

Fixed version is available for download:
http://drupal.org/node/1018496

Commit log:
http://drupal.org/commitlog/commit/12120/f9156219a6e1070e53e01b4eeb012d1...

russellb’s picture

russellb commented

For now I've done a sweep on taxonomy_nco variable namespace so that that we are clean on uninstall. I've pasted your comments there bago into the code with some todos to remind us where this is up to. This would need refinement if we want to make use of the separate modules (taxonomy_nco and taxonomy_nco_analysis) and if we need separate control over the namespace in the future (eg. if we port taxonomy_nco_clustering and need to be able to install / uninstall the modules separately).

http://drupal.org/commitlog/commit/12120/0dee8fb3b2261ffde140cc3c8fd863e...

russellb’s picture

russellb commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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

russellb’s picture

russellb commented

re. defaults for the NCO block:
The vocabulary selector is new for NCO blocks, and I'm not sure if it is preserving the previous functionality here. It should perhaps default to the vocabularies selected in analysis. Otherwise I'm worried existing users will see their blocks stop working, and new users will get caught out when the block doesn't work out of the box.

russellb’s picture

russellb commented
Status: Closed (fixed) » Needs review
russellb’s picture

russellb commented

I've added a new level of defaults to the block view and config, so that blocks should now default to the analysis vocabularies if the user doesn't make a selection in the block config:

http://drupal.org/commitlog/commit/12120/3a2121d1c788e4f8ae5003b7765cf9e...

Would be great if you were able to test there bago. I'm hoping to get this out as 7.x-1.3 soon and I'm keen to make sure existing installations will behave the same as before on existing config.

russellb’s picture

russellb commented
Status: Needs review » Fixed
russellb’s picture

russellb commented

Multi-vocabulary support is now on release in 7.x-1.3

Status: Fixed » Closed (fixed)

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