Clean up API docs for search module

@Sree: Are you still planning to work on this issue? If so, please respond. Otherwise, we'll come back in a few days and unassign it so someone else can. Thanks!

First attempt. Not ready for review yet.

Ready for a first review.

Thanks -- looks pretty good! I noticed a few things that need fixing (just looking at the patch so far; I didn't go look at the files to see if anything not in the patch needed fixing too):

a)

+++ b/core/modules/search/search.admin.inc
 @@ -14,7 +14,7 @@ function search_reindex_confirm() {
 }
 
 /**
- * Handler for wipe confirmation
+ * Handler for the wipe confirmation.
  */
 function search_reindex_confirm_submit(&$form, &$form_state) {

Isn't this a submit handler for a form? If so, http://drupal.org/node/1354#forms

b)

@@ -41,13 +41,13 @@ function _search_get_module_names() {
 }
 
 /**
- * Menu callback: displays the search module settings page.
- *
- * @ingroup forms
+ * Menu callback: Displays the Search module settings page.

Should say Page callback... Hmm... Actually it looks like it's a form generating function?

c)

@@ -140,6 +140,8 @@ function search_admin_settings($form) {
 
 /**
  * Form validation handler for search_admin_settings().
+ *
+ * @see search_admin_settings()
  */
 function search_admin_settings_validate($form, &$form_state) {

@see should be omitted here. See above link for form generation functions standards. Same for the submit handler in the next block.

d)

/**
  * Form submission handler for reindex button on search_admin_settings_form().
+ *
+ * @see search_admin_settings_form()
  */
 function search_admin_reindex_submit($form, &$form_state) {

I think it's actually search_admin_settings() without form() on the end? And no @see as in (c).

e)

+++ b/core/modules/search/search.api.php
@@ -11,30 +11,30 @@
  */
 
 /**
- * Define a custom search type.
+ * Defines a custom search type.

This is a hook. Wrong verb tense. See http://drupal.org/node/1354#hooks -- same for all other hooks in this file.

f)

+++ b/core/modules/search/search.extender.inc
@@ -36,7 +37,7 @@ class SearchQuery extends SelectExtender {
   protected $searchExpression;
 
   /**
-   * Type of search (search module).
+   * Type of search (Search module).

This is actually referring to the module that initiates the search, not the Search module itself, so should be left lower-case.

g)

@@ -56,7 +57,7 @@ class SearchQuery extends SelectExtender {
   /**
    * Indicates whether the first pass query requires complex conditions (LIKE).
    *
-   * @var boolean.
+   * @var Boolean.

Actually should be bool and should not end in . -- see http://drupal.org/node/1354#classes and http://drupal.org/node/1354#param-return-data-type -- and this applies to other @var lines in this file.

h)

@@ -404,6 +405,10 @@ class SearchQuery extends SelectExtender {
+   *
+   * @return object
+   *   An object that contains scores, scoresArguments, and multiply if it
+   *   exists.
    */
   public function addScore($score, $arguments = array(), $multiply = FALSE) {

This is not accurate. The function returns $this, so it can be chained. I'm not sure how we're documenting that, but look at the other database API functions, such as SelectQuery->fields() etc.

i) search.module

@@ -297,14 +297,14 @@ function search_get_default_module_info() {
 }
 
 /**
- * Access callback for search tabs.
+ * Access callback: Determines access for the search tabs.
  */
 function _search_menu_access($name) {

Needs @see to search_menu(), see http://drupal.org/node/1354#menu-callback

j) search.module

@@ -1006,10 +1006,11 @@ function search_form($form, &$form_state, $action = '', $keys = '', $module = NU
 }
 
 /**
- * Form builder; Output a search form for the search block's search box.
+ * Form builder: Output a search form for the search block's search box.

Doesn't follow form-generating function standards. Also, the following function is the submit handler and needs to be documented as such.

k) search.pages.inc, top of file:

/**
- * Menu callback; presents the search form and/or search results.
+ * Menu callback: Presents the search form and/or search results.
  *

Should say Page callback.

l) search.pages.inc

@@ -73,12 +73,13 @@ function search_view($module = NULL, $keys = '') {
 
...

+ * @param $variables
+ *   The $variables array contains the following arguments:
+ *   - $results: Search results array.
+ *   - $module: Module the search results came from (module implementing
+ *     hook_search_info()).

Should be something more like:

 *   An array with the following elements:
 *    - results: Search results array.
...

And the same applies to the next function.

m) search.pages.inc:

 /**
- * Process a search form submission.
+ * Processes a search form submission.
  */
 function search_form_submit($form, &$form_state) {

Isn't this a form submission handler?

n) search.test

