Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ayelet_Cr’s picture

Assigned: Unassigned » ayelet_Cr
tim.plunkett’s picture

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.

kim.pepper’s picture

Assigned: ayelet_Cr » Unassigned

Unassigning as it's been more than a week.

Crell’s picture

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

Crell’s picture

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

kim.pepper’s picture

Assigned: Unassigned » kim.pepper
kim.pepper’s picture

Status: Active » Needs review
FileSize
9.29 KB

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.
plach’s picture

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

kim.pepper’s picture

Assigned: kim.pepper » Unassigned
kim.pepper’s picture

Issue tags: +Needs manual testing

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

ParisLiakos’s picture

+++ 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() :)

kim.pepper’s picture

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

jibran’s picture

jibran’s picture

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.

jibran’s picture

Status: Needs work » Needs review
FileSize
2 KB
9.64 KB

reroll.

plach’s picture

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.

Crell’s picture

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

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work
jibran’s picture

Status: Needs work » Needs review
FileSize
1.27 KB
9.63 KB

reroll

Crell’s picture

Status: Needs review » Reviewed & tested by the community

OK then.

effulgentsia’s picture

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?

jibran’s picture

FileSize
1.48 KB
10.43 KB

docs added back.

effulgentsia’s picture

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.

alexpott’s picture

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
jibran’s picture

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

Working on a reroll.

jibran’s picture

Component: ajax system » content_translation.module
Status: Needs work » Needs review
FileSize
10.48 KB

Reroll

alexpott’s picture

Title: Convert all confirm_form() in translation_entity.admin.inc and translation_entity.pages.inc to the new form interface » Convert 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.

jibran’s picture

Assigned: jibran » Unassigned
Status: Needs work » Needs review
FileSize
601 bytes
10.48 KB

Thanks @alexpott for the review. Fixed the comment.

kim.pepper’s picture

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

Looks good to me!

YesCT’s picture

Issue tags: +RTBC July 1

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

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

tstoeckler’s picture

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.

YesCT’s picture

Status: Needs work » Needs review
Issue tags: -Needs manual testing +D8MI
FileSize
1.08 KB
+++ 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.

tstoeckler’s picture

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.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Well spotted! Committed/pushed to 8.x.

Pancho’s picture

Title: Convert all confirm_form() in content_translation.admin.inc and content_translation.pages.inc to the new form interface » Convert 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.

tstoeckler’s picture

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.

Pancho’s picture

Status: Needs work » Needs review
FileSize
2.44 KB

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

YesCT’s picture

Status: Needs review » Needs work
FileSize
169.28 KB
141.12 KB
123.84 KB
132.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.png

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

Pancho’s picture

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.

jibran’s picture

Status: Needs work » Needs review

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

tim.plunkett’s picture

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

Rerolled for the FormBase changes.

tim.plunkett’s picture

Gábor Hojtsy’s picture

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.

Gábor Hojtsy’s picture

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.

Gábor Hojtsy’s picture

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

andypost’s picture

andypost’s picture

Status: Needs work » Needs review
FileSize
5.55 KB

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

andypost’s picture

Tested manually via simpletest.me - Disable does not work

andypost’s picture

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

andypost’s picture

Translation should be the same

plach’s picture

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

andypost’s picture

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

plach’s picture

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.

googletorp’s picture

andypost’s picture

Gábor Hojtsy’s picture

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.

andypost’s picture

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

related issue is in

plach’s picture

Status: Needs work » Closed (works as designed)

I think we no longer need this.