Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updates trans return type to be string or array #195

Merged
merged 1 commit into from
Sep 16, 2021

Conversation

jeremy-smith-maco
Copy link

@jeremy-smith-maco jeremy-smith-maco commented Aug 18, 2021

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 mr-feek added the fix label Sep 16, 2021
@mr-feek mr-feek merged commit 51879f2 into psalm:master Sep 16, 2021
@mr-feek
Copy link
Collaborator

mr-feek commented Sep 16, 2021

thanks!

@caugner
Copy link
Contributor

caugner commented Feb 18, 2022

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
Copy link
Author

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
Copy link
Contributor

caugner commented Feb 20, 2022

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

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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants