If field translatability is switched when a field already holds data, field_attach_form() might stop behaving correctly since it expects that all translatable fields match the entity language. This will cause empty widgets to be the displayed on the entity edit form.

We need to update field languages when switching a field's translatability to ensure that translatable fields match the entity language and untranslatable fields are always set from the entity language to LANGUAGE_NONE.

Example:

<?php>
$node->language = 'en'
$node->body = array(LANGUAGE_NONE => array('value' => 'test'));
// the body field language should be changed to 
$node->body = array('en' => array('value' => 'test'));

$node->language = LANGAUGE_NONE
$node->body = array(LANGUAGE_NONE => array('value' => 'test'));
// the body field language should not be changed since it already matches node language
?>

When switching a field from transatable to untranslatable we should worry about multiple language values being held by a field. In this case only the original values should be marked as LANGUAGE_NONE while their translation should be left alone. We should have an option to delete them while updating original values.

The process above should kick-in when updating a single field's translatability (probably we need a batch process). We need to code it to be reusable so that an additional UI can be built to mass change all field's translatability from a single screen.

Files: 
CommentFileSizeAuthor
#89 et-translatable-switch-1279372-89.patch15.62 KBplach
PASSED: [[SimpleTest]]: [MySQL] 60 pass(es). View
#89 et-translatable-switch-1279372-89.interdiff.D0.patch1.55 KBplach
#79 et-translatable-switch-1279372-79.patch15.13 KBplach
PASSED: [[SimpleTest]]: [MySQL] 60 pass(es). View
#75 et-translatable-switch-fix-permission-name-label-checkbox.patch15.13 KBKristen Pol
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-translatable-switch-fix-permission-name-label-checkbox_0.patch. Unable to apply patch. See the log in the details link for more information. View
#70 et-translatable-switch-fix-permission-name-label-checkbox.patch15.13 KBKristen Pol
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-translatable-switch-fix-permission-name-label-checkbox.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View
#70 field_translation_checkbox_with_label.png5.96 KBKristen Pol
#69 et-translatable-switch-fix-permission-name-label.patch15.06 KBKristen Pol
PASSED: [[SimpleTest]]: [MySQL] 60 pass(es). View
#64 et-translatable-switch-fix-permission-name-label.patch15.07 KBKristen Pol
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in sites/default/modules/entity_translation/entity_translation.module. View
#64 field_translation_label.png7.36 KBKristen Pol
#59 et-translatable-switch-fix-permission-name-bold.patch15.05 KBKristen Pol
PASSED: [[SimpleTest]]: [MySQL] 60 pass(es). View
#58 et-translatable-switch-fix-permission-name.patch15.02 KBKristen Pol
PASSED: [[SimpleTest]]: [MySQL] 60 pass(es). View
#56 et-translatable-switch-1279372-56.patch15.02 KBplach
PASSED: [[SimpleTest]]: [MySQL] 60 pass(es). View
#54 et-translatable-switch-1279372-54.patch14.67 KBplach
FAILED: [[SimpleTest]]: [MySQL] 64 pass(es), 9 fail(s), and 1 exception(es). View
#52 et-translatable-switch-1279372-52.patch12.01 KBgetgood
PASSED: [[SimpleTest]]: [MySQL] 75 pass(es). View
#49 et-translatable-switch-1279372-49.patch11.97 KBgetgood
PASSED: [[SimpleTest]]: [MySQL] 75 pass(es). View
#47 et-translatable-switch-1279372-47.patch10.01 KBgetgood
PASSED: [[SimpleTest]]: [MySQL] 75 pass(es). View
#45 et-translatable-switch-1279372-45.patch10.03 KBgetgood
PASSED: [[SimpleTest]]: [MySQL] 75 pass(es). View
#39 et-translatable-switch-1279372-39.patch9.5 KBgetgood
FAILED: [[SimpleTest]]: [MySQL] 75 pass(es), 0 fail(s), and 3 exception(es). View
#37 et-translatable-switch-1279372-37.patch9.02 KBgetgood
PASSED: [[SimpleTest]]: [MySQL] 75 pass(es). View
#31 et-translatable-switch-1279372-31.patch7.56 KBgetgood
FAILED: [[SimpleTest]]: [MySQL] 65 pass(es), 8 fail(s), and 1 exception(es). View
#29 et-translatable-switch-1279372-29.patch7.53 KBgetgood
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-translatable-switch-1279372-29.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View
#26 et-translatable-switch-1279372-26.patch6.32 KBgetgood
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#22 et-translatable-switch-1279372-22.patch7.31 KBgetgood
PASSED: [[SimpleTest]]: [MySQL] 75 pass(es). View
#20 et_translatable_switch-1279372-20.patch7.3 KBplach
FAILED: [[SimpleTest]]: [MySQL] 69 pass(es), 1 fail(s), and 0 exception(es). View
#19 switch-translatability-1279372-19.patch7.31 KBgetgood
FAILED: [[SimpleTest]]: [MySQL] 296 pass(es), 2 fail(s), and 85,130 exception(es). View
#14 switch-translatable-1279372-14.patch9.88 KBgetgood
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch switch-translatable-1279372-14.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View
#10 switch-translatable-1279372-10.patch9.63 KBgetgood
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch switch-translatable-1279372-10.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View
#8 switch-translatability-1279372-8.patch10.55 KBgetgood
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch switch-translatability-1279372-8.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View
#7 switch-translatability-1279372-7.patch9.17 KBgetgood
FAILED: [[SimpleTest]]: [MySQL] 73 pass(es), 1 fail(s), and 0 exception(es). View
#5 entity_translation_toggle_translatable.patch12.59 KBgetgood
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch entity_translation_toggle_translatable.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View

Comments

dergachev’s picture

