Updated: Comment #99

Problem/Motivation

Follow up for #1498674: Refactor node properties to multilingual. Node base fields are translatable in storage but not configurable in the UI.

Steps to Test

From Kristen Pol in #30 (plus updates).

  1. Reset 8.x
  2. Apply the latest patch
  3. Install Drupal in English
  4. Enable language and content translation modules
  5. Add a language
  6. Go to content language settings page (admin/config/regional/content-language) Home > Administration > Configuration > Regional and language
  7. For Custom Language Settings, check content, and then check translatable for article.
  8. Notice checkbox for title (not to be confused with the title of the image field).

Proposed resolution

Show on the content language settings page also non-configurable fields having the translatable property set to true in their definition. Content Translation uses this information to tell whether the field supports translation and alters the field definition based on the actual translatability status stored in its own config settings. For instance:

  1. The node title definitions says it is translatable
  2. The content language settings page shows it as untranslatable, because CT has no settings stored for it yet
  3. Node title is marked as translatable in the UI
  4. Content Translation alters the field definition to reflect that (actually nothing changes)
  5. Node title is marked as untranslatable in the UI
  6. Content Translation alters the field definition to reflect that (the field definition is switched to untranslatable)

The title field is used as a proof of concept here, other node fields will be handled in a follow-up.

Remaining tasks

  • Write a patch
  • Add test coverage
  • Reviews
  • Make the issue summary proposed resolution accurate

User interface changes

Properties will show up alongside configurable fields in node translation configuration.

