All-capitalized NULL, FALSE, TRUE in comments?

[herbdool]

I believe it should be @param array|NULL, for example.

Looks like it's incorrect here?:

phpcs/Backdrop/Sniffs/Commenting/FunctionCommentSniff.php

Line 34 in a588dc2

public static $invalidTypes = [

. If I understand correctly, it's saying it should be null not NULL and false / true not FALSE / TRUE. But I don't think that is correct.

[indigoxela]

@herbdool many thanks for sneaking in.

Can you provide a case, where this rule makes wrong suggestions? (Note that the rule itself is forked from the Drupal ruleset.)

And - are you interested in becoming a co-maintainer? 😏

[herbdool]

Sure, I can co-maintain. I haven't used this much but I'm increasingly exposed to it with my PRs so may as well help a bit.

I saw this issue: backdrop/backdrop-issues#6004 and the first PR backdrop/backdrop#4369 passed the linting with this @param string|null for example. But in Backdrop the standard is @param string|NULL (or at least that's been feedback I've received when Nate's reviewed my PRs).

[kiamlaluno]

Indicating data types in documentation says to use null, false, and true when indicating a variable type in a documentation comment.

[ghost]

@herbdool Normally in Backdrop we use uppercase for these things, except in this context where our documentation standards specifically say to use the lowercase versions (as @kiamlaluno linked to).

Maybe @quicksketch forgot about that...

[herbdool]

Maybe, though there are a lot of examples so I don't think it was an accident. It's equally plausible that this part of the coding standards was overlooked.

I didn't intend to throw a wrench/spanner into getting the code updated, but I think it's worth getting some clarification on which way core committers and others lean on this.

I was basing my response just on looking at the code. And I found examples like https://github.com/backdrop/backdrop/blob/d688729ab0b31a1f0d16d0af33c97807cfad61e4/core/includes/config.inc#L277 and https://github.com/backdrop/backdrop/blob/d688729ab0b31a1f0d16d0af33c97807cfad61e4/core/modules/layout/includes/block.class.inc#L36, which are from the more recent code.

And I figured it made some sense since NULL, TRUE, FALSE are not data types per se, they are the actual values. Where as 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 another 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]

@herbdool Thanks for wanting to bring consistency to this! Nothing worse than having things done differently in different places.

But that's why we have documentation/coding standards. And in this instance they're pretty clear:

• null (NOT "NULL")
• false (NOT "FALSE", see bool)
• true (NOT "TRUE", see bool)

(https://docs.backdropcms.org/doc-standards#types)

I don't think finding counter examples of this in core is reason to think the standards are wrong. More like the code is wrong and needs to be fixed.

Also, the examples you linked to are not very recent (not that that's really a factor anyway) - one's from 2016, the other's from 2013...

I'd suggest bringing this up in the core issue queue if you want to confirm the standards are correct, or if you'd like to suggest they be updated.

IMO, this repo is just about enforcing the already-documented standards.

[yorkshire-pudding]

Of course, PHP 8.2 confuses matters even further by creating true, false and null types - https://php.watch/versions/8.2#type-safety
Doesn't make sense to me

[indigoxela]

In fact, I've always used lowercase terms in comments. @kiamlaluno already linked our official docs. And that's also how this rule specifies. So, unless there's a change in our specs, the rule conforms to that. 😉

[herbdool]

This is something that still needs to be resolved, I think, since it's some of the same people adding the official docs and then doing something different in practice. The official docs are not written in stone, so we can either edit the official docs or go back and change the code that was being added.

As one of the core committers I wasn't involved in adding or editing the docs, so I'm not aware of how much discussion took place at the time they were added.

[herbdool]

Take a look at this issue: backdrop/backdrop-issues#1937

It was a systematic updating of core, which also involved adding NULL into various docblocks. This happened around the same time that the code standards page 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.

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

[ghost]

This is something that still needs to be resolved, I think, since it's some of the same people adding the official docs and then doing something different in practice.

Agreed @herbdool, but as I said above:

I'd suggest bringing this up in the core issue queue if you want to confirm the standards are correct, or if you'd like to suggest they be updated.

Because the people you're referring to likely aren't reading this issue. And as @indigoxela said:

unless there's a change in our specs, the rule conforms to that.

Meaning until there's concensus in core that our coding standards are wrong and/or need updating, there's no point debating all this in this queue.

[herbdool]

I've created an issue there backdrop/backdrop-issues#6014

[ghost]

As per the other issue, we decided nothing needed to change. So closing this issue too.