The D8 documentation standards at https://www.drupal.org/node/1354 state that for the built-in PHP data type NULL the string 'null' should be used in docblocks rather than 'NULL' when specifying a parameter or return value. Attached is a patch correcting a number of invalid formats that currently exist within core. Advice would be appreciated for how large patches like this can become before it is considered "disruptive".

This patch also includes a few docblock corrections for lines that were touched by the 'NULL' type changes. These include the correction of indentation and/or addition of '(optional)' description where needed.

CommentFileSizeAuthor
#7 2608444-7-fix-type-null-d8.patch36.83 KBlars toomre
#7 interdiff-260844-7-2.txt12.65 KBlars toomre
#4 interdiff-2608444-2-4.txt1.83 KBr_sharma08
#4 2608444-4.patch36.61 KBr_sharma08
#2 2608444-2-fix-type-null-d8.patch36.18 KBlars toomre

Comments

Lars Toomre created an issue. See original summary.

lars toomre’s picture

lars toomre commented
Status: Active » Needs review
StatusFileSize
new 2608444-2-fix-type-null-d8.patch36.18 KB

The attached patch touches 45 files, most of which are tests, interfaces or base classes, or which appear to not have been touched in the last few weeks. Is this patch too large for release candidate consideration? Feedback would be appreciated.

eojthebrave’s picture

eojthebrave
he/him
Primary language English
Location Minneapolis, MN
commented
Status: Needs review » Needs work 
  1. +++ b/core/lib/Drupal/Core/Menu/MenuParentFormSelector.php
@@ -140,8 +140,9 @@ protected function getParentDepthLimit($id) {
+   * @param \Drupal\Core\Cache\CacheableMetadata|null &$cacheability
+   *   (optional) The object to add cacheability metadata to, if not NULL.
+   *   Defaults to NULL.

    Should the use of 'NULL' here be changed to 'null' as well? It seems to me like it probably should. 

  2. +++ b/core/lib/Drupal/Core/PageCache/ResponsePolicyInterface.php
@@ -32,10 +32,10 @@
-   *   the cache unless static::DENY is returned. Returns NULL if the policy
-   *   policy is not specified for the given response.
+   *   the cache unless static::DENY is returned. Returns NULL if the policy is
+   *   not specified for the given response.

    Ditto. Should we lowercase this one to 'null' as well?

There are a few other examples of this as well. I honestly don't know what the policy is for NULL vs null in a sentence inside a @docblock. It's clear what it should be what indicating a data type for a @param or @return value though. So maybe I'm off and these can be left as is.

Also, FWIW. My guess is that you'll have a better chance getting a patch like this accepted quickly if you stick to changing just one class of problems in the patch. So just the NULL > null conversion without the other nearby but not necessarily related changes. If nothing else it makes the patches easier to quickly review as they get larger and larger because you're just looking for one specific change and not having to understand the context for every single edit. However, someone that actually has permission to commit a patch like this may feel differently and I would defer to them. :)

r_sharma08’s picture

r_sharma08 commented
Status: Needs work » Needs review
StatusFileSize
new 2608444-4.patch36.61 KB
new interdiff-2608444-2-4.txt1.83 KB

@eojthebrave: looks fine to me. Attached the patch for that.

lars toomre’s picture

lars toomre commented

Thanks for the feedback @eojthebrave and @r_shamra08. For several releases now, Drupal description lines have used TRUE, FALSE and NULL to describe the values of parameters and return variables. If you look at the core code base, you will find probably a couple of thousand occurances.

Examining the case in #3.2, the NULL string was already there. That variable description only was being changed to remove the second occurance of the word policy. In short, I think the changes made in #4 are not correct and we should still consider the patch in #2 to be the most current. If I recall correctly, there is an option for creating a patch with more lines displayed before and after the changes. I will try to find and use it when I next reroll a patch for this issue.

Did either of you have any other comments on the content in the patch in #2? Or *winks* did you agree that all of the proposed changes were just perfect?

eojthebrave’s picture

eojthebrave
he/him
Primary language English
Location Minneapolis, MN
commented

I'm personally fine with NULL or null in the description text. And see no reason to change it. The documented coding standard is very clear about using 'null' for defining the @param type, but not about NULL vs null in the text itself. So lets just let it ride.

However, I do still think that your chances of getting the patch in are higher if you stick to just one class of change here and leave things like the addition of "(optional)" in a few places for another patch. Like this issue for example: #2608732: Some fixes for 'optional' parameter in core/modules

lars toomre’s picture

lars toomre commented
Issue tags: +Novice
StatusFileSize
new interdiff-260844-7-2.txt12.65 KB
new 2608444-7-fix-type-null-d8.patch36.83 KB

Thanks for your feedback @eojthebrave. I have re-rolled the patch from #2 excluding most of the corrections that were made to the descriptions for the specific @param or @return directive that were being changed. I agree with you that such a move will result in a patch that is much easier to review and commit.

I have retained a couple of changes for spelling or an extra word in the description text of the changed @param or @return directives. Presumably those changes are easy check from a patch or interdiff file.

The '(optional)' changes here led me in fact to open up #2608732: Some fixes for 'optional' parameter in core/modules in order to catch a cross-section of other missing '(optional)' docblock issues. I will add the appropriate changes from the interdiff here to those issue(s) when I next re-roll a patch there.

