Integrate with core taxonomy

interesting, thanks

-> you can shorten this code by just using array_filter($form_values['nodes']).

+ foreach ($form_values['nodes'] as $key => $node_type) {
+ if (empty($node_type)) {
+ unset($settings['nodes'][$key]);
+ }
+ }

this doesn't look good?
+ $settings['weight_vocabulary'] ? $settings['weight'] = $settings['weight_vocabulary'] : $settings['weight'] = 0;

It'd better be
$settings['weight'] = $settings['weight_vocabulary'] ? ...

Anyway, you could save this all, by just using $settings['weight'] everywhere, but don't using $form['weigt'] and adjusting the name in the submit handler $settings['weight'] = $form_values['form_name'];

I need to give this more testing, anyway I'd prefer more node related taxonomy integration like: Assign a term to a node + Remove a term from a node.

hm, let me explain it better.

as for the form 'weight' can'T be used you can use, e.g. 'vocabulary_weight'.

The in your forms submit handler you return the settings as array('weight' => $form_values('vocabulary_weight')). So the value ends up in $settings['weight'] - where you can use it for setting the #default_value for the form element 'vocabulary_weight'.

When I added the latest patch as the two .inc files, I got an error when trying to add a vocab:

Parse error: syntax error, unexpected T_VARIABLE in /sites/all/modules/workflow_ng/workflow_ng/modules/workflow_ng_taxonomy_forms.inc on line 44

I think there was an issue with my patch, I ran it again and it worked fine.

I'm a bit confused about the methodology for removing taxonomy terms. I have used the patch to add them fine, but not remove existing terms. Any help?

thanks, I did another review.

I think we should focus on this use case:
* Add a term to content and so also
* Remove a term from content.

Modifying vocabularies is another use case. I think the patch should full fill at least one *entire* use case - as the module is already considered stable.
So just for completeness when adding "add term from content" please also do "remove term".

So for making this use-cases easy to grok and do I'd suggest to remove the entity "term" and treat it like strings - just let the user specify or choose a term when it's needed. So the add term action could just present the taxonomy form instead of using an "term" argument - so it's much easier to use.

Another possible idea which come to my mind, is providing actions per vocabulary ala "Add a term of "vocabulary XYZ" to content" - then you can reuse the common taxonomy form which is a big + for auto-complete taxonomies. Probably this would need the backport of the "base" or "callback" properties for actions - just an idea. I'm ok with an action for all too.

In regard to modifying vocabularies:
Again I'd remove the entity 'vocabulary' and just present a select form for choosing one of the existing vocabularies on action configuration time instead of using the argument. So it's easier to use! User friendliness is a really important point and we should not accept a bad user experience for having better abstracted code.

hm, I'm still not sure about it.

let's consider I want assign a term to content.

approach A with separate loading actions:
1. the action doesn't appear. Users have to know they have to load a vocabulary first
2. configure an action load the vocabulary
3. now the action appears, configure "assign term to content"

vs
approach B, one action "assign term" for all vocabularies:
1. just see and configure the action, choose from a select containing all terms of all vocabs the term

approach C: multiple actions per vocabulary
1. select and configure action "Assign term of vocabulary XYZ to content"
advantage: reusing of the usually taxonomy form possible, so we could allow using an auto complete like known.

I think loading a vocabulary previously is a unnecessary and quite complicated step which is difficult to understand, so I'd prefer B or C. I think C is best, but is also most work to implement.

I'd be interested in the opinion of others - so we make sure we do it right. !?

@missing action: yep, I like the idea. If you want to work on it - just go ahead. :) However I don't think that this is enough for point A, as it's still complicated and difficult to grasp.

@B: I'd make a select with vocabulary names as optgroups, this should make it clear to which vocabulary a term belongs. As tids are unique it's no problem to make one select.

@C: hm, indeed. But when does someone create a vocab on the fly? For what? Usually one hasn't many vocabularies and I can't think of a use case where I want to add them on the fly?

hm, interesting. thanks.

Yes probably C is best for now. For other use cases we could still provide a more generic advanced action: Add an arbitrary term for a arbitrary vocab. As term selection is not possible for an arbitrary vocab, we would have to provide a simple, plain textfield for the term anyway.