Before (from #46):
2004626screen1.png

After (from #46):
2004626screen7.png

Without the patch, the admin content listing has a row for each translation and the original source node title is shown for all. With the patch, the translated title is shown for translation, gotten from the database which now saves the translated title for each translation.

After (from #46):
2004626screen8.png

API changes

An exception is thrown when trying to make entity id, uuid, revision id, bundle and language fields translatable.

In #38 on 6/27, @Dries states,

As this discussed with the D8MI team, this is part of 4 critical issues for D8MI:

. So this is an approved API change at #43

CommentFileSizeAuthor
#4 et-nc_field_config-2004626-4.patch12.59 KBplach
#22 et-nc_field_config-2004626-22.patch8.81 KBplach
#24 et-nc_field_config-2004626-24.patch8.96 KBplach
#28 et-nc_field_config-2004626-28.patch9.23 KBplach
#30 d8mi-prop-trans-content-config-all-lang.png167.65 KBKristen Pol
#30 d8mi-prop-trans-content-config-no-props.png47.71 KBKristen Pol
#37 et-nc_field_config-2004626-37.patch9.26 KBPancho
#46 2004626screen9.png54.57 KBandymartha
#46 2004626screen8.png31.65 KBandymartha
#46 2004626screen7.png58.64 KBandymartha
#46 2004626screen6.png47.14 KBandymartha
#46 2004626screen5.png24.43 KBandymartha
#46 2004626screen4.png73.04 KBandymartha
#46 2004626screen3.png56.41 KBandymartha
#46 2004626screen2.png61.69 KBandymartha
#46 2004626screen1.png75.28 KBandymartha
#49 2004626-non-configurable-field-translation-49.patch9.15 KBvijaycs85
#51 2004626-diff-49-51.txt3.54 KBvijaycs85
#51 2004626-non-configurable-field-translation-51.patch9.18 KBvijaycs85
#57 2004626-non-configurable-field-translation-51-57-interdiff.txt4.28 KBkfritsche
#57 2004626-non-configurable-field-translation-57.patch13.01 KBkfritsche
#57 2004626-non-configurable-field-translation-57-test-only.patch1.23 KBkfritsche
#59 2004626-non-configurable-field-translation-59.patch10.79 KBkfritsche
#59 2004626-non-configurable-field-translation-57-59-interdiff.txt1.56 KBkfritsche
#69 et-nc_field_config-2004626-69.patch16.57 KBplach
#69 et-nc_field_config-2004626-69.interdiff.txt8.47 KBplach
#70 content-language-settings.png106.94 KBkfritsche
#70 et-nc_field_config-2004626-70.patch16.56 KBkfritsche
#70 et-nc_field_config-2004626-69-70-interdiff.txt1.09 KBkfritsche
#73 et-nc_field_config-2004626-73.patch16.59 KBpenyaskito
#73 interdiff.txt815 bytespenyaskito
#82 et-nc_field_config-2004626-82.patch20.36 KBplach
#82 et-nc_field_config-2004626-82.interdiff.txt5.01 KBplach
#84 et-nc_field_config-2004626-84.patch73.46 KBplach
#87 et-nc_field_config-2004626-87.interdiff.txt1.89 KBplach
#87 et-nc_field_config-2004626-87.patch21.11 KBplach
#94 et-nc_field_config-2004626-94.patch17.47 KBplach
#94 et-nc_field_config-2004626-94.interdiff.txt1.45 KBplach
#98 et-nc_field_config-2004626-98.patch19.02 KBplach
#98 et-nc_field_config-2004626-98.interdiff.txt3.04 KBplach
#101 et-nc_field_config-2004626-101.patch19.02 KBplach
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Title: Properties Translation UI » Make node property translation configurable with field translation
Issue tags: +sprint

Retitle to be more specific, add sprint tag.

Pancho’s picture

Issue tags: +Entity Field API

Leveraging the new Entity Field API. The tag hopefully bring additional visibility to this one.

plach’s picture

Assigned: Unassigned » plach

Working a bit on this.

plach’s picture

Status: Active » Needs review
FileSize
12.59 KB

Here is an initial patch. Keep in mind that although the configuration screen works correctly, node translation support is stilly buggy due to the lack of #1810370: Entity Translation API improvements. AAMOF you'll be able to enter translated values (you can check the DB), but node form and visualization will still always present (non-configurable) field values in the original language.

Status: Needs review » Needs work

The last submitted patch, et-nc_field_config-2004626-4.patch, failed testing.

andypost’s picture

+++ b/core/modules/translation_entity/translation_entity.admin.incundefined
@@ -85,35 +87,55 @@ function _translation_entity_form_language_content_settings_form_alter(array &$f
+          $field_settings = translation_entity_get_config($entity_type, $bundle, 'fields');
+          foreach ($fields as $field_name => $definition) {
+            $translatable = !empty($field_settings[$field_name]);

this makes all fields translatable so no ability to prevent field translatable

plach’s picture

this makes all fields translatable so no ability to prevent field translatable

I dont' see why: the patch handling of non-configurable fields simply ignores fields that are maked as non translatable in the definition, hence there will never be a config setting for those fields. Non-configurable fields that are marked as translatable can have translation enabled/disabled from the UI.

plach’s picture

Assigned: plach » Unassigned

I'll work on another issue tonight. Feel free to work on this.

yched’s picture

Does this mean the 'translatable' bool property on "Field API" configurable fields would/should be stored by translation_entity.module in a CMI record of its own, rather than as part of the $field config entity currently ?

plach’s picture

Yes, currently this is the situation. I already thought this might need some kind of cleanup as I don't really like the current special-casing of Field API fields: #2018685: Remove field_is_translatable() seems the right place to do that clean-up :)

Once we are done with the Field NG conversion, we could make all Field API fields translatable by default and store any alteration to their translatability status in the ET settings only. That is we would extend the patch behavior for non-configurable fields to all fields.

yched’s picture

Sounds like a sensible approach, +1.

plach’s picture

Yesterday I tried this combined with #1810370: Entity Translation API improvements but it still fails, because we need also the node form parts of #1939994: Complete conversion of nodes to the new Entity Field API and #2019055: Switch from field-level language fallback to entity-level language fallback for everything to work correctly.

This will also be heavily affected by #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets, as it turned out in #2018685: Remove field_is_translatable(). If we want to commit this now, I am afraid we need to revert node changes and reduce the scope to the ET module alone. If we want to see translated titles we need to postpone this to the issues above :(

Gábor Hojtsy’s picture

How can we fix (and get committed) all these issues in two weeks?

Gábor Hojtsy’s picture

Status: Needs work » Postponed
plach’s picture

Status: Postponed » Needs work

Well, I hope #1810370: Entity Translation API improvements and #2019055: Switch from field-level language fallback to entity-level language fallback will be ready this week. But in no way the node conversion will be complete in two weeks, I'd guess.

That's why I'd suggest to rescope this to the test entity for now. Once the conversion is over, enabling the node properties on the UI (or the ones of any other entity type) will be automatic by simply setting their field definitions to multilingual.

plach’s picture

Gábor Hojtsy’s picture

If we only solve this for the test entity, it will still be an API change for nodes, no? Nodes are our most important entities in core, so if we say we have an API freeze and then freely change node behaviour, that is not too honest.

plach’s picture

I don't see this as an API change: the whole functionality would be in place. The only change would be in node field definitions, which as part of the NG conversion should be allowed to be done after code freeze, if I didn't misunderstand http://buytaert.net/reducing-risk-in-the-drupal-8-release-schedule.

Anyway, the only viable alternative I see is fixing tests and committing this as-is, although translatable titles won't work because of the incomplete node NG conversion.

Gábor Hojtsy’s picture

Status: Needs work » Postponed

"as part of the NG conversion should be allowed to be done after code freeze" sounds like even more things API changing to be done after a freeze? Anyway, sounds like there is no point in this issue in itself, let's work on the others.

plach’s picture

I really think we should try and get the non-node changes in ASAP, just in case. I don't see the risk honestly. If we are'nt allowed to make node field definitions translatable afterwards, too bad, but the situation will be way more complex if we won't have this in.

Gábor Hojtsy’s picture

Status: Postponed » Needs work
plach’s picture

Title: Make node property translation configurable with field translation » Make non-configurable field translation settings available in the content language settings
Status: Needs work » Needs review
FileSize
8.81 KB

Ok, rescoped the issue. I reverted the node-related changes. I'll work on test coverage tonight.

Status: Needs review » Needs work

The last submitted patch, et-nc_field_config-2004626-22.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
8.96 KB

This should be green.

Kristen Pol’s picture

Kristen Pol’s picture

I got an error when patching so I'm running tests again. I tried patching two times and same result for both.

[mac:kristen:d8dev]$ patch -p1 < et-nc_field_config-2004626-24.patch
patching file core/lib/Drupal/Core/Entity/DatabaseStorageController.php
Hunk #1 FAILED at 583.
1 out of 1 hunk FAILED -- saving rejects to file core/lib/Drupal/Core/Entity/DatabaseStorageController.php.rej
patching file core/modules/translation_entity/translation_entity.admin.inc
patching file core/modules/translation_entity/translation_entity.module
***************
*** 583,595 ****
          $hooks = array('entity_field_info', $this->entityType . '_property_info');
          drupal_alter($hooks, $this->entityFieldInfo, $this->entityType);
  
-         // Enforce fields to be multiple by default.
-         foreach ($this->entityFieldInfo['definitions'] as &$definition) {
-           $definition['list'] = TRUE;
-         }
-         foreach ($this->entityFieldInfo['optional'] as &$definition) {
-           $definition['list'] = TRUE;
          }
          cache()->set($cid, $this->entityFieldInfo, CacheBackendInterface::CACHE_PERMANENT, array('entity_info' => TRUE));
        }
      }
--- 583,600 ----
          $hooks = array('entity_field_info', $this->entityType . '_property_info');
          drupal_alter($hooks, $this->entityFieldInfo, $this->entityType);
  
+         // Enforce fields to be multiple and untranslatable by default.
+         $base_fields = array_flip(array_filter(array($this->idKey, $this->revisionKey, $this->bundleKey, $this->uuidKey, 'langcode')));
+         foreach (array('definitions', 'optional') as $key) {
+           foreach ($this->entityFieldInfo[$key] as $name => &$definition) {
+             $definition['list'] = TRUE;
+             // Ensure base fields are never made translatable.
+             if (isset($base_fields[$name]) || !isset($definition['translatable'])) {
+               $definition['translatable'] = FALSE;
+             }
+           }
          }
+ 
          cache()->set($cid, $this->entityFieldInfo, CacheBackendInterface::CACHE_PERMANENT, array('entity_info' => TRUE));
        }
      }

Status: Needs review » Needs work

The last submitted patch, et-nc_field_config-2004626-24.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
9.23 KB

Here is a reroll. Test coverage still todo.

Kristen Pol’s picture

@plach - Thanks for the reroll... I might be not configuring things correctly but I see "all languages" next to all my properties and the properties don't show up on the content language settings page for me to enable for translation. Am I missing a step?

Kristen Pol’s picture

Here's what I did:

  • Reset 8.x
  • Applied patch from #28
  • Installed Drupal in English
  • Enabled language and content translation modules
  • Added a language
  • Went to content language settings page
  • Chose all fields available (no properties were shown - see image below)
  • Added content in English
  • Translated content (all fields are translatable and all properties show "all languages" - see image below)

d8mi-prop-trans-content-config-no-props.png

d8mi-prop-trans-content-config-all-lang.png

plach’s picture

@Kristen:

This is just the API part now. As written above making node properties translatable is not working properly because the node form and visualization code are not fully converted to NG yet. The patch provides only the infrastructral changes needed to make those translatable once the NG conversion is complete.

If you wish to manually test this you can change the base field definitions in the node storage controller and make them translatable (add a 'translatable' => TRUE entry). See the earlier patches for details.

Kristen Pol’s picture

Ah... thanks for the explanation... as for:

If you wish to manually test this you can change the base field definitions in the node storage controller and make them translatable (add a 'translatable' => TRUE entry). See the earlier patches for details.

I'm not seeing anything obvious in the comments about how to do this. I don't know what the "node storage controller" is ;)

