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.

CommentFileSizeAuthor
#22 interdiff-2622012-17-22.txt501 byteswalangitan
#22 doc-error-2622012-22.patch977 byteswalangitan
#17 doc-error-2622012-17.patch984 byteswalangitan
#16 doc-error-2622012-16.patch966 byteswalangitan
#15 doc-error-2622012-14.patch968 bytesshreya shetty
#10 doc-error-2622012-9.patch854 bytessnehi
#10 interdiff-2622012-2-9.txt908 bytessnehi
#2 doc-error-2622012-2.patch853 bytesrashid_786

Comments

joachim created an issue. See original summary.

rashid_786’s picture

rashid_786 commented
StatusFileSize
new doc-error-2622012-2.patch853 bytes

changes made as per suggestions.

rashid_786’s picture

rashid_786 commented
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

David_Rothstein commented
Status: Needs work » Needs review
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
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

priya.chat commented
Assigned: Unassigned » priya.chat
snehi’s picture

snehi commented
Assigned: priya.chat » snehi

Assigning to myself due to no activity.

snehi’s picture

snehi commented
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

snehi commented
StatusFileSize
new interdiff-2622012-2-9.txt908 bytes
new doc-error-2622012-9.patch854 bytes

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

snehi commented
Assigned: snehi » Unassigned
joachim’s picture

joachim commented 
  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

shreya shetty commented
Assigned: Unassigned » shreya shetty
shreya shetty’s picture

shreya shetty commented
Assigned: shreya shetty » Unassigned
Status: Needs work » Needs review
StatusFileSize
new doc-error-2622012-14.patch968 bytes
walangitan’s picture

walangitan commented
StatusFileSize
new doc-error-2622012-16.patch966 bytes

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

walangitan commented
StatusFileSize
new doc-error-2622012-17.patch984 bytes

Fixed some more issues with spacing from #16.

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

snehi’s picture

snehi commented

@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

snehi commented
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

joachim commented

> + * 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

walangitan commented
StatusFileSize
new doc-error-2622012-22.patch977 bytes
new interdiff-2622012-17-22.txt501 bytes

Rerolled the patch based on comments from 20 & 21.

walangitan’s picture

walangitan commented
Status: Needs work » Needs review
joachim’s picture

joachim commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Agreed, thanks!

David_Rothstein’s picture

David_Rothstein commented
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.