Enable bulk field language updates when switching field translatability

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)

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.

working on it.

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.

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.

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.

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.

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.

didn't mean to change the status...

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

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...

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

BTW awesome review.

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.

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.

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.

For testing.

* 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?

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.

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

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

if (!$count) {

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.

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.

Forgot to pull first. Try again.

#31: et-translatable-switch-1279372-31.patch queued for re-testing.

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

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.

Clean up as per Logan's comments.

+++ 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.

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');

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.

#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.

I think I've covered everything Logan said.

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

+++ 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.

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.

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?

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

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

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

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

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

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

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

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

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

Kristen

Whoops... let's try that again.

Kristen

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

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

Kristen

#70: et-translatable-switch-fix-permission-name-label-checkbox.patch queued for re-testing.

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

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

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

Great!

Thanks again,
Kristen

+++ 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.

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.

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

@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?

Assuming this all passes, I'm happy.

Yeah!! Thanks everyone!

Kristen

Wooo!

sorry duplicated post