API page: http://api.drupal.org/api/drupal/modules--taxonomy--taxonomy.module/func...

Describe the problem you have found:

No return value described in the documentation. From the code I would say it's an integer with 0, 1 or 2. Something like:

@return lowest level of hirarchy of the given vocabulary.

Could be added to the docs.

(I see that IN the PhpDoc the 0,1,2 stuff is described, but the RETURN value is not separatly described, which gets used in may IDEs for Intellisense and also in the Drupal Docs it creates an extra H3 tag, which is more visible and eyecatching)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Title: Documentation problem with taxonomy_check_vocabulary_hierarchy » taxonomy_check_vocabulary_hierarchy doc lacks return value
Version: 7.2 » 8.x-dev
Issue tags: +Needs backport to D7

Thanks for reporting. 8.x, then backport to 7.x

James_Stallings’s picture

Status: Active » Needs review
FileSize
593 bytes
James_Stallings’s picture

Text editor blew up the spacing in previous patch

James_Stallings’s picture

Let's try this again..

jhodgdon’s picture

Status: Needs review » Needs work

This is almost OK, but:

a) While you're editing this function's doc, you could also update the verb tense in the first line (check -> checks) ... and probably get rid of that word "dynamically", which seems utterly meaningless to me?

b) In the description paragraph:
updates the vocabularies hierarchy
->
vocabulary's hierarchy

c) and:
If any term contain multiple
->
contains

d) You need a blank line between the @param and @return section.

e) In your patch:
of the $vocabulary's hierarchy.
-->
of the vocabulary's hierarchy
or
of $vocabulary's hierarchy
(Either use it as a variable or don't use it as a noun.)

James_Stallings’s picture

Sounds good. I'll make those adjustments.

James_Stallings’s picture

Assigned: Unassigned » James_Stallings
James_Stallings’s picture

I modified the description further. The sentence describing the first return condition didn't make sense.

James_Stallings’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

Hmmm... I have a few suggestions:

a) There are spaces at the ends of several lines. (Many programming editors have an option to highlight or remove end-of-line text -- you might check and see if you can turn that on in your editor.)

b) If any term contain multiple parents
contain -> contains

c) Actually, I don't like the word "contain" in this paragraph. Terms don't "contain" parents -- if anything, it's the other way around. Probably it would be better to change it back to have/has. This phrase in particular I thought was confusing: "If no term contains a single parent..." -- I don't think it's as clear as the original... How about "If no term has parent terms"?

James_Stallings’s picture

Status: Needs work » Needs review
FileSize
1.48 KB

Thanks, I've made the recommended changes. BTW is there a way to receive email notifications when a new comment has been added to a post your user account is active on.

jhodgdon’s picture

Status: Needs review » Needs work

To receive notifications, go to http://drupal.org/project/issues/drupal and click on Subscribe. If you choose "Own issues" you will get email for updates on any issue you either created or have commented on.

So... This patch is still using the word "contains" in a way that I think is confusing:

If any term contains a single parent
If any term contains multiple parents

Could you change those to "has" like you did for the first one? Thanks!

James_Stallings’s picture

Assigned: Unassigned » James_Stallings
Status: Needs work » Active
jhodgdon’s picture

Status: Active » Needs work

status changed back

James_Stallings’s picture

Status: Needs work » Needs review
FileSize
1.47 KB
jhodgdon’s picture

Issue tags: +Needs backport to D6

Thanks! This version reads well to me. 8.x/7.x please... Then it looks like we should backport to Drupal 6, which has the same problem (and pretty much if not exactly the same function code).

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

Version: 8.x-dev » 6.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: -Needs backport to D7

Committed and pushed to 8.x and 7.x. Thanks!

Marking to be ported for 6.x.

James_Stallings’s picture

Assigned: James_Stallings » Unassigned
jhodgdon’s picture

Issue tags: +Novice

The backport to D6 should be a good novice project.

noisetteprod’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.47 KB
jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, thanks!

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Thanks, committed, pushed.

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