Psalter: generate more portable types

[staabm]

Use psalter --issues=InvalidReturnType --php-version=5.6

On

function rex_mediapool_deleteMedia($filename)
{
    if ($uses = rex_mediapool_mediaIsInUse($filename)) {
        $msg = '<strong>' . rex_i18n::msg('pool_file_delete_error', $filename) . ' '
            . rex_i18n::msg('pool_object_in_use_by') . '</strong><br />' . $uses;
        return ['ok' => false, 'msg' => $msg];
    }

    $sql = rex_sql::factory();
    $sql->setQuery('DELETE FROM ' . rex::getTable('media') . ' WHERE filename = ? LIMIT 1', [$filename]);

    rex_file::delete(rex_path::media($filename));

    rex_media_cache::delete($filename);

    rex_extension::registerPoint(new rex_extension_point('MEDIA_DELETED', '', [
        'filename' => $filename,
    ]));

    return ['ok' => true, 'msg' => rex_i18n::msg('pool_file_deleted')];
}

We get

The type @return (bool|string)[] is rather exotic, and is not supported in some IDEs/tools. Since psalter also generates a very specific @psalm-return array{ok: bool, msg: string}, wouldnt it make sense to let the @return be generated in a form which is compatible with a lot of tools e.g. array (since psalm/phpstan will nevertheless pick the psalm-return)?

Tested on 3.8.0

[muglug]

The type @return (bool|string)[] is rather exotic, and is not supported in some IDEs/tools.

It's valid PHPDoc - what tools is it not supported by?

[orklah]

It's valid PHPDoc - what tools is it not supported by?

just tested, PhpStorm don't support it.

[staabm]

just tested, PhpStorm don't support it.

Thats what I had in mind.. wanted to verify first.. but no time yet

[muglug]

just tested, PhpStorm don't support it.

UGH.

[ADmad]

@staabm I hope you have submitted an issue for phpstorm too :slightly_smiling_face:.

[staabm]

Nope, @ADmad feel free to do so :)

[orklah]

@staabm I hope you have submitted an issue for phpstorm too 🙂.

There's already a few tickets to ask for support of more powerful phpdoc annotations, either directly or through implementation of a static analysis tool: https://youtrack.jetbrains.com/issue/WI-43843 https://youtrack.jetbrains.com/issue/WI-6558 https://youtrack.jetbrains.com/issue/WI-47158 https://youtrack.jetbrains.com/issue/WI-45248 https://youtrack.jetbrains.com/issue/WI-35063

Feel free to vote for some of them :)

EDIT: also https://youtrack.jetbrains.com/issue/WI-5304

[ADmad]

If I was a phpstorm user I would have :)

I don't see why psalm should stop generating valid phpdoc tags, it's phpstorm which needs fixing.

[orklah]

If I was a phpstorm user I would have :)

I don't see why pslam should stop generating valid phpdoc tags, it's phpstorm which needs fixing.

In the report, @staabm said that psalter also generates an "@psalm-return" notation, which can be as specific as needed. If the "@return" is also generated, it's for the sole purpose of tools that doesn't support specific notation. The fact that the "compatibility" notation is not compatible with major tools is an issue 🙂.

If you look at the tickets I posted, there are very old one that suggests totally different annotations, and the history of PHPDoc annotations was not a straight line, there was attempts and deprecations of existing annotations. You can't expect every tool to be compatible with every annotation at any time.🙂

[ADmad]

If the "@return" is also generated, it's for the sole purpose of tools that doesn't support specific notation.

It's for tools that support standard PHPDoc type notation.

The fact that the "compatibility" notation is not compatible with major tools is an issue

Which other major tool besides phpstorm has issues with such type? For example VSCode (with Intelephense extension) does support this type of notation.

Phpstorm might be the dominant PHP IDE these days but others shouldn't suffer for it's inadequacies.

One can manually edit the notations which their tool doesn't support after running psalter.

[orklah]

Things finally changed in PHPStorm those last months.

It now supports advanced notation like array<string|bool>. However, no progress have been made on phpdoc notation like (string|bool)[].

That means the compatibility mode put in @return is actually not readable for phpstorm, but the non-compatible mode put in @psalm-return is almost always readable.

Unfortunately, there is still a last incompatibility in PHPStorm: it doesn't support array shapes yet like array{string, bool}.

I'll see how it progress in weeks to come, but I'm inclined to create new configs in psalter to choose what we want in @return and @psalm-return

[orklah]

For whoever it may concern: PHPStorm released a new EAP release where they introduced limited support for array shapes and they're working on templates. There are still holes in their support but syntax-wise, it's pretty good.