Regression: Only title (of base fields) on nodes is marked as translatable

Bump priority since this is a regression from Drupal 7.

#2019055: Switch from field-level language fallback to entity-level language fallback landed as well, so all dependencies are in.

I started to solve the issue and made a patch for it. In patch made translatable uid, status, created, changed, promote, sticky, log properties. It seems to me it works.
I tested with two languages English and Hungarian.
A found an issue, the "ALL LANGUAGES" title every time appears near the settings label, see on the picture.

@plach: I don't think this would be committable with the UI saying it does not work :D Do you propose we postpone this or at least not commit this before #1498724: Introduce a #multilingual key (or similar) for form element?

@plach: re #9: Well, looks like the screenshot demonstrates the author, publishing info, etc. don't have the widget label right :/ All say they apply to all languages even though the configuration is fully multilingual on the screenshot.

Fixed the failures from the last test.
Problem here was that as the status is now translatable, we have to set the published set to the translation too, otherwise the translation was unpublished. Which lead to the errors.

This already proofed that we have a test for the status. Also looking at the other tests, we already had tests for title, created and uid. In the doTestAuthoringInfo Test I added sticky and promote so that we have tests for all base fields.

Also I had to add langcode to getFormSubmitAction method, as the status is now translatable and can differ from the entity.

Also did a manual test:

• Fresh installation of Drupal
• Enabled language, content_translation
• Added German language
• Made article and all fields translatable in content translation settings
• Created a new node
• Translated this node with different base field settings
• Checked that all base fields (title, author, created time, sticky, promoted, published) could be different in the original node and the translated node

Node edit form of original node (after translation was done, to check if all values are still correct):

Node edit form of translated node (after translation was done, to check if all values are still correct):

Node view of original node (after translation was done, to check if all values are still correct):

Node view of translated node (after translation was done, to check if all values are still correct):

Seems to work fine for me now.

Used at two places the wrong langcode. When adding a translation the values of the source language entity will be used so I have to use the source language there for the submit message.

Should work now.

15: language-node_property-2111887-15.patch queued for re-testing.

Patch looks good, a few remarks:

1. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeTranslationUITest.php
   @@ -147,25 +149,36 @@ protected function doTestAuthoringInfo() {
   +    // Post different base field information for each translation.
   +    $flip = TRUE;
   

   Is there a reason for the flipping, could it just be random?

2. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeTranslationUITest.php
   @@ -230,6 +243,7 @@ function testTranslationRendering() {
   +    /** @var \Drupal\node\Entity\Node $node */
   

   Shouldn't be in there.

3. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeTranslationUITest.php
   @@ -237,7 +251,10 @@ function testTranslationRendering() {
   +      // Promoted and Published is translatable so it needs to be set for the
   +      // translation too.
   
   @@ -251,7 +268,7 @@ function testTranslationRendering() {
   +   * Tests that the given path displays the correct translation values.
   

   Maybe it could just say:
   // Publish and promote the translation to frontpage.

Fixed 1, 2 and 3 ;)

I changed the flip now to random. I wanted to just get sure there that these are different for sure for each language for my internal testing. Changed this too.

So this patch looks great now. I looked at how to solve the UI problem where it says "(all languages)" erroneously and **that is a whole mess**. The form items are nested in all kinds of different ways, so the code where it tries to match up the form[key] with the name of the item totally does not match. The #multilingual form element array key is already used extensively, so the title of #1498724: Introduce a #multilingual key (or similar) for form element is misleading at least... We need to figure out how to match up the node base fields to their form elements.... which sounds like would be the domain of #1988612: Apply formatters and widgets to rendered entity base fields, starting with node.title in fact...

Thanks, this looks ready to me. The UI problem is dealt with over at #1498724: Introduce a #multilingual key (or similar) for form element, or where?

Yeah but base fields don't have widgets, and not even after #1988612: Apply formatters and widgets to rendered entity base fields, starting with node.title (at least for anything beyond title). So we need an interim solution if we want this to land and find all the bugs related to this :) Like the search and entity reference bugs we found based on the title translatability already. I think its important to get this in as soon as possible, however, with the UI as-is I think its hard to argue this should land as-is. :(

Well, the problem now is promote is in $form['options']['promote'], uid is edited in $form['author']['name'], etc. There is really no relation of item names or their position in the form structure to base fields. Whatever we do to fieldsets is not going to help us match up items with elements.

Coming here from #2010930: [META] Apply formatters and widgets to rendered entity base fields (all besides node.title), and only reading #23 and #25, I see three options for how to move forward on this then:

1) Allow this patch in despite the UI problem.
2) Work on a subset of #2010930: [META] Apply formatters and widgets to rendered entity base fields (all besides node.title) in this issue (or a spin-off issue that this issue becomes postponed on). By subset I mean: only for those fields that this issue requires, and not changing any field types (e.g., not changing node.created from int to date), but rather creating temporary widgets/formatters to replicate existing behavior, just as a way to unblock this issue, and then cleaning that all up in #2010930: [META] Apply formatters and widgets to rendered entity base fields (all besides node.title).
3) Postpone this issue on #2010930: [META] Apply formatters and widgets to rendered entity base fields (all besides node.title), but that might be a while.

