docblocks coding standard: NULL, FALSE, TRUE or null, false, true

[herbdool]

Does the Code Standards need to be updated, or is it the codebase docblocks?

There are number of places in core where we've got @return string|NULL or @return array|FALSE, for example. Yet, the current revision of the code standards state they should be lowercase: null, false, true. Many of the uppercase examples were added as part of: #1937 and #2060 which were cleaning up code standards in core. This happened around the same time that the code standards was added, which was inherited from Drupal 7. Some things were updated in the code standards, but not the notes about null etc in the docblocks.

I'm moving this discussion here from backdrop-ops/phpcs#5.

My quick and dirty search in core (PHP files only):

• |null - 21 results
• |NULL - 52 results
• |false - 9 results
• |FALSE - 33 results
• |true - 0 results
• |TRUE - 1 result

I had figured it made some sense to use capitals since NULL, TRUE, FALSE are not like the other data types, they are the actual values. So perhaps it would be good to match the case with where it appears in the code? Whereas array, bool, int, string are data types. While it's true they are case-insensitive, I figured that, since we've standardized to using all-caps in the code, that in the comments when we are explicitly saying what value is returned that we use the all-caps so as not to confuse it with a data type, and so that it matches the explanation below it. Here's an example from core/includes/ajax.inc:

 * @return string|NULL
 *   The name of the theme to be used, or NULL if the default should be used.

or core/includes/menu.inc:

 * @return array|FALSE
 *   The router item or, if an error occurs in _menu_translate(), FALSE. A
 *   router item is an associative array corresponding to one row in the
 *   menu_router table. The value corresponding to the key 'map' holds the
 *   loaded objects. The value corresponding to the key 'access' is TRUE if the
 *   current user can access this page. The values corresponding to the keys
 *   'title', 'page_arguments', 'access_arguments', and 'theme_arguments' will
 *   be filled in based on the database values and the objects loaded.

[ghost]

My quick and dirty search in core (PHP files only):

Yeah, it's annoying to have inconsistencies in core. I don't mind one way or the other whether we decide to use uppercase or lowercase. I just want to make sure we settle on something so our standards are properly documented and then we can fix places in core that don't conform.

[avpaderno]

Type-hinting in PHP documentation use lowercase characters, in documentation comments or in code. We should use lowercase characters for them too.

[quicksketch]

Type-hinting in PHP documentation use lowercase characters, in documentation comments or in code.

PHP documentation is all over the place. Whether by intention or not, https://www.php.net/manual/en/language.types.boolean.php uses:

• True
• true
• TRUE

All within the same short page. Though it does seem like they have a preference for true in the documentation outside of code samples. Maybe they were just trying to demonstrate that any capitalization is valid.

The Type Declarations indicates lower-case true, but I haven't tested if uppercase is also supported (though I bet it will).

I could see benefits either way. We're already using all uppercase in most places, so I'd be inclined to go that way.

[avpaderno]

Actually, Booleans is not about type hinting, but values to assign to variables, since it shows examples like $foo = True; which is different from function returns_always_true(string $s1, string $s2): true { /* ... */ }.

Type declarations is relevant for how types are declared (and which types are allowed).

[klonos]

I don't have any strong preference, but as I also mentioned in the dev meeting today, having everything be uppercase makes it easier to remember and less for newcomers to remember as an "exception". Consider this guide:

You should use uppercase for NULL/TRUE/FALSE in code, but you should use lowercase null/true/false in docblocks and type-hinting.

vs. this one:

You should use uppercase for NULL/TRUE/FALSE everywhere.

[herbdool]

I was looking at what CiviCRM does (officially it follows the Drupal code standards but not consistently) and found a mention here civicrm/civicrm-core#22515 of the FIG standards PSR-5, and that CiviCRM was changing the uppercase to lower. php-fig describes them as "keywords": "A keyword defines the purpose of this type."

And some of the keywords:

1. bool: the element to which this type applies only has state TRUE or FALSE.
2. int: the element to which this type applies is a whole number or integer.

...

11. null: the element to which this type applies is a NULL value or, in technical terms, does not exist.

...

12. false or true: the element to which this type applies will have the value TRUE or FALSE. No other value will be returned from this element.

I was leaning uppercase (it is easier to explain), but now I'm fine with just leaving the Backdrop standard as-is on this matter and fixing the code instead.

I can see there is some logic to making the type keyword lowercase and then differentiating it from the value that gets assigned to the variable. Plus PSR-5 and Drupal standard uses lowercase so it doesn't hurt to be consistent with that. Not that it's that difficult to read either way. (It is easier to remember to always write all-caps but we'll probably get used to the two different ways).

So this means we'll need to write docblocks like:

@return array|null
   NULL if nothing found, otherwise an array of things.

We'll just have to be able to explain that the first null is not the "value" per se, but the type which happens to have the same name as the value. Or something like that.

[klonos]

I'm fine with that ^^ as well

Not being helpful, I know 😅 ...but I'm basically with what @BWPanda said: make a decision and then either fix our coding standards, or fix the code (or both).

[ghost]

I was also thinking it might make things more confusing if we change some data types to uppercase and leave others as lowercase:

For the PHP built-in types, use the following names:

• array (NOT "Array")
• bool (NOT "boolean" or "Boolean")
• FALSE (NOT "false")
• float
• int (NOT "integer")
• NULL (NOT "null")
• object (NOT "stdClass")
• string
• TRUE (NOT "true")

As it is now, all the data types listed on that page are lowercase 👍
And the mention of typing all constants (true, false, etc.) in uppercase is in a totally separate part of the documentation (so I feel the risk of getting them confused is somewhat mitigated).

So if we don't mind leaving the standards as they are, that's the easiest route (no need to update documentation, and existing code is already being fixed in #6004)...

Shall we close this as 'works as designed' then?

[quicksketch]

Thanks @kiamlaluno, @herbdool, and @BWPanda for the additional context. That really helps push me into the direction of keeping docblock keywords as all lowercase. I'm probably the author of most of our uppercase FALSE, TRUE, and NULL docblocks, since that's what I've done in the past when there didn't seem to be a documented standard for it yet (or maybe I just was unaware).

If @BWPanda is planning on fixing the coding standards as they are currently documented and implemented in phpcs in #6004, I'm on board with simply closing this issue. We'll proceed with following the coding standards as written, and fix existing docblocks that use FALSE, TRUE, and NULL to be all lowercase. Type-hinting if added in the future will also be lower case.

Does that sit well with everyone?

[herbdool]

I think everyone is ok with that so I'll close this issue.