dev/translation#67 - Define "Translation" table. Add during installation/upgrade. #20478
Conversation
…tion/upgrade.
This creates an entity, `Translation` (`civicrm_translation`), to represent a single translated database value. Loosely speaking,
any field in the database can be designated as translatable -- and then it will be permitted to store values like:
```sql
INSERT INTO civicrm_translation (entity_table, entity_id, entity_field, language, string)
VALUES ('civicrm_event', 100, 'title', 'fr_FR', 'La nouvelle chaine')
```
This is based on a `civi-data-translate` strings table, but with some changes:
* Entity names are usually singular, but `String` is conflicted. I previously used hybrid
String/Strings (depending on context), but we negotiated `Translation` on tcon.
* The language only needs 5 characters (NN_nn).
* Consolidated `bool is_active` and `bool is_default` into one `int status_id`.
* Added indexing
* Mark dynamic foreign key
This commit includes the BAO with some of the backing-methods required for
API exposure. However, the API won't really work until we have the
validation-values event, so the API has been kicked to a subsequent PR.
The list of translatable entities/fields will be signficant because it will
determine when/how to redirect data in API calls. This patch does not
commit to specific translatable fields - but it does provide a hook to
determine them.
When the API PR becomes unblocked, it will include test-coverage that hits the API, BAO, and hook.
|
(Standard links)
|
totten
changed the title
|
CRM_Core_DAOConformanceTest::testFieldsHaveTitles with data set #9 ('CRM_Core_DAO_Translation') |
|
@eileenmcnaughton Yup, updated. |
| <!-- Prediction: In a large DB with many events/contribution-pages/groups/mailings/etc, entity ID will have best selectivity. --> | ||
| <!-- Prediction: Over diverse set of deployments, the selectivity of 'table' and 'language' will be similar. --> | ||
| <fieldName>entity_id</fieldName> | ||
| <fieldName>entity_table</fieldName> |
There was a problem hiding this comment.
@totten note that in a combined index the second 2 parts of the index will never be hit if the first is not - ie
SELECT * FROM civicrm_translation WHERE id = 5 AND entity_table = 'civicrm_discount' AND language = 'en_US'
will utilise the full index.
If the query is
SELECT * FROM civicrm_translation WHERE entity_table = 'civicrm_discount' AND language = 'en_US'
then the index will not be used at all.
For this query
SELECT * FROM civicrm_translation WHERE id = 5 AND AND language = 'en_US'
the index on id is used (yay) but not on language
I'm actually fine merging as is - I suspect the entity_id is what we really need but it might be over time that we add other other indexes on the other 2 fields so we can retrieve all by language or all by entity.
There was a problem hiding this comment.
I'm actually fine merging as is - I suspect the entity_id is what we really need but it might be over time that we add other other indexes...
Yeah, this is my theory too - at least for the current cases where you start by filtering the main entity (MessageTemplate, Event, etc) and then (as a final step) merge-in the translations for the output.
When/if we have a case for textual filtering on translated columns, then we'll need to consider other indexes...
CRM/Utils/Hook.php
Outdated
| * Ex: $fields['civicrm_event']['summary'] = TRUE | ||
| * Ex: $fields['civicrm_event']['summary'] = 'yesplease'; | ||
| * | ||
| * At time of writing, the `$fields` list x is prepopulated based on `<localizable>` fields in core's `xml/schema`. |
There was a problem hiding this comment.
@totten I thought we decided we would not use existing localizable as that would lead to both systems trying to do the same field (we could add 'translatable' with 1 being ignored if both is set)
There was a problem hiding this comment.
Correct. I changed the logic, but I'll update the stale comment.
CRM/Utils/Hook.php
Outdated
| * @param array $fields | ||
| * List of data fields to translate, organized by table and column. | ||
| * Omitted/unlisted fields are not translated. Any listed field may be translated. | ||
| * Values should be TRUE-ish. |
There was a problem hiding this comment.
I find this confusing - the value should be TRUE - but you are WARNING that setting a different value might resolve to true - it comes across as you saying 'there is no recommendation - do what you want' and also is a lot more cognitive load than just daying 'the value should be boolean' and expecting people to have enough sense/coding skills to realise that they should set a boolean or something weird might happen
There was a problem hiding this comment.
It was because the original list of translatable fields (SchemaStructure) had TRUE-ish things rather than strict booleans. But since we're not feeding off that now, we can document it as straight-up boolean.
The previous comments reflected a developmental iteration.
|
@eileenmcnaughton Pushed clarifications on the hook docblock. |
| * At time of writing, the `$fields` list x is prepopulated based on `<localizable>` fields in core's `xml/schema`. | ||
| * In the future, it may also be prepopulated with `<localizable>` fields in ext's `xml/schema`. | ||
| * For the interim, you may wish to fill-in `<localizable>` fields from ext's. | ||
| * @return mixed |
There was a problem hiding this comment.
In other places we have * @return null
- the return value is ignored
Do we ignore this one?
There was a problem hiding this comment.
@eileenmcnaughton Yes, the return value is ignored here.
Of course, both @return mixed and @return null feel misleading to me (for the ~95% of hooks which don't really return anything). To my skimming, the @return mixed looks more consistent with the style of surrounding code. (~77x mixed vs ~35x null).
If we wanted it to make sense, then I suppose those ~110 regular (non-returning) hook-stubs should actually be made truly void...
|
OK - just noting that this is as discussed in the gitlab & the usage will follow to get the schema changes merged in a non-staley way. The main difference is the change of table name (the original, strings, seemed risky in terms of having a special meaning in php) |
Overview
This creates a table/entity,
Translation(civicrm_translation), to represent a single translated string (e.g. a translated message-title or a translated event-description). Loosely speaking, any field in the database can be designated as translatable -- and then it will be permitted to store values like:(See also: https://lab.civicrm.org/dev/translation/-/issues/67)
ping @eileenmcnaughton
After
The SQL table, DAO, and BAO are defined.
Comments
This is based on a
civi-data-translatestrings table, but with some changes:Stringis conflicted. This use "Translation" which fits in a bit better grammatically. (I previously used hybrid String/Strings depending on context, but we negotiatedTranslationon tcon.)bool is_activeandbool is_defaultinto oneint status_id.hook_civicrm_translatedFields.This commit includes the BAO with some of the backing-methods required for API exposure. However, the API won't really work until we have the validation-values event, so the API has been kicked to a subsequent PR.
The list of translatable entities/fields will be signficant because it will determine when/how to redirect data in API calls. This patch does not commit to specific translatable fields - but it does provide a hook to determine them.
When the API PR becomes unblocked, it will include test-coverage that hits the API, BAO, and hook.