Clean up API docs for locale module

Hi Lars, based on the documentation cleanup issue it looked like you were no longer working on this issue, so I am assigning it to myself. Let me know if that is wrong.

Here's a patch.

We had a discussion on #1315992: No standard for documenting menu callbacks and we have (unfortunately) changed the standard for hook_menu() callbacks:
http://drupal.org/node/1354#menu-callback

So this patch will need an update. In particular, we're not putting in the Path: lines any more (not on Forms either).

tagging

Another thing Gábor pointed out this morning is that a lot of the adjusted class docblocks don't begin with verbs (and they should). Reference: http://drupal.org/node/1354#classes

The attached patch includes the updates from comment #3 above, and attempts to follow the menu-callback and class docblock changes.

I'll go ahead and review this for the sprint.

Patch in #8: Alright, this patch is HUGE, but it shows a lot of hard work done documenting returns and params, and doing verb correction and capitalization. Any problems with this patch are far fewer than the number of real fixes it provides. I'd say if we can get one more set of eyes on this we can get it rolling

Thanks for the partial review... We actually *do* need someone to go over this very carefully and not just say "it's better than what was there" but "it's correct". The reason is that once the patch is finalized and committed, it's unlikely someone is going to come back and fix the remaining problems, so we are trying to get them all taken care of on the first pass.

So ... what we're looking for is for someone to go over the patched module and verify that everything listed at
#1310084: [meta] API documentation cleanup sprint
has been taken care of, and that in general the documentation in the module is correct and good.

Understood. I did actually do a rigorous review of the patch, though considering it's extreme length I don't have the time to ensure that it's 100% for all of locale. Perhaps I shouldn't have doubted myself.

If I resubmitted a patch with the two debatable fixes I can remember off the top of my head we'd still be in the same boat of needing another set of eyes to ensure there's nothing else that needs to be added and that my patch isn't corrupted.

Unless anyone else wants to grab it, I'll re-assign to myself when I get back home from Denver and ensure that anything questionable (no matter how small) it dealt with and resubmitted.

Thanks for the patch and review! Looks pretty good; there are still a few things to be fixed:

a)

--- a/core/modules/locale/locale-rtl.css
+++ b/core/modules/locale/locale-rtl.css
@@ -1,3 +1,7 @@
+/**
+ * @file
+ * RTL CSS for locale.module.
+ */

This should say:
CSS for the Locale module for right-to-left languages.
(at least I think that is more consistent with what we've been doing for these)

Similarly:

--- a/core/modules/locale/locale.css
+++ b/core/modules/locale/locale.css
@@ -1,3 +1,7 @@
+/**
+ * @file
+ * CSS for locale.module.
+ */

Should refer to "the Locale module" rather than "locale.module". Check the @file docs in other files too please. :)

b) locale.bulk.inc

@@ -180,7 +191,10 @@ function locale_translate_export_po_form_submit($form, &$form_state) {
 }
 
 /**
- * Set a batch for newly added language.
+ * Sets a batch for newly added language.
+ *
+ * @param $langcode
+ *  The language code for the newly added language.
  */
 function locale_translate_add_language_set_batch($langcode) {

First line I think could use another 'a' and a hyphen: ... for a newly-added language.

c) locale.module near the top:

 // ---------------------------------------------------------------------------------

We can get rid of lines like this.

d) locale.module

@@ -1047,7 +1054,9 @@ function locale_form_language_admin_add_form_alter(&$form, &$form_state) {
 }
 
 /**
- * Set a batch for newly added language.
+ * Form submission handler for language_admin_add_form().
+ *
+ * Sets a batch for newly added language.
  */
 function locale_form_language_admin_add_form_alter_submit($form, $form_state) {

see item (b) above.

e)

