Files: 
CommentFileSizeAuthor
#53 interdiff.txt903 bytesandypost
#53 drupal8.content-translation.module.1946462-53.patch11.25 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 58,526 pass(es).
[ View ]
#43 content_translation_translatable_form-1946462-43.patch3.85 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch content_translation_translatable_form-1946462-43.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#43 interdiff.txt3.08 KBtim.plunkett
#40 enabling-translation-before.png132.87 KBYesCT
#40 before-29-enabling.png123.84 KBYesCT
#40 before-29-disabling.png141.12 KBYesCT
#40 8x-enablelink.png169.28 KBYesCT
#39 1946462-second_followup-39.patch2.44 KBPancho
PASSED: [[SimpleTest]]: [MySQL] 58,175 pass(es).
[ View ]
#34 1946462-fix_t-34.patch1.08 KBYesCT
PASSED: [[SimpleTest]]: [MySQL] 56,830 pass(es).
[ View ]
#29 1946462-29.patch10.48 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 58,111 pass(es).
[ View ]
#29 interdiff.txt601 bytesjibran
#27 1946462-27.patch10.48 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 58,121 pass(es).
[ View ]
#23 1946462-23.patch10.43 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 57,993 pass(es).
[ View ]
#23 interdiff.txt1.48 KBjibran
#20 1946462-20.patch9.63 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 57,744 pass(es).
[ View ]
#20 interdiff.txt1.27 KBjibran
#16 1946462-16.patch9.64 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 57,868 pass(es).
[ View ]
#16 diff.txt2 KBjibran
#12 1946462-translation-entity-form-interface-12.patch9.37 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1946462-translation-entity-form-interface-12.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#12 interdiff.txt1.03 KBkim.pepper
#7 1946462-translation-entity-form-interface-7.patch9.29 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 54,586 pass(es).
[ View ]

Comments

Assigned:Unassigned» ayelet_Cr

You might want to just start with translation_entity_translatable_form(), since translation_entity_delete_confirm() is going to be very tricky because of the translation_entity_delete_access() access callback.

Assigned:ayelet_Cr» Unassigned

Unassigning as it's been more than a week.

She's still working on this one, actually. Just a short distraction.

Correction. She's moved on to another patch that isn't as tricksy. This is available if anyone wants it.

Assigned:Unassigned» kim.pepper

Status:Active» Needs review
StatusFileSize
new9.29 KB
PASSED: [[SimpleTest]]: [MySQL] 54,586 pass(es).
[ View ]

First go at this.

There is some weirdness with this that I don't quite get.

  1. submitForm() is calling a batch function in translation_entity.admin.inc which I don't think is loaded. I didn't see any examples of this anywhere to see how it should be done
  2. I'm not sure how to manually test this. I know it involves adding a language, making a field translatable, adding content to different versions of the field, then toggling off translatability, but I couldn't get it to show me a confirm form.

@kim.pepper:

submitForm() is calling a batch function in translation_entity.admin.inc which I don't think is loaded. I didn't see any examples of this anywhere to see how it should be done

All the migration code for field data is going away as soon as we complete the transition to the Entiy Field API. I think for now it's fine to manually include the file through form_load_include() if you need it.

I'm not sure how to manually test this.

I think the confirmation form is shown only when switching translatablity of a single field from the Field UI field configuration page.

Assigned:kim.pepper» Unassigned

Issue tags:+Needs manual testing

I need someone who knows translation system to test this. Not sure the automated tests are covering it.

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Form/TranslatableForm.phpundefined
@@ -0,0 +1,128 @@
+    $this->fieldInfo = field_info_field($field_name);

now we can use Field::fieldInfo() :)

StatusFileSize
new1.03 KB
new9.37 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1946462-translation-entity-form-interface-12.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Converted to using Field::fieldInfo() is per #11.

Status:Needs review» Needs work
Issue tags:+Needs manual testing, +FormInterface, +WSCCI-conversion

The last submitted patch, 1946462-translation-entity-form-interface-12.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2 KB
new9.64 KB
PASSED: [[SimpleTest]]: [MySQL] 57,868 pass(es).
[ View ]

reroll.

Not sure if you need/wish to proceed anyway, but the translatable migration code is going to be removed as soon as we are done with the Field NG conversion.

