Posted by xjm on October 30, 2011 at 1:10pm
6 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | documentation |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | docs-cleanup-2011 |
Issue Summary
Problem/Motivation
From #1315886: Clean up API docs for includes directory, files starting with A-C. There are undocumented parameters in the following files:
- bootstrap.inc
- common.inc
Proposed resolution
Add documentation for all parameters in these files.
Remaining tasks
Followup issues
- #1326458: Add missing @return in includes A-C
- #1326452: Clean up documentation of optional parameters in includes A-C
- #1533094: Make includes directory, files starting with A-C pass Coder Review
- #1333534: Further cleanup for documentation in core/includes files starting with A-G
- Render API callbacks in
common.inc.
Miscellaneous
- _filter_xss_split() has a parameter called
$m. - Malformatted @param/@return blocks.
- The reuse of the
$formatvariable in format_date() is confusing. - The parameter use for _format_date_callback() and _drupal_build_css_path() is confusing. IMO it would be better to have a separate function to store the static cache.
- Many nonstandard function summaries for JS stuff have been reintroduced. :(
Comments
#1
Setting active since #1315886: Clean up API docs for includes directory, files starting with A-C is closed.
#2
Attached adds parameter documentation for all functions in
bootstrap.incandcommon.incthat were missing it. Few notes:drupal_json_encode()is legitimatelymixed; it can accept any PHP datatype.$elementrather than the$elementsthat actually appears in the function signature and is used in the function.)#3
#4
+++ 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.
#5
The scope is only what is indicated in the summary and title. There's a separate issue for the list cleanups (#1333534: Further cleanup for documentation in core/includes files starting with A-G ), and it's very important to me to keep these tasks separate because these particular files are so large and so full of legacy stuff that they patches would be impossible to review otherwise. :)
Thanks for checking though!
#6
To review this patch:
+or-in front of them, not the other lines which are provided for context.)function_name($foo_bar), the parameter should be listed as$foo_barin the docblock, not$foobar.NULLvalue for a string parameter, then it should be documented asstring|null.#7
Reviewing
#8
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.
#9
I'm checking step 10.
#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 maybearray|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.
#11
I don't think this is correct. See drupal_array_merge_deep(). It's not an array of arrays as one parameter; it's a non-fixed number of array parameters passed to the function and retrieved with
func_get_args(). E.g.:<?php
// Could be:
$foo = bar($array1);
// Or:
$foo = bar($array1, $array2);
// Or:
$foo = bar($array1, $array2, $array3, $array4, $array5, $array6, $array7, $array8, $array9, $array10);
?>
I omitted this because it's a preg_replace callback, which takes
$matchesas the first parameter without$matchesbeing passed by the actual calling code. (PHP creates and passes this parameter internally.) See http://us.php.net/preg_replace_callback under the description of the callback parameter, and also see how these are used in (e.g.) drupal_build_css_cache().When we originally set the callback standards, we talked about omitting parameter descriptions that were always the same like we do for hook implementations and Drupal-specific callbacks, although it looks like this didn't make it into 1354 explicitly. Do you think that's unclear here? Maybe it needs to be something like:
The matches found for the regular expression passed to preg_replace_callback() with this callback. See preg_match() for more information.Yeah, that seems... wrong. I checked and core only calls it once, in filter_process_format(), which is only used as the
#processcallback for filter_element_info(). Maybe we should remove the typecast in D8, as I don't see how an object should ever get passed there?#12
Oh, regarding:
Yeah, I thought that was code smell too. I have it on a list of followup issues to file (summary under "miscellaneous").
#13
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.
#14
#15
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.
#16
Helps if I change the status!
#17
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!
#18
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.
#19
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.
#20
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".
#21
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.
#22
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.