plach’s picture

I'm not seeing anything obvious in the comments about how to do this. I don't know what the "node storage controller" is ;)

lol, silly me :)

You have to change this file manually as described in #31.

Kristen Pol’s picture

Hmm... tried that and cleared the cache multiple times but the properties still are changing for both languages upon node editing (and still say "all languages"). I'll leave it up to someone more familiar with this issue to test.

plach’s picture

Gábor Hojtsy’s picture

Component: translation_entity.module » content_translation.module
Pancho’s picture

Dries’s picture

Priority: Major » Critical

As this discussed with the D8MI team, this is part of 4 critical issues for D8MI:

As the product owner, I consider this to be critical for Drupal 8. Not being able to translate node titles is bad. Not having this in core would be a regression from Drupal 7.

plach’s picture

Gábor Hojtsy’s picture

@plach: what do you think should still be done here?

plach’s picture

I think we are just missing some test coveragel I'll check later tonight.

plach’s picture

Gábor Hojtsy’s picture

Adding tags as per #38.

Gábor Hojtsy’s picture

andymartha’s picture

I am working on a review message + issue summary now; hold on...

andymartha’s picture

After applying the patch et-nc_field_config-2004626-37.patch by Pancho in #37 to a Drupal 8 fresh install 8/16 , here are my findings via screenshots.