* your indention has sometimes wrong. E.g.
+ foreach ($vocabularies as $vocabulary) {
+ $info['rules_action_taxonomy_term_relation_to_content_'. $vocabulary->vid] = array(
+ 'label' => t('Assign or remove a term from vocabulary') .' "'. $vocabulary->name .'"',
+ 'argument

*
@+ 'label' => t('Assign or remove a term from vocabulary') .' "'. $vocabulary->name .'"',
First off you need to check the vocab name. Furthermore better use t('bla @name').
Make sure to escape the vocab name everywhere.

+ 'description' => t('Assign a certain taxonomy term to the content.'),
It's 'help' now, and only for real help. So I'd just delete this line, as this should be clear.

* Only put the vid (vocabulary id) into the action info, as you only need the vid and the other stuff just unnecessary bloats the configured action.

* For action perhaps better use readable keys like, 'add' and 'del' - this makes the action code itself a bit better readable.

* Why not just use http://api.drupal.org/api/function/taxonomy_form and for tags a autocomplete? See http://api.drupal.org/api/function/taxonomy_form_alter

Something else. This patch let me think again over this all and how this fits into hook_taxonomy, which we could support. However without a 'taxonomy' data type, it isn't really useful. So a new approach came to my mind, a mixture between your suggestion A and the current C:

* Don't use the vocabulary data type - stay with per vocabulary actions like now.
* Create a taxonomy data type, which has to be identifiable and probable savable.
* Do a Load term per vocab action - reusing the taxonomy form. We could split it up into two base actions, one for tagging vocabs and one for hierarchical vocabs.
* Do a assign term to content action, which has the term as argument
* Now assigning a term to content, requires again 1. loading and 2. assigning it.
* But now we can support hook_taxonomy, and e.g. assign newly created terms of a user to his profile node and do other useful stuff.
* Perhaps then a rename and delete term action would make sense too.

What do you think about it? Should we do it that way?

I've a guilty conscience about changing my opinion again. :(

:)

1. Actually it would work without the data type too. But when we want rules to intelligently save terms, we have to make theme 'savable' and implement the save() routine for the data type. Just have a look at the node data type.

2. Rename a term. rename Term "A" to "B", while the tid stays the same.

The difference is, I'd say that it makes sense to automatically create/edit/delete terms, but I don't think that's the case with vocabularies.

Yeah, og vocab does that, but even in that situation I don't think it's the ideal way to go - as you say you'll get a lot of vocabularies... (We are looking at doing it with content taxonomy and one big taxonomy instead for d6). So there might be some better use cases for that, but even that I think they are seldom.

So this approach would still have a simple way for selecting vocabularies. Then we can re-use existing ways for generating per-vocab taxonomy forms, letting users selecting their existing terms in an easy way as well as adding new terms upon it. So I think it's the best way as it fullfills 99% of the use cases but keeps things simple to use.

@cluttered actions list: indeed, we could do a multistep form instead like the CCK integration does for selecting fields. So we would have only one action even if there are thousands of vocabs.

* You don't need to check if taxonomy module exists - the integration will only be loaded when the module is there.
* I'd suggest to rename 'Assign or remove a term' to 'Assign or remove a term to content'
* hm, I'd remove both help properties. +1 for adding help, but this help doesn't say more than the label.

  empty($settings['term']['term_text']) ? $term = $settings['term']['term_select'] : $term = $settings['term']['term_text'];

Use $term = condition ? value1 : value2; Same thing with $action in the forms.inc.

* When you set your data type to be savable, you have to implement save().

* The description in your term loading action talk about adding/removing.

When configuring load term I got

* notice: Undefined index: term in /var/www/fago/web/drupal-6/sites/all/modules/rules_6/rules/modules/taxonomy.rules_forms.inc on line 51.
* notice: Undefined index: term in /var/www/fago/web/drupal-6/sites/all/modules/rules_6/rules/modules/taxonomy.rules_forms.inc on line 59.

When configuring assign I got
notice: Undefined index: action in /var/www/fago/web/drupal-6/sites/all/modules/rules_6/rules/modules/taxonomy.rules_forms.inc on line 78.

@load form:
What about reusing taxonomy_form or taxonomy_term_select? Ok it's odd that we can't reuse the autocomplete form, as it sits directly in taxonomy_form_alter. But having at least hierarchy in the select, if there is one would be nice imho.

@assign or remove action:
Hm, it's quite strange to have to choose the action inside of one action. Perhaps we should make better two separate actions?

+1 for the by term id feature. Perhaps we should call it "Select by term id" and make a textarea out of it, so one can put more php code into it.

+1 for doing this in two steps, so let's get this stuff right and get it in - then we can proceed with more.

subscribing

subscribing

thanks, that's already better, I've had a look at it too:

* taxonomy_form returns a multiple select when the voc is multiple - I've fixed by introducing an own copy of that function. :(
* I've fixed the multistep form to only show the vocab select in the first step and to save the changes to the label
* I've improved the actions to only save, when they actually changes something.
* I tried to make the data type savable, by taxonomy_save_term() lost it's parent relationship for my test. So for now, it's still not savable. Perhaps we should just implement our own saving function in the save() method and bypass taxonomy_save_term() - which is a bad api function anyway. (form values ?).

So far it seems to be working for me - please test. -> Committed.

For catch 2 of the taxonomy integration, I think we should open another issue.
What could be added there?
* Rename a term
* condition for checking whether a node has a term assigned
* add a new term
* make the data type savable
* delete a term
* support the taxonomy hook -> event

Thanks for your patience!

Updated component.