Correct list stuff in docs for includes/database [#1326574]

Problem/Motivation

This issue is a spin-off from #1315886-42: Clean up API docs for includes directory, files starting with A-C.

Currently, Drupal core files inconsistently follow the documentation standards regarding lists. Sometimes the documentation of lists follows the pattern "- key: ". In other places, the pattern "- 'key': " is used. According to the Documentation Standards, the first pattern is the required format.

Proposed resolution

The attached patch attempts to correct this list formatting issue. This patch is particularly large both because of the frequency of the formatting problem and because a number of docblocks had to be adjusted for 80 character limits.

While correcting the list formatting in various docblocks, I have also added type hinting to both the @param and @return directives in those docblocks. The patch also includes a few corrections of other documentation errors encountered while editing the approximately 35 files touched herein.

This patch will have to be re-rolled after the November 1st move of files to the /core directory. This patch also touches a number of files that will be changed as part of #1310084: [meta] API documentation cleanup sprint. I will endeavor to keep this patch current until it is committed.

This patch includes three @todo additions that need to be addressed before this patch is committed. I purposely have left those line counts greater than 80 characters so reviewers will notice them.

Follow-up issues

While rolling this patch, I noticed that documentation lists generally appear in two places: the input $options variable and the returned value when it is an associative array. Drupal's documentation standards are unclear on how the variable type of each key value of such associative arrays should be defined.

For instance, the revised return value documentation for drupal_parse_url() is now:

- * @return
+ * @return array
  *   An associative array containing the keys:
- *   - 'path': The path of the URL. If the given $url is external, this includes
+ *   - path: The path of the URL. If the given $url is external, this includes
  *     the scheme and host.
- *   - 'query': An array of query parameters of $url, if existent.
- *   - 'fragment': The fragment of $url, if existent.
+ *   - query: An array of query parameters of $url, if existent.
+ *   - fragment: The fragment of $url, if existent.

Should we adopt a standard like like:

 * @return array
 *   An associative array containing the following keys:
 *   - path: (string) The ...
 *   - query: (array) An ...
 *   - fragment: (string) The ...

If the '(required)' or '(optional)' key words are present, should the variable type go before or after such keywords?

The current Drupal documentation standards are not clear on regarding literal string like '=' and '!='. Should multiple literals appear on the same list line like " * - =, !=: " or should they be separated into individual list items? If separated, should the same text explanation be repeated for each or perhaps only for the last in the cascade like a switch statement? See the documentation for entityCondition() for reference.

Comment	File	Size	Author
#25	database_list_formats-1326574-25.patch	14.61 KB	lars toomre
#23	database_list_formats-1326574-23.patch	14.63 KB	lars toomre
#11	database_list_formats-1326574-9.patch	14.69 KB	lars toomre
	list.patch	154.47 KB	lars toomre

Comments

Comment #1

lars toomre commented 30 October 2011 at 16:46

Status:

Active

» Needs review

I think the status should be 'needs review' so the test bots will kick off.

Comment #2

xjm

she/her

English

commented 31 October 2011 at 01:21

Hmm, I thought we were not going to add any datatype documentation until we added it all at once, so that core wouldn't be inconsistent on that point? There also seem to be some changes to theme templates that don't seem to be related to the patch.

Also, this patch is going to collide with the docs cleanup sprint like crazy. You mentioned that you're okay with rerolling it, so I hope that isn't too painful. :)

The pattern you suggest for datatype documentation in arrays makes sense to me. We probably would want to have folks test that in their IDEs. Whether (optional) or (type) should come first might also depend on that. Maybe we should reopen #711918: Documentation standard for @param and @return data types with that question, or (better) post a followup and reference it there so that the people who tested the new standard with their IDEs see it?

Regarding the parameter documentation EntityFieldQuery::entityCondition() -- Boy, that is pretty hard to read in its current form. One line per value would seem a lot more legible. Would it be reasonable to separate it into three different lists, each prefaced by the explanations that are there now?