1) drupal head 8/16/13 does not contain settings for translatable node titles at admin/config/regional/content-language
2004626screen1.png

2) without patch et-nc_field_config-2004626-37.patch, following steps at #30 , the title is still an "all languages" field and there is an error message shown as "invalid argument supplied for foreach() ...
2004626screen2.png

3) without patch, the translated node shows a translated body without translated node title
2004626screen3.png

4) after applying patch, the error message goes away in #2 but the node edit form title field still displays "all languages"
2004626screen4.png

5) before/after applying patch, editing a translated version of the saved node for me produces a WSOD.
PHP Fatal error: Call to a member function bundle() on a non-object in /Applications/MAMP/htdocs/drupal/core/modules/node/node.module on line 136
This error is being addressed in #1831846: Help block is broken with language path prefixes
2004626screen5.png

6) after applying patch, creating a new node, then creating a translation of that node, it shows the translated title in drupal_set_message save message and saved in the database in table node_field_data
2004626screen6.png

7) after applying patch, Drupal DOES contain settings for translatable node titles at admin/config/regional/content-language (this is a change to point 1)
2004626screen7.png

8) the content page at admin/content shows the translated titles of the nodes. Good. But, for example, node/1 shows up as Node 1 and es/node/1 shows up as Cosa Uno, but both link to the original translation...ie...node/1
2004626screen8.png

