[samuelsov]

Overview

Internationalization support for FormBuilder.

Before

Translation is working if the translation is provided in core using .mo files but not for custom text.

After

Translation is possible using the civicrm_translation table and a new civicrm_translation_source table to store what could be translated.

To test:

• enable Multilingual mode + disable Asset Caching + enable extension kamlanguage, i.e. Language switcher (not required but easier)
• create or edit a form builder and add custom strings as label, placeholder....
• upon saving the form, the table civicrm_translation_source is populated with the new strings
• to translate, go to the menu Administer -> Localisation -> Translations to XXX

- then visit the form builder page and switch the language (using the kamlanguage menu) - Result: the form should be translated

Technical Details

Instead of adding ts to .aff.html file, which causes issues in the GUI, we now extract any strings from the .aff.html files that should be localizable (title, button, text...) and put them in a new table civicrm_translation_source.

When saving an afform in the GUI, the extractor is called to save the new strings as data in civicrm_translation_source.

Using a specialized SearchKit based on a SQL view, we are able to do inline edit translations of string data in the civicrm_translation table. It works even if there are no translations yet in the civicrm_translation table.

The ts function will now look in the civicrm_translation first and then fallback to the regular transifex mo files. This allows one to provide a default for packaged afform and yet still be able to change them.

Comments

Discussed but excluded from scope (and will lead to future PRs):

• the title of the afform is not yet translated
• the asset cache needs to be manually cleaned-up after adding some translation (or disabled)
• while the database supports context (i.e. which afform/field the translation applies to), it is not yet taken into account which means that the same source string will be translated the same across all afform
• some database changes due to the use of civicrm_translation impact current Message translation intended to support translations of strings from other entities
• the existing word replacement feature is very similar and could/should probably use this new table
• remove all ts from existing afform - some needs extra change in the tags or angular

[civibot]

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...

• If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
• A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
• A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
• If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
• Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
• A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
• To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
• For more information about contributing, see CONTRIBUTING.md.

Quick links for reviewers...

• ➡️ PR Demo site (can be rebuilt with the command test this please).
• 📖 Review standards
• 🗒️ Review template (brief or verbose)

➡️ Online demo of this PR 🔗

[colemanw]

@samuelsov @shaneonabike after some expermentation I found that a specialized api4 entity/view was not necessary. I fixed a couple bugs in SK editable and was able to get the job done by autogenerating 1 search/displayafform per language. You'll find them in the Localization menu :)

[colemanw]

@samuelsov I just tried running the upgrader and got a few odd results

1. A couple source translations got saved with nothing but an empty string.
2. Some markup was stipped from source strings. I'm not sure if that's ok but probably not. Example:

civicrm-core/ang/afform/afformSiteToken.aff.html

Lines 6 to 7 in 5d7ffd1

	<p><strong>Key</strong><br />
	The <em>Key</em>&nbsp;field is not editable because this token is reserved.&nbsp;</p>

Gets stored as

Key The Key field is not editable because this token is reserved.

[shaneonabike]

Yeah I noticed those as well @colemanw. I fixed a bunch of that on the plane home but I just have to merge it with your new things! I also caught te missing placeholders too. Thanks for your help!

[colemanw]

@shaneonabike sounds good. Since I already rebased this branch, the thing to do would be to checkout a new branch locally, delete your local afformi18n, fetch the samelsov remote, and then checkout afformi18n again. Then you can cherry-pick your new commits and push. As long as you don't do git merge or git push --force it should work fine.

[colemanw]

@samuelsov @shaneonabike the test fail is:

In AfformSaveTrait.php line 107:
                                                                               
  [ArgumentCountError]                                                         
  Too few arguments to function CRM_Afform_Upgrader::saveTranslations(), 1 pa  
  ssed in /home/homer/buildkit/build/build-1/web/sites/all/modules/civicrm/ex  
  t/afform/core/CRM/Afform/Upgrader.php on line 115 and exactly 2 expected     
                                                                               

Exception trace:
  at /home/homer/buildkit/build/build-1/web/sites/all/modules/civicrm/ext/afform/core/Civi/Api4/Utils/AfformSaveTrait.php:107
 CRM_Afform_Upgrader->saveTranslations() at /home/homer/buildkit/build/build-1/web/sites/all/modules/civicrm/ext/afform/core/CRM/Afform/Upgrader.php:115
 CRM_Afform_Upgrader->upgrade_1004()

[shaneonabike]

My bad I changed something in the definition to catch the title of the Form which is not being translated. I still need to determine how to translate the titles but they are being recorded properly.

We have the following issues still to deal with