@@ -1076,14 +1085,17 @@ function locale_form_language_admin_edit_form_alter(&$form, &$form_state) {
 }
 
 /**
- * Submission handler to record our custom setting.
+ * Form submission handler for locale_languages_edit_form().
  */
 function locale_form_language_admin_edit_form_alter_submit($form, $form_state) {

I don't think tihs is actually for locale_languages_edit_form()?

f) (just below)

 /**
- * Utility function to tell if locale translates to English.
+ * Checks if locale translates to English.

Needs "the", and technically if -> whether

g) locale.pages.inc now...

@@ -19,7 +21,10 @@ function locale_translate_seek_screen() {
 }
 
 /**
- * Perform a string search and display results in a table
+ * Performs a string search and displays the results in a table

First line needs a . at end.

h)

@@ -128,7 +141,10 @@ function _locale_translate_language_list($translation, $limit_language) {
 }
 
 /**
- * Build array out of search criteria specified in request variables
+ * Builds an array from the search criteria specified in the request variables.
+ *
+ * @return
+ *   An array with values for the keys "string", "language", and "translation".
  */
 function _locale_translate_seek_query() {

The return description doesn't really tell me what is returned.

i) Top docblock in locale.test has a typo at the end:

+ * - A functional test fot language types/negotiation info.
  */

fot -> for

j)

@@ -1058,7 +1060,9 @@ class LocaleImportFunctionalTest extends DrupalWebTestCase {
   }
 
   /**
-   * Test empty msgstr at end of .po file see #611786.
+   * Tests empty msgstr at end of .po file.
+   *
+   * See #611786.

That last line should probably be an @see, and it should give the issue URL (issue numbers do not automatically turn into links in documentation).

k)

@@ -1239,7 +1258,10 @@ msgstr ""
 EOF;
   }
   /**
-   * Helper function that returns a .po file with an empty last item.
+   * Returns a .po file with an empty last item.
+   *
+   * @return
+   *   A .po file.
    */
   function getPoFileWithMsgstr() {

I do not think this description is accurate, given the name of the function and that it's an exact copy of the previous method getPoFileWithEmptyMsgstr().

l)

@@ -1287,7 +1309,7 @@ class LocaleExportFunctionalTest extends DrupalWebTestCase {
   }
 
   /**
-   * Test exportation of translations.
+   * Tests exportation of translations.
    */
   function testExportTranslation() {

exportation -> export
(same should be done in the next docblock too)

m)