9) viewing the translated node still shows the original language node title at es/node/3 instead of translated title, but it seems that that may not be this issue's problem.
I'm not sure I understand the actual goal of this issue totally yet but as @plach says in #31

As written above making node properties translatable is not working properly because the node form and visualization code are not fully converted to NG yet. The patch provides only the infrastructral changes needed to make those translatable once the NG conversion is complete.

2004626screen9.png

andymartha’s picture

Issue summary: View changes

Update

andymartha’s picture

Issue summary: View changes

from the mwds sprint on technical debt

YesCT’s picture

Issue summary: View changes

a bit of formatting.

YesCT’s picture

Issue summary: View changes

tried to update resolution, added more issues that have come up as related

YesCT’s picture

Issue summary: View changes

more formatting.

andymartha’s picture

Issue summary: View changes

after discussing with YesCT, decided to clarify UI changes section.

YesCT’s picture

Issue summary: View changes

wasn't really a UI change, saving to the db. but the content listing is a ui change.

YesCT’s picture

Issue summary: View changes

put ui change screenshots in summary

YesCT’s picture

Issue summary: View changes

putting class name in text

YesCT’s picture

Issue summary: View changes

added remaining task to still update proposed resolution.

Berdir’s picture

The last submitted patch, et-nc_field_config-2004626-37.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
9.15 KB

Re-rolling #28

Status: Needs review » Needs work

The last submitted patch, 2004626-non-configurable-field-translation-49.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
3.54 KB
9.18 KB

Fixing test fails in #49

rteijeiro’s picture

I'm going to review this one :)

rteijeiro’s picture

Status: Needs review » Needs work

Patch applied and followed the steps to reproduce but it's still not working.

pfrenssen’s picture

Issue tags: +Needs tests

If it is not working and the tests are green, then we have a problem with our test coverage.

vijaycs85’s picture

The patch in #51 is green, but it is not doing what it suppose to. We don't have the title field (#8 in steps to test).

vijaycs85’s picture

The patch in #51 is green, but it is not doing what it suppose to. We don't have the title field (#8 in steps to test).

kfritsche’s picture

Adding a very simple test which just tries to enable the title in the content translation settings page.
This fails if not exists.
Added a test-only patch for this, which has to fail.

Second attached patch is the patch from #51 with the test and a temporary fix to make title translatable like discussed with vijaycs85.

Also did a manual testing on the patch.
* Title appears now in the settings form.
* Title is always translatable - setting doesn't change this

This is because content_translation_entity_field_info_alter won't be called as title is no field. So the settings is never really called.
Also I'm not sure if the syncing would work, as it is no field and so it will be not synced.

But the main intention of the ticket to make it possible to configure the properties is working and we now have a small test for it.

Status: Needs review » Needs work

The last submitted patch, 2004626-non-configurable-field-translation-57.patch, failed testing.

kfritsche’s picture

Something seems still not working with the comment subjects.
Removed comment subject as translatable to get the tests passing.

vijaycs85’s picture

Great work @kfritsche, can you post a test-only patch as well please?

kfritsche’s picture

@vijaycs85: See #57 first attachment there is the test only patch.

As you can see in the details there, this is failing with another error than the second "real" patch.
Patch in #59 just fixed this new error.

andypost’s picture

probably this broken because we are going to drop the way we translate fields, see @plach comments in content translation form controller issue

plach’s picture

@andypost:

That's a bit misleading :)

To clarify for people here, I just said we should drop the current code to migrate data when switching field translatability, as it will hopefully become unnecessary. See #2076445: Make sure language codes for original field values always match entity language regardless of field translatability.

penyaskito’s picture

Status: Needs review » Needs work

Only the title config is translatable at the moment, and there are a lot more, but can be handled as follow-ups.