Based on #2010930-20: [META] Apply formatters and widgets to rendered entity base fields (all besides node.title), I actually think we should proceed here by renaming $form['author']['name'] to $form['author']['uid'] and similar for all other mismatched field elements, because seems like that's all that's needed to unblock this issue, and it will end up being needed by the work in that issue anyway. Would the people working on this issue prefer to do that in this issue, or a spin-off?

I can hardly imagine a core committer committing this issue without the UI being honest about how it works. So I think we need to rename the elements and do the traversing of the form to put in the right labels (and write tests for that :D). Updated the issue summary for that.

Rerolled quickly for the recent changes (in Node.php). Anybody can/want to help with the base field form element renames?

The search test should not assume specific order of results. Fixed and it works finding the same nodes (but possibly in a different order). Passes green for me now :)

The more I tried to restructure the form, the more evident it became that doing it here would be prohibitive and counter-intuitive. We should not mix up the translatability related changes with changing the whole form structure. Therefore I opened #2177469: Move node base widgets to the top level of the form and marking this postponed. Sadly :/

Ok, #2177469: Move node base widgets to the top level of the form is in now, that should make this unblocked.

32: language-node_property-2111887-32.patch queued for re-testing.

Rerolled so it applies again. Let's see this one.

One more thing got committed in the meantime.

getAuthorId() is now getOwnerId() following #2078387: Add an EntityOwnerInterface.

1. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeTranslationUITest.php
   @@ -147,25 +149,34 @@ protected function doTestAuthoringInfo() {
   +      $this->assertEqual($translation->getOwnerId() == $values[$langcode]['uid'], 'Author of translation correctly stored.');
   +      $this->assertEqual($translation->getCreatedTime() == $values[$langcode]['created'], 'Date of Translation correctly stored.');
   +      $this->assertEqual($translation->isSticky() == $values[$langcode]['sticky'], 'Sticky of Translation correctly stored.');
   +      $this->assertEqual($translation->isPromoted() == $values[$langcode]['promote'], 'Promoted of Translation correctly stored.');
   

   This is broken. Instead of doing the comparison (==) manually you should pass the two things that are equal as separate arguments.

2. +++ b/core/modules/search/lib/Drupal/search/Tests/SearchMultilingualEntityTest.php
   @@ -128,9 +128,11 @@ function testSearchingMultilingualFieldValues() {
   +    $this->assertEqual(in_array('Third node this is the Hungarian title', $results), 'The search finds the correct Hungarian title.');
   +    $this->assertEqual(in_array('Second node this is the English title', $results), 'The search finds the correct English title.');
   

   Similar problem: Here assertTrue() should be used.

yep.

https://api.drupal.org/api/drupal/core%21modules%21simpletest%21lib%21Dr...

https://api.drupal.org/api/drupal/core%21modules%21simpletest%21lib%21Dr...

I'll do this.

1. why didn't those fail?

2. they still pass.
needs review for the bot,

but needs work:

3. the language settings vertical tab on the content type (article) edit is empty with the patch!

4. And we might want to come up with an order for the base fields (used to be properties)

Order is:
User ID
Article Publishing status
Article Created
Article Changed
Article Promote
Article Sticky
Article Log

5. Now, the only place to set the base field language and translation settings is on the content language and translation overview page (other fields can have their language set, or enable translation on each field settings page accessible from the content type manage fields pages.

Is that ok?
I think so.

@YesCT: 1/2: they passed because assertEqual(TRUE, "some text") will pass, since "some text" is also TRUE.
3: can you check the #language_select field added? it may not be adding the right group now.
4: The order comes from the base field definitions. We should define them in an order that makes sense IMHO and/or introduce weights. But then defining them in an order that does not equal weights would be strange, so we should define them in the order that makes sense :D
5. Right, base fields dont have settings pages on their own. I think this is fine. Not in scope here IMHO.

Quick straight reroll since it did not apply.

@YesCT: I also tried different ways to reproduce the bug with the article content type editing, but could not. The language settings show up fine. Even cross-checked after adjusting language settings in different ways.

Looked into applying labels properly. Now that #2177469: Move node base widgets to the top level of the form landed, the base fields have form elements at the top of the form, so the code in content_translation_form_alter() automagically applies :) Yay. We only need to make details elements exempt from the label marking and then individual items will get proper labels. Such as in this case I configured author to be translatable but not dates and promoted to be translatable but not sticky. I think its probably safe to argue that this may be an artificial situation, but not sure how to handle this, the fields are individual not maintained in the API as field-groups, so we need to support settings on them one by one, we cannot enforce a group of fields to have the same setting. So the helpful note will be on the individual field.

I also fixed the base field labels, so User ID became Author and Promote became Promoted to be aligned with the others.

I've been looking into what to do with the submit button (which is to some degree status based), but that is very custom, not trivial to put into this system. Still pondering there :/

So the top items are #type item from the seven.theme file's form alter, not less! Since these are essentially the same as the other top items, we cannot put them into the form as top items either, they already have widgets with that name, duh. The 'published' item also would need this title addition, but that is special cased in the theme too. Hum. It should use an item title probably so we can alter it consistently. Still to figure this out, it does not seem to be trivial.

In this update I covered the buttons on the form. We need some kind of treatment for them, but not sure about this extra long text :/

Hm. I can't either, but probably worth tagging to see if any of the UX folks have better ideas.

The button labels in tests need an update as well. Will not fix all fails, but will fix quite some.

More button text changes needed in tests due to the button text change added above.

Translatability has other conditions that we need to check based on which the suffix may not be present at all. Heh.

Fixed error php.

Rerolled so it applies following changes to node changed and created field type changes.

Fixed the test, so it uses the right conditions (same as in runtime), which required $langcode to be passed further down. Also in some cases the langcode was not the proper one sent. Finally, add some comments to make clearer how this works. The term and user translation tests (both of which were failing before) work fine locally, let's see the full suite.

I have issues running NodeTranslationUITest locally, so that will be interesting... :/ Anyone with insights. If not soon, I think I will experiment with some alternate ways to run this test locally tomorrow. I could run the other tests that were failing before, but now this is the only one...

That test has lots of methods and each of them runs thrugh the lengthy set-up process. If you didn't try that yet, I'd suggest to disable (by renaming testBla() to dTestBla() all tests out except a single one that is failing and then just try that.

Had a look at this, the problem is that the test where it decides how the submit button is labelled, he sees 'it' as unpublished, but when the testcode itself runs, it is published. So they don't match.

I'm not sure why, I guess there's some sort of initialization going on that sets the default status when a new translation is added?

I'll work on this, I'm in the Multilingual table.

The issue is in the NodeTranslationUITest::getFormSubmitAction(). I guess that the logic there does not match the one in NodeFormController::actions(). In the later one, the text on the buttons depends on two variables (if the node is new and/or published). In the test function, we check only if the entity is published.

And more specific, this code in the test class:

if ($entity->getTranslation($langcode)->isPublished()) {

returns false, so the button will have the text: "Save and keep unpublished"
In the form controller class, the $node->isPublished() seems to be true. So, the buttons will have "Save and unpublish" and "Save and keep published" texts. No one matches the one from the test class...
This raises a question: is there an issue in the controller, or should we change the test class?

The logic is the same IMHO but the state of the entity is not. @fago is sitting front if me (is laptop is at least), I'll ask him.

Attached the code that should fix the tests.

@Berdir, @fago: can you please also have a look on it (especially the interdiff).

ok, let's try again. The interdiff should be the same.

Patch seems reasonable for me.
Tested it manually and the form is fine, but the labels aren't working at all, like mentioned in #49.

But currently it seems, like its the problem of the field definition.

      foreach ($entity->getFieldDefinitions() as $property_name => $definition) {
          $definition->isTranslatable(); // this returns always TRUE if the property is translatable, but don't care about the settings made in the content language overview
      }

What I did:
* Fresh install in German with patch
* Enabled content translation and added a language
* Only set body translatable for basic pages and everything (also the new base fields) for articles
* Created a article and basic page
* Created a translation for the article and the basic page
* Noticed similar behavior like in #49 (last saved, author has "all languages" and fields which shouldn't be translatable don't have "all languages")
* After saving the translations, I was able to have different values in the title, even that I disallowed for basic pages

Setting back to needs review, as the goal of the issue is working and everything seems good.

The issue I posted is tackled in #2217543: Cannot translate title of Basic Page node if other content types are not translatable and #2143291: Clarify handling of field translatability.

Not sure if we really have to postpone this now, as what this issue is about is working and tests are green. [me want this in finally...]

Rerolled and fixed the issues noted by plach.

Except the one with the search results. I ran the test locally a couple of times and it seems to switch the order from time to time. Will ask Gabor at D8MI meeting later about it.

Ah now I got it.

Moved the status part to the node translation and added a comment to the multilingual search part after talking to Gabor. Also moved the rest to the part it was again.

Since Gábor's been very involved here, it'd be nice to see him sign off on this RTBC.

The changes look good. Especially where the form suffix handling is moved to the node handler instead of the general content translation handler. We even discussed the search changes in person.

Committed 1ad4630 and pushed to 8.x. Thanks!

diff --git a/core/modules/node/lib/Drupal/node/NodeTranslationController.php b/core/modules/node/lib/Drupal/node/NodeTranslationController.php
index bb4d094..e934fef 100644
--- a/core/modules/node/lib/Drupal/node/NodeTranslationController.php
+++ b/core/modules/node/lib/Drupal/node/NodeTranslationController.php
@@ -55,7 +55,7 @@ public function entityFormAlter(array &$form, array &$form_state, EntityInterfac
       if (isset($status_translatable)) {
         foreach (array('publish', 'unpublish', 'submit') as $button) {
           if (isset($form['actions'][$button])) {
-            $form['actions'][$button]['#value'] .= ' ' . ($status_translatable ? t('(this translation)') : t('(all translations)'));
+            $form['actions'][$button]['#value'] .= ' ' . ($status_translatable ? $this->t('(this translation)') : $this->t('(all translations)'));
           }
         }
       }

Fixed the use of t() during commit.

Fixed the use of t() during commit.

Which seems to have broken HEAD. This patch just reverses that change.

Hm. No idea why that would be the case, but committed and pushed #90 to 8.x in the hope that fixes testbot. :)

Testbot trying to apply patch that's already committed to HEAD. Resetting status.

FWIW though, bizarrely, that did seem to fix the problem. https://qa.drupal.org/8.x-status is showing a green board now. So marking as "needs followup" because we should figure out a way to apply that patch in a way that doesn't break HEAD, but don't need to do so in a critical task.

Well, not very surprising, this is a translation controller, which doesn't have the t() or any other of those fancy methods :)

Oh, well there you go. ;)

Yay! Thanks all for your help!