WIP : Lokalise integration - test on Exception #110

abbadon1334 · 2019-09-28T18:38:41Z

in composer i add : "atk4/data": "dev-lokalise-2019-09-28_17-10-29"
because i need : https://github.com/atk4/data/blob/lokalise-2019-09-28_17-10-29/src/Locale.php
to get the folder of the language definitions path.

I create a singleton Translation with an interface adapter where all the call to _ will come, to allow replace the default Translator with another.

I change the structure of the Lokalise export, because usually every Translator or most of them load with definitions with this structure {root-folder}{language-code}{translations-files}

Cleaned up some things related to Translations.
Removed Symfony Test, will readd later with TranslatorAdapter implementation.

It works even for plural forms, but need some clean up and discussions, like how to add more translations and how to define a different path to load translations, probably i need to split Adapter\Generic in Adapter\Generic that pass calls to _ method to a GenericTranslator.

acicovic · 2019-09-30T17:34:45Z

Thanks for all the work!

This is too much for me to review at the moment, if @romaninsh and @PhilippGrashoff are OK with this then so am I :)

pkly · 2019-10-01T06:53:39Z

src/Translator/Translator.php

+        }
+
+        return self::$instance = new static();
+    }


Isn't this usually written like so?

if (self::$instance === null) { self::$instance = new static(); } return self::$instance;

you know, less code and simpler.

abbadon1334

@pkly Translator is a singleton, all calls to method Translator::_ will be passed to Translator->adapter->_, you can set an external adapter to Translator following the iTranslatorAdapter interface which require only definition of method _

I think this is the only way to decouple Translator, because is not intended to be used only in atk4/ui.

About the code syntax, there are for sure other point to review, consider it like a concept because it will be the first singleton in the whole atk.

I try to put out code as fast as i can after last friday discussion, because we need to discuss about this, which is a controversial argument in the Team.

pkly · 2019-10-01T07:24:33Z

Yes, yes, I wrote that before I noticed the adapter in the code for the Translator.

If you'll be discussing translation as a whole, mind if I join in? Do you have the dates and links listed somewhere?

PhilippGrashoff · 2019-10-01T07:32:49Z

@pkly our weekly hangouts are at 6pm London time on thursdays. Of course you can join in :)

abbadon1334 · 2019-10-01T07:34:59Z

Yes, yes, I wrote that before I noticed the adapter in the code for the Translator.

If you'll be discussing translation as a whole, mind if I join in? Do you have the dates and links listed somewhere?

is a pleasure to have one more in, @romaninsh changed work and is a little bit busy, but we are trying to continue with the meeting that speed up the whole process.

Last time we made a meeting directly in gitter, it was good and @gowrav-vishwakarma popup with an idea about definition of what has to be translated in objects suggesting to define it an array like :

public $translate = [
'properties' => ['caption']
'fields' => ['name','sex']
'template' => ['area']
];

For now i think the subject to be discussed is the implementation of the first singleton in ATK, which, like i said before is a change too big to be discussed without the whole team.

pkly · 2019-10-01T07:43:46Z

Alright, I'll try to be there (on gitter anyway). I'm mostly interested in implementation of this because of how I'm integrating atk4 into the stuff we're writing, and we need translations. I've already bodged them in there once, but I'm changing the library we're using to the one I'm writing atm. which is a port of i18next.

pkly · 2019-10-03T13:01:03Z

So since this is still in the works, I'd like to know if it's trying to be compatible with any kind of syntax?
I saw that @abbadon1334 is creating a "port" of i18next, but I'm not sure it's actually 100% compatible with i18next's syntax for fetching files?
From what I understand the i18next syntax is something like [namespace:]key[.deep], so what is context here then?

I believe there's been keys like ns:key listed but then there's context... Is that just like ns:key.context ?

I guess extracting all that data is rather painful, but still.

abbadon1334 · 2019-10-03T15:26:15Z

