API page: https://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/lang...

> $property: Optional property of the language object to return

The parameter is not properly documented:
- should say '(optional)'
- wording could do to be clearer, eg, 'The name of a property on the language object to return.'

The @return value is missing.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim created an issue. See original summary.

rashid_786’s picture

changes made as per suggestions.

rashid_786’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: doc-error-2622012-2.patch, failed testing.

David_Rothstein’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

Thanks for the issue and patch! It needs some work though:

  1. +++ b/includes/bootstrap.inc
    @@ -2786,10 +2786,13 @@ function language_list($field = 'language') {
    - * Returns the default language used on the site
    + * The name of a property on the language object to return.
    

    This is a function. The first line summary should still start with a verb.

  2. +++ b/includes/bootstrap.inc
    @@ -2786,10 +2786,13 @@ function language_list($field = 'language') {
    + *   (Optional) property of the language object to return
    

    (optional) should be lower-case by standard. Then Property should be upper-case, and the line should end in .

  3. +++ b/includes/bootstrap.inc
    @@ -2786,10 +2786,13 @@ function language_list($field = 'language') {
    + *   default language used on the site.
    

    Default should be capitalized. And we should be specific here: is it returning a language code or a language object? Or really, if you see the $property parameter, I guess it can return all sorts of things, so this line is not really correct or complete.

priya.chat’s picture

Assigned: Unassigned » priya.chat
snehi’s picture

Assigned: priya.chat » snehi

Assigning to myself due to no activity.

snehi’s picture

Status: Needs work » Needs review

Attaching patch as per #6.
@johngdon i think

+++ b/includes/bootstrap.inc
@@ -2786,10 +2786,13 @@ function language_list($field = 'language') {
+ *   default language used on the site.

I think this is correct except default keyword.
Please see this https://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/lang...
I may be wrong.

snehi’s picture

Forgot to attach patch and interdiff.

Status: Needs review » Needs work

The last submitted patch, 10: doc-error-2622012-9.patch, failed testing.

snehi’s picture

Assigned: snehi » Unassigned
joachim’s picture

  1. +++ b/includes/bootstrap.inc
    @@ -2786,10 +2786,13 @@ function language_list($field = 'language') {
    - * Returns the default language used on the site
    + * Returns the name of the property on the language object.
    

    We've lost the fact it's the default we're returning here.

    It's pretty tricky to do a first-line for this function, as it has two fairly different return values and we really want to mention both in the first line.

    I would put this as:

    Returns the default language, as an object, or one of its properties.

    That fits in 80 characters.

  2. +++ b/includes/bootstrap.inc
    @@ -2786,10 +2786,13 @@ function language_list($field = 'language') {
    + *   Default language used on the site.
    

    This needs expanding. Here there's no one-line limit so we can say something like:

    Returns either the language object for the default language used on the site, or the property of that object named in the $property parameter.

Shreya Shetty’s picture

Assigned: Unassigned » Shreya Shetty
Shreya Shetty’s picture

Assigned: Shreya Shetty » Unassigned
Status: Needs work » Needs review
FileSize
968 bytes
walangitan’s picture

There were a couple of extra spaces in #15 that I removed and also used the description in #13 to re-include the default that's being returned.

walangitan’s picture

FileSize
984 bytes

Fixed some more issues with spacing from #16.

The last submitted patch, 16: doc-error-2622012-16.patch, failed testing.

snehi’s picture

@Shreya Shetty and @walangitan
Whenever you upload a new patch please upload interdiff.
It is easy for reviewer to see what has been changed since last patch.

snehi’s picture

Status: Needs review » Needs work
+++ b/includes/bootstrap.inc
@@ -2779,10 +2779,14 @@ function language_list($field = 'language') {
+ *   Returns either the language object for the default language used on the site,

please wrap it to 80 columns please.

joachim’s picture

> + * Returns either the language object for the default language used on the site,

If this is getting rerolled anyway, then remove the 'Returns' from the start of that sentence.

'Returns foo' describes the whole function. The @return line doesn't need to say it.

walangitan’s picture

Rerolled the patch based on comments from 20 & 21.

walangitan’s picture

Status: Needs work » Needs review
joachim’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

jhodgdon’s picture

Agreed, thanks!

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x - thanks!

  • David_Rothstein committed 70aafba on 7.x
    Issue #2622012 by walangitan, snehi, Shreya Shetty, rashid_786, joachim...

Status: Fixed » Closed (fixed)

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