When I mark the title for translation, the title is translation on the UI. However, hen I save a translation, the translation is not shown, but the source, (the process ends successfully, but after editing a node the title is shown in the target language.

Clearing cache does not help to fix the problem.

Gábor Hojtsy’s picture

I'm not entirely clear on this. Is the title saved and loaded back properly in the form? Is it only not applied on the display? Is the title saved at the end to the db?

Gábor Hojtsy’s picture

@penyaskito: So as for displaying the proper node title for the translation, #2019055: Switch from field-level language fallback to entity-level language fallback is the issue that generally solves that for fields. This issue would be the configuration so the right form element shows and its saved to the right place (and loaded back on the next form). If the title is not rendered properly at the end, it is still fine for this patch.

We need to ensure here that the form saves to DB and edit again in that language works and original language edit also works and is still the original value. Tests for this would be essential too :)

penyaskito’s picture

Status: Needs work » Needs review

We need to ensure here that the form saves to DB and edit again in that language works and original language edit also works and is still the original value.

This works. The title is saved to node_field_data correctly, and it is loaded correctly in the edit form.

Tests for this would be essential too :)

AFAIK, ContentTranslationUITest::assertBasicTranslation is testing this in a generic way, but there is no explicit test for the title. I would say that still needs tests.

plach’s picture

Assigned: Unassigned » plach

I'm working on improving test coverage, patch coming soon.

plach’s picture

Ok, cleaned-up some stuff and added test coverage for node title translation and display. The latter is done by checking the output of the translation overview page since, as Gabor was pointing out above, display will need to be fixed after #2019055: Switch from field-level language fallback to entity-level language fallback goes in. We should open a follow-up to make all node base fields are translatable (just change their definitions) and ensure node titles appear in the correct language everywhere.