@@ -2269,30 +2300,21 @@ class LocaleContentFunctionalTest extends DrupalWebTestCase {
...
+ * - URL (PATH) > DEFAULT: UI Language based on URL prefix, browser language
+ *   preference has no influence.
+ *   - admin/config: UI in site default language.
+ *   - zh-hans/admin/config: UI in Chinese.
+ *   - blah-blah/admin/config: 404.

The line before a list or sub-list should end in a :. And I think the comma in the first line there should be a :

- Remaining tasks: h, k, m
- Needs to be improved: e

Patch will also need an update and have hunks moved to #1392962: Clean up API docs for the language module once #1546752: Move negotiation tests to language module is in.

I planning to working on this issue. So assigning it to myself. Not sure if its the correct process.

The attached patch includes the updates from comment #13 above, plus some more relatively small changes.

ykhadilkar,

Do you still plan on working on this? I know you unassigned it from yourself but you seemed a little unsure about the process so I wanted to confirm.

If I'm wrong on any of the items listed please let me know as I'm still learning also.

Here are a few things that I found that still need some work in this module
The following functions need to be updated according to the form standard
locale_translate_import_form()
locale_translate_import_form_submit()
locale_translate_export_form()
locale_translate_export_form_submit()

The @file block needs to be updated for locale.module
Additionally there are a lot of other items in locale.module need to be updated.

The @file in locale.install should be
* Install, update, and uninstall functions for the Locale module.

1. +++ b/core/modules/locale/locale.bulk.incundefined
   @@ -211,7 +211,11 @@ function locale_translate_export_form_submit($form, &$form_state) {
   + *  The language code for the newly added language.
   + *
   

There is an extra blank line here.

2. +++ b/core/modules/locale/locale.datepicker.jsundefined
   @@ -1,3 +1,8 @@
   + * Datepicker JavaScript for locale.module.
   

I would change this to "for the Locale module."

3. +++ b/core/modules/locale/locale.testundefined
   @@ -5,24 +5,23 @@
   + * - A functional test for the language configuration forms.
   + * - Functional tests for the translation functionalities, including searching.
   + * - A functional test for the PO file import feature, including validation.
   + * - Functional tests for the translation and template export feature.
   + * - Functional tests for the uninstall process.
   + * - A functional test for the language switching feature.
   + * - A functional test for a user's ability to change their default language.
   + * - A functional test for configuring a different path alias per language.
   + * - A functional test for multilingual support by content type and on nodes.
   + * - A functional test for multilingual fields.
   + * - A functional test for comment language.
   

If we are changing these would it be possible to put the similar descriptions next to each other to improve readability.

4. +++ b/core/modules/locale/locale.testundefined
   @@ -38,7 +37,7 @@ class LocaleConfigurationTest extends DrupalWebTestCase {
   -   * Functional tests for adding, editing and deleting languages.
   +   * Tests adding, editing, and deleting languages.
   

I would keep this as Tests for

5. +++ b/core/modules/locale/locale.testundefined
   @@ -898,7 +898,7 @@ EOF;
   + * Tests import of translation files..
   

I would say Tests the and remove the extra period.

6. +++ b/core/modules/locale/locale.testundefined
   @@ -1110,7 +1110,9 @@ class LocaleImportFunctionalTest extends DrupalWebTestCase {
   +   * @See http://drupal.org/node/611786.
   

The @See should be @see and this should not end in a period

7. +++ b/core/modules/locale/locale.testundefined
   @@ -1375,19 +1395,19 @@ EOF;
   -      'description' => 'Tests the exportation of locale files.',
   +      'description' => 'Tests the export of locale files.',
   

This should not be changed in the doc cleanup

8. +++ b/core/modules/locale/locale.testundefined
   @@ -2245,7 +2268,7 @@ class LocaleContentFunctionalTest extends DrupalWebTestCase {
   +   * Tests if a content type can be set to multilingual and language is present..
   

This should not have two periods

NROTC_Webmaster thanks for reviewing. The attached patch includes the updates from comment #21 above.

Updated this patch so that it applies cleanly. There have been several things removed since this patch, so I have removed those hunks. Here's a summary-

No longer exists:
core/modules/locale/locale-rtl.css
core/modules/locale/locale.css

Moved:
core/modules/locale/locale.admin.inc -> core/modules/system/system.admin.inc

Looks pretty good, thanks!

A few things still need updating in this patch:

a)

/**
  * User interface for the translation import screen.
+ *
+ * @see locale_translate_import_form_submit()
+ * @ingroup forms
  */
 function locale_translate_import_form($form, &$form_state) {

First line should follow http://drupal.org/node/1354#forms
Same for locale_translate_export_form() later on in the patch.

b)

+++ b/core/modules/locale/locale.install
@@ -2,7 +2,7 @@
 
 /**
  * @file
- * Install, update and uninstall functions for the locale module.
+ * Install, update and uninstall functions for the Locale module.
  */

Needs , before and

c) @file for locale.module:

+ * When enabled, multiple languages can be set up. The site interface
+ * can be displayed in different languages, as well as nodes can have languages
+ * assigned. The setup of languages and translations is completely web based.
+ * Gettext portable object files are supported.
  */

Can this be wrapped to as near 80 characters as possible, and re-written with better grammar please? I guess the only thing that jumped out at me is the "as well as" -> "and"

d)

-// ---------------------------------------------------------------------------------
 // Hook implementations
-

I agree with removing those two lines... how about the one in the middle too? Other core modules do not have section headers like this.

e)

+ * Checks whether locale translates to the English.
  */
 function locale_translate_english() {

Should be:
Checks whether the locale translates to English.
Also there should be a @return doc?

f)

/**
- * User interface for string editing as one table.
+ * Form constructor for the string editing form.
+ *
+ * @param $lid
+ *   The string ID.

I think this param got added to the wrong function?

g)

+ * Form submission handler for locale_date_format_form().
  */
 function system_date_format_localize_form_submit($form, &$form_state) {

Wrong form function in first line.

h)

+ * @see locale_date_format_reset_form_submit()
+ * @ingroup forms
  */
 function system_date_format_localize_reset_form($form, &$form_state, $langcode) {
   $form['langcode'] = array('#type' => 'value', '#value' => $langcode);
@@ -3356,7 +3369,7 @@ function system_date_format_localize_reset_form($form, &$form_state, $langcode)
 }
 
 /**
- * Reset date formats for a specific language to global defaults.
+ * Form submission handler for locale_date_format_reset_form().
  */
 function system_date_format_localize_reset_form_submit($form, &$form_state) {

Wrong function names in the documentation.

i) As a note, I didn't look through the files to see if other things need to be added to this patch. I just looked through the patch itself. Usually there is a need to clean up docs for the test files in lib/* directories.

Thanks for the review @jhodgdon!

#29: locale_clean_up_documentation-1326618-29.patch queued for re-testing.

Looks like this one needs a reroll again, sorry for the delay in reviewing! I'm trying to get through these...

Re-rolled.

Thanks for all the attention to this issue!!!!

I found one little error in this latest patch: in function locale_translate_english() in locale.module:
+ * Checks whether locale translates to the English.
No "the" :) I fixed this before I committed the patch -- I think it's gone through enough iterations, reviews, and rerolls at this point!