@eojthebrave, could you review this patch and provide additional feedback, perhaps even going so far as to RTBC if you think it is appropriate? Thanks for your help.

eojthebrave’s picture

eojthebrave
he/him
Primary language English
Location Minneapolis, MN
commented
Status: Needs review » Reviewed & tested by the community

Looks good to me. Thanks @Lars Toomre for cleaning up the scope of the patch, and for taking the time to update these comments in the first place. Yay for consistency!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: 2608444-7-fix-type-null-d8.patch, failed testing.

lars toomre’s picture

lars toomre commented
Status: Needs work » Reviewed & tested by the community

Looks like that was a random fail in a migration test. Resubmitted and it passes again. Hence, setting back to RTBC as per #8.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: 2608444-7-fix-type-null-d8.patch, failed testing.

lars toomre’s picture

lars toomre commented
Status: Needs work » Reviewed & tested by the community

More random test bot issues. This is strictly a documentation patch. Back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: 2608444-7-fix-type-null-d8.patch, failed testing.

lars toomre’s picture

lars toomre commented
Status: Needs work » Reviewed & tested by the community

This is a doc only patch, testbot! Please stop your random failures. *smiles*

xjm’s picture

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

Thanks @Lars Toomre and @eojthebrave!

I agree the scope of this should only be to change NULL to null in @param, @return, @var etc. and nothing else. (So @eojthebrave's review was correct about this.) There are still just a couple out-of-scope changes here (e.g. where @var is incorrectly used in HEAD instead of @param; that would be better to fix in its own patch).

...Before we do that, though, this may also be duplicating some of #2571965: [meta] Fix PHP coding standards in core, stage 1. We need to check if there's an existing coder rule for this and look for the child issue of that meta that may already address it, and if not, create one. (As of Amsterdam, as explained in #1518116-82: [meta] Make Core pass Coder Review, we decided that coding standards cleanups need to have a coder rule and testing support before we clean them up, and as of Barcelona we are actually preparing core to support coder rules and automated coding standards checking on DrupalCI.) For an excellent example of how this can be done, see #2606860: Docblock sections out of order in core/lib/Drupal and the related issue #2605986: Missing check for ordering of comment tags. Also, given the nature of this cleanup, we can clean up all of the core instances in one patch rather than just "some". (Edit: Because it's very easy to review with git diff --color-words that way.)

Finally, note that this and similar coding standards fixes might be targeted for the RC phase of 8.1.x in a few months, similar to what was done recently for 8.0.x. As mentioned above, the fix for @var where it should be @param can be in its own separate issue that goes into 8.0.x now, since that is an actual bug rather than standards compliance. Thanks!

lars toomre’s picture

lars toomre commented

Thanks @xjm for your more expansive comments in #15. You are free to repurpose this issue as you wish. You can focus it so that it captures all incorrect uses of NULL in the variable directives within core's docblocks.

That was not my intent when I created this issue and submitted an initial patch. I simply was reading some of the new migration system code and noticed some violations of Drupal's documentation standards. Hence, I created a patch that corrected what I had identified in my code review. I then also included in the patch similar fixes that were identified by a simple grep.

This patch never was intended to be a comprehensive correction of one type of documentation standard violation. I long had understood that incremental (e.g. some) fixes would be appreciated by others in the Drupal community. If that is not acceptable and needs to be a comprehensive fix, I will refrain from creating such patches in the future. Thanks for the feedback.

lars toomre’s picture

lars toomre commented
Related issues: +#2620826: Fix @var directive misuse in interface method docblock

Created #2620826: Fix @var directive misuse in interface method docblock with a patch to address incorrect use of @var directive that was included in patch #7 above.

lars toomre’s picture

lars toomre commented
Related issues: +#2620856: Extend FunctionCommentSniff for incorrect casing of data types such as 'NULL'

Created #2620856: Extend FunctionCommentSniff for incorrect casing of data types such as 'NULL' to address missing code sniff that hopefully will identify this type of documentation standard violation on a comprehensive basis.

lars toomre’s picture

lars toomre commented

There now is a patch in #2620856: Extend FunctionCommentSniff for incorrect casing of data types such as 'NULL' that when applied should catch all instances of this violation of Drupal's documentation standards. As an added bonus, it also catches instances where TRUE and/or FALSE also are incorrectly used in a @var, @param and/or @return directives. The patch there needs test(s) so any help in completing that portion of this exercise would be appreciated.

@xjm, thanks for committing the patch in #2620856: Extend FunctionCommentSniff for incorrect casing of data types such as 'NULL'. As a result, the patch in #7 of this issue is no longer valid. How would you like me to proceed from here? Would you like me to re-roll the current patch that captures some instances of the incorrect use of NULL? Or would you like to wait until the sniff exists and a comprehensive patch can be machine generated? Thanks in advance for your advice.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Issue tags: +DrupalBCDays

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs work » Closed (duplicate)

Fixed in these issues: #2783393: Fix 'Drupal.Commenting.FunctionComment.IncorrectParamVarName' coding standard #2783395: Fix 'Drupal.Commenting.FunctionComment.InvalidReturn' coding standard