kfritsche’s picture

  1. +++ b/core/modules/content_translation/content_translation.admin.inc
    @@ -93,24 +95,42 @@ function _content_translation_form_language_content_settings_form_alter(array &$
    +            // @todo Remove this special casing as soon as condfigurable and
    

    Minor spelling -> Fixed it

Setting it to RTBC as couple of people reviewed it and as Gabor mentioned in #66 this is just so that the title field is available. For the part mentioned by penyaskito in #64 we have another issues.

Also did a final manual check. The title appears now correctly in the content language settings page.

content-language-settings.png

kfritsche’s picture

Status: Needs review » Reviewed & tested by the community

RTBC when testbots agree once more. (Should be just a comment change)

alexpott’s picture

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

Patch no longer applies.

penyaskito’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
16.59 KB
815 bytes

Rerolled.

plach’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

fago’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -495,13 +495,20 @@ public function getFieldDefinitions($entity_type, $bundle = NULL) {
+            // Ensure base fields are never made translatable.
+            if (isset($base_fields[$name]) || !isset($definition['translatable'])) {
+              $definition['translatable'] = FALSE;

I cannot follow the reasoning here. We have translatable node fields, so why shouldn't they be marked as translatable?

plach’s picture

Status: Needs work » Reviewed & tested by the community

I cannot follow the reasoning here. We have translatable node fields, so why shouldn't they be marked as translatable?

We are making the node title translatable just as a proof of concept. To make every node property translatable we might need to adapt some code to ensure everything works correctly in a multilingual context, which is out of scope here, we need a dedicated issue for that.

fago’s picture

Status: Reviewed & tested by the community » Needs work

Still, I cannot follow the reasoning of the code cited in #75. Why would we dis-allow base fields to be translatable? If the node title is translatable, we need a way to mark it as translatable - but the patch removes this?

plach’s picture

Status: Needs work » Needs review

The patch ensures that only entity_id, revision_id, bundle, uuid and langcode fields are never made translatable. Maybe the $base_fields variable name is misleading.

Then it just makes field definitions default to untranslatable, that is we require fields to be explicitly set to translatable. Aside from title, which is already made translatable, we will define node fields as translatable in a follow-up.

plach’s picture

@fago:

I honestly do not understand your concerns in turn :)
Since this has been stuck way too much in the past months, can we please unblock this situation quickly?

fago’s picture

The patch ensures that only entity_id, revision_id, bundle, uuid and langcode fields are never made translatable. Maybe the $base_fields variable name is misleading.

I see, thanks. Yeah "base fields" is really misleading - it's just about "entity keys" then.

But still, why do we need the special case for those? Content translation should not try to enable translation for them in the first place, so if we need to special case it why not do it on the content translation side?

If we want the entity system to forbid translation being active on those fields, it shouldn't silently change it around but throw an exception to tell developers that something is wrong and won't work as defined.

> Since this has been stuck way too much in the past months, can we please unblock this situation quickly?

yeah, sry for that. Feel free to ping me on irc/skype/mail regarding to topic to sort it out asap.

plach’s picture

If we want the entity system to forbid translation being active on those fields, it shouldn't silently change it around but throw an exception to tell developers that something is wrong and won't work as defined.

Ok, this works for me either. I'll provide a new patch ASAP.

plach’s picture

Here is the exception plus test coverage.

Status: Needs review » Needs work

The last submitted patch, et-nc_field_config-2004626-82.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
73.46 KB

No failures here, trying a reroll.

Gábor Hojtsy’s picture

@fago: ok, what do you think?

fago’s picture

Yeah, the interdiff looks good - thanks. However, looking at the patch size I guess the latest re-roll includes something else also?

plach’s picture

Sorry, I was on the wrong branch.

We actually have a new failure due to #2106349: Comment translation overview has broken local tasks, which completely breaks the comment translation overview. We need either to postpone this on that one or provide a stop gap fix and skip testing the comment translation overview.

Since for the purpose of this issue having nodes test-covered is enough, the attached patch implements the latter.

Gábor Hojtsy’s picture

Yeah I don't think this is committable as-is due to the comment workaround. Let's fix that. I posted a patch there that needs a slight update I think to apply/pass :)

Gábor Hojtsy’s picture

Status: Needs review » Postponed
plach’s picture

Status: Postponed » Needs review

OTOH if we commited this, it would be way easier to provide test coverage for the other issue.

plach’s picture

Status: Needs review » Postponed

crosspost

Gábor Hojtsy’s picture

Status: Postponed » Needs work

#2106349: Comment translation overview has broken local tasks landed. Let's reroll this without the workaround and then get it in :)

plach’s picture

Assigned: Unassigned » plach

On it

plach’s picture

Assigned: plach » Unassigned
Status: Needs work » Needs review
FileSize
17.47 KB
1.45 KB

Rerolled and removed the workaround :)

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC as per #86. Although the latest interdiff does not include removal of the doTest... changed from the previous patch, I manually verified they are not there anymore either.

swentel’s picture