So... I went back and looked through the locale module directory and all sub-directories after this patch was committed. There are still a lot of things in this module that need to be fixed in the next pass through -- so I'm setting this issue back to "active" for Drupal 8.x:

a) locale.pages.inc - has quite a few functions that need first-line attention. I saw at least 4. Note that we have a special standard for theme_* functions at http://drupal.org/node/1354#themeable

b) typo in locale.pages.inc "translatione"

c) locale.module - @file first line is longer than 80 characters. [probably first "the" can be eliminated?]

d) I think we can get rid of the comment in locale.module that says "// Locale core functionality". The previous patch eliminated the // ----------------- but not that comment.

e) function locale() in locale.module needs @return docs.

f) function locale_reset() in locale.module - first line verb tense is wrong. Also locale_library_info_alter(), locale_string_is_safe(), and perhaps other functions [please check through the file].

g) In locale.module I saw at least one place where a blank line was missing between @param / @return sections.

h) _locale_parse_js_file() in locale.module = does not have one-line description.

i) locale.compare.inc - at least 5 verb tense in first line problems, please check through this file.

j) locale_translation_get_projects() in locale.compare.inc - @see should be at end of doc block.

k) locale.bulk.js, locale.admin.js, locale.admin.css - no @file

l) locale.bulk.inc - has verb tense first line problems

m) locale_translate_batch_import() in locale.bulk.inc - should not have blank lines between @param docs.

n) locale.api.php has some line wrapping problems. Note that @code segments can go over 80 characters though!

o) It doesn't look like the tests and other classes in sub-directory lib have been cleaned up -- many need attention.

Well, I just did the review. :)

The documentation for test classes should eventually be cleaned up on this issue, not on a separate issue... but that's OK, they can wait for a separate patch. No need to file a separate issue though.

Regarding this patch, it looks mostly good, thanks!

A few things before it's ready to commit:

a) Not sure why an extra blank line was added at the top of locale.bulk.inc? We just need one.

b) Also, please don't change *any* code lines, such as:

use Drupal\Component\Gettext\PoStreamWriter;
+use Drupal\Core\Language\Language;
 use Drupal\locale\Gettext;
 use Drupal\locale\PoDatabaseReader;