Status:Needs review» Reviewed & tested by the community

Let's not worry about that now. This eliminates another legacy route, and we need to get through those ASAP. :-)

Status:Reviewed & tested by the community» Needs work

Status:Needs work» Needs review
StatusFileSize
new1.27 KB
new9.63 KB
PASSED: [[SimpleTest]]: [MySQL] 57,744 pass(es).
[ View ]

reroll

Status:Needs review» Reviewed & tested by the community

OK then.

Status:Reviewed & tested by the community» Needs review

+++ b/core/modules/translation_entity/translation_entity.admin.inc
@@ -367,87 +367,6 @@ function _translation_entity_update_field_translatability($settings) {
- * This submit handler maintains consistency between the translatability of an
- * entity and the language under which the field data is stored. When a field is
- * marked as translatable, all the data in
- * $entity->{field_name}[Language::LANGCODE_NOT_SPECIFIED] is moved to
- * $entity->{field_name}[$entity_language]. When a field is marked as
- * untranslatable the opposite process occurs. Note that marking a field as
- * untranslatable will cause all of its translations to be permanently removed,
- * with the exception of the one corresponding to the entity language.

Was this documentation intentionally deleted rather than moved?

StatusFileSize
new1.48 KB
new10.43 KB
PASSED: [[SimpleTest]]: [MySQL] 57,993 pass(es).
[ View ]

docs added back.

Status:Needs review» Reviewed & tested by the community

Thanks. I haven't reviewed the full patch in detail, but it was RTBC already in #21, and the one thing I found on quick scanning was addressed, so back to RTBC.

Status:Reviewed & tested by the community» Needs work

So the module has been renamed in #2024867: Rename translation_entity to content_translation

git ac https://drupal.org/files/1946462-23.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 10679  100 10679    0     0   5067      0  0:00:02  0:00:02 --:--:--  5644
error: core/modules/translation_entity/translation_entity.admin.inc: does not exist in index
error: core/modules/translation_entity/translation_entity.module: does not exist in index

Component:translation_entity.module» ajax system
Assigned:Unassigned» jibran

Working on a reroll.

Component:ajax system» content_translation.module
Status:Needs work» Needs review
StatusFileSize
new10.48 KB
PASSED: [[SimpleTest]]: [MySQL] 58,121 pass(es).
[ View ]

Reroll

Title:Convert all confirm_form() in translation_entity.admin.inc and translation_entity.pages.inc to the new form interfaceConvert all confirm_form() in content_translation.admin.inc and content_translation.pages.inc to the new form interface
Status:Needs review» Needs work

+++ b/core/modules/content_translation/lib/Drupal/content_translation/Form/TranslatableForm.phpundefined
@@ -0,0 +1,147 @@
+ * Provides a confirm form for changing translatable status on translation
+ * entities.

This appears to change the translatable status on fields not entities.

Assigned:jibran» Unassigned
Status:Needs work» Needs review
StatusFileSize
new601 bytes
new10.48 KB
PASSED: [[SimpleTest]]: [MySQL] 58,111 pass(es).
[ View ]

Thanks @alexpott for the review. Fixed the comment.

Status:Needs review» Reviewed & tested by the community
Issue tags:-WSCCI-conversion

Looks good to me!

Issue tags:+RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

Status:Fixed» Needs work

+++ b/core/modules/content_translation/lib/Drupal/content_translation/Form/TranslatableForm.php
@@ -0,0 +1,147 @@
+    $action = $this->fieldInfo['translatable'] ? 'disable' : 'enable';
+    return t('Are you sure you want to %action translation for the %name field?',
+      array('%action' => $action, '%name' => $this->fieldName)
+    );

This breaks translatability of 'disable' and 'enable'. Can we have a quick follow-up that goes back to a regular if-else for the entire t()? Thanks.

Status:Needs work» Needs review
Issue tags:-Needs manual testing+D8MI
StatusFileSize
new1.08 KB
PASSED: [[SimpleTest]]: [MySQL] 56,830 pass(es).
[ View ]

+++ b/core/modules/content_translation/lib/Drupal/content_translation/Form/TranslatableForm.phpundefined
@@ -0,0 +1,147 @@
+    $action = $this->fieldInfo['translatable'] ? 'disable' : 'enable';
+    return t('Are you sure you want to %action translation for the %name field?',
+      array('%action' => $action, '%name' => $this->fieldName)

can we:
$action = $this->fieldInfo['translatable'] ? t('disable') : t('enable');
If there is button, or other place those individual words are translated, we might want to use exactly the same as those single words.

Or is it not good for translators to break apart phrases like that?

This just adds back the if else.

--
also removing the needs tag since this is in now.

Status:Needs review» Reviewed & tested by the community

That looks great. It is not good to break strings apart like that because the assumption "those strings can be re-used elsewhere" is valid in every language out there. Also, there might be languages in which the two cases described here require slightly different wording or structure than simply replacing a single word.

Status:Reviewed & tested by the community» Fixed

Well spotted! Committed/pushed to 8.x.

Title:Convert all confirm_form() in content_translation.admin.inc and content_translation.pages.inc to the new form interfaceConvert content_translation_translatable_form() to the new form interface

Retitled to make it clear that content_translation_delete_confirm() is not covered and still available.

Status:Fixed» Needs work

+++ b/core/modules/content_translation/lib/Drupal/content_translation/Form/TranslatableForm.php
@@ -42,10 +42,13 @@ public function getFormID() {
-    $action = $this->fieldInfo['translatable'] ? 'disable' : 'enable';
...
+    if ($field['translatable']) {

Sorry, to open this yet again, and sorry for RTBC-ing this before, but $field isn't defined anywhere. Let's use $this->fieldInfo['translatable'] as before.

Status:Needs work» Needs review
StatusFileSize
new2.44 KB
PASSED: [[SimpleTest]]: [MySQL] 58,175 pass(es).
[ View ]

#38 plus two more bugs fixed here.
This really needs tests, unless it's going away anyway per #17.

Status:Needs review» Needs work
StatusFileSize
new169.28 KB
new141.12 KB
new123.84 KB
new132.87 KB

With a recent git pull,
I made an article, titled One, with a body: One.
enabled content translation module,
added language Spanish at admin/config/regional/language
enabled translation at admin/config/regional/content-language on Content, Article, Body

And there was no confirmation question.

With the change to $this->
I tried again.

I translated One's body to Uno.
then disabled translation at admin/config/regional/content-language on Content, Article, Body
... and still no question or description warning about translations being deleted.
It was not actually disabled,
disabled it again,
and it was disabled.

We know there are problems with batch and enabling/disabling translation.

Hmm.. That was enabling and disabling from the language content settings overview.
enabling-translation-before.png

Ah, reading more above, I see we only expected this confirmation form when doing individual field enabling and disabling from the field settings pages. (See #8)

I enabled Article body translation at admin/structure/types/manage/article/fields/node.article.body/field

8x-enablelink.png

and got a white screen.
going back, I eventually get a notice:

Notice: Trying to get property of non-object in Drupal\Core\Form\ConfirmFormBase->buildForm() (line 51 of core/lib/Drupal/Core/Form/ConfirmFormBase.php).

So I did:

  534  git pull --rebase
  535  git log
  536  git status
  537  git checkout -b question
  538  curl -O https://drupal.org/files/1946462-fix_t-34.patch
  539  git apply --reverse 1946462-fix_t-34.patch
  540  git diff
  541  git add core*
  542  git commit -m "undo 34 fix"
  543  curl -O https://drupal.org/files/1946462-29.patch
  544  git status
  545  git apply --reverse 1946462-29.patch
  546  git diff
  547  git status
  548  git add -u core*
  549  git status
  550  git commit -m "undo orig committed 29"

And the confirmations, with the extra warning on disabling show when enabling and disabling from the body field settings page.

before-29-enabling.pngbefore-29-disabling.png

And, I confirm that from the language content settings page, before this issue, the confirmations did not appear anyway.

Just doing the fix in #38 does not work...

Ah, I see something just posted in #39..
Tried that.
But that does not seem to fix the whitescreen either.
I cleared the caches... I should not need to reinstall.
Hm.

Assigned:Unassigned» Pancho
Category:task» bug
Priority:Normal» Major

This isn't automatically tested and I believe you're first one to manually test the code...
But I'll see into it tonight. Might be that there's still something missing in #39.
It's a regression, so if it weren't to be removed soon, it would be even critical.

Status:Needs work» Needs review

#39: 1946462-second_followup-39.patch queued for re-testing.

Issue tags:+Needs tests
StatusFileSize
new3.08 KB
new3.85 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch content_translation_translatable_form-1946462-43.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

This should have tests to prove the obvious failure. If it's removed from core later, that's fine.

Rerolled for the FormBase changes.

Issue tags:+language-content

This is not the module being removed from core (translation.module is), so it should have the mentioned missing test coverage filled in.

Status:Needs review» Needs work
Issue tags:+Needs tests, +D8MI, +language-content, +FormInterface, +RTBC July 1

The last submitted patch, content_translation_translatable_form-1946462-43.patch, failed testing.

Since the delete confirm form is not covered here, I opened #2086479: Convert content_translation_delete_confirm() to the new form interface, that should be an easy one. :)

Status:Needs work» Needs review
StatusFileSize
new5.55 KB
PASSED: [[SimpleTest]]: [MySQL] 59,515 pass(es).
[ View ]

Added back 'translatable' field to form but as 'value' and using isTranslatable() method

Tested manually via simpletest.me - Disable does not work

StatusFileSize
new11.25 KB
PASSED: [[SimpleTest]]: [MySQL] 59,464 pass(es).
[ View ]
new9.72 KB

The TranslatableForm form itself have check for outdated 'translatable' hidden value that was not migrated right from procedural implementation but submitForm() trying to access this value. This check should be dropped because submit always have fresh values.

The issue needs bigger changes because content_translation_translatable_batch() is broken after #2083811: Remove langcode nesting for fields in entity $form structures and have no tests.
There are 2 bugs:
1) if different entities have same-named fields the function switches translation on all fields, so patch adds $entity_type argument and limits entity types array
2) The code to change values is broken because now there's no way to access $entity->{$field_name}[$langcode] field data this way, so I'm using $entity->getTranslation()->set()

So added @todo and filed #2092573: Refactor content_translation_translatable_batch() for single entity type to polish implementation to remove loop for entity types.

PS: related #2092641: node_help() does not allows to edit node translations would allow to edit translations

StatusFileSize
new11.25 KB
PASSED: [[SimpleTest]]: [MySQL] 58,526 pass(es).
[ View ]
new903 bytes

Translation should be the same

@andypost:

Thanks for working on this but I really don't think we should keep wasting our time on maintaining this broken piece of code with no test coverage. Since the beginning I didn't want this feature to be part of D8 core and it went in only because it seemed it was required to make ET itself go in. With the NG conversion being almost over we should really focus on getting rid of this instead of keeping fixing it.

I'd suggest to move over #2076445: Make sure language codes for original field values always match entity language regardless of field translatability, fix that one, and delete this stuff ASAP.

@plach I think core should provide a sane API to change translatability of the fields or do you think that #2076445: Make sure language codes for original field values always match entity language regardless of field translatability will remove translation from fields?

One of the initial goals of the Entity Field API was changing the way language codes are applied to untranslatable fields so that switching translatability does not imply a data migration. This is what is being discussed in #2076445: Make sure language codes for original field values always match entity language regardless of field translatability among the rest. If we can accomplish that, switching translatability just means changing the 'translatable' value in the field definition.

Status:Needs review» Postponed

As per Prague discussion (https://docs.google.com/document/d/1O4igHjkfMnep96rCPVEE-2F5vG62M3q__gUr...):

CONCLUSION TO IMPLEMENT in https://drupal.org/node/2076445
- Only use ‘und’ if the original entity was ‘und’
- If the original entity is not ‘und’ then inherit from the entity language code (‘de’ entity would get ‘de’ fields even for untranslatable values) NOT ‘und'

Once that is changed, there is no need for the batches that are fixed in this patch. It is better for the user and for the developers too. If you see 'und', you don't need to move up the stack to figure out things. It is also easier to do language fallback in display, etc. Let's not do this once/if #2076445: Make sure language codes for original field values always match entity language regardless of field translatability lands.

Issue summary:View changes
Status:Postponed» Needs work
Parent issue:» #1945406: [meta] Convert all of confirm_form() to ConfirmFormBase

related issue is in

Status:Needs work» Closed (works as designed)

I think we no longer need this.