-
Notifications
You must be signed in to change notification settings - Fork 13
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
Wip translation #93
Wip translation #93
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #93 +/- ##
=============================================
- Coverage 99.43% 99.18% -0.26%
- Complexity 302 322 +20
=============================================
Files 14 16 +2
Lines 710 734 +24
=============================================
+ Hits 706 728 +22
- Misses 4 6 +2
Continue to review full report at Codecov.
|
src/TranslatableTrait.php
Outdated
* | ||
* @return string | ||
*/ | ||
public function __(string $string, ...$args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not simply call return $this->app->_($string, ...$args); ?
but only if app is set though, otherwise lets return without translation.
that would be a good backwards compatibility..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i change it with the new implementation defined in meeting 13-06
src/Translator.php
Outdated
|
||
final class Translator implements TranslatorInterface | ||
{ | ||
use ConfigTrait; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i believe this shouldn't be inside "core" but rather outside and "suggested" for those that want ATK translations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it also would probably require dependencies..
src/TranslatorInterface.php
Outdated
|
||
namespace atk4\core; | ||
|
||
interface TranslatorInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just wondering - is there already a interface for translators?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the only one i found is this : https://gist.github.com/oscarotero/33e3af8741045c2a1a5a89310571cbdb
tests/TranslatableMock.php
Outdated
1 => 'string translated', | ||
], | ||
'string not translated with plurals' => [ | ||
0 => 'string translated zero', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps we can use Italian? 💃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i will do :D
src/TranslatableTrait.php
Outdated
} | ||
} | ||
|
||
return call_user_func_array('sprintf', $translated_args); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- use TransatableTrait
- $this->__('we add %s into %s', ['milk', 'bowl']);
^^ the only thing trait should do is check for API and fallback to sprintf..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do it but i add two helper method that make the usage of the trait much short and declarative with array. look in this test for helper usage :
https://github.com/abbadon1334/core/blob/dc07b17f949fdea4f42c7b2b56351b5dd15736e1/tests/TranslatableTraitAppTest.php#L271
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, can we use $this->__('we add {thing} into {container}', ['thing'=>'milk', 'container'=>'bowl'])
?
That should be compatible with http://userguide.icu-project.org/formatparse/messages.
@todo : test for domain
Simplify everything.
_(string $message, int $count = 1, string $context = 'atk4'): string
public function _(string $message, int $count = 1, string $context = 'atk4'): string;
public function _d(string $message, string $context = 'atk4', int $count = 1): string; just because the first can be used like this : $this->_('apple'); // => mela ( singular )
// OR
$this->_('apple',2); // => mele ( plural 2 form )
// OR
$this->_('apple',0); // => nessuna mela ( zero form)
// WITH DOMAINS
$this->_d('apple', 'my_domain'); // => mela ( singular )
// OR
$this->_d('apple', 'my_domain', 2); // => mele ( plural 2 form )
// OR
$this->_d('apple', 'my_domain', 0); // => nessuna mela ( zero form)
// every call above will call the method if exists
$this->app->_(string $message, int $count = 1, string $context = 'atk4'): string i choose to split the method in two for easy usage because we need to avoid the check for numeric or string or count of parameters, in this way even autocompletation works correctly, TODO : added test for context domain @romaninsh, @DarkSide666 what we do in the app for adding the method _?
i think the second is correct, but i have an idea : |
I wanted to propose so that the App controls all translations, but that doesn't seem like the greatest idea if you'd ever want to translate anything that's not using the Atk4/ui components and the App class. I thought that we could just add the translation trait to App and View, and have the other elements simply use a simplified trait, that'd try to pass the translation to either App (via AppScope) or View (via TrackableTrait -> owner?) and run the translation there. That way all classes (outside of Atk4/ui) would not "support" translation, but instead have a dummy trait that'd specify all the required functions, and when used together with UI, they would have access to the translation class governed by App/View. How does that sound? |
If you use singleton you can use it even outside of atk4, if you want to integrate in atk4 you can do it in 3 way :
i already put out an idea in the previous message :
in this way we can avoid the use of AppScope in library out of atk and use : @pkly If you want to contribute even with a total different implementation, you are welcome! but i think the problem is not how to use it in atk than using it with minimum changes out of atk4 without confusing the developer |
Ah, right, my bad. As for how useful it'd be outside of atk4 and everything related to that - can't say I really have an idea there, as I'm already using a custom solution that's just a singleton, mostly because I need it to be bound with the CMS I'm building on top of atk. |
@DarkSide666 said something really interesting, think about a page with two view with different languages without using two app, that can be awesome in many situation like managing languages in a cms or manage template emails, not needed OK, but in some specific cases can be a saver. Using a singleton make everything much easier, but after you enter that path you cannot return and make everything too much coupled. |
Well yeah, I do understand the use scenario. I already went the singleton route for my translation needs, I needed to have every string possible already translated from the get-go, because I can't release a product that doesn't support unlimited UI languages. Talking about languages and all, if you'd like I could give you the code for the translation engine I've already built - you could pull out the functions for searching files and such, it allows to load translated strings from .json files (lazily), so it's pretty comfortable to use. You'd just need to modify it for the numbered scenarios. |
Sure can be inspiring because you probably have already faced some problem and found a solution. |
src/TranslatableTrait.php
Outdated
*/ | ||
private $translator; | ||
|
||
public function setTranslator(TranslatorInterface $translator) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i forget comment on method
src/TranslatableTrait.php
Outdated
$this->translator = $translator; | ||
} | ||
|
||
public function getTranslator() : ?TranslatorInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i forget comment on method
src/TranslatableTrait.php
Outdated
/** | ||
* @var TranslatorInterface | ||
*/ | ||
private $translator; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still think about if this private can be raise to protected, user must use SetTranslator to set and after that is all in the hand of method _ that will check to which object delegate the translations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait,
TranslatableTrait is applied on ANY class that wants to use translations.
it adds method _
which routes translations requests to APP or returns string as-is in case of a fallback.
Why do we need translator defined here? Would that mean that pretty much all models and fields and whatnot will carry reference to translator?
* | ||
* @var bool | ||
*/ | ||
public $_translatableTrait = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added only for consistency with other traits but not used here
src/TranslatableTrait.php
Outdated
// if not use TranslatableTrait and is not a Child of App which use TranslatableTrait | ||
$translator = new class() implements TranslatorInterface { | ||
use TranslatorTrait; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kind of wasteful to create a object every time, maybe add a static member, or a private one and initialize that if it's null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think it's a misuse of "trait", in this case - just use of regular class would suffice, although I don't want ATK to handle the case when $app is undefined and should simply return $id
;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about if we want to have a fallback class here, but I still believe it's bad, because some users may end up relying on it and it would re-read language files on every invocation, which is an INSANE performance impact.
Either we use $app->translator or rely on service locator pr #99 inside App class. Either way - trait should assume that translator would be pre-initialized with the language files loaded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the fallback class should not ever translate anything, only parse the incoming string, I believe.
As for this specific line of code I'd simply suggest something like
// where self::$_translationFallback = static private member defined to null
if (self::$_translationFallback === null) {
self::$_translationFallback = new class() implements TranslatorInterface {
use TranslatorTrait;
};
}
// use self::$_translationFallback
But that's only assuming that the TranslatorInterface/TranslatorTrait is required to parse the text, not actually translate it, or create this object somehow that it doesn't actually try to load language files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that's only assuming that the TranslatorInterface/TranslatorTrait is required to parse the text, not actually translate it, or create this object somehow that it doesn't actually try to load language files.
As you said, it only parse the arguments to return an excepted results.
My idea is that in this way we don't have to delegate to $this->app in case of external use, i think that $_translatorFallback will be called only if the Class not use AppScope or $translator is not set, just to have a fallback for usage of syntax excepted by Symfony\Components\Translation, that's it.
The idea was to use on every Object the same Trait and always use the method _ in place of $this->app, because in this case we have to move everything in AppScope or make 2 trait TranslatableTrait and TranslatorTrait.
If you make it today at 18:00 London, we have a meeting.
I don't know in which timezone you are, but you can come and partecipate, at this link : https://meet.google.com/dfb-vquj-kkk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well yes, I understand why you did it and I prefer having the _
function available in the classes themselves instead of calling $this->app
constantly, that does seem like the better way to do it, since you can move any error checking to _
as well.
I should be able to make it, I'm in +2 UT so it's not really an issue.
@romaninsh I can discuss it any time during bussiness hours (as long as I'm not working on another project) and maybe a bit later as well, but I don't really know how to get in touch outside of github and the forum, and this doesn't seem like the best way to talk really. I'm fine with either sprintf-like syntax or tag-like syntax, the latter could be a bit more easier to change down the line. Though if you're gonna use that, probably just add a static method somewhere in core so that it's called by the translator (only tag parsing, not translation itself). I would much prefer use of atk\core namespaces for translation, since that could add support for translation outside of using atk\ui. There's no real need to use multiple translators, at least I don't think so, but I mentioned it as a possibility since we're running objects, so each one could have a different instance of a translator, and a different class all together. Can't say I'm a huge fan of adding dependencies for everything for something that could be implemented in the library, but I won't really object, since it seems that this is just a few files. @abbadon1334 ['There are 10 apples', '{0} There are no apples|one: There is one apple|more: There are %count% apples', 10], I took a peek at the symphony code and I still don't quite get it. |
['There are 10 apples', '{0} There are no apples|one: There is one apple|more: There are %count% apples', 10],
$expected => 'There are 10 apples' which are used as dataprovider in this test : $app->_($id, ['%count%' => $number]); // if no translations, return $excepted below equivalent definition : // Symfony ICU-Message
['There are 10 apples', '{0} There are no apples|{1} There is one apple|]1,Inf] There are %count% apples', 10],
// Symfony STD
['There are 10 apples', '{0} There are no apples|one: There is one apple|more: There are %count% apples', 10], |
So one would use it as $text = '{0} There are no apples|one: There is one apple|more: There are %count% apples';
$res = $app->_($text, ['%count%' => 0]);
// $res: 'There are no apples'
$res = $app->_($text, ['%count%' => 1]);
// $res: 'There is one apple'
$res = $app->_($text, ['%count%' => 15]);
// $res: 'There are 15 apples' Kinda like that? Does the symphony implementation support custom tags as well, like @romaninsh requested earlier? It seems like it would with the %count% and whatnot. |
if translator is defined and a translation entry is found for |
Right. I understand it now, but it's still a bit hard to get by simply looking at test cases, overall the implementation seems good but there's still the 'tag' question - does this support tag replacement for translations? I haven't seen the code that'd allow that, but if it's there then other than some docs with typical use cases it's fine. Also you didn't answer the question about tags. Is that also supported here? |
yes, and if there is a translation for that specific combination it will translate, if not it will only substitute. i think more test is needed to show better usage and obviously some examples |
the whole package symfony/translation is needed only for translations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still things to address. BTW - could we have symfony dependency as "dev" ? we can run a test against it, but it looks like we are not really using it. I'd been fine if it was a "psr/translator".
src/TranslatableTrait.php
Outdated
/** | ||
* @var TranslatorInterface | ||
*/ | ||
private $translator; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait,
TranslatableTrait is applied on ANY class that wants to use translations.
it adds method _
which routes translations requests to APP or returns string as-is in case of a fallback.
Why do we need translator defined here? Would that mean that pretty much all models and fields and whatnot will carry reference to translator?
src/TranslatableTrait.php
Outdated
// if not use TranslatableTrait and is not a Child of App which use TranslatableTrait | ||
$translator = new class() implements TranslatorInterface { | ||
use TranslatorTrait; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think it's a misuse of "trait", in this case - just use of regular class would suffice, although I don't want ATK to handle the case when $app is undefined and should simply return $id
;
src/TranslatableTrait.php
Outdated
break; | ||
} | ||
} | ||
return isset($this->app) && $this->app !== $this && method_exists($this->app, '_'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this must be consistent with this:
if (!isset($this->app) || !isset($this->app->logger) || !$this->app->logger instanceof \Psr\Log\LoggerInterface) {
https://github.com/atk4/core/blob/develop/src/DebugTrait.php#L53
src/TranslatableTrait.php
Outdated
// if not use TranslatableTrait and is not a Child of App which use TranslatableTrait | ||
$translator = new class() implements TranslatorInterface { | ||
use TranslatorTrait; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about if we want to have a fallback class here, but I still believe it's bad, because some users may end up relying on it and it would re-read language files on every invocation, which is an INSANE performance impact.
Either we use $app->translator or rely on service locator pr #99 inside App class. Either way - trait should assume that translator would be pre-initialized with the language files loaded.
@abbadon1334 OK i see what you did there 😄 . you are using TranslatableTrait for both class-trait and a translator. Sorry you had to move things so much, please split this up:
As a test - we can try integrating this: "https://github.com/symfony/translation/blob/master/Translator.php#L195" into a mock app, then trying to translate, also adding 'symfony/translation' as dev-dependency and in recommended section. Few last bits:
|
you said that you want to use $this->app->translator which means that translator can be done only using AppScopeTrait?
i agree too, but i see that there are two wide used libs : Symfony or i18next.com, which are the only one that permits usage of translations without static reference with complete cover of cases like the ones we speak in chat
you point me to a specific line you mean the method alone or the whole Symfony Translator? just to be sure
that "return $id" kills me :) , because returning $id is easy, the problem is to replicate all the implementation of translator library and in the future remaining complaint without BC.
Probably translations can be loaded even with ConfigTrait, divided in files or folders. I'm probably too defensive, but adding things must be simple and strictly defined, if not i think is better to delegate it, like in this case, SetTranslator => i translate, No Translator => i return processed string from a sort of NullTranslatorInterface that replicate everything and return an excepted result. Let's speak again tomorrow :D |
be consistent means implementing this, which i have done => https://github.com/symfony/contracts/blob/master/Translation/TranslatorTrait.php @romaninsh you are right that if implemented like this, we are strictly coupling with it, but if we do it on our own, if there is a change we have to figure out what is wrong with our code. |
i started making some changes based on last discussions. |
in AppScope for more complex translations
src/TranslatorSymfonyTrait.php
Outdated
*/ | ||
public function setTranslator($translator) | ||
{ | ||
if ($translator instanceof TranslatorInterface) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So Symfony/contracts will be a hard-dependency then?
Or does that not actually load this class until you reference the TranslatorSymfonyTrait at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has you see : src/TranslatorSymfonyTrait.php
is the Trait helper to use Symfony, if you want to use it you are free to use it.
every "bridged" translation class must pass from here :
public function _($message, ?array $parameters = NULL, ?string $domain = NULL, ?string $locale = NULL): string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well yes, I understand that, I think I understand it a bit better now that I looked a bit at the tests.
So basically you're creating an app and adding the translator TranslatorSymfonyTrait to it, otherwise (and by default) using the TranslateableTrait and it's minimal handling, correct?
But does that support the syntax that Symfony supports? This to be exact:
'{0} There are no apples|]0,1[ There is almost one apple|{1} There is one apple|[1,Inf] There is more than one apple'
Your code simply explodes the text on |
and assumes that each item is the next one in the counter, yes?
If you don't support that by default there could be issues with the translations, if you allow this syntax to be used at all by anyone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
take a look here :
[https://github.com/abbadon1334/core/blob/wip-translation/tests/TranslatableTraitTest.php]
and here :
[https://github.com/abbadon1334/core/blob/wip-translation/tests/TranslatorSymfonyTraitTest.php]
TranslatorSymfonyTrait => is only a helper to implement method _
in App, setTranslator and nothing more, to be used with a standard Symfony\Translation object
I'm working on rewrite the php library of I18next.com, actual implementation in php really sucks and doesn't support all the features, is used in many js project but support really easy even nested plural and substitution, the most interesting features is that accepts interpolation of token with array objects which can be interesting in atk4/data implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right.
Well from what I've seen their listed implementation is really basic, so... It actually looks a lot like my translator, which is fine for things that I'm using it for.
Still, you're just gonna use Symphony for translations for atk, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the last discussion we want to use an internal translation system but really basic, just to cover the few needs of actual atk4 translations, and leave any other complex implementation out of atk4.
i think this is the cases which at now will be covered, interpolation and simple plurals:
https://github.com/abbadon1334/core/blob/e0c538c6179444e478cf711ae4baa1aa84147f99/tests/TranslatableTraitTest.php#L40-L53
work in progress, but already useable without using gettext because for what i have seen is used as a singletone at low level and can't be changed ( or i didn't find any prove i was wrong about this).
I still think about last discussions about this topic and we speak about having App has a reference for object translator, but after that we speak about addons that can have some defined translation even in saasty, this translations can impact the standard translations that we have in ui or another addon, 3 possible solution :
Choose your destiny :D and give feedback on this wip PR