My client wants per-vocabulary directories. Currently, you can only easily access the global directory and from there filter per term. What my client needs, is portions of that global directory, i.e. per-vocabulary.

This is already supported by the module, but in a very obscure, unusable way, as described here: http://drupal.org/node/144743. Hence I propose a new, clean, URL lay-out at the same time.

  • directory: the global directory
  • directory/vocabulary/[vid]: a portion of the directory (the part that's of vocabulary with vid [vid])
  • directory/term/[tid]: filter by term
  • (optional) directory/vocabulary/[vid]/term/[tid]

A patch that does all of this at once is out of the question. I will however implement #2, which is what my client wanted. I will not follow the coding style of the original author(s) of this module, which is far from compliant with the Drupal Coding Standards.

CommentFileSizeAuthor
#15 per-vocab-directory.patch3.04 KBbeginner
#7 per-vocabulary_directory.patch3.65 KBwim leers

Comments

beginner’s picture

beginner commented

This is more or less what I had in mind.
Go ahead and I'll review your patch, and commit it when it's ready.

@Inactivist, I hope you're still tracking the issues here: Do you have any objection to this?

This is taking the module in a new direction so we might want to reconsider the directory home page and its settings after that (or maybe not).

Go ahead with #2.

About #3 (directory/term/[tid]): this is the next logical step, but we run into backward compatibility problems: what about all the links to the current directory/[tid] ?

I am not sure what #4 would provide more...

mcurry’s picture

mcurry commented

@beginner, yes, still monitoring. Not sure if I can provide useful feedback in a timely manner, though. I'm open to these kinds of changes, and I'm more than willing to delegate decisions about this feature (or collection of features) for now. I'll jump in if I see anything that seems inappropriate.

My guess is that very few people are using these features, anyway, otherwise, we'd have seen more issues related to them.

Once again, thanks for the assistance on this module. It's greatly appreciated!

beginner’s picture

beginner commented

Ok. Thanks for the reply

:)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

#4 is to filter by tid within a vocabulary.

beginner’s picture

beginner commented

Yes, I understood that, but a tid necessarily belong to one single vocab, so whether or not you filter by the vocabulary, you get the same result:
directory/term/$tid == directory/vocabulary/$vid/term/$tid (provided that $term->vid == $vid, otherwise the second returns an empty set).

So, why bother with #4?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Mwehehe, stupidity on my side. Just ignore that. :)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new per-vocabulary_directory.patch3.65 KB

Normally I don't submit patches until delivery to client, but I will make an exception in this case, so you can continue your work.

Patch attached. Had to re-roll because you had just commited a new version :P

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Note that I also cleaned up two pieces of code: hook_menu() and theme_directory_home_vocabulary(). The latter one is different now, to link to the per-vocabulary directories.

beginner’s picture

beginner commented
Status: Needs review » Needs work

here:
1) the best is not to mix code style fixes with new functionality. The former can be committed much quicker. Since the patch needs to be redone (see below), I have committed a code-style fix for hook_menu(), so you don't have to do it again.

2) The functionality doesn't work. After applying the patch, I go to directory/vocabulary/nnn but I get the same output as on the main page.
??

3) In the function theme_directory_home_vocabulary(), why do you change $terms = taxonomy_get_children(0, $vid); to $terms = taxonomy_get_tree($vid);

4) a minor point: be careful not to add extra spaces: it makes the patch longer than it really is, and harder to read.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

1) Sorry for the code style fixes in the same patch. But honestly, I do this because when I see incorrect code style, it drives me nuts and I cannot stand modifying code that's not conform to the standards. I know I'll have to get rid of this bad habit.

2) It works here... note that you should have multiple vocabularies to see any difference. And the vocabulary must be in the allowed vocabularies as well:

  // If no valid vid is specified, show the entire directory.
  if ($vid < 1 || !is_numeric($vid) || !in_array($vid, $allowed_vids)) {

3) Because $terms = taxonomy_get_children(0, $vid); is theoretically incorrect code: you request the children of the term with tid 0, which obviously does not exist. What you really want, is to get the entire vocabulary tree, hence I changed it to $terms = taxonomy_get_tree($vid);

4) Sorry again.

beginner’s picture

beginner commented

re 1 and 4: no need to apologize. Those were minor points.
Anyway, you are right: code style stuff can be and ought to be fixed. My point was to submit separate patches for them, because reviewing them is usually easier and the patch can be committed very rapidly. Then we are free to work on the real stuff.
What I do, is that I have a separate checkout of the directory module for such small stuff. So, I work on a big patch on one side, and when I see a small code style thing, I correct it in the other side and I can commit it straight away.
If you did the same, you could easily give be a patch with only code style fixes (i.e. without any other changes you are working on).

2) I do have multiple vocabularies and they are allowed. It seems that there is a callback issue.

3) I'd need to check that. I remember I switched someplace from one to the other for a reason.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

The difference is that you can commit your fixes right away, and I not. It'd be a complete pain to make separate patches, for obvious reasons.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Any news on this? What do you want me to change?

beginner’s picture

beginner commented

It occurs to me why the patch didn't work for me: it may have been due to caching. I'll make sure I empty my cache next time.
The patch needs to be rerolled (it won't apply, now), and I'll review it and commit it early next week.

beginner’s picture

beginner commented
Assigned: wim leers » beginner
Status: Needs work » Needs review
StatusFileSize
new per-vocab-directory.patch3.04 KB

This patch seems a better approach.

1) you have a menu item you can enable for each vocabulary.
2) all invalid values of $tid and $vid are redirected to the main directory page
3) the js collapsed setting is not used if we are only listing one vocabulary.
4) the listing for each vocabulary uses the same theming function as for the main page.

What do you think?

beginner’s picture

beginner commented
Status: Needs review » Fixed

I guess there was no objection to the patch.
Committed.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)