-use Drupal\Core\Language\Language;

c) I can't commit patches with ??? in them:

+ *
+ * @param bool $success
+ *   ???
+ * @param array $results
+ *   ???

If you need help figuring out what functions' param/return values are, go ahead and make your patch, then mark it "needs work" instead of "needs review" to indicate it isn't ready for review/commit, and ask your questions (or at least say "I'm working on figuring this out, patch isn't ready yet").

d) The list syntax cleanups are great to do, thanks! One thing I noticed is that some cleaned-up list items still don't end in "." , such as:

+ *   - core: Core release version, e.g. 8.x
+ *   - version: Project release version, e.g. 8.x-1.0

e) Update functions in locale.install -- those actually should not have verb updates. See
http://drupal.org/node/1354#hookimpl

This is looking better, thanks! There are still a couple of items to fix:

a) The change in locale.api.php in this patch is not correct -- see
http://drupal.org/node/1354#hooks

b)

+ * @param string $string
+ *   (optional) A string to look up translation for. If omitted, all the cached
+ *   strings will be returned in all languages already used on the page.
+ *   Defaults to NULL.
+ * @param string $context
+ *   (optional) The context of this string. Defaults to NULL.
+ * @param string $langcode
+ *   (optional) Language code to use for the lookup. Defaults to NULL.
+ *
+ * @return string
+ *   Either the translated string or the original string
  */
 function locale($string = NULL, $context = NULL, $langcode = NULL) {

- The @return needs to end in .
- Those string typed params/returns... If they default to NULL, then the type can't really be 'string'... I guess it should be string|null?

c)

+ * @return bool
+ *   TRUE indicates the desired operation was successful; othrewise, FALSE.
  */
 function _locale_rebuild_js($langcode = NULL) {

othrewise is misspelled

d)

+ * @return array
+ *   An array of translation filters thta are avilable.
  */
 function locale_translate_filters() {

thta and avilable are misspelled

e)

- * @see locale_translate_edit_form_validate()
  * @see locale_translate_edit_form_submit()
+ * @see locale_translate_edit_form_validate()

I think the previous order is more logical (and in line with our coding standards).

f) You might read through all of the first-line summaries you added and see if they need a/an/the added or have other grammar issues. For instance:
- Returns HTML for translation edit form. ==> for *the* translation...
- Gets an array of available interface translation file. ==> translation file*s*
- Gets array of projects which are available for interface translation. ==> Gets *an* array...

And I also noticed another one that's in the patch file but not modified:

/**
  * Finished callback of system page locale import batch.
...
 function locale_translate_batch_finished($success, $results) {

Thanks!

Large patch... I looked through the first part (see below), and found a few issues:

a)

+++ b/core/modules/locale/lib/Drupal/locale/LocaleConfigSubscriber.php
@@ -19,7 +19,7 @@
  */
 class LocaleConfigSubscriber implements EventSubscriberInterface {
... 
   /**
-   * Get configuration name for this language.
+   * Gets configuration name for this language.
    *
    * It will be the same name with a prefix depending on language code:
-   * locale.config.LANGCODE.NAME
+   * - locale.config.LANGCODE.NAME
+   *
+   * @param string $name
+   *   The configuration name of a given language.
+   * @param object $langcode
+   *   A language code object.
+   *
+   * @return string
+   *   The configuration name of a language as a string.
    */
   public function getLocaleConfigName($name, $language) {

- First line: Gets *the* config...
- I don't understand why you made that locale.config.... into a list item? It's not a list of options.
- The documentation for the two parameters doesn't make sense to me. It's partly because, for instance "a given language" -- what given language? And the second parameter -- is it $language (language object) or $langcode (language code)?

b) This documentation tells me nothing:

+++ b/core/modules/locale/lib/Drupal/locale/LocaleLookup.php
@@ -18,25 +18,34 @@ class LocaleLookup extends CacheArray {
 
   /**
    * A language code.
+   *
    * @var string
    */
   protected $langcode;

"A" language code? What language code?

c) Same file

/**
-   * The locale storage
+   * The locale storage location.
    *
    * @var Drupal\locale\StringStorageInterface
    */
   protected $stringStorage;

I don't think this description is accurate, given what this interface actually is. Same with (later in same file):

+   * @param string $langcode
+   *   A language code.
+   * @param string $context
+   *   The msgctxt context.
+   * @param \Drupal\locale\StringStorageInterface $stringStorage
+   *   The locale storage location.
    */
   public function __construct($langcode, $context, $stringStorage) {

Also please be consistent in a/the (it should be "the" probably) within one set of new @param docs -- it's very jarring to switch. It's also helpful if you're going to add documentation with "the" in it to say what it's used for, such as "The language code for ...".

d)

+++ b/core/modules/locale/lib/Drupal/locale/PoDatabaseReader.php
@@ -22,10 +22,10 @@ class PoDatabaseReader implements PoReaderInterface {
    * An associative array indicating which type of strings should be read.
    *
    * Elements of the array:
-   *  - not_customized: boolean indicating if not customized strings should be
+   *  - not_customized: A Boolean indicating if not customized strings should be

Fix list indentation here.

e) Same file

/**
    * Get the options used by the reader.
+   *
+   * @return array
+   *   An associative array indicating which type of strings should be read.
    */
   function getOptions() {

Get -> Gets... and there are other methods that need verb tense attention in this class.

f) PoDatabaseWriter

-   * - customized: the strings being imported should be saved as customized.
+   * - customized: The strings being imported should be saved as customized.
    *     One of LOCALE_CUSTOMIZED or LOCALE_NOT_CUSTOMIZED.

Last line needs less indentation.

g) same file

+   * The keys for the array are:
+   *  - additions: The number of source strings newly added.
+   *  - updates: The number of translations updated.
+   *  - deletes: The number of translations deleted.
+   *  - skips: The number of strings skipped due to disallowed HTML.
    *
    * @var array
    */
   private $_report;

Fix list indentation.

h) This file (PoDatabaseWriter) needs some verb tense attention too.

i)

+++ b/core/modules/locale/lib/Drupal/locale/StringBase.php
@@ -60,7 +60,7 @@
    * Constructs a new locale string object.
    *
    * @param object|array $values
-   *   Object or array with initial values.
+   *   An object or array with initial values.
    */
   public function __construct($values = array()) {
     $this->setValues((array)$values);
diff --git a/core/modules/locale/lib/Drupal/locale/StringDatabaseStorage.php b/core/modules/locale/lib/Drupal/locale/StringDatabaseStorage.php
index 38e9df5..b953989 100644
--- a/core/modules/locale/lib/Drupal/locale/StringDatabaseStorage.php
+++ b/core/modules/locale/lib/Drupal/locale/StringDatabaseStorage.php
@@ -39,6 +39,7 @@ class StringDatabaseStorage implements StringStorageInterface {
    *   A Database connection to use for reading and writing configuration data.
    * @param array $options
    *   (optional) Any additional database connection options to use in queries.
+   *   Defaults to an empty array.
    */
   public function __construct(Connection $connection, array $options = array()) {

I really don't know why you bother with these types of changes. They don't add a lot to the documentation since anyone can look and see what the default values are. I would only document what the default values are if you are going to add further explanation, like "Defaults to ..., which ...". It just makes these patches bigger and harder to review, and we don't have a standard saying that we should list out every default value.

And in the first example here, adding "An" doesn't really add a lot of value either.

j) StringDatabaseStorage

+   *
+   * @param ??? $query
+   *   ???
+   * @param array $args
+   *   ???
+   *
+   * @return ???
+   *   ???
    */
   protected function dbExecute($query, array $args = array()) {

Uh oh, I can't commit a patch with ??? in it. I stopped reviewing here, as this patch is obviously not done.

Re-roll of #46 to apply to head. No new updates.

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!