• Translate titles on the fly
• Language specific caching (I know the caching was changed but I think we might need to add the language to the cache_key for multilingual sites or else chances are the caching will cache one language until it is reset -- seen this before)

[colemanw]

@samuelsov @shaneonabike style errors are here: https://test.civicrm.org/job/CiviCRM-Style/6325/console

[totten]

@samuelsov @shaneonabike @colemanw - I wanted to give a whirl and felt it would be easier on my brain with a unit-test. So I've pushed up a unit-test. It could probably use more cases (to check on attributes and defns).

In the process, I noticed a bug where (eg) <p class="af-text">Hello</p> became was rendered as bare-text Hello (without the containing <p class="af-text">). So I included a fix for that.

However, there is still one test-case that's failing on my local (basic-translation), and I couldn't immediately see why.

[samuelsov]

@totten ok, great! Shane and I fixed a few issues yesterday. I'm working on merging them with the change you made.

[samuelsov]

I need to do a few more tests but it's seems pretty good to me. The only missing translation on a afform is the title of the form. It's populated in the civicrm_translation_source but we miss the part to translate it when loading the afform.

[totten]

Suggestion: It seems to me that these two chunks are largely the same:

• civicrm-core/ext/afform/core/Civi/Afform/Translator.php

  Lines 41 to 79 in 7c1ef39

  	$contentSelectors = \CRM_Utils_JS::getContentSelectors();
  	$contentSelectors = implode(',', $contentSelectors);
  	$doc->find($contentSelectors)->each(
  	function (\DOMElement $item) use ($contentSelectors) {
  	$pqItem = pq($item);
  	$markup = $pqItem->html();
  	$markup = trim($markup);
  	if (!empty($markup)) {
  	$translated = html_entity_decode(_ts(htmlentities($markup)));
  	if ($markup !== $translated) {
  	$pqItem->html($translated);
  	}
  	}
  	}
  	);
  	// Translate Attributes.
  	$attributeSelectors = \CRM_Utils_JS::getAttributeSelectors();
  	foreach ($attributeSelectors as $attribute) {
  	$doc->find('[' . $attribute . ']')->each(
  	function (\DOMElement $item) use ($attribute) {
  	$this->translateAttribute($item, $attribute);
  	}
  	);
  	}
  	// Translate Defn values.
  	$defnSelectors = \CRM_Utils_JS::getDefnSelectors();
  	$doc->find('af-field[defn]')->each(
  	function (\DOMElement $item) use ($defnSelectors) {
  	$defn = \CRM_Utils_JS::decode($item->getAttribute('defn'), 1);
  	foreach ($defnSelectors as $selector) {
  	$this->defnLookupTranslate($defn, $selector);
  	}
  	$item->setAttribute('defn', \CRM_Utils_JS::encode($defn));
  	}
  	);
  	}
  	);

• civicrm-core/ext/afform/core/Civi/Afform/StringScanner.php

  Lines 33 to 60 in 7c1ef39

  	$contentSelectors = \CRM_Utils_JS::getContentSelectors();
  	$contentSelectors = implode(',', $contentSelectors);
  	$doc->find($contentSelectors)->each(function (\DOMElement $item) {
  	// FIXME: Should this be: $this->scanString(pq($item)->html());
  	$markup = '';
  	foreach ($item->childNodes as $child) {
  	$markup .= $child->ownerDocument->saveXML($child);
  	}
  	$this->scanString($markup);
  	});
  	// Find attributes to be translated.
  	$attributes = \CRM_Utils_JS::getAttributeSelectors();
  	foreach ($attributes as $attribute) {
  	$doc->find('[' . $attribute . ']')->each(function (\DOMElement $item) use ($attribute) {
  	$this->scanString($item->getAttribute($attribute));
  	});
  	}
  	// Get sub-attributes to be translated.
  	$defnSelectors = \CRM_Utils_JS::getDefnSelectors();
  	$doc->find('af-field[defn]')->each(function (\DOMElement $item) use ($defnSelectors, $inputSelectors) {
  	$defn = \CRM_Utils_JS::decode($item->getAttribute('defn'));
  	// Check Defn Selectors.
  	foreach ($defnSelectors as $selector) {
  	$this->defnLookupTranslate($defn, $selector);
  	}
  	});

In both cases, they are addressing the same complex task: take different kinds of selectors (tag-selectors, attribute-selectors, and defn-selectors) and visit every match. They differ in what they do with the match.

One can see how they might've started in different places... but I think there's a way to improve. They could share the same code for walking/visiting each of the strings. For example, suppose you had StringVisitor:

/** Update $doc with translations */
function translateDoc(phpQueryObject $doc): void {
  $v = new StringVisitor();
  $v->visit($doc, fn($string) => _ts($string));
}

