Problem

We have an inconsistent behavior when initializing the field 'translatable' property all around core: the most relevant difference is between the default value provided by field_create_field() (FALSE) and the default value provided by the Field UI (TRUE). This leads to a scenario where all fields created programmatically without specifying the field's translatability are non translatable, while all fields created by the Field UI are translatable. A side problem is that it is not possible to change the default value provided by the Field UI while creating the field, as this is hard-coded in the submit handler.

This would be a major concern but is mitigated by the fact that if there is no module enabled capable to handle field languages (a translation handler), translatable fields behave as non translatable. Nonetheless this should be fixed because once a translation handler is enabled this inconsistent behavior may manifest itself. At the moment the only known translation handlers are Locale (core) and Entity translation (contrib), and only the latter allows to change field's translatability and thus fix this inconsistency.

The only thing that Locale does as a field translation handler is assiging a language to node fields matching the node language. This can lead to field rendering/editing issues (#1205504: Uninstalling Content Translation disassociates fields), if Locale is disabled after creating content, but this was a design decision to avoid to mess with field languages in a potentially data-disruptive way. We simply decided to require people either to leave Locale enabled or to manipulate their database manually, not an ideal solution but this is what we could come up with in the timeframe we had.

Provided that Locale is enabled, as it provides language fallback (field values are always showed even if they do not match the current language), field translatability can be changed at any time, so this inconsistency can be fixed through contrib modules such as the aforementioned Entity translation. However we should try to ensure that new sites do not incur in it, hence the major priority.

Note: the fix we came up with (see the description below) introduced a serious regression in the D6 upgrade path since we made a double mistake: first we changed node_update_7006 instead of introducing a new update function, second we introduced only a partial change thus making node bodies for sites upgraded from D6 to D7.7 disappear. They were not lost, they just were not displayed due to a mismatch between language and translatability. This is fixed in the 7.x branch.

Proposed resolution

There is consensus (#7, #8, #9) that the right way to solve this is defaulting to FALSE everywhere. This is issue has been generated by the fact that ET was originally planned to be part of core: provided that we don't have anything in core that actually exploits field language support, defaulting to FALSE sounds as the sanest thing we can do. This should bring also a little performance improvement for sites not having translation handlers enabled (#654934-14: Translatable option on field creation).

The tricky part is how to deal with existing sites: since translatability can be changed in any moment, originally we agreed that only sites that never dealt with field languages should get a fix, this means sites with no translation handler enabled and no field value in the database with a language assigned. This could be checked in a field-storage-safe fashion through EntityFieldQuery. However since update functions cannot rely on entity information (#1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed) and they cannot cause hooks to be invoked, there is no way to implement a perfectly working update function. We decided to fall back to an update function only checking if Locale is enabled (since the only other known traslation handler depends on it), in which case no change to field's transltability should be performed.

This means only new sites will get the fix, old sites having Locale enabled will still have to live with the inconsistent behavior unless they manually apply the fix or install a contrib module, such as ET, allowing them to change field translatability.

Note: to fix the regression introduced with the first commit, we decided to provide two update functions. The first one is the one that should have been part of the initial patch: it changes the behavior of node_update_7006 in a safe way by switching the body translatability and languages only if the update is run during an upgrade from D6. The second update is meant to fix D6 sites that upgraded to D7.7 by detecting body fields having only one single language being different from LANGUAGE_NONE and switching them to LANGUAGE_NONE. Fields with more than one language value are left alone since they are likely to be correct values or manually fixed ones. This is fixed in the 7.x branch.

Remaining tasks

  • The initial solution has been committed without the related update. This should be ready but we need test coverage for it, which is not available at the moment; the related issue is postponed to #1182290: Add boilerplate upgrade path tests. We have now a dedicated issue for this.
  • The fix for the regression introduced with the first commit is tested, test-covered and should be more or less ready to go. This is fixed in the 7.x branch.
  • The fix for the D6 sites upgraded to D7.7 also needs tests, hence it is postponed to #1182296: Add tests for 7.0->7.x upgrade path (random test failures) too.
    This has been deemed unecessary since upgrade instructions require users to backup their DB and it's enough to roll it back and upgrade to D7.8 to recover from the regression. This allows us to avoid to maintain two distinct upgrade paths. Therefore D7.7 should be considered abandoned, at least for what pertains the upgrade path.

User interface changes

No changes.

API changes

The are no proper API changes introduced by the patch, since the Field UI behavior has been changed to match the Field API one. However after committing it, it would be possible to change the Field UI behavior through a hook_form_alter() implementation.

Original report by KarenS

I have been working with upgrades from D6 to D7 and have been running into inconsistencies and problems with the way that fields are marked (or not marked) as translatable.

First, there is an inconsistency between the default used by the Field API and the Field UI. One defaults 'translatable' to FALSE, the other to TRUE.

The API at least can be overridden to change the value, the UI cannot, and as written will make every field translatable. There is no possibility for any module to override this, other than by coming in after the field has been created and altering it.

field.crud.inc

function field_create_field($field) {
...
  $field += array(
    'entity_types' => array(),
    'cardinality' => 1,
    'translatable' => FALSE,
    'locked' => FALSE,
    'settings' => array(),
    'storage' => array(),
    'deleted' => 0,
  );
field_ui.admin.inc

function field_ui_field_overview_form_submit($form, &$form_state) {
...
  // Create new field.
  $field = array();
  if (!empty($form_values['_add_new_field']['field_name'])) {
    $values = $form_values['_add_new_field'];

    $field = array(
      'field_name' => $values['field_name'],
      'type' => $values['type'],
      'translatable' => TRUE,
    );

Next we have the problem of what has happened to fields that are migrated from D6, since all the upgrade code uses the API. The field module contains a wrapper function for creating fields that looks like the following. Note that it again defaults the value for 'translatable' to FALSE. So all fields migrated from D6 will end up with 'translation' set to FALSE, while any new fields added in the UI will end up with 'translatable' set to TRUE, resulting in an odd mixture where fields of the same type are sometimes translatable and sometimes not.

field.install

function _update_7000_field_create_field(&$field) {
  // Merge in default values.`
  $field += array(
    'entity_types' => array(),
    'cardinality' => 1,
    'translatable' => FALSE,
    'locked' => FALSE,
    'settings' => array(),
    'indexes' => array(),
    'deleted' => 0,
    'active' => 1,
  );

That wrapper function was used in numerous field upgrades. The node body field uses it, but that update specifically sets 'translatable' to TRUE, so migrated node bodies get properly created as being translatable ('properly' both because it matches the UI behavior and because they really ought to be translatable).

node.install

function node_update_7006(&$sandbox) {
...
    $body_field = array(
      'field_name' => 'body',
      'type' => 'text_with_summary',
      'module' => 'text',
      'cardinality' => 1,
      'entity_types' => array('node'),
      'translatable' => TRUE,
    );
    _update_7000_field_create_field($body_field);

But all the other core field upgrades end up with 'translatable' as FALSE. This includes upgrades of comment bodies, file fields, and taxonomy fields. Comment bodies, at least, should be translatable. You could make a case that file fields should not be translatable, but any file fields added later in the UI will end up being translatable.

taxonomy.install

function taxonomy_update_7004() {
.....
  foreach ($vocabularies as $vocabulary) {
    $field_name = 'taxonomy_' . $vocabulary->machine_name;
    $field = array(
      'field_name' => $field_name,
      'module' => 'taxonomy',
      'type' => 'taxonomy_term_reference',
      'cardinality' => $vocabulary->multiple || $vocabulary->tags ? FIELD_CARDINALITY_UNLIMITED : 1,
      'settings' => array(
        'required' => $vocabulary->required ? TRUE : FALSE,
        'allowed_values' => array(
          array(
            'vocabulary' => $vocabulary->machine_name,
            'parent' => 0,
          ),
        ),
      ),
    );
    _update_7000_field_create_field($field);
   
   ....
   
  $field_name = 'taxonomyextra';
  $field = array(
    'field_name' => $field_name,
    'module' => 'taxonomy',
    'type' => 'taxonomy_term_reference',
    'cardinality' => FIELD_CARDINALITY_UNLIMITED,
    'settings' => array(
      'required' => FALSE,
      'allowed_values' => $allowed_values,
    ),
  );
  _update_7000_field_create_field($field);
comment.install

function comment_update_7005() {
...
  $field = array(
    'field_name' => 'comment_body',
    'type' => 'text_long',
    'module' => 'text',
    'entity_types' => array(
      'comment',
    ),
    'settings' => array(),
    'cardinality' => 1,
  );
  _update_7000_field_create_field($field);
system.install

function system_update_7060() {
...
    $field = array(
      'field_name' => 'upload',
      'type' => 'file',
      'module' => 'file',
      'locked' => FALSE,
      'cardinality' => FIELD_CARDINALITY_UNLIMITED,
      'translatable' => FALSE,
      'settings' => array(
        'display_field' => 1,
        'display_default' => variable_get('upload_list_default', 1),
        'uri_scheme' => file_default_scheme(),
        'default_file' => 0,
      ),
    );

....
    // Create the field.
    _update_7000_field_create_field($field);

We obviously need to clear up these discrepancies. There are several questions to resolve:

1) What should be controlling whether a field is translatable or not? Currently it is decided either by the UI or by the code that creates the field. It makes more sense to me to have the field itself declare whether or not its fields should be translatable. That is probably an API change, so I don't hold out much hope of getting it in, but I think it is the right answer. Without this change we end up with silly things like numbers, files, and dates being marked 'translatable'.

2) Assuming we cannot make the API change above, we need a sensible, and consistent, default. It probably makes sense to default 'translatable' to TRUE everywhere, in the absence of any information from the field about the nature of the data it holds.

3) The UI 'translation' value needs to be made overridable. At the very least a drupal_alter. But then we also need to consider if some core fields (like file fields) should be using that to make sure that their fields created in the UI are left as non-translatable.

4) Once we figure out what values *should* have been created, all the field updates need additional updates to scrub the wrong values that are there now.

CommentFileSizeAuthor
#144 tf-1164852-144.patch9.69 KBplach
#144 tf-1164852-144-test.patch6.29 KBplach
#143 tf-1164852-143.patch8.43 KBplach
#135 tf-1164852-135.patch8.59 KBplach
#132 tf-1164852-132.patch8.62 KBplach
#125 drupal-translatable_node_field_upgrade-124.patch14.89 KBBoobaa
#118 drupal-translatable_node_field_upgrade-03-test_and_d77d78-118.patch12.08 KBBoobaa
#118 drupal-translatable_node_field_upgrade-04-test_and_everything-for_webchick_to_commit-118.patch12.99 KBBoobaa
#109 drupal-translatable_node_field_upgrade-01-only_tests.patch5.49 KBBoobaa
#109 drupal-translatable_node_field_upgrade-02-test_and_d6d78.patch8.85 KBBoobaa
#109 drupal-translatable_node_field_upgrade-03-test_and_d77d78.patch9.65 KBBoobaa
#109 drupal-translatable_node_field_upgrade-04-test_and_everything-for_webchick_to_commit.patch10.56 KBBoobaa
#104 drupal.system-update-module-enable.104.patch7.98 KBsun
#103 drupal.system-update-module-enable.103.patch2.82 KBsun
#100 drupal.system-update-module-enable.patch910 bytessun
#99 drupal-translatable_node_field_upgrade-03-test_and_d77d78.patch9.39 KBBoobaa
#97 drupal-translatable_node_upgrade-tests_d6d7_d77d78-97.patch10.46 KBBoobaa
#96 drupal-translatable_node_upgrade-tests_only-96.patch5.49 KBBoobaa
#96 drupal-translatable_node_upgrade-tests_and_patch-96.patch8.85 KBBoobaa
#91 drupal-translatable_node_upgrade-tests_only-91.patch5.48 KBBoobaa
#91 drupal-translatable_node_upgrade-tests_and_patch-91.patch8.84 KBBoobaa
#81 drupal-field_translatable_update-81.patch3.37 KBBoobaa
#78 drupal-field_translatable_update-78.patch907 bytesBoobaa
#70 drupal.field-translatable-update.70.patch2.29 KBsun
#68 drupal.field-translatable-update.68.patch2.3 KBsun
#66 drupal.field-translatable-update.66.patch2.37 KBsun
#65 drupal.field-translatable-update.65.patch1.91 KBsun
#64 tf-1164852-64.patch1.69 KBplach
#60 tf-1164852-57.patch1.54 KBplach
#42 tf_update-1164852-42.php_.txt1.83 KBplach
#42 tf_reset-1164852-42.php_.txt969 bytesplach
#41 tf-1164852-41.patch406 bytesplach
#40 tf_update_1164852.php_.txt1.69 KBandypost
#37 tf_update_1164852.php_.txt1.66 KBplach
#35 tf-1164852-34.patch7.39 KBplach
#27 tf-1164852-27.D7.patch9.69 KBplach
#26 tf-1164852-26.D7.patch9.22 KBplach
#22 tf-1164852-22.D7.patch8.66 KBplach
#21 tf-1164852-21.patch7.39 KBplach
#16 tf-1164852-16.patch1.74 KBplach
#11 shotoftable_field_data_field_imagefield.JPG130.2 KBjoris.verschueren
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

NancyDru’s picture

Thank you , Karen for bringing this to light. Since I have never created a multi-lingual site, I have to defer to those who have when the field is indeed translatable. However, I hate the idea of having even a small amount of overhead added to my sites that are never going to be translated. So my opinion is that the field should be non-translatable by default, whatever the path of creating the field.

As for a possible API change after launch, remember 6.2 that changed the menu API?

Gábor Hojtsy’s picture

Yes, I think this is a missing piece in the API. I was unfortunately not well involved in the process of building this tool (will try to get some attention to this issue from people who where), but my understanding is that once you install entity_translation module, fields default to non-translatable (like node types are with the core translation module), and you specify which are in fact translatable. I think that is a saner default. Until you actually install a UI like that for translating your fields, I don't think it is sensible to mark them translatable, is it?

plach’s picture

Issue tags: +translatable fields

First of all, let me say I think that we should always default to FALSE everywhere. That said I have some remarks:

1) What should be controlling whether a field is translatable or not? Currently it is decided either by the UI or by the code that creates the field. It makes more sense to me to have the field itself declare whether or not its fields should be translatable. That is probably an API change, so I don't hold out much hope of getting it in, but I think it is the right answer. Without this change we end up with silly things like numbers, files, and dates being marked 'translatable'.

No, this is totally against the original design: perhaps the term 'translatable' may be misleading, but it means that the field may have different values for each available language, it does not mean the field type is inherently translatable. A file field may well be translatable as I might want to upload a different PDF for each language. In the same way I might want to have different dates for different language versions of the same content, say I'm storing when a translation has been modified the last time. The problem here is that whether makes sense to mark a field translatable totally depends on the use cases.

3) The UI 'translation' value needs to be made overridable. At the very least a drupal_alter. [...]

I ain't sure we actually need a drupal_alter, why a form alter wouldn't be enough?

4) Once we figure out what values *should* have been created, all the field updates need additional updates to scrub the wrong values that are there now.

Sure.

my understanding is that once you install entity_translation module, fields default to non-translatable (like node types are with the core translation module), and you specify which are in fact translatable. I think that is a saner default. Until you actually install a UI like that for translating your fields, I don't think it is sensible to mark them translatable, is it?

Totally agreed, see #654934-14: Translatable option on field creation.

KarenS’s picture

I do not profess to understand the translation system at all. I definitely did not understand what 'translatable' was intended to mean. It helps to understand that.

So as a start, it looks like everything should definitely default to untranslatable if Content Translation is not enabled? But if Content Translation is enabled, what values should be used by any module that is programatically creating fields, and by Field UI? Is there a way for them to know or should they make all fields untranslatable and assume some process will come in after and fix that?

And in the case of my field migration where I have existing fields and data -- it may have been created using some translation system, or may not (I don't know how I could tell). How should those fields be created and what value should go into the language field?

KarenS’s picture

Also the reason I thought drupal_alter was needed instead of form_alter, is because the 'translatable' value is being set in the form submit function and the field is immediately created afterward. So the only way to override it now is to bypass that submit function somehow.

plach’s picture

Well, IMO we have to keep in mind two things:

  1. Switching a field's translatablity should cause no problem at all provided that the Locale module is enabled; AAMOF it provides language fallback, i. e. if a field is not available in the current language it is displayed in another available one. For instance, if a field value is created as untranslatable it has language 'und', if then it's made translatable it will be showed anyway, see field_language. For this reason I don't think we should bother to introduce a drupal_alter for the Field UI code. Translatability can be changed subsequently if needed.
  2. So as a start, it looks like everything should definitely default to untranslatable if Content Translation is not enabled? But if Content Translation is enabled, what values should be used by any module that is programatically creating fields, and by Field UI? Is there a way for them to know or should they make all fields untranslatable and assume some process will come in after and fix that?

    As I said, I think we should always stick with translatability set to FALSE, at least for performance reasons. The Entity translation module could provide a page where setting Fields's translatability en masse. Content translation performs the D6-style node-based translation and does not care about field's translatability. Locale does: it registrates itself as a translation handler, i.e. a module capable of handling language-aware fields. The alternative here could be to set the default value based on the availablity of a translation handler, see field_has_translation_handler. Gabor?

And in the case of my field migration where I have existing fields and data -- it may have been created using some translation system, or may not (I don't know how I could tell). How should those fields be created and what value should go into the language field?

I think it should be safe to create all fields as non translatable, since the field language concept has been introduced in D7, existing sites and the deployed code should keep working as fields didn't have a language associated. However this could always be changed afterwards, as I said before.

Interested people can have a look to the Field language API overview for more details ;)

webchick’s picture

Issue tags: +Needs backport to D7

Tagging this, because after talking to plach on IRC, it's likely we need to do something with this in D7, too...

Something we talked about was switching the default to FALSE, since core doesn't ship with any mechanism to deal with translatable fields, so that's a much saner default. An upgrade path could check to see if there's a translation handler enabled for the entity to which the field belongs, and if so, don't change it. Else, switch it to FALSE.

This might be a terrible idea to do in a stable release (I need some help from the i18n team in understanding the implications) but it certainly makes logical sense, and seems relatively risk-free, if we do enough testing.

plach’s picture

Version: 7.x-dev » 8.x-dev

Just an additional note on this: if code is using the right way to check if a field is translatable, on sites not having translation handlers enabled switching to FALSE should make no difference since fields are already untranslatable.

Gábor Hojtsy’s picture

Yes, just like @plach, I think it would be safe to change field translatability on sites where no field translation modules are installed/enabled.

joris.verschueren’s picture

First of all, thanks to the people who keep digging at this.
I had problems with imagefield upgrades on a multilanguage site, upgrading from D6 to D7.
I must admit that most of the technical discussion above exceeds my understanding of how the modules actually work. I post here also because of KarenS's call in #1063852: D6 > D7 non-english migration bug to centralise bug reports here. I thought this description might document the coming about of (possible) inconsistencies.

I have English and Dutch (code nl) enabled on my site.
I first migrated from Image module to imagefield module (6.x-3.10) on my D6 site, using the script in this issue queue #201983: How To: Migrate from Image.module to ImageField Documentation Project. About half of my image nodes were dutch, the other half was language neutral. The migration script requires a new node type (image2) and a filefield to contain the image (imagefield). I created this field without adding any custom attributes.
This migration worked flawless, image nodes were converted into nodes of this new type (image2) with an imagefield. I haven't found out in which table the language attributes of the converted image nodes were stored.

After upgrading from D6 to D7 (CCK and content migrate modules 7.x-2.x-dev), only the images that stem from a language neutral image node were shown. In the field_data_field_imagefield table, these correspond to 'data' that have 'und' as 'language for this data item'. Manually changing the language from 'nl' to 'und' in this table didn't solve anything.
I returned to my D6 site, set all image nodes as language neutral and went through the migration and upgrade again. Now all data contained in the image fields on the image2 nodes have 'und' as their language and are shown on my D7 site. The problem (logically) persists for what were previously attached images. These still don't show on their nodes.

Strange thing is that the language problems are somehow inherited from nodes, and that the imagefields were never meant to have a language or to be translatable.

I personally don't even care for the language of the images themselves, but I can't of course set all my nodes to 'language neutral' before migrating. The image nodes did only get their language because dutch is set as the default language for my site.
I personnally could do with a workaround (stripping attached images of language attributes), but I hope this finds a proper solution. It's heartening to see the endeavours that are made to make this work.

plach’s picture

#11 seems to suggest that

I think it should be safe to create all fields as non translatable, since the field language concept has been introduced in D7, existing sites and the deployed code should keep working as fields didn't have a language associated.

should be the right way

steinmb’s picture

Hmmm, tricky, but setting all fields as non translatable should be safe? Anyone talked to the i18n team? Subscribing pondering.

Gábor Hojtsy’s picture

@steinmb: yes, @plach and me are from that team :)

steinmb’s picture

LOL, sorry about that. Did not check the i18n-list before posting, and I was expecting a more "On behalf of the i18n, do we recommend ..." —kind of comment :)

plach’s picture

Assigned: Unassigned » plach
Status: Active » Needs review
FileSize
1.74 KB

Here is a patch for D8: in the Field UI the translatable value is now set to FALSE as a form value so that a hook_form_alter can change it. Working on the update function for D7. Tests are still to be fixed.

Status: Needs review » Needs work

The last submitted patch, tf-1164852-16.patch, failed testing.

KarenS’s picture

FYI, the Content Migrate code has been changed to set all migrated field values as 'und'. I am assuming, based on this discussion, that if anything else needs to be done with those values one of the translation modules will take care of it.

plach’s picture

@KarenS:

Nice! Probably you already know but, just to be sure, there is the LANGUAGE_NONE constant defined for 'und'.

Gábor Hojtsy’s picture

Issue tags: +D8MI

Looked at the patch, it looks good on a visual review. Needs to pass test of course :)

plach’s picture

Status: Needs work » Needs review
FileSize
7.39 KB

The attached patch should fix tests. 7.x update yet to be written.

plach’s picture

FileSize
8.66 KB

Here is the update function. It might not work if for any reason the update is performed with locale disabled and then re-enabled. Initially I tried to introduce a check for every field to verify that it does not have language-aware values, but this turned out to be impossibile when translation handlers are not available, since the field storage won't load those values as they are supposed to be invalid. We cannot directly query the db since we might have different storage engines in use.

Perhaps we should simply skip the update function and let current installations live as they did since now.

plach’s picture

After fixing this, things like #1205504: Uninstalling Content Translation disassociates fields should stop happening.

plach’s picture

Spoke with @chx and @DamZ about this, the proper solution would be #1206200: Add support for field meta conditions in EntityFieldQuery.

renat’s picture

Subscribe

plach’s picture

FileSize
9.22 KB

Here is an updated D7 patch, exploiting the field meta conditions introduced in #1206200: Add support for field meta conditions in EntityFieldQuery. The patch obviously depends on that one.

I'm working on a simpletest to check that the update function is correct, but we don't have anything like this in core atm (we only test D6 > D7 upgrades), so this is experimental and I might not get through it.

Please review! The patch must be RTBC by tomorrow to be committed as @webchick wants to have some days during which people can test the dev version and we absolutely need to avoid to wait for another release, as more sites could evolve in a way that makes impossible for the update function to perform its task (Locale or ET being enabled, custom modules assigning languages to fields, ...).

plach’s picture

FileSize
9.69 KB

I missed one translatable = TRUE occurrence in node.install. The 8.x patch is still #21.

sun’s picture

We cannot directly query the db since we might have different storage engines in use.

Alternative field storage engines are supported at the API-level, but not at the update-level.

We already went through this drama in #934050: Change format into string, #937442: Field type modules cannot maintain their field schema (field type schema change C(R)UD is needed), and #937554: Field storage config entities 'indexes' key, so there's no need to iterate over all of this again. Don't look at me; it's illogical. However, the core developer front that is actively working with and on support of alternative field storage engines collectively stated that maintenance updates for fields are not supported in D7.

Field updates are limited to field_sql_storage and execute regular SQL queries. An example is text_update_7000().

Even with #1206200: Add support for field meta conditions in EntityFieldQuery, you may be able to retrieve the entity IDs that contain affected fields, but you are not able to load and save those entities in an update function, because these operations rely on module hooks, and module updates must not invoke hooks.

plach’s picture

@sun:

Alternative field storage engines are supported at the API-level, but not at the update-level.

Nobody told me this before, however now EFQ should support it, and there is no harm in doing that, I guess.

Even with #1206200: Add support for field meta conditions in EntityFieldQuery, you may be able to retrieve the entity IDs that contain affected fields, but you are not able to load and save those entities in an update function, because these operations rely on module hooks, and module updates must not invoke hooks.

The patch is only running count queries to see if any field value with a language exists in the DB, in which case it skips all the pending updates. Nothing else, no saves, no hook invocations. Again, I think this is more than what we usually guarantee, so this should be totally fine.

Gábor Hojtsy’s picture

The patch (minus the update) looks good. The update in my reading changes any field only if there are not any fields on the site whatsoever which have language values in any of their instances. This means if you had even one field that has language, you'll not get any changes. Which sounds reasonable to me. All-in-all patch looks good on a visual review.

sun’s picture

Status: Needs review » Needs work
+++ b/modules/field/field.install
@@ -435,5 +435,53 @@ function field_update_7001() {
+function field_update_7002() {
...
+              field_update_field($field);

field_update_field() invokes a full range of hooks down the line.

We either need to update the field config table directly, or need a new _update_7000_field_update_field() helper function.

+++ b/modules/field/field.install
@@ -435,5 +435,53 @@ function field_update_7001() {
+  $field_info = field_info_fields();

Most likely needs to be replaced with the existing _update_7000_field_read_fields()

+++ b/modules/field/field.install
@@ -435,5 +435,53 @@ function field_update_7001() {
+    foreach (entity_get_info() as $entity_type => $info) {

entity_get_info() is purely based on hook implementations. No idea how to handle this here. But most probably, we need to kinda reverse the entity type selection by selecting all entity types actively being used from the field config table.

8 days to next Drupal core point release.

catch’s picture

So it's impossible to run this update safely without entity_get_info().

It is impossible to run entity_get_info() during updates, because it relies on hook invocations, which cannot be fired during hook_update_N().

This is a catch 22, so we will just have to leave existing sites inconsistent until #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed gets resolved.

However nothing stops existing sites running this code themselves as a workaround, and we should fix the bug for new sites ASAP. Plach says #21 applies cleanly to both versions, so let's go with that for now, and one day fix the update system to it can be used to actually update things.

plach’s picture

After talking in irc with @sun and @catch it seem there is no way to perform an update relying on entity info:

[15:11] catch plach: if you just skipped the update entirely, what would happen?
[15:11] plach catch: it seems the only hook invoked in EFQ in hook_entity_query_alter(), perhaps we can make it optional
[15:12] sun I may be mistaken wrt to EFQ, but the field storage engine itself is a hook
[15:12] plach catch: not a great deal, more sites won't get the fix, I think perhaps multisites might have problems as new installations of the same codebase might behave differently
[15:12] catch plach: on the update issue, all I can see is http://drupal.org/node/1199946
[15:12] Druplicon http://drupal.org/node/1199946 => Disabled modules are broken in every possible way => Drupal core, base system, major, active, 14 comments, 2 IRC mentions
[15:12] catch s/see/say
[15:13] catch plach: I would repost the patch with no update, and link to that. There is no way on earth to do an update that depends on entity info.
[15:14] catch Well nothing stops people doing it in contrib but core has given up at this point.

Gábor Hojtsy’s picture

Well, that sounds pretty broken but understandable based on the architecture of the field API and the assumptions we should not make in updates. Looks like it is indeed best to concentrate on getting the code fixed for new sites at least, and then deal with the update later. It will be able to use the same conditions applying to sites, but will have no work on new sites which are fixed at least.

plach’s picture

Issue summary: View changes

Issue summary

plach’s picture

Status: Needs work » Needs review
FileSize
7.39 KB

So it seems we need to go on without an update. Here is the patch from #21 rerolled. It applies cleanly to D8 and D7. I'll post a procedure to perform the update manually for people wishing to.

Updated the issue summary accordingly.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Looks like sun and Gabor are happy apart from the update which we've now quietly forgotten about. Patch looks fine to me as well.

plach’s picture

FileSize
1.66 KB

Here is an "unofficial" update script implementing what originally planned for the update function, can be run via web or via drush scr. It still relies on #1206200: Add support for field meta conditions in EntityFieldQuery. A complete manual procedure for sites having a translation handler enabled or field languages present in the DB is coming later.

fago’s picture

@update:
Imho the problem is we need to separate our schema-updates and our data-updates. We have fine system for schema-updates, but for data updates we need to be able to use our APIs - thus data-updates can only run when the schema-updates passed.

@issue:
Patch makes much sense to me, and helps with the related issue #1208856: Enabling locale module changes field default language I opened recently.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

Hm. If that manual thing is in reference to our IRC conversation the other day, I didn't mean providing a write-up for how people can do the actual field conversion manually, I meant more providing a write-up around "things you should test in 7.x-dev so that we know 7.5 will be solid and not break international/english sites" so we can rally some testing power around this in the next few days.

But from talking to Gábor and catch tonight, sounds like not much at all should be affected by this change, beyond new sites/fields being consistent. Most D7 sites are already probably in this mixed-mode because of the API vs. UI thing, so there's probably not much likely to break (and what does break is likely going to be ET's responsibility to allow to fix).

It really sucks that we can't do the upgrade path as a hook_update_N() though. :( This janky manual script thing is not going to fly for future patches; we need to come up with some other solution. I pressed catch on this point, and he basically laid out a scenario where you update the code of an entity-providing module (let's say commerce) whose hook_update_N() happens to include a schema change, as well as core to do the 7.5 update. When you run update.php, if it's attempting to run hooks, you could cause errors on your site from attempting to update columns that are no longer there, etc. This seems like kind of a far-off concern for *this* particular patch, since the "language" field is a "meta" field built into Field API. However, I suppose it's still possible for an unrelated schema change to cause issues. Hrm.

Anyway, it's definitely better to solve this sooner than later. I'm really very sorry plach for causing you to go on such a wild goose chase. :(

Committed and pushed to 8.x and 7.x. I guess we'll need to reference that update script in the release notes for people.

I think this is worthy of a change notification: http://drupal.org/node/add/changenotice Marking needs work.

andypost’s picture

FileSize
1.69 KB

This script could be run by drush scr tf_update_1164852.php without notices

plach’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs work » Needs review
FileSize
406 bytes

While removing the update function I reverted also a needed change in node.install: node bodies are still upgraded as translatable :(

plach’s picture

In the change record I mentioned the patch above, which I believe does not need discussion as it was part of previously reviewed patches.

Attached there are two "unofficial" update scripts: one is the update script already posted with some corrections to fix notices while running it through drush and improving performance (thanks chx!); the other is a brute force reset script which simply remove any field language support, making everything untranslatable.

plach’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

Yep #41 is trivial.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Committed and pushed #41 to 7.x.

Not... sure what to do with this now. Needs review for the upgrade scripts?

catch’s picture

Status: Needs review » Fixed

Since the docs are done, I think we should just move this to fixed. The manual upgrade scripts could be moved to a new issue (we probably need a major issue for the fact they have to exist at all but there are probably existing ones this falls under like documenting the _update_1234() functions and disabled modules).

webchick’s picture

Status: Fixed » Needs review

Well. I still need to know which of the now 3 upgrade scripts we have here that I should point people at. It should probably also be documented at http://drupal.org/node/1225062.

catch’s picture

Title: Inconsistencies in field language handling » Manual updates for inconsistencies in field language handling

OK let me do this so I don't keep thinking this isn't in yet though.

plach’s picture

@webchick:

I think people should choose between the two in #42: the update script is for people not being sure if they are using field languages, while the reset script is for people not wishing to deal with field language but having, say, Locale enabled to translate their interface.

webchick’s picture

Plach, could you please check over / fix / add the following text to the bottom of the change node at http://drupal.org/node/1225062/edit ? Then I think we can mark this fixed.


<h3>Upgrade path</h3>
If you have a Drupal 7 site pre-dating the 7.5 release on July 27, it may be desirable to run one of the following scripts so that your pre-existing data conforms.

<ul>
<li>If you have a pure English site which has never made use of Drupal's multilingual capabilities, use the script at  http://drupal.org/files/issues/tf_reset-1164852-42.php_.txt which will set the translate flag for all fields to FALSE.</li>
<li>If you have a multilingual site, or non-English monolingual site, use the script at http://drupal.org/files/issues/tf_update-1164852-42.php_.txt to set the FALSE flag only on fields not currently handled by a module such as Entity translation.</li>
</ul>

To use either script:

1. Download and rename its extension from .php_.txt to php.
2. Place into the script your Drupal root directory.
3. Execute it either by navigating to the URL http://example.com/tf_XXX-1164852-42.php or with the <c   o   d e >drush scr</ c o d e> command.
4. Check your logs at http://example.com/admin/report/dblog to ensure the script ran properly.
webchick’s picture

Ugh. It made me completely throw up in my mouth to write that. :(

Guys, why can't field we make this a regular, standard, non-completely-bizarre hook_update_N(), and direct field modules that are attempting to do schema updates at the same time to throw in a hook_update_dependencies()?

Gábor Hojtsy’s picture

@webchick: would that mean that for all the time that D7 updates are supported, each field update would then need to depend on this one in all core and contrib field related modules?

chx’s picture

All the usual caveats about broken-nonupdated data striuctures do not apply to a minor update. So we probably can survive with http://drupal.org/files/issues/tf_update-1164852-42.php_.txt as a 7.x update hook but under no circumstances this can get into 8.x. Before going ahead to make it system_update_7000 or whatnot, get a green light from DamZ as well.

catch’s picture

Doing that could affect at least the following kinds of modules:

anything implenting field info and alter hooks, or field crud hooks (update field).

Any module providing entity types where bundles are configured in the db or rely on the db in some other way, that includes node, taxonomy and comment in core.

Any module implementing entity info alter, like rdf, again if the altering is based on the db in some way.

Additionally it is going to make core inconsistent in terms of whether updatei functions invoke hooks or not, when imposing this limitation was the only thing that got us close to a working upgrade path in D7. I say close because it still has critical and major bugs that are unresolved with new ones being opened faster than the old ones get closed.

catch’s picture

@chx this is not a minor update since it can easily run during a 6-7 upgrade. So at a minimum it will need strict dependencies added, and better hope it's possible for contrib modules to figure out their own dependencies when they copy the pattern.

webchick’s picture

Yeah, hook_update_dependencies() is probably a silly idea. But the basic problem is this.

The entire point of writing an update script is to get older sites inline with newer sites in terms of how the translatable flag is treated. However, if you do these one-off upgrade scripts out-of-band, which require people to read release notes in order to see that they exist in the first place, then download, then run them in some other weird process they've never done before, I can guarantee that less than 10% of sites will actually do this.

Why? Because most peoples' workflows involve "drush dl drupal; drush updb" or the browser equivalent of that. No "read the actual fricking release announcement" involved.

Also, there's the minor detail that if a multilingual site accidentally runs the reset script instead of the update script above, they will destroy data.

So if what we actually want is to bring 99% of sites inline with the current state of this field, then we need to put this in hook_update_N().

catch's fears stem from major version upgrades, when we recommend to people to turn off all of their contributed modules. Yes, there are all kinds of issues with that. However, this isn't 6.x -> 7.x, this is 7.x => 7.5.

Will people doing a 6.x -> 7.x upgrade hit this update function? Yes, yes they will. But they will have no incorrect 'translate' values to correct, and therefore it will do nothing.

As a general rule, I think we should adhere to "no hooks fired in update.php". But when there's only theoretical reasons to oppose it, I'm not sure I can agree.

chx’s picture

I wish for an unsubscribe. This update script calls field_has_translation_handler(). That in turn calls entity_get_info($entity_type);. That's the thorny issue.

However, translation key in entity info is not provided by core. Therefore, I have no idea what are we doing here so I am just wasting my time in here.

catch’s picture

Summarizing via irc.

plach has a new approach which will just hard-code a check for whether locale module is enabled or not, this avoids invoking any hooks (possibly EFQ hooks but they should not depend on schema and we can add a massive comment explaining why it's OK), and it's a best guess which should fix 99% of cases.

Actually I just realised EFQ ends up invoking hook_query_alter() and that might depend on configuration, maybe we need a "no alter" flag there - might be useful for that 403/404 issue too. Anyway that's a relatively minor point compared to hook_entity_info() and hook_entity_info_alter().

As long as we do not re-introduce hook invocations in update functions I don't care what we do here, with the caveat that it'd be nice to sort out #1182296: Add tests for 7.0->7.x upgrade path (random test failures) so that we can test both 6-7 and 7-7 automatically.

I hope that explains it, I arrived late during that conversation and apparently both me and chx misunderstood what was being proposed. If I got something wrong please correct it.

chx’s picture

Note that this issue is extremely hard because multilanguage field language handling is completely undocumented and we have no idea what does it even mean a field is "translatable" or not.

plach’s picture

Title: Manual updates for inconsistencies in field language handling » Updates for inconsistencies in field language handling
FileSize
1.54 KB

Here is an attempt to write a patch not involving hook invocations. I deeply debugged it and it seems that th only hook invocations is hook_entity_query_alter, which is equivalent to the query alteration allowed in any tagged query. Hence I think this is fine.

The update function firstly check if Locale is installed, in this case it skips the update. This should cover even cases where prople disable every module before updating. It should be a reasonable asusmption that any site wishing to deal with field languages has Locale enabled.
Then it runs the EFQ count query to verify that no field value has a language in the DB.

In this case (and only in this case) all fields are switched to untranslatable.

plach’s picture

I already clarified this with @chx in IRC, however here is the link to the multingual field documentation:

http://api.drupal.org/api/drupal/modules--field--field.multilingual.inc/...

Obvioulsy that is not enough. An issue to improve TF DX in D7 (lol) will follow soon.

plach’s picture

Just seen #58:

Actually I just realised EFQ ends up invoking hook_query_alter() and that might depend on configuration, maybe we need a "no alter" flag there - might be useful for that 403/404 issue too. Anyway that's a relatively minor point compared to hook_entity_info() and hook_entity_info_alter().

This looks a good suggestion, should this go here or in another issue?

I think this is now for D7.6 anyway.

catch’s picture

  $result = db_select('system', 's')
+    ->fields('s', array('name'))
+    ->condition('name', 'locale')
+    ->condition('schema_version', -1)
+    ->countQuery()
+    ->range(0, 1)
+    ->execute()
+    ->fetchObject();

Why not just straight db_query_range()?

+    $query = new EntityFieldQuery();

Could we note that this invokes the entity field query hook and hook_query_alter() but that these shouldn't rely on schema directly or indirectly so it's OK? Actually making the alter optional let's definitely do that in another issue.

field_info_cache_clear()

I checked this and the only function it ends up calling is drupal_static_reset() and cache_clear_all(), so this is fine to leave in assuming it stays that way and doesn't introduce any rebuilds.

plach’s picture

FileSize
1.69 KB

Fixed #63.

sun’s picture

I'd suggest to simply do what I mentioned earlier already. See attached patch.

sun’s picture

Fixed the condition, switched to db_select() per @catch's request, and added a dsm() to make the site admin aware of having to double-check the field config of affected fields.

plach’s picture

Tested the patch and it behaves correctly. We agreed in IRC that the Locale check can be skipped as checking field languages is enough. A couple of minor things:

+++ b/modules/field/field.install
@@ -435,5 +435,51 @@ function field_update_7001() {
+      // 'und' denotes LANGUAGE_NONE.
+      $query = db_select($table)->condition('language', 'und', '<>')->range(0, 1);

Why not use LANGUAGE_NONE directly instead of 'und'?

+++ b/modules/field/field.install
@@ -435,5 +435,51 @@ function field_update_7001() {
+    drupal_set_message(t('The following fields have been changed to be no longer translatable: %field-list. Verify whether any of them were intended to be translatable and manually re-configure them accordingly.', array(
+      '%field-list' => implode(', ', $changed_fields),

I'd remove the last sentence as only people having ET would know what it means, and it looks pretty scary considered that it is really an edge case.

5 days to next Drupal core point release.

sun’s picture

plach’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

Hopefully this will solve the update issue. I ain't sure we are still in time for D7.5 (I'd say no), in which case the change record issue should be updated.

sun’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.29 KB

Changed 'warning' to 'status' per @catch's request.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, back to RTBC. As long as tests pass this is good to go.

chx’s picture

Well, sure that's an update function which is good as an update function. It does not even resemble the problematic ones. If this works for you, good for you.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Awesome, thanks a lot for rallying on this folks. Sorry I was afk all weekend.

I need confirmation from at least one person other than plach, who wrote the patch, that this upgrade path works in order for me to chance it in 7.5. "Passes tests" is not enough, because as you can see this patch adds no new tests to confirm what it's doing.

We need two scenarios:

1. I have some fields and I don't have Locale / Entity Translation installed
2. I have some fields and I *do* have Locale / Entity Translation installed

I plan to lock 7.5 down in the next 2-3 hours, and widely announce its availability for testing, hopefully to flesh out any bugs here. I would LOVE for this patch to be ready by then; if not, the upgrade path might have to wait until 7.6. :(

catch’s picture

Status: Needs review » Needs work

Was discussing #1069080: Fix node_comment_statistics.last_comment_timestamp description in irc and realised we don't have a defgroup for 7.x-extra yet.

That needs to be added here, or wait on the other patch (in which case this will need to be re-rolled).

plach’s picture

Assigned: plach » Unassigned

I thought this would be rolled back and postponed to 7.7 to allow the update function to be properly tested. However I updated the version marked in the change record.

webchick’s picture

Yes, I debated a lot about rolling back the entire fix, but in the end, figured that the update function can come in a later release, and it's worth postponing it to make sure it's tested well.

So 7.7 has the change to the translatable flag, but not the update path.

webchick’s picture

Issue tags: +Release blocker

However, since I would really like this upgrade path in 7.8, tagging as a release blocker.

Boobaa’s picture

Upgrading a D6 site with translatable content type(s) to D7.7 yields broken site: node bodies are not displayed at all.

D6 did not support translatable fields, so {field_config}.translatable is always 0, which is good. OTOH affected nodes have $node->field_body[$node->language], which is wrong: they should have $node->field_body[LANGUAGE_NONE] since fields upgraded from D6 are not translatable.

Attached patch solves this by not assigning the $node->language to body values during the upgrade. (We don't have too much upgrade path tests for multi-language sites, if any at all, so @Gábor Hojtsy said on IRC that I don't have to provide tests for this particular case either.)

EDIT: re-posted this as a separate issue: #1235604: D6 -> D7 multilingual node body upgrade broken.

sun’s picture

+++ b/modules/node/node.install
@@ -710,7 +710,10 @@ function node_update_7006(&$sandbox) {
           // After node_update_7009() we will always have LANGUAGE_NONE as
           // language neutral language code, but here we still have empty
           // strings.
-          $langcode = empty($revision->language) ? LANGUAGE_NONE : $revision->language;
+          // Drupal 6 has translation support only for content types, not for
+          // fields. Any and all fields upgraded from Drupal 6 should NOT be
+          // language-aware, including body.
+          $langcode = LANGUAGE_NONE;

We need to adjust/change the existing comment instead to clarify that field values are stored as LANGUAGE_NONE regardless of Locale's/Translation's language assigned to {node}, which is updated from '' to LANGUAGE_NONE in node_update_7009(), but does not affect field languages.

Lastly, we should merge this new patch with #70.

30 days to next Drupal core point release.

Gábor Hojtsy’s picture

@sun: do you think we should work on / test the D7 -> D7 upgrade path and remaining issues with the D6 -> D7 upgrade path (which might need more tests written) in the same patch / issue? (Reason for opening a separate issue was to support working on them separate).

Boobaa’s picture

Though I don't really get why should we mix up D6->D7.7 and D7.4(?)->D7.7 upgrade path problems in a single patch, I'm providing the patch @sun asked for. OTOH, I still can't provide tests for neither of the cases the patch addresses.

Gábor Hojtsy’s picture

Priority: Major » Critical

As per @sun's and @catch's suggestion via IRC, moving this to critical, now that we've introduced a major regression in the D6 -> D7 upgrade path that makes nodes not work at all on updated sites when your node had a language on D6. They wanted to either pull out the original patch or commit @boobaa's patch and release 7.8 right away. I don't think that's a good idea, and its best to have a "better feel" if we actually have it done right this time. "Better feel" would ideally be coupled with upgrade test coverage which was not requested above yet if I'm not mistaken, but looks like in light of this regression that would be best added.

Gábor Hojtsy’s picture

Also added this to the Drupal 7.7 announcement (http://drupal.org/drupal-7.6):

Known issues

Drupal 7.6 and 7.7 introduced an issue in node data migration in the Drupal 6 upgrade path for foreign language sites that migrates node bodies as untranslatable but assigns language to the data which makes nodes appear in an unsupported state. If you are looking to upgrade a foreign language Drupal 6 site, use Drupal 7.5 for now, and track progress on the issue at http://drupal.org/node/1164852#comment-4807524

Gábor Hojtsy’s picture

Note that now we'll also now need an upgrade path for the state that D6 -> D7.7 sites are (which is very different to how D7 sites were). Although theoretically people should not use that site upgraded to D7.7 (nodes will not work for them), we provide increment updates by nature and they might still ponder around there and want to get this update to fix their stuff instead of going back and forth.

NancyDru’s picture

Are you saying that I cannot upgrade any sites to D7 now? How about sites that didn't add fields in D6? How about content in node modules?

plach’s picture

The problem is with node bodies, so CCK fields don't matter here. As Gabor pointed out in the D7.7 release notes, for now people not wishing to patch core have to update to D7.5 :(

plach’s picture

Issue tags: +Needs tests
catch’s picture

Title: Updates for inconsistencies in field language handling » (regression) Node bodies display as empty after upgrade to 7.7 (was field multilingual handling inconsistencies)

#1164852: Inconsistencies in field language handling was duplicate.

I'm changing the title in the hope that people who run into the regression find this issue instead of opening loads of duplicates.

Lessons learned:

- it is right to be paranoid about the upgrade path
- we should be as paranoid about one-line changes to the existing upgrade path as entirely new updates :(

webchick’s picture

Oh boy. :(

catch’s picture

Title: (regression) Node bodies display as empty after upgrade to 7.7 (was field multilingual handling inconsistencies) » (regression) Node bodies display as empty after upgrade to 7.7 from 6.x if the nodes have a language

Sorry one more title tweak.

Boobaa’s picture

To be honest I don't know these patches address all the things this issue contains, so I'm not bumping the issue to "needs review" yet -- but now there are tests for proper upgrading of bodies (and teasers as a bonus) of translatable node types. These tests fail miserably without/before correcting node_update_7006(). The -tests_only- has only the tests, so it should fail; the -tests_and_patch- has both the tests and the actual fix, so it should pass.

webchick’s picture

Status: Needs work » Needs review

Marking needs review to give testbot a crack at it.

NancyDru’s picture

So now all of us good fast-following girls and boys have to drop back to 7.5 to upgrade sites. "Oh boy" is right!

plach’s picture

Well, actually you can update to D7.7 after upgrading from D6 to D7.5, obviously this undoes any improvement to the upgrade path achieved in D7.6...

Gábor Hojtsy’s picture

Status: Needs review » Needs work

@NancyDru: this applies if you have a foreign language or multilingual site. Do you? As plach said, you can update to 7.5 and *then* to 7.7.

@boobaa: took a quick look at the patch. First of all thanks for working on the upgrade path! Second: the bug is with the node having a language, not necessarily that it is translatable, so the patch already passes/fails with/without the fix. But if you look at the test, you create a translatable_page content type and then modify language support on the page node type, not the translatable page node type. This needs to be fixed.

Boobaa’s picture

Boobaa’s picture

Above patches and tests are only addressing the D6->D7.8(?) upgrade path, but has nothing to do with broken D7.7->D7.8(?) upgrades. Here is a patch that address this latter one as well, although without any tests for it (I'm not that ninja enough yet to craft them).

Rationale: there can be broken D7.7 sites out there because of the wrecked upgrade path, and those should be fixed as well; so we can't have a D7.8 without a working (and tested) upgrade path both from D6 and (broken) D7.7.

I do have tested the attached patch with a local copy of a production D6 site by upgrading to D7.7 first (so the site became broken), then applying the patches and upgrading again (so the site became as-it-should-be) with success.

NancyDru’s picture

@Gábor: I have only created one multilingual site, and will not be upgrading it. However, I have seen several sites with a mixture of English and undefined language codes, probably caused by some blondness on my part.

Boobaa’s picture

Today we have discussed this with @Gábor and @sun. Here is an excerpt.

We have to cover two different upgrade paths: D6->D7.8 and D7.7->D7.8. We should consider D7.7 broken: translatable nodes' fields (most prominently the body field) do not display at all, since their values are localized (we have $node->body[$node->language], which is wrong) while the field themselves are not translatable (we should have $node->body[LANGUAGE_NONE]).

So, there needs to be two different fixes: one for 6->7.8 (which does the upgrade "right") and a different one for 7.7->7.8 (which "corrects" the broken 7.7 sites out there). This asks for (at least) three different patches:

1st patch should consist only of a test that checks if the bodies (of a translatable D6 content type upgraded to D7.8) are displayed. This patch should fail.

2nd patch should consists of the test and the 6->7.8 fix (which is in/for node_update_7006()), so the patch should pass.

3rd: Here comes trouble: how to create a broken 7.7 site in a test environment? The most straightforward/easy way to achieve this is to start from a D6 dump, do not have the 6->7.8 upgrade path fix, but have only the 7.7->7.8 fix (accompanied by the test) in the patch. This patch should pass, too. The same test can be used as it is totally indifferent how we got to a working D7.8 site: either by a direct 6->7.8 upgrade, or through a broken 7.7 (going through 6->7.7->7.8, the 7.7->7.8 fix is field_update_7003()).

Studying the already-submitted patches reveals that we do have every bit in, though not (yet) in a proper way. The failed patch of #96 is the above-mentioned 1st one, so this one is done already. #96's passing patch contains the test and the 6->7.8 fix (I'm not totally sure if field_update_7002() should be considered part of the 7.7->7.8 path), so it's the above-mentioned 2nd one.

But I/we didn't pay attention enough with the others: the passing patch of #97 is not the one we wanted as 3rd, but something that is to be easily committed by @webchick, which consists of both the 6->7.8 and 7.7->7.8 fix, and the test. The test of #97 passes, since it tests only the 6->7.8 fix, because it's quite hard to test the 7.7->7.8 fix by itself.

So, what we need now is a passing patch which consists only of the test and the 7.7->7.8 fix, but I can't seem to be able to craft it, because of the following.
- If I load a D6 dump (with only core modules enabled) into an empty DB, run update.php without patching 7.7, then add field_update_7002() and field_update_7003() (from #97), then run update.php again, everything is OK: body fields get displayed properly, field.module's schema_version is bumped to 7003 as intended. So the 7.7->7.8 fix is tested to be OK by hand; let's automate this.
- If I load the same D6 dump into an empty DB, and add field_update_7002() and field_update_7003() (from #97), then run update.php (for the first and only time), then field.module's schema_version is bumped to 7003 (as intended), BUT the added functions do NOT run AT ALL, so body fields do NOT get displayed. (When I say they "do NOT run AT ALL" I mean that I have even tried adding some file_put_contents('/tmp/d78test', 'foo') to both of them and the file didn't even got created. When I tried this same technique with the two-runs-of-update.php method, the file did get created.)

Additionally, this is the one we should be doing for the 3rd patch mentioned at the beginning of this comment (packed into a patch along with the test), and this patch should theoretically pass. I'm attaching this patch here for the records, although I do have ran the simpletests locally and they failed.

I don't know how to continue from here, and @sun could not help me out either (anyways, it's quite possible that I was the one who could not describe things properly on IRC). Hopefully someone who have read this long litany and understood the concerns could help us moving forward.

sun’s picture

I may be utterly wrong, but I can't see where and how new modules in D7 are adjusted to have any updates run. If my suspicion is correct, then the existing updates for them were only executed, because other modules depend on them.

ksenzee’s picture

Yes, sun is right. From update_get_update_list():

  $modules = drupal_get_installed_schema_version(NULL, FALSE, TRUE);
  foreach ($modules as $module => $schema_version) {
    // Skip uninstalled and incompatible modules.
    if ($schema_version == SCHEMA_UNINSTALLED || update_check_incompatibility($module)) {
      continue;
    }

At the beginning of a fresh update from D6, field.module is uninstalled, so its $schema_version == SCHEMA_UNINSTALLED. That means no field module updates will get run at all, which is correct behavior. It seems like any code intended to turn D6 nodes into D7 nodes with fields should go into node.install.

Status: Needs review » Needs work

The last submitted patch, drupal.system-update-module-enable.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
2.82 KB

Attached patch is what I believe we need to do.

sun’s picture

err, rather this ;)

Status: Needs review » Needs work

The last submitted patch, drupal.system-update-module-enable.104.patch, failed testing.

catch’s picture

I think we need to split the 6-7 and 7-7 upgrade path issues into two.

7-7 upgrade path testing depends on #1182290: Add boilerplate upgrade path tests and #1182296: Add tests for 7.0->7.x upgrade path (random test failures).

If we don't want to commit untested upgrade path issues, we need to get that in place, this is not the issue for adding those tests, although it shows the urgent need for them.

However adding those should not hold up the 6-7 upgrade path issue getting fixed so that people who upgrade from 6-7 over the next month or two don't run into it - if that just means we roll back the patch here as a stop gap then work on everything together again, that's fine with me but this risks spilling all over the place (like so many other upgrade path issues).

webchick’s picture

I'm basically fine separating out the 7.x -> 7.8 tests from the upgrade path fixes, if that's what it takes. But I'm not interested at all in splitting this into two issues. Everyone who understands the problem is already subscribed here.

catch’s picture

Hmm, so can we do this then?

edit - whoops, missed a bit, added a new #2 and bumped the rest up a number...

1. Fix the 6.x-7.x upgrade path issue. As far as I can see, http://drupal.org/node/1164852#comment-4811420 has the fix, and http://drupal.org/node/1164852#comment-4807524 has the tests.

Fixing this has no dependencies on anything else and it is a couple of lines bug fix plus a good looking test which look very close to being ready if not RTBC. That will stop the bleeding for any of the 300,000+ D6 sites who try to upgrade to 7.8.

Makes loads of sense to do that here, and asap.

That leaves us with the following which are more complicated:

2. Add ability to test databases from Drupal 7, this is #1182290: Add boilerplate upgrade path tests, mainly the dump script.

3. #1182290: Add boilerplate upgrade path tests - although we don't necessarily need generic databases to test this - we could add dedicated ones specific to this issue here, so it might not be a dependency at all.

4. The fact that all newly added modules in Drupal 7 do not have any updates run during a 6.x-7.x upgrade, even if the update is required for every site - the issue sun just found and started to work on. This is also not restricted to this issue at all and risks getting lost in over 100 replies already. Needs wider discussion without other distractions IMO.

5. Drupal 6 sites that upgraded from 6.x to 7.7 and caught the regression- that depends on having 7.x - 7.x upgrade path testing, which would need to include a specific test including a database upgraded from 7.x - 7.7 that we would then upgrade to 7.x-latest in the test. Completely makes sense to work on that here.

6. Drupal 7 sites installed from 7.0-7.6 which is the upgrade path plach worked on that I steamrollered but now also looks fine except the need for testing (see #2 and #3). That needs tests from 7.0 (or any other 7.x point release we might want to choose) to 7.x-latest. Also makes sense to do that here.

I make that five six distinct tasks, three of them are related to this issue, two were a known issue before it, one we found as a byproduct of working on it.

phew ;)

Boobaa’s picture

Okay, after a longish discussion with @catch on IRC let's clean things up a bit.

#108/1 is the D6->7.8 path, which is both fixed and tested already. One tick.

#108/2 & #108/3 are a bit offtopic here (we concluded that this issue should concentrate on fixing what 7.7 broke, and not more), and beyond me.

#108/4 has serious other, mostly unrelated things in it (@catch used the phrase "can of worms"), which are even more far beyond me, and has almost nothing to do with our actual issue here.

#108/5 is the one and only thing we have to deal with here, as it's the D7.7->7.8 fix (and test) to bring out sites out in the wild from their brokenness. (See below.)

#108/6 has nothing to do with our current issue either: as we all (included @Gábor) agree it is not possible to get to a broken D7.7 site starting from any of the D7.x releases.

So we have to have at least three different patches.

1st is only the test (starting from a D6 site/dump and upgrading just like 7.7 does), which should fail. (The test actually checks if node bodies get displayed properly after an upgrade.)

2nd is the test with the D6->7.8 fix, which should pass. (This fix is in node_update_7006().) If this passes, then it proves that the D6->7.8 upgrade path fix works fine.

3rd is the test with the D7.7->7.8 fix, which should pass too. This is the most trickiest part here: as we don't (yet) have tools to create a dump of a broken D7.7 site, then that "broken D7.7" state should be produced somehow. But it's not even possible to produce starting from any known D7.x state, which leaves us two possibilities: (a) have D7.7 installed (by the test) and db_update() it as needed to knock it to a "broken" state, or (b) use the _broken_ D6->7.7 upgrade path, which starts from a known D6 state (the one we're using all over the tests here) but lacks the fix in the previous, 2nd paragraph, but do has the fix for D7.7->7.8 upgrade. (This fix is in node_update_7012(), since that one do gets run while upgrading, as @sun and @ksenzee has dig it up.) If this passes, then it proves that the (D6->)7.7->7.8 upgrade path (with the fix focusing on the D7.7->7.8 part) works fine, too.

4th patch is just a "for @webchick with love" thing, which consists all of the aforementioned stuff: the test, the D6->7.8 fix and the D7.7->7.8 fix. This one should pass, too, but does not test both the upgrade paths by itself, only accompanied by the 2nd and the 3rd one. (For the records: the test in this patch tests only the D6->7.8 fix, since if we have both fixes in, then we can't reach the "broken D7.7" state in the test.)

I am attaching all the four patches, wishing good luck to the testbots -- and please don't mix this issue with the (seems-to-be-somewhat-related) others mentioned: I think this one should get in ASAP to correct the broken D7.7 sites out there in the wild.

EDIT: Forgot to mention that I do have tested all of these both "by hand" and running the simpletests, which yielded the expected results.

plach’s picture

Status: Needs review » Needs work
+++ b/modules/node/node.install
@@ -863,3 +863,43 @@ function node_update_7011() {
+      // 'und' denotes LANGUAGE_NONE; constant values may change over time.
+      $query = db_select($table)->condition('language', 'und', '<>')->range(0, 1);
+      $query->addExpression(1);
+      // Only in case there is language-specific field value, update the field
+      // value entries.
+      if ($query->execute()->fetchField()) {
+        $changed_fields[] = $field['field_name'];
+        db_update($table)
+          ->condition('language', 'und', '<>')
+          ->fields(array('language' => LANGUAGE_NONE))
+          ->execute();
+      }

I think we need a more strict check here: there might be untranslatable fields that previously were translatable holding both 'und' and 'en' values. Alternatively people could have "re-entered" the lost node bodies causing 'und' values to be saved without 'en' ones being replaced. In both cases changing the 'en' value to 'und' would lead to a primary key constraint violation.

27 days to next Drupal core point release.

plach’s picture

I guess we need to exclude every row having entity type and id in the following set:

SELECT DISTINCT b.entity_type, b.entity_id
FROM field_data_body b
JOIN field_data_body b2 ON b.entity_type = b2.entity_type AND b.entity_id = b2.entity_id AND b.delta = b2.delta
WHERE b.deleted = 0 AND b.language = 'und' AND b.language <> b2.language;
Boobaa’s picture

Status: Needs work » Needs review
Boobaa’s picture

I don't get why that one has failed; will look into what @plach suggested in a bit.

plach’s picture

Forget #111: we need to exclude from the update any row having more than one language otherwise we will screw every site using Entity Translation:

SELECT entity_type, entity_id, COUNT(language) as languages, delta
FROM field_data_body
WHERE deleted = 0
GROUP BY entity_type, entity_id, delta
HAVING languages > 1;
plach’s picture

Status: Needs review » Needs work

One more thing: we need to update also the field_revision_body table.

sun’s picture

Status: Needs work » Needs review

Created #1239370: Module updates of new modules in core are not executed

Note: While we could commit the change to node module's update in order to release 7.8, I don't think we really want to commit yet another change to module updates without tests. We can only test the upgrade path after aforementioned issue has been resolved and committed. So, as sad as this sounds, count me -1 on moving forward here without having the other issue fixed.

plach’s picture

Status: Needs review » Needs work

The latest patch cannot work as-is, so the proper status is NW.

About #116, I agree that releasing D7.8 to fix this ASAP without test coverage is undesirable. However I'd wish that, when the natural time to release D7.8 comes, we have at least the missing line in node_update_7006() and the related test committed, since they should have already been part of D7.7.

Boobaa’s picture

Attaching reworked 3rd and 4th patches of #109 as @plach asked in #110, #111 and #114. field_revision_body table is already dealt with, starting from as early as #97, so #115 seems to be noise (if I understand everything correctly).

@sun in #116: I do understand your concerns about testing upgrade path in general, and you have reason; but for this particular issue we have not only solved it, but have tests as well.
Regarding to your concerns about fixing the D7.7->7.8 problem in node_update_7013() (rather than in field_update_7003() that I have tried in #99, or any better place if it should belong to somewhere else) I have had my point, but let me rephrase: I would gladly help out with forging test for all over the upgrade path, but that "can of worms" (as @catch called) is far beyond me; and even worse: I'm afraid that one will not be properly finished by the time we are to release D7.8. So to avoid releasing a still-broken D7.8 (besides the fact that we do know it's broken and do have fixes for it and do have tests for those fixes), I'd recommend solving this issue here asap. I don't like using big words (since being the little fish here), but this issue has the "Release blocker" tag and #1239370: Module updates of new modules in core are not executed doesn't (yet, at the time I'm typing this, though that one's a critical, too).

Regarding to the patches I'm attaching now: they should scale for even big(ger than I have) sites as well, since now the fix uses batch API to correct what can be corrected automatically.

elfur’s picture

Status: Needs review » Needs work

Hopefully I can help with testing.

FYI: When you've got a reviewed copy of a patch I do have a backup of my D6.22 DB that broke when I attempted to upgrade it to D7.7.

I upgraded the site to D7.5 and will wait for D7.8 before I am completely up to date with the site, but I still hold the D6 backup and can therefore test a direct upgrade from D6.22 to D7.8 (when the patch is ready, before rollout of that version).

You'll just holler if that's necessary. It's best to do with private message via this site as I'm not really an active member of the drupal.org community these days.
I'm still happy to help of course, whenever I can.

my apologize if this post is just noise, I haven't read through all 118 comments above.

/elfur

webchick’s picture

Status: Needs work » Needs review

Restoring back to needs review; it wasn't clear if #119 actually tested and found a failure, but it didn't seem so.

Boobaa’s picture

@elfur, there are two main attack vectors here. One is for the D7.7->7.8 upgrade that should correct a broken site, which is addressed by the 03-test_and_d77d78 patch in #118. To be able to test it you should upgrade your D6 site to D7.7 first (so it becomes broken), then apply the patch and run update.php again: if the patch is OK (which both me and the test think is), then your site should be OK.

The other attack vector is for the D6->7.8 upgrade that should work without the flaw in D7.7, which is addressed by the 02-test_and_d6d78 patch in #109. To test this one you should apply the patch first then run update.php on your site only once: if the patch is OK (which both me and the test think is), then your site should be OK (without any broken state, since this patch corrects the flaw that was introduced in D7.7 at its roots).

In either case, please report back with what you found.

chx’s picture

Status: Needs review » Needs work

This is our situation, because every edit of node_update_7006 opens a new branch:

        +-------------------------------------------------> 7.7
        |
        |node_update_7006
6055 --->-------------------------------------------------> 7.4
        |
        |
        +-------------------------------------------------> 7.8

which would mean that we need separate updates from 7.7 and 7.4 to 7.8. To avoid that, what we can do is go back to the node_update_7006 which existed before 7.7 broke it and add a new upgrade that gets us to 7.8:

        +------------------------------------------------->  7.7
        |
        |node_update_7006
6055 +-->-------------------------------------------------+  7.4
                                                          |
                                                          |
                                                          +> 7.8

Before we edited update functions, life was easy because we didnt need minor upgrade test: we knew that the upgrade from 7.0 to 7.2 worked because if you start with a database at 6055 and upgrade to 7.0 and then apply upgrades to 7.2 then you arrive to the same point as running all upgrades up to 7.2

If we do the sort of update I outlined above then all we need to add is an upgrade fix from 7.7 to 7.8 and a test that starts from a dump of the test databases upgraded to 7.7, runs the upgrades to 7.8 and runs the same asserts. This means that both update_N and upgrade tests now need to verify schema versions and fork but we brought that upon ourselves.

To make the new test pass on 7.4 you need to change to

    $body_items = field_get_items('node', $node, 'body');
    $this->assertEqual($body_items[0]['value'], $body, 'Body of the node survided upgrade properly');
    $this->assertEqual($body_items[0]['summary'], $teaser, 'Teaser of the node survided upgrade properly');
Boobaa’s picture

Status: Needs work » Needs review

Discussed it with @chx on IRC this approach is as follows (in easier-to-understand terms, at least for myself):
- Revert node_update_7006() to as it was in D7.5 to get rid of the upgrade path fork.
- Rename my node_update_7012() to node_update_7013() to correct broken D7.7 sites, and to merge the upgrade path back to the one-and-only branch we always wanted to have, starting from D7.8.
- Prepare something named node_update_7012() that corrects the body field definitions (the monster that caused the change 7006) by setting them (back) to untranslatable. Beware: if we do this unconditionally, we would screw up valid 7.x sites (built as 7.x, not upgraded from D6) that have their body fields set to translatable, so this change must be done only if we're upgrading from D6.

This asks for a database dump of a broken D7.7 site to be able to properly test. Expressed in asciiflow by chx (7006' is that broken stuff we have in D7.7, 7006 is the thing we had in D7.4):

6055 ----- 7006' ---> 7.7 [new database dumps]  ----- (7012 this won't run) ---- 7013 ---> 7.8 [run tests on the new database dumps]
6055 ----- 7006 ---> 7.4  ----- 7012 ---- 7013 (this won't run) ---> 7.8 [run tests on the old database dumps]

Anyways, it seems I won't have time for this nowadays. :(

PS. Oh, and we should introduce a new rule of thumb: do not try to correct hook_update_N() problems after they have been released by correcting the function that introduced the problem (this leads to the issue we're currently facing with); we should only correct them by adding new hook_update_N() implementations.

chx’s picture

It's easy to discern between D6 and D7 upgrades: update_prepare_d7_bootstrap has if ($system_schema < 7000) { in that if, set a $foo and later , after the drupal_bootstrap(DRUPAL_BOOTSTRAP_VARIABLES); do a variable_set ('foo', $foo)

Boobaa’s picture

Status: Needs review » Needs work
FileSize
14.89 KB

Attached is a patch that:
- reverts that one-liner monster introduced in #41-#45,
- has field_update_7002() from @sun though I don't really know whether it's needed at all,
- has node_update_7012() that switches body fields to untranslatable while upgrading from D6, but does nothing for D7->D7 upgrades (this is considered to be the "correct" way to solve the issue, instead of #41, both by me and @chx),
- has node_update_7013() to correct broken D7.7 sites.

While the attached patch does have tests as well, I don't know (yet) if they are covering everything needed here (thus CNW). OTOH I do have tested the patch locally both with a copy of a production D6 site and it's (broken) D7.7 version, both time with success. Wish I had the time for (help) forging the tests needed here, anyways.

NancyDru’s picture

Is there any way to set Update to stop encouraging people to upgrade beyond 7.5?

renat’s picture

Don't think we should try to change Update settings. Looks like this problem emerge only after direct upgrade from D6.x to D7.7. Neither previously upgraded from D6.x, nor native D7 sites are affected. And upgrade script will not encourage you to make major version upgrade.

Boobaa’s picture

Summing up, from the grounds.

We need two different things, and we have one complication:
1. Upgrading from D6 should yield an untranslatable body field with LANGUAGE_NONE data.
2. Upgrading from D7.x (including D7.7) should yield the same, but only if the body field does not have any other data than $node->language. If we unconditionally switch the body field to untranslatable, then we can't avoid the opportunity to screw up valid D7.x sites that has translatable body field. If we have any body field data besides $node->language, then we cannot decide automatically whether we're facing with a valid D7.x site with intentionally translatable body field, or a broken D7.7 site that is getting "fixed" by its users by (re)saving data into the body field.

The patch in #125 upgrades D6 sites just like D7.4 did, so it definitely needs work. Anyways, I don't know how to continue from here, because of that "we cannot decide automatically" problem. If somebody could craft an algorithm to do that decision right and automatically, feel free to post only with the algorithm, and I'll try to translate it into PHP code as time permits.

Boobaa’s picture

@Gábor Hojtsy suggested if we cannot make that decision automatically, then we should assume that we're facing with a proper D7.x site (with translatable body field and localized data). Thinking aloud following this line: if we have a translatable body field that is available only in one language (which is not LANGUAGE_NONE, of course), then does it mean we're upgrading from D6 or a broken D7.7 – or could this also mean that it's a proper D7.x site with translatable body field and localized data (which is available for only one language at the moment of upgrade)?

Am I the only one who is losing control of this issue? :S

catch’s picture

#129 sounds sane. If we can't tell between broken data and valid data, then we should assume it is valid data.

I have definitely lost track of what is going on at this point.

chx is right about 6.x upgrades - we can do a variable_set() somewhere early, check it later, then delete it again.

plach’s picture

Let's sum up again. We are addressing 3 (wow!) problems here:

  1. Broken D6 to D7 upgrade path: this can be easily fixed and the solution is more or less in #125 (see comments below). We should commit the upgrade path fix + tests ASAP, since there is no reason to hold them back. Moreover people that tried to upgrade with D7.7 might have a second chance with a dev tarball.
  2. D7.7 to D7.8 update: this is what is being discussed from #128. IMO the solution here is what have been proposed by Gabor: we can switch language safely only for untranslatable node bodies (not any field!) having only one language and this language is different from LANGUAGE_NONE. Anything else must be left alone. This totally needs test coverage since messing with field languages is exactly what I wanted to avoid before the fu**ing mess introduced in #41/#45. Hence this must be postponed to being able to test D7 > D7 updates (I can't find the issue). We need a separate patch for this.
  3. What this issue was left open for: solve the inconsistencies left out from the committed "fix". The update function provided by @sun is fine, we just need tests for it, again we are stuck on being able to test D7 > D7 updates.

So we need a patch including anything in #125, with the exception of field_update_7002() and node_update_7013(), incorporating the fixes suggested below. This is the only one that can and must be committed immediately.

+++ b/includes/update.inc
@@ -178,6 +178,17 @@ function update_prepare_d7_bootstrap() {
+    // @see http://drupal.org/node/1164852

Usually we don't link to issues in comments, don't know if this issue deserves an exception.

+++ b/modules/node/node.install
@@ -861,5 +862,126 @@ function node_update_7011() {
+    // does not run for D7->D7 upgrades. If somebody ever wants to do
+    // something like this (modifying anything in the {field_config} table
+    // directly) in a D7->D7 upgrade, then it may lead directly to hell, and
+    // the gatekeeper there is chx. You have been warned.

Funny paragraph. I ain't sure we actually want to ship with it in this exact form, above all because we are messing with 'field_config' in field_update_7002.

+++ b/modules/node/node.install
@@ -861,5 +862,126 @@ function node_update_7011() {
+    node_type_cache_reset();

We totally should move the code for making D6 node bodies always have LANGUAGE_NONE here. This will allow us to clear our way safely, because in the D6 > D7 upgrade context we are sure no translatable body should exist. Also, we would achieve a clearer separation between the code that performs a regular D6 > D7 upgrade and the one that fixes sites upgraded to 7.7.

21 days to next Drupal core point release.

plach’s picture

Status: Needs work » Needs review
FileSize
8.62 KB

This is what I meant. Let's see what the bot has to say.

Status: Needs review » Needs work

The last submitted patch, tf-1164852-132.patch, failed testing.

plach’s picture

plach’s picture

Status: Needs work » Needs review
FileSize
8.59 KB

I managed to fix the issue above. Now this one should work.

Boobaa’s picture

Status: Needs review » Needs work

#135 upgrades from D6 well: body field is not translatable, its values do show up (they are stored as 'und' in the DB). However, it does not fix the broken D7.7 sites at all, although (in my case) it could be done, since it was a straight upgrade to D7.7 from D6, without any tries to "correct" it. So this patch definitely still needs work. :S

plach’s picture

Status: Needs work » Needs review

@Boobaa:

Did you read #131? IMHO the right thing to do here is committing the fix for the regression which is, thanks to you, brilliantly test-covered. The other 2 problems instead are held back by the ability to test D7 > D7 updates. I guess it makes no sense to provide one single patch that might be ready only after D7.8 ships. At least let's make D7.8 behave as planned for D7.7.

We can provide a separate patch for the rest of the issues open here while waiting for #135 (or equivalent) to be committed, and merge them back into one patch if we come up with a complete fix before D7.8 comes out.

plach’s picture

Quoting an I email I just received, I asked the author to confirm here on the queue:

Hi,
just wanted to let you know that I've just tried the patch in http://drupal.org/node/1164852#comment-4859592 and so far soo good.

I updated a multilingual (5 languages) site from 4.7.4 -> 4.7.11 -> 5.23 -> 6.22 -> 7.7 and it broke in the last step (body content not showing). Did the last step again, 6.22 -> 7.7 after applying patch #135 first to 7.7. i18n etc seems to be working well too.

More tests to follow. Please let me know if I can help test in any way.

chx’s picture

I do not think "screwed" is appropriate comment language. Also please add a summary to "remaining tasks" in the issue summary. Also open and link the other issue for D7->D8 upgrades. Also: AWESOME WORK!

catch’s picture

There are issues open already for 7-8 and 7-7 testing. I'm not able to review the patch likely until tomorrow but I agree with getting a patch in to fix the regression before anything else. Doesn't stop other patches getting in but we can't ship 7.8 with this as it is currently under any circumstances.

akejoha’s picture

Confirming #138.

yched’s picture

Note : the original patch in #35 broke the layout of Field UI's "Manage fields" table.
#1248940: "Manage fields" screens : broken table layout

yched’s picture

Issue summary: View changes

Changed the summary since we cannot implement an update function.

plach’s picture

FileSize
8.43 KB

Patch rerolled with more appropriate (:D) comments. Also updated the issue summary. Also: thanks @chx!

Code is the same as in #135.

plach’s picture

I realized that in the previous patch the D6 flag variable was not deleted once the update was complete. This could break any subsequent minor update relying on that one to check whether a D6 upgrade was going on.

New patch attached with fix + tests.

Status: Needs review » Needs work

The last submitted patch, tf-1164852-144-test.patch, failed testing.

catch’s picture

Patch looks good to me (although so did the broken one we committed), should swap the order so the test bot doesn't choke on the failures.

plach’s picture

Status: Needs work » Needs review

@catch:

Yes, ideally everyone here should review and test the patch and confirm the fix is good.

joris.verschueren’s picture

back from a long holiday (since comment #11), I will take up my upgrade from 6.x to 7.x again. Thank you very much for working so hard on this frustrating issue. I'll certainly report back.

Do I understand it correctly that an upgrade from 6.22 to 7.7 should work, provided 7.7 is patched before running update.php?
should I use the patch from #144 or the one from #135?
it isn't very clear to me whether the patch in 144 is only meant for updates from 7.5 to 7.7, or from 6.x to 7.7 - generally, which of these two problems you're still working on.

(FYI: I'm managing only one site as part of an entirely unrelated job [half a day per week], which explains why I'm not so savvy about most of what's being discussed here. It also explains part of my absence, I was working on an upgrade path for my testsite but was then absorbed by entirely different jobs. I hope the results on my homegrown site are of any interest in bringing a solution closer. If you want particular tests to be performed, I'm willing to use my backups.)

akejoha’s picture

Just applied tf-1164852-144.patch from #144 to a clean ver. 7.7 and then imported a 6.22 database. Ran update and everything seems to work so far, i18n applied and all.

Great work, thanx!

plach’s picture

@akejoha:

Thanks!

@joris.verschueren:

The patch in #144 should fix the regression introduced by the first commit and provide a working D6 > D7 upgrade path on a patched D7.7.

See the (updated) issue summary for details.

Fannon’s picture

Got the same problem here.
But I wasn't using multilingual Features. Its just a german drupalcenter.de Profile Installation

subscribing

Simon

steinmb’s picture

@Fannon: Pls. try the patch on #144, and report back if that did the trick.

Fannon’s picture

Maybe I did something wrong when applying the Patch #144 (I used Netbeans) but I've still got the same Problem. Nodes are empty.
Is there a way to fix this with a SQL Query?

Greets,
Simon

Fannon’s picture

I used the reset patch in #42 and this looks like its working for me.

plach’s picture

@fannon:

You should apply #144 before upgrading in order to be able to test it. Good to know that also the script in #42 worked for you.

Fannon’s picture

@plach:
I did apply the patch before upgrading - of course ;) Didn't work for me though, but as i stated: I may have done something wrong with applying the patch.

joris.verschueren’s picture

sorry for not reporting earlier. I wasn't aware of the change to Git, and frankly, I'm struggling to get that working.
I suppose I'll try the script in #42 rather than patching.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Based on #146 and #149 and my own review.

joris.verschueren’s picture

I've applied the patch and it worked as expected - for about half of my image fields. a certain number of imagefields is migrated nicely, disregarding language. there, the patch seems to perform what one expects. I'm impressed.
but then on other nodes, the imagefield is populated but the image is not being displayed. I can't think why some images are, and why others aren't being displayed. It seems pretty random at first sight. At least, it can't be systematically traced to language settings.
Could this still have to do with the patch (anyone tested on a site with over 200 imagefields)? In any case, my results seem to prove that the patch achieves its goal - but perhaps not site-wide. Or do you think this is a different issue altogether?

Any suggestion is welcome (not that I want to go off-topic). Do images have to be rebuilt in order to be displayed? running cron doesn't seem to change the display issue, and then again, before upgrading the images (i.e. field content) were simply displayed.

guygg’s picture

OK, I've tried to dig through some of this thread, but have gotten more confused as I go than less. I had a 6.x site that I upgraded to 7.7 a week ago and tried to go through all the proper steps that I could find in the directions. One (of the numerous) problems after the upgrade are many nodes that have their body fields showing up as empty. I've looked at them in the DB and every single one of them that doesn't display properly that I've checked so far all have the language field set as EN. The ones with UND seem to work fine. I have zero use for any language than English for my site. I don't recall having enabled any language modules in the 6.x version of the site, but apparently something must have set that language value to EN on that content. I tried enabling Locale & Content Translation, which didn't help (update.php said there were no updates when I ran it after enabling those). I tried installing Entity Translation and it's upgrade submodule (again, updata.php said there were no updates), which didn't help. I tried unchecking Node in Translatable Entity Types (which was checked when I first looked at it), which didn't help, so I put the check mark back on it. Tried running the Content Translation Upgrade, which seemed to do nothing.

I'm confused as to what to do to get previous 6.x content that's coming up blank to show. Like I said, I don't have any use at all for content translation. Do I need to modify every node that's EN over to UND? Is there a better way? An idiots guide paragraph or two for people who are already experiencing this situation and have already gone through the full upgrade process might be helpful. If that already exists on a comment in this thread, what post # is it?

Thanks.

Fannon’s picture

@guygg:
I've used the reset Patch in #42 and it worked for me.
If you don't know how to execute a PHP Snippet: Install the devel-Module and there's a "Execute PHP Block". You don't need the <?php tag as the module will tell you too.

guygg’s picture

OK, that reset from #42 seems to have done the trick. Thanks. Is there any reason for me to not turn Entity Translation & Locale back off again now?

webchick’s picture

Status: Reviewed & tested by the community » Fixed

I'm feeling a little trepidation about the RTBC in #158, as the end user reports in this issue are conflicting. However, this at least pushes the ball further along, and once this is in the dev release it becomes much easier for wider-spread testing. We also still have some time at DrupalCon and next week before 7.8.

So, committed and pushed #144 to 7.x. Thank you SO much for all of your hard work on this, guys.

I'm going to tentatively mark this fixed.

plach’s picture

Status: Fixed » Postponed

Ok, the worst part of this should be closed. I'd wish to thank everyone here (especially Boobaa :) for their great work.

We are still missing the update for existing sites and the update for broken 7.7 sites. Both of these are blocked by #1182290: Add boilerplate upgrade path tests.

plach’s picture

Issue summary: View changes

Updating issue summary after the first commit.

joris.verschueren’s picture

I have found out that the problem with my images that didn't show was caused by the fact that the image files have disappeared from the /files/images folder. when restoring them to the /files/images folder, things work as expected.
I assume that this has nothing to do with the core modules or this patch.
the /files/images folder in my installation was a duplicate from another post-upgrade D7 testsite where I followed a squarely different upgrade path (D6 image.module -> D7 core image module + image legacy). It is very likely that these latter modules interfered with the content of 'their' folder. I wished I had noticed this before posting #159. Just to make sure, I'll go through an upgrade again to see when and why they are deleted from the folder.

It is clear that the patch works as expected for multilanguage sites (provided the images remain in place) and did solve my initial upgrade problem.
a big thank you to you developers!

cpelham’s picture

Thank you and a great job on a very thorny but important issue! I spent a lot of hours on this when D7 was first released but clearly not nearly as many hours as you guys did in coming up with a working upgrade script. I really appreciate it.

chx’s picture

Status: Postponed » Closed (fixed)

Please post new issues from this point on if there are any.

chx’s picture

Issue summary: View changes

Updated the issue summary after the last commit.

plach’s picture

Title: (regression) Node bodies display as empty after upgrade to 7.7 from 6.x if the nodes have a language » Inconsistencies in field language handling

I updated the issue summary and created a dedicated issue for #1266430: Updates for inconsistencies in field language handling where I posted @sun's latest patch.

Reverting the title to the original one since with D7.8 out people should not need to look for this anymore. Moreover this is linked from the release notes.

Gábor Hojtsy’s picture

Issue tags: +language-content

Tagging for the content handling leg of D8MI.

my-family’s picture

Do I understand it well, that: if I now install a fresh Drupal 7.12 and I see all fields with 'und' key (even if they ARE translatable and translated), it is correct? It is not a bug?

And I will have no problems with updates from earlier D7 versions, where I have, e.g., 'en' and other language keys?

Thank you in advance.

WilliamB’s picture

I'm confused, i installed a Drupal 7.12 setting up French as default language during the installation.
However when i create a content type, the node language is set to fr but same as #171, all my field are using the und key.

Why arent they using fr ?

Please help....

plach’s picture

Just read the opening post.

WilliamB’s picture

Plach, sorry i've read the original post, but this is a bit too advanced for me to be able to follow through.
I'm using Drupal 7.12, all my fields are created through the content type creation UI.
Locale has been enabled since the install, as i set the default language during the install with the french translation pack.

If i understand right, now all the field will be created with und if there is no multi-language going on.

A previous site i did in 7.7 seems to have some fields in und and some in fr. However i updated it to Drupal 7.12, created new content and the field seems to still use fr as they should.
Do i risk breaking my old site by upgrading to 7.12?

If you could be so kind as to give me some pointer, i would be very grateful...

plach’s picture

Ah, you are updating your site to D7.12! Read #1266430: Updates for inconsistencies in field language handling, you shpould find some answers.

WilliamB’s picture

Thanks for your answers.

In fact i'm working on 2 different sites.

1) is a new Drupal site, started off with 7.12, with the default language being fr.
All the fields are set in the database as translatable = 0, resulting in all the node's language being fr but all field being und. This seems to be the new way this is handled when there is no multilangue, or at least this is what i understood from the top post.
Thanks for clearing it that up.

2) I've another site, which was originaly developped in 7.2, then 7.7 and that i want to upgrade to 7.12.
There are some fields are that in und and some other in fr, although there was never multilanguage going on. Checking in the database, i see that some fields are translatable = 0 and some other = 1.

I've tried creating new contents with existing content type, no problem fields that were previously set as fr are still fr.
However creating new content type generates an error: "DatabaseSchemaObjectExistsException : the table field_data_body already exists. in DatabaseSchema->createTable() (line 652 in D:\Projects\my site in Drupal 7.12\includes\database\schema.inc)."

I'm still confused as to what is going on with the update to 7.12. A lot of my existing code is picking on field through fr and und and if they were to change i would have to edit a lot of code.

Anyway thanks for the info about the field_config table. Didn't know where language were set.

plach’s picture

The error you are reporting does not seem related to this issue, try the solution propsed in http://drupal.org/node/1266430#comment-5183572 to fix the language of your fields if you have no multilingual features enabled.

xjm’s picture

xjm’s picture

Issue summary: View changes

Updated remainig tasks after D7.8 release.