Comment #3

xjm

she/her

English

commented 31 October 2011 at 01:35

+++ b/includes/common.incundefined
@@ -5545,20 +5556,20 @@ function drupal_render_page($page) {
+ *   - granularity: (optional) Define the cache granularity using binary
+ *     combinations of the cache granularity constants, e.g. DRUPAL_CACHE_PER_USER

This also goes over 80 chars. (I remember rewrapping this to fix the indentation and line length, so probably it doesn't matter at this point, because rerolling the hunk will get the new wrapping.)

+++ b/modules/field/field.default.incundefined
@@ -47,29 +47,29 @@ function field_default_extract_form_values($entity_type, $entity, $field, $insta
+ * @param ??? $items                                                              // Need help on this type hinting definition
  *   $entity->{$field['field_name']}[$langcode], or an empty array if unset.

I can help with this one I think. $items is an array of field items, indexed by a $delta (integer). Each item is an associative array of field data.

Comment #4

jhodgdon

she/her

English

commented 31 October 2011 at 14:42

Status:

Needs review

» Needs work

Agreed with #2 - please keep this issue on target for fixing lists, and don't add data types in this patch. It makes the patch a LOT longer and MUCH more difficult to review. Thanks!

Also, a few other things:

- * @param $type
- *   The type of the message. One of the following values are possible:
- *   - 'status'
- *   - 'warning'
- *   - 'error'
+ * @param string $type
+ *   The type of the message. One of the following strings are permissible:
+ *   - status
+ *   - warning
+ *   - error

We omit the ' in the case of array keys, but I don't think we should in the case of literal strings being the param/return value. This applies to several of the hunks in your patch.

- *     - 'key': element name
- *     - 'value': element contents
- *     - 'attributes': associative array of element attributes
+ *     - key: element name
+ *     - value: element contents
+ *     - attributes: associative array of element attributes

If you are going to bring lists up to standards, this one should be:

 *     - key: Element name.
 *     - value: Element contents.
 *     - attributes: Associative array of element attributes.

c) I also noticed "varaible" (spelling error).

Comment #5

lars toomre commented 31 October 2011 at 15:56

Thanks for the feedback both @xjm and @jhodgdon. While working on the documentation in several modules, I have noticed that a bunch of functions already have type hinting. Hence, core is already inconsistent in its application of type hinting. Hence, I figured if I was touching a docblock for a list, I should update all of the @param and @return directives in that docblock rather than just the one containing the list of items.

Also, in many cases, the description line immediately below the directive states what type of variable it is. What is your preference for the @ directive that includes the list? Should it be '@param $foo' or '@param array $foo'? '@param $foo' just seems wrong to me.

I will open up a new issue shortly about documentation standards for the variable types of keys in an associative array.

Another issue related to lists came up last night while I was working on the image module. That module used the pattern \ -"key": \ for its list items instead of the standard \ -key: \. I did not catch this pattern in the above patch. After further reviewing all of the core files, it appears that this pattern is used in a further 17 files in about another 150 places. Clearly these too need to be fixed.

However, such a fix will balloon the size of the above patch yet further. Suggestions on how to proceed? To keep things more manageable maybe we should have one patch for each malformed pattern?

Comment #6

jhodgdon

she/her

English

commented 31 October 2011 at 16:19

Regarding type hinting, it is a new standard, and you are right it hasn't been applied uniformly. I am in favor of adding them in small patches that are just fixing up one function. However, not in mega-patches such as this issue or the generic cleanup issues. The time to review a data type hinting line is relatively large (I would have to go look at the function and verify it is correct), and if there are many of them, it's prohibitive. Here's the issue where this standard was discussed and adopted, and the strategy for updates, if you're interested:
#711918: Documentation standard for @param and @return data types

I think fixing the "key" (double quotes) in this patch would be good though.

Comment #7

lars toomre commented 31 October 2011 at 17:14

Thanks @jhodgdon. I participated in the referenced issue and am a big proponent of type hinting. My habit is to always specify a variable type (a habit formed from my days as a FORTRAN programmer). For this documentation sprint, I will consciously cease to do so henceforth.

Be forewarned, though, that the forthcoming patch for the image module does have type hinting (based on last night's work). I am checking a few documentation standard items for that patch now and really would hate to take out all of the diligent type hinting work. I will add no more type hinting going forward.

As a side note, much of Connecticut is without power and hence internet connections after Saturday's very early snow storm. I am able to work off-line on these documentation patches and will post periodically when visiting the local town hall or Starbuck's (like I am now). Hence, it might seem like there is a burst of activity and then nothing. Officials are guessing it might be a week or more until electricity is fully restored.

Comment #8

jhodgdon

she/her

English

commented 31 October 2011 at 17:20

Please put the type hinting on SEPARATE issues. And please don't discuss it any more on this issue, which is getting sidetracked!

Hope your power gets restored soon!

Comment #9

lars toomre commented 2 November 2011 at 13:57

My git skills are not good. I am having problems with a three way merge I am being told with a modified file bootstrap.inc. Once I figure that out, a couple of patches regarding this issue should be forthcoming.

Comment #10

lars toomre commented 2 November 2011 at 23:25

Title:

Correct malformed list formats in documentation blocks

» Correct list stuff in docs for includes/database

I have messed something up with my edits to bootstrap.inc and common.inc that did not work well with the /core files move. Hence, it is not yet possible to re-roll the large patch from the original issue yet.

However, I am seeing that patch will need to be broken up anyways into smaller pieces any way. Hence, I am refocusing this thread to deal with one of the smaller patches that will be referenced by a new meta issue.

Comment #11

lars toomre commented 2 November 2011 at 23:31

Status:

Needs work

» Needs review

Status	File	Size
new	database_list_formats-1326574-9.patch	14.69 KB

The attached patch addresses 'malformed list stuff' in three files related to core/includes/database.

When I say ' malformed list stuff' I was reviewing core code files for:

Correct indentation of list items,
Format of list items in docblocks,
Use of '(optional)' key word,
Use of '(required)', and
Where else '(optional)' keyword otherwise should be used.

Comments and thoughts are welcome. A revised patch (if needed) for these files will be easy to re-roll.

Comment #12

jhodgdon

she/her

English

commented 2 November 2011 at 23:39

If you need to break the patch up, please file separate issues for the other pieces. Let's keep this issue focused on the cleanup listed in the original issue.

Also, I was specifically asked by webchick for these large cleanup issues to do *all of includes* in one patch. I broke it up into 4 or 5 chunks already. Please don't break it up more.

Comment #13

jhodgdon

she/her

English

commented 3 November 2011 at 00:01

Oh wait. This is the list patch. I thought I was on another issue. Sorry! Disregard #12.

Anyway, it would probably be best not to break it up into too many small issues/patches, but 2 is probably OK if the patch would otherwise be huge.

Comment #14

lars toomre commented 3 November 2011 at 00:14

Sorry about that @jhodgdon... I was trying to match the way that you had split up the co-current docs sprint.

I still need to hand merge my changes to bootstrap.inc and common.inc with those from the /core move. After I finish that in the next day or so, I should be able to roll the big patch in whatever fashion you would prefer.

Noting that I do not trust the change numbers below from bootstrap.inc and common.inc yet, the preliminary list of files that will be changing includes:

My thought was that we might want to break it up around:

The first part of /includes... (including common.inc)
Rest of /includes ...
Big patch for all of the *.api.php changes.
Files for entity and field stuff ...
Files for image module.
Rest of module * changes *

I am open though to how ever you want it broken up given the approximate details above. Please advise.

Comment #15

lars toomre commented 3 November 2011 at 00:24

Just saw #13 after I posted #14.

Let me suggest that we leave this issue focused just on the patch that is all green in #11.

Tomorrow I will open a new meta issue that will then reference this issue as a sub issue. I will add a reference there to the still to be opened issue for how to document variable types in the elements of a return or $options associative array. I would do so now but I am being kicked out of this wifi location and we still have no electricity.

Cheers!!

Comment #16

jhodgdon

she/her

English

commented 3 November 2011 at 14:47

6 sub-issues sounds reasonable to me, go for it!

Comment #17

lars toomre commented 3 November 2011 at 15:01

Could we make that six sub-issues in addition to this one?

This patch is primarily fixing the indentation of a big block of list text and then re-wrapping for 80 characters. It should be relatively easier for all to review as a result.

I also would like a review here to make sure that you are comfortable with what is in #11 since the patterns will be repeated in the other six patches.

Thanks.

Comment #18

jhodgdon

she/her

English

commented 3 November 2011 at 15:18

Status:

Needs review

» Needs work

How about if this is one of the sub-issues, and you make a new meta-issue to manage the sub-issues? If that's what you meant, then yes. :)

So. I took a look at the patch in #11. A few comments:

a) If you want it to be easy to review:
- Adding "(optional)" -- this is easy to review:

-   *   Optional name of the savepoint.
+   *   (optional) Name of the savepoint.

This is not, because I would have to find the function in question and read it to see if your assessment of it being optional is correct:

- *   An array of options to control how the transaction operates:
+ *   (optional) An array of options to control how the transaction operates:

So my preference would be just to correct errors on how (optional) and (required) are given, not to go around researching whether they should be given for parameters and list items.

b) Please don't add data types, as here:

-   * @return
+   * @return object

Again, I have to think to review this, and as we've discussed, this should be a separate effort.

c) In that big long re-indent in core/includes/database/schema.inc, the first line I checked had a problem:

- *   - 'description': A string in non-markup plain text describing this table
- *     and its purpose. References to other tables should be enclosed in
...
+ * - description: A string in non-markup plain text describing this table are
+ *   its purpose. References to other tables should be enclosed in

The word "and" changed to "are". I didn't read through the rest of that long section carefully yet, but ... are you doing this re-wrapping manually? Doesn't your editor do this kind of thing for you?

Comment #19

lars toomre commented 3 November 2011 at 15:53

Thanks @jhodgdon. My intention is to have this issue as one of the sub-issues and to create a new issue as the meta issue for both this issue and the other six forthcoming sub-issues (and patches) about malformed lists in doc blocks.

This patch also includes the first big block that had an indentation problem. At this point, I do not recall whether I started to do all of the reformatting by hand or with an automated function. I will check and reroll the patch shortly.

The changes in the mega patch include some detailed review of what is required and where '(optional') is missing. From memory now, I think that type of change is concentrated in the bootstrap.inc and common.inc files. I will check though.

I added the variable type only if we were changing the list for that @param or @return directive. My thought was that one would want to look at the docblock and function call together anyways. If you do not to look at the function call at all, I will take all of those variable type hints out.

I will try to re-roll a modified patch for this sub-issue later this afternoon. Thanks for the review.

Comment #20

