Laravel-Backpack / theme-tabler

UI for Backpack v6 that uses Tabler and Bootstrap v5.
MIT License
21 stars 12 forks source link

Using `trans(simple_string)` instead of namespaced `backpack.simple_string` #107

Closed pxpm closed 9 months ago

pxpm commented 12 months ago

Screenshot 2023-07-12 174026

Hi, Sorry for writing after the question was closed, but I thought you would look for all cases where the trans('one_word') structure is used and fix them. <label class="form-label" for="{{ $username }}">{{ trans(config('backpack.base.authentication_column_name')) }}</label> - resources/views/vendor/backpack/theme-tabler/auth/login/inc/form.blade.php trans(config('backpack.base.authentication_column_name')) == __('Email') and it will also throw an error if there is an email.php file in the lang folder.

Originally posted by @smart4you in https://github.com/Laravel-Backpack/theme-tabler/issues/96#issuecomment-1632675374

pxpm commented 12 months ago

@promatik picked up a PR I had https://github.com/Laravel-Backpack/theme-tabler/pull/97 (it was good but not complete). I've also found this issue that the user now mentioned but I haven't gone back to work on the PR.

We still have this place where we use trans('string') instead of trans('namespace.string').

tabacitu commented 12 months ago

Sorry for writing after the question was closed, but I thought you would look for all cases where the trans('one_word') structure is used and fix them.

We don't have a way to look for all places, since it can be a simple string like before but it can also be a config (like here) that just outputs a string. So we're counting on you to tell us if there are more places where we do this, thanks.


@smart4you I can't seem to replicate your error.

I've created an email.php lang file:

CleanShot 2023-07-13 at 08 32 22@2x

But the login form is loading fine for me:

CleanShot 2023-07-13 at 08 33 17@2x

Do I need to do anything else to reproduce this? Are you sure this is the cause for your error? Can you please share your full error stack? You can do that using the Share button in the Flare error page.

Cheers!

smart4you commented 12 months ago

Hi, Yes, you are right, there is indeed a nuance. I installed a new Laravel project and Backpack and everything worked, but when I added the translation to the file, I immediately got an error. Probably the email.php file is somehow ignored if it is empty without translations. I just added "test" => "test" for the test and the error appeared. So far I've only seen this behavior on the Login and Register forms.

promatik commented 11 months ago

@pxpm sorry, I didn't know there was more, the first mistake was actually mine 🙃 I did the __('Admin') while working on themes. So I though it was only that one.

Anyway, I searched for all the trans( and __( and I couldn't find more. I found 2 using the same config config('backpack.base.authentication_column_name'). But I don't think this one is an issue.

1) It should never be an array, it should be a string. 2) It is a column with a name provided by the developer, not on the backpack scope.

Let me know if I'm wrong, otherwise I think we can close this one.

smart4you commented 11 months ago

Hi, My appeal to the Backpack team was to inform you that there is a recommendation in the official documentation of Laravel https://laravel.com/docs/10.x/localization#defining-translation-strings about the fact that it is better not to use this behavior. And the fact that this situation is real and not theoretical.

Key / File Conflicts You should not define translation string keys that conflict with other translation filenames. For example, translating __('Action') for the "NL" locale while a nl/action.php file exists but a nl.json file does not exist will result in the translator returning the entire contents of nl/action.php.

But you yourself will decide whether it is worth taking this recommendation or can be ignored.

promatik commented 10 months ago

Hey everyone, I searched on the project for not namespaced translations, and carefully went one by one and I couldn't find any. I'll close this one for now, if anyone finds something, please reopen 🙌

image

pxpm commented 10 months ago

In your image, on the form.blade.php there is one of those cases. trans(config('backpack.base.authentication_column_name)) that will translate into something like: trans('email').

Cheers

promatik commented 9 months ago

Fixed with https://github.com/Laravel-Backpack/theme-tabler/pull/145 👌