Posted by TravisCarden on September 9, 2012 at 1:21am
3 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | documentation |
| Category: | task |
| Priority: | normal |
| Assigned: | TravisCarden |
| Status: | closed (fixed) |
| Issue tags: | Novice |
Issue Summary
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
@paramor@returntypes. - It doesn't specify the parameter defaults.
- It doesn't indicate whether the
$textshould be translated witht()or not. - It doesn't
@seeclosely-related functions. - It has the tiniest little grammar mistake.
Patch to follow.
Comments
#1
+++ 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
$textneeds 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 inl(), but sometimes it's a good alternative to it.#2
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!
#3
Okay. Then how about this?
#4
Much better, thanks! I'll get this one committed shortly.
#5
Thanks again! Committed to 8.x and 7.x.
#6
Automatically closed -- issue fixed for 2 weeks with no activity.