jhodgdon

she/her

English

commented 3 November 2011 at 16:58

OK. I was kind of assuming this issue was for formatting changes only -- I would say mega-patches should only be for that kind of thing. So I would still avoid adding/changing any meaning in the docs:
- data types
- optional/required where it wasn't already specified in a non-standard way
- rewriting docs

Stick to just reformatting. If you find other issues (wrong doc, etc.), file separate issues.

Is that clear enough? My thought is that in general, a mega-patch should not require any real thinking to review. You should just have to verify that the text is the same, but formatted correctly.

Comment #21

lars toomre commented 3 November 2011 at 18:21

OK @jhodgdon. Your directions are clear and I understand my marching orders so to speak... What I have now in the 'mega lists' branch is a mangling of 'easy' and 'hard' issues and will need to be sorted out.

Some of the patches forthcoming will be easy to review bigger patches that require little thinking. Other patches will be smaller units (like just the 'optional' changes to common.inc file) that will require looking at both the docblock and the function itself for confirmation.

I will focus first on rolling the easier to review patches. (We are still dealing with no power here in CT so the forthcoming patches may be a bit delayed.)

Comment #22

lars toomre commented 4 November 2011 at 02:41

This issue is now listed as a sub-issue of the newly created meta issue #1331240: [Meta] Correct documentation for 'list' related issues.

