API page: http://api.drupal.org/api/drupal/core%21includes%21common.inc/function/l/8

The docblock for l() needs improvement. It has at least the following issues:

  • It doesn't specify all @param or @return types.
  • It doesn't specify the parameter defaults.
  • It doesn't indicate whether the $text should be translated with t() or not.
  • It doesn't @see closely-related functions.
  • It has the tiniest little grammar mistake.

Patch to follow.

CommentFileSizeAuthor
#3 drupal-l-1779120-3.patch2.05 KBtraviscarden
#3 interdiff-1779120-1-3.txt1.24 KBtraviscarden
#1 drupal-l-1779120-1.patch2.13 KBtraviscarden

Comments

traviscarden’s picture

traviscarden commented
Status: Active » Needs review
StatusFileSize
new drupal-l-1779120-1.patch2.13 KB 
  • +++ b/core/includes/common.inc
@@ -2247,21 +2247,27 @@ function drupal_http_header_attributes(array $attributes = array()) {
- * This function correctly handles aliased paths, and adds an 'active' class
+ * This function correctly handles aliased paths and adds an 'active' class

    Nitpick!: The comma here implies a compound sentence break, but without an explicit subject after it, this isn't one. 

  • +++ b/core/includes/common.inc
@@ -2247,21 +2247,27 @@ function drupal_http_header_attributes(array $attributes = array()) {
- * @param $text
- *   The link text for the anchor tag.
- * @param $path
+ * Example usage:
+ * @code
+ * l(t('Add new content'), 'node/add');
+ * @endcode
+ *
+ * @param string $text
+ *   The translated link text for the anchor tag.
+ * @param string $path

    +++ b/core/includes/common.inc
@@ -2278,8 +2284,10 @@ function drupal_http_header_attributes(array $attributes = array()) {
- * @return
+ * @return string
    • Add types.
    • Indicate that $text needs to be translated, and add an example as a fast, visual reminder.
  • +++ b/core/includes/common.inc
@@ -2247,21 +2247,27 @@ function drupal_http_header_attributes(array $attributes = array()) {
- *   An associative array of additional options, with the following elements:
+ *   An associative array of additional options, with the following elements.
+ *   Defaults to an empty array.

    Indicate the default value of the parameter. 

  • +++ b/core/includes/common.inc
@@ -2278,8 +2284,10 @@ function drupal_http_header_attributes(array $attributes = array()) {
+ * @see url()

    url() is closely-related. It's not only used in l(), but sometimes it's a good alternative to it.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

That example you added, IMO, is entirely unnecessary. We aren't going to add usage examples to every function, and this one particularly doesn't need one since the function is called hundreds of times in core.

Also, this change in the patch:

*   An associative array of additional options, with the following elements:
+ *   An associative array of additional options, with the following elements.
+ *   Defaults to an empty array.
*   - 'attributes': An associative array of HTML attributes to apply to the
...

This needs to be done in a different way. The sentence that was there was preparing for a list that follows. Adding that other sentence there breaks that.

Other that that... I don't have a problem with adding @see url and the param/return types, so that part is fine. Thanks!

traviscarden’s picture

traviscarden commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-1779120-1-3.txt1.24 KB
new drupal-l-1779120-3.patch2.05 KB

Okay. Then how about this?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Much better, thanks! I'll get this one committed shortly.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Thanks again! Committed to 8.x and 7.x.

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