Add system for registering voting tags (axes) [#335668]

Voting API supports multiple voting "tags", the default being "vote".

There is a fairly standard set of data that a module using voting tags will need to know:

* the tag itself (machine-readable name)
* a title for the tag
* the data type or types supported by the tag (e.g., percent)

There may be use cases for other properties, e.g., the content type or types that a tag may apply to.

In #317317: Rate on multiple axis with admin UI we're working on implementing multiple tags (voting axes) for Fivestar. There is a need for admin-defined voting tags.

This could be implemented directly in Fivestar. But the need seems to be a general one: to define the available voting tags. Multiple modules may need admin-defined voting tags. Also, a given voting module may need to know what voting tags are implemented by other modules.

So this seems to be an API feature that belongs, if anywhere, in Voting API.

Proposed approach: model the implementation on the core node type system.

1. Modules may define a set of voting tags by implementing a hook, hook_votingapi_tags(). Sample implementation:


function votingapi_votingapi_tags() {
  $tags = array();
  $tags[] = array(
    'name' => 'vote',
    'title' => 'Vote',
    'data_types' => array(
      'percent',
    ),
  );
  return $tags;
}

These hook implementations define the module-defined voting tags.

Like node types, module-defined voting tags are read into the database.

2. An admin UI can be used both to customize module-defined voting tags (e.g., to give them a custom title) and to add new voting tags. This works like the node types system. Admin-defined voting tags may be deleted. Module-defined voting tags may be disabled but not deleted.

3. We store voting tag data in two tables. votingapi_tag stores basic data: for now, name, title, status. Possibly in future we would add here an i18n field. votingapi_tag_type stores the association between a tag and its (potentially multiple) data types.

4. A method, votingapi_get_tags(), returns available voting tags from the database, comparably to node_get_types(). Arguments may be fed to get only enabled tags or only tags that support a given data type (e.g., 'percent').

Comment	File	Size	Author
#9	votingapi_335668.patch	35.51 KB	stella
#9
#2	votingapi-tags.patch	25.85 KB	nedjo
#2

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

catch

he/him

English

CreditAttribution: catch commented 18 November 2008 at 09:27

Subscribing to this so I can review later. Seems like a good approach.

Comment #2

nedjo

he/him/his

English

CreditAttribution: nedjo commented 25 November 2008 at 00:42

Status:

Active

» Needs review

File	Size
votingapi-tags.patch	25.85 KB

Here's an initial, incomplete patch.

Following as closely as possible the node type system, this patch adds both module-defined and custom voting tags. To minimize later update work and to take advantage of some improvements, I based the code on current Drupal core HEAD rather than on the Drupal 6 branch.

votingapi_get_tags() is the relevant public method, working a lot like node_get_types() (accepts 'tags', 'names', 'tag', and 'name' arguments).

Missing from the patch so far is handling for multiple data types per voting tag (percent, etc.). I'll work in the next day or two on finishing this. Review meantime would be great. Also missing is an adaptation of the D7 Javascript for handling machine-name changes. If wanted, that should be easy to adapt.

Comment #3

eaton CreditAttribution: eaton commented 25 November 2008 at 22:49

A couple points: I need to double-check, but I believe HEAD is currently out of date. I sync back TO head when I move to a new version of Drupal, but development has been going on in the DRUPAL-6--2 branch for some time.

I'm still concerned about the tag registry system. The concept of 'tags' has very different meaning in different contexts; one *content_type* of vote may use different tags than another *content_type* of vote, for example. I'm doubly concerned that additional tables, UI, etc are involved in this. I'm not clear what the payoff of the system is. How would it be used? WHy should it be in the API, rather than in modules that need admin-defined tags?

This could be implemented directly in Fivestar. But the need seems to be a general one: to define the available voting tags. Multiple modules may need admin-defined voting tags. Also, a given voting module may need to know what voting tags are implemented by other modules.

While it might be useful to provide a hook that lets modules announce what kind of meta-data they are inserting into the DB, I'm not seeing the benefit of centralizing defining this stuff in the VotingAPI itself.

I'm not opposed to it inherently, just skeptical at present. The problems I've seen with multi-axis voting are UI related and AJAX/roundtrip efficiency related, not 'lack of a central screen for CRUD'...

Comment #4

nedjo

he/him/his

English

CreditAttribution: nedjo commented 25 November 2008 at 23:19

Version:

6.x-2.0-beta7

» 6.x-2.x-dev

The patch is on the DRUPAL-6--2 branch. I meant *core* HEAD--I've followed core HEAD for node types code rather than the 6.x branch.

I'm not clear what the payoff of the system is. How would it be used? WHy should it be in the API, rather than in modules that need admin-defined tags?

Good questions.

Take a given voting module, like fivestar. It needs to handle at least one built-in, code-defined voting tag, 'vote'. And it also needs the ability to have admin-defined tags. An added bonus might be admin customizations to the code-defined tags.

We could do all this in fivestar, and in fact we started to do so. But there are two problems with that.

1. Any other voting module wanting the same would need either to repeat the significant amount of code required or to rely on fivestar. The first is suboptimal, the second plainly wrong.

2. Fivestar would have no good way of knowing about tags potentially defined or used by other modules, aside from looking in the voting tables and seeing what happens to have been saved there. And other modules would have no knowledge of fivestar's tags.

Say a site featured three different voting modules, each of which relies on custom (admin-defined) tags. With no central API, each of those modules would have to implement its own solution for defining and storing those tags. Site admins would have to define the tags in three different places, probably using different UIs. What if an admin wants the same tag to be reused in all three voting modules? Presumably she would have to manually enter (and then synchronize) the tags in these different places.

quicksketch and I concluded after talking this over that support for both machine- and admin-defined tags is a feature that belongs, if anywhere, as part of a central voting API.

Does that make sense? What alternatives should we consider, if any?

If they indeed belong in a central voting API, is that central place necessarily votingapi.module? Could it instead be another voting module (e.g., votingapi_tags) that voting modules could optionally require? I think the answer is yes, that could be done--so long as a voting tag registry is optional. That is, so long as e.g. we don't enforce a requirement that tags saved with votes be valid, registered tags.

Does that sound better? Rather than adding this to votingapi, we write it as a separate part of votingapi, votingapi_tags?

Comment #5

nedjo

he/him/his

English

CreditAttribution: nedjo commented 26 November 2008 at 04:42

The concept of 'tags' has very different meaning in different contexts; one *content_type* of vote may use different tags than another *content_type* of vote, for example.

@Eaton: Could you explain this issue a bit more? Do we need to designate the content_type or types that a particular tag is applicable to?

Another possibility for implementation: almost all of this goes into votingapi_tags, but the votingapi_get_tags() method stays in votingapi. If votingapi_tags is not present, it returns the hook call. That is, no database storage, and only code-defined types. If votingapi_tags is present, it reads from the DB.

Something vaguely like:


if (module_exists('votingapi_tags')) {
  $_tags = votingapi_tags_get_tags();
  $_names = votingapi_tags_get_tags('names');
}
else {
  $_tags = _votingapi_get_tags();
  $_names = _votingapi_get_tags('names');
}

Comment #6

nedjo

he/him/his

English

CreditAttribution: nedjo commented 19 December 2008 at 20:30

Eaton and I discussed this issue further. We're leaning towards a votingapi_tags or similar module that would be part of votingapi and could optionally be required by other modules.

Some further ideas:

1. Each tag should be part of a named tag set, providing admins with cues as to what tags go together. Eaton:

API hook for modules to announce their sets of tags. they can send back a 'tag set' that's named, and has a child array of value/name pairs for the tags themselves. this is for modules that want tag sets to be reusable.

2. Make the "add a new voting" form reusable, so that it can be embedded in e.g. the fivestar configuration form, maybe in a simplified form. Like we do in core with menu items and url aliases and nodes. They have their own UI (e.g., add a menu item) but also a simlified one embedded in the node form.

Comment #7

bcn CreditAttribution: bcn commented 20 December 2008 at 10:51

Regarding this,

2. Make the "add a new voting" form reusable, so that it can be embedded in e.g. the fivestar configuration form, maybe in a simplified form. Like we do in core with menu items and url aliases and nodes.

Is the use case here that you want to vote-enable a single node? Wouldn't VotingAPI allow for this now, it just that no modules have yet to expose this? Would you have to have 'pre-enabled' voting for that node type, or were you thinking this is something that could be added ad hoc, ala menus in the node form?

I recently applied for maintainership of the 'voting' module, in the hopes to add to it a set of generalized voting features (e.g. custom voting axes/tags, node type assignment, widgets per node type or axis) to help reduce redundant code that is appearing in other voting modules while allowing VotingAPI to stay lean. I don't want to hijack this away anymore, but Nedjo's comments here seem to be the first time this issue has come up.

Comment #8

andypost

he/him

Russian

CreditAttribution: andypost commented 27 December 2008 at 12:01

It looks as extension...

If vote is fact and tags are axes ?

So how to mark a vote by different tags without modifying 'votingapi_vote' table?

field tag is only one

What are the ideas? possible to make 'votingapi_tags' table with relation vote_id and different hook to store vote with multiple tags?

Comment #9

stella CreditAttribution: stella commented 19 February 2009 at 11:31

File	Size
votingapi_335668.patch	35.51 KB

Here's a new patch which implements some of the suggestions above:

The tags stuff has now been separated out into its own sub-module votingapi_tags, but leaving votingapi_get_tags() and an implementation of hook_votingapi_tags() in votingapi.module.

The votingapi_get_tags() function in votingapi.module will return any module defined tags if the votingapi_tags module is not enabled, otherwise it calls a function in votingapi_tags to return a list of both module defined and custom tags.

I've fixed some issues with the configuration of custom voting tags and included some simpletests for the new forms.

Cheers,
Stella

Comment #10

Fanaile CreditAttribution: Fanaile commented 24 March 2009 at 07:38

Hi Stella;

Thanks for this. I tried to apply the patch into my test site, and I think I might be doing something wrong. No errors were shown after I applied the patch at all. Then I went in and enabled the votingtags api module and then went to double check the permissions (to be safe). But when I go into the administration menus and click on the link for Votingapi, I get this error:

Fatal error: require_once() [function.require]: Failed opening required 'sites/all/modules/votingapi/votingapi_tags/votingapi_tags.admin.inc' (include_path='.:/usr/share/pear') in /var/www/test/drupal6/drupal-6.2/includes/menu.inc on line 346

I can click back on the browser to get back into the site with no problem, and I only get the error when I click on the link. Have I set it up wrong or did I miss a step? I'd like to be able to test this out.

Comment #11

stella CreditAttribution: stella commented 24 March 2009 at 10:38

That file is included in the last patch, so I'm not sure why that is happening. You might need to resave the modules listing - this patch includes a new module called 'votingapi_tags' by the way.

Cheers,
Stella

Comment #12

Fanaile CreditAttribution: Fanaile commented 24 March 2009 at 18:41

Hi Stella;

Thanks for getting back to me. I do think I'm doing something wrong (I'm not good at php or the backend interface...) So... here's what I tried.

