psalm / psalm-plugin-laravel

A Psalm plugin for Laravel
MIT License
307 stars 72 forks source link

Updates trans return type to be string or array #195

Closed jeremy-smith-maco closed 3 years ago

jeremy-smith-maco commented 3 years ago

This is needed because this can be a valid translation file:

<?php

return [
    'some-string' => 'Abc',
    'some-array' => [
        'some-value' => 'Def',
        'some-other-value' => 'Ghi',
    ],
];

Laravel allows you to do trans('some-string') which would return a string type or do trans('some-array') which will return an array type or do trans('some-array.some-value') which will return a string type, all of which are valid. Therefore the return type should be changed to a string|array type.

mr-feek commented 3 years ago

thanks!

caugner commented 2 years ago

I stumbled upon this improvement, because it causes new issues like this:

ERROR: PossiblyInvalidCast - app/Notifications/TeamInvitationAccepted.php:49:23 - array<array-key, mixed> cannot be cast to string (see https://psalm.dev/190)
            ->subject(trans('myapp.team.mail.accepted.subject'))

Maybe I'm wrong, but to me it looks like this improvement, while correct in itself, doesn't really help, because now I essentially get an error whenever I pass the return value of trans() into anything that accepts either string or array, but not both.

Maybe it would be possible to parse the locale and to decide the return type based on the key, if Psalm knows it's literal value? 🤔

jeremy-smith-maco commented 2 years ago

Seems pretty complex for little gain when you can just assert() before passing it into ->subject(). It does mean having to store it to a temporary variable though. I am not sure if it's possible to do some sort of lookup with psalm.

caugner commented 2 years ago

I'm not sure if forcing developers to assert() every return value of trans() really makes a good developer experience. :shrug:

I am not sure if it's possible to do some sort of lookup with psalm.

This plugin already does some lookups, e.g. to infer the current database schema (and therefore available properties and their types): https://github.com/psalm/psalm-plugin-laravel/blob/master/src/Handlers/Eloquent/Schema/SchemaAggregator.php

jeremy-smith-maco commented 2 years ago

Yeah I mean you can make something like that which goes through every single language file and determine what each type of translation is but leaving it as a string return type is wrong. It's more correct having the union return type. Sure, it's more of a hassle having to assert it but you have to do that anywhere that has union return types in most cases unless you do some complex lookup thing like you are suggesting.