/** Search $doc and return list of strings */
function extractStrings(phpQueryObject $doc): array {
  $allStrings = [];
  $v = new StringVisitor();
  $v->visit($doc, function(string $string) use (&$allStrings) {
    $allStrings[] = $string;
    return $string;
  });
  return $allStrings;
}

class StringVisitor {
  /**
   * Explore an Afform-HTML document. Run a callback on every translateable string.
   *
   * @param 
  public function visit(phpQueryObject $doc, $callback): void {
    // Go through all the selectors.
    // Fire $callback on each.
    // Check if $callback has provided a new value.
  }
}

The upshot of this would be keeping the two operations closely aligned. For example...

• Example 1: Line-break normalization
  • On the input side, translateable strings might include <br> and/or <br/>.
  • Will the canonical source-string include the trailing /?
  • Right now, both generate the "inner HTML", but they call different helpers:
    • pq($item)->html() versus...
    • foreach ... childNodes ... saveXML()...
  • Consequently, both identify a canonical source-string with a well-formed line-break. But they disagree with each other:
    • One identifies the source-string with <br>
    • The other uses <br/>.
• Example 2: Skipping {{..}} expressions
  • In one case, it excludes any strings that contain {{ from translation.
  • In the other case, it still processes them for translation.

If translation and scanning both used the same utility for visiting strings, then these kinds of discrepancies would become impossible.

[shaneonabike]

@totten I was thinking the same. Originally, I had setup a callback when the result was discovered, but it got complicated so we abandoned it and didn't mention that it needed work.

If you suggest where I could create a new class to do this exploring I'm all ears? We would need this to be available for Searchkit in the future also b/c that would be the next iteration?

[samuelsov]

After testing a bit more, there is 2 problems that I'm working on:

• in defn, empty options and input_attrs gets added if they are not there which breaks the display
• html content is not always translated because some mismatch (accentued character are escaped in Translator but not in StringScanner)

[totten]

(@shaneonabike) If you suggest where I could create a new class to do this exploring I'm all ears? We would need this to be available for Searchkit in the future also b/c that would be the next iteration?

I'm not sure I see the difficulty, so I took a whirl and produced samuelsov@29be919

(Before trying that, I wanted some more test-coverage. So full proposal is https://github.com/samuelsov/civicrm-core/compare/afformi18n...totten:civicrm-core:afformi18n-consolidate?expand=1. The other big thing in that branch is splitting the TranslatorTest into separate files. This makes it easier to copy-in more complex examples.)

(EDIT) Searchkit will probably need a similar scanner/filter/visitor pattern, but I think it will not be literally the same. For afform, you're using CSS-like selectors on HTML data. For SK, you're using different selector applied to JSON data. Similar... but different enough that they don't need to go through the exact same files.

(@samuelsov) html content is not always translated because some mismatch (accentued character are escaped in Translator but not in StringScanner)

Ah, yeah. I noticed something similar while taking a stab at consolidating the classes. My feeling is that Translator is compensating for a bug that's somewhere else. If that bug is fixed, then Translator and StringScanner become more similar (as in the proposed StringVisitor.) samuelsov@29be919

To see what I mean...

• Forget Afform for a moment. Just think about the APIs (e.g. Translation::create() and TranslationSource::create()) and SQL tables (civicrm_translation, civicrm_translation_source).
• Any time you write to these tables via API, it goes through HTMLInputCoder:
  • If you say setValues(['string' => 'Hello<br/>'])...
  • Then SQL stores... string='Hello<br/>'.
• We normally don't notice because API's have automatic-decoding.
• But for ts() integration, it uses an optimized read SQL (CRM_Core_BAO_TranslationSource::getTranslationSources()) ==> no automatic-decoding.
• If we teach getTranslationSources() to apply the decoding, then it should help handling of all ts() calls involving data from Translation/TranslationSource (including, but not limited to, afform).

[totten]

@samuelsov I suspect there was a contemporaneous PR that touched the upgrader-class. Test failures are indicating a rebase would be appropriate.

[totten]

OK, the newer failure appears in the same test-class... but is actually a different thing. (It's an interaction with 4f1c71f.) For this, the test needs an update. But it's gonna be a lot easier to do that with a rebase....

[colemanw]

Congratulations @samuelsov @shaneonabike! Great accomplishment getting this finished 🎉🍾

[shaneonabike]

Thanks but you gotta open some champagne for yourself too! I appreciate that help at the sprint from both @colemanw @totten and others who helped us brainstorm, and add certain elements. @samuelsov and I are already generating a larger list of work that will need to be tackled to fully integrate this 🧨