So since this is still in the works, I'd like to know if it's trying to be compatible with any kind of syntax?

There is no standard, just stay simple on core translation i think we can accomplish something near that.
I add the adaptor, because already know that no translator will be perfect compatible now or in the future. If we choose only one we will have coupling problems.

I saw that @abbadon1334 is creating a "port" of i18next, but I'm not sure it's actually 100% compatible with i18next's syntax for fetching files?

From what I understand the i18next syntax is something like [namespace:]key[.deep], so what is context here then?

Namespace are optional, deep definition and context too.
context is in definition or via parameters : https://www.i18next.com/translation-function/context
namespaces : are like override of definitions over the default ones and can be prioritized
e.g. group => group, myproject:group => people
if priority is human first for group will look in myproject:group and fallback to group

I believe there's been keys like ns:key listed but then there's context... Is that just like ns:key.context ?

it's a start we can see how move further, i suggest use keys because with keys we are much compliant with other translator, everyone has special reserved chars and special format.

With key in place of phrases everyone can added a translator much easy, otherwise is a return $defintion[$key] ?? $key dictionary but can't be improved with other functionalities.

I guess extracting all that data is rather painful, but still.

Few days ago we speak about adding backend caching like redis, when we add it to ATK will be much easier to cache heavier procedure for production, but for core translations is not a so heavy task to be done with this simple implementation.

romaninsh · 2019-10-03T17:20:43Z

src/Exception.php

@@ -41,6 +56,8 @@ public function __construct(
            $message = array_shift($this->params);
        }

+        $message = $this->_($message);


we won't have access to $api yet ..

also - lets move to $app->caughtException() - or whatever it's called now.

romaninsh · 2019-10-03T17:23:48Z

src/Exception.php

-        $output .= "\n\033[1;31m-------------------------------------------------------\n";
-
-        return $output."\033[0m";
+        return (string) new Console($this);


possibly pass translator inside this.. ?

how about default callback here if exception is occured inside?

App wouldn't really be available in here though, would it? So the singleton is the only option left.

romaninsh · 2019-10-03T17:24:33Z

src/ExceptionRenderer/Console.php

