Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Issue tags: +Novice

To be more explicit: We recently adopted a new standard way to document "callback functions" -- see
http://drupal.org/coding-standards/docs#callback-def
for the standard and
#1250500: [policy adopted; patch done] Find a (standard) way to document callback signatures
for the issue where this was adopted.

In both Drupal 7 and 8, hook_search_info() uses a "conditions callback", and this issue is asking for this to be documented as a callback function... probably it should be called callback_search_conditions(), and its definition should be put into core/modules/search/search.api.php and referred to in hook_search_info(). In addition, we should check the core implementations of hook_search_info() to see if they are defining any conditions callback functions, and if so, make sure their documentation says "Implements callback_search_conditions()." as the first line (probably).

Might be a good Novice project?

aitiba’s picture

Status: Active » Needs review
FileSize
1.99 KB

This is my first patch to an issue, so I'm novice. This is what I do.

joachim’s picture

> Might be a good Novice project?

Are you sure? I didn't tag it as such because install profiles are a fairly hairy topic! But I see we have a novice taking a stab at it, so who am I to judge! :D

joachim’s picture

Oops. I am commenting on the wrong issue. Sorry!!!

jhodgdon’s picture

Status: Needs review » Needs work

Excellent start!

A few things to fix:

a)

 /**
- * Implements hook_search_info().
+ * Implements callback_search_conditions().
  */
 function search_extra_type_search_info() {

I don't think this change is correct. Based on the function name, I think this is actually an implementation of hook_search_info() not a conditions callback. There is another function in there called search_extra_type_conditions() that is actually the conditions callback; that function doc should be changed to say "Implements callback_search_conditions()."

b) In the hook_search_info() documentation in search.api.php, I'd like to see the wording "... an implementation of callback_search_conditions()..." in the documentation, rather than "Sample callback function: callback_search_conditions()".

c) In search.api.php, I think we need to change the first line of the callback_search_conditions() function doc to conform to our standards for these callback functions. It should be something like "Provide search query conditions."

See http://drupal.org/coding-standards/docs#callback-def for standards for these changes.

Thanks!

aitiba’s picture

Status: Needs work » Needs review
FileSize
2.72 KB

Fixed (or try to do it!) :-)

joachim’s picture

+++ b/core/modules/search/search.api.php
@@ -46,22 +42,29 @@ function hook_search_info() {
+ * @param $keys
+ *   Name of a callback function that is invoked by

Does this callback get another callback as a parameter? O_o

jhodgdon’s picture

Status: Needs review » Needs work

Thanks! The changes to hook_search_info() documentation look perfect. And the changes to the file search_extra_type.module look good, except that you have added an extra space to the end of one line, and there should not be a blank line at the end of the documentation block:

+ *
  */
 function search_extra_type_conditions() {

The documentation for callback_search_conditions() needs some work though:
- The first line is good.
- The next line about hook_search_info() is good.
- After that, we should have this text that was previously in hook_search_info() (with a small change):

This callback is invoked by search_view() to get an array of additional search conditions to pass to search_data(). For example, a search module may get additional keywords, filters, or modifiers for the search from the query string.

- After that, we should have the text that was already in the sample callback (in a new paragraph):

 *
 * This example pulls additional search keywords out of the $_REQUEST variable,
 * (i.e. from the query string of the request). The conditions may also be
 * generated internally - for example based on a module's settings.

- Then we need @param $keys, and the text for that should be something like "The search keywords string."
- Then we need the @return. The text in the patch is good for that, except it should not say "Defaults to NULL" (that does not make sense in a return value).
- The documentation block must not have a blank line between it and the code:

  */
+
+function callback_search_conditions($keys) {
aitiba’s picture

Status: Needs work » Needs review
FileSize
2.63 KB

Thanks jhodgdon for the detailed comment! Very grateful for that.

A new patch with the commented things fixed.

jhodgdon’s picture

Status: Needs review » Needs work

Great! You added one extra space at the end of the /** for function search_extra_type_conditions(), but other than that, the patch looks good to me!

We don't ever want extra spaces at ends of lines in Drupal code... If you plan to keep doing Drupal patches, try to set up your code editor to either remove spaces at the end of lines, or to show you spaces at the end of lines.

joachim’s picture

Some text editors have a command to strip all trailing whitespace (eg TextMate which I use does). It's a good habit to get into just hitting the shortcut for that at the same time as you save :)

Also, some git GUIs show trailing whitespace in the diff. I think git diff on the terminal does too.

aitiba’s picture

Status: Needs work » Needs review
FileSize
2.62 KB

Fix it!

As I have intention to continue making patches I set up my editor. On save a document, it remove the trailing white space/s

As help: To remove trailing white space on save on subline text 2, set to TRUE "trim_trailing_white_space_on_save" clicing on Preferences -> Settings - Default

jhodgdon’s picture

Assigned: Unassigned » jhodgdon
Status: Needs review » Reviewed & tested by the community

Looks good! I'll get this committed soon, unless someone else beats me to it.

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Patch (to be ported)

Thanks again! Committed to 8.x

The patch does not apply to 7.x and needs to be ported.

bdgreen’s picture

Status: Patch (to be ported) » Needs review
FileSize
3.6 KB

Backport to 7.x ... ;)

jhodgdon’s picture

Assigned: Unassigned » jhodgdon
Status: Needs review » Reviewed & tested by the community

Looks good, thanks! I'll get it committed shortly.

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Fixed

Thanks again! Committed to 7.x.

joachim’s picture

I think the D8 patch may have been missing an ingroup/addtogroup: http://api.drupal.org/api/drupal/core!modules!search!search.api.php/func... -- this is showing in the 'hooks' group.

jhodgdon’s picture

Version: 7.x-dev » 8.x-dev
Status: Fixed » Needs work

Ah, yes. The callback_search_conditions() function in search.api.php should be taken out of the @addtogroup hooks {} section that is in search.api.php, and it should have @ingroup callbacks added to it. I missed that -- thanks for finding it joachim! Needs a quick follow-up patch for 8.x/7.x.

bdgreen’s picture

Status: Needs work » Needs review
FileSize
2.82 KB

Hmm :-( Revision due to #19 ...

jhodgdon’s picture

Status: Needs review » Needs work

Thanks! That's just about right; two little things to fix:
- I think we still want it to have @ingroup search as well as @ingroup callbacks.
- No newline at end of file

bdgreen’s picture

Status: Needs work » Needs review
FileSize
2.81 KB

@jhodgdon returned @ingroup search and added newline :$ - thanks!

jhodgdon’s picture

Assigned: Unassigned » jhodgdon

Looks good, thanks! I'll get it committed shortly.

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Assigned: jhodgdon » Unassigned
Status: Needs review » Patch (to be ported)

Thanks again! Committed to 8.x. Needs backport to 7.x.

sarahdavies’s picture

Status: Patch (to be ported) » Needs review
FileSize
2.81 KB

Trying a backport for the first time.

jhodgdon’s picture

Status: Needs review » Fixed

The file is missing a newline at the end... but other than that the patch is good. To save time, I just added the newline and committed the patch. Thanks!

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