Add missing @param in includes A-C, plus other corrections to some docblocks

Setting active since #1315886: Clean up API docs for includes directory, files starting with A-C is closed.

+++ b/core/includes/common.incundefined
@@ -3316,7 +3352,7 @@ function drupal_aggregate_css(&$css_groups) {
+ * @param array $elements
  *   A render array containing:
  *   - '#items': The CSS items as returned by drupal_add_css() and altered by
  *     drupal_get_css().

Did you leave the single quotes in intentionally? I'm just not sure how many incidental things you were trying to catch in this one.

Reviewing

Reviewed for steps 1-9, and I think everything checks out.
Step 10 references I think hundreds of calls, so that is still pending.
Applied the patch to drupal-8.x-dev, and verified that all params are documented.

Apart from the unverified callers and string references, looks good.

I'm checking step 10.

+++ b/core/includes/bootstrap.incundefined
@@ -1923,6 +1941,12 @@ function drupal_array_merge_deep() {
+ * @param ...
+ *   Arrays to merge.

This should be @param array $arrays

+++ b/core/includes/common.incundefined
@@ -2055,6 +2087,10 @@ function date_iso8601($date) {
  * Translates a formatted date string.
  *
  * Callback for preg_replace_callback() within format_date().
+ *
+ * @param string|null $new_langcode
+ *   (optional) The language code to used for translation. Cached statically.
+ *   This parameter is ignored if $matches is passed.
  */
 function _format_date_callback(array $matches = NULL, $new_langcode = NULL) {
   // We cache translations to avoid redundant and rather costly calls to t().

Missing @param array $matches, or maybe array|null? The data type in the function signature directly conflicts with the default, that's weird.

+++ b/core/includes/common.incundefined
@@ -3582,6 +3618,14 @@ function drupal_build_css_cache($css) {
 /**
  * Prefixes all paths within a CSS file for drupal_build_css_cache().
+ *
+ * Callback for preg_replace_callback() within:
+ * - drupal_build_css_cache()
+ * - color_scheme_form_submit()
+ *
+ * @param string|null $base
+ *   (optional) The base path for the CSS files. Cached statically. This
+ *   parameter is not used if $matches is passed.
  */
 function _drupal_build_css_path($matches, $base = NULL) {
   $_base = &drupal_static(__FUNCTION__);
@@ -3716,8 +3760,10 @@ function drupal_load_stylesheet_content($contents, $optimize = FALSE) {

@@ -3716,8 +3760,10 @@ function drupal_load_stylesheet_content($contents, $optimize = FALSE) {
 /**
  * Loads stylesheets recursively and returns contents with corrected paths.
  *
- * This function is used for recursive loading of stylesheets and
- * returns the stylesheet content with all url() paths corrected.
+ * Callback for preg_replace_callback() in drupal_load_stylesheet_content().
+ *
+ * This function is used for recursive loading of stylesheets and returns the
+ * stylesheet content with all url() paths corrected.
  */
 function _drupal_load_stylesheet($matches) {

Also missing $matches

+++ b/core/includes/common.incundefined
@@ -6465,6 +6523,9 @@ function element_property($key) {
  * Gets properties of a structured array element (keys beginning with '#').
+ *
+ * @param array $element
+ *   The structured array to filter.
  */
 function element_properties($element) {
   return array_filter(array_keys((array) $element), 'element_property');

Weird that this casts it to an array.

The rest looks fine.

That's true of drupal_array_merge_deep(), but this docblock is for drupal_array_merge_deep_array(), which actually does take an array of arrays.

It just looks like its for the other one because of dreditor's context.

About the callback stuff, that's fine. As long as it makes it into 1354.

Attached is an updated patch for this issue.

This patch includes what I understood from all of the intervening comments. In addition, only for those docblocks in which @xjm added a type hinting @param, I added type hinting to all other @param and @return directives in that docblock (as I thought appropriate).

My reasoning was that if reviewers and committers had to check one type hint, let's do that whole docblock in one go. It will ease the burden of getting type hinting committed. As a result, there were several missing @return directives that I added just to those docblocks @xjm had touched.

The instructions from #6 still apply in reviewing this patch.

Edit: Interdiff was same as patch. Not sure what to I did there.

Helps if I change the status!

This patch is too substantial for me to want to review/commit all in one go with no other oversight. Can someone else please give it a thorough review for content/correctness? Thanks!

It would be great if we could get someone (other than @jhodgdon) to review the patch in #15. I was reviewing bootstrap.inc in another issue and noticed a couple of missing @param directives that this patch adds. It would help both bootstrap.inc and common.inc to get this committed soon. Thanks.

An issue which overlaps with this one came up in the core queue: #1816008: drupal_cron_run() @return parameter documentation incorrect, should specify returns TRUE or FALSE

I have posted a patch there, if it is RTBC I guess we should probably incorporate it here.

This patch looks really great and clean! Very small things I noticed:

+++ b/core/includes/bootstrap.incundefined
@@ -953,6 +956,15 @@ function drupal_get_filename($type, $name, $filename = NULL) {
+ *
+ * @return array
+ *   An associative array of variable keys and  values.

Looks like there are two spaces before "values."

+++ b/core/includes/bootstrap.incundefined
@@ -1221,6 +1233,9 @@ function drupal_get_http_header($name = NULL) {
+ * @param string|null $name
+ *   (optional) An HTTP header name. Defaults to NULL.

The string|null notation confused me so I checked node/1354 - great to see this has been adopted for 8!

+++ b/core/includes/common.incundefined
@@ -6686,7 +6782,14 @@ function watchdog_severity_levels() {
+ * @return array
+ *   An emty array or an array of strings.

Typo on "emty".

Attached are the results of a self-review after letting this sit for several weeks. A patch is forthcoming that will incorporate this and all other comments back to #15.

+++ b/core/includes/bootstrap.incundefined
@@ -459,16 +459,16 @@ function conf_path($require_settings = TRUE, $reset = FALSE) {
+ * @param string $http_host
  *   The hostname and optional port number, e.g. "www.example.com" or
  *   "www.example.com:8080".

Should be 'e.g.,'

+++ b/core/includes/bootstrap.incundefined
@@ -459,16 +459,16 @@ function conf_path($require_settings = TRUE, $reset = FALSE) {
+ * @param bool $require_settings
+ *   (optional) Boolean indicating whether to recognize only configuration
+ *   directories with an existing settings.php file. Defaults to TRUE.

Should be 'A Boolean'

+++ b/core/includes/common.incundefined
@@ -2017,6 +2055,17 @@ function date_iso8601($date) {
+ * @param string|null $new_langcode
+ *   (optional) The language code to used for translation. Cached statically.
+ *   This parameter is ignored if $matches is passed.

Need to add 'Defaults to NULL.'

+++ b/core/includes/common.incundefined
@@ -3253,6 +3302,21 @@ function drupal_build_css_cache($css) {
+ * @param string|null $base
+ *   (optional) The base path for the CSS files. Cached statically. This
+ *   parameter is not used if $matches is passed.

Needs to add 'Defaults to NULL.'

+++ b/core/includes/common.incundefined
@@ -4665,6 +4731,7 @@ function drupal_get_library($module, $name = NULL) {
  * @param $limit
  *   (optional) Limit the maximum amount of parenting in this table.

Needs to specify what the default value is.

The attached patch is untested locally. An interdiff with the patch from #15 is also attached.

This patch includes all of the noted issues through #21. It also includes the fix from #1816008: drupal_cron_run() @return parameter documentation incorrect, should specify returns TRUE or FALSE.

While re-rolling the patch, I noticed a few docblocks were missing the '(optional)' explanation start string. Hence, I included that here too. Also, in correcting the first point from #21, I went through both bootstrap.inc and common.inc and corrected all other incorrect uses of 'e.g.' followed by a space. The correct use is 'e.g.,' as I understand.

Once this issue gets committed, I would recommend that we do a thorough review of both bootstrap.inc and common.inc probably as separate follow-up issues (since both are so big and have lots of legacy functions). There still is stuff to do in both to bring them up to D8 documentation standards.

.

Argh, it looks like this should be re-rolled again.

+++ b/core/includes/bootstrap.inc
@@ -411,13 +411,13 @@ function timer_stop($name) {
- *   Only configuration directories with an existing settings.php file
- *   will be recognized. Defaults to TRUE. During initial installation,
- *   this is set to FALSE so that Drupal can detect a matching directory,
- *   then create a new settings.php file in it.
+ *   (optional) Only configuration directories with an existing settings.php
+ *   file will be recognized. Defaults to TRUE. During initial installation,
+ *   this is set to FALSE so that Drupal can detect a matching directory, then
+ *   create a new settings.php file in it.

It's hard to keep this from needing rerolls when the issue is about adding missing @param, but instead the patch touches almost every docblock.

Most of the "cleanup" issues have been marked "won't fix" at this point, as they're so hard to get right, review, and commit, for little benefit.

I am inclined to do the same with this one.

If someone is passionate about adding just the missing @param statements and wants to roll a *small* patch that just does that, maybe we could get it in (go ahead and reopen if you want), but for now I'd say let's not bother.

I am working on this, but I need to post a rant before I post yet another patch.
(after 2x of posting the wrong patch in the wrong issue, see [#8543447-21] and [#8543307-1])
@jhodgdon: I am happy to do this on IRC. Just don't see you online atm.

Producing the code changes mentioned above was really easy. But then jumping from one issue to the next to find the correct place, and then having to disect the patch to only commit those parts that fit into a specific issue is demotivating and time-consuming.
I suspect this is part of the reason why this is still not fixed after years, and that we are losing a lot of potentially valuable "casual" contributions from people who are angry at the code style in a specific file, want to fix it and move on.

This being said, i agree with tim.plunkett in #24 that we should focus on stuff that is straightforward and easy to review. Like just adding @param or @return. But slowing it down to match the exact scope of an issue that has been idle for years sounds like quite unproductive to me.

We don't need to close an issue once a piecemeal patch has landed. We can just leave it open, and the next person has less work to do.

(this is tim.plunkett's quote)

It's hard to keep this from needing rerolls when the issue is about adding missing @param, but instead the patch touches almost every docblock.

This was the rant, next comes a patch. git add -p is awesome.

Important questions, before I post the patch:
- Does @see go after the @param and @return, or before it?
- Does @throws go before @return or after? phpstorm puts it before.

https://drupal.org/coding-standards/docs#throws does not really address these questions of order for @see and @throws. And looking at existing code does not guarantee the right answers.

And another question:
Is this just about missing @param, or do we also add missing type declarations on existing @param tags?
And for those @param tags we add, do we add docblock text? Or just the empty tag?
Imo, it would be easier to commit the empty tags and then continue from there by leaving the issue open. This will be a lot easier to review in the first step, and means less rerolling.

Also, I am going to leave out anything where I don't know the exact type. This again should help to speed up the review process and reduce the number of rerolls.

I found the answer about the order question :)
https://drupal.org/node/1354#order (don't know how I missed that)

I talked with jhodgdon on IRC.
The conclusion:
This family of issues should remain closed, because it is an approach that failed. This does exactly confirm the concerns I expressed above.
Instead, issues like #2210565: Docblock and code style improvements in core/includes/bootstrap.inc are exactly the way to go. So I am going to re-open that!

OK then, closing this issue back up.