I uninstalled and deleted the voting api module. Then downloaded and installed a new module. Then I saved a new patch file. I moved the patch file into the votingapi directory and applied the patch. Here is that result:

[root@progressiveu admin]# cd /var/www/test/drupal6/drupal-6.2/sites/all/modules/votingapi
[root@progressiveu votingapi]# ls
API.txt        LICENSE.txt  tests         views                votingapi.info     votingapi.module
CHANGELOG.txt  README.txt   translations  votingapi.admin.inc  votingapi.install  votingapi_335668.patch
[root@progressiveu votingapi]# patch < votingapi_335668.patch
(Stripping trailing CRs from patch.)
patching file votingapi.module
(Stripping trailing CRs from patch.)
patching file votingapi_tags.admin.inc
(Stripping trailing CRs from patch.)
patching file votingapi_tags.info
(Stripping trailing CRs from patch.)
patching file votingapi_tags.install
(Stripping trailing CRs from patch.)
patching file votingapi_tags.module
(Stripping trailing CRs from patch.)
patching file votingapi_tags.test
[root@progressiveu votingapi]# ls
API.txt        README.txt    views                votingapi.install       votingapi_tags.admin.inc  votingapi_tags.module
CHANGELOG.txt  tests         votingapi.admin.inc  votingapi.module        votingapi_tags.info       votingapi_tags.test
LICENSE.txt    translations  votingapi.info       votingapi_335668.patch  votingapi_tags.install
[root@progressiveu votingapi]#