+++ b/core/modules/content_translation/content_translation.admin.inc
@@ -84,31 +86,51 @@ function _content_translation_form_language_content_settings_form_alter(array &$
+            // @todo Remove this special casing as soon as condfigurable and

nitpick, typo in 'condfigurable'. Don't want to hold up on the commit for that though, can maybe cleaned up during that :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, et-nc_field_config-2004626-94.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
19.02 KB
3.04 KB

Fixed a merge issue and the image field sync test, which was not behaving correctly after the changes introduced here. Hopefully we should be green again now.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

The typo from #96 was also fixed :) Re-rtbc :)

Gábor Hojtsy’s picture

Issue summary: View changes

about duplicates in content list.

plach’s picture

Issue summary: View changes

Updated issue summary

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue tags: -Needs tests
plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Just a reroll

YesCT’s picture

+++ b/core/modules/content_translation/content_translation.admin.inc
@@ -84,31 +86,51 @@ function _content_translation_form_language_content_settings_form_alter(array &$
+            // @todo Remove this special casing as soon as configurable and
+            //   base field definitions are "unified".
+            if (!empty($definition['configurable']) && ($field = FieldService::fieldInfo()->getField($entity_type, $field_name))) {
...
+                // @todo This should not concern only files.
+                if (isset($column_element['#options']['file'])) {
+                  $dependent_options_settings["settings[{$entity_type}][{$bundle}][columns][{$field_name}]"] = array('file');

do we want issues opened for these?

plach’s picture

Not sure about that, those lines would probably need to be converted in the issue actually unfiying field definitions.

YesCT’s picture

plach’s picture

Yes, I did not look at that one yet. If the Field API field definitions are removed then it will need to convert also the code we have here, otherwise follow-up :)

catch’s picture

Status: Reviewed & tested by the community » Fixed

I was going to ask if we need issues for those as well, but if we already have an issue that's fine.

Committed/pushed to 8.x, thanks!

catch’s picture

Title: Make non-configurable field translation settings available in the content language settings » Change notice: Make non-configurable field translation settings available in the content language settings
Priority: Critical » Major
Status: Fixed » Active

Or this..

Gábor Hojtsy’s picture

Looks like the next big issue is #2019055: Switch from field-level language fallback to entity-level language fallback which would let us work on fixing the *display* of the translations to actually work, which it does not do yet. What is blocking making other node properties configurable to be translatable?

Also I'm not sure I can meaningfully write a change notice for this, so hope plach can do it :)

plach’s picture

Status: Active » Needs review

Not much to say on the API side, changes here are mostly in the Content Translation code and UI, but AFAIK we do not provide HEAD 2 HEAD change notices so I didn't mention those:

https://drupal.org/node/2111871

plach’s picture

What is blocking making other node properties configurable to be translatable?

Strictly speaking, nothing. I created #2111887: Regression: Only title (of base fields) on nodes is marked as translatable for that. However it would be probably better to have #2019055: Switch from field-level language fallback to entity-level language fallback in first.

fago’s picture

I'm wondering why we need a change notice here at all? "Field definitions for entity keys and language cannot be made translatable" is not new compared to D7 - they've never been field-translatable?

Gábor Hojtsy’s picture

IMHO the significant change in this issue is rather that if you define a base field in your baseFieldDefinitions() as 'translatable' => TRUE, then it will be offered up for translatability configuration and properly participate in translation forms (if configured translatable) and stored translated proper. That is what was not here before this issue :)

The exception, yeah, well, that is also there.

Gábor Hojtsy’s picture

Note that I found #2112303: Random extra text around translatability configuration is confusing while reviewing what this patch changed, but that is not related to what this patch changed. Just very confusing.

YesCT’s picture

yeah, I tried out #112 and got the revision log to show up in the language settings page.

plach’s picture

I am not sure this is exactly change notice material, at least according to the idea I had of those, however I added the information suggested in #112.

plach’s picture

Status: Needs review » Reviewed & tested by the community

Changes from Gabor look good to me, I updated the change notice to clarify that this applies to any field, not just base fields.

Gábor Hojtsy’s picture

Title: Change notice: Make non-configurable field translation settings available in the content language settings » Make non-configurable field translation settings available in the content language settings
Priority: Major » Critical
Status: Reviewed & tested by the community » Fixed
Issue tags: -sprint

I made some further changes as pointed out by @plach, making sure there is cross-reference for needing to make the schema multilingual as well as that the property does not itself become translatable with this definition, it just informs the system it can be made translatable. Full change notice at https://drupal.org/node/2111871. I think this should be good now.

plach’s picture

To answer to #102: if the @todos are not cleaned-up in the unification issue we will deal with those in #2018685: Remove field_is_translatable().

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

klonos’s picture

Issue summary: View changes
Issue tags: +API change, +D8MI, +language-content, +Entity Field API, +translation editorial workflow, +language-content-property, +Approved API change
Parent issue: » #1810370: Entity Translation API improvements
Related issues: +#1893568: Make each field instance have its own translatable property, instead of sharing translatable property with others., +#1939994: Complete conversion of nodes to the new Entity Field API, +#1950632: Create a FieldDefinitionInterface and use it for formatters and widgets, +#2006606: Views rendering ignores entity language, eg: Identical nodes rendered in views when nodes have translations, +#2018685: Remove field_is_translatable(), +#2019055: Switch from field-level language fallback to entity-level language fallback

...restoring tags eaten by the tag monster.

While at it, also moved related issues to their dedicated metadata section.

klonos’s picture