I noticed that "mongodb_field_storage" and "entity_translation" modules have compatibility issues for translatable field values when the field instance allows only a single value.

PS. I marked this as critical because under the given conditions, it causes data loss.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

math3usmartins’s picture

Status: Active » Needs review
FileSize
3.9 KB

This patch helps to explain what I think is wrong with the current source code.

Basically, the current code causes data loss because it forces a single/flat value storage when the field instance is set to support only single value per entity (field instance config form). However, with translatable field values, it must store multiple values (a value for each translation).

So currently, this compatibility issue causes something like this:

1. submitted data (Drupal):

$node->field_custom_acme['pt-br'][0]['value'] = 'Empresa';
$node->field_custom_acme['en'][0]['value'] = 'Company';

2. stored values (mongodb)

{
  field_custom_acme: { value: 'Company', _language: 'en' }
}

The reason is that the condition "if ($field['cardinality'] == 1)" isn't enough to flatten the stored information. We may need both "en" and "pt-br" keys, for example.

By the way, I think we should never flatten the stored information. In fact there's probably a backward compatibility with already existing sites and field values depending on the field instance settings, because the patch changes the way values are parsed/loaded from the mongodb collection.

This backward compatibility can be solved using an updated_N_hook() to adjust already existing content, but this may raise other issues like long execution time for sites with large amount of data -- which should be the case for those who chose mongodb.

PS. I couldn't test this patch properly but I wanted to report the bug quickly and share the code ASAP. Anyway, I'm sure the Drupal community will find a good solution.

Best regards,

Status: Needs review » Needs work

The last submitted patch, 1: mongodb-entitytranslation-2197361-1.patch, failed testing.

math3usmartins’s picture

Title: Incompatibility with translable field values (entity_translation) » Incompatibility with translatable field values (entity_translation)

fix typo on the issue title.

math3usmartins’s picture

fix relative path within the patch file.

math3usmartins’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 4: mongodb-entitytranslation-2197361-1.patch, failed testing.

math3usmartins’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 4: mongodb-entitytranslation-2197361-1.patch, failed testing.

marcingy’s picture

Priority: Critical » Normal

So the issue you are going to run into is that mongo only allows one array element in an index. This approach is going to make multi element indexing pretty much impossible. Also as this is doesn't affect drupal it isn't critical

math3usmartins’s picture

Actually, MongoDB follows the JSON structure, so it's very flexible.

The error isn't caused by a limitation on the mongodb side. I believe it's simply that this module wanted to simplify/flatten the data structure when there's only a single value for the field, but that simplification causes a conflict with i18n + entity_translation (translation per field) because it may need multiple values (one for each translation).

See the original version where the module changes the data structure right before saving it into mongodb:

// function mongodb_field_storage_field_storage_write()

if ($field['cardinality'] == 1) {
  foreach ($values[0] as $column_name => $column_value) {
    if (isset($field['columns'][$column_name])) {
      $field_values[$column_name] = _mongodb_field_storage_value($field['columns'][$column_name]['type'], $column_value);
    }
    if ($language != LANGUAGE_NONE) {
      $field_values['_language'] = $language;
    }
  }
}

Basically I think this module shouldn't change the data structure. And I think it's not really necessary to change the data structure. Moreover, it causes an additional process on write/load, to alter/revert the changes.

marcingy’s picture

Title: Incompatibility with translatable field values (entity_translation) » Make the module compatible with translatable field values (entity_translation)
Category: Bug report » Task

Mongo in terms of storage is flexible I agree. So you end with the structure

{
  field_custom_acme: {
    'en': {
      'value': 'a'
    }
    'a': {
      'value': 'b'
    }
  }
}

now you can index field_custom_acme but that is an array or you can add an index for field_custom_acme.en.value and an index for field_custom_acme.a.value. The latter of course works but you only have 64 indexes on a collection and for single language it matters little beyond the backward compatibility break which to be honest is likely to not be allowed to happen given how this could pretty much destroy a large site in terms of how long the update hook would take to run. I am not sure of the answer but I am not sure how this can be supported under d7, of course for d8 it will be as the module you reference is now in core.

math3usmartins’s picture

Just to make it clear, this structure works:

// fields_current.node
{
    "type" : "custom_type",
    "language" : "pt-br",
    "field_custom_single_language" : {
        "value" : "this value can't be translated"
    },
    "field_custom_multi_language" : [ 
        {
            "value" : "leite de coco",
            "_language" : "pt-br",
            "_delta" : 0
        }, 
        {
            "value" : "coconut milk",
            "_language" : "en",
            "_delta" : 0
        }
    ]
}

And that is the goal of the patch #2197361-4: Make the module compatible with translatable field values (entity_translation)

math3usmartins’s picture

To keep backward compatibility and support the entity_translation module for new sites, we can do something like this:

$conf['mongodb_field_storage_flatten_single_value'] = TRUE;

The module would check that variable during the write/load functions.

Sites using i18n + entity_translation must change that variable to FALSE.

That's basically introducing a config variable to support backward compatibility.

Just like what we had in the core:
https://drupal.org/drupal-7.21-release-notes

Caused by this:
https://drupal.org/drupal-7.20-release-notes

That's ugly, but helps. Anyway, that's just an idea to move on.

marcingy’s picture

The structure in #12 is going to be an issue as you are now going to have to index arrays of arrays to do anything and as I say mongo has a limit of one array of arrays per index. So while it might be ok from the question of can mongo create the json - the answer for which is yes, it is going to limit how complex of indexes you can create.