Then I went into my test site and enabled the votingapi tags module. When I went back to Administer, again, it gave me the same error as before when I tried to click on the voting api link:

When I clicked on the link provided (function.require), it takes me back to the administer > site configuration page and shows a warning:

warning: require_once(sites/all/modules/votingapi/votingapi_tags/votingapi_tags.admin.inc) [function.require-once]: failed to open stream: No such file or directory in /var/www/test/drupal6/drupal-6.2/includes/menu.inc on line 346.

So I went into includes/menu.inc file to look at line 346 and here is what I see there (lines 346-357):

          // Fall-through
        case '/li':
        case '/dd':
          array_pop($indent);
          break;
        case '/h3':
        case '/h4':
          array_pop($indent);
        case '/h5':
        case '/h6':
          $chunk = ''; // Ensure blank new-line.
          break;

Do you see anything I've missed or do you know how I can repair that error? I appreciate your help.

Comment #13

eaton CreditAttribution: eaton commented 31 March 2009 at 16:26

Greetings!

Apologies that I haven't had a chance to really tackle this yet. I've been looking over the code and I think that with a few modifications, it will go in. I'm not particularly happy about the votingapi module being tied to a large chunk of UI code -- this patch is larger than the entirety of VotingAPI currently. But I can definitely see the usefulness of a central repository for value tags.

