kontenta / kontour

Admin page manager for Laravel
2 stars 1 forks source link

Translation strings may reference file names in some apps #178

Open bjuppa opened 4 years ago

bjuppa commented 4 years ago

In a Laravel app where devs have created own language files for different areas we sometimes run into troubles with the __() translation method in the Kontour views.

For example if someone has created resources/lang/en/login.php for login related strings, our login form using __('Login') will throw an error that is really hard for user to find out why 🙀

I'm not sure what we can do about this in the package though, apart from documenting it... just interesting that it pops up the first week after releasing 1.0 😸

This is the message you'll get for the specific case above! Including it here for search engines to index.

htmlspecialchars() expects parameter 1 to be string, array given (View: /*****/vendor/kontenta/kontour/resources/views/auth/login.blade.php)
bjuppa commented 4 years ago

Maybe we shouldn’t use __() in a package? Look deeper what lang-methods are available to just pull out from the json translations!

bjuppa commented 4 years ago

The research

__() is declared to @return string|array|null and internally it just calls trans() which is declared to @return \Illuminate\Contracts\Translation\Translator|string|array|null (but the Translator is only returned if parameter is null, which __() does early return for anyway). So we're still guaranteed to have string|array|null.

trans() then resolves the Translator contract, and calls get() which is one of only two methods on the contract to retrieve translations, the other is choice() which doesn't look for json translations at all.

The Translator implementation does all its internal workings in get() and doesn't have separate internal support to differentiate json translations from the typical files... which makes sense as the contract provides no way to differentiate them!

There is also the @lang blade directive, but it doesn't seem to do anything more than passing along to get(). Which is a bit weird I must say, as the blade template would always require a string in the result, getting an array back would be weird!

Conclusion

Laravel provides no helper or method to pick values just from json translations or to return the original string if the translation is an array.

bjuppa commented 4 years ago

Ideally, the __() helper would return the original string whenever a non-string is returned from the Translator, or at least the @lang blade directive should behave that way.

I don't know why the Laravel helper is not defined to do that... maybe there is a use case for it? Is it worth raising the issue for discussion in the Laravel community? It surely won't get noticed here unless we help 😄

bjuppa commented 4 years ago

Now for Kontour, getting changes into Laravel won't help as long as we want to support some older versions of Laravel... Unless we document that this issue may occur in older versions and leave it at that?

The changes we can do in Kontour is to actually add a kontour.php translations file for english with proper keys.

We can't possibly provide the strings in many languages in the package, so we absolutely need the fallback language in Kontour requests to be en... which we would need to set in a middleware I guess? Or can it be set in Kontour's layout provider? 🤔

Whatever way we do it it'll make displaying any language config info in the Kontour dashboard confusing!

bjuppa commented 4 years ago

Actually, I may be too worried about that case when someone changes the fallback language in their app... If they do it’ll be pretty obvious that they’ll need to translate the kontour langue file into their fallback language. And they actually don’t even need to translate it, they can copy it and keep the English strings for minimal work!

bjuppa commented 4 years ago

PR's for modifying __() has been closed without merging in laravel/framework before, see: