Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Comment | File | Size | Author |
---|---|---|---|
#53 | interdiff.txt | 903 bytes | andypost |
#53 | drupal8.content-translation.module.1946462-53.patch | 11.25 KB | andypost |
#43 | content_translation_translatable_form-1946462-43.patch | 3.85 KB | tim.plunkett |
#43 | interdiff.txt | 3.08 KB | tim.plunkett |
#40 | enabling-translation-before.png | 132.87 KB | YesCT |
Comments
Comment #1
ayelet_Cr CreditAttribution: ayelet_Cr commentedComment #2
tim.plunkettYou 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.
Comment #3
kim.pepperUnassigning as it's been more than a week.
Comment #4
Crell CreditAttribution: Crell commentedShe's still working on this one, actually. Just a short distraction.
Comment #5
Crell CreditAttribution: Crell commentedCorrection. She's moved on to another patch that isn't as tricksy. This is available if anyone wants it.
Comment #6
kim.pepperComment #7
kim.pepperFirst go at this.
There is some weirdness with this that I don't quite get.
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 doneComment #8
plach@kim.pepper:
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 think the confirmation form is shown only when switching translatablity of a single field from the Field UI field configuration page.
Comment #9
kim.pepperComment #10
kim.pepperI need someone who knows translation system to test this. Not sure the automated tests are covering it.
Comment #11
ParisLiakos CreditAttribution: ParisLiakos commentednow we can use Field::fieldInfo() :)
Comment #12
kim.pepperConverted to using Field::fieldInfo() is per #11.
Comment #13
jibran#12: 1946462-translation-entity-form-interface-12.patch queued for re-testing.
Comment #14
jibran#12: 1946462-translation-entity-form-interface-12.patch queued for re-testing.
Comment #16
jibranreroll.
Comment #17
plachNot 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.
Comment #18
Crell CreditAttribution: Crell commentedLet's not worry about that now. This eliminates another legacy route, and we need to get through those ASAP. :-)
Comment #19
tim.plunkettNeeds reroll, see #2011018: Reconcile entity forms and confirm forms
Comment #20
jibranreroll
Comment #21
Crell CreditAttribution: Crell commentedOK then.
Comment #22
effulgentsia CreditAttribution: effulgentsia commentedWas this documentation intentionally deleted rather than moved?
Comment #23
jibrandocs added back.
Comment #24
effulgentsia CreditAttribution: effulgentsia commentedThanks. 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.
Comment #25
alexpottSo the module has been renamed in #2024867: Rename translation_entity to content_translation
Comment #26
jibranWorking on a reroll.
Comment #27
jibranReroll
Comment #28
alexpottThis appears to change the translatable status on fields not entities.
Comment #29
jibranThanks @alexpott for the review. Fixed the comment.
Comment #30
kim.pepperLooks good to me!
Comment #31
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #32
catchCommitted/pushed to 8.x, thanks!
Comment #33
tstoecklerThis 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.
Comment #34
YesCT CreditAttribution: YesCT commentedcan 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.
Comment #35
tstoecklerThat 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.
Comment #36
catchWell spotted! Committed/pushed to 8.x.
Comment #37
PanchoRetitled to make it clear that content_translation_delete_confirm() is not covered and still available.
Comment #38
tstoecklerSorry, 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.
Comment #39
Pancho#38 plus two more bugs fixed here.
This really needs tests, unless it's going away anyway per #17.
Comment #40
YesCT CreditAttribution: YesCT commentedWith 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.
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
and got a white screen.
going back, I eventually get a notice:
So I did:
And the confirmations, with the extra warning on disabling show when enabling and disabling from the body field settings page.
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.
Comment #41
PanchoThis 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.
Comment #42
jibran#39: 1946462-second_followup-39.patch queued for re-testing.
Comment #43
tim.plunkettThis should have tests to prove the obvious failure. If it's removed from core later, that's fine.
Rerolled for the FormBase changes.
Comment #44
tim.plunkett#43: content_translation_translatable_form-1946462-43.patch queued for re-testing.
Comment #45
Gábor HojtsyThis is not the module being removed from core (translation.module is), so it should have the mentioned missing test coverage filled in.
Comment #46
Gábor Hojtsy#43: content_translation_translatable_form-1946462-43.patch queued for re-testing.
Comment #48
Gábor HojtsySince 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. :)
Comment #49
andypostClosed as duplicates
#2089853: TranslatableForm totally broken
#2050359: Fix TranslatableForm in content translation
Comment #50
andypostAdded back 'translatable' field to form but as 'value' and using
isTranslatable()
methodComment #51
andypostTested manually via simpletest.me - Disable does not work
Comment #52
andypostThe
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 array2) 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
Comment #53
andypostTranslation should be the same
Comment #54
plach@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.
Comment #55
andypost@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?
Comment #56
plachOne 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.Comment #57
googletorp CreditAttribution: googletorp commented#53: drupal8.content-translation.module.1946462-53.patch queued for re-testing.
Comment #58
andypostMerked as duplicate #2019485: 500 Internal server error, during batch enable of translation from the content language settings page
Comment #59
Gábor HojtsyAs per Prague discussion (https://docs.google.com/document/d/1O4igHjkfMnep96rCPVEE-2F5vG62M3q__gUr...):
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.
Comment #60
andypostrelated issue is in
Comment #61
plachI think we no longer need this.