Example result of the autofix:
https://gerrit.wikimedia.org/r/#/c/439294/1/includes/Block/Block.php

I think we should distinguish two separate issues here:

1. Whether we want to have [optional] in the description of optional parameters.

2. Whether to fix incorrect use of [optional], which is also breaking Phan.

These are separate because we normally use [optional in the following way, which is not a problem for Phan:

/**
 * @param array|string|null $foo [optional] Description
 */

Here, the word [optional] is simply the first word of the free-form text description. The problem happens when it is wrongly injected in the middle of the typehint. I do not think this is intentional, but rather the result of many subsequent changes. The linked example is:

/**
 * @param string[optional]|null $type
 */

This is indeed invalid, but I have not seen this before and should be fairly rare. It was perhaps a written by someone (or an IDE generated it) with a different documentation syntax in mind, not the one we use at Wikimedia.

Should we repurpose this this task for updating these uses to consistently place [optional] in the description, to resolve the issue with Phan first?

Personally I fully agree with what is briefly outlined in the task description: If a parameter is optional or not is always visible from the function header in PHP (in contrast to JavaScript, where it is not). Extra documentation for this is not only not necessary, it is brittle and error-prone. For example, this is wrong:

/**
 * @param Foo $foo [optional]
 * @param string $bar
 */
function demo( Foo $foo = null, $bar )

The "optional" would mean one could call demo( $bar ), but this is (usually, except extra code is in place) not possible.

I believe we either need to write a PHPCS sniff that is able to fully understand even edge-cases like the one above. Or we remove and disallow any "optional". Personally, I vote for removal, because: It's the most simple thing we can do. We don't loose anything. The documentation even becomes more slick and cleaner. And most importantly: We remove a possible source of errors.

I believe we either need to write a PHPCS sniff that is able to fully understand even edge-cases like the one above

This is caught by Phan's PhanParamReqAfterOpt rule, which it seems we already enforce.

We remove a possible source of errors.

I agree that the duplication is undesirable. I didn't propose removal, because our Doxygen output does not (in my opinion) indicate the required/optional state in a way that is obvious and easy to understand.

Compare:

• https://doc.wikimedia.org/mediawiki-core/master/js/#!/api/mw.html-method-element
• https://doc.wikimedia.org/mediawiki-core/master/php/classHtml.html#a1c74fee14762ec4d50e09a94c94327ef

Doxygen does not process the signature, it doesn't extract information from it. The parameter listing has no indication of "optional" for the optional.

The only way to extract it is for the reader to mentally re-process the signature (included as-is by Doxygen on top), and assume that those with a default value are optional. While that should be correct in most cases, I find it confusing. For example, when the default value is of a different type, or when the "effective" default value is computed elsewhere.

Oh well. Thanks a lot for the additional insight!

In JavaScript, we do something like [param='default value'] to indicate optional parameters with their exact default value, no matter where that comes from (what you call "computed elsewhere"). This is quite nice. But the [optional] we are talking about here just can't do this. Therefor I still tend to remove it, for the reasons already listed above.

On the other hand: As long as the [optional] is just text and does not mess with the first section of the @params type $var tag, why should we disallow it? It's not really different from an other pattern I have seen a hundred times in our codebases: To indicate a parameter is optional, along with it's default value, we write Defaults to "default value". at the end of the comment. Done. If this is fine, why should it be forbidden to write [optional] in the comment?

Indeed. It is a purely textual convention with no run-time impact on any of the tooling involved. Within the objectcache and rdbms libs, @aaron and I have been (loosely) following the convention of using [optional] Description. Default: "x".

As such, I proposed to limit this task to only be about fixing the odd-cases that (unintentionally?) placed the [optional] marker earlier in the parameter syntax, which affects Phan's ability to process the information.

I've gone ahead and renamed the task summary. @Umherirrender Does that address your concern?

Change 472525 had a related patch set uploaded (by Umherirrender; owner: Umherirrender):
[mediawiki/tools/codesniffer@master] Remove [optional] from types in @param

https://gerrit.wikimedia.org/r/472525

Change 472525 merged by jenkins-bot:
[mediawiki/tools/codesniffer@master] Remove [optional] from types in @param

https://gerrit.wikimedia.org/r/472525