Comment #23

lars toomre commented 4 November 2011 at 03:03

Status:

Needs work

» Needs review

Status	File	Size
new	database_list_formats-1326574-23.patch	14.63 KB

Attached is a re-roll of the patch from #11 including @jhodgdon's comments from #18.

If it passes the test bot, this patch nonetheless will need to be re-rolled once more to address the one question asked. Please focus on how best to document 'mysql_type, pgsql_type, sqlite_type,' ... type of array keys (line 45 of schema.inc). Should they be moved perhaps to a note of their own at the bottom of the specification definition? Should each be on a separate line or should they just left as is?

The text also will need to be carefully reviewed because it includes a few small edits like \e.g \e.g., \, the addition of a couple of commas at the end of phrases, and importantly the separation of 'scale' and 'precision' into their own key definitions.

Comment #24

jhodgdon

she/her

English

commented 4 November 2011 at 15:00

Status:

Needs review

» Needs work

The way you did the mysql_type, etc. lines:

+ *   - mysql_type, pgsql_type, sqlite_type, etc.: If you need to use a record
+ *     type not included in the officially supported list of types above, you
+ *     can specify a type for each database backend. In this case, you can leave

seems fine to me. It's clear anyway. There's no way to move each to a separate line, because of the "etc.", and it's a group that belongs together anyway. What I don't know is why the schema API topic/group is in the pgsql include file, but ... whatever. :)

So go ahead and remove that question line, and make a patch that is ready for review, and I'll be happy to review it.

Comment #25

lars toomre commented 4 November 2011 at 15:41

Status:

Needs work

» Needs review

Status	File	Size
new	database_list_formats-1326574-25.patch	14.61 KB

Attached is a revised patch with the one-line question removed.

I too have no idea why schema API topic/group is in the pgsql file. However, as you have asked me in other places, please just focus on the formatting in this issue. Let's open another issue to correct your off-topic observation or add it a follow up section in the issue summary of this sub-issue.

Thanks for reviewing this patch @jhodgdon. My other comments about review of schema keys from #23 still applies.

Comment #26

jhodgdon

she/her

English

commented 6 December 2011 at 19:53

#25: database_list_formats-1326574-25.patch queued for re-testing.

Comment #27

6 December 2011 at 19:55

Status:

Needs review

» Needs work

The last submitted patch, database_list_formats-1326574-25.patch, failed testing.

Comment #28

jhodgdon

she/her

English

commented 15 July 2015 at 21:23

Status:

Needs work

» Closed (cannot reproduce)

This patch is very obsolete at this point and I think most of it was probably already addressed, closing this as stale.

Correct list stuff in docs for includes/database

Problem/Motivation

Proposed resolution

Follow-up issues

Comments

Comment #1

Comment #2

Comment #3

Comment #4

Comment #5

Comment #6

Comment #7

Comment #8

Comment #9

Comment #10

Comment #11

Comment #12

Comment #13

Comment #14

Comment #15

Comment #16

Comment #17

Comment #18

Comment #19

Comment #20

Comment #21

Comment #22

Comment #23

Comment #24

Comment #25

Comment #26

Comment #27

Comment #28

News items

Our community

Documentation

Drupal code base

Governance of community