Not a bug Missing parameter in DocBlock for \XF::isAddOnActive

[Kirby]

Affected version

2.2.15

/**
 * @param int|null $versionId
 * @param string|null $operator
 *
 * @return int|bool
 */
public static function isAddOnActive(
    string $addOnId,
    $versionId = null,
    $operator = '>='
)

The DocBlock is missing @param string $addOnId.

 

[Jeremy P]

This is deliberate, as the type hint is in the method signature. The docblock is only included at all because our minimum version requirements mean we cannot use native union type hints yet.

 

[Kirby]

I am not sure if I understand your comment @Jeremy P.

The method has three parameters (string $addOnId, int or null $operator, string or null $operator).

According to the DocBlock it has only 2 parameters (int or null $versionId, string $operator)

IMHO this is clearly inconsistent and triggers static code analysis issues.

 

[Jeremy P]

Can you clarify which static analyzer does not merge native type hints with PHPDoc? PHPStorm, Intelephense, and PHPStan at least all seem to correctly infer the type of $addOnId and understand that the PHPDocs supplement the other arguments without native type hints.

Redundant PHPDoc hints feel superfluous to me, though I respect it can be a matter of preference.

If the method were introduced in a version with PHP 8.0 as a minimum, the PHPDoc would have been omitted entirely:

public static function isAddOnActive(
    string $addOnId,
    int|null $versionId = null,
    string|null $operator = '>='
): int|bool

 

[Kirby]

Can you clarify which static analyzer does not merge native type hints with PHPDoc?

I used the following simplified example code:

<?php

class XF
{
        /**
         * @param int|null $versionId
         * @param string|null $operator
         *
         * @return int|bool
         */
        public static function isAddOnActive(
                string $addOnId,
                $versionId = null,
                $operator = '>='
        )
        {
                return false;
        }
}

Intelephense

Not entirely wrong, but the order is mixed up.

Inline Parameters Extended for VSCode


PHP DocBlock Checker

PHP Docblock Checker by Dan Cryer (https://www.dancryer.com)

F                                                   1/1 (100%)

Checked 1 files in 0.03 seconds.
0 Passed / 1 Errors / 1 Warnings

ERROR    src/XF.php:3 - Class \XF is missing a docblock.
WARNING  \XF::isAddOnActive - @param $addOnId missing.

PHP Code Sniffer
(Latest version, no ruleset/config given, warnings unrelated to this bug report stripped)

FILE: /tmp/xenforo/src/XF.php
------------------------------------------------------------------------------------------------------------------------------
FOUND 36 ERRORS AFFECTING 16 LINES
------------------------------------------------------------------------------------------------------------------------------

  6 | ERROR | [ ] Doc comment for parameter $versionId does not match actual variable name $addOnId
  7 | ERROR | [ ] Missing parameter comment
  7 | ERROR | [ ] Doc comment for parameter $operator does not match actual variable name $versionId

... and probably many more.

If the method were introduced in a version with PHP 8.0 as a minimum, the PHPDoc would have been omitted entirely

I am fine if there is no PHPDoc (for parameters) at all in this case - but if it is there it IMHO should correctly list all parameters to avoid issues like those posted above.

 

[Jeremy P]

Those analyzers/rules appear to be purely cosmetic (ie. concerned with style convention but not actual software bugs), similar in nature to XF failing to pass a PSR-12 style checker. Does the PHPDoc and native type hint combination cause any (widely used) static analysis tool to fail inform you of a potential software bug (such as passing incorrect types to the method call), where including $addOnId in the PHPDoc results in the correct behavior?

Many Symfony components use the same convention (omitting superfluous PHPDoc), so it is fairly common. I would regard the VSCode extension issues as upstream bugs (there is an open report about it).