Problem/Motivation

Currently there is no way to process field data before is stored into a translation job data property and before it is saved back as a translation, regardless from source or translator plugins. For example, some remote services might have problem in "understanding" Drupal tokens, hence it would be handy to have a way to encode and decode tokens when data is stored in a translation job and applied on an entity.

Proposed solution

In order to allow for inbound/outbound data processing it might be enough to provide an alter hook in the following functions:

  • Before creating a translation job: tmgmt_field_get_source_data(...)
  • Before applying translations to an entity: tmgmt_field_populate_entity(...)

Remaining tasks

- Propose patch with two alter hooks as described above.

CommentFileSizeAuthor
#14 interdiff.txt1.57 KBademarco
#14 tmgmt-alter-field-values-2614100-14.patch2.9 KBademarco
#13 tmgmt-alter-field-values-2614100-13.patch2.9 KBademarco
#13 interdiff.txt498 bytesademarco
#11 interdiff.txt2.84 KBademarco
#11 tmgmt-alter-field-values-2614100-11.patch2.9 KBademarco
#5 tmgmt-alter-field-values-2614100-5.patch2.27 KBademarco
#2 tmgmt-alter-field-values-2614100-1.patch2.27 KBademarco

Comments

ademarco created an issue. See original summary.

ademarco’s picture

ademarco commented
Issue summary: View changes
StatusFileSize
new tmgmt-alter-field-values-2614100-1.patch2.27 KB
ademarco’s picture

ademarco commented
Status: Active » Needs review
ademarco’s picture

ademarco commented
Status: Needs review » Needs work

Badly rolled-out patch, I'll re-post!

ademarco’s picture

ademarco commented
Status: Needs work » Needs review
StatusFileSize
new tmgmt-alter-field-values-2614100-5.patch2.27 KB
berdir’s picture

berdir
Primary language German
Location Switzerland
commented

OK with adding such a hook in general. We did implement an escape API at some point, but it's not widely used. See the locale source for an example. 

  1. +++ b/sources/field/tmgmt_field.module
@@ -75,6 +75,8 @@ function tmgmt_field_get_source_data($entity_type, $entity, $langcode, $only_tra
+
+  drupal_alter('tmgmt_field_get_source_data', $data, $entity_type, $entity, $langcode);
   return $fields;

    I think the hook name would make more sense without the get, just tmgmt_field_source_data? 

  2. +++ b/sources/field/tmgmt_field.module
@@ -93,6 +95,8 @@ function tmgmt_field_get_source_data($entity_type, $entity, $langcode, $only_tra
 function tmgmt_field_populate_entity($entity_type, $entity, $langcode, $data, $use_field_translation = TRUE) {
+  drupal_alter('tmgmt_field_populate_entity', $data, $entity_type, $entity, $langcode);

    Wondering if it wouldn't make more sense to alter the entity afterwards? maybe both?

ademarco’s picture

ademarco commented

Thanks for the quick feedback! I'd propose the following hooks then:

function hook_tmgmt_field_source_data_alter(&$data, $entity_type, $entity, $langcode) { }

function hook_tmgmt_field_pre_populate_entity_alter(&$data, $entity_type, $entity, $langcode) { }

function hook_tmgmt_field_post_populate_entity_alter(&$data, $entity_type, $entity, $langcode) { }

If ok I can re-roll the patch.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

First, something that I just forgot. drupal_alter() only supports 3 arguments.. the main thing to be altered and two contexts. Given that, I would recommend that you put all three additional arguments into a single $context array.

Second, I think for post we should switch the $data and $entity aguments then. It makes no sense to alter $data in post_populate as it has no effect. Instead, you want to alter the entity. And the thing that should be altered is always the first in alter hooks.

ademarco’s picture

ademarco commented

Actually drupal_alter() supports 4 arguments:

function drupal_alter($type, &$data, &$context1 = NULL, &$context2 = NULL, &$context3 = NULL)

Although documentation says:

 * @param $context3
 *   (optional) An additional variable that is passed by reference. This
 *   parameter is deprecated and will not exist in Drupal 8; consequently, it
 *   should not be used for new Drupal 7 code either. It is here only for
 *   backwards compatibility with older code that passed additional arguments
 *   to drupal_alter().

Shall we drop it?

About your second comment: sure, I'll switch the two first args as you suggested!

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Ah, right, the third argument was added at some point because there were calls with 3 contexts in core and that was the easiest way to fix it.

8.x will look very different anyway, for starters, we don't need a $entity_type variable and likely also can skip the $langcode so we don't have to worry about that.

ademarco’s picture

ademarco commented
StatusFileSize
new tmgmt-alter-field-values-2614100-11.patch2.9 KB
new interdiff.txt2.84 KB

Re-rolled including latest feedback, we should also provide some tests for the added functionality: which tests would you recommend me to look at in order to start working on it?

ademarco’s picture

ademarco commented
Status: Needs work » Needs review
ademarco’s picture

ademarco commented
StatusFileSize
new interdiff.txt498 bytes
new tmgmt-alter-field-values-2614100-13.patch2.9 KB

Re-rolling since it was altering the wrong "$data".

ademarco’s picture

ademarco commented
StatusFileSize
new tmgmt-alter-field-values-2614100-14.patch2.9 KB
new interdiff.txt1.57 KB

Actually hook_tmgmt_field_pre_populate_entity_alter() on we want still to alter $data and not $entity, I've rerolled the patch by switching the order of the two arguments.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Version: 7.x-1.x-dev » 8.x-1.x-dev
Status: Needs review » Patch (to be ported)

Thanks, committed and pushed. Moving to 8.x-1.x to evaluate if this is useful there too.

  • Berdir committed bd307cb on 7.x-1.x authored by ademarco 
    Issue #2614100 by ademarco: Allow to alter retrieved translatable field...
miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

I also was thinking about this and if it makes sense to add it.

However the example you provide sounds odd to me:
If we are talking of a translator specific situation, then the translator should do the encoding forward and backwards. It should receive enough context to identify such a situation.

Altering the item that is captured makes the data in TMGMT dependent on a specific not yet fixed translator...
(If you need custom variant of a translator to add special handling, you can subclass its implementation.)

I would be happy to understand this requirements and have a good example before adding the complexity in 8.x-1.x.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Yeah, I don't really believe that's a good use either, although if you just use a single translator then you can do whatever you want, i don't really care.

Better use cases would be to implement custom support for field types that we don't support property yet or adding special cases for some fields/field types.

The last submitted patch, 2: tmgmt-alter-field-values-2614100-1.patch, failed testing.

The last submitted patch, 5: tmgmt-alter-field-values-2614100-5.patch, failed testing.

The last submitted patch, 11: tmgmt-alter-field-values-2614100-11.patch, failed testing.

The last submitted patch, 13: tmgmt-alter-field-values-2614100-13.patch, failed testing.

Status: Patch (to be ported) » Needs work

The last submitted patch, 14: tmgmt-alter-field-values-2614100-14.patch, failed testing.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Hmm wrote quite a bit about this and then decided to drop. Retrying. ;-)

Yeah a source might be incomplete and should possibly offer adding more values.
What we tried with suggestions is currently too limited. I originally also wanted to add flags to suggestions that lead to force add or priorisation in UI.
A similar thing is what we do with resolving references now that add more data.

For these kind of extensions, it's just important that they don't overlap with internal workflows and are really only triggered on source capture and on write back for pre / post processing... so that TMGMT could introduce a translation memory that is not tainted with altered values and that can still be kept in sync with everything.