hook_filter_info() uses several callback functions.

We have a new standard for how to document callback functions:
http://drupal.org/coding-standards/docs#callback-def

The existing callback documentation for functions hook_filter_FILTER_* should be changed to use our new standard. For instance:
hook_filter_FILTER_tips() should be renamed to callback_filter_tips(), and it should be given 

@ingroup callbacks

This needs to be changed in three places I know of:
- in the hook_filter_info() documentation block
- in the sample function body for hook_filter_info()
- in the function definition for hook_filter_FILTER_tips()
but we'll also need to see if it is referenced anywhere else in code.

Also, we should fix up any core implementations of hook_filter_info() so that their callback function documentation is changed to read "Implements callback_filter_tips()." instead of whatever it says now.

The same needs to happen for the other hook_filter_FILTER functions mentioned in the hook_filter_info() documentation.

This will take a little time, but it seems like a fairly straightforward project, so I'm marking it Novice.

It is a 7.x project only, since filters in 8.x use a class instead (at least I think so).

CommentFileSizeAuthor
#14 filter_callback_docs-2002182-14.patch10.13 KBw00zle
#8 filter_callback_docs-2002182-8.patch10.37 KBw00zle
#6 filter_callback_docs-2002182-6.patch4.93 KBw00zle
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Samvel’s picture

Samvel CreditAttribution: Samvel commented
Assigned: Unassigned » Samvel
jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Samvel: Thanks! Are you still working on this?

Samvel’s picture

Samvel CreditAttribution: Samvel commented

Yes, i will do it. i took this task beforehand.

Samvel’s picture

Samvel CreditAttribution: Samvel commented
Assigned: Samvel » Unassigned

Now i haven't time, sorry

w00zle’s picture

w00zle CreditAttribution: w00zle commented
Assigned: Unassigned » w00zle

I will take a look at this.

w00zle’s picture

w00zle CreditAttribution: w00zle commented
Status: Active » Needs review
FileSize
filter_callback_docs-2002182-6.patch4.93 KB

I have attached a patch for review. There did not appear to be that many implementations of these callbacks. If that seems intuitively wrong to anyone familiar with the core code, please check my patch.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

Thanks, this is a great start, and most of the patch is looking good!

A few things need updates before the patch is quite ready though:

a) Let's take out the paragraphs in the filter.api.php callback function documentation that start with "Note: This is not really a hook..." -- we don't need those any more.

b) For the filter.api.php callback functions, their first couple of lines need to be updated. They're currently something like "Settings callback for hook_filter_info()" and they need to follow the standards in
https://drupal.org/coding-standards/docs#callback-def
So it should be something like:

/**
 * Provide a settings form for filter settings.
 *
 * Callback for hook_filter_info().
...

c) The actual callback functions, such as the ones in filter.module... The first line should be something like:

/**
 * Implements callback_filter_settings().

And you can remove the paragraph about "See ... for documentation of the parameters and return value".

The change you made in php.module is probably good though, since this function has more than one purpose.

d) You are correct -- there are not too many filters in Drupal Core... Let's see. This page shows which functions implement hook_filter_info():
https://api.drupal.org/api/drupal/modules!filter!filter.api.php/function...
There are only 3.

1. Looking at php_filter_info(), it looks like there is a process callback and a tips callback, and you've taken care of both of those, great!

2. Looking at filter_test_filter_info(), it looks like the process callback in that module needs to be added to this patch.

3. Looking at filter_filter_info(), it looks like there are quite a few callback functions in filter.module that need to be updated. This patch only takes care of one of them, so the rest also need to be added to the patch.

w00zle’s picture

w00zle CreditAttribution: w00zle commented
Status: Needs work » Needs review
FileSize
filter_callback_docs-2002182-8.patch10.37 KB

Thanks for the detailed feedback, jhodgdon! I believe I have made all of the changes you pointed out in your post. It's worth pointing out that I found quite a few comments like this:

Filter tips callback: Provides help for the HTML filter.

While reworking them, I removed the part before the colon ("Filter tips callback"), but kept the part after it, so the final comment would be something like:

/**
 * Implements callback_filter_tips().
 *
 * Provides help for the HTML filter.
 *
 * @see filter_filter_info()
 */

I hope that this is the desired format. If not, please let me know and I will roll a new patch. I appreciate you taking the time to work with me on this!

Status: Needs review » Needs work

The last submitted patch, filter_callback_docs-2002182-8.patch, failed testing.

w00zle’s picture

w00zle CreditAttribution: w00zle commented

I'm going to be honest; I can't decipher why my patch failed. Can anyone give me some pointers?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

RE #8 - that is exactly correct. Thanks! RE: patch failure, I think the test bot had a glitch. I'll hit retest and see if we can get this to pass.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs work » Needs review

#8: filter_callback_docs-2002182-8.patch queued for re-testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

Anyway, I also reviewed the patch, and it is looking really good!

The only things I would change now would be:

a) callback_filter_tips() ==> I think the first line should probably not say "Provide a callback to define tips..." but instead just "Return help text for a filter.". (The function itself *is* a callback; it is not *providing* a callback.)

b) php.module --> The process callback for the PHP filter: Since this function can be used as both a filter process callback and as a PHP evaluator (stand-alone outside the filtering system), I think we should leave the first line as it previously was and leave out the "Implements callback_filter_process()." line entirely.

Other than that, looks great, thanks!

w00zle’s picture

w00zle CreditAttribution: w00zle commented
Status: Needs work » Needs review
FileSize
filter_callback_docs-2002182-14.patch10.13 KB

Thanks so much for all of your help! I have made the changes to the callback_filter_tips comment and php.module, so with any luck this patch should do the trick. Let me know if anything else needs to be changed.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Reviewed & tested by the community

Thanks! This is ready to go.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Reviewed & tested by the community » Fixed

Thanks again! Committed to 7.x. Great work!

w00zle’s picture

w00zle CreditAttribution: w00zle commented

Awesome! Thanks for all of your support! As you might have noticed, this was my first attempt at grabbing a core issue and patching it, so I really appreciate all the help.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

You picked a kind of complicated issue to start with, as documentation issues go. You did a great job, and I hope you continue to contribute by patching core issues. See you in the issue queue!

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