-   * Verify if a query produces the correct results.
+   * Verifies if a query produces the correct results.
    */
   function _testQueryMatching($query, $set, $results) {

Maybe "Verifies that..."? (same for the other _test functions in the file). Also this type of wording:

+ * Tests if the search form block is available and working.

Should probably be "Tests that...", which is sometimes used in this file (let's be consistent).

o) This made me laugh in search.test:

+   * Verifies if a query produces normalized, monotonous scores.
    */
   function _testQueryScores($query, $set, $results) {

monotonous -> monatomic (I mean, they might be boring, but I think what was meant was uniformly increasing).

p)

@@ -1022,14 +1042,14 @@ class SearchExpressionInsertExtractTestCase extends DrupalUnitTestCase {
 }
 ...
+ * Nodes with comment status set to Open should always show comment counts.
+ * Nodes with comment status set to Closed should show comment counts only when
+ * there are comments. 
+ * Nodes with comment status set to Hidden should never show comment counts.

This either needs to be made into a list or a paragraph (probably paragraph). Wrapping is not good currently.

q)

@@ -1922,6 +1942,9 @@ class SearchPageOverride extends SearchWebTestCase {
     menu_rebuild();
   }
 
+  /**
+   * Tests that hook_search_page runs.
+   */
   function testSearchPageHook() {

hook_search_page() needs ()

I will try to correct the issues identified, but with the addition of new subfolders I have a question.
The SearchQuery.php file starts off with

<?php

/**
 * @file
 * Definition of Drupal\search\SearchQuery.
 */

namespace Drupal\search;

use Drupal\Core\Database\Query\SelectExtender;
use Drupal\Core\Database\StatementEmpty;

/**
 * @file
 * Search query extender and helper functions.
 */

I'm not sure if this is correct or not.

I also don't know if you actually meant monatomic or more likely monotonic. The definition for monatomic is - Consisting of one atom.

Hopefully the next version will be better. I have problems with verb tense and I'm still learning the doc standards. Thank you for taking the time to explain things.

Additionally, for c) why should the @see be omitted? It is listed in the example for both validation and submission handlers.
for h) tim.plunkett was kind enough to point me to #1336726: Document which database methods can be chained (make sure it's correct) but since it is still open I'm not sure what the right answer is here either. I also couldn't find any examples that had the return documented.

This is only a partial patch and I still need to go through the new files in the directory along with the answers to the questions noted above.

Re: #6: It should be omitted because it's redundant.

• The form constructor should have @see to both its validation and submission handlers.
• The validation and submission handlers should have @see to each other, but they don't need to have @see to the constructor because it is already linked in the function summary, e.g.:
  Form validation handler for my_form_constructor_here_form()

This is actually exactly what's documented in the examples at http://drupal.org/node/1354#forms. :)

Oh, regarding #5, there's an issue for that too, when a file defines one class. Let me see if I can find it.

Edit: Sorry for the noise. See the @file docs under "Files containing the definition of exactly one class": http://drupal.org/node/1354#files. The original issue was #1392754: Comply with new documentation standards for @file for namespaced class files.

Edit 2: snarky aside at snarky comment:

I also don't know if you actually meant monatomic or more likely monotonic. The definition for monatomic is - Consisting of one atom.

I imagine you probably can infer that it's neither boringly repetitive nor one atom in size. Quantum computing hasn't advanced that far yet. ;)

xjm,

RE #7 That makes sense. I don't know why I didn't get it at first.
RE #5 Maybe I wasn't clear but the file has two @file in the file. Is this correct? I would think that it is wrong but I don't know of anywhere that specifies you cannot have two @file in one file.

For #4 Comment h) should I list the return as

 * @return
 *   The called object.

Oh! Haha. Yeah two file docblocks is definitely wrong. If there is only one class in the file (which I think there must be for PSR-0):

• Delete the second docblock.
• Move the first down below the namespace declaration. (The namespace has to be the first thing in the file, as far as I understand.)

Edit: Regarding the return value, how about something like "The updated query object" or something?

In both Select.php and Query.php the @file is above the namespace declaration so I'm going to leave it there for now. If after reviewing the others you think it should still be moved I will.