This chunk of code in particular causes me to scratch my head:

>     // If the votingapi_tags module is enabled, use that to get the list of tags
>     // defined in the database and via the 'votingapi_tags' hook.
>     if (module_exists('votingapi_tags')) {
>       list($_tags, $_names) = _votingapi_tags_tags_build();
>     }
> 
>     // Otherwise just return the ones defined via the 'votingapi_tags' hook.
>     else {
>       $tags_array = module_invoke_all('votingapi_tags');
>       foreach ($tags_array as $tag => $info) {
>         $info['tag'] = $tag;
>         $_tags[$tag] = votingapi_tags_tag_set_defaults($info);
>         $_names[$tag] = $info['name'];
>       }
>       asort($_names);
>     }

Is there any reason to special-case this? Can't votingapi_tags module just return the database-defined tags itself using the hook? Worst case, if it needs to take over the tags entirely, it seems that a drupal_alter() call, giving it a chance to change the resulting collection, might be a better approach that ties them together a bit less.

I'm very, very close to committing the internal changes to votingapi module and then committing votingapi_tags as a separate but related project. It's only necessary in a specific handful of cases. In addition, I'm wondering if a similar hook might be useful for aggregate functions - some of them are a bit opaque if one just goes by the name.

Thoughts?

Comment #14

eaton CreditAttribution: eaton commented 24 June 2009 at 19:24

Status:

Needs review

» Fixed

I've committed a patch that adds a votingapi_metadata() function for getting metadata, and hook_votingapi_metadata_alter() for adding new tags/functions/value_types or altering existing ones.

I'm not comfortable with rolling a relatively heavy "Tag definition module" into VotingAPI's official distribution, but this feature should make it relatively easy to put together. I know it's a VERY stripped down version of what this patch proposed, but it gives us something to reliably build upon without forcing a very heavy solution on everyone.