According to plach, steps to reproduce:
* install d7 (latest)
* enable locale (nodes will now be assigned default lang, not LANG_NONE)
* create a node
* enable entity_translation, mark a field (ex: body) translatable (initally it's not translatable(
* then in node edit form, notice that existing value is lost (though still exists unreferenced in the DB somewhere)

dergachev’s picture

I was able to reproduce above, although noticed that using "drush generate-content 1 0" will generate nodes with language LANGUAGE_NONE rather than site default, even after the locale module is enabled.

plach’s picture

Issue tags: +montreal
dergachev’s picture

Assigned: Unassigned » dergachev

working on it.

getgood’s picture

Assigned: dergachev » getgood
Status: Active » Needs review
FileSize
12.59 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch entity_translation_toggle_translatable.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View

evolving web and I have got the issue worked out for nodes; you can switch translatable and back fine, nothing gets lost. It still needs to be generalised to all entities which have a language attribute.

loganfsmyth’s picture

Sooo, I have a pretty big list of things, sorry :P

  • Patch rolled with wrong base dir. Has to be applied with -p5. Since this is a module, the patch base should be the module's directory.
  • Make sure you have you user.name and user.email configured in git. Your patch has "thomas@spike"
  • Patches are supposed to be only one per file, so squash everything first.
  • Is the UI for disabling translation really enough to stop people from accidentally erasing data? All other places in core have a separate confirmation step.
  • I really feel like there should be a confirmation page in here somewhere, and then you'd be able to avoid the weird duplicate submit function thing I think too.
  • When I uncheck the box, I get *two* "Updated field Body field settings." messages. I guess this is because your duplicate submit function?
  • Submit function name isn't very clear. To know that it is a submit handler, you have to search for where it is called from. Maybe 'entity_translation_translatable_submit', 'entity_translation_translatable_batch_operation', and 'entity_translation_translatable_batch_finished' instead.
  • Lots of trailing whitespace.
  • More specific *initial* query, only need to process nodes with bundles using that field.
  • Pass batch args as associative array for clarity?
  • If you do add a confirmation step, maybe move all this into entity_translation.admin.inc
  • entity_translation_get_next_entities isn't needed. Just do the query and use fetchAllAssoc()
  • Should use drupal_set_message instead of dsm, since dsm is part of devel and not core.
  • Why only 10 items per batch?
  • Can probably speed up loading of field_attach_load if you pass it the 'field_id' option.
  • Should be converted to use more general entity lookups for both main queries in order to support all entity types.
  • * Normally the easy thing to do would be to use EntityFieldQuery, but the results from that are stub objects, and not full objects, so they don't have language info.
  • * You can't use entity_load either because it will try to load field data.
  • * Writing a custom query might work, but it'll be complicated.
  • * I don't actually know the best course of action here. I kind of wish language were a first class property.
  • Is all of this going to break terribly for nodes or other things using revisioning?

@plach Has there ever been any talk of making language a first class property and having it be part of all this stuff?
e. g.

list($id, $revision_id, $bundle, $langcode) = entity_extract_ids($entity_type, $entity);

With that, you could also have language for entity_create_stub_entity and in results from EntityFieldQuery.
I think that would require language to be a field in to db though.

getgood’s picture

Status: Needs review » Needs work
FileSize
9.17 KB
FAILED: [[SimpleTest]]: [MySQL] 73 pass(es), 1 fail(s), and 0 exception(es). View

I've rerolled the patch to deal with the first few points and make life easier.

To answer a few of the other points:

  • I agree that the confirmation page is a must, but I'm putting the rest up before it's done.
  • The two updated field settings come from submitting the same form twice with different content; the confirmation page should make that unnecessary.
  • The 10 items per batch was a compromise between high initialisation time with many per batch and lots of time moving from one batch op to the next with only one entity per batch. This is just a value I found to be fast.

For the rest, I'll start back on the code soon and address those then.

getgood’s picture

FileSize
10.55 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch switch-translatability-1279372-8.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View

Here's take 3.

I've addressed most of the issues Logan brought up and generalised the patch to all entities; it no longer makes any reference to nodes in particular. There are still a few major considerations though:

  • Since the code queries field base tables, it will break when dealing with revisions. This is the biggest stopping point and to be honest I don't know how to fix it. I'll figure it out eventually, but any advice would be appreciated.
  • As pointed out above, there is no consistent way to know the language of an entity --- indeed they needn't even have them. When switching off translatability, I delegate the processing to the entity translation module by querying its table, but when switching it on, this isn't available, so as of yet I'm just taking the global default language. The only thing better I can see is to implement a bunch of custom hacks for standard entity types and maybe a hook so that entities that care can tell us their language. If an entitiy's language can not be discerned, I think we should just ignore it, but that's debatable.
  • We still need a confirmation screen and a better custom submit handler. I've just been putting these off in favour of the more interesting stuff. Now that I'm a bit stuck, I'll probably get to them.

I'm sure there's more to fix, but those are the big ones.

All thoughts and criticisms are welcome.

plach’s picture

Status: Needs work » Needs review

Thanks! Hope to be able to review this tomorrow night.

getgood’s picture

Status: Needs review » Needs work
FileSize
9.63 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch switch-translatable-1279372-10.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View

Logan pointed out a much better way to get the list of entities which I think should be okay with revisions. Here's another take.

getgood’s picture

Status: Needs work » Needs review

didn't mean to change the status...

loganfsmyth’s picture

Just for reference, I've created an issue for the EntityFieldQuery count bug that you had to work around.
#1292922: EntityFieldQuery count query broken for translatable fields

plach’s picture

I ain't sure the batch API is being used the right way here, but I might be wrong. Since the Title module performs a very similar operation (acting on all values of a particular field, only of a given entity type though), would you please check how it's done there and if it would make sense here? See lines 273-349:

http://drupalcode.org/project/title.git/blob/refs/heads/7.x-1.x:/title.m...

In particular I don't think the operations array is intended to be populated with every step of a particular operation, but with different operations that can be divided in various steps.

getgood’s picture

FileSize
9.88 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch switch-translatable-1279372-14.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View

you're right about the batch api. The comments starting at line 4010 in form.inc are very similar to your example with more explanation.

Here's a version with the batch done properly. I think the bulk of what's left is making the confirmation form and moving most of the code I've added into entity_translation.admin.inc.

Though there is still the issue of how to decide the language of entities...

plach’s picture

Status: Needs review » Needs work

Great work! We seem to be almost there: after fixing the stuff below I guess the only pending thing will be the confirmation page. I think we can postpone revision handling for now.

+++ b/sites/all/modules/entity_translation/entity_translation.module
@@ -469,6 +469,243 @@ function entity_translation_form_field_ui_field_settings_form_alter(&$form, $for
+  $form['field']['warning']['warning'] = array(
+    '#markup' => t('By submitting this form you will trigger a batch operation. THIS OPERATION COULD RESULT IN LOSS OF DATA. Please ensure that all effected nodes are backed up before proceeding.'),
+  );

We might want to use a warning style here. Also the warning about losing data should be showed only when making a field untranslatable. Moreover ususally we don't use uppercase to emphasize important sentences. I guess something like the following would be ok:

<?php
  $warning = t('By submitting this form you will trigger a batch operation.');
  if ($field['translatable']) {
    $warning .= ' ' . t('<em>All field translations will be removed except for the original ones.</em> Please ensure that all affected content is backed up before proceeding.');
  }
  $form['field']['warning']['warning'] = array(
    '#markup' => '<div class="messages warning">' . $warning . '</div>',
  );
?>
+++ b/sites/all/modules/entity_translation/entity_translation.module
@@ -469,6 +469,243 @@ function entity_translation_form_field_ui_field_settings_form_alter(&$form, $for
+  if (isset($form['#submit'])) {
+    array_unshift($form['#submit'], 'entity_translation_translatable_submit');
+  }
+  else {
+    $form['#submit'] = array('entity_translations_translatable_submit');
+  }

To avoid hacks making the submit handlers work, I tested pushing the submit handler on the front or on the back of the submit handler array depending on the field's translatability:

<?php
  if (!isset($form['#submit'])) {
    $form['#submit'] = array();
  }
  if ($field['translatable']) {
    array_unshift($form['#submit'], 'entity_translation_translatable_submit');
  }
  else {
    $form['#submit'][] = 'entity_translation_translatable_submit';
  }
?>
+++ b/sites/all/modules/entity_translation/entity_translation.module
@@ -469,6 +469,243 @@ function entity_translation_form_field_ui_field_settings_form_alter(&$form, $for
+  $checked = $form_state['values']['field']['translatable'];
+  $prior = $form['field']['translatable']['#default_value'];

I'd use $translatable and $previous respectively for consistency with other parts of ET.

+++ b/sites/all/modules/entity_translation/entity_translation.module
@@ -469,6 +469,243 @@ function entity_translation_form_field_ui_field_settings_form_alter(&$form, $for
+  // When making field untranslatable, we need to make sure that the other submit handler
+  // can't see the changed translatable setting. We change it ourselves after the batch operation.
+  //
+  // For some reason, adding the custom submit handler to the batch job doesn't work; it never gets called.
+  // Why this is is yet to be determined, but a temporary workaround is to pass the data into the batch results
+  // and then call the submit handler from the finalising callback.
+  if (! $checked) {
+    $ops[] = array('entity_translation_translatable_batch_data_store', array($form, $form_state));
+    $form_state['values']['field']['translatable'] = $prior;
+  }
+
+  // Somehow this submit handler doesn't get called unless it comes first. So, we look through the list and
+  // if we find one, we call it ourselves and remove it from the list. It is removed and executed at most once.
+  //
+  // This is not a good solution, but it works. fixme if you see how.
+  foreach ($form['#submit'] as $key => &$value) {
+    if ($value === 'field_ui_field_settings_form_submit') {
+      unset($value);
+      field_ui_field_settings_form_submit($form, $form_state);
+      break;
+    }
+  }

This part can go away if we set the submit handler order conditionally as suggestd above. However to make this approach work we must work around a bug I discovered while reviewing this patch (see below).

+++ b/sites/all/modules/entity_translation/entity_translation.module
@@ -469,6 +469,243 @@ function entity_translation_form_field_ui_field_settings_form_alter(&$form, $for
+
+/**
+ * Hacky helper function to pass the form data off to entity_translation_translatable_batch_done.
+ */
+function entity_translation_translatable_batch_data_store($form, $form_state, &$context) {
+  $context['results'] = array($form, $form_state);
+}

Also this can go away.

+++ b/sites/all/modules/entity_translation/entity_translation.module
@@ -469,6 +469,243 @@ function entity_translation_form_field_ui_field_settings_form_alter(&$form, $for
+    // How many entities will need processing?
+    // Unfortunately EFQ->count() counts all translations of all fields, not just the entities that have them
+    $count = 0;
+    $EFQ = new EntityFieldQuery();
+    $entities = $EFQ->fieldCondition($field_name)
+                 ->execute();
+
+    foreach($entities as $type => $list) {
+      $count += count($list);
+    }

The related issue has been committed, so we can use a plain count efq here.

+++ b/sites/all/modules/entity_translation/entity_translation.module
@@ -469,6 +469,243 @@ function entity_translation_form_field_ui_field_settings_form_alter(&$form, $for
+  dsm($query);

Please remove the dsm() calls before rolling the patch.

+++ b/sites/all/modules/entity_translation/entity_translation.module
@@ -469,6 +469,243 @@ function entity_translation_form_field_ui_field_settings_form_alter(&$form, $for
+  foreach ($query as $entity_type => $entities) {
+    field_attach_load($entity_type, $entities, FIELD_LOAD_CURRENT, array('field_id' => $field_name));
+  }
+
+  foreach ($query as $entity_type => $entities) {

These two loops can be merged.

+++ b/sites/all/modules/entity_translation/entity_translation.module
@@ -469,6 +469,243 @@ function entity_translation_form_field_ui_field_settings_form_alter(&$form, $for
+    // Getting the default language of the entities is still an open problem.
+    // When turning off translatability, the obvious place to look is the source language
+    // attribute in the entity_translation table, but that table doesn't exist until after
+    // the field in question has been made translatable, so when turning translatability
+    // on...
+    //
+    // I think the best thing to do is implement a hook_..._language_alter since the site admins
+    // likely have a much better idea of what language things are than Drupal does.
+    //
+    if (!$make_translatable) {
+      // This exploits the fact that in the entity_translation table, if the source is blank then the
+      // language is the source language.
+      $language = db_select('entity_translation', 'et')
+                ->fields('et', array('entity_id', 'language'))
+                ->condition('source', '', '=')
+                ->condition('entity_id', array_keys($entities), 'IN')
+                ->execute()
+                ->fetchAllKeyed();
+    }
+    else {
+      // Far from ideal interim solution. fixme
+      $language = array_fill_keys(array_keys($entities), language_default()->language);
+    }

The correct way to handle the entity language is using the translation handler. Unfortunately we need a full entity to instantiate it. The good news are that now entities are optimized for multiple loading and that the entity cache module speeds this up even more:

<?php
$entities = entity_load($entity_type, array_keys($entities));
?>
+++ b/sites/all/modules/entity_translation/entity_translation.module
@@ -469,6 +469,243 @@ function entity_translation_form_field_ui_field_settings_form_alter(&$form, $for
+      $lang = $language[$id];

As per above this can be replaced by:

<?php
$langcode = entity_translation_get_handler($entity_type, $entity)->getLanguage();
?>

($langcode is the variable name commonly used)

+++ b/sites/all/modules/entity_translation/entity_translation.module
@@ -469,6 +469,243 @@ function entity_translation_form_field_ui_field_settings_form_alter(&$form, $for
+  $context['sandbox']['progress'] += $inc;
+  if ($content['sandbox']['progress'] < $content['sandbox']['max']) {
+    $context['finished'] = $content['sandbox']['progress'] / $content['sandbox']['max'];
+  }

This cannot work :) $content should be replaced by $context. However incrementing the progress key by $inc (I think the batch API example uses the $limit name btw) is not correct since we might go over the max value. I'd simply increment the progress key by 1 in the previous foreach loop. This should allow to remove the if check.

To workaround the bug I mentioned before we need to add the following lines here:

<?php
  if ($context['finished'] >= 1) {
    module_load_include('inc', 'field_ui', 'field_ui.admin');
  }
?>

See #1300928: Form submit handlers not called during batch processing if not already included.

+++ b/sites/all/modules/entity_translation/entity_translation.module
@@ -469,6 +469,243 @@ function entity_translation_form_field_ui_field_settings_form_alter(&$form, $for
+  if (isset($results) && count($results) === 2) {
+    // Run our custom submit handler.
+    entity_translation_custom_submit($results[0], $results[1]);
+  }

This can go away.

+++ b/sites/all/modules/entity_translation/entity_translation.module
@@ -469,6 +469,243 @@ function entity_translation_form_field_ui_field_settings_form_alter(&$form, $for
+
+/**
+ * This is a shameless cut and paste of field_ui_field_settings_form_submit from field_ui.admin.inc.
+ *
+ * The reason is that we need to submit the form, but keep one property (translatabe) aside to be updated
+ * after we've run our batch operation; if we make a field untranslatable before moving the data we lose it.
+ *
+ * TODO: Find the right way to do this.
+ */
+function entity_translation_custom_submit($form, &$form_state) {
+  $form_values = $form_state['values'];
+  $field_values = $form_values['field'];
+
+  // Merge incoming form values into the existing field.
+  $field = field_info_field($field_values['field_name']);
+
+  $entity_type = $form['#entity_type'];
+  $bundle = $form['#bundle'];
+  $instance = field_info_instance($entity_type, $field['field_name'], $bundle);
+
+  // Update the field.
+  $field = array_merge($field, $field_values);
+
+  try {
+    field_update_field($field);
+    drupal_set_message(t('Updated field %label translatable setting.', array('%label' => $instance['label'])));
+  }
+  catch (FieldException $e) {
+    drupal_set_message(t('Attempt to update field %label failed: %message.', array('%label' => $instance['label'], '%message' => $e->getMessage())), 'error');
+  }
 }

This can go away.

Aside from the things above, there are some coding standard issues with comments: they should always start with a capital letter and end with a dot. Moreover they should wrap at column 80. See http://drupal.org/coding-standards for details.

plach’s picture

(bump)

Any news?

dergachev’s picture

Sorry, it looks like Thomas doesn't have email notifications setup properly. Just pinged him about this.

BTW awesome review.

getgood’s picture

Lot's of good advice, thanks plach.

I've implemented most of your suggestions and started on the confirm page but got side tracked. I'll clean up what I have and try to wrap it up tonight if I can. I'll put something up either way. I had an issue with loading the translation handlers, but I think I know what I was doing wrong. I'll let you know soon if I in fact don't.

getgood’s picture

FileSize
7.31 KB
FAILED: [[SimpleTest]]: [MySQL] 296 pass(es), 2 fail(s), and 85,130 exception(es). View

Sorry for the delay. I was trying to get the confirm page working but I haven't had much time and am having some trouble with getting the redirect to work properly.

As it is I've implemented all of your suggestions and cleaned up a bit.

Given that this is major and I'm not exactly speeding along, I'll understand if someone wants to take over. If not, I'll keep at it as I have time.

plach’s picture

Status: Needs work » Needs review
FileSize
7.3 KB
FAILED: [[SimpleTest]]: [MySQL] 69 pass(es), 1 fail(s), and 0 exception(es). View

Minor code style fixes. Confirmation dialog stil missing. Let's test this.

Status: Needs review » Needs work

The last submitted patch, et_translatable_switch-1279372-20.patch, failed testing.

getgood’s picture

FileSize
7.31 KB
PASSED: [[SimpleTest]]: [MySQL] 75 pass(es). View

After an absurd hiatus, another attempt to please the testbot. I've got some time now, so hopefully that confirmation dialog will be done soon too.

loganfsmyth’s picture

Status: Needs work » Needs review

For testing.

loganfsmyth’s picture

* Try not to include big hunks of commented out stuff in patches. I usually use git add -p to build up patch contents, the use git diff --cached to output the patch.
* Could we add some explanation of why it is important that the translations get moved before the field becomes untranslatable? The comment by the #submit stuff is kind of vague.
* Can't the translatable submit just always run before the field config is updated? Be we need to two conditions?
* Plach said above to ignore revision handling for now, so maybe it can show a warning at least so it is clear?
** EntityFieldQuery is only going to look at the current revisions. You'll need to do two EntityFieldQuerys with different ages.
** http://api.drupal.org/api/drupal/includes--entity.inc/function/EntityFie...
** This will also mean a change in the way entity_load is executed.
* Need to call field_attach_presave too. The blog post you linked
* Would it be a good idea to put the site in maintenance mode while this batch job runs? I feel like things could go wrong if it is running while people try to edit content. Is that something we is just left up to the site author to do?

plach’s picture

Status: Needs review » Needs work

Thanks Thomas! Besides Logan's comments, which look sensible:

+++ b/entity_translation.module
@@ -465,11 +489,164 @@ function entity_translation_edit_form_submit($form, &$form_state) {
+  if ($field['translatable']) {
+    array_unshift($form['#submit'], 'entity_translation_form_translatable_submit');
+  }
+  else {
+    $form['#submit'][] = 'entity_translation_form_translatable_submit';
+  }

As Logan's comment shows, it's not immediately clear why we need two different orders: a comment here saying something like "Since a field translatabilty determines which values can be retrieved from/saved to the storage, we need to ensure we perform the needed changes after/before we enable/disable translatability to be able to deal with multilingual values." would be fine.

+++ b/entity_translation.module
@@ -465,11 +489,164 @@ function entity_translation_edit_form_submit($form, &$form_state) {
+    $warning .= ' ' . t('<em> All translations of this field will be deleted. </em>. Please backup your data before proceeding.');

I'd trim the whitespaces around the emmed part. Also, there is a double dot.

+++ b/entity_translation.module
@@ -465,11 +489,164 @@ function entity_translation_edit_form_submit($form, &$form_state) {
+ * Batch operation. Switch the translatability of the given field. ¶

Trailing whitespace.

+++ b/entity_translation.module
@@ -465,11 +489,164 @@ function entity_translation_edit_form_submit($form, &$form_state) {
+    if (intval($count) === 0) {

Wouldn't empty($count) work here?

+++ b/entity_translation.module
@@ -465,11 +489,164 @@ function entity_translation_edit_form_submit($form, &$form_state) {
+        // Nno need to save unchanged entities.

Typo.

About

Would it be a good idea to put the site in maintenance mode while this batch job runs? I feel like things could go wrong if it is running while people try to edit content. Is that something we is just left up to the site author to do?

I ain't sure which is the recommended behavior: I'm not aware of modules silently putting the site in maintenance mode (but they might exist :)). Moreover, this really look like an operation one would do in a dev environment or in maintenance mode anyway. Perhaps a warning could be enough?

getgood’s picture

FileSize
6.32 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Before I get carried away, here's the cleaned up version.

I used intval($count) === 0 instead of empty($count) because it's more obvious that way what's actually being checked. If you think that will cause problems, I can change it.

loganfsmyth’s picture

if (intval($count) === 0) {

is just highly verbose when you could simply be doing this.

if (!$count) {
getgood’s picture

The whole system of what's true and what's false in php bothers me; I like to know what I'm checking. But I'll change it for the next round.

getgood’s picture

Status: Needs work » Needs review
FileSize
7.53 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-translatable-switch-1279372-29.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View

I've got a confirm page working, though I'm sure it needs some work.

The cleanest way I could think of after talking to Logan was to get rid of the checkbox completely and replace it with a link to a confirm page which runs the batch op. This breaks some of the tests, but I'm loath to change them until I get some feedback on the flow I built.

The code also breaks when I refactor it into admin.inc, but that's comparatively minor.

Status: Needs review » Needs work

The last submitted patch, et-translatable-switch-1279372-29.patch, failed testing.

getgood’s picture

Status: Needs work » Needs review
FileSize
7.56 KB
FAILED: [[SimpleTest]]: [MySQL] 65 pass(es), 8 fail(s), and 1 exception(es). View

Forgot to pull first. Try again.

Status: Needs review » Needs work
Issue tags: -montreal

The last submitted patch, et-translatable-switch-1279372-31.patch, failed testing.

plach’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, et-translatable-switch-1279372-31.patch, failed testing.

getgood’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +montreal

The last submitted patch, et-translatable-switch-1279372-31.patch, failed testing.

getgood’s picture

Status: Needs work » Needs review
FileSize
9.02 KB
PASSED: [[SimpleTest]]: [MySQL] 75 pass(es). View

I've modified the tests to check the new form instead of the no longer extant checkbox.

loganfsmyth’s picture

Status: Needs review » Needs work

Quick review of the code. This looks great, just have a few comments on the way the form is built up, and how the data is passed around.

+++ b/entity_translation.module
@@ -205,12 +205,44 @@ function entity_translation_menu_alter(&$items) {
+  $items['admin/structure/types/manage/%/fields/%/translatable'] = array(
+    'title' => t('Confirm Change in translatability.'),
+    'description' => 'Confirm page for changing field translatability.',
+    'page callback' => 'drupal_get_form',
+    'page arguments' => array('entity_translation_warning_form', 4, 6),
+    'access arguments' => array('administer content types'),
+  );
+

This menu path will only work for Nodes, so the menu path should probably be something more generic like 'admin/config/regional/entity_translation/translate/%', with % as the field_name. You don't need the bundle for this conversion.

+++ b/entity_translation.module
@@ -205,12 +205,44 @@ function entity_translation_menu_alter(&$items) {
+  $form['#submit'] = array('entity_translation_form_translatable_submit');

Better to do $form['#submit'][] = 'entity_translation_form_translatable_submit';

+++ b/entity_translation.module
@@ -205,12 +205,44 @@ function entity_translation_menu_alter(&$items) {
+  // We need to attach the field name for future processing.
+  $form['#etfield'] = $field;
+

Rather than saving the whole field here, I would recommend adding two items to $form, 'field_name' and 'translatable' as #type => 'hidden'. That way your intent is more directly sent when you submit the page. Otherwise, when you submit the page, you are essentially just saying 'toggle', which might conflict.

+++ b/entity_translation.module
@@ -481,13 +513,161 @@ function entity_translation_edit_form_submit($form, &$form_state) {
+  $field_name = $form_state['build_info']['args'][0]['field_name'];

This lookup is a bit weird. Is there nowhere in $form or $form_state that you can get that is a more straightforward way?

+++ b/entity_translation.module
@@ -481,13 +513,161 @@ function entity_translation_edit_form_submit($form, &$form_state) {
+  $field = field_read_fields(array('field_name' => $field_name));

field_info_field($field_name) is what you want here.

+++ b/entity_translation.module
@@ -481,13 +513,161 @@ function entity_translation_edit_form_submit($form, &$form_state) {
   $form['field']['translatable'] = array(
-    '#type' => 'checkbox',
-    '#title' => t('Users may translate this field.'),
-    '#default_value' => $field['translatable'],
+    '#markup' => "<div>" . $status . ' ' . l(t('Change Translatability'), $path, array('query' => drupal_get_destination())) . "</div>",
+  );

There is a #type => 'link' that might be better to use here. You can wrap it in the div with #prefix and #suffix.

+++ b/entity_translation.module
@@ -481,13 +513,161 @@ function entity_translation_edit_form_submit($form, &$form_state) {
+  $translatable = $form['#etfield']['translatable'];
+
+  $field_name = $form['#etfield']['field_name'];

As I said in my comments above, it would be better to read these directly from $form_state. And be sure to do another field_info_field and make sure that translatability isn't the value you want.

+++ b/entity_translation.module
@@ -481,13 +513,161 @@ function entity_translation_edit_form_submit($form, &$form_state) {
+  $field = field_read_fields(array('field_name' => $field_name));

field_info_field() again.

getgood’s picture

Status: Needs work » Needs review
FileSize
9.5 KB
FAILED: [[SimpleTest]]: [MySQL] 75 pass(es), 0 fail(s), and 3 exception(es). View

Clean up as per Logan's comments.

Status: Needs review » Needs work

The last submitted patch, et-translatable-switch-1279372-39.patch, failed testing.

loganfsmyth’s picture

+++ b/entity_translation.moduleundefined
@@ -205,12 +205,48 @@ function entity_translation_menu_alter(&$items) {
+    'title' => t('Confirm Change in translatability.'),

Menu items are one of the only places where you aren't supposed to wrap the strings in t() calls. The strings are cached, so if you cleared the cache when viewing the site in French, then all the menus wrapped in t() would change to French for everyone in the site.

+++ b/entity_translation.moduleundefined
@@ -205,12 +205,48 @@ function entity_translation_menu_alter(&$items) {
+  $redirect = $_GET['destination'] ? $_GET['destination'] : 'admin/structure/types/manage';
+
+  $form = confirm_form($form, t('Warning.'), $redirect, $warning, t('Convert'), t('Cancel'));

confirm_form actually overrides $path with $_GET['destination'] automatically, so I would just set $path as '<front>' or something.

+++ b/entity_translation.moduleundefined
@@ -205,12 +205,48 @@ function entity_translation_menu_alter(&$items) {
+  $form['#submit'][] = 'entity_translation_form_translatable_submit';

If you change your submit function to be named entity_translation_warning_form_submit, then you won't need to have this line at all.

+++ b/entity_translation.moduleundefined
@@ -205,12 +205,48 @@ function entity_translation_menu_alter(&$items) {
+  $form['#etfield'] = array(
+    '#type' => 'hidden',
+    '#field_name' => $field_name,
+    '#field_translatable' => $field['translatable'],
+  );

Close, but not quite what I meant. #type hidden is just like any other form element.

$form['field_name'] = array(
  '#type' => 'hidden',
  '#default_value' => $field_name,
);
$form['translatable'] = array(
  '#type' => 'hidden',
  '#default_value' => $field['translatable'],
);
+++ b/entity_translation.moduleundefined
@@ -481,16 +517,181 @@ function entity_translation_edit_form_submit($form, &$form_state) {
+  $translatable = $form['#etfield']['#field_translatable'];
+  $field_name = $form['#etfield']['#field_name'];

Then these would be:

$field_name = $form_state['values']['field_name'];
$translatable = $form_state['values']['translatable'];

The point of this is so that the values for field_name and translatability are actually stored in the page and are submitted right along with the rest when you press Submit.

As it is now, pressing Submit basically instructs Drupal to take the current translatability setting and toggle it. So say you opened two pages with this form and both said "Submit to make non-translatable" and you submitted one. The field would be non-translatable. At this point if you submitted the other one, even though it said that submitting would make it non-translatable, submitting it would actually toggle the value again and make it translatable.

loganfsmyth’s picture

I found a bug that won't come up very often, but it's a big one. It's also not your fault.
I make a core issue here: #1380660: field_available_languages static cache never cleared

In a default drupal install, this is not a problem because your form doesn't trigger populating that cache, but you can't rely on that.

If anything in the site triggers the loading of values for the field you are converting, then this bug will cause all the values to be erased. In my testing, I loaded a node revision using node_load(1,1) in a hook_init, and that was enough to erase all body field data.

The simplest fix for now is just to add this after your field_update_field:

drupal_static_reset('field_available_languages');
loganfsmyth’s picture

I'd also just like to point out that this all works because LANGUAGE_NONE is one of the available languages for translatable fields. If for instance someone implemented hook_field_available_languages_alter and removed LANGUAGE_NONE, the conversions in this patch would fail in a few different places causing data loss.

It would be a good idea to put in some verifications that field_available_languages contains the languages being converted to and from.

loganfsmyth’s picture

#1380660: field_available_languages static cache never cleared has been fixed in 7.x and 8.x. It still makes sense to clear that cache yourself after calling field_update_field for now, in order to support older versions of 7.x, but add a comment so it can be removed at some later time.

getgood’s picture

Status: Needs work » Needs review
FileSize
10.03 KB
PASSED: [[SimpleTest]]: [MySQL] 75 pass(es). View

I think I've covered everything Logan said.

I also refactored the bulk of the logic into entity_translation.admin.inc.

loganfsmyth’s picture

Status: Needs review » Needs work
+++ b/entity_translation.admin.inc
@@ -489,3 +489,180 @@ function entity_translation_delete_confirm_submit($form, &$form_state) {
+  $form['field_name'] = array(
+    '#type' => 'hidden',
+    '#default_value' => $field_name,
+  );

Sorry, I realize that I recommended this, but I think that since $field_name is an argument in the URL, it is not needed to have this one in the form as a hidden value. This value you CAN pass in $form['#field_name']. 'translatable' definitely should be here though.

+++ b/entity_translation.admin.inc
@@ -489,3 +489,180 @@ function entity_translation_delete_confirm_submit($form, &$form_state) {
+    'file' => drupal_get_path('module', 'entity_translation') .'/entity_translation.admin.inc',

Missing a space between the "." and "'".

+++ b/entity_translation.admin.inc
@@ -489,3 +489,180 @@ function entity_translation_delete_confirm_submit($form, &$form_state) {
+      // Save field data.
+      field_attach_update($entity_type, $entity);

We need to add some safeguards here to try to be 100% sure that the data will actually save properly. I think before calling field_attach_update you should confirm that the new field language exists in the results of field_available_languages. @plach, do you think that's necessary?

+++ b/tests/entity_translation.test
@@ -147,11 +147,13 @@ class EntityTranslationTestCase extends DrupalWebTestCase {
+    $this->drupalGet('admin/config/regional/entity_translation/translate/body', array(
+      'query' => array(
+        'destination' => 'admin/structure/types/manage/page/fields/body/field-settings')
+      )
+    );

I don't think this does anything. Any idea why it was needed before? drupalPost automatically does a GET before processing the form, so this is just duplicate logic I think. If you want to pass a destination for the POST, then the fourth param of drupalPost is options for the GET.
http://api.drupal.org/api/drupal/modules--simpletest--drupal_web_test_ca...

+++ b/tests/entity_translation.test
@@ -147,11 +147,13 @@ class EntityTranslationTestCase extends DrupalWebTestCase {
+    $this->drupalPost('admin/config/regional/entity_translation/translate/body', array(), t('Convert'));

Since this form now has those the hidden variables, we should include actual values in the second param of this POST.

getgood’s picture

Status: Needs work » Needs review
FileSize
10.01 KB
PASSED: [[SimpleTest]]: [MySQL] 75 pass(es). View

I've explicitly set translatable in the test and moved field_name out of the hidden field. As for the others, the GET request helped me find a problem at one point and it's the general structure of the whole test file, so I'd rather not change that. As for the double checking that the language being translated to exists, I understood that the translation handler must have an enabled language. If I'm mistaken, I still think it would make more sense to ensure that --- if it's reasonable --- so that things are consistent.

plach’s picture

Status: Needs review » Needs work

Thanks for keeping this issue going!

Edit: updated review :)

+++ b/entity_translation.admin.inc
@@ -489,3 +489,178 @@ function entity_translation_delete_confirm_submit($form, &$form_state) {
+

Surplus empty line.

+++ b/entity_translation.admin.inc
@@ -489,3 +489,178 @@ function entity_translation_delete_confirm_submit($form, &$form_state) {
+  $warning = t('By submitting this form you will trigger a batch operation.');
+  if ($field['translatable']) {
+    $warning .= "<br>" . t("<strong>All translations of this field will be deleted.</strong><br>Please backup your data before proceeding.");
+  }
+
+  $form = confirm_form($form, t('Warning.'), '', $warning, t('Convert'), t('Cancel'));

Usually we don't recommend to backup data in confirmation forms, although it can be a good practice. I'd remove the last sentence and turn the warning into an explicit question. We might just want to reuse (more or less) the same confirmation message appearing when multiple nodes are going to be deleted.

+++ b/entity_translation.admin.inc
@@ -489,3 +489,178 @@ function entity_translation_delete_confirm_submit($form, &$form_state) {
+  // If a field is untranslatable, it can have no data except under LANGUAGE_NONE,
+  // Thus we need a field to be translatable before we convert data to the entity

Wrong comment wrapping.

+++ b/entity_translation.admin.inc
@@ -489,3 +489,178 @@ function entity_translation_delete_confirm_submit($form, &$form_state) {
+  // language. Conversely we need to convert data back to LANGUAGE_NONE before

Can we use something like "switch data back" instead of "convert data back"? It somehow clashes with "Conversely".

+++ b/entity_translation.admin.inc
@@ -489,3 +489,178 @@ function entity_translation_delete_confirm_submit($form, &$form_state) {
+    'title' => t('Converting'),

Can we add field name and translatabilty in the title here?

+++ b/entity_translation.admin.inc
@@ -489,3 +489,178 @@ function entity_translation_delete_confirm_submit($form, &$form_state) {
+function entity_translation_translatable_batch($make_translatable, $field_name, &$context) {

I'd like to have a consistent variable name: either $make_translatable or $translatable. No strong preference about which one.

+++ b/entity_translation.admin.inc
@@ -489,3 +489,178 @@ function entity_translation_delete_confirm_submit($form, &$form_state) {
+
+  }

Please move the empty line below the closing brace.

+++ b/entity_translation.admin.inc
@@ -489,3 +489,178 @@ function entity_translation_delete_confirm_submit($form, &$form_state) {
+  $limit = 10;

Let's use a system variable here, so this value can be configured. Something like "entity_translation_translatable_batch_limit"

+++ b/entity_translation.admin.inc
@@ -489,3 +489,178 @@ function entity_translation_delete_confirm_submit($form, &$form_state) {
+          // It's not enough to simply delete the field, we need to tell
+          // the translation handler to wipe it out.

Wrong comment wrapping.

+++ b/entity_translation.admin.inc
@@ -489,3 +489,178 @@ function entity_translation_delete_confirm_submit($form, &$form_state) {
+          $handler->removeTranslation($lang);

This is not completely correct: we need to check whether there is no translatable field left before removing an entity translation.

+++ b/entity_translation.admin.inc
@@ -489,3 +489,178 @@ function entity_translation_delete_confirm_submit($form, &$form_state) {
+      field_attach_update($entity_type, $entity);

We need a field_attach_presave() call before saving the field values.

+++ b/entity_translation.admin.inc
@@ -489,3 +489,178 @@ function entity_translation_delete_confirm_submit($form, &$form_state) {
+  $context['finished'] = $context['sandbox']['progress']/$context['sandbox']['max'];

Missing spaces around the / operator.

+++ b/entity_translation.admin.inc
@@ -489,3 +489,178 @@ function entity_translation_delete_confirm_submit($form, &$form_state) {
+ * Check the exit status of the batch operation, and possibly resubmit the changed translatable status.

Let's make the PHP doc reflect what the function actually does: I'd remove the second sentence until we actually turn the @todo below into code.

+++ b/entity_translation.admin.inc
@@ -489,3 +489,178 @@ function entity_translation_delete_confirm_submit($form, &$form_state) {
+    // TODO: Do something about this case.

The @todo form should be used.

+++ b/entity_translation.module
@@ -205,12 +205,23 @@ function entity_translation_menu_alter(&$items) {
+  $items['admin/config/regional/entity_translation/translate/%'] = array(
+    'title' => 'Confirm Change in translatability.',
+    'description' => 'Confirm page for changing field translatability.',
+    'page callback' => 'drupal_get_form',
+    'page arguments' => array('entity_translation_warning_form', 5),
+    'access arguments' => array('administer content types'),
+    'file' => 'entity_translation.admin.inc',
+    'module' => 'entity_translation',
+  );

This belongs to entity_translation_menu(), let's move it over there and remove the module key. The other admin item declaration will follow in another patch. Moreover the 'access arguments' key is wrong: ideally it should match the one used to access the Manage fields page, and every entity type has its own, see field_ui_menu(). However, to avoid making this overly complex I'd add an ET permission explictly granting access to the bulk update form. We should add an '#access' key to $form['field']['translatable']['link'] checking this permission.

getgood’s picture

Status: Needs work » Needs review
FileSize
11.97 KB
PASSED: [[SimpleTest]]: [MySQL] 75 pass(es). View

Thanks for all of the feed back. It's getting much cleaner.

I can't find a good way to check that an entity has no translatable fields left before deleting the translation handler. I've put in a clumsy iteration over all of the fields that its type has, but that's not very efficient. Should we consider letting the translation handler keep track of that sort of thing? or is that too far beyond its original purpose?

loganfsmyth’s picture

Status: Needs review » Needs work

Few very quick comments.

+++ b/entity_translation.admin.inc
@@ -489,3 +489,207 @@ function entity_translation_delete_confirm_submit($form, &$form_state) {
+        $fields = field_info_instances($entity_type, $entity->type);

$entity->type will only work for nodes. You need to specifically look up the bundle id.

list(,,$bundle) = entity_extract_ids($entity_type, $entity);
$fields = field_info_instances($entity_type, $bundle);
+++ b/entity_translation.admin.inc
@@ -489,3 +489,207 @@ function entity_translation_delete_confirm_submit($form, &$form_state) {
+        dsm($has_translatables, "as");

Oops :P

plach’s picture

+++ b/entity_translation.admin.inc
@@ -489,3 +489,207 @@ function entity_translation_delete_confirm_submit($form, &$form_state) {
+        // Check if the entity has any more translatable fields. If not, then
+        // we must delete the translation handler.

Minor thing: the actual stuff being deleted is the entity translation, not the translation handler.

getgood’s picture

Status: Needs work » Needs review
FileSize
12.01 KB
PASSED: [[SimpleTest]]: [MySQL] 75 pass(es). View

That's what I get for posting in a hurry. Anything else?

plach’s picture

From a first look everything seems ok! I'll perform some manual testing later today.

plach’s picture

Status: Needs review » Needs work
FileSize
14.67 KB
FAILED: [[SimpleTest]]: [MySQL] 64 pass(es), 9 fail(s), and 1 exception(es). View

After a thorough testing session I found out some edge cases that were not handled by #52:

  • Toggling translatability for a field shared accross multiple entity types, not every one of which enabled for translation caused a fatal error since the translation handler class was not available for those.
  • Making field images untranslatable caused the images to be deleted due to the internal working of the File module.

Moreover:

  • I restored the "Users may translate this field" checkbox in case no data has been created for the field yet, since in this situation we don't need any bulk update.
  • I removed the automatic deletion of the entity translations when no translatable field is left: I don't think it's safe to this since with the new UI there might be other non-field translatable bits that might be negatively affected by this behavior.

I also changed some UI texts, trying to make them more consistent with the core UI. Finally I performed some cosmetic changes to make code suit my personal taste :)

I'd like to commit this in a few days and then start thinking about revisions. Let me know if everything looks ok.

plach’s picture

plach’s picture

Status: Needs work » Needs review
FileSize
15.02 KB
PASSED: [[SimpleTest]]: [MySQL] 60 pass(es). View
+++ b/entity_translation.admin.inc
@@ -520,3 +520,195 @@ function entity_translation_delete_confirm_submit($form, &$form_state) {
+      // old translation might occur before the new ones are stored.

"old translations"

+++ b/entity_translation.module
@@ -276,6 +299,10 @@ function entity_translation_permission() {
+    'change field translatability' => array(

This should be 'toggle field translatability'.

+++ b/tests/entity_translation.test
@@ -48,6 +48,7 @@ class EntityTranslationTestCase extends DrupalWebTestCase {
+        'change field translatability',

As above

+++ b/tests/entity_translation.test
@@ -147,11 +148,16 @@ class EntityTranslationTestCase extends DrupalWebTestCase {
+    $this->drupalGet('admin/config/regional/entity_translation/translate/body', array(
+      'query' => array(
+        'destination' => 'admin/structure/types/manage/page/fields/body/field-settings')
+      )
+    );
+    $edit = array(
+      'translatable' => 0
+    );
+    $this->drupalPost('admin/config/regional/entity_translation/translate/body', $edit, t('Convert'));
+    $this->assertRaw(t('Data successfully converted.'));

This does not work anymore.

Kristen Pol’s picture

I had a hard time finding this issue because of its title so I added a new issue and marked it as duplicate of this one:

#1413502: Fields show up empty/blank if field translation is turned on after field has content

I'll test the patch right now.

Kristen

Kristen Pol’s picture

FileSize
15.02 KB
PASSED: [[SimpleTest]]: [MySQL] 60 pass(es). View

The patch from #56 was using two different permission names:

change field translatability
and
toggle field translatability

Since the latter was used more, I've updated the patch to use that.

Kristen

Kristen Pol’s picture

FileSize
15.05 KB
PASSED: [[SimpleTest]]: [MySQL] 60 pass(es). View

It is very hard to notice this option because it is no longer a label and no longer bold. This patch makes the text bold. It still includes the permission name fix as well.

Kristen

Kristen Pol’s picture

It wasn't obvious to me how to test but I figured it out by looking at the code...

How to test patch from #59:

  1. apply patch
  2. flush caches
  3. update permissions so your user can "toggle field translatability"
  4. flush caches
  5. edit the field you want to translate (main edit tab, not field settings anymore)
  6. go to very bottom of form
  7. you will see: "This field is shared among the entity translations. Enable translation"
  8. click "Enable translation"
  9. wait for batch process
  10. done!
  11. note: once enabled, the text changes to: "Users may translate this field. Disable translation" so you can click "Disable translation" if you need to disable it

I tried out patch #59 and it works for me... but, I can't mark as reviewed & tested since I provided that patch.

I would *love* (love!) someone to try it out very, very soon and get it committed as I'm writing a book on D7 i18n for Packt right now and am almost done with the first draft (as in I'll be done tomorrow or the next day)! I currently have the old checkbox (on the field settings form) in the book exercises. I will need to update the book so that it uses this new method. Pretty please?

Thanks!
Kristen

plach’s picture

Status: Needs review » Needs work
+++ b/entity_translation.module
@@ -480,14 +507,39 @@ function entity_translation_edit_form_submit($form, &$form_state) {
+  $title = t('<strong>Users may translate this field.</strong>');

I don't think making this bold is the right thing to do here. AAMOF the checkbox title still gets a normal weight. Maybe we could introduce a <label/> element as a prefix in both cases.

Kristen Pol’s picture

Thanks for the fast response!

I guess it was more obvious before because it had the checkbox. I think we need something to make this more obvious. I'll look at the label option.

Thanks,
Kristen

plach’s picture

Just a note: the checkbox is still there, but it is showed only when the field has no data. In this case translatabilty can be toggled without having to update field languages.

Kristen Pol’s picture

FileSize
7.36 KB
15.07 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in sites/default/modules/entity_translation/entity_translation.module. View

The label looks good to me (better than the <strong>, definitely!). Here's a new patch and a screenshot of what the label looks like.

Kristen

Kristen Pol’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, et-translatable-switch-fix-permission-name-label.patch, failed testing.

Kristen Pol’s picture

Status: Needs work » Needs review

@plach - Thanks for the heads-up on the checkbox... I'll make sure to explain both methods.

Kristen

plach’s picture

Status: Needs review » Needs work

As I was saying above also the checkbox could use a Field translation label, at very least for consistency. Would you mind to create a new field and post a screenshot also for this case?

Kristen Pol’s picture

Status: Needs work » Needs review
FileSize
15.06 KB
PASSED: [[SimpleTest]]: [MySQL] 60 pass(es). View

Whoops... let's try that again.

Kristen

Kristen Pol’s picture

Status: Needs work » Needs review
FileSize
5.96 KB
15.13 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-translatable-switch-fix-permission-name-label-checkbox.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View

Ok, well, since there is a checkbox, that really needs the label too! So, sorry for the flurry of patches, but I think this one is the best.

[update: @plach - you are so responsive that I didn't even see your post before I update patch ;) That's a good thing!]

Kristen

Status: Needs review » Needs work

The last submitted patch, et-translatable-switch-fix-permission-name-label-checkbox.patch, failed testing.

Kristen Pol’s picture

Status: Needs review » Needs work

Hmmm... trying to figure out what it doesn't like.

Kristen

Kristen Pol’s picture

Status: Needs work » Needs review
Issue tags: -montreal

Status: Needs review » Needs work
Issue tags: +montreal

The last submitted patch, et-translatable-switch-fix-permission-name-label-checkbox.patch, failed testing.

Kristen Pol’s picture

Status: Needs work » Needs review
FileSize
15.13 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-translatable-switch-fix-permission-name-label-checkbox_0.patch. Unable to apply patch. See the log in the details link for more information. View

Status: Needs review » Needs work

The last submitted patch, et-translatable-switch-fix-permission-name-label-checkbox.patch, failed testing.

Kristen Pol’s picture

Status: Needs work » Needs review

Argh. It's complaining about line 337 which I didn't touch. Not sure what to do here. I didn't reroll the whole patch from scratch... just add the changes... which worked for the first rounds of patches. I'll see about rerolling the thing.

Kristen

Kristen Pol’s picture

Ok, so I've only done simple patches before... I'm assuming I need to follow the guide for using git for making patches?? Looked through it and it sounds fun... ha. The changes are trivial, but looks like I will be scratching my head for awhile trying to get a patch working... unless someone else wants to take a stab?

The changes are:

  • change
      $translatable = $field['translatable'];
      $title = t('Users may translate this field.');
    

    to

      $translatable = $field['translatable'];
      $label = t('Field translation');
      $title = t('Users may translate this field.');
    
  • change
        $form['field']['translatable'] = array(
          '#prefix' => '<div class="translatable">',
          '#suffix' => '</div>',
    

    to

        $form['field']['translatable'] = array(
          '#prefix' => '<div class="translatable"><label>' . $label . '</label>',
          '#suffix' => '</div>',
    
  • change
        $form['field']['translatable'] = array(
          '#type' => 'checkbox',
    

    to

        $form['field']['translatable'] = array(
          '#prefix' => '<label>' . $label . '</label>',
          '#type' => 'checkbox',
    
    

Kristen

plach’s picture

FileSize
15.13 KB
PASSED: [[SimpleTest]]: [MySQL] 60 pass(es). View
Kristen Pol’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @plach! I've been trying to figure out how to deal with making a "p1" patch... I don't have the "diff --git" option on my computer (still running an older version of Linux). I guess I will need to figure this out soon...

I have tested your patch against the latest dev version and it is working as expected:

  • If no data exists, checkbox is shown (tested enabling and disabling)
  • If data exists, link is shown (tested enabling and disabling)
  • "Field translation" label shows up for both of the above
  • If field has content and then field translation is turned on, content is not missing from edit form

Thanks!
Kristen

plach’s picture

Ok, I'll wait a couple of days before committing to give a chance to Thomas and Logan, who did most of the work here, to have a final review.

Kristen Pol’s picture

Great!

Thanks again,
Kristen

loganfsmyth’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/entity_translation.admin.inc
@@ -520,3 +520,195 @@ function entity_translation_delete_confirm_submit($form, &$form_state) {
+  $form_state['field'] = $field;

I feel like this is a bad way to do this. As I mentioned in comment #38 and 41, this separates the operation being performed at render time from the operation being performed at submission time. By simply storing the field object, and not having the 'translatable' option embedded in a hidden field on the page, you change the meaning of the form submission from 'Set Translatable' and 'Set Untranslatable' to 'Toggle Translatabliliy'.

In 99% of cases this is never going to be an issue, but I feel that it's silly just to ignore it, or remove the fix Thomas already had in there. As it is now, if someone opened that form twice then they would both say "Enable Translations" or something. If the user submitted one but forgot or something, then went to submit the second one, even though the page clearly says submitted will make the field translatable, submitting in fact would toggle the translatablility back to untranslatable.

+++ b/entity_translation.admin.inc
@@ -520,3 +520,195 @@ function entity_translation_delete_confirm_submit($form, &$form_state) {
+        $entity->{$field_name}[LANGUAGE_NONE] = $entity->{$field_name}[$langcode];
+        // Store the original value.
+        _entity_translation_update_field($entity_type, $entity, $field_name);
+        // Remove translations.
+        foreach ($entity->{$field_name} as $langcode => $items) {
+          if ($langcode != LANGUAGE_NONE) {
+            $entity->{$field_name}[$langcode] = array();
+          }
+        }
+        _entity_translation_update_field($entity_type, $entity, $field_name);

What is the motivation for running _entity_translation_update_field twice per entity? Seems like it should work fine with just one.

+++ b/entity_translation.admin.inc
@@ -520,3 +520,195 @@ function entity_translation_delete_confirm_submit($form, &$form_state) {
+    $entity->{$field_name}[$langcode] = _field_filter_items($field, $entity->{$field_name}[$langcode]);

When would invalid data be introduced in this workflow? I'm not the biggest fan of calling 'private' field API functions unless it is really needed.

getgood’s picture

The reason I chose to delete the translation handler, was that as long as it was around, I was getting odd behaviour when translating nodes.

When you go to translate a node and it has no translatable fields, there should be a message in the right hand column of "node/%/translate" saying "No translatable fields". When the entity has translatable fields, it should be a link reading "edit translations". Now, if you convert the field to translatable, add a translation, and then switch it back to untranslatable, the link in that column reading "edit translation" remains, but it now goes to a blank form because there aren't actually any translatable fields.

If we're not going to delete the handler, then we have to change how the form at /node/%/translate renders that link. I'm not entirely sure how this applies to more general entities, however.

plach’s picture

@loganfsmyth:

I feel like this is a bad way to do this [...]

I'd have to check, but if I'm not mistaken the form storage is cached though a build id, hence if a field definition is there, two form views/submits would be idempotent. AAMOF the $field variable would not be updated after the first submit, but would be retrieved from the form cache.

What is the motivation for running _entity_translation_update_field twice per entity? Seems like it should work fine with just one.

Files are lost when making (file) fields untranslatable because files are deleted when removing the field translations they belong to. Only afterwards the language neutral "translation" is created, but the files corresponding to set 'fid' do not exist anymore.

When would invalid data be introduced in this workflow? I'm not the biggest fan of calling 'private' field API functions unless it is really needed.

Apparently submitting empty file widgets result in non-empty invalid items: $item == array(array('fid' => NULL)).

plach’s picture

@getgood:

The reason I chose to delete the translation handler, was that as long as it was around, I was getting odd behaviour when translating nodes. [...]

As I was saying above, the new UI will behave differently and present a full entity form, with all the fields there, so this behavior would be more sensible. Moreover every entity translation has some metadata associated (author, creation, last change): silently deleting it might result in uninteded data loss.

If we're not going to delete the handler, then we have to change how the form at /node/%/translate renders that link

Just a note: it's not the translation handler that would be deleted, but an entity translation.

getgood’s picture

As long as that's being considered in the new UI I'm happy. Everything else looks reasonable to me.

loganfsmyth’s picture

@plach

I'd have to check, but if I'm not mistaken the form storage is cached though a build id

I wasn't 100% sure either, but I just checked to confirm. It does indeed toggle instead of set/unset. I think if we wanted caching then we'd have to set $form_state['cache'] = TRUE; but it's still adding an unneeded dependency on caching when we could just pass the data through the form.

I figured there was a reason for calling it twice and I can't think of a simpler way without fixing the File module.

Since we are only doing entity_load, where do the fid => NULL rows get in there? Weird, is this an issue with File?

plach’s picture

Status: Needs work » Needs review
FileSize
1.55 KB
15.62 KB
PASSED: [[SimpleTest]]: [MySQL] 60 pass(es). View

but it's still adding an unneeded dependency on caching when we could just pass the data through the form.

Makes sense. Sorry for missing this.

Since we are only doing entity_load, where do the fid => NULL rows get in there? Weird, is this an issue with File?

Honestly no idea. Probably it's a File module issue, but I'm not very keen to fix the third core bug just to get through this issue :)

loganfsmyth’s picture

Assuming this all passes, I'm happy.

plach’s picture

Status: Needs review » Fixed

Committed and pushed to the master branch!

Thanks to everyone working hard here, especially Thomas and Logan: awesome work, guys!

Kristen Pol’s picture

Yeah!! Thanks everyone!

Kristen

loganfsmyth’s picture

Wooo!

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

  • Commit bc8506c on master, et-permissions-1829630, factory, et-fc, revisions authored by getgood, committed by plach:
    Issue #1279372 by getgood, loganfsmyth, evolvingweb, plach, Kristen Pol...

  • Commit bc8506c on master, et-permissions-1829630, factory, et-fc, revisions, workbench authored by getgood, committed by plach:
    Issue #1279372 by getgood, loganfsmyth, evolvingweb, plach, Kristen Pol...
andrezstar’s picture

sorry duplicated post