Also in search.test it has

  /**
   * _test_: Helper method for generating snippets of content.
   *
   * Generated items to test against:
   *   1  ipsum
   *   2  dolore sit
   *   3  sit am ut
   *   4  am ut enim am
   *   5  ut enim am minim veniam
   *   6  enim am minim veniam es cillum
   *   7  am minim veniam es cillum dolore eu
   */

Should I change the description or just update the list formatting?

I'm having problems creating the interdiff. I'll upload it if I get what I think you are looking for.

RE question in #11 - yes definitely change the description! And update the list formatting. :)

Here is the updated patch.

I'm also attaching an interdiff from #3 but I'm still not sure if this is the right way to do it or not.

This patch looks great to me. I only found a few things:

1. +++ b/core/modules/search/lib/Drupal/search/SearchQuery.phpundefined
   @@ -11,19 +11,14 @@ use Drupal\Core\Database\Query\SelectExtender;
     * Do a query on the full-text search index for a word or words.
   

   I think this should begin with a third-person verb; maybe "performs"?

2. +++ b/core/modules/search/search.api.phpundefined
   @@ -179,17 +180,17 @@ function hook_search_admin() {
   + *   - link: Required. The URL of the found item.
   ...
   + *   - title: Required. The name of the item.
   

   I think @jhodgdon previously suggested elsewhere marking "required" and "default" keys similarly to "optional" ones, e.g.:

   - link: (required) The URL of the found item.
   

3. +++ b/core/modules/search/search.pages.incundefined
   @@ -73,12 +73,13 @@ function search_view($module = NULL, $keys = '') {
   + *   - $results: Search results array.
   + *   - $module: Module the search results came from (module implementing
   + *     hook_search_info()).
   
   @@ -95,11 +96,13 @@ function template_preprocess_search_results(&$variables) {
   + *   - $result: Search results array.
   + *   - $module: Module the search results came from (module implementing
   + *     hook_search_info()).
   

   I think $result and $module should not have $; they are just string literal keys for the array (even though they are used as variables in the theme layer).

4. +++ b/core/modules/search/search.testundefined
   @@ -71,16 +77,16 @@ class SearchMatchTestCase extends SearchWebTestCase {
   +   * Generates snippets of content for _test_.
   
   @@ -88,14 +94,14 @@ class SearchMatchTestCase extends SearchWebTestCase {
   +   * Generates snippets of content for _test2_.
   

   I guess the only remaining thing is to explain what _test_ and _test2_ are (I meant to mention this earlier but forgot, sorry).

5. +++ b/core/modules/search/search.testundefined
   @@ -559,7 +576,7 @@ class SearchRankingTestCase extends SearchWebTestCase {
   +   * @see http://drupal.org/node/771596 (issue)
   
   @@ -1022,14 +1045,14 @@ class SearchExpressionInsertExtractTestCase extends DrupalUnitTestCase {
   + * @see http://drupal.org/node/537278 (issue)
   

   We need to remove the (issue) from the end of these lines; the @see http://example.com should be all by itself on the line.

6. +++ b/core/modules/search/search.testundefined
   @@ -1600,11 +1622,10 @@ class SearchExcerptTestCase extends DrupalUnitTestCase {
   +   * Passes keywords and a sample marked up string, "The quick brown fox jumps
   +   * over the lazy dog", and compares it to the correctly marked up string. The
   +   * correctly marked up string contains either highlighted keywords or the
   

   I think "marked-up" should be hypenated here. Or maybe just "a string with markup" or something.

7. +++ b/core/modules/search/tests/modules/search_embedded_form/search_embedded_form.moduleundefined
   @@ -4,9 +4,9 @@
   + * Embedded forms are important, for example, for ecommerce sites where each
   + * search result may includ an embedded form with buttons like "Add to cart" for
   + * each individual product (node) listed in the search results.
   

   This test predates the patch, of course, but this docblock contains several errors. "include" is misspelled; I think "e-commerce" should be hyphenated; and there's no need to editorialize about the forms being important. I'd just say something like "A sample use of an embedded form is..."

Regarding #4 at the top of the search.test file it is explained as

// The search index can contain different types of content. Typically the type is 'node'.
// Here we test with _test_ and _test2_ as the type.
const SEARCH_TYPE = '_test_';
const SEARCH_TYPE_2 = '_test2_';
const SEARCH_TYPE_JPN = '_test3_';

Should I do @see SEARCH_TYPE
The only problem with this is that they have single line comments which do not show up in the documentation.

Interdiff from #13

Actually disregard the last. I will add docblocks for the constants as required #constants

Here is the latest version of the patch. I'm having problems creating a diff against #13 but there is a diff from that to 15 and here is the one from 15 which only adds the @see to the constants and the docblocks for the constants.

I took a look at the patch in #17, and it is mostly very good! However, it still has a few problems:

a) Why is this patch removing the @file block here?

--- a/core/modules/search/lib/Drupal/search/SearchQuery.php
+++ b/core/modules/search/lib/Drupal/search/SearchQuery.php
@@ -11,19 +11,14 @@ use Drupal\Core\Database\Query\SelectExtender;
 use Drupal\Core\Database\StatementEmpty;
 
 /**
- * @file
- * Search query extender and helper functions.
- */

b) SearchQuery.php

-   *   The search module. This maps to {search_index}.type in the database.
+   *   The Search module. This maps to {search_index}.type in the database.

"Search" should not be capitalized here. It is not referring to "The Core Search Module", it is referring to any add-on module that performs searches.

c) search-result.tpl.php

+ * search-results.tpl.php. This and the parent template are dependent to one
+ * another sharing the markup for definition lists.

- dependent to -> dependent on
- I think there should be a comma after "another".

d) search-result.tpl.php has the same problems as (c).

e) search.admin.inc

 /**
- * Menu callback: confirm wiping of the index.
+ * Menu callback: Confirm wiping of the index.
  */
 function search_reindex_confirm() {

Confirm -> Confirms

f) search.admin.inc

 /**
- * Handler for wipe confirmation
+ * Form submission handler for the wipe confirmation.
+ *
+ * @see search_reindex()
+ * @see search_reindex_confirm()
  */
 function search_reindex_confirm_submit(&$form, &$form_state) {

First line - see http://drupal.org/node/1354#forms - should have search_reindex_confirm() in it, not "for the wipe confirmation". This also applies to search.module:

+ * Processes a block search form submission.
  */
 function search_box_form_submit($form, &$form_state) {

g) search.module

+ * @param int $name
+ *   The account ID to check.
+ *
+ * @return bool
+ *   TRUE if a user has access to the search tabs; FALSE otherwise.
+ *
+ * @see search_menu()
  */
 function _search_menu_access($name) {

This parameter description is not correct. $name is the name of a search tab (i.e., search module), not a user ID. And the @return should then say "TRUE if the current user has access to the given search tab; FALSE otherwise.".

h) search.pages.inc - doc for template_preprocess_search_results():

/**
- * Process variables for search-results.tpl.php.
+ * Processes the variables for search-results.tpl.php.
  *
- * The $variables array contains the following arguments:
- * - $results: Search results array.
- * - $module: Module the search results came from (module implementing
- *   hook_search_info()).
+ * @param $variables
+ *   An array with the following elements:
+ *   - $results: Search results array.
+ *   - $module: Module the search results came from (module implementing
+ *     hook_search_info()).

- First line should probably say preprocesses, not processes?
- $variables elements are not "$results" but "results", etc.
- Same two comments apply to template_preprocess_search_result() immediately after this function.

i) search.test

+ * Nodes with comment status set to Open should always show comment counts.
+ * Nodes with comment status set to Closed should show comment counts only when
+ * there are comments, and nodes with comment status set to Hidden should never
+ * show comment counts.
  *
- * - Nodes with comment status set to Open should always how comment counts
- * - Nodes with comment status set to Closed should show comment counts
- *     only when there are comments
- * - Nodes with comment status set to Hidden should never show comment counts
+ * @see http://drupal.org/node/537278
  */
 class SearchCommentCountToggleTestCase extends SearchWebTestCase {

I actually think this was clearer with the original list notation. Why was the list removed?

tagging

This patch no longer applies

@jhodgdon there were 2 @file blocks it seems.

Here's a re-roll trying to take into account the comments in #18.

This looks good -- thanks to all who participated -- committed to 8.x!

Let's backport as much as we can of this to 7.x as well. The tests will of course be in different locations/files, but the rest will probably port quite easily.

Backported #21 to D7.

These issues are a lot of work with very little tangible payoff, so I'm closing the rest of them as "won't fix". Your efforts on working on this issue were appreciated... it was just my fault for starting a task that was very difficult to get right.

Let's instead put our effort into fixing and reviewing documentation that is really unclear and/or wrong, and I hope that the people who worked on these issues are not afraid to jump into a more reasonable issue!