When working on a multi lingual page, the correct translated Terms will not get pulled. Neither the term names.

Comments

tassilogroeper created an issue. See original summary.

tassilogroeper’s picture

Assigned: Unassigned » tassilogroeper
StatusFileSize
new3.02 KB

Please test with non multilingual environment

tassilogroeper’s picture

Assigned: tassilogroeper » Unassigned
Status: Active » Needs review
tassilogroeper’s picture

btw. the first bock /onomasticon/src/Plugin/Filter/FilterOnomasticon.php:213-242 can be refactored, since the defaults for $this->settings are always set. see #2831423: Refactoring defaults

tassilogroeper’s picture

StatusFileSize
new3.04 KB

added early return if there is no translation for the current term. No false language will appear in the tooltips. sorry no interdiff.

broon’s picture

+    $terms = \Drupal::entityTypeManager()
+      ->getStorage('taxonomy_term')
+      ->loadTree($this->settings['onomasticon_vocabulary'], 0 , NULL, true);

I decided to not load the full entities on purpose. Imagine having a quite large glossary vocabulary, this will result in high memory consumption. So, I tried to fetch a full term entity only if the term name is found in text. In fact, on my test page, I ran out of memory despite having increased the limit to 1024M. And there is only a simple set of terms.
I am aware that ->has/getTranslation() won't work with the partial term objects, though.

+      if (!$term->hasTranslation($language)) {
+        continue;
+      }

This would potentially disable all terms that have no language set.

broon’s picture

Status: Needs review » Needs work
tassilogroeper’s picture

Status: Needs work » Needs review
StatusFileSize
new3.51 KB

patch #5 won't apply, thus forget about it.

in this new patch I tested with a multilingual and a non multilingual environment. Unfortunately, I found no other way to enable translation than actually loading the hole Term - instead of a stdClass as right now.

I also found a memory leek:
Since the FilterOnomasticon::process() will get called multiple times, there is a limitation to use a private singleton with the fetched Terms. But if you "cache" at least the recursive calls of FilterOnomasticon::processChildren(), there is a better memory usage now. In one environment I counted 20 calls without and only 5 with the singleton.

btw. the $this->termCache would only need the key to be set. no value needed here.

Since I did not said it before: very good progress! Really like this module. Thank you for the nice work!

broon’s picture

$this->termCache gets values because in the latest dev version the user can choose whether to only replace the first occurrence of a term (default, indeed no value needed) or to replace all occurrences (in this case the value is taken from termCache and not loaded again from database.

Regarding the translation, I was looking into that as well and neither found any way to just get the available translations without fetching the complete Term objects. We'll need some smoother way to handle this. I just started Drupal 8 a few weeks ago after almost eight years of procedural D6 and D7 and still need to dig through the possibilities of the new system. Also, last time I used Symfony in 2009, it was in major version 1, so I have a pretty steep learning curve right now.

That said, thanks for your kind words and also thank you for the patches provided and all the input. I really appreciate it and it helps me to find my way through the D8 jungle.

broon’s picture

Just had a quick look at the new patch. Since you created the new $terms class variable, my remark about termCache is of course invalid and you are right, now it doesn't need the values.

I am unlikely to test it all until Monday due to some deadlines I have to meet, but I will try to do it as soon as possibly.

broon’s picture

Status: Needs review » Fixed

Couldn't sleep well, so I actually did test it. Very good job, it seems to work quite well for Latin-based alphabets. I was tinkering around the last hours trying to get it work for Russian, but failed so far. But in general, the pure translation stuff works.

tassilogroeper’s picture

@paul you are welcome :-)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.