+    {
+        $text = <<<TEXT
+\e[0m{FILE}\e[0m:\e[0;31m{LINE}\e[0m {OBJECT} {CLASS}{FUNCTION_COLOR}{FUNCTION}{FUNCTION_ARGS}
+


love this ;)

romaninsh · 2019-10-03T17:29:09Z

src/TranslatableTrait.php

-    public function _($message, ?array $parameters = null, ?string $domain = null, ?string $locale = null): string
-    {
-        if (isset($this->app) && method_exists($this->app, '_')) {
-            return $this->app->_($message, $parameters, $domain, $locale);


i think we should keep it so we can user have control of translating process in the app.

(those 3 lines), the rest can fall back to singleton.

I more or less second this

pkly · 2019-10-03T18:09:32Z

As for the talk in the meet, I'd only like to say a few more things:
Please use namespaces of some sort. From my experience static text as a namespace is way simpler than doing something like __CLASS__ and working back from there. Namespaces could change, and the more you have the more complicated could the thing become.

It would be more than enough to have a few namespaces for atk, for example:
atk_core, atk_ui, atk_data, atk_dsql and so on.
It would simplify the function calls, lookups and maybe improve runtime? I'm not quite sure how PHP handles things like __NAMESPACE__, but it's obviously runtime.

abbadon1334 · 2019-10-07T06:04:34Z

i add the method :

core/src/Exception.php

Lines 233 to 236 in cce424e

    
           public function translate(?ITranslatorAdapter $adapter = null): void 
        
           { 
        
               $this->message = null !== $adapter ? $adapter->_($this->message) : Translator::instance()->_($this->message); 
        
           }

for the Exception specific to "inject" the translator and translate the needed translations

composer require-dev

codecov · 2019-10-07T08:23:33Z

Codecov Report

Merging #110 into develop will increase coverage by 0.05%.
The diff coverage is 100%.

@@              Coverage Diff              @@
##             develop     #110      +/-   ##
=============================================
+ Coverage      99.33%   99.39%   +0.05%     
- Complexity       387      420      +33     
=============================================
  Files             21       22       +1     
  Lines            899      987      +88     
=============================================
+ Hits             893      981      +88     
  Misses             6        6

Impacted Files	Coverage Δ	Complexity Δ
src/ExceptionRenderer/HTML.php	`100% <100%> (ø)`	`25 <0> (ø)`	⬇️
src/Exception.php	`100% <100%> (ø)`	`18 <6> (+3)`	⬆️
src/TrackableTrait.php	`100% <100%> (ø)`	`7 <0> (+3)`	⬆️
src/ExceptionRenderer/Console.php	`100% <100%> (ø)`	`23 <0> (ø)`	⬇️
src/ExceptionRenderer/HTMLText.php	`100% <100%> (ø)`	`23 <0> (ø)`	⬇️
src/ExceptionRenderer/RendererAbstract.php	`100% <100%> (ø)`	`21 <6> (+6)`	⬆️
src/ConfigTrait.php	`100% <100%> (ø)`	`25 <0> (ø)`	⬇️
src/Translator/Adapter/Generic.php	`100% <100%> (ø)`	`18 <18> (?)`
src/Translator/Translator.php	`100% <100%> (ø)`	`17 <17> (?)`
src/ExceptionRenderer/JSON.php	`100% <100%> (ø)`	`23 <0> (+1)`	⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3fd7171...a893180. Read the comment docs.

pkly · 2019-10-07T08:34:46Z

i add the method :

core/src/Exception.php

Lines 233 to 236 in cce424e

public function translate(?ITranslatorAdapter $adapter = null): void

{

$this->message = null !== $adapter ? $adapter->_($this->message) : Translator::instance()->_($this->message);

}

for the Exception specific to "inject" the translator and translate the needed translations

Didn't we talk about this during the meet and @romaninsh specifically asked to not translate this property at all (which I agree with completely), but instead translate it when rendering (so that'd be in getColorfulText(), getHTMLText(), getHTML() and getJSON()?

abbadon1334 · 2019-10-07T08:42:00Z

i add the method :

core/src/Exception.php

Lines 233 to 236 in cce424e

public function translate(?ITranslatorAdapter $adapter = null): void

{

$this->message = null !== $adapter ? $adapter->_($this->message) : Translator::instance()->_($this->message);

}

for the Exception specific to "inject" the translator and translate the needed translations

Didn't we talk about this during the meet and @romaninsh specifically asked to not translate this property at all (which I agree with completely), but instead translate it when rendering (so that'd be in getColorfulText(), getHTMLText(), getHTML() and getJSON()?

didn't understand

but to do that we have to pass it like this? If you understand it better, can be better this way?

    public function getHTML(?ITranslatorAdapter $adapter=null)
    {
        return (string) new HTML($this, $adapter);
    }

pkly · 2019-10-07T09:00:00Z

didn't understand

but to do that we have to pass it like this? If you understand it better, can be better this way?
    public function getHTML(?ITranslatorAdapter $adapter=null)
    {
        return (string) new HTML($this, $adapter);
    }

That, or you could just add a function like

public function setTranslationAdapter(?ITranslationAdapter $adapter = null) {
    $this->adapter = $adapter;
}

// and then

public function getHTML(): HTML {
    return (string) new HTML($this, $this->adapter);
}

Although just adding another function like

public function getTranslationAdapter(): ITranslationAdapter {
    return $this->adapter ?? Translation::instance();
}

would also remove the need to pass the adapters to getHTML() at all, since then in the HTML object you could just do $m = $exception->getTranslationAdapter()->_($exception->getMessage());
which is the most optimal solution.

Also please use more of the ?? operator, it makes it way easier to read code and atk4 is supposed to run on 7.1+ anyway where it's supported.

…okalise/wip-integration

abbadon1334 · 2019-10-07T11:22:25Z

didn't understand
but to do that we have to pass it like this? If you understand it better, can be better this way?
    public function getHTML(?ITranslatorAdapter $adapter=null)
    {
        return (string) new HTML($this, $adapter);
    }

That, or you could just add a function like

public function setTranslationAdapter(?ITranslationAdapter $adapter = null) {
    $this->adapter = $adapter;
}

// and then

public function getHTML(): HTML {
    return (string) new HTML($this, $this->adapter);
}

Although just adding another function like

public function getTranslationAdapter(): ITranslationAdapter {
    return $this->adapter ?? Translation::instance();
}

there is no need to have getAdapter, because or is set (already configured) from outside or is Generic in Translator and is managed by this, adapter act as a repository nearly passive, the active part is Translator.

Also please use more of the ?? operator, it makes it way easier to read code and atk4 is supposed to run on 7.1+ anyway where it's supported.

I understand, but in many cases i prefer early exit to leave space for more conditions to exit before return.

abbadon1334 · 2019-10-07T11:31:39Z

Composer error
i recreate the composer error locally :
i delete vendor + composer.lock
composer clear-cache
composer install --no-ansi
error happens
if i add to composer :

    "replace": {
        "atk4/core": "self.version"
    }

no more happens locally, but in travis still happens, anyone ideas?

locally works

This reverts commit 3b62e23

abbadon1334 · 2019-10-07T13:06:35Z

If someone can do something on this "composer issue".

IMHO the best option to avoid at all this problem is to remove atk4/data dependency and move in atk4/core all the locales, in a directory structure like :

locales
- ui
  - en
  - it
  - ru
  - fr
- data
  - en
  - it
  - ru
  - fr

Core is a base dependency.

pkly · 2019-10-08T11:24:35Z

If someone can do something on this "composer issue".

IMHO the best option to avoid at all this problem is to remove atk4/data dependency and move in atk4/core all the locales, in a directory structure like :
[...]
Core is a base dependency.

I'd go with /language/namespace.json [or other] but yeah that's fine with me, I prefer that to the alternatives honestly.

abbadon1334 · 2019-10-08T11:33:17Z

If someone can do something on this "composer issue".
IMHO the best option to avoid at all this problem is to remove atk4/data dependency and move in atk4/core all the locales, in a directory structure like :

locales

ui

en

it

ru

fr

data

en

it

ru

fr

Core is a base dependency.

I'd go with /language/namespace.json [or other] but yeah that's fine with me, I prefer that to the alternatives honestly.

i agree with you but i speak strictly for atk translations, the actually hardcoded, did you agree on the rest of modifications done after your comment?

To add custom translation with specific namespace (domain) can be used this 2 methods in generic :

core/src/Translator/Adapter/Generic.php

Lines 115 to 154 in a7906aa

    
               public function addDefinitionFromFile(string $file, string $locale, string $domain, string $format): void 
        
               { 
        
                   $this->loadDefinitionATK($locale); // need to be called before manual add 
        
                   $this->readConfig($file, $format); 
        
                   $this->definitions = array_replace_recursive( 
        
                       $this->definitions, 
        
                       [ 
        
                           $locale => [ 
        
                               $domain => $this->config, 
        
                           ], 
        
                       ] 
        
                   ); 
        
                   $this->config = []; 
        
               } 
        
               /** 
        
                * Set or Replace a single definition within a domain. 
        
                * 
        
                * @param string       $key 
        
                * @param string|array $definition 
        
                * @param string       $locale 
        
                * @param string       $domain 
        
                * 
        
                * @return $this 
        
                */ 
        
               public function setDefinitionSingle(string $key, $definition, string $locale = 'en', string $domain = 'atk') 
        
               { 
        
                   $this->loadDefinitionATK($locale); // need to be called before manual add 
        
                   if (is_string($definition)) { 
        
                       $definition = [$definition]; 
        
                   } 
        
                   $this->definitions[$locale][$domain][$key] = $definition; 
        
                   return $this; 
        
               }

What do you think there is everything in place?

pkly · 2019-10-08T11:57:20Z

src/Exception.php

-        $output .= "\n\033[1;31m-------------------------------------------------------\n";
-
-        return $output."\033[0m";
+        return (string) new Console($this, $this->adapter);


I'm still wondering if it wouldn't be better to simply add a getAdapter() method to Exception instead of passing this argument 4 times. Unsure.

is a specific renderer that needs 2 args to do his job and is internal, i think is ok

pkly · 2019-10-08T11:58:53Z

src/ExceptionRenderer/RendererAbstract.php

+
+    public function __construct($exception, ?ITranslatorAdapter $adapter = null)
+    {
+        $this->adapter = $adapter;


Like I said above, maybe don't bother with this?

pkly · 2019-10-08T11:59:55Z

src/ExceptionRenderer/RendererAbstract.php

+            return $this->output;
+        } catch (\Throwable $e) {
+            // fallback if Exception occur in renderer
+            return get_class($this->exception).' ['.$this->exception->getCode().'] Error:'.$this->_($this->exception->getMessage());


Maybe translate the 'Error:' string?

maybe, but probably using a key for common words like this, i suggest exception_string_error

pkly · 2019-10-08T12:01:06Z

src/ExceptionRenderer/RendererAbstract.php

+        if (is_object($val) && !$val instanceof \Closure) {
+            return isset($val->_trackableTrait)
+                ? get_class($val).' ('.$val->name.')'
+                : 'Object '.get_class($val);


Maybe translate the 'Object' string here?

maybe, but probably using a key for common words like this, i suggest exception_string_object

pkly · 2019-10-08T12:07:33Z

src/ExceptionRenderer/RendererAbstract.php

+    public function _($message, array $parameters = [], ?string $domain = null, ?string $locale = null): string
+    {
+        return $this->adapter
+            ? $this->adapter->_($message, $parameters, $domain, $locale)
+            : Translator::instance()->_($message, $parameters, $domain, $locale);
+    }
+}


If a getAdapter or getTranslationAdapter method would be added to the exception class this could be basically removed.
Mock implementation in the Exception class:

public function getTranslationAdapter(): ITranslatorAdapter { return $this->adapter ?? Translator::instance(); }

which could be later used here like so:

if ($this->exception instanceof \atk4\core\Exception) // Maybe make it into an interface instead for use in exceptions, or a trait return $this->exception->getTranslationAdapter()->_($message, $parameters, $domain, $locale); // We don't wanna translate other exceptions return $message;

Translator::instance() is not an ITranslatorAdapter, it can be because is conform to Interface but is not an ITranslatorAdapter, because is inteded to be used with the setted ITranslatorAdapter

Why added something that is intended to stay there forever after set?

Translator -> setAdapter ->_($translate)
or
new Adapter -> configure adapter...
Translator -> set adapter (already configured) -> _($translate)

Probably i'm too defensive, but IMHO if you put getTranslator you leave too much space for possible strange and wrong usage of the class

pkly · 2019-10-08T12:33:55Z

src/Translator/Adapter/Generic.php

+            $path = Locale::getPath();
+            $this->addDefinitionFromFile($path.$locale.'/atk.php', $locale, 'atk', 'php-inline');


Probably shouldn't delegate this to atk4\data

Agree even more because to test it, i encounter circular dependency in composer which i think it will knock to our door in the future

abbadon1334 and others added 7 commits September 28, 2019 21:14

add atk4\data Lokalise dev branch

affd513

create bridge class Translator as a singleton

1a24f3e

simplify TranslatableTrait

40da4c4

Create TranslatorAdapter with create a Generic one

1763a78

add call to translator in Exception

f0dcfda

cleanup and add Test

32891d5

Apply fixes from StyleCI

c2652ce

abbadon1334 requested review from romaninsh, acicovic and PhilippGrashoff September 28, 2019 18:38

abbadon1334 added the Hangout Agenda label Oct 1, 2019

pkly reviewed Oct 1, 2019

View reviewed changes

abbadon1334 commented Oct 1, 2019

View reviewed changes

romaninsh requested changes Oct 3, 2019

View reviewed changes

abbadon1334 added 8 commits October 5, 2019 17:31

add Test for Localization

dbab0c7

fix memory leaks in case of AppScope is present

a303c6c

add php-cs-fixer config to help StyleCI

c616191

move out of foreach array_merge_recursive

db6ae5d

little refactor ExceptionRenderer

e7b9fca

add check in $this->app before go to singleton

2c7c810

small changes

5e2d934

improve coverage

1a869ca

abbadon1334 added 5 commits October 7, 2019 09:07

added @dev to ignore stability of atk4/data

5319e15

composer require-dev

try solving issue : circular dependency composer

f8cdf1c

removed composer diagnose

7827a9b

revert composer.json

f1841f7

fix travis duplicate urls (not related to build error)

bbedcce

abbadon1334 and others added 5 commits October 7, 2019 14:03

Refactor

e45421f

Fix dependency circular (locally does)

5b92b9a

Apply fixes from StyleCI

7387f70

Fix Unit Test & example

089be4a

Merge remote-tracking branch 'origin/lokalise/wip-integration' into l…

18dc7ef

…okalise/wip-integration

abbadon1334 added 2 commits October 7, 2019 15:44

Fix composer.json - circular dependency

3b62e23

locally works

Revert "Fix composer.json - circular dependency"

a7906aa

This reverts commit 3b62e23

abbadon1334 requested review from DarkSide666 and romaninsh October 7, 2019 13:10

pkly reviewed Oct 8, 2019

View reviewed changes

Merge branch 'develop' into lokalise/wip-integration

a893180

romaninsh merged commit 627972d into develop Oct 17, 2019

romaninsh deleted the lokalise/wip-integration branch October 17, 2019 17:25

romaninsh restored the lokalise/wip-integration branch October 6, 2020 09:10

romaninsh deleted the lokalise/wip-integration branch October 6, 2020 09:11

		$path = Locale::getPath();
		$this->addDefinitionFromFile($path.$locale.'/atk.php', $locale, 'atk', 'php-inline');

WIP : Lokalise integration - test on Exception #110

WIP : Lokalise integration - test on Exception #110

Conversation

abbadon1334 commented Sep 28, 2019

acicovic commented Sep 30, 2019

pkly Oct 1, 2019 • edited Loading

Choose a reason for hiding this comment

abbadon1334 left a comment

Choose a reason for hiding this comment

pkly commented Oct 1, 2019

PhilippGrashoff commented Oct 1, 2019

abbadon1334 commented Oct 1, 2019

pkly commented Oct 1, 2019

pkly commented Oct 3, 2019

abbadon1334 commented Oct 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkly commented Oct 3, 2019

abbadon1334 commented Oct 7, 2019

codecov bot commented Oct 7, 2019 • edited Loading

Codecov Report

pkly commented Oct 7, 2019

abbadon1334 commented Oct 7, 2019

pkly commented Oct 7, 2019

abbadon1334 commented Oct 7, 2019

abbadon1334 commented Oct 7, 2019

abbadon1334 commented Oct 7, 2019

pkly commented Oct 8, 2019 • edited Loading

abbadon1334 commented Oct 8, 2019

pkly Oct 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkly Oct 1, 2019 •

edited

Loading

codecov bot commented Oct 7, 2019 •

edited

Loading

pkly commented Oct 8, 2019 •

edited

Loading

pkly Oct 8, 2019 •

edited

Loading