Please leave this issue alone and post any follow-up in #1836086: [meta] Entity Translation UI improvements :)

Problem/Motivation

In core we have native support for multilingual entity properties and fields. This API has been designed with the main goal of allowing to translate any entity and replace the current Content translation module, which takes care only of nodes, but nothing takes actually advantage of it. This is problematic because in D7 there are two concurring translation models (Node translation in core and Entity Translation in contrib) and both site builders and developers have to familiarize with/support both of them.

Proposed resolution

Implement a generic Entity Translation UI (original proposed screenshots) that can be attached with minimal effort to the add/edit UI of any entity type, and that provides a way to enter different values for each enabled language. This is implemented as a standalone module that provides also a way to configure whether an entity and an entity field should be translatable or not. Every entity gets a translation overview page where all the available translations are listed along with their status (a translation can be up-to-date or not). The regular entity form is used to enter the field values in a particular language and to create new translations. When doing this the user has the possibility to choose which language translate from through a "source language" selector, which populates the entity form widgets with the values in the specified language.

To achieve a consistent UX wrt the usual edit workflow the current (content) language is used to select the values to be loaded into the entity form widgets. This allows for the usual contextual editing: a user viewing a French content and clicking the 'edit' tab will end-up in a form where she can change the French values for that entity. The Entity Translation module exposes content language in the language negotiation settings page and allows to configure it independently from the UI language, this way a user can keep her favorite UI language and edit any content/translation on the site.

All the core entities that currently have a UI are supported: nodes, comments, taxonomy terms and users.

This solution is based on the D7 contrib project Entity Translation. The architecture is pretty simple: we have an EntityTranslationController class that alters the entity form; it can be subclassed to provide the desired UX for a particular entity type. Having a separate controller means this UI stays pluggable and can easily enabled/disabled separately.

Implementation

The ongoing work is hosted in the following sandbox: http://drupal.org/sandbox/plach/1719670.

Credits

  • aspilicious: code review
  • bforchhammer: code review
  • Berdir: code review
  • Bojhan: UX review
  • cosmicdreams: manual testing
  • fago: architectural review, code review
  • Gábor Hojtsy: coordination, architectural review
  • peximo: development
  • plach: design and development
  • podarok: manual testing
  • YesCT: coordination, UX review, manual testing, screenshots (!), help text, PHP docs
  • webchick: UX review

Commit message:

Issue #1188388 by plach, peximo, YesCT | Gábor Hojtsy, fago, webchick, Bojhan, podarok, cosmicdreams, Berdir, aspilicious, bforchhammer, penyaskito: Added Entity translation UI in core.

Remaining tasks in this issue

  • This needs a re-roll since EntityFieldQuery v2 is in #1801726: EntityFieldQuery v2
  • commit blocker: @webchick wants to do a review of the help to verify the instructions there are accurate
  • other commit blockers: done
  • reviews:
    • ui: Done. Bojhan says the UI is fine to get the basics in. (Discussion of possible future improvements is ongoing but does not need to block this.)
    • ui phone call: Done. Gabor, Bojhan, webchick, plach identified commit blockers (and some follow-up tasks.
    • code: Done.
  • screenshots: Done.
  • tests: Done. Super great tests that are extendable.
  • documentation: PHP docs and a help section are in place. No d.o. documentation has been written yet. We will probably need a page describing how to setup everything. (See #120)

User interface changes

  • test entities #200
  • nodes #127
  • comments #130
  • vocabulary/terms #132
  • users #133
  • some updated shots with wording changes #184
  • lang select hidden and global settings expanded in #195
  • (all languages) on shared fields #199

API changes

Almost none. Just some minor clean-up of the Entity Traslation API.

Follow-up issues

See #19, #81, #82 and #181 for details.

Other related issues belonging to the original roadmap:

Original report by Gábor Hojtsy

The module matures in the http://drupal.org/project/entity_translation project, and it needs to be cleaned up and integrated to Drupal 8 for the system to be a viable multilingual contender. The original issue for Drupal 7 was at #539110: TF #4: Translatable fields UI with 110 responses, it was deemed inappropriate to reopen for Drupal 8. Therefore this issue.

CommentFileSizeAuthor
#267 03.11.12 20:41-Schnappschuss.jpeg825.25 KBSchnitzel
#267 03.11.12 20:40-Schnappschuss.jpeg973.84 KBSchnitzel
#267 03.11.12 20:40-Schnappschuss.jpeg1004.78 KBSchnitzel
#258 et-ui-1188388-258.patch137.55 KBplach
#258 et-ui-1188388-258.nofix_.patch137.55 KBplach
#258 et-ui-1188388-258.tests_.interdiff.do_not_test.patch28.87 KBplach
#258 et-ui-1188388-258.interdiff.do_not_test.patch39.74 KBplach
#251 entity-translation-ui-1188388.243-251.txt1.69 KBpenyaskito
#251 entity-translation-ui-1188388-251.patch123.29 KBpenyaskito
#248 et-ui-243-newuser-transtab-2012-11-02_0728.png82.3 KBYesCT
#248 et-ui-243-s02-2012-11-02_0736.png20.4 KBYesCT
#248 et-ui-243-s03-justTransFieldOnForm-2012-11-02_0747.png55.64 KBYesCT
#243 et-ui-1188388-243.patch123.29 KBplach
#242 et-ui-1188388-242.interdiff.do_not_test.patch1.26 KBplach
#242 et-ui-1188388-242.patch0 bytesplach
#241 et-ui-1188388-241.patch123.49 KBYesCT
#241 interdiff-234-241.txt12.7 KBYesCT
#241 interdiff-225-241.txt21.39 KBYesCT
#234 et-ui-1188388-234.patch121.07 KBYesCT
#234 interdiff-230-234.txt6.9 KBYesCT
#232 et-ui-1188388-232.patch120.89 KBYesCT
#232 interdiff-225-232.txt4.75 KBYesCT
#226 et-ui-1188388-222.patch120.92 KBYesCT
#226 interdiff-215-222.txt6.94 KBYesCT
#225 et-ui-1188388-225.interdiff.do_not_test.patch1.46 KBplach
#225 et-ui-1188388-225.patch120.91 KBplach
#224 et-ui-1188388-224.patch120.88 KBplach
#224 et-ui-1188388-224.interdiff.do_not_test.patch6.9 KBplach
#215 et-ui-1188388-215.interdiff.do_not_test.patch18.86 KBplach
#215 et-ui-1188388-215.patch120.6 KBplach
#211 et-ui-1188388-211.interdiff.do_not_test.patch6.11 KBplach
#211 et-ui-1188388-211.patch120.63 KBplach
#200 et-ui-197-s02-testEntityEdit-2012-10-29_2324.png98.46 KBYesCT
#200 et-ui-197-s03-testEntityTranslationTab-2012-10-29_2334.png61.43 KBYesCT
#200 et-ui-197-s04-editHasLangSelect-2012-10-29_2342.png35.34 KBYesCT
#200 et-ui-197-s05-testEntitySelectHidden-2012-10-29_2349.png41 KBYesCT
#199 et-ui-197-s01-allLanguages-2012-10-29_2239.png138.37 KBYesCT
#197 et-ui-1188388-197.interdiff.do_not_test.patch773 bytesplach
#197 et-ui-1188388-197.patch115.81 KBplach
#195 et-ui-191-s03-addtrans-langselecthidden-2012-10-29_0952.png164.11 KBYesCT
#195 et-ui-191-s01a-globalexpanded-2012-10-29_0945.png105.56 KBYesCT
#195 et-ui-191-s01b-globalexpanded-2012-10-29_0946.png94.01 KBYesCT
#195 et-ui-191-s02-globalcolapsedwhenfieldtransenabled-2012-10-29_0949.png77.47 KBYesCT
#191 et-ui-1188388-191.interdiff.do_not_test.patch16.22 KBplach
#191 et-ui-1188388-191.patch115.8 KBplach
#190 et-ui-1188388-190-without_flag_outdated_188.patch111.78 KBYesCT
#190 interdiff-184-190.txt1.84 KBYesCT
#188 et-ui-1188388-188.patch112.01 KBYesCT
#188 interdiff-184-188.txt6.21 KBYesCT
#186 et-ui-1188388-186.patch112.01 KBYesCT
#186 interdiff-184-186.txt6.24 KBYesCT
#186 et-ui-186-s01-allow_individual_translations_outdated-2012-10-27_0538.png84.67 KBYesCT
#184 et-ui-1188388-184.patch111.71 KBYesCT
#184 interdiff-176-184.txt4.14 KBYesCT
#184 et-ui-184-s01-lingering_all_translations-2012-10-27_0317.png70.25 KBYesCT
#184 et-ui-184-s02-link_to_fields_directly-2012-10-27_0324.png86.39 KBYesCT
#184 et-ui-184-s03-remove_batch_process_words-2012-10-27_0331.png78.17 KBYesCT
#184 et-ui-184-s04-no_data_successfully_processed-2012-10-27_0334.png82.03 KBYesCT
#184 et-ui-184-s05-shared_to_existing_datat-2012-10-27_0327.png69.66 KBYesCT
#184 et-ui-184-s06-no_because_source-2012-10-27_0339.png78.4 KBYesCT
#176 et-ui-1188388-176.patch111.51 KBYesCT
#176 interdiff-173-176.txt6.83 KBYesCT
#173 et-ui-1188388-173.interdiff.do_not_test.patch8.38 KBplach
#173 et-ui-1188388-173.patch111.04 KBplach
#171 et-ui-1188388-171.interdiff.do_not_test.patch7.13 KBplach
#171 et-ui-1188388-171.patch110.03 KBplach
#170 et-ui-1188388-170.patch110.92 KBplach
#162 et-ui-1188388-162.patch.interdiff.do_not_test.patch10.75 KBplach
#162 et-ui-1188388-162.patch.patch115.86 KBplach
#158 et-ui-1188388-158.interdiff.do_not_test.patch12.89 KBplach
#158 et-ui-1188388-158.patch110.85 KBplach
#154 et-ui-1188388-154.interdiff.do_not_test.patch8.27 KBplach
#154 et-ui-1188388-154.patch108.32 KBplach
#151 et-ui-1188388-151.interdiff.do_not_test.patch16.83 KBplach
#151 et-ui-1188388-151.patch103.99 KBplach
#133 et-ui-p120-u-s01-enableTranslationOnUsers-2012-10-16_0504.png108.73 KBYesCT
#133 et-ui-p120-u-s02-translationTabOnAUser-2012-10-16_0505.png87.79 KBYesCT
#132 et-ui-p120-t-s01-enableTranslationOnAVocabulary-2012-10-16_0451.png64.33 KBYesCT
#132 et-ui-p120-t-s02-addingATag-2012-10-16_0454.png96.41 KBYesCT
#132 et-ui-p120-t-s03-termNoLangSpecified-2012-10-16_0458.png108.04 KBYesCT
#132 et-ui-p120-t-s04-languageSpecifiedGetsTranslationTab-2012-10-16_0500.png140.5 KBYesCT
#132 et-ui-p120-t-s05-translationsTab-2012-10-16_0501.png85.49 KBYesCT
#130 et-ui-p120-c-s01-stuffToCommentOn-2012-10-16_0402.png75.92 KBYesCT
#130 et-ui-p120-c-s02-addingComment-2012-10-16_0409.png74.49 KBYesCT
#130 et-ui-p120-c-s03-aComment-2012-10-16_0408.png78.34 KBYesCT
#130 et-ui-p120-c-s04-enableCommentTranslationOnArticle-2012-10-16_0412.png118.01 KBYesCT
#130 et-ui-p120-c-s05-commentTranslationsLink-2012-10-16_0413.png63.35 KBYesCT
#130 et-ui-p120-c-s06-translationsTabOfAComment-2012-10-16_0415.png88.35 KBYesCT
#130 et-ui-p120-c-s07-enableTranslationsOnCommentBodyField-2012-10-16_0417.png103.65 KBYesCT
#130 et-ui-p120-c-s08-commentTranslationsTabWithAdd-2012-10-16_0423.png73.92 KBYesCT
#130 et-ui-p120-c-s09-addingCommentTranslation-2012-10-16_0425.png101.64 KBYesCT
#130 et-ui-p120-c-s10-addCommentTrans-TransSettingToOutdate-2012-10-16_0427.png101.05 KBYesCT
#130 et-ui-p120-c-s11-viewJustAdded-2012-10-16_0428.png66.52 KBYesCT
#130 et-ui-p120-c-s12-addCommentInOtherLang-2012-10-16_0430.png70.31 KBYesCT
#130 et-ui-p120-c-s13-commentAddedInLangOfContentDetected-2012-10-16_0431.png100.33 KBYesCT
#130 et-ui-p120-c-s14-commentsHaveNoLanguageField-2012-10-16_0433.png77.49 KBYesCT
#130 et-ui-p120-c-s15-addingTranslation-flagOutdated-2012-10-16_0435.png105.43 KBYesCT
#130 et-ui-p120-c-s16-translationsWithOutdatedFlag-2012-10-16_0437.png101.79 KBYesCT
#127 et-ui-p120-s01-addLanguages-2012-10-15_2331.png103.38 KBYesCT
#127 et-ui-p120-s02-contenttypeEnableTrans-2012-10-15_2342.png108.23 KBYesCT
#127 et-ui-p120-s03-unhideLanguageSelectorOnArticle-2012-10-16_0000.png97.7 KBYesCT
#127 et-ui-p120-s04-createContent-2012-10-16_0006.png104.27 KBYesCT
#127 et-ui-p120-s05-transTab-2012-10-16_0009.png79.01 KBYesCT
#127 et-ui-p120-s06-transTab-2012-10-16_0011.png88.62 KBYesCT
#127 et-ui-p120-s07-moveLangFieldSetting-2012-10-16_0018.png125.15 KBYesCT
#127 et-ui-p120-s08-enableTransOnBody-2012-10-16_0022.png109.69 KBYesCT
#127 et-ui-p120-s09-confirmBatch-2012-10-16_0024.png80.9 KBYesCT
#127 et-ui-p120-s10-bodyEnabled-2012-10-16_0025.png105.23 KBYesCT
#127 et-ui-p120-s10-bodyEnabled-2012-10-16_0025.png105.23 KBYesCT
#127 et-ui-p120-s11-addTrans-2012-10-16_0028.png118.1 KBYesCT
#127 et-ui-p120-s12-addingTrans-transSettingOutdateOthers-2012-10-16_0039.png167.58 KBYesCT
#127 et-ui-p120-s13-afterSaveSeeTrans-2012-10-16_0051.png61.02 KBYesCT
#127 et-ui-p120-s14-addTrans-multTrans-2012-10-16_0055.png145.22 KBYesCT
#127 et-ui-p120-s15-canSelectSource-2012-10-16_0106.png81.19 KBYesCT
#127 et-ui-p120-s17-changedSource-2012-10-16_0112.png87.11 KBYesCT
#127 et-ui-p120-s16-changeSource-2012-10-16_0110.png69.93 KBYesCT
#127 et-ui-p120-s18-useFlagOutdatedSetting-2012-10-16_0116.png141.77 KBYesCT
#127 et-ui-p120-s19-showOutdated-editTrans-2012-10-16_0125.png108.45 KBYesCT
#127 et-ui-p120-s20-editTrans-2012-10-16_0139.png123.86 KBYesCT
#127 et-ui-p120-s21-niceEditTrans-2012-10-16_0134.png121.99 KBYesCT
#127 et-ui-p120-s22-uncheckWhenUpdated-2012-10-16_0144.png110.76 KBYesCT
#127 et-ui-p120-s23-updatedTrans-nodeTitleViewLink-2012-10-16_0150.png108 KBYesCT
#127 et-ui-p120-s24-editingTheNode-2012-10-16_0154.png54.41 KBYesCT
#127 et-ui-p120-s25-editingTheNodeChangeLanguage-2012-10-16_0155.png80.81 KBYesCT
#127 et-ui-p120-s26-weirdnessChangeLangOfNode-2012-10-16_0201.png90.11 KBYesCT
#127 et-ui-p120-s27-diffLangOfNode-2012-10-16_0208.png94.56 KBYesCT
#127 et-ui-p120-s28-editToDeleteTrans-2012-10-16_0210.png70.35 KBYesCT
#127 et-ui-p120-s29-confirmationDeleteTrans-2012-10-16_0212.png79.33 KBYesCT
#127 et-ui-p120-s30-deleteNodeShowsWhenEditInLangOfNode-2012-10-16_0217.png119.91 KBYesCT
#127 et-ui-p120-s31-deleteNodeConfirmsDeleteAllTranslations-2012-10-16_0220.png71.91 KBYesCT
#120 et-ui-1188388-120.interdiff.do_not_test.patch10.88 KBplach
#120 et-ui-1188388-120.patch101.48 KBplach
#115 et-ui-1188388-115.interdiff.do_not_test.patch13.42 KBplach
#115 et-ui-1188388-115.patch100.32 KBplach
#110 et-ui-78-s19-translationsettingsinmanagefields-2012-10-09_1450.png114.53 KBYesCT
#110 et-ui-78-s20-languagesettinglocationonedit-2012-10-09_1452.png119.88 KBYesCT
#110 et-ui-78-s21-changethelocation-2012-10-09_1456.png119.51 KBYesCT
#110 et-ui-78-s22-languageselectionmoved-2012-10-09_1458.png119.18 KBYesCT
#110 et-ui-78-s23-coulddisplaylanguagefieldonnode-2012-10-09_1503.png121.02 KBYesCT
#110 et-ui-78-s24-cannotshowlanguageoncomment-2012-10-09_1505.png102.39 KBYesCT
#110 et-ui-78-s25-addingcomment-sitedeflang-2012-10-09_1511.png109.11 KBYesCT
#110 et-ui-78-s26-commenttranslinkadded-2012-10-09_1514.png67.36 KBYesCT
#110 et-ui-78-s27-commenttranssettingstrangelocation-2012-10-09_1519.png126.56 KBYesCT
#110 et-ui-78-s28-translationsettingexpandedoncommentstrangeindent-2012-10-09_1524.png109.36 KBYesCT
#110 et-ui-78-s29-showstranscommentandbodyofnontrans-2012-10-09_1527.png116.94 KBYesCT
#110 et-ui-78-s30-dontwanttransabovecomment-2012-10-09_1532.png110.09 KBYesCT
#101 et-config-1188388-101-1.png36.29 KBplach
#101 et-config-1188388-101-2.png57.13 KBplach
#99 et-ui-78-s18-contentlist-2012-10-09_0519.png107.47 KBYesCT
#96 et-ui-78-s15-prep-origlang-2012-10-09_0417.png107.99 KBYesCT
#96 et-ui-78-s16-allowsSwitchToEnglish-2012-10-09_0420.png95.23 KBYesCT
#96 et-ui-78-s17-origLanguageNotOrig-2012-10-09_0423.png115.91 KBYesCT
#92 et-ui-78-s13-clickable-2012-10-09_0347.png109.59 KBYesCT
#92 et-ui-78-s14-notclickable-2012-10-09_0349.png118.66 KBYesCT
#85 et-ui-78-s03b-niceother-2012-10-09_0038.png110.72 KBYesCT
#85 et-ui-78-s04-niceeditwithoutbrackets-2012-10-09_0046.png123.47 KBYesCT
#85 et-ui-78-s05-nobracketsonoriglanguage-2012-10-09_0048.png88.1 KBYesCT
#85 et-ui-78-s06-transtabaddtrans-2012-10-09_0110.png113.92 KBYesCT
#85 et-ui-78-s07-nicehintcreatetrans-2012-10-09_0117.png89.59 KBYesCT
#85 et-ui-78-s08-transtabedittranslink-2012-10-09_0126.png110.44 KBYesCT
#85 et-ui-78-s09-edittransnicebrackets-2012-10-09_0131.png105.05 KBYesCT
#85 et-ui-78-s10-addtranswhenmultexist-2012-10-09_0142.png98.2 KBYesCT
#85 et-ui-78-s11-languagedetection-2012-10-09_0157.png170.2 KBYesCT
#87 et-ui-78-s12-suggesturllang2012-10-09_0254.png139.61 KBYesCT
#78 et-ui-1188388-78.interdiff.do_not_test.patch709 bytesplach
#78 et-ui-1188388-78.patch100.13 KBplach
#76 et-ui-1188388-76.interdiff.do_not_test.patch581 bytesplach
#76 et-ui-1188388-76.patch100.16 KBplach
#72 et-ui-1188388-72.interdiff.do_not_test.patch16.62 KBplach
#72 et-ui-1188388-72.patch100.1 KBplach
#71 et-ui-1188388-71.interdiff.do_not_test.patch6.41 KBplach
#71 et-ui-1188388-71.patch97.51 KBplach
#69 et-ui-1188388-69.interdiff.do_not_test.patch21.28 KBplach
#69 et-ui-1188388-69.patch97.35 KBplach
#54 et-ui-s001-what is are the brackets in the title area of the node edit 2012-10-03_1204.png72.73 KBYesCT
#54 et-ui-s002-visualcluetotranslatebits-2012-10-03_1221.png91.01 KBYesCT
#54 et-ui-s003-edit language detection 2012-10-03_1237.png91.01 KBYesCT
#54 et-ui-s004-translations tab strangeness 2012-10-03_1242.png84.53 KBYesCT
#54 et-ui-s005-can revisions tab be translation aware 2012-10-03_1248.png113.9 KBYesCT
#54 et-ui-s006-editing the node not translation 2012-10-03_1312.png97 KBYesCT
#54 et-ui-s007-source language 2012-10-03_1321.png66.56 KBYesCT
#54 et-ui-s008-flag _other_ translations as outdated 2012-10-03_1325.png94.77 KBYesCT
#54 et-ui-s009-not the source just was marked outdated 2012-10-03_1333.png100.71 KBYesCT
#54 et-ui-s010-cannot arrange settings like admin and translation order 2012-10-03_1354.png94.33 KBYesCT
#54 et-ui-s011-comment edit order of translation settings 2012-10-03_1355.png69.73 KBYesCT
#51 s11-noticeGetProperty-2012-10-03_1558.png62.83 KBYesCT
#52 s12-addlink-2012-10-03_1612.png65.81 KBYesCT
#52 s13-wronglang-2012-10-03_1614.png43.01 KBYesCT
#52 s14-addenglishlink-2012-10-03_1615.png63.44 KBYesCT
#52 s15-english-2012-10-03_1616.png38.63 KBYesCT
#49 s01-enableTranslationOnArticle-2012-10-03_1509.png95.39 KBYesCT
#49 s02-addsomecontent-2012-10-03_1517.png46.95 KBYesCT
#49 s03-HomeFrontPageShowsContent-2012-10-03_1518.png50.62 KBYesCT
#49 s04-TranslationsTabOnNode-2012-10-03_1519.png50.88 KBYesCT
#49 s06-AddHintInCollapsedGlobalFieldSet-2012-10-03_1523.png85.88 KBYesCT
#49 s05-EnableTranslationOnBodyField-2012-10-03_1521.png84.3 KBYesCT
#49 s07-GlobalSettingsTranslationEnabled-2012-10-03_1534.png77.21 KBYesCT
#49 s08-CanAddTranslation-2012-10-03_1545.png49.54 KBYesCT
#49 s09-accessdenied-2012-10-03_1549.png34.93 KBYesCT
#49 s10-PermissionToTranslateEntitiesOfTypeNode-2012-10-03_1551.png100.25 KBYesCT
#35 et-ui-1188388-35.patch100.07 KBplach
#28 duplicated_translation_option.png71.86 KBpodarok
#28 taxonomy_error.png53.43 KBpodarok
#26 et-ui-1188388-26.interdiff.do_not_test.patch636 bytesplach
#26 et-ui-1188388-26.patch100.77 KBplach
#23 et-ui-1188388-23.interdiff.do_not_test.patch1.63 KBplach
#23 et-ui-1188388-23.patch100.85 KBplach
#22 et-ui.png485.88 KBsun
#19 et-ui-1188388-19.zip443.24 KBplach
#19 et-ui-1188388-19.patch100.01 KBplach
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach’s picture

I'm considering to introduce in ET a dependency on CTools to better handle some use cases, which reveal that we are not able to handle entities in a fully generic way:

#1155134: Integrate pathauto bulk generation

At least this is an option I took into consideration. I'm wondering it is a wise choice given that in D8 we'll probably have a plugin system but it might heavily differ from CTools.

plach’s picture

Issue tags: +translatable fields

tagging

renat’s picture

Subscribe.

zilverdistel’s picture

subscribe

sun’s picture

Issue tags: +Entity system
Gábor Hojtsy’s picture

Issue tags: +language-content

Tagging for the content handling leg of D8MI.

Gábor Hojtsy’s picture

Title: Move translatable fields UI into core » META: Move translatable fields UI into core
Issue tags: +sprint

Tagging for sprint tomorrow to get started with and adding META to title, since we want to break this up most likely.

Gábor Hojtsy’s picture

Issue summary: View changes

Fixed the link to entity_translation module

plach’s picture

Title: META: Move translatable fields UI into core » META: Move the entity translation UI into core

Updated the issue summary.

plach’s picture

Issue summary: View changes

Added issue summary

Gábor Hojtsy’s picture

The updated issue summary does not have any reference to having an issue to introduce the module to be able to control whether an entity and its fields are translatable. I think going with that would be a great way to stir up some enthusiasms. We can try to crawl away with the lower level tasks but that did not work out so far. There is little interest if you cannot show how it fits into the bigger picture.

Gábor Hojtsy’s picture

Issue tags: -sprint

The current focus is #1498634: [meta] Define revision/translation SQL schema for core entities (no patch) as part of this larger effort, cleaning up the sprint board.

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue summary: View changes

Updated issue summary.

Gábor Hojtsy’s picture

@plach: can you post an issue / patch with the form solutions from the sandbox so people can review them? As it is currently, it just stands there and its precious time we could get people to look at it.

87 days to go before Drupal 8 feature freeze.

plach’s picture

As noted in the summary, the UI is being worked on in the following sandbox: http://drupal.org/sandbox/plach/1719670.

This is the main branch: http://drupalcode.org/sandbox/plach/1719670.git/tree/refs/heads/8.x-et-u...

webchick’s picture

Issue tags: +Usability, +Needs screenshots

Tagging. :) EXCITING that this is being worked on!!

plach’s picture

Just to clarify:

  • 8.x-et-ui-1188388-plach is the branch I'm working on currently: here I'm porting the basic ET functionality and cleaning it up in the process to match the new D8 architecture
  • 8.x-et is the global branch where I'm merging all the active issues.

They should be identical at the moment.

I'll provide some updated screenshots ASAP.

plach’s picture

Just an update for early reviewers: the topic branch now holds a functional UI for all the core entity types. I'm providing test coverage for it ASAP mainly to be able to test multilingual properties, which may reveal some architectural flaw. The first patch and the related screenshots will immediately follow.

We should try to get #1757232: Improve test coverage for the entity form controller in meanwhile.

Gábor Hojtsy’s picture

Woot! Looking forward to reviewing the progress :) It would make sense to update the summary as well, eg. no Storage API to build on.

plach’s picture

Just completed the basic tests for the UI and... with some adjustment multiligual properties work! It's all in the sandbox. Too late now to provide patch and related screenshots as I want to point out some outstanding issues. Hopefully tomorrow :)

plach’s picture

Title: META: Move the entity translation UI into core » Move the entity translation UI into core
FileSize
100.01 KB
443.24 KB

Ok, finally I've managed to put together some screenshots, you can find them attached (I did not embed them, since they would make the post ridiculously long). They sum up the implemented features for node translation, and then briefly show that they are available also for the other core entity types including the test entity. Long story short: we have configurable content language detection, entity translatability setting, field translatibility setting, multilingual entity forms, translation overview, source language selection and basic workflow handling through the "outdated" flag.

Please note that the focus of this issue is getting the raw UI in core so that we can start testing all the plumbing stuff we've been working on since now. UX reviews are welcome, but probably detailed discussion and implementations should go into follow-up issues, unless they involve big refactorings of the current architecture (which I hope not :).

The architecture is pretty simple: we have an EntityTranslationController class that alters the entity form; it can be subclassed to provide the desired UX for a particular entity type. Some implementation details:

  • I had to perform some changes to the entity test storage controller since it was not dealing correctly with deleted translations.
  • I generalized field language change handling on submit and moved it from the NodeFormController to the base one, to support the language switcher being available to any entity type.
  • As discussed with @fago in Munich Entity::translation should return also the original language, since in most situations we want to iterate on all the available languages, not only on the translations.

As stated above I'd like to point out that there are still many outstanding issues, however the basic functionality is implemented and working and I think most of the follow-ups would not be bound to the feature freeze. More specifically:

  • The are some methods of EntityTranslationControllerInterface that I think would be best located elsewhere:
    • The getAccess() method should be removed as soon as we have entity access available and replaced with EnttyInterface::access() calls or something along those lines.
    • The removeTranslation() should probably go into the EntityInterface as well, since all the data-structure handling methods concerning entity translation are there, the translation controller is focused only on the UI.
    • The get*Path() methods, which provide the paths to be used when building the UI for a particular entity type, should belong to a UI controller or whatever comes up from #1783964: Allow entity types to provide menu items and #1781372: Add an API for listing (configuration) entities where the same issues we have here are being addressed. AAMOF those methods are not specific to translation and atm we need to inject menu router info in the entity info to be able to generate the translation UI in a generic fashion.
  • We need to find clean way to deal with translation metadata and the bare existence of translations: currently to determine whether a translation exists we just scan (each time) all the fields and properties to collect any language we can find. That collection is returned as the existing translations. This approach is IMHO inefficient and fragile, as all the changes I had to do the EntityTest and the related storage controller demonstrate. @fago and I discussed about a related issue in Munich: how to deal with language when accessing properties trough the upcoming Property API. We agreed that a variant of the previously suggested active language would work: basically we would have an EntityInterface::getTranslation($langcode) method that would return a class implementing the EntityInterface having as default the passed language. This would be used to access the properties in the usual way. We agreed that this could be a viable solution since it does not introduce the concept of state to the entity itself, we just retrieve a facet to act on (note that the class would not be a full entity class, just a wrapper or something like that). Building upon this I think we could have a full-fledged EntityTranslation class with its own storage controller and its properties, which would hold metadata like the status or the source language for that particular translation and an entity reference to the parent entity. As an alternative we could go the current way and declare metadata as multilingual properties of the parent entity, but the former is a solution worth to be explored as we would get the CRUD hooks or per-language access for free, just to make a couple of examples. If there are perfomance concerns for monolingual sites we could have a null implementation of the storage controller that performs no query in this case.
  • No matter what we decide for the previous point, we should consider whether we want an independent status for each translation, even on entities like terms that have no status concept at all. I think we should because supporting any (custom?) field/property used as status in a custom workflow implemented through Rules or a specific module like Workbench or TMGMT could add lots of complexity. However also the translation status involves some complexity as you may end up with a published node with no published translation, for instance.
  • Currently the implementation of the entity translatability setting is a bit hacky, but I think we can clean it up as soon as #1739928: Generalize language configuration on content types to apply to terms and other entities lands.
  • As soon as the new routing system supports multiple access callbacks we should leverage them to add per-language access checking to the entity form and exploit the 'edit original values' permission. The latter has been introduced and will be paired with a 'edit shared values' one to allow to mimick the old translation form we had in the early incarnations of the contrib Entity Translation. With none of them a translator can only access a form that has only translatable fields/properties and can act only on translations.
  • I'm not totally happy with the translation addition as implemented right now: currenlty we have a dedicated page callback which provides an entity form having as form language the target one. What I'm unhappy about is that this way we are skipping all the access checks we would get if accessing the original page callback providing the entity form plus any alteration of the data returned by entity_get_form() performed in its context.

Here are features not yet ported as I need some feedback:

  • Field column synchronization: this is used to translate stuff like the core image field which provides Alt and Title columns, which should hold translated values, and the regular Fid column that may need to be shared accross translations. If the media initiative succeeds in making them regular fields attached to File entity we would solve this use case, not sure if it would deserve some love anyway.
  • If we introduce translation status we need to provide fallback when viewing an unpublished translation.
  • We need a more granular way to define whether fallback is enabled or not, mainly make it a per-bundle settings as translatability.
  • We need to introduce (optional) per-language filtering on comment lists, so that only the comments matching the current content language are displayed. We also need language-aware node comment statistics for this.
  • We need to figure out how to deal with Path when deleting a translation, should we keep the current approach and supporting it explictly? I don't think so as this won't scale, instead we need to provide it a translation deletion hook to react to. Entity translation classes would help here.
  • Same story for menu translation, but here would probably also need some work in the NodeTranslationController to adjust the node form to support multilingual menus.
  • Lastly we may want to retain ET's ability to show authoring information and translation information together on nodes. Not sure about this.

I think most of these should be deferred to followups to make the patch reviewable.

plach’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, et-ui-1188388-19.patch, failed testing.

sun’s picture

FileSize
485.88 KB

Let me introduce to you... ImageMagick

> convert *.png -append et-ui.png

;)

plach’s picture

Status: Needs work » Needs review
FileSize
100.85 KB
1.63 KB

This should fix the failing tests.

plach’s picture

Component: language system » entity system
Issue tags: -Entity system, -Needs screenshots

I think this should be part of the entity system queue.

Status: Needs review » Needs work

The last submitted patch, et-ui-1188388-23.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
100.77 KB
636 bytes

Let's try this one.

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Title: Move the entity translation UI into core » Entity translation UI in core
Priority: Normal » Major
podarok’s picture

Status: Needs review » Needs work
FileSize
53.43 KB
71.86 KB

#26 just installed locally for review
one bug with this patch is duplicated Translation option in content type editing form

duplicated_translation_option.png

podarok’s picture

plach’s picture

Status: Needs work » Needs review

@podarok:

You probably enabled also the legacy Content translation module: they are not designed to live together anymore and if we succed here it will be removed from core. No point in testing them together, I think.

podarok’s picture

Status: Needs review » Reviewed & tested by the community

#30 yup, it is
So without legacy Content translation module #26 looks like RTBC for me

plach’s picture

Thanks a lot for actually testing the patch but I think we need at least some feedback on #19 before actually committing this :)

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +sprint

Tagging for D8MI sprint.

plach’s picture

Assigned: Unassigned » plach
plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

FileSize
100.07 KB

Rerolled after the latest commits.

Status: Needs review » Needs work
Issue tags: -Usability, -translatable fields, -D8MI, -sprint, -language-content

The last submitted patch, et-ui-1188388-35.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
Issue tags: +Usability, +translatable fields, +D8MI, +sprint, +language-content

No failures here on my box.

#35: et-ui-1188388-35.patch queued for re-testing.

fago’s picture

just discussed this and its architecture with plach via skype. In short, I think the current architecture is fine. Anyway, here is a more detailled summary:

  • We've a separate translation controller, so this UI stays pluggable and can easily enabled/disabled separately.
  • As noted in #19 there are some methods in the translation controller that - in the end - belong elsewhere, i.e. checking access (#1696660: Add an entity access API for single entity access) and getting edit/deletion paths (list controller?).
  • What brings us to the relationship to the list controller, see #1781372: Add an API for listing (configuration) entities. Right now, it's not there so we prefer to not introduce dependencies now. Once its there we should leverage it to either extend from it, or if not suiting to leverage a list controller for getting edit/deletion paths and inject the list controller to be used (dependency injection).
  • Regarding to the form, we figured alteration is the best way to go now. Later on we might want to look into embedding the entity form into a translation form instead, but for that to work out more work on entity forms would have to happen first to support this properly. So this is a possible follow-up.

Then, we agreed to move on with the new entity property api first (#1696640: Implement API to unify entity properties and fields) (given it's moving fast) as this improves the property translation API as well and converts the test-entity, then build the translation UI on that. So we can leverage the new property translation API and avoid redoing stuff afterwards.

Gábor Hojtsy’s picture

Postponed on #1696640: Implement API to unify entity properties and fields or there are good things to work on inbetween? Sounds like there might be.

Gábor Hojtsy’s picture

Priority: Major » Critical

Highten priority as it is required to remove the existing translation module, etc.

YesCT’s picture

I was checking in on this to redo a ui review I started. I'll hold off based on #38.

plach’s picture

@YesCT:

Thanks! If you are testing the UI, please, feel free to go on: nothing mentioned in #28 will affect UX.

YesCT’s picture

OK. Will do.

mtomaizy’s picture

#35: et-ui-1188388-35.patch queued for re-testing.

plach’s picture

Working on a reroll after the Property UI patch landed. I just posted a fix for #1534674: Comment field language is completely broken that is somehow related to the work here.

Gábor Hojtsy’s picture

Please summarise outstanding questions then with the new patch so we can quickly look at those and figure out solutions if needed.

plach’s picture

Ok, I'll update the issue summary. However most of #19 still stands.

YesCT’s picture

I went through the ui and I have a bunch of comments. I'm writing them up to make them understandable by others.

YesCT’s picture

Few preliminary notes: I'm going to use numbers and headings so we can refer back later to parts of this comment. I'm expecting some things will be just observations, suggestions, questions, as designed, follow up issues or might end up marked not related at all to this issue.

  1. In general

    I tested this with an older version of d8 that the patch in comment #35 applied to.
    In general, this is pretty slick and it works.

  2. Steps to test

    steps to test (later steps are integrated in with the screen shots and comments

    >drush -y si --account-name=admin --account-pass=admin --site-name=d8-et-ui-1188388-35 --db-url="mysql://root:root@localhost/d8-git"
    at /admin/modules enable language and entity translation modules
    at /admin/config/regional/language add some languages. I added spanish and german.
    at /admin/structure/types/manage/article enable translation in the Language settings vertical tab http://drupal.org/files/s01-enableTranslationOnArticle-2012-10-03_1509.png
    add some content http://drupal.org/files/s02-addsomecontent-2012-10-03_1517.png
    content shows on home/front page http://drupal.org/files/s03-HomeFrontPageShowsContent-2012-10-03_1518.png
    look at translation tab on content
    s04-TranslationsTabOnNode-2012-10-03_1519.png
    cannot translate because no translatable fields
    edit the content type and go to manage fields to edit the body to enable translation

  3. Suggest adding hint near the global collapsed field settings on the (body) field

    Took me a bit to find where to enable translation. I suggest adding a hint to the collapsed global settings like: GLOBAL SETTINGS: Number of values (1), Field translation (disabled)
    s06-AddHintInCollapsedGlobalFieldSet-2012-10-03_1523.png

    Expanded it has wording like:
    Field translation
    This field is shared among the entity translations. --link--Enable translation--endlink--
    and looks like:
    s05-EnableTranslationOnBodyField-2012-10-03_1521.png

    Once it's enabled it has the wording:
    Field translation
    Users may translate this field. --link--Disable translation--endlink--
    And looks like:
    s07-GlobalSettingsTranslationEnabled-2012-10-03_1534.png

  4. More steps to testing

    Now on the translation tab of the content can add a translation
    s08-CanAddTranslation-2012-10-03_1545.png
    Access Denied when adding (german) translation (hmmm. I dont remember running into this when I was testing it the first time.)
    at /de/node/1/translations/add/en http://drupal.org/files/s09-accessdenied-2012-10-03_1549.png
    Edit permissions http://drupal.org/files/s10-PermissionToTranslateEntitiesOfTypeNode-2012...

I'm going to save this much and look into that a bit more. Back soon with more comments.

Bojhan’s picture

I don't fully get what to review. It feels a little like most internationalization, checkboxes all over :D

YesCT’s picture

Sorting out permission denied...

Notice

I disabled hiding of selecting the language on the article content type, and created a node/article and selected the language spanish. Then clicked on the edit link in the translations tab for that content and got a red notice message:

Error message
Notice: Trying to get property of non-object in node_help() (line 141 of core/modules/node/node.module).

files/s11-noticeGetProperty-2012-10-03_1558.png

But did not get permission denied.

And I could add a translation (/de/node/2/translations/add/es) without getting permission denied.

Still get permission denied on node 1 to add a translation.

Here is the link on node 1 translation tab to add a german translation: /de/node/1/translations/add/en
hmm link on node 2 to add a german translation: /de/node/2/translations/add/es (also permission denied)

deleted the english translation of node 2 and tried to add a german one /de/node/2/translations/add/es (worked fine)
strangeness. [English] showing both for german add translation and english add translation. In the location where the notice error showed up.
s12-addlink-2012-10-03_1612.png
s13-wronglang-2012-10-03_1614.png
s14-addenglishlink-2012-10-03_1615.png
s15-english-2012-10-03_1616.png

Clear caches.

That didn't help. When I was testing before (when it worked) I had at some point changed the default language of the site...

Well, that didn't help either. So.. I'll post my comments and the screen shots from earlier (So I dont get distracted and them not get posted at all) and come back to sort this out later.

YesCT’s picture

Tor Arne Thune’s picture

Bojhan: I think the attachment in #22 is your best bet :)

YesCT’s picture

Reusing my not quite as organized screenshots from earlier and I dont have notes to exactly reproduce these steps... sorry.

  1. Edit and brackets

    Observation: When I first added a translation, I was confused. I think it was because I clicked to "add" a translation and ended up at the page with the "edit". And the [ ] at first seemed like it was part of the title. Not sure if anything is needed to fix.
    et-ui-s001-what is are the brackets in the title area of the node edit 2012-10-03_1204.png

  2. Add visual clue to what is same across translations

    I suggest adding some text or visual clues as to which properties/fields/stuff-to-fill-out is the same across all translations, or specific to the translation being edited (the [German]). Maybe adding [All Translations: (Entity Language: Spanish)] or [Translation: German] to each?!
    et-ui-s002-visualcluetotranslatebits-2012-10-03_1221.png

  3. url Lang

    Sometimes the url and the links to click on seemed strange or to not be consistant
    et-ui-s003-edit language detection 2012-10-03_1237.png

  4. Translation Tab Redesign

    I think some reordering of the columns could make this translations tab be easier to use.
    et-ui-s004-translations tab strangeness 2012-10-03_1242.png
    I suggest the first column be the language as it is. The bold on the current node's language is good. But the wording "original content" is not strictly accurate. It's really the "translation" that is the language selected for the node/entity. Instead of "original content" use... ? "current entity language"?

    I suggest the translation column (column with node titles) be the next (2nd column). It was confusing for me to see it next to for example english, (when english was the "source language" and the translation I was going to click on was the german one.

    I suggest the next column (third) should be operations (edit), then the status (published) then the source language last.

  5. Revisions Tab

    I think it would be helpful for the revisions tab to be aware of translations. It could add something like english translation edited, or german translation added, spanish translation deleted.
    et-ui-s005-can revisions tab be translation aware 2012-10-03_1248.png

  6. Some hint when editing the special translation which is the language of the node/entity

    The only difference when editing a translation that is the language of the node is that the select is ... active.
    et-ui-s006-editing the node not translation 2012-10-03_1312.png

  7. Source Language collapsed field set

    When adding a translation and more than one other translation already exists, a source language can be selected. I think it would be nice to put the currently selected source in in the collapsed field set label.
    et-ui-s007-source language 2012-10-03_1321.png

  8. Add word *other* translations

    Add the word *other* since it doesn't mark all the translations as outdated, it just marks all the *other* ones outdated. Also, I think it uses the word "post" and sometimes it will be a vocabulary, term, or comment or node or other entity.
    et-ui-s008-flag _other_ translations as outdated 2012-10-03_1325.png

  9. Change wording, it's not the "source" (and not "post")

    When editing a translation that has been marked outdated, the wording says "because the source post has changed". But that's not strictly accurate. It could be marked outdated when someone has edited one of the translations that was not the source of that translation *and* also not the translation that is the current language of the node either. So it was just "because another entity translation has changed significantly"? Something like that. (Also, use something other than post since could be a vocab, term, etc.)
    et-ui-s009-not the source just was marked outdated 2012-10-03_1333.png

  10. Order of settings and fields on comment edit

    I suggest moving down the location of the translation setting collapsed field set.
    et-ui-s011-comment edit order of translation settings 2012-10-03_1355.png
    Cannot reorder in the ui.
    et-ui-s010-cannot arrange settings like admin and translation order 2012-10-03_1354.png

I have some more comments about... comments. :) To come.

YesCT’s picture

@Bojhan (#50 #53) I added some more things to look at.

YesCT’s picture

Adding final thoughts for the day before I forget them.

There was something strange the first time I tested, where the front/home page did not list any content once I started translating nodes.

A comment added, is added with the language of the entity being viewed.

When viewing a node with comments, the comments have links like: delete, edit, reply
I suggest adding translate link there which goes to the translations tab (same as edit, and then the translations tab)

I want to look later at what it is like to have a bunch of comments, some with translations in every language, some same as node language, some missing translations.

When a comment is missing a translation in the language that is being displayed, the comment title/subject is there (same across translations) but the body is just empty.

Observation: reverting a revision, reverts all the changes to translations (adding, editing, deleting) that had taken place. Seems reasonable.

While going through this I wondered how to translate titles. is title a property? leads maybe in issue #1188394: Make title behave as a configurable, translatable field (again).

Gábor Hojtsy’s picture

Status: Needs review » Needs work

@YesCT:

First of all thanks for executing your signature extensive review here, it is highly appreciated :) Then all the comments on specific points:

#49/1: on older Drupal 8 required; indeed, this applies to an older version of core, plach is working on a reroll

#49/4: on access denieds; BUG looks like from the first linked screenshot and comments later that you had your content type set to a specific language (the screenshot shows site's default language and hide language selector); this should have the effect, that content can only be created in that one language for that content type and translation should not be possible to enable; the core translation module has validation for this case on the server side and JS #states on the client side to make the user not be able to turn on translation if the language selector is hidden; that essentially ties a node type to a specific language; this should be implemented in this patch as well

#51/1: on node_help() notice; the patch does not touch node_help(), so I'm not sure how this would be caused by this patch, looks unrelated

#51/2,3, #54/1: on [English]; indeed, the language name seems like improperly displayed in the title or does not mean what we think it does :)

#54/2: on shared fields; I'm not sure we can directly solve this in this patch, we should solve it for Drupal 8 definitely; we had several discussions about this with yoroy, UserAdvocate, etc.

#54/3: on URL languages; I think this could be put into perspective if we look at the URL settings and the site default and the node languages; there are lot of moving parts which affect how the URL language is constructed; it is hard to tell if there is a bug based on the screenshots; eg. the missing 'es' in the URL could very well be due to the default setup of default languages having no prefix in the URL

#54/4: translation tab table; good suggestions for improvement IMHO :)

#54/5: on revisions; through the UI people can only edit one translation at once, so we could theoretically inject some log message information on which language they edited; not sure what language would that log message be? also would it be injected after the user provided message? should we automate that or just prefill the log message and let the user edit it as needed/intended? on the diff module side-note, yes, diff module should be compatible with this, the underlying field language support was already in Drupal 7 :)

#54/6: on source language version; agreed, some hint would be useful

#54/7,8: on other translations & source language fieldset; agreed with the suggestions

#54/9: on source post; I'm not sure what you experienced here; theoretically only the source content (original language / current language version) of the post should let you mark other translations as outdated; if you see this checkbox on a translation it is constrained to the specific translation only; so I think the "source" explanation is right (or the behavior is buggy :), but I agree "post" should be "content" for consistency

#54/10: on comment edit; I agree with the suggestions; would ideally be reoderable indeed

#56:

- it would be great if you have reproduction recipes for the no-content-on-frontpage situation
- agreed with the translation link suggestion for comments (if comments are configured to be translatable; I assume this would be a minority case :)
- on comment missing translations, the title is not translatable so far, so it is shared across languages; I don't think empty comments should be shown though, it should fall back on another language or not display the comment
- translating titles should become possible as entity schemas are reworked and property language support is added; this was already done for the test entity #1658712: Refactor test_entity schema to be multilingual, and should still be done for other entities, primarily for nodes (but also for others, which could be done after Dec 1st we believe); see #1498674: Refactor node properties to multilingual

Gábor Hojtsy’s picture

@Bojhan: the giga-image in #22 (http://drupal.org/files/et-ui.png) summarises the main UI steps. Let us know if you need more help in reviewing or want to set up a meeting to discuss. This is a cornerstone of the D8MI initiative and we want to get it right and want to get it in. :) It also is targeted to remove the existing content translation module, so this will be THE UI that people use to translate entities (node, taxonomy terms, etc.) in core.

Bojhan’s picture

I have looked at all of this stuff throughout the process, I think I do understand what is being proposed here. In general as I have been mentioning throughout this initiative I am woried, less and less people will be able to setup internationalization. We proudly entyise, fieldinze, and abstract everything - which leaves the user going from palce to place to set things up.

The individual screens as posted in #22 seem fine. The only thing that worries me is when you click make my content type translatable, in actuality it does nothing. You still have to go to each field and click the checkbox, is there a way we can make a [Enable translation for all fields], the first time you click the Enable translations box? Because that would decrease the setup flow significantly.

Almost all of the labels and descriptions could use some more work, but I'd think that's better to fix in followups. For example the Source language fieldset - could use a description, as I am sure many people have no idea what a source language is.

Gábor Hojtsy’s picture

I think that sounds like a good suggestion and combined with YesCT's suggestions can elevate the usability of this patch quite a bit :)

plach’s picture

Can we defer the bulk translatability setting to a separate issue? I was thinking about something like that too, but I'd like at least the basic functionality to go in ASAP.

YesCT’s picture

Regarding #59, I agree, enabling translation on all fields/properties when enabling translation on an entity is a good idea. I'd like to add a suggestion to default new fields to translatable if the entity is translatable.

[Thought process (can be skipped for those in a hurry):
I think that assumes that the fields will be added first and then translation enabled.
Maybe (ack another check box? action link?) in that same area that enables translation on an entity, there is another action: make all fields (aka stuff that can be translated) translatable. If I'm building a content type, I might make it translatable, and then add the fields? Hmm. Or make new fields default to translatable if the entity is translatable? (that's hidden in the global settings collapsed field set, but adding the hint to it would alleviate that concern.) I guess some of this will be a little less strange when some of the stuff other than fields: properties(?) like the title for nodes, subject for comments, ...term for term, are translatable. Because then, when enabling translation on an entity, even without adding fields, something will be translatable. Hold on. If properties work like the fields... then maybe that wont help. Because if it works like it does now, translation on the content type would be enabled, and then the property (title) would have to be edited to enable translation. So that does add weight to the suggestion to enable translation on all possible fields/properties when enabling translation on an entity.]

YesCT’s picture

@plach in #61 Do you think the bulk setting followup could be done without architecture changes? Also, if you post a reroll (mentioned in #45) of what you might have so far, you can leave a bunch of the easy stuff to me, and I can double check some of the troubles I had.

Bojhan’s picture

@plach I am generally not really confident i18n followups on usability happen, but sure :)

plach’s picture

Status: Needs work » Needs review

@YesCT:

Actually the issue here is pretty tricky since we have to deal both with the test entity, which has been converted to the new Property API, and the regular entities which haven't. Moreover all the new Entity hierarchy (apparently) doesn't deal correctly with fields in the default language (I guess the reason is that we would have to convert tons of Field API stuff in the initial patch to achieve that) and this makes things even harder. I'd prefer to keep working on this myself to avoid having to deal with other hacks than mine ;)

However I'll give you commit access to the sandbox as soon I sort this stuff out so you can work on your topic branches directly. My current work is on http://drupalcode.org/sandbox/plach/1719670.git/tree/refs/heads/tmp.

@Bojhan:

I have a good history of not leaving half-baked stuff around if I'm allowed to work on it :)

YesCT’s picture

@plach So does that mean, you want to get this in without the properties stuff? And do that in a separate issue too?

I dont know about a sandbox. I'm used to working with patches.

Gábor Hojtsy’s picture

@YesCT: the property work in the test entity already happened, it is to still happen for nodes and other entities, eg. #1498674: Refactor node properties to multilingual (hope to get that one done before Dec 1st, but other entities might be after feature freeze).

plach’s picture

Status: Needs review » Needs work

Didn't mean to change the status.

plach’s picture

Status: Needs work » Needs review
FileSize
97.35 KB
21.28 KB

I was finally able to rework everything to make it support the new Entity Field API. I couldn't address the UX reviews above. I'll do that ASAP. Let's see if the bot is happy.

Status: Needs review » Needs work

The last submitted patch, et-ui-1188388-69.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
97.51 KB
6.41 KB

Let's try this one.

plach’s picture

The attached patch should address more or less all the issues raised in the UX reviews above, with a couple of exceptions. Here is a summary of changes:

55dee26 Issue #1188388: Enabled URL language detection for content language by default.
a3bd813 Issue #1188388: Added target language to translation addition URLs.
740b6b3 Issue #1188388: Made entity form titles vary when adding translations and skipped alterations for original values.
a78002e Issue #1188388: Reordered translation overview columns.
0db6dc0 Issue #1188388: Injected entity info in the controller.
66bda4b Issue #1188388: Added source language hint.
7f53222 Issue #1188388: Improved wording for outdating translations.
5149a69 Issue #1188388: Added translation settings to the extra fields.
9b14f6a Issue #1188388: Added translations to comment links.
1666b8a Issue #1188388: Removed hardcoded admin theme default for the translations overview.
cannot translate because no translatable fields
edit the content type and go to manage fields to edit the body to enable translation

I agree that making all fields translatable when enabling translation on a bundle, and defaulting to translatable afterwards, could ease the setup process a lot, since when translation is enabled shared fields should be the exception and not the rule. An explicit option to authorize the conversion might be a good idea too. As I was saying above, I'd wish to defer this issue as it will be far easier to address after fixing the default language handling for fields to match the Entity Field API one. AAMOF it won't be needed a migration anymore since all original values will always be stored with a fixed language code indicating the default entity language. I believe this is the kind of small UI improvement that can be worked on after feature freeze (it should be matter of adding a checkbox or so).

Suggest adding hint near the global collapsed field settings on the (body) field

Might make sense but such a summary is out of scope for this issue: we would need to add summaries to collapsible fieldset as we do for vertical tabs, first.

Access Denied when adding (german) translation [...]

In general I need the steps to reproduce the bugs mentioned here as I was not able to see them. In this particular case I suspect the culprit might be some misconfigured language negotiation settings. The attached patch enables URL language negotiation for content language and adds an explicit target language parameter when adding a translation, instead of using the current content language. This should prevent access denied errors from happening.

Add visual clue to what is same across translations

We have a follow-up for this: #1498724: Introduce a #multilingual key (or similar) for form element.

Revisions Tab

I think simply suggesting a log message in the current interface language might work. Thoughts?

Translations Tab

The attached patch renames original content to original language. Moreover I moved the source language column after the Translation one as we always have operation links as last column. I thought having the source langauge column next to the link could be confusing too.

[...] this should have the effect, that content can only be created in that one language for that content type and translation should not be possible to enable;

I have to disagree here: a hidden language selector does not necessarily imply we don't want translations. For instance I might want comments to be always created in the current content language but be able to translate them afterwards. If you don't want translation just don't enable it :)

node_help() notice;

Looks like it's tied to URL prefixes, IIRC this happens also without the patch.

[...] on source post; I'm not sure what you experienced here; theoretically only the source content (original language / current language version) of the post should let you mark other translations as outdated;

Not sure about this either. The implemented behavior allows to flag translations as outdated from any translation. This could be a legitimate use case: say I originally created a blog article in italian and then translated it to english and an english comment suggests an important correction; I might not have the time to fix the italian article and just copy/paste the suggestion in the english one. Marking the italian translation as outdated would be useful here. AAMOF I got a feature request about this in ET D7.

@Bojhan:

[...] We proudly entyise, fieldinze, and abstract everything - which leaves the user going from palce to place to set things up.

Do you mean we would need a central place where enabling translation for all the available entities?

Bojhan’s picture

@plach Yes, that is what I mean - not just place, workflow :)

plach’s picture

Would you have a look to the Entity translation settings page of the D7 project to see whether it looks more like what you have in mind? Just install the module and visit: admin/config/regional/entity_translation.

Status: Needs review » Needs work

The last submitted patch, et-ui-1188388-72.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
100.16 KB
581 bytes

Forgot an include...

Status: Needs review » Needs work

The last submitted patch, et-ui-1188388-76.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
100.13 KB
709 bytes

wtf

Gábor Hojtsy’s picture

Tried to get a hold of where this patch is by reading the summary, but that is awfully out of date :/ Can you please update it to the current status? What's outstanding, what is complete, etc.

plach’s picture

I'll do ASAP, yesterday night it was too late :)

However, aside from the UX improvements in #72, #19 is more or less what will end up in the summary.

Gábor Hojtsy’s picture

The getAccess() method should be removed as soon as we have entity access available and replaced with EnttyInterface::access() calls or something along those lines.

This sounds like it would be a followup to this patch once/if entity access lands. I'm not seeing an outpouring of activity on #777578: Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter(), so looks like this is likely a no-go for Drupal 8 anyway, and nodes will stay special.

The removeTranslation() should probably go into the EntityInterface as well, since all the data-structure handling methods concerning entity translation are there, the translation controller is focused only on the UI.

Sounds good.

The get*Path() methods, which provide the paths to be used when building the UI for a particular entity type, should belong to a UI controller or whatever comes up from #1783964: Allow entity types to provide menu items and #1781372: Add an API for listing (configuration) entities where the same issues we have here are being addressed. AAMOF those methods are not specific to translation and atm we need to inject menu router info in the entity info to be able to generate the translation UI in a generic fashion.

There was a lot of discussion on the entity listing patch about paths known to the controllers, and the need for getURI if only admin paths are known, etc. I think these are fine to discuss in the mentioned related issues and we can keep refining this without the need for any major rework, making this patch free to use its current ways.

We need to find clean way to deal with translation metadata and the bare existence of translations: currently to determine whether a translation exists we just scan (each time) all the fields and properties to collect any language we can find. That collection is returned as the existing translations. This approach is IMHO inefficient and fragile, as all the changes I had to do the EntityTest and the related storage controller demonstrate. @fago and I discussed about a related issue in Munich: how to deal with language when accessing properties trough the upcoming Property API. We agreed that a variant of the previously suggested active language would work: basically we would have an EntityInterface::getTranslation($langcode) method that would return a class implementing the EntityInterface having as default the passed language. This would be used to access the properties in the usual way. We agreed that this could be a viable solution since it does not introduce the concept of state to the entity itself, we just retrieve a facet to act on (note that the class would not be a full entity class, just a wrapper or something like that). Building upon this I think we could have a full-fledged EntityTranslation class with its own storage controller and its properties, which would hold metadata like the status or the source language for that particular translation and an entity reference to the parent entity. As an alternative we could go the current way and declare metadata as multilingual properties of the parent entity, but the former is a solution worth to be explored as we would get the CRUD hooks or per-language access for free, just to make a couple of examples. If there are perfomance concerns for monolingual sites we could have a null implementation of the storage controller that performs no query in this case.

I don't think people would have null implementations for mono-lingual sites per say, unless we provide that explicitly. Also, this sounds like a rather big body of development to do to optimise translation access and handling. Sounds like if we want to do this, then it would need to be done before December 1st, regardless if we want to make it happen as part of the initial patch or not. My understanding is that the current discovery of translations would need to be kept to be a source mechanism to access those EntityTranslations, so this is a first step anyway. We can open a major followup unless you need this implemented for other reasons in the first pass.

No matter what we decide for the previous point, we should consider whether we want an independent status for each translation, even on entities like terms that have no status concept at all. I think we should because supporting any (custom?) field/property used as status in a custom workflow implemented through Rules or a specific module like Workbench or TMGMT could add lots of complexity. However also the translation status involves some complexity as you may end up with a published node with no published translation, for instance.

One of the expressed goals of this work is to make node-copy translation reproducible. If we don't have per translation statuses, then this is a no-go as a replacement of node-copy-translation. So I don't see how this is a question.

Currently the implementation of the entity translatability setting is a bit hacky, but I think we can clean it up as soon as #1739928: Generalize language configuration on content types to apply to terms and other entities lands.

That is still in the works. It might or might not get committed before this one. I'd not make either a dependency on the other one.

As soon as the new routing system supports multiple access callbacks we should leverage them to add per-language access checking to the entity form and exploit the 'edit original values' permission. The latter has been introduced and will be paired with a 'edit shared values' one to allow to mimick the old translation form we had in the early incarnations of the contrib Entity Translation. With none of them a translator can only access a form that has only translatable fields/properties and can act only on translations.

Your explanation is not clear if you want to introduce the new permission anyway and later make more use of it, or just want to introduce it later depending on routing feature additions? Sounds like a pretty major todo regarding the patch if you want to implement it right away.

I'm not totally happy with the translation addition as implemented right now: currenlty we have a dedicated page callback which provides an entity form having as form language the target one. What I'm unhappy about is that this way we are skipping all the access checks we would get if accessing the original page callback providing the entity form plus any alteration of the data returned by entity_get_form() performed in its context.

Right, one of the goals with the entity form language introduction was so we can just step in there and set the form to a different language instead of reproducing the same page. This would allow other augmenting modules to be compatible with the form out of the box. Looks like yet another todo for the patch.

Field column synchronization: this is used to translate stuff like the core image field which provides Alt and Title columns, which should hold translated values, and the regular Fid column that may need to be shared accross translations. If the media initiative succeeds in making them regular fields attached to File entity we would solve this use case, not sure if it would deserve some love anyway.

I think it is possible to say image fields are only translatable with a contributed module (which would implement this syncing if not core). IF we can get it into core, it is obviously better, but it should not be put as a blocker for the main functionality itself IMHO.

If we introduce translation status we need to provide fallback when viewing an unpublished translation.

Right, I think this is discussed above.

We need a more granular way to define whether fallback is enabled or not, mainly make it a per-bundle settings as translatability.

Don't know how would you make a meaningful UI for that though.

We need to introduce (optional) per-language filtering on comment lists, so that only the comments matching the current content language are displayed. We also need language-aware node comment statistics for this.

I don't think this belongs in this patch. Let's not blow up the scope.

We need to figure out how to deal with Path when deleting a translation, should we keep the current approach and supporting it explictly? I don't think so as this won't scale, instead we need to provide it a translation deletion hook to react to. Entity translation classes would help here.

I think a hook should be provided, which path module reacts to. Definitely not explicit. Sounds like yet another todo for this patch.

Same story for menu translation, but here would probably also need some work in the NodeTranslationController to adjust the node form to support multilingual menus.

This might need to happen in this patch as well :/

Lastly we may want to retain ET's ability to show authoring information and translation information together on nodes. Not sure about this.

I'm not sure what do you mean here, and why it would be core functionality. We need translations to be able to edit their own authoring information (authors, creation date, publication status, etc.). Not sure why would we need to show other information together, that is not done by core translation module either.

--------

All-in-all reading through these outstanding things, I'm starting to be a lot more worried of the change that this patch has. It would be great to get goal oriented and try not to blue-sky every plan if we want to make this work in Drupal 8 and not postpone to Drupal 9 :/ All-in-all I'm worried you got started on this so late and only you working on this patch with little cooperation from the entity API people. Can we somehow get Berdir or fago more involved? :)

plach’s picture

Let me just note, perhaps it wasn't clear in #19, that I consider almost all the bullets above nice to haves or clean ups. Some of them are just features already implemented in D7 which we may or may not want to port. Very few of them are IMO bound to the feature freeze. And none of them should hold this patch up.

I don't think people would have null implementations for mono-lingual sites per say, unless we provide that explicitly. Also, this sounds like a rather big body of development to do to optimise translation access and handling.

This probably won't be needed as EntityNG::getTranslation() returns the entity itself when the passed langcode is the default language.

My understanding is that the current discovery of translations would need to be kept to be a source mechanism to access those EntityTranslations, so this is a first step anyway.

This is an internal cleanup and totally not bound to the feature freeze, but we need a better way: things break all the time with this approach. I really had to struggle to make it kinda work here. For instance if you ask for an unexisting translation unknowingly, you actually get an EntityTranslation object and fields are poulated with empty values, and suddenly getTranslationLanguages() returns also the unexisting translation among languages. We need a more reliable API here.

One of the expressed goals of this work is to make node-copy translation reproducible. If we don't have per translation statuses, then this is a no-go as a replacement of node-copy-translation. So I don't see how this is a question.

Perhaps my question wasn't clear: once nodes are migrated to the new schema we will have per-language statuses. This is not the matter here. What I'm wondering is whether we should make the publication status a property of the entity translations, and thus making it available for any entity type. In D7 we have this, and node status, which is not differentiated per language, is used as a sort of global switch: you can have a publishd node with no published translations, which is pretty weird. Here I'd like to understand whether we should only map per-language statuses to entity translation statuses for entities that support that (in core only nodes) or remove the special-casing from nodes and make the status a generally available property, that any entity type can leverage, at least when dealing with translation.

Your explanation is not clear if you want to introduce the new permission anyway and later make more use of it, or just want to introduce it later depending on routing feature additions? Sounds like a pretty major todo regarding the patch if you want to implement it right away.

My plan would be to introduce these two permissions in a follow-up. The 'edit original values' can only be leveraged once we have multiple access callbacks. Does not sound as a new feature if we introduce the permission before feature freeze but actually use it afterwards. We could even code the access callback and have a small patch registering it when this is possible. #1793520: Add access control mechanism for new router system is critical stuff and the question is when it will be done, not if.

The shared fields permission is strictly tied to #1498724: Introduce a #multilingual key (or similar) for form element and could fit into it or be a follow-up.

Right, one of the goals with the entity form language introduction was so we can just step in there and set the form to a different language instead of reproducing the same page.

I'd prefer not addressing this here for one simple reason: to be able to switch the entity form language to a desired language we need to pass it in a query string parameter (i.e. something not requiring to alter the original router path). Reading this value from the entity form controller would mean hardcoding its logic into the entity system which is something I'd prefer to avoid. Rather I'd wish to read this URL parameter from an Entity Translation implementation of the upcoming hook_entity_prepare() hook and set the form language from there. This would decouple the entity form controller from the language switch logic implemented by ET. However what we have now just works and is enough to consider the feature "done". The rest is code clean up.

I think it is possible to say image fields are only translatable with a contributed module (which would implement this syncing if not core). IF we can get it into core, it is obviously better, but it should not be put as a blocker for the main functionality itself IMHO.

Consider that this capability is already implemented in D7: it's just a couple of functions and porting it should be really straightforward. Should totally be a follow-up and I expect it to be a quick one if we agree on the approach.

If we introduce translation status we need to provide fallback when viewing an unpublished translation.

Right, I think this is discussed above.

Yes, but do we want this for any entity type?

Don't know how would you make a meaningful UI for that though.

I think it can be another option below the one enabling translation.

[comment filtering]
I don't think this belongs in this patch. Let's not blow up the scope.

Again, all of this is already implemented in D7 and should totally be a follow-up :)

I think a hook should be provided, which path module reacts to. Definitely not explicit. Sounds like yet another todo for this patch.

Hooks are tricky if entity translations are not implemented as entities because there is no, well, entity to refer to. We are just dealing with some properties getting new values for a particular language and this may not be easy to intercept. Surely not when programmatically saving an entity. We would need at least some helper method in the Entity Field API to detect that a new transation has been pouplated/removed and, most importantly, stored/deleted. We cannot to this here. Do you think adding a couple of hooks would be problematic after feature freeze considering that most of the low level subsystems will be still moving after December 1st?

This might need to happen in this patch as well :/

I'm sorry but I don't think there is a way we can support menu translation here, since we don't even know if/how menus will be translatable. The right way to address this is the same for any "special" mulitingual widget (path, menu, whatever): use the form language to store the values and implement the Entity Translation hooks to react on storage changes. Definitely something we cannot do yet, but that is not exactly a new feature IMO: the UI would stay exactly the same, we'd just make things work the right way. Menu links and paths might even become fields so nothing special would be needed to support them.

I'm not sure what do you mean here, and why it would be core functionality.

This was implemented in the early days of the D7 TF4 patch: since we didn't have translatable properties but we already did have per-translation authoring metadata, ET provided (and provides) the ability to choose whether: always showing the node author in all languages, show the translation author per-language or showing both. Do we need something like this? It's done and working it just need to be ported.

All-in-all reading through these outstanding things, I'm starting to be a lot more worried of the change that this patch has. It would be great to get goal oriented and try not to blue-sky every plan if we want to make this work in Drupal 8 and not postpone to Drupal 9 :/

That's exaclty what I'm trying to do here. The patch here holds the bare essential to make the system work as already confirmed by a couple of persons who actually bothered to test the patch. Most of the rest is optional and nothing more should be done here. This is a self-contained change and rolling it back is just matter of removing a module. I don't think we should shold this up any longer (it has been sitting here for three weeks or so) otherwise, yes, we will have perhaps entity translation in core in D9, if someone will feel so brave to try this endeavour again.

All-in-all I'm worried you got started on this so late and only you working on this patch with little cooperation from the entity API people.

What I'm worried about is that no initative in D8 is really in a better shape than us. And we need to work on a continuously moving ground, since we rely on most of the others. Moreover none seems to have an idea of what the big picture will look like. I'm not worried about deadlines because what we will need to achieve after feature freeze, if we plan our work in a clever way, will be far less invasive than what other initiatives will have to do just to take D8 in a releasable state.

Time for a serious code review and RTBC comments are now welcome :)

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

I updated the issue summary leaving out from the reaming tasks section the features not ported yet, since we didn't reach a consensus about those yet.

plach’s picture

Issue summary: View changes

Updated issue summary

plach’s picture

Issue summary: View changes

Updated issue summary.

YesCT’s picture

@plach I'm going to work on opening follow up issues.

YesCT’s picture

  1. Improvement done: "other" translations out of date

    From #54/8 added the word other to say other content should be flagged as outdated, also took out "post" or changed post to content where appropriate.
    et-ui-78-s03b-niceother-2012-10-09_0038.png

  2. note: orig language edit link

    Translations tab edit link in original language row (with node before change the language select of the node) goes to /node/1/edit
    et-ui-78-s04-niceeditwithoutbrackets-2012-10-09_0046.png

  3. note: orig language no [language] bracket on edit page title

    edit of original content /node/1/edit has no [language]
    et-ui-78-s05-nobracketsonoriglanguage-2012-10-09_0048.png

  4. add translations link from translations tab

    add translations link from translations tab /node/1#overlay=node/1/translations goes to /de/node/1/translations/add/en/de
    Where does that first /de come from, why?
    At the end of the url the add/en/de, is that used to indicate it's using the english as the source for a german translation?
    et-ui-78-s06-transtabaddtrans-2012-10-09_0110.png

  5. Improvement done: adding a translation shows nice page title

    the add link takes me to: /de/node/1/translations/add/en/de#overlay-context=node/1
    nice hint in the page title that i'm creating the translation in a particular language (german in this case)
    I'm not sure though why I'm not in the overlay (not a huge issue, just noting because it was strange)
    et-ui-78-s07-nicehintcreatetrans-2012-10-09_0117.png

  6. note: translation tab edit translation link

    (I took the /de bit out of the url so I'm starting at /node/1#overlay=node/1/translations)
    As expected, the german row shows the source was english
    The edit translation link (in the german row in this case) goes to /de/node/1/edit
    et-ui-78-s08-transtabedittranslink-2012-10-09_0126.png

  7. improvement done: added "translation" to the inside of the [language] brakets in the page title on translation edit

    note that edit translation link took me to /node/1#overlay=de/node/1/edit (in the overlay)
    I think adding the word translation to the inside of the brackets at the end of the page title is a nice improvement
    (related to #54/1)
    et-ui-78-s09-edittransnicebrackets-2012-10-09_0131.png

  8. shows the source when adding a translation and multiple translations exist already

    the orig was english, and a german translation was already added. so when adding a spanish translation, it shows english is being used as the default.
    note that the source cannot be changed (because the language selection setting is set to be hidden on the article content type)
    et-ui-78-s10-addtranswhenmultexist-2012-10-09_0142.png

  9. /de or /es getting added to the urls and whoa when tried to config detection and selection

    a language was being appended to the urls after I would add a translation. Like if I added a german translation and then went to the home page it would go to /de So I thought I'd go to the language configuration to the detection and selection and take off the url method.
    And I saw this... a whole section added. My first thought was, I've applied this patch onto another patch I was testing ... but I did a git reset hard before applying this patch, so dont think that is what happened. Perhaps in #72 when plach said "55dee26 Issue #1188388: Enabled URL language detection for content language by default." this is the result of that?
    et-ui-78-s11-languagedetection-2012-10-09_0157.png

(I'm going to save this feedback so far and continue in another comment. So far, these are observations, and no suggestions for changes yet.)
[edit: I took out less than and greater than signs which were hiding some words.]

Gábor Hojtsy’s picture

Let me just note, perhaps it wasn't clear in #19, that I consider almost all the bullets above nice to haves or clean ups. Some of them are just features already implemented in D7 which we may or may not want to port. Very few of them are IMO bound to the feature freeze. And none of them should hold this patch up.

Then I don't understand why are we discussing those here? Discussing followup/unrelated points clearly holds the patch up. Also when followup tasks are opened, the background discussions will not be present there. Can we please focus on the issue and the real remaining tasks then instead, and NOT discuss unrelated questions? :)

YesCT’s picture

I suggest changing the links in the translation tab for adding a translation or editing a translation to use the current ui language independently of the translation being added and the source being used.

et-ui-78-s12-suggesturllang2012-10-09_0254.png

So in this example, of content with a spanish original language, and I'm in the english user interface language (site default language is english, and the url has no language specifier to change the user interface language), the url for adding a hungarian translation of node 3 (using spanish as the source language)
is currently
/hu/node/3/translations/add/es/hu#overlay-context=node/3
and I suggest it should be
/node/3/translations/add/es/hu#overlay-context=node/3

so the suggested pattern is:
/urlUsedForLanguageDetection/node/NID/translations/add/sourceLanguage/langOfTranslationBeingAdded
[edit: I took out less than and greater than signs which were hiding some words.]

plach’s picture

@Gabor:

Well, some of the issues above may have an impact on the work here, depending on which direction we take, and I don't take for granted that every single word I write is agreed upon :)

IMHO this patch is functionally complete. At least for a first step. Would be good to know if this is ok for everyone here and if the code is in a committable state.

YesCT’s picture

In #85/8 I thought that I was not able to change the source language when adding a new translation because I had not unchecked the hide language selection on the content type. But testing it with a content type that has the setting to allow language selection, I still cannot change the source. So if content has it's language set to be spanish, and a german translation exists, I cannot select to use the german translation as the source for a hungarian translation.

I don't think the inability to select the source was intended.

Doing a bit of testing,

/hu/node/3/translations/add/es/hu#overlay-context=es/node/3
does not allow the selection of the source of the translation

/hu/node/3/translations/add/es/hu
does allow the selection of the source of the translation

Both for the content type whose language selector is hidden and the content type that allows language selection (the hide checkbox is unchecked)

plach’s picture

@YesCT:

#85/4: The language prefix is there to switch the current content language: the point is that when I save my content I end up seeing the values I've just entered.

#85/5: The reason you don't get the overlay is that page is not considered an administrative path anymore (no admin theme). Not sure why.

#85/8: This is weird since there is no functionality to lock the source language when the language selector is hidden. Are you sure it is not clickable?

#85/9: That is a language negotiation feature already present in D7. Entity Translation just makes the content language configurable independently from UI language.

#87: Currently the etity form inherits the current content language for the UX reasons explained in the updated issue summary. The URL language prefixes are not impacted by this patch, you are just seeing the core behavior in action. The reason why english is not prefixed is just that there is no default prefix for the default language. This allows adding a second language after launching the site and not breaking the original URLs.

plach’s picture

I don't think the inability to select the source was intended.

This is weird. Did you clear the cache after applying the new patch?

YesCT’s picture

@plach regarding #89 and #91 (source language selection not possible sometimes) yep. I double checked the cache is cleared.

et-ui-78-s13-clickable-2012-10-09_0347.png

et-ui-78-s14-notclickable-2012-10-09_0349.png

YesCT’s picture

@plach in response to #90 and #85/9 ah. I see. I must not have noticed it before because I only had content translation on and not locale.

plach’s picture

#92 looks like a bug in the overlay: it seems some JS is not working properly.

YesCT’s picture

@plach #90 response to #85/4 ok. after creating a translation, seeing it (viewing the node) in that language does make sense. And as you pointed out, if I wanted the ui to not use the url language detection but the content to use it, that is possible since the settings are separate.

YesCT’s picture

in #54/4 I mentioned that "original content" was not accurate. And in the patch from #78, it's now using the phrase "original language" but that's still not really what it is. It's *the* (current) language of the node/content. If I create content in a language, and then edit that to change the language of that content, the "original language" tag moves to that new language... which thus is not the original language of the content. it's *the* language of the content. the language of the whole node, and not a translation of the node.

here are the steps I did:

  1. orig language spanish, english has no translation

    et-ui-78-s15-prep-origlang-2012-10-09_0417.png

  2. using the edit link in the spanish row (editing the spanish version/translation/originalcontent) can change the language to english

    et-ui-78-s16-allowsSwitchToEnglish-2012-10-09_0420.png

  3. now it says that english is the original language but it's not the original language, it's *the* language of the node

    et-ui-78-s17-origLanguageNotOrig-2012-10-09_0423.png

plach’s picture

It's the language of the original values. Perhaps just "original" would be better? We should avoid the entity wording in the UI as much as possible, as it unecessarily exposes the underlying terminology which will not be clear to the average user. Also the module name should probably change.

YesCT’s picture

in #56
me:
I want to look later at what it is like to have a bunch of comments, some with translations in every language, some same as node language, some missing translations.

When a comment is missing a translation in the language that is being displayed, the comment title/subject is there (same across translations) but the body is just empty.

then in #57
gabor: - on comment missing translations, the title is not translatable so far, so it is shared across languages; I don't think empty comments should be shown though, it should fall back on another language or not display the comment

in #82
gabor: [comment filtering]
I don't think this belongs in this patch. Let's not blow up the scope.

plach: Again, all of this is already implemented in D7 and should totally be a follow-up :)

Here is the follow-up. #1807322: Filter comments taking into account the current content language
@plach Please clarify the wording in that issue (and maybe assign it to yourself?)

YesCT’s picture

@plach in response to #97 I suggest "n/a" Since it is the source (it's the default source for new translations). It's also *the* language of the content... it's the language listed in the content list.

Or, if "original" is usually accurate, then maybe keep it, and make a note somewhere that original means that it is the current language of the content, and that it might have been some other language and changed since it's creation to be a different language.

Content list shows the current language aka original language http://drupal.org/files/et-ui-78-s18-contentlist-2012-10-09_0519.png

(Could this be a sticky point on my part because english is my first language and original means something specific to me? I'll let it go.)

YesCT’s picture

Doing a bulk/mass enabling of translation on fields and properties would help the user experience of enabling translation for an entity.

Background:
#49/2 where after enabling translation on a content type, looking at the translation tab, translations cannot be added because there are no translatable fields. Each field that is translatable has to be edited and the translation enabled for that field.

#59 Bojhan:
The individual screens as posted in #22 seem fine. The only thing that worries me is when you click make my content type translatable, in actuality it does nothing. You still have to go to each field and click the checkbox, is there a way we can make a [Enable translation for all fields], the first time you click the Enable translations box? Because that would decrease the setup flow significantly.

#60 Gabor agrees

#61 plach suggests a separate issue for bulk translatability

#62 I agree mass enabling of translation would help

Here is the follow-up #1807366: Make fields translatable by default when enabling translation on a bundle

It's a follow-up because it can be done after the feature freeze. For the feature freeze, just this Entity Translation UI is the base.

plach’s picture

@Bojhan:

Made a couple of screenshots of the UI I was referring to in #74: it's the D7 Entity Translation configuration page. Here you can enable entity types for translation and configure per-bundle settings. In D7 per-bundle translation is supported only for certain entity types so we don't have it here.

Not sure if it brings a good UX, but just to understand whether it's more close to what you are suggesting.

et-config-1188388-101-1.png

et-config-1188388-101-2.png

YesCT’s picture

[ background #49/4, #57, #72 55dee26 Issue #1188388: Enabled URL language detection for content language by default., #85/9, #87, #90, #93, #95 ]

About language detection: enabling entity translation module adds a table of settings to language settings detection and selection and defaults to url (the first row) being checked. If that is unchecked, then the links in the translation tab of content to "add" a translation work /node/1/translations/add/es/en (create an english translation using spanish as the default source) and after saving the new translation the node is shown /node/1 which shows in the site default language (or rather in the language detected by the content language detection admin/config/regional/language/detection

Is this as designed? That the url language detection has to be enabled for content language detection and first in that detection and selection table in order to be able to do translations in the ui?

I think doing translations would still be doable without url content language detection if like the add link, the "edit" link on the content translation tab was aware of what translation it was editing /node/1/translations/edit/hu Viewing the node after it's edited would be weird as it would use whatever language the content language detection settings selected [edit: change suggestion from /node/1/edit/hu to /node/1/translations/edit/hu]

I think it would be useful to have the content title in the translations tab translation column be a link that forced viewing of that translation no matter what the detection and selection settings were. A view translation link like: /node/1/translations/view/hu
And I think that could be what the page redirects to after adding or editing a translation. The view tab on the content would still go to /node/1 (or whatever like /hu/node/1 ) based on the content language detection and selection.

Does that make this a nicety to have in a follow up issue? (Because the translation does work pretty well using this patch if the url is left as the content language detection method.)

YesCT’s picture

Trying to add information to help answer plach's question about if hooks can be added later from #82 (and #81?)

gabor quoted: I think a hook should be provided, which path module reacts to. Definitely not explicit. Sounds like yet another todo for this patch.

plach: Hooks are tricky if entity translations are not implemented as entities because there is no, well, entity to refer to. We are just dealing with some properties getting new values for a particular language and this may not be easy to intercept. Surely not when programmatically saving an entity. We would need at least some helper method in the Entity Field API to detect that a new transation has been pouplated/removed and, most importantly, stored/deleted. We cannot to this here. Do you think adding a couple of hooks would be problematic after feature freeze considering that most of the low level subsystems will be still moving after December 1st?

Dries's http://buytaert.net/updated-drupal-8-release-schedule says "During feature freeze (December 1 to April 1)
The goal of the feature freeze is to improve the implementation of existing features and to improve consistency and coherence of core by removing special cases, and unifying duplicate ways of doing things by converting core to use new APIs, etc.

During this phase we stop adding new subsystems. Refactoring of existing subsystems, smaller API changes and API additions are allowed if they help improve consistency and coherence of the existing functionality or are necessary in order to resolve specific critical tasks and bugs."

YesCT’s picture

Summarizing a follow-up for column synchronization for image field etc. that use shared fid column across translations
from #82
plach first bullet point under Here are features not yet ported as I need some feedback: in #19: Field column synchronization: this is used to translate the core image field which provides Alt and Title columns, which should hold translated values, and the regular Fid column that may need to be shared accross translations. If the media initiative succeeds in making them regular fields attached to File entity we would solve this use case, not sure if it would deserve some love anyway.

gabor in #81: I think it is possible to say image fields are only translatable with a contributed module (which would implement this syncing if not core). IF we can get it into core, it is obviously better, but it should not be put as a blocker for the main functionality itself IMHO.

plach in #82: Consider that this capability is already implemented in D7: it's just a couple of functions and porting it should be really straightforward. Should totally be a follow-up and I expect it to be a quick one if we agree on the approach.

Here is the follow-up #1807692: Introduce a column synchronization capability and use it to translate alt and titles through the image field widget
[edit: removed duplicate phrase from cut and paste error]

YesCT’s picture

Topics: Translation Status, Publication status per entity translation property, Special case for nodes, Fallback for unpublished translation

background

  1. Translation Status does not need a follow-up to implement per-language statuses it's being handled in a different issue "once nodes are migrated to the new schema we will have per-language statuses"
  2. gabor quoted from #81 One of the expressed goals of this work is to make node-copy translation reproducible. If we don't have per translation statuses, then this is a no-go as a replacement of node-copy-translation. So I don't see how this is a question.
  3. plach in #82 Perhaps my question wasn't clear: once nodes are migrated to the new schema we will have per-language statuses. This is not the matter here. What I'm wondering is whether we should make the publication status a property of the entity translations, and thus making it available for any entity type. In D7 we have this, and node status, which is not differentiated per language, is used as a sort of global switch: you can have a publishd node with no published translations, which is pretty weird. Here I'd like to understand whether we should only map per-language statuses to entity translation statuses for entities that support that (in core only nodes) or remove the special-casing from nodes and make the status a generally available property, that any entity type can leverage, at least when dealing with translation.
  4. and then further down about half way through #82
    plach quoted from first bullet point under Here are features not yet ported as I need some feedback: in #19
    If we introduce translation status we need to provide fallback when viewing an unpublished translation.
  5. plach: Right, I think this is discussed above.

Questions

  • Q1: @plach please clarify the "if we introduce translation status" phrase from background part 4. I think this patch would not introduce translation status because in part 3 of the background it says "once nodes are migrated to new schema" and that we dont have to do it in this patch.
  • Q2: @plach if you can, please give the issue number or link to what is taking care of "once nodes are migrated to new schema"
  • Q3: (for anyone, plach asks in background part 3) should we make the publication status a property of the entity translations generically for any entity?
  • Q4: (my question to clarify Q3) is the publication status already makde a property of nodes as a special case? And is that the translation status referred to in Q1 and Q2?
  • Q5: do we need to provide fallback when viewing an unpublished translation? In this issue? In a follow-up issue?

[edit: @plach, sorry I found these related questions and added them to this comment]

YesCT’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

@plach please clarify the "if we introduce translation status". Is that done in this patch already?

Nope, this is one of the major follow-ups. At the moment only nodes have published status support.

YesCT’s picture

Summarizing a follow-up for permission to edit fields shared across translations.

background

issue summary remaining tasks (and originally plach in #19: As soon as the new routing system supports multiple access callbacks we should leverage them to add per-language access checking to the entity form and exploit the 'edit original values' permission. The latter has been introduced, and will be paired with a 'edit shared values' one, to allow to display a stripped-down entity translation form: with none of them a translator can only access an entity form that has only translatable fields/properties and can act only on translations: #1793520: Add access control mechanism for new router system, #1498724: UX: Introduce a #multilingual key (or similar) for form element

gabor in #81 Your explanation is not clear if you want to introduce the new permission anyway and later make more use of it, or just want to introduce it later depending on routing feature additions? Sounds like a pretty major todo regarding the patch if you want to implement it right away.

plach in #82 My plan would be to introduce these two permissions in a follow-up. The 'edit original values' can only be leveraged once we have multiple access callbacks. Does not sound as a new feature if we introduce the permission before feature freeze but actually use it afterwards. We could even code the access callback and have a small patch registering it when this is possible. #1793520: Add access control mechanism for new router system is critical stuff and the question is when it will be done, not if.

The shared fields permission is strictly tied to #1498724: UX: Introduce a #multilingual key (or similar) for form element and could fit into it or be a follow-up.

follow-up

Here is the follow-up #1807776: Support both simple and editorial workflows for translating entities I'll set it to be postponed on #1793520: Add access control mechanism for new router system and #1498724: Introduce a #multilingual key (or similar) for form element

YesCT’s picture

Issue summary: View changes

Updated issue summary.

YesCT’s picture

I think the remaining task in the issue summary to add support for entity generic translation status (not specific to nodes) ties in with #105

plach #19 No matter what we decide for the previous point, we should consider whether we want an independent status for each translation, even on entities like terms that have no status concept at all. I think we should because supporting any (custom?) field/property used as status in a custom workflow implemented through Rules or a specific module like Workbench or TMGMT could add lots of complexity. However also the translation status involves some complexity as you may end up with a published node with no published translation, for instance.

gabor in #81 One of the expressed goals of this work is to make node-copy translation reproducible. If we don't have per translation statuses, then this is a no-go as a replacement of node-copy-translation. So I don't see how this is a question

Q1: @gabor is the phrase "node-copy translation" used somewhere else? I'm not sure what that is. Is it already implemented in this patch as the functionality to "add" a translation in a language in that on that create page, it populates values form a source translation?

Looking at the placement in the issue summary, I think that means this (and the items from #105) are to be a follow-up issue.

Here is that follow-up issue #1807800: Add status and authoring information as generic entity translation metadata

YesCT’s picture

Issue summary: View changes

Updated issue summary

YesCT’s picture

I updated the issue summary with links to the actual follow-up issues. There appear to still be some remaining tasks that need follow-up issues created.

YesCT’s picture

I did some more testing with the new patch from 78 about the translation field settings positioning for nodes and comments. Summary, nice improvements but changing the location of the translations settings on the comment translation edit is not quite right yet, the field set shows up *below* the save button.

  1. translations settings can be positioned on the node manage fields tab

    et-ui-78-s19-translationsettingsinmanagefields-2012-10-09_1450.png

  2. looking at default position of language setting on node edit

    et-ui-78-s20-languagesettinglocationonedit-2012-10-09_1452.png

  3. moved it on the content type manage fields tab

    et-ui-78-s21-changethelocation-2012-10-09_1456.png

  4. reload the node edit and see the language select position is changed.

    (I like it closer to the title)
    et-ui-78-s22-languageselectionmoved-2012-10-09_1458.png

  5. check out the manage display tab on content type.

    translation (language setting) is by default hidden.
    et-ui-78-s23-coulddisplaylanguagefieldonnode-2012-10-09_1503.png

  6. there is no language or translation field in the comment display (and the table border is strange)

    et-ui-78-s24-cannotshowlanguageoncomment-2012-10-09_1505.png

  7. adding comment to a node

    defaults to detected language (not sure if that's the user interface detected language or the content detected language)
    et-ui-78-s25-addingcomment-sitedeflang-2012-10-09_1511.png

  8. comment has nice translations link added

    et-ui-78-s26-commenttranslinkadded-2012-10-09_1514.png

  9. add a translation of a comment. strange order: translation setting below save button

    et-ui-78-s27-commenttranssettingstrangelocation-2012-10-09_1519.png

  10. see what is in that translation collapsed fieldset

    it's the checkbox to mark other translations outdated. and it has strange spacing for indenting.
    it mentions published status, and just noting not all entities will have "published" but it's a common example of a status and is good to keep that wording.
    et-ui-78-s28-translationsettingexpandedoncommentstrangeindent-2012-10-09_1524.png

  11. nice. looking at what a node is like with multiple comments not all translated into the detected language

    et-ui-78-s29-showstranscommentandbodyofnontrans-2012-10-09_1527.png

  12. went to change the order of the translation setting on the comment edit page to put it above the save button

    but dont see a way to do it without putting the setting above the body. ... and I dont think people will want it above the body. They will just want it above the save button. I tried showing the weights and changing the numbers directly and it didn't help.
    et-ui-78-s30-dontwanttransabovecomment-2012-10-09_1532.png

Berdir’s picture

Status: Needs review » Needs work

Looked through it, haven't really read it in detail, so this is mostly coding style stuff.

Disclaimer: I haven't read any of this issue, just looked at the code.

+++ b/core/includes/entity.incundefined
@@ -106,6 +106,14 @@ function entity_info_cache_clear() {
 
 /**
+ * Returns the defined bundles for the given entity type.
+ */
+function entity_get_bundles($entity_type) {

Missing @param and @return documentation.

+++ b/core/lib/Drupal/Core/Entity/Entity.phpundefined
@@ -248,36 +248,41 @@ public function getTranslation($langcode, $strict = TRUE) {
-   * TranslatableInterface::getTranslationLanguages().
+   * is deprecated by TranslatableInterface::getTranslationLanguages().
    */
   public function translations() {
-    $languages = array();
+    return $this->getTranslationLanguages(FALSE);
+  }
+
+  /**
+   * Implements TranslatableInterface::getTranslationLanguages().

Class references in docblocks need to be fully qualified. Drupal\Core\Entity\...

+++ b/core/modules/comment/comment.moduleundefined
@@ -150,6 +150,12 @@ function comment_entity_info() {
+  if (module_exists('translation_entity')) {
+    $return['comment']['translation']['translation_entity'] = array(
+      'class' => 'Drupal\comment\CommentTranslationController',
+    );
+  }

Lot's of module_exists() checks added, wondering if there are ways to avoid this.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityTranslationTest.phpundefined
@@ -24,7 +24,7 @@ class EntityTranslationTest extends WebTestBase {
    *
    * @var array
    */
-  public static $modules = array('entity_test', 'locale');
+  public static $modules = array('entity_test', 'locale', 'node');

Isn't node enabled by default?

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/VocabularyFormController.phpundefined
@@ -139,6 +146,11 @@ public function save(array $form, array &$form_state) {
+    $variable_name = 'translation_entity_enabled_taxonomy_term_' . $form_state['values']['machine_name'];
+    if (isset($form_state['values'][$variable_name])) {
+      variable_set($variable_name, $form_state['values'][$variable_name]);

If this is a new setting, then it should be converted to config(), I don't think code with new variables is being committed still.

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/EntityTranslationController.phpundefined
@@ -0,0 +1,322 @@
+    drupal_set_message(t('Source language set to: %language', array('%language' => t($languages[$source]->name))));

I think we're not translating language names anymore?

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/EntityTranslationController.phpundefined
@@ -0,0 +1,322 @@
+      drupal_set_message(t('This will delete all the @entity_type translations.', array('@entity_type' => drupal_strtolower($info['label']))), 'warning');

Not sure if this a valid thing to do (the strtolower). Maybe keep it in the original case and rewrite the message a bit?

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/EntityTranslationControllerInterface.phpundefined
@@ -0,0 +1,96 @@
+   *
+   * @param EntityInterface $entity
+   *   The entity to the path should refer to.
+   */
+  public function getBasePath(EntityInterface $entity);
+
+  /**
+   * Returns the path of the entity edit form.
+   *
+   * @param EntityInterface $entity
+   *   The entity to the path should refer to.

Replace "@param Entity" with @param Drupal\Core\Entity\Entity"

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/EntityTranslationControllerInterface.phpundefined
@@ -0,0 +1,96 @@
+   * @param $op
+   *   The operation to be performed.
+   *
+   * @return
+   *   TRUE if the user is allowed to perform the given operation, FALSE
+   *   otherwise.
+   */
+  public function getAccess(EntityInterface $entity, $op);

What are the valid operations? I assume this is partially translation specific and should probably be listed...

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Tests/EntityTranslationUITest.phpundefined
--- /dev/null
+++ b/core/modules/translation_entity/tests/modules/translation_entity_test/translation_entity_test.infoundefined

+++ b/core/modules/translation_entity/tests/modules/translation_entity_test/translation_entity_test.infoundefined
+++ b/core/modules/translation_entity/tests/modules/translation_entity_test/translation_entity_test.infoundefined
@@ -0,0 +1,8 @@

@@ -0,0 +1,8 @@
+name = Entity Translation test module
+description = Implements hooks to test the Entity Translaion functionality.

I think we can remove the modules directory there. tests/ doesn't contain anything else than test modules anymore...

+++ b/core/modules/translation_entity/translation_entity.admin.incundefined
@@ -0,0 +1,222 @@
+  if ($field['translatable'] !== $translatable) {
+    // Field translatability has changed since form creation, abort.
+    $t_args = array('%field_name' => $field_name, '!translatable' => $translatable ? t('untranslatable') : t('translatable'));
+    drupal_set_message(t('The field %field_name is already !translatable. No change was performed.', $t_args), 'warning');

Why !translatable instead of @trans... ?

Also, I'm not sure if this is valid or it if should be split into two different t() strings?

+++ b/core/modules/translation_entity/translation_entity.admin.incundefined
@@ -0,0 +1,222 @@
+    // How many entities will need processing?
+    $query = new EntityFieldQuery();
+    $count = $query
+      ->fieldCondition($field_name)
+      ->count()

EFQ is currently being refactored, support for queries across multiple entity types is been removed. Is there a way we can rewrite this to make it easier to port, whichever patch gets in first...

+++ b/core/modules/translation_entity/translation_entity.moduleundefined
@@ -0,0 +1,503 @@
+ *
+ * @param $entity
+ *   The entity being translated.
+ *
+ * @return Drupal\translation_entity\EntityTranslationControllerInterface
+ *   An instance of the entity translation controller interface.
+ */
+function translation_entity_controller($entity_type) {

@param and actual argument mismatch.

+++ b/core/modules/translation_entity/translation_entity.moduleundefined
@@ -0,0 +1,503 @@
+ * @param $entity
+ *   The entity to be accessed.
+ * @param $langcode
+ *   The language of the translation to be accessed.
+ *
+ * @return
+ *   TRUE if the current user is allowed to view the translation.
+ */
+function translation_entity_access(EntityInterface $entity, $langcode) {

types missing.

+++ b/core/modules/translation_entity/translation_entity.pages.incundefined
@@ -0,0 +1,223 @@
+        $status = !isset($translation->status) || $translation->status ? t('Published') : t('Not published');
+        // @todo Add a theming function here.
+        $status .= !empty($entity->retranslate[$langcode]) ? ' - <span class="marker">' . t('outdated') . '</span>' : '';

I guess this would mess up with RTL languages?

+++ b/core/modules/translation_entity/translation_entity.pages.incundefined
@@ -0,0 +1,223 @@
+  // Add metadata to the build render array to let other modules know about
+  // which entity this is.
+  $build['#entity'] = $entity;

Hah, sounds familar :p

+++ b/core/modules/translation_entity/translation_entity.pages.incundefined
@@ -0,0 +1,223 @@
+ * Populates target values with the source values.
+ */
+function translation_entity_prepare_translation(EntityInterface $entity, Language $source, Language $target) {
+  // @todo Unify field and propesrty handling.

Typo. No param documentation.

Bojhan’s picture

@Plach Essentially that is what I mean, however I would imagine a different workflow. Feel free to contact me over IRC/Skype to discuss this.

Bojhan’s picture

Issue summary: View changes

Updated issue summary. I added links to the follow-up issues that were created.

YesCT’s picture

Issue summary: View changes

clarified what the remaining tasks are for this issue and what are follow up issues

YesCT’s picture

I'm prepping this issue to get it commit ready.

I updated the issue summary. I think there was some earlier confusion that the follow up issues were under remaining issues, and the issue summary template uses the term remaining issues to mean issues that have to be addressed in the issue... See http://drupal.org/node/1155816 So I moved them into their own follow-up issue heading. And I took the info from the issue summary template and put it in, which triggered some of the following questions:

@plach Can you (or someone else) clarify if any documentation has been written, or if any need to be written?

@plach Can you (or someone else) describe the current test coverage?

@plach I guessing that the patch in #78 is not what we are going to ask to be committed. Please post what your current code is.

I gave myself some tasks.

I'll get started now opening up issues for the last follow-up issues.

I gave myself the task of doing a final big bunch of screen shots because there have been changes since #22 and the rest are spread out through the issue and also are a bit out of date since there have been improvements to the code. I'll do that once plach's most recent code is up.

Also, once plach's most recent code is posted, we'll need a good code review to see if it's RTBC or not.

YesCT’s picture

Issue summary: View changes

Updated issue summary. clarified the ui review is done and assigned some stuff to plach and some to me.

YesCT’s picture

I opened the remaining follow-ups and updated the issue summary to reference them.

Maybe some of the earlier follow-ups I opened that have status active should be marked postponed on this issue.

The remaining steps in the issue summary should be accurate for what needs to happen next.

YesCT’s picture

Issue summary: View changes

Updated issue summary. Added references to the specific follow-up issues

plach’s picture

Status: Needs work » Needs review
FileSize
100.32 KB
13.42 KB

The attached patch takes care of @Berdir's review plus a small bug fix to the admin theme. Couldn't address @YesCT's latest comments yet.

76ca214 Issue #1188388: Fixed PHP docs (#111).
2b32014 Issue #1188388: Removed unneeded module dependency in the entity translation tests (#111).
64698ad Issue #1188388: Fixed various messages (#111).
d98541b Issue #1188388: Improved translations overview UX (#111).
0c62c50 Issue #1188388: Removed empty test module (#111).
4d928ed Issue #1188388: Fixed admin theme for translation addition/deletion.
44614c1 Issue #1188388: Improved entity deletion warning.
Lot's of module_exists() checks added, wondering if there are ways to avoid this.

There will be: as soon as we can exploit #1739928: Generalize language configuration on content types to apply to terms and other entities, we will be able to remove many module_exists(). I'm also planning to simplify the field translation handler stuff: once that's done, defining the entity translation stuff in the entity info even if ET is disabled will be harmless. Right now it would erroneously imply that ET is a valid field translation handler for the various entities albeit being disabled.

If this is a new setting, then it should be converted to config(), I don't think code with new variables is being committed still.

This is a totally temporary hack until #1739928: Generalize language configuration on content types to apply to terms and other entities is available, the patch doesn't even bother to uninstall the variables.

I think we can remove the modules directory there. tests/ doesn't contain anything else than test modules anymore...

The test module was empty, I dropped it. We can resurrect it when/if needed.

EFQ is currently being refactored, support for queries across multiple entity types is been removed. Is there a way we can rewrite this to make it easier to port, whichever patch gets in first...

When field storage is updated to match the Entity Field API logic, we won't need a migration anymore so all this code will be just dropped. I'd avoid wasting time with it.

cosmicdreams’s picture

in that case, can you put in @todo comments for that future work?

cosmicdreams’s picture

Issue summary: View changes

Updated issue summary to note all follow-ups have been opened.

aspilicious’s picture

+++ b/core/modules/translation_entity/translation_entity.admin.incundefined
@@ -0,0 +1,225 @@
+ * Confirm form for changing field translatability.

We need 3th person verbs for this. And this doens't have any docs.

+++ b/core/modules/translation_entity/translation_entity.admin.incundefined
@@ -0,0 +1,225 @@
+ * Toggle translatability of the given field.

Toggles

+++ b/core/modules/translation_entity/translation_entity.admin.incundefined
@@ -0,0 +1,225 @@
+ * Batch operation. Convert field data to or from LANGUAGE_NOT_SPECIFIED.

Same

Don't we need some kind of reference in this function? Or does php pass stuff into arrays as reference?

+++ b/core/modules/translation_entity/translation_entity.moduleundefined
@@ -0,0 +1,503 @@
+ * Widget to enable entity translation per entity bundle.

Should start with 3th person verb

+++ b/core/modules/translation_entity/translation_entity.moduleundefined
@@ -0,0 +1,503 @@
+ * @param $suffix
+ *   An element name suffix, usually the entity type such as 'node' or 'user'.
+ * @param $bundle
+ *   The bundle name.

It's best to typehint the @param stuff

nod_’s picture

Issue tags: +JavaScript

tag

YesCT’s picture

Status: Needs review » Needs work

@plach Seems like there is a bit left to do. So setting this to needs work. If it should be needs review, and you are looking for a review with a particular focus, post back and we'll try and round up someone for this.

Otherwise, I have a couple questions

  • Q1 Should we have @todo in the code to address @cosmicdream's #115 response to #115 or should we have separate follow-up issues opened for them?
  • Q2 in #115 plach
    Couldn't address @YesCT's latest comments yet.

    Does that mean you think you'll have a couple little fixes in a new patch, or that follow-up minor bug issues should be opened?

[edit: grammar fix for clarity]

plach’s picture

The attached patch takes care of the latest reviews including @YesCT's ones. Specifically:

afc6cb5 Issue #1188388: Fixed comment form weights.
597be98 Issue #1188388: Improved UX of the translations overview.
1cdf161 Issue #1188388: Fixed PHP docs.
b72c3dc Issue #1188388: Added @todos for follow-up issues.

Some answers:

Is this as designed? That the url language detection has to be enabled for content language detection and first in that detection and selection table in order to be able to do translations in the ui?

Well, currently we use the detected content language for the UX reasons outlined above. I agree it would make sense to be able to force form language and ignore the deteced language. However it won't be possible to implement that cleanly until we have hook_entity_prepare() in place. Same issue as the translation addition mentioned somewhere above.

I think doing translations would still be doable without url content language detection [...] [edit: change suggestion from /node/1/edit/hu to /node/1/translations/edit/hu]

We don't want a different menu router for editing translations as we would lose access checks and page callback form massaging. We need to use a query string parameter.

I think it would be useful to have the content title in the translations tab translation column be a link that forced viewing of that translation no matter what the detection and selection settings were. A view translation link like: /node/1/translations/view/hu [...]

Not sure about this. Content language negotiation aims exactly at letting you choose which language you wish to view content in. If you disable it, you are basically saying "I have a translated site but no one will be able to see its translations". Does not really sound as a valid use case to me.

Q1: @plach please clarify the "if we introduce translation status" phrase from background part 4. I think this patch would not introduce translation status because in part 3 of the background it says "once nodes are migrated to new schema" and that we dont have to do it in this patch.

This patch does not introduce per-language status for nodes. This will come with #1498674: Refactor node properties to multilingual.

Q4: (my question to clarify Q3) is the publication status already makde a property of nodes as a special case? And is that the translation status referred to in Q1 and Q2?

The publication status is a property, albeit not migrated yet to the Entity Field API, and is indeed a special case: only nodes in core have it. It is what is referred to in Q1 and Q2.

Q5: do we need to provide fallback when viewing an unpublished translation? In this issue? In a follow-up issue?

In a follow-up. I'm not planning to work on anything but addressing code reviews, unless someone comes up with a very good reason to add new stuff in this patch.

there is no language or translation field in the comment display (and the table border is strange)

The language extra fields is not provided by this patch.

defaults to detected language (not sure if that's the user interface detected language or the content detected language)

The detected content language.

no place to specify the language of the comment

Language selectors are not provided by this patch.

@plach Can you (or someone else) clarify if any documentation has been written, or if any need to be written?

No documentation has been written yet. We will probably need a page describing how to setup everything.

@plach Can you (or someone else) describe the current test coverage?

The current tests should cover more or less all the features implemented here.

@aspilicious:

Thanks for reviewing :)

And this doens't have any docs.

Usually we don't provide docs for form builder functions, do we? However as I was saying above that code is going away soon. No point in documenting it.

Don't we need some kind of reference in this function? Or does php pass stuff into arrays as reference?

Not sure I get what you mean. Anyway, same as above: that code is going once Field API language support is converted to be compatible with the Entity Field API.

YesCT’s picture

Status: Needs work » Needs review

needs review to get the bot to look at it (and the people!) Thanks plach

YesCT’s picture

Status: Needs review » Needs work
+++ b/core/modules/comment/comment.moduleundefined
@@ -1250,10 +1268,25 @@ function comment_form_node_type_form_alter(&$form, $form_state) {
+    // @todo Remove this check once language settings are generalized.

Note there's no "requirement" for an issue (or linking to the issue) for @todo 's http://drupal.org/node/1354#todo

These many @todo's to for once language settings are generalized are referring to #1810386: Create workflow to setup multilingual for entity types, bundles and fields

@webchick (or anyone else) Do all the @todo's need followup issues? Do such follow-ups need to have links in the @todo comments in the code?

YesCT’s picture

Issue summary: View changes

Updated issue summary. took out the remaining task to post the most recent code. done.

YesCT’s picture

Issue summary: View changes

Updated issue summary with information about tests and documentation

plach’s picture

Status: Needs work » Needs review

No idea of what that needs work stands for.

YesCT’s picture

@plach Sorry, I'm not used to dreditor auto changing status to needs work. Need review is exactly where this should be. :)

plach’s picture

Whew :)

YesCT’s picture

I talked to webchick in irc about #122 and she says only major @todo's which require collaboration with others need issue numbers.

I have a lingering feeling that if a todo corresponds to an issue already created, that it would be handy to ref the issue number in the @todo, but that sounds optional too.

So I think that leaves this just needing an up-to-date bunch of screen shots which I can tackle late tonight and then we put this out there.

YesCT’s picture

FileSize
71.91 KB
119.91 KB
79.33 KB
70.35 KB
94.56 KB
90.11 KB
80.81 KB
54.41 KB
108 KB
110.76 KB
121.99 KB
123.86 KB
108.45 KB
141.77 KB
69.93 KB
87.11 KB
81.19 KB
145.22 KB
61.02 KB
167.58 KB
118.1 KB
105.23 KB
105.23 KB
80.9 KB
109.69 KB
125.15 KB
88.62 KB
79.01 KB
104.27 KB
97.7 KB
108.23 KB
103.38 KB

Getting ready

It applies to the most recent 8.x (I had to do some rm'ing to clean out some stuff)
rm -r core/modules/language/config
git checkout core/modules/language/config
sudo rm -r sites/default
git checkout sites/default
git pull --rebase
git reset --hard
curl -O http://drupal.org/files/et-ui-1188388-120.patch
rm -r core/modules
git checkout core/modules
git pull --rebase
git reset --hard
git checkout -b et-ui-1188388-120
git apply et-ui-1188388-120.patch
drush -y si --account-name=admin --account-pass=admin --db-url="mysql://root:root@localhost/d8-patch" --account-pass=admin --site-name=d8-patch-et-ui-1188388-120

also,
at /admin/modules enable language and entity translation modules

Screenshots

Summary:
Even more nice things fixed by plach.
Just posting node stuff now; will follow-up with comments etc later.
There was a little weirdness with language detection when node did not have a translation that lang detection tried to detect. We can come back to look at that later; it might be a lang detection thing and not a et-ui thing anyway.

  1. at Configuration -> Languages /admin/config/regional/language add some languages. I added german, hungarian and spanish.
    et-ui-p120-s01-addLanguages-2012-10-15_2331.png
  2. at Structure -> Content Types -> Article -> edit /admin/structure/types/manage/article enable translation in the Language settings vertical tab
  3. and disable hiding language
    et-ui-p120-s03-unhideLanguageSelectorOnArticle-2012-10-16_0000.png
  4. add a couple articles.
    Can see the language select since the hide setting was unchecked on the article content type.
    The language select is in the default location.
    et-ui-p120-s04-createContent-2012-10-16_0006.png
  5. Translation tab.
    After creating an article, viewing it has a translations tab along with the view and edit tab.
    et-ui-p120-s05-transTab-2012-10-16_0009.png
  6. Translations tab.
    Shows overview of translations. Right now, nothing is translatable, so cannot add translations.
    et-ui-p120-s06-transTab-2012-10-16_0011.png
  7. Can reorder fields.
    (moved the language field while at the edit content type page to enable translation on body.)
    et-ui-p120-s07-moveLangFieldSetting-2012-10-16_0018.png
  8. Edit fields (body) to enable translation for them.
    et-ui-p120-s08-enableTransOnBody-2012-10-16_0022.png
  9. Confirmation of enabling translation on body field.
    et-ui-p120-s09-confirmBatch-2012-10-16_0024.png
  10. After translation is enabled, the global settings when editing the field updates to reflect: Users may translate this field. And it has a Disable translation link there.
    et-ui-p120-s10-bodyEnabled-2012-10-16_0025.png
  11. Translation tab.
    In general, process of translating goes smooth(est) when using the url language detection and selection method.
    Add link to add a translation uses the url method to show the entity in the language for the translation being added after it is saved. Add link uses the language of the entity as the source for the translation.
    et-ui-p120-s11-addTrans-2012-10-16_0028.png
  12. Adding a translation goes to the create form
    which has nice hints that a translation is being added and in what language.
    by default the flag other translations as outdated is unchecked, but can see the wording used.
    et-ui-p120-s12-addingTrans-transSettingOutdateOthers-2012-10-16_0039.png
  13. After saving a translation, see it in that language
    et-ui-p120-s13-afterSaveSeeTrans-2012-10-16_0051.png
  14. Translation tab (with more info)
    Translations get their source that was used recorded.
    The entity is indicated by having it's language bolded and does not have a source (it is the [default] source) so it's source is n/a.
    et-ui-p120-s14-addTrans-multTrans-2012-10-16_0055.png
  15. Select source
    Can select the source when adding a translation if there is more than one translation when a new translation is being added.
    Setting for source is in a collapsed fieldset (with a nice hint as to the value of the source)
    et-ui-p120-s15-canSelectSource-2012-10-16_0106.png
  16. show the source setting expanded
    et-ui-p120-s16-changeSource-2012-10-16_0110.png
  17. after changing source
    the hint in the collapsed fieldset has the right value, and the fields in the form have values from the new source.
    et-ui-p120-s17-changedSource-2012-10-16_0112.png
  18. Flag other translations as outdated
    Can check the box in the translation vertical tab translation setting to flag other translations as needing to be updated.
    et-ui-p120-s18-useFlagOutdatedSetting-2012-10-16_0116.png
  19. Translation tab (showing outdated translations)
    and showing the edit link
    et-ui-p120-s19-showOutdated-editTrans-2012-10-16_0125.png
  20. Editing of a translation
    (showing it without my red marks)
    et-ui-p120-s20-editTrans-2012-10-16_0139.png
  21. Editing of a translation is nice
    has lots of hints
    no changing source while editing a translation
    et-ui-p120-s21-niceEditTrans-2012-10-16_0134.png
  22. When editing an outdated translation can unflag it
    can uncheck to indicate it has been updated in the translation setting in the vertical tab. shows wording.
    et-ui-p120-s22-uncheckWhenUpdated-2012-10-16_0144.png
  23. Translation Tab (outdated indicator does away)
    When saving a translation and unchecking the outdated box, the indicator on the translation tab goes away.
    The node titles for translation rows use the url lang detection to show the content translated into that language.
    et-ui-p120-s23-updatedTrans-nodeTitleViewLink-2012-10-16_0150.png
  24. Editing the entity
    (showing it without my red marks)
    et-ui-p120-s24-editingTheNode-2012-10-16_0154.png
  25. Editing the entity
    The hint that this is the node and not a translation is that [WhateverLanguage Translation] is not there in the page/form title
    Editing *the* node allows a different language to be selected, if there is a different language that does not have a translation yet.
    et-ui-p120-s25-editingTheNodeChangeLanguage-2012-10-16_0155.png
  26. Saved after changing lang of entity
    This is a little weird, but probably a general language detection issue and not specific to this et-ui.
    et-ui-p120-s26-weirdnessChangeLangOfNode-2012-10-16_0201.png
  27. Translation Tab (with different entity language)
    et-ui-p120-s27-diffLangOfNode-2012-10-16_0208.png
  28. Edit a translation to Delete the translation
    et-ui-p120-s28-editToDeleteTrans-2012-10-16_0210.png
  29. Confirmation to delete translation
    et-ui-p120-s29-confirmationDeleteTrans-2012-10-16_0212.png
  30. Edit *the* entity (and not a translation) to delete the whole thing including all the translations
    et-ui-p120-s30-deleteNodeShowsWhenEditInLangOfNode-2012-10-16_0217.png
  31. Confirmation to delete whole thing
    et-ui-p120-s31-deleteNodeConfirmsDeleteAllTranslations-2012-10-16_0220.png
YesCT’s picture

Issue summary: View changes

Updated issue summary to reflect latest code is posted and update remaining tasks.

plach’s picture

Wonderful. Anyone wishing to RTBC this or push it back to needs work? ;)

Gábor Hojtsy’s picture

I got considerable pushback earlier when trying to introduce new variable_*() variables citing it is inconsiderate to not use the new systems and then lump the conversion up on someone's plate later. Would the commit of #1739928: Generalize language configuration on content types to apply to terms and other entities help with resolving that?

YesCT’s picture

  1. Content before enable translation on comments
    et-ui-p120-c-s01-stuffToCommentOn-2012-10-16_0402.png
  2. add comment (without comment translation)
    et-ui-p120-c-s02-addingComment-2012-10-16_0409.png
  3. A comment (without comment translation)
    et-ui-p120-c-s03-aComment-2012-10-16_0408.png
  4. Enable comment translation on Article
    et-ui-p120-c-s04-enableCommentTranslationOnArticle-2012-10-16_0412.png
  5. Comment Translation Link
    et-ui-p120-c-s05-commentTranslationsLink-2012-10-16_0413.png
  6. Comment Translation Link/Tab
    similar to node, nothing to translate
    et-ui-p120-c-s06-translationsTabOfAComment-2012-10-16_0415.png
  7. Enable comment translation on comment body field
    Structure -> Content Types -> Article -> Edit -> Comment Fields tab
    at admin/structure/types/manage/article/comment/fields/comment_body
    Comment body field edit has translation settings in the collapsed global fieldset:
    "This field is shared among the entity translations. Enable translation"
    et-ui-p120-c-s07-enableTranslationsOnCommentBodyField-2012-10-16_0417.png
  8. Now translations to add
    et-ui-p120-c-s08-commentTranslationsTabWithAdd-2012-10-16_0423.png
  9. Adding comment translation
    et-ui-p120-c-s09-addingCommentTranslation-2012-10-16_0425.png
  10. Add comment translation, other translations outdated setting defaults to not flag
    et-ui-p120-c-s10-addCommentTrans-TransSettingToOutdate-2012-10-16_0427.png
  11. Just added comment translation (views thing commented on)
    et-ui-p120-c-s11-viewJustAdded-2012-10-16_0428.png
  12. Add comment (not lang specified in url)
    et-ui-p120-c-s12-addCommentInOtherLang-2012-10-16_0430.png
  13. comment is in language that was detected for content
    et-ui-p120-c-s13-commentAddedInLangOfContentDetected-2012-10-16_0431.png
  14. Edit comment, no lang field to change language of comment
    et-ui-p120-c-s14-commentsHaveNoLanguageField-2012-10-16_0433.png
  15. Add a translation, use flag other comments as outdated
    et-ui-p120-c-s15-addingTranslation-flagOutdated-2012-10-16_0435.png
  16. comment translations with outdated flag
    et-ui-p120-c-s16-translationsWithOutdatedFlag-2012-10-16_0437.png
plach’s picture

@Gabor:

Yes, if #1739928: Generalize language configuration on content types to apply to terms and other entities goes in soon, I think it makes sense to adjust the stuff here to leverage it.

YesCT’s picture

Vocabulary and Terms

  1. enable translation on a vocabulary
    et-ui-p120-t-s01-enableTranslationOnAVocabulary-2012-10-16_0451.png
  2. add a tag
    et-ui-p120-t-s02-addingATag-2012-10-16_0454.png
  3. term has no language specified
    et-ui-p120-t-s03-termNoLangSpecified-2012-10-16_0458.png
  4. with a language specified, gets a translation tab
    et-ui-p120-t-s04-languageSpecifiedGetsTranslationTab-2012-10-16_0500.png
  5. translations tab
    et-ui-p120-t-s05-translationsTab-2012-10-16_0501.png
YesCT’s picture

Users

  1. enable translation on users under Configuration
    et-ui-p120-u-s01-enableTranslationOnUsers-2012-10-16_0504.png
  2. with translation on user enabled, get a translation tab
    et-ui-p120-u-s02-translationTabOnAUser-2012-10-16_0505.png
YesCT’s picture

Status: Needs review » Reviewed & tested by the community

changing status and updating summary

YesCT’s picture

Issue summary: View changes

Updated issue summary.

YesCT’s picture

Issue summary: View changes

Updated issue summary to reflect screenshots posted and might be blocked on generalize language cmi 1739928

plach’s picture

Issue summary: View changes

Updated issue summary.

cosmicdreams’s picture

OMG YesCT you are freaking AMAZING. Are you creating those screenshots manually or are you using a program to help you? Sorry for the off-topic comment but it needs to be said.

YesCT’s picture

Oh, I see I missed one of the important things to get screenshots of: the test entity. plach put #22 in the list of UI changes in the issue summary to remedy that. It's the last shot in that string of screen shots from #22.

@plach can you point me at how to get to the test entity?

YesCT’s picture

@cosmicdreams amazing or crazy? Manually. I'm using jing ... just because dougvann recommended it a year ago or so and it makes it pretty smooth. I try and have the url in the shots, so I can know where I was when it was taken, and I use a prefix, a numbering, and some description in the filename which helps a lot later put the files into something comprehendible. I keep notes in a text file while I go through things, so I can note anything funky and try later and decide if it was a mis-step on my part or a bug. In the notes I'll write down like: go back and try it with different site default, or look into ...whatever, so I dont forget things that pop into my head while documenting a certain workflow series. I have an interest in seeing what others do to be efficient in testing/writing up results and started to collect some ideas in the comments on http://drupal.org/node/1489010 I don't know if there is a similar "task" page or other documentation to look at for some recommended ways of writing up test results.. maybe http://drupal.org/node/1468198 If someone points to a good doc page, we can continue conversation there too. And, I'll take any advice or hints.

YesCT’s picture

Issue summary: View changes

Updated issue summary.

YesCT’s picture

#120: et-ui-1188388-120.patch queued for re-testing.

plach’s picture

Issue summary: View changes

Updated issue summary clarifying where the test entity screen is.

plach’s picture

On commit please credit also @peximo who did lots of work in the sandbox...

Edit: please refer to the updated credits section in the summary.

plach’s picture

@YesCT:

@plach can you point me at how to get to the test entity?

Just enable the Testing module, run the Entity Translation UI tests and you will find the pages ready for screenshot in the verbose messages.

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue summary: View changes

Updated issue summary.

Dries’s picture

A lot going on in this issue. I like what I see, but I think this patch could really benefit from some real usability testing.

I'm going to spend some more time testing this patch.

catch’s picture

How far does this get us towards being able to remove the translation module from core? I'm assuming pretty far but it's not explicitly listed as a follow-up in the issue summary.

Bojhan’s picture

The UX-Team, as our resources are spread out right now - do not have the capacity to test this.

I would most definitely applaud anyone, doing usability testing on i18n and fully here to support in setting up the testplan.

plach’s picture

@catch:

How far does this get us towards being able to remove the translation module from core? I'm assuming pretty far but it's not explicitly listed as a follow-up in the issue summary.

Pretty far but not there. To just replicate what core is able to do with the Content Translation module we need at very least to get #1498674: Refactor node properties to multilingual in. There is no issue for removing the Content Translation module from core because, realistically, we cannot yet. Hopefully git rm core/modules/translation + upgrade can be done after feature freeze. However if the related issue is needed to get this one committed I'll create it ASAP.

catch’s picture

I think it's fine to nuke the translation module after feature freeze when adding an upgrade path for the data, it's certainly not adding a feature and we can't realistically ship with two competing UIs in core.

If for some reason we didn't get to that point we could still move the translation module to contrib as a reverse of the Drupal 7 situation. I was already following [##1498674] but I'll make sure to keep an eye on it.

YesCT’s picture

Not dissuading any UI review, but for background, a summary of Bojhan in #59: the individual screens are ok, improvements in labels and descriptions would be better in follow-ups and the general approach could use centralization of configuration and specifically that bulk enabling of translation on translatable fields and properties when translation is enabled on an entity would greatly help. #1807366: Make fields translatable by default when enabling translation on a bundle is one of the follow-ups listed in the issue summary.

@Dries, and others let us know if you need anything. So great to get more eyes on this.

webchick’s picture

I'm going to try really hard to review this tomorrow night or Friday. Unfortunately, I don't have a lot of 2+ hour unbroken chunks of time, and this patch is a doozy. :)

plach’s picture

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

Issue summary: View changes

Updated issue summary.

YesCT’s picture

This had a follow-up issue opened to handle that, #1810386: Create workflow to setup multilingual for entity types, bundles and fields. So if it gets handled here directly, we'll get to close that issue. (I updated the summary)

plach’s picture

Well, when I wrote that stuff I assumed the UI would be ready earlier than the other issue. As Gabor was pointing out in #129, not having variable_get/variable_set() here is likely to ease commit.

Working on an updated patch.

plach’s picture

Status: Needs work » Needs review
FileSize
103.99 KB
16.83 KB

Here is an updated patch using the regular config storage for settings instead of variables and integrating with the new language configuration. The patch includes a backward compatibility layer for core entities not supporting language configuration yet (comments and users). The result is cleaner but not perfectly clean :)

Status: Needs review » Needs work

The last submitted patch, et-ui-1188388-151.patch, failed testing.

fago’s picture

Great work! I had a short look at the code. Most issues can perfectly be fixed in a follow-up, however the first one needs to be fixed before commit. The compiled list of follow-ups looks good.

+    'menu base path' => 'entity-test/manage/%entity_test',
+    'translation' => array(
+      'translation_entity' => array(
+        'class' => 'Drupal\entity_test\EntityTestTranslationController',

Shouldn't we just follow the usual pattern and call it 'translation controller'? Why the nesting?
This misses documentation - as well as all the other added hook_entity_info() keys. This needs to be fixed before commit.

+   * Returns TRUE if the enity form language is matches the entity one.
+   */
+  public function isDefaultFormLangcode($form_state);

is matches. Misses @return.

+    $definitions = $translation->getPropertyDefinitions();
     foreach ($values_excluding_fields as $key => $value) {
-      $entity->$key = $value;
+      if (isset($definitions[$key])) {
+        $translation->$key = $value;
+      }

Why not just loop over $translation ? That gives you the same.

ad translation_entity_controller():

A lot of EntityTranslationControllerInterface misses @return docs, e.g. getBasePath()/getEditPath()/getViewPath() and getSourceLangcode().

plach’s picture

Status: Needs work » Needs review
FileSize
108.32 KB
8.27 KB

Aside from a typo in the ET-specific tests, the failures were caused by not completely migrating taxonomy term language configuration from using the 'vocabulary' entity type to the 'taxonomy_term' entity type, which is the correct one, since those affect how the taxonomy term language works (see also #1739928-153: Generalize language configuration on content types to apply to terms and other entities). This one should pass tests.

I'll address @fago's review later, meanwhile people can test the UI with the latest core HEAD. Just one note:

Shouldn't we just follow the usual pattern and call it 'translation controller'? Why the nesting?

This is already documented in D7: it's the way to declare field translation handlers. I think we don't need them anymore and wish the get rid of those, but this can safely happen after feature freeze. When they are gone we should totally use a key structure consistent with the rest of entity controllers. What about a @todo meanwhile?

Status: Needs review » Needs work

The last submitted patch, et-ui-1188388-154.patch, failed testing.

fago’s picture

>This is already documented in D7: it's the way to declare field translation handlers.
Yes, but we are adding an entity translation controller. That's not a field translation handler so it should not be in there?

plach’s picture

Status: Needs work » Needs review

#154: et-ui-1188388-154.patch queued for re-testing.

plach’s picture

The attached patch addresses #153, except:

Why not just loop over $translation ? That gives you the same.

Won't this iterate on Field API fields too? Here we are updating plain properties, or am I missing something?

fago’s picture

Won't this iterate on Field API fields too? Here we are updating plain properties, or am I missing something?

Oh, sure - you are right. I did not realize this is only about base-fields. Instead, we could check the 'configurable' key in the definition, but nevermind.

The changes look good to me - great to see so much documentation being added! However, I'm a little confused by the (doc) function hook_entity_info() which duplicates the regular one. Is that the usual approach for documenting keys that a module adds somewhere? If not, I fear api.drupal.org might have troubles with it.

webchick’s picture

Status: Needs review » Needs work

Initial review. UX only. I have tried really hard not to look at this issue at all, so I can come in blind as a new user using Drupal 8 and trying to translate entities. I apologize in advance, because there's a lot in here that is not positive. This is in no way meant to disparage the excellent hard work that's gone into this, is only meant to be proactive about staving off a frustrating user experience. I think we want to nail this.

Here's the process I went through.

1) I went to admin/modules and enabled the Entity Translation module.

PROBLEM: There's no hook_help(). Needs to be resolved before commit, since it's part of the documentation gate.

PROBLEM?: There's no link to a settings page from the modules page. Not sure if this is an actual problem yet.

2) I click "Permissions" to check out what permissions are available. Listed there are:

"Edit original values" (Access the entity form in the original language)
"Toggle field translatability" (Toggle translatability of fields performing a bulk update.)
"Translate any entity" (Translate field content for any fieldable entity.)

PROBLEM: The only of these permissions that makes any sense to me is "Translate any entity." I do not understand why the other two are permissions. Probably needs some better wording.

PROBLEM: I would expect given the "Translate any entity" permission that there would also be permission for "Translate FOO entity" per each FOO. Like for example, "Basic page: Translate own content / Translate any content"

3) I go to node/add/article and make an article.

PROBLEM: I do not see any options related to translation here on the form, nor on the subsequently published node.

4) I go back to Extend, click help on Language module since that was listed as a dependency for this one. It mentions the "list of languages." While it doesn't mention that it's required to add languages there in order to get translation functionality, I click the link to admin/config/regional/language and try it.

PROBLEM: Probably add some more wording there to point this out as a required step in the translation configuration process? (Assuming that's true?)

5) I add a new language, French. I keep English as the default.

6) I go back to my node. Still no mention of any language stuff.

7) I go to Structure > Content types > Article > Edit. YAY! Now I see language settings. I see that the default is for the language selector to be off, and translation to be off as well. I reverse the checkboxes there, keep the default language set to site default (English).

PROBLEM: Do those make sense as the default settings..? I figured if I enabled this module, I probably want my entities translated, no?

8) Go back to my node. Bingo. NOW I see Translations tab! Click it. Taken to kind of a useless screen that just tells me I don't have a translation for French.

PROBLEM: No way to add a translation from here, we should add one.

PROBLEM: I do not really know what "Source language" means, nor why it's "n/a" in both cases. Shouldn't it be English, since this is the site default?

PROBLEM: I think "edit" operations should be in a dropbutton?

9) Since that was a dead-end, I move to edit instead. I don't immediately see a way of adding a translation here. In desperation, I change Language to French and edit/save the text.

10) Back to Translations tab, and now the order is reversed. English shows not translated, and French shows edit.

PROBLEM!: I have no idea how to provide a translation to my content. :( :(

11) Ok, one last try. In that table under "operations" for English it says "No translatable fields." I go to Structure > Content Types > Article > Manage fields. Click "Edit" on Body. Expected to see "Translatable" option but didn't.

PROBLEM: It actually is there, but is under a collapsed fieldset of "Global settings." Should probably be front-and-center since I decided this content type should be translatable.

12) I expand "Global settings" and click "Enable translation." Says "By submitting this form you will trigger a batch operation."

PROBLEM: Do normal people know what a "batch operation" is? At the very least we should probably tell people something about this, like "This could take a few minutes to process, and in the meantime your content will [what? be inaccessible?]"

13) I am bold and click Confirm. :P Says it is completing 1 of 2 something, then "Data successfully processed."

PROBLEM: That error message is meaningless to me. What just happened? What data? How was it processed?

14) I go back to my node one more time. Click Translations tab. Ah-HA! Now there is "add" next to English, under operations. We are getting somewhere. I click it. I change the title and body back to what they were before. I also change the taxonomy term for fun.

PROBLEM: The Language field still says "French" and I cannot change it. That's really confusing. I thought I was adding an English translation?

15) Now I go back to the Translations tab, and…

PROBLEM!! It changed the title on both of my translations to the English version. :(

PROBLEM: There is no indication on the node page of what language version I am viewing. I see that there is a tiny indicator in the path, but only because I am very carefully observant. ;) IMO the node language should default to displaying here.

PROBLEM: Even upon noticing that, while fr/node/1 took me to the french translation, en/node/1 took me to a 404. :(

16) I go back to admin/structure/types/manage/article/fields but I have no idea how to make Title translatable like I did Body. :( It's not a field.

17) I give up go play video games for awhile. :P

---

So all in all, this was a bit of a frustrating experience. :( Here are some things that would make it better:

1) A help page for Entity Translation that threw me some breadcrumbs on what I need to do to get entities actually translatable. I believe from playing around with this that this entails:

a) At least two languages in the configured languages screen.
b) A content type that has "Enable translation" selected.
c) One or more fields within that content type that are marked translatable.

2) Even better, some sensible defaults so all of that set up was not required for me, and/or a wizard-like thing to walk me through how to configure these things in advance. I have literally zero idea how someone not already familiar with Drupal would've ever have completed this task. I'm still not actually sure that I completed it successfully.

3) Translatable titles (and probably bodies?), on by default for content types marked for entity translation. Also, probably make entities default to translation on if this module is enabled?

4) A visual indicator on the forms of translatable entities as to which fields would change globally and which fields would change only for this one translation. It was a nasty surprise that changing my taxonomy terms to "le cat" changed them for all the other translations as well.

plach’s picture

Initial review. UX only. I have tried really hard not to look at this issue at all, so I can come in blind as a new user using Drupal 8 and trying to translate entities. I apologize in advance, because there's a lot in here that is not positive. This is in no way meant to disparage the excellent hard work that's gone into this, is only meant to be proactive about staving off a frustrating user experience. I think we want to nail this.

First of all, thanks for reviewing, Angie :)

I understand you wanted to have a look to this with fresh eyes, and you did a great job IMHO, however most of the UX issues you identified are already covered by planned follow-ups (see the issue summmary). As I was pointing out in #19 I think we should not hold this up just because the UX is not perfect yet: we have all the feature freeze to improve it. Lots of low-level features went in with crappy performance, which is meant to be fixed later in the development cycle, I don't see how this is different. We need a UI to start seeing multilingual entities in action and find out what's broken and needs to be fixed in the lower subsystems.
OTOH I think you nailed at least one blocking issue (hook_help()) which I'm going to fix ASAP.

Let me answer to you points:

PROBLEM?: There's no link to a settings page from the modules page. Not sure if this is an actual problem yet.

We have no entity translation settings at the moment.

PROBLEM: The only of these permissions that makes any sense to me is "Translate any entity." I do not understand why the other two are permissions. Probably needs some better wording.

The 'edit original values' is meant to grant access to the entity form in the entity language. Translators are not supposed to be able to change the original values, just enter translations of them. There is another permission planned which is going to hide any form widget dealing with non-translatable elements. The combination of the two permissions will let us provide a form that exposes to translators only translatable elements and forbid them to alter the original ones. Any suggestion for wording this better is welcome.

About the toggle translatability one, it's going away with all the data migration stuff (see the OP) as soon as field language handling is updated to match the Entity Field API one.

PROBLEM: I would expect given the "Translate any entity" permission that there would also be permission for "Translate FOO entity" per each FOO. Like for example, "Basic page: Translate own content / Translate any content"

This is already implemented: when you enable translation for a bundle you will find a new permission for the related entity type allowing you to specify which roles can translate it.

PROBLEM: Probably add some more wording there to point this out as a required step in the translation configuration process? (Assuming that's true?)

Do you mean a message when enabling ET? Might make sense, suggestions for the exact wording are welcome as I'm no native english speaker.

PROBLEM: Do those make sense as the default settings..? I figured if I enabled this module, I probably want my entities translated, no?

The language selector stuff is not provided by this module. Wrt to the translation checkbox we cannot assume you wish to translate every content type, hence I think explictly enabling it for translation should be required. We should inform our users of that in a more clear way, however.

PROBLEM: No way to add a translation from here, we should add one.

Actually we are already planning to make all fields translatable by default when a bundle is enabled for translation, so you should not need this last step. However perhaps linking the 'Manage fields' page from the 'No translatable fields' text could improve UX a bit?

PROBLEM: I do not really know what "Source language" means, nor why it's "n/a" in both cases. Shouldn't it be English, since this is the site default?

Source language should be pretty familiar to a translator: it's the language you translate from. The original values have no source language (n/a), a missing translation has no source language either (n/a). See #99 and previous for a related discussion.

PROBLEM: I think "edit" operations should be in a dropbutton?

In the current UI we have only one operation, however it might make sense to improve consistency.

PROBLEM: It actually is there, but is under a collapsed fieldset of "Global settings." Should probably be front-and-center since I decided this content type should be translatable.

This is really not something we can address here. Field settings are collapsed by default and translatability is a field setting. The solution is having the proper default in place so that only people having to change it will need fo find the related checkbox (all the migration stuff is going away).

PROBLEM: The Language field still says "French" and I cannot change it. That's really confusing. I thought I was adding an English translation?

This is another heavily discussed pain point: that form element is meant to change the entity language. It indicates the language the entity was created in, not the current translation language. And it's not meant to be changed. In D7 it's labelled "Original language" but also this does not seem to be fitting. Not really sure which is the best way to address this, but whichever route we choose it should be an easy fix, no need to block the whole stuff here.

PROBLEM!! It changed the title on both of my translations to the English version. :(

Node properties are not translatable yet, see #1498674: Refactor node properties to multilingual. However ET already works with translatable properties as demostrated by the included test case.

PROBLEM: There is no indication on the node page of what language version I am viewing. I see that there is a tiny indicator in the path, but only because I am very carefully observant. ;) IMO the node language should default to displaying here.

I have no idea of how you noticed the URL prefix and not the page title saying you are creating/editing a translation :(
This is a problem I don't have an answer to. However I'd classfy this as UX improvement and not blocker too.

PROBLEM: Even upon noticing that, while fr/node/1 took me to the french translation, en/node/1 took me to a 404. :(

This is because the default language negotiation settings don't assign a URL prefix to the default language (for very good reasons). It's not something related to this patch.

I go back to admin/structure/types/manage/article/fields but I have no idea how to make Title translatable like I did Body. :( It's not a field.

If I understood @fago correctly there will never be a UI to configure plain properties. They will just be marked as translatable or not internally based on the current business logic.

1) A help page for Entity Translation that threw me some breadcrumbs on what I need to do to get entities actually translatable.

Agreed. Going to work on that ASAP.

2) Even better, some sensible defaults

Agreed. There already is a planned follow-up for this: #1807366: Make fields translatable by default when enabling translation on a bundle.

3) Translatable titles (and probably bodies?), on by default for content types marked for entity translation.

Agreed. When node properties are multilingual (see above).

Also, probably make entities default to translation on if this module is enabled?

Don't agree. This would make Translate tabs pop up everywhere (ET handles all the entity types). We need at very least more dicussion about this.

A visual indicator on the forms of translatable entities as to which fields would change globally and which fields would change only for this one translation.

Agreed. There already is a planned follow-up for this: #1498724: Introduce a #multilingual key (or similar) for form element.

To sum up I think that there are only a couple of issues left we didn't cover in the previous discussion and IMHO neither of them should block this patch, as both are typical UX improvements that can be handled in the feature freeze phase.

plach’s picture

Issue summary: View changes

Updated issue summary to reflect 1810386 might not be done as a follow up since 1739928 has landed already

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue summary: View changes

Updated issue summary

plach’s picture

Status: Needs work » Needs review
FileSize
115.86 KB
10.75 KB

Here is a patch providing a new help section for ET, fixing @webchick's "fixable" UX concerns and #159:

c43ab87 Issue #1188388: Improved initial setup UX.
cc2cdd0 Issue #1188388: Added dropbuttons to the translations overview.
d0fdcc8 Issue #1188388: Added help entry for Entity Translation.
aa2c0c3 Issue #1188388: Fixed PHP docs to avoid duplicate hook_entity_info() descriptions.
cdd0f98 Issue #1188388: Fixed fatal error when missing Field UI.
plach’s picture

About

PROBLEM: There is no indication on the node page of what language version I am viewing. I see that there is a tiny indicator in the path, but only because I am very carefully observant. ;) IMO the node language should default to displaying here.

We have also #1498880: Theme language switcher for seven theme that should help to make more clear which one is the current active form language.

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue summary: View changes

Updated issue summary.

jhodgdon’s picture

platch asked me to comment in #159 - and the answer is NO, you should not create a new hook_entity_info() documentation function. You should just add the new fields to the existing hook_entity_info() documentation, which is in... wherever it is living these days. Thanks!

plach’s picture

@jhogdon:

Even if the new keys are provided by a module and not by the core entity system?

Bojhan’s picture

Could we please just have a call/irc chat with demo to resolve the remaining Usability bugs, its rather impossible for me to isolate what of webchick's comments are part of general UX WTF's around translation and what is part of this feature request.

jhodgdon’s picture

RE #165/platch - yes I would say so. Someone implementing hook_entity_info() will need to know about these keys. You can make a note in the docs saying that these keys are ignored if the blah blah blah module isn't enabled.

Gábor Hojtsy’s picture

#162: et-ui-1188388-162.patch.patch queued for re-testing.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
plach’s picture

Status: Needs work » Needs review
FileSize
110.92 KB

Here it is.

plach’s picture

Reviewing/fixing my own patch :)

9aa50ca Issue #1188388: Removed unnecessary changes.
93962b9 Issue #1188388: Fixed PHP docs and comments.
509e1cf Issue #1188388: Removed last occurence of variable_get().
+++ b/core/modules/comment/comment.module
@@ -1137,10 +1148,31 @@ function comment_form_node_type_form_alter(&$form, $form_state) {
+ * The comment settings form is embedded

Incomplete PHP doc.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/VocabularyFormController.php
@@ -180,6 +183,11 @@ public function save(array $form, array &$form_state) {
+    $variable_name = 'translation_entity_enabled_taxonomy_term_' . $form_state['values']['machine_name'];
+    if (isset($form_state['values'][$variable_name])) {
+      variable_set($variable_name, $form_state['values'][$variable_name]);
+    }
+

This is not needed anymore.

+++ b/core/modules/translation_entity/translation_entity.admin.inc
@@ -0,0 +1,225 @@
+ * marked as translatable, all the data in $entity->{field_name}[LANGUAGE_NONE]
...
+        // language: we need to move it to LANGUAGE_NONE and drop the other

LANGUAGE_NOT_SPECFIED

+++ b/core/modules/translation_entity/translation_entity.api.php
@@ -0,0 +1,55 @@
+      'entity_translation' => array(

translation_entity

+++ b/core/modules/translation_entity/translation_entity.admin.inc
@@ -0,0 +1,225 @@
+  $limit = variable_get('translation_entity_translatable_batch_limit', 10);

variable_get(), hm

+++ b/core/modules/user/user.admin.inc
@@ -687,6 +698,11 @@ function user_admin_settings_submit($form, &$form_state) {
+
+  // @todo Move this to the config system.
+  if (isset($form_state['values']['translation_entity_enabled_user_user'])) {
+    variable_set('translation_entity_enabled_user_user', $form_state['values']['translation_entity_enabled_user_user']);
+  }

Remove this

+++ b/core/modules/user/user.module
@@ -142,7 +142,7 @@ function user_theme() {
-  return array(
+  $return = array(

@@ -175,8 +178,11 @@ function user_entity_info() {
+
+  return $return;

Unnecessary changes.

fago’s picture

As said, patch looks but I think the hook_entity_info docs still need some clarifications:

+++ b/core/includes/entity.api.php
@@ -141,6 +141,12 @@
+ *   - menu base path: The base menu router path to which the administration
+ *     user interface will be attached. Defaults to ENTITY_TYPE/%ENTITY_TYPE.

This makes one think the system will generate an entity UI, but it doesn't. Best just describe what it is and mark that it is optional. Then the translation controller should clarify it requires that. Same for view and edit.

+++ b/core/modules/translation_entity/translation_entity.api.php
@@ -0,0 +1,55 @@
+ *   to 'array($entity_position)'.
+ */
+function custom_module_entity_info() {

I think we should have this docs at the translation controller interface + add the translation controller class to the hook_entity_info() docs + link the interface from there.

plach’s picture

YesCT’s picture

In #49, step/screenshot 3, there is a suggestion to add hints to the global settings collapsed fieldset. Doing that might help with the pain point (problem) @webchick mentions in #160 step 11:

11) Ok, one last try. In that table under "operations" for English it says "No translatable fields." I go to Structure > Content Types > Article > Manage fields. Click "Edit" on Body. Expected to see "Translatable" option but didn't.

PROBLEM: It actually is there, but is under a collapsed fieldset of "Global settings." Should probably be front-and-center since I decided this content type should be translatable.

Here is the suggestion from 49:
s06-AddHintInCollapsedGlobalFieldSet-2012-10-03_1523.png

Oh, and @plach points out in #161

This is really not something we can address here. Field settings are collapsed by default and translatability is a field setting. The solution is having the proper default in place so that only people having to change it will need fo find the related checkbox (all the migration stuff is going away).

Right, re-reading that reminded me that: when we get the bulk enabling of translation on all translatable fields when translation is enabled on an entity #1807366: Make fields translatable by default when enabling translation on a bundle, then this, not finding the setting buried in the global settings collapsed fieldset, wont be that much of a problem. Although for someone looking to disable translation then, the hint will be helpful in finding the place to do that.

But I think adding the hint to the global settings collapsed fieldset would be a separate issue as I think it effects more than just putting the translation hint there, and really putting information for all the stuff in the collapsed field set there. Or... is it that we need to change something about the global settings collapsed fieldset that allows things to be added to the hint, and then each thing in the field set adds it's own trigger that puts information in there. So then entity translation would have something that adds its own hint. (But, number of values would be added ... by whatever controls that.)

YesCT’s picture

I re-read the comments @webchick had from her run through in #160 and went the steps again using the patch from #173 and it looks like most of the problems are addressed (except the batch processing ones, steps 12 and 13 from #160),

OR would be covered in one of these three follow-ups (which are also in the issue summary)

YesCT’s picture

Issue summary: View changes

Updated issue summary.

YesCT’s picture

Changes to the help.

Some spelling and grammatical changes. Some rewording to better match what is in the ui (like "add" instead of create is couple places). Some re-organization to better match the patterns used on some other help pages; I looked at the taxonomy, node, image, comment help. Even looking at those other pages, the convention for what to use em on to emphasis was not quite clear.

YesCT’s picture

+++ b/core/modules/translation_entity/translation_entity.moduleundefined
@@ -20,17 +20,18 @@ function translation_entity_help($path, $arg) {
       $output .= '<dt>' . t('Maintaining translations') . '</dt>';
-      $output .= '<dd>' . t('If editing content in one language requires that translated versions also be updated to reflect the change, use the <em>Flag other translations as outdated</em> check box to mark the translations as outdated and in need of revision. Individual translations may also be marked for revision by selecting the <em>This translation needs to be updated</em> check box on the translation editing form.') . '</dd>';
+      $output .= '<dd>' . t('If editing content in one language requires that translated versions also be updated to reflect the change, use the <em>Flag other translations as outdated</em> check box to mark the translations as outdated and in need of revision.') . '</dd>';

Because I could not find the check box to mark just one translation as outdated, I took out this sentence from the help: Individual translations may also be marked for revision by selecting the This translation needs to be updated check box on the translation editing form.

Also, more links were added, like the link to the content types admin page and the vocabulary admin page.

And, a sentence was added pointing out that the set of permissions changes on the permission page after translation is enabled on more types of content.

plach’s picture

Issue summary: View changes

Updated issue summary to add #1498674 in with the follow-up's.

plach’s picture

@YesCT:

Thanks! Tonight I'll have a call with Gabor, webchick and Bojhan to identify any pending commit blocker. Hopefully we should go back RTBC again shortly afterwards.

fago’s picture

Ad #173, thanks that looks good to me now.

+++ b/core/includes/entity.api.php
@@ -141,12 +141,20 @@
+ *   - menu base path: Optional. The base menu router path to which the entity

Minor: It should use our standard for (optional) stuff though, e.g.
menu base path: (optional) The base...

plach’s picture

Minor: It should use our standard for (optional) stuff though, e.g.

Great, I guess I looked at the wrong code to get inspiration :)

webchick’s picture

Ok, just had a great call with Gábor, plach, and Bojhan where we walked through the current UI, proposed UI, and the D7 UI of Entity Translation module. Here were our findings:

COMMIT BLOCKERS:
- Clearly designating fields for "only this translation" vs. "all translations" — append (All languages) to the end of global fields.
- Hide source language selector from translation forms.
- Help text: tweaking to make sure that instructions work. (webchick)
- Auto-open global settings fieldset based on whether entity is marked translatable.

Follow-ups related to this patch:
- (major task) Global settings page for all translatable entities, alongside the per-entity ones. - http://drupal.org/node/1810386
- (major task) Auto-enable translatability on every field. - http://drupal.org/node/1807366
- (normal task) Discuss how to view original translation from translation form?
- (normal task) Only show show source translation column if there are > 2 source languages (more than n/a and the original language).
- (normal task) Change dropbutton labels on translations tab to "add translation" / "edt translation"?
- (normal task) Add the "Make field translatable" checkbox on the add field form

Follow-ups we encountered while testing:
- (critical task) Node properties are not translatable (title, status) - http://drupal.org/node/1498674
- (normal task) Notice: Trying to get property of non-object in node_help (line 142 of core/modules/node/node.module)
- (normal task) "Translation is not supported if language is always one of: Not specified, Not applicable, Multiple" reword to: "...."

webchick’s picture

Issue summary: View changes

Updated issue summary.

YesCT’s picture

Issue summary: View changes

Updated issue summary to integrate notes in #181 that resulted from the phone call to identify commit blockers.

YesCT’s picture

I updated the issue summary.

From the phone call, the commit blocker listed in #181

- Clearly designating fields for "only this translation" vs. "all translations" — append (All languages) to the end of global fields.

@plach Can something be implemented to accomplish that without #1498724: Introduce a #multilingual key (or similar) for form element?

YesCT’s picture

Issue summary: View changes

Updated issue summary remaining tasks section.

YesCT’s picture

In #181

- (major task) Global settings page for all translatable entities, alongside the per-entity ones. - http://drupal.org/node/1810386

it lists #1810386: Create workflow to setup multilingual for entity types, bundles and fields as covering that task.

@plach Would #1810386: Create workflow to setup multilingual for entity types, bundles and fields cover a global settings page for all translatable entities? It sounded to be more like an internal code clean up than a ui clean up. But I could be reading it differently.

YesCT’s picture

Fixes for little things I noticed while testing the last patches since #120 and a couple of comments from @webchick. These are not yet listed as follow-up issues and are not commit blockers, so if this is disruptive, these can be ignored for now and redone later.

The changes were:

1: fix lingering "mark all tranlsaitons outdated" to all _other_ translations
et-ui-184-s01-lingering_all_translations-2012-10-27_0317.png

2: link the "no translatable fields" in the translations tab directly to the fields tab (instead of to the entity tab) to make it easier to locate the place to enable translation on a field or fields. I checked and this works for node types, user settings, vocabularies, and comments.
et-ui-184-s02-link_to_fields_directly-2012-10-27_0324.png

3: replace "will trigger batch operation" with a warning "it will effect everywhere field" to address the comment from @webchick in step 12 of #160

12) I expand "Global settings" and click "Enable translation." Says "By submitting this form you will trigger a batch operation."

PROBLEM: Do normal people know what a "batch operation" is? At the very least we should probably tell people something about this, like "This could take a few minutes to process, and in the meantime your content will [what? be inaccessible?]"

I dont think the "will trigger batch operation" wording was there to warn that it was going to do something, for example, to 1034 nodes, because no matter how many entities used that field, or how many pieces of content did, the batch progress bar always said 2 of 2. And I think the real warning was meant to say it would effect other entities, for example, when the body is used on both the article and page content type.
et-ui-184-s03-remove_batch_process_words-2012-10-27_0331.png

4: better success message for disabling or enabling translation to address the comment from @webchick in step 13 of #160

13) I am bold and click Confirm. :P Says it is completing 1 of 2 something, then "Data successfully processed."

PROBLEM: That error message is meaningless to me. What just happened? What data? How was it processed?

"Data successfully processed." changed to "Successfully changed field translation setting." because I think that is more accurate and eliminates the wth? webchick had.
et-ui-184-s04-no_data_successfully_processed-2012-10-27_0334.png

5: "This field is shared among the entity translations." -> "This field has data in existing content". "This field is shared among the entity translations." status in front of the "Enable translation" link in the global field settings was not technically accurate. That message and the link is shown when there is any content with data in the field, instead of the simple checkbox in front of the "Users may translate this field." that is there when there is not content with data in the field. So I think it just means: hey, there is data is that field already and this change is going to effect that. (I almost think that phrase can just be removed completely and just leave the "Enable translation" link there by itself because at the beginning of the global settings fieldset, it already says that there is existing data, and reminds that changes will effect the field everywhere it is used, AND the next page asking for confirmation restates that idea too.)
et-ui-184-s05-shared_to_existing_datat-2012-10-27_0327.png

6: took out the phrase: "because the source content has changed." from the translation settings message for a translation that is outdated, because it could have been marked when any translation was edited, not just the source. so the source itself could have a message that says it is outdated because the source was updated, and that is just weird (and not accurate).
et-ui-184-s06-no_because_source-2012-10-27_0339.png

YesCT’s picture

Status: Needs review » Needs work
+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/EntityTranslationController.phpundefined
@@ -0,0 +1,321 @@
+
+      $translate = !$new_translation && $entity->retranslate[$form_langcode];
+      if (!$translate) {
+        $form['translation']['retranslate'] = array(
+          '#type' => 'checkbox',
+          '#title' => t('Flag other translations as outdated'),
+          '#default_value' => FALSE,
+          '#description' => t('If you made a significant change, which means the other translations should be updated, you can flag all other translations of this content as outdated. This will not change any other property of them, like whether they are published or not.'),
+        );
+      }
+      else {
+        $form['translation']['translate'] = array(
+          '#type' => 'checkbox',
+          '#title' => t('This translation needs to be updated'),
+          '#default_value' => $translate,
+          '#description' => t('When this option is checked, this translation needs to be updated. Uncheck when the translation is up to date again.'),
+        );
+      }
+    }

Does + $translate = !$new_translation && $entity->retranslate[$form_langcode]; mean: it's not a new translation and not already outdated? so it's editing an existing up-to-date translation?

I think $translate is not a specific enough variable name. What is it really representing?

I also suggest adding a comment like
// Editing an existing translation that is not marked outdated
or
// Editing an existing translation that does not need to be updated.

Also, I think section in the else:

else {
+        $form['translation']['translate'] = array(

is meant to show on any content that is not already marked outdated. That would allow any individual translation to be marked outdated without having to edit a different translation, use the "flag all other translations" there to mark something as outdated.

See #177 where the line in the help "Individual translations may also be marked for revision by selecting the This translation needs to be updated check box on the translation editing form." was removed (because there is no way to mark an individual translation as outdated) for another reason to think this.

YesCT’s picture

This is my attempt to allow individual translations to be marked outdated. After reorganizing the blocks that show the checkboxes, I could not get good wording for comments. The logic there seems to be about assumptions, like assuming that when adding a translation, you would not ever want to at the same time, mark other translations as outdated, and if a translation is already flagged outdated, that when editing it, the choice to mark all other translations as outdated should not be available.

I'm not sure if tests need to change, or if an additional test is needed. When I tried to run the et-ui tests locally I got: "An AJAX HTTP error occurred. HTTP Result Code: 500 Debugging information follows. Path: /d8-patch/batch?id=29&op=do_nojs&op=do StatusText: Internal Server Error ResponseText" Maybe it was timing out? I'm making this needs review to see what the bot says. But it actually needs work on the commit blockers.

It adds the line back to the help too.

et-ui-186-s01-allow_individual_translations_outdated-2012-10-27_0538.png

Status: Needs review » Needs work

The last submitted patch, et-ui-1188388-186.patch, failed testing.

YesCT’s picture

Oops in #186 I changed adding a new translation to not have any translation settings, so this puts it back to have the Flag all others as outdated when adding a new translation.
The interdiff to 184 is probably the one that makes sense to look at.

YesCT’s picture

Status: Needs work » Needs review

tests passed locally, trying test bot.

YesCT’s picture

this addresses the commit blocker to hide the language select on translations.
this needs a review to see if I updated the test the correct way to check if something is not there (I actually took out the check for the disabled select and added a check for a disabled, hidden input)

This is based on 184 and skips the allowing individual translations to be marked outdated from #188, but if someone wants that too, just also do:
git apply interdiff-184-186.txt

plach’s picture

This takes care of the commit blockers from #181 and fixes #179. It also includes @YesCT's text improvements from #184. The only thing left out is the help text review that @webchick wants to perform herself. I guess a review of the interdiff should be enough to RTBC this and assign it to Angie.

4d1b325 Issue #1188388: Wording and UX improvements.
2689389 Issue #1188388: Hidden language selector.
7cba2c2 Issue #1188388: Opened the global field settings when field is not translatable.
b67fc7b Issue #1188388: Added a clue about form element translatability.
12ebfa2 Issue #1188388: Fixed notice when preparing an entity for translation.
3b723d7 Issue #1188388: Fixed Entity API docs.

We still need #1498724: Introduce a #multilingual key (or similar) for form element to fix things reliably.

plach’s picture

Issue summary: View changes

Updated issue summary to track minor follow-up for comment style fix needed from #179

cosmicdreams’s picture

Status: Needs review » Reviewed & tested by the community

Manually tested, works great

cosmicdreams’s picture

Status: Reviewed & tested by the community » Needs review

whoops didn't mean to RTBC as this needs Webchick's help text still.

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Assigned: plach » webchick
Status: Needs review » Reviewed & tested by the community

Thanks! I think this might work :)

Edit: Angie, if you decide this is good to go, please refer to the Credits section for the commit message.

YesCT’s picture

1: question about test for hidden language select on translations

I have a confusion about the test; there may not be anything wrong.

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/EntityTranslationController.phpundefined
@@ -174,7 +174,7 @@ public function entityFormAlter(array &$form, array &$form_state, EntityInterfac
     if ($is_translation) {
       if ($language_widget) {
-        $form['langcode']['#disabled'] = TRUE;
+        $form['langcode']['#access'] = FALSE;
       }

hidden using #access FALSE

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Tests/EntityTranslationUITest.phpundefined
@@ -102,12 +102,13 @@ function testTranslationUI() {
     $this->drupalPost($langcode . '/entity-test/manage/' . $entity->id() . '/translations/add/' . $default_langcode . '/' . $langcode, $this->getEditValues($values, $langcode), t('Save'));
-    $this->assertFieldByXPath('//select[@id="edit-langcode" and @disabled]', $default_langcode, 'Language selector correclty disabled on translations.');
+    $this->assertNoFieldByXPath('//select[@id="edit-langcode"]', NULL, 'Language selector correclty disabled on translations.');
+    $this->assertNoRaw('all languages', t('All form elements are translatable.'));
     $entity = entity_test_load($entity->id(), TRUE);

the change to the test that the language select is hidden.. checks if the field (select) is there, but has NULL instead of $default_langcode? That seems weird.

And another question: why is the 'all languages' string paired with the test all form elements are translatable?

2: shared fields getting (all languages) does not work

(all languages) is added to all fields (and the fieldset settings) and not just the shared fields.

New notices when adding a translation (/node/1/translations/add/en/de)

    Notice: Undefined variable: entity_type in translation_entity_form_alter() (line 410 of core/modules/translation_entity/translation_entity.module).
    Notice: Undefined index: field_name in translation_entity_form_alter() (line 411 of core/modules/translation_entity/translation_entity.module).
    Notice: Undefined index: field_name in translation_entity_form_alter() (line 411 of core/modules/translation_entity/translation_entity.module).
  $items['admin/config/regional/translation_entity/translatable/%'] = array(
    'title' => 'Confirm change in translatability.',
    'description' => 'Confirm page for changing field translatability.',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('translation_entity_translatable_form', 5),
    'access arguments' => array('toggle field translatability'),
    'file' => 'translation_entity.admin.inc',
  );

line 140 is the page arguments line.

et-ui-191-s03-addtrans-langselecthidden-2012-10-29_0952.png

3: global settings expanded works

global settings fieldset is expanded when editing a field that is not translatable on a entity that is translatable
not: it's collapsed on a field that is translatable. that makes sense to me (they would know where to go to disable it, ... they went there already to enable it.)

http://drupal.org/files/et-ui-191-s01a-globalexpanded-2012-10-29_0945_0.png
http://drupal.org/files/et-ui-191-s01b-globalexpanded-2012-10-29_0946_0.png
http://drupal.org/files/et-ui-191-s02-globalcolapsedwhenfieldtransenable...
(those are long, and nothing really that interesting to see, so I didn't embed them.)

YesCT’s picture

Issue summary: View changes

Updated issue summary.

YesCT’s picture

On more thinking, it might be ok that (all languages) is appended to the vertical tab fieldsets. As changing those settings will effect the entity across all translations.

Please would someone verify they have the same notices as I got though, and that (all languages) should not be, for example, on the body field.

plach’s picture

Status: Needs work » Needs review
FileSize
115.81 KB
773 bytes

Oops. I vaguely remember of having moved that code from elsewhere just before posting the patch. I guess I should have tested also the final iteration. Easy fix.

hidden using #access FALSE

This is the correct way to hide a form element without manipulating or unsetting it. Other pieces of code might expect it to be unchanged to work correctly.

the change to the test that the language select is hidden.. checks if the field (select) is there, but has NULL instead of $default_langcode? That seems weird.

It's checking that the language selector is not there. No point in specifying a value.

And another question: why is the 'all languages' string paired with the test all form elements are translatable?

It's checking that there is no 'all languages' string in the form. If this is true it means that every form element is translatable (actually multilingual).

On more thinking, it might be ok that (all languages) is appended to the vertical tab fieldsets. As changing those settings will effect the entity across all translations.

Yes, this is by design.

YesCT’s picture

It's checking that there is no 'all languages' string in the form. If this is true it means that every form element is translatable (actually multilingual).

ah. so, to deal with the recent commit blocker to add all languages to the label of fields that are shared, 'all languages' is added to fields that are shared across all translations. So if there is no 'all languages' then every element is translatable. And every element would mean all the settings too...

Does it do something special if every element is translatable?

I could look at that by looking at the test entity, right?

YesCT’s picture

here is a screen shot showing the latest patch from #197 working for the (all languages) addition to field labels
et-ui-197-s01-allLanguages-2012-10-29_2239.png

YesCT’s picture

Issue summary: View changes

Updated issue summary with status of commit blocker: webchick review instructions and marking shared fields (all languages) still in progress.

YesCT’s picture

Issue summary: View changes

Updated issue summary to give comment numbers which contain updated screenshots.

YesCT’s picture

Here are some screenshots of the test entity (for completeness) This all seems to be working fine.

et-ui-197-s02-testEntityEdit-2012-10-29_2324.png

et-ui-197-s03-testEntityTranslationTab-2012-10-29_2334.png

et-ui-197-s04-editHasLangSelect-2012-10-29_2342.png

et-ui-197-s05-testEntitySelectHidden-2012-10-29_2349.png

YesCT’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityFormController.phpundefined
@@ -234,6 +235,49 @@ public function getFormLangcode(array $form_state) {
+  /**
+   * Handle possible entity language changes.
+   */
+  protected function submitEntityLanguage(array $form, array &$form_state) {

Does this function need a fuller documentation block with @param?
There are quite a few functions without @param's...

I looked at http://drupal.org/core-gates#documentation-block-requirements and http://drupal.org/node/1354 and I think it might.
I also looked elsewhere in core, and I see sometimes functions with $form and $form_state do list out the @param (like line 187 in core/lib/Drupal/Core/Entity/EntityFormController.php) and sometimes just a one line simple doc block is used (like line 119)

YesCT’s picture

In #176 I asked about help page guidelines. I found them http://drupal.org/node/632280

YesCT’s picture

Issue summary: View changes

Updated issue summary to reflect "all languages" commit blocker no longer a remaining task.

YesCT’s picture

Issue summary: View changes

Updated issue summary with comment number that has test entity screenshots that result from recent patch

Berdir’s picture

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/EntityTranslationController.phpundefined
@@ -0,0 +1,397 @@
+  /**
+   * Implements EntityTranslationControllerInterface::getTranslationAccess().
+   */
+  public function getTranslationAccess(EntityInterface $entity, $langcode) {
+    $entity_type = $entity->entityType();
+    return (user_access('translate any entity') || user_access("translate $entity_type entities")) && ($langcode != $entity->language()->langcode || user_access('edit original values'));

I'm not really convinced about the edit original values permission stuff, that complicates things and is confusing.

It just confused the hell out of a site builder on a 7.x project in my office right now (and me, who had to figure this out ;)), because this doesn't even respect the bypass all node access permission.

I'm fine with discussing the details in a follow-up, but this is IMHO quite important and should work better out of the box. Why should just enabling a *translation UI* take away all your edit permissions? The current content translation module isn't doing anything like this either? So it should at least be an optional feature, that is disabled by default.

Maybe we can at least add an override to this function to respect the bypass permission for nodes for now?

plach’s picture

It just confused the hell out of a site builder on a 7.x project in my office right now (and me, who had to figure this out ;)), because this doesn't even respect the bypass all node access permission.

This is clearly an oversight :)

I'm fine with discussing the details in a follow-up, but this is IMHO quite important and should work better out of the box. Why should just enabling a *translation UI* take away all your edit permissions? The current content translation module isn't doing anything like this either? So it should at least be an optional feature, that is disabled by default.

In D7 we have a hook_enable() implementation, which is probably not working correctly AFAICS, that should automatically grant the needed permissions to roles already being able to edit the translatable entities. OTOH I'm not sure there is another way to allow users to edit only the translated values.

Here's the follow-up if we want to start discussing the details: #1807776: Support both simple and editorial workflows for translating entities.

plach’s picture

@Bojhan:

We are having a product-oriented discussion in #1807776: Support both simple and editorial workflows for translating entities: it would be great if you could chime-in.

Berdir’s picture

EFQ v2 is in, so I'm quite sure this needs to be updated, will trigger a re-test.

Berdir’s picture

Status: Needs review » Needs work
Issue tags: +JavaScript, +Usability, +translatable fields, +D8MI, +sprint, +language-content

The last submitted patch, et-ui-1188388-197.patch, failed testing.

YesCT’s picture

Issue summary: View changes

Updated issue summary to reflect screenshots have been posted to reflect commit blocker fixes.

YesCT’s picture

Here is the link to the issue mentioned in #206 #1801726: EntityFieldQuery v2
I've updated the issue summary remaining tasks...
[edit: added the actual link I meant to link]

plach’s picture

Will reroll this tonight.

plach’s picture

Status: Needs work » Needs review
Issue tags: +Avoid commit conflicts
FileSize
120.63 KB
6.11 KB

This was awful :(

Had to reroll entity types as plugins + EFQ2 (and found a bug in the count logic). I cannot provide a test for it right now, and it should probably be a separate issue. Please let's open a follow-up for that otherwise this would be postponed on that one. The interdiff covers only the EFQ adjustment as the other stuff should be pretty straightforward.

Also please let's postpone also further code cleanups/documentation for a follow-up as we did for EFQ2.

Status: Needs review » Needs work
Issue tags: -JavaScript, -Usability, -translatable fields, -D8MI, -sprint, -language-content, -Avoid commit conflicts

The last submitted patch, et-ui-1188388-211.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review

#211: et-ui-1188388-211.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +JavaScript, +Usability, +translatable fields, +D8MI, +sprint, +language-content, +Avoid commit conflicts

The last submitted patch, et-ui-1188388-211.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
120.6 KB
18.86 KB

I reverted the EFQ changes as I was able to get around those so this does not depend on that bug fix. PHP docs should be fixed either. Hopefully we are good to go now.

d91d2b9 Issue #1188388: Fixed Entity Query tests.
c1d865e Issue #1188388: Improved/fixed PHP docs.
plach’s picture

Status: Needs review » Needs work
Issue tags: -JavaScript, -Usability, -translatable fields, -D8MI, -sprint, -language-content, -Avoid commit conflicts

The last submitted patch, et-ui-1188388-215.patch, failed testing.

webchick’s picture

Status: Needs work » Needs review
Issue tags: +JavaScript, +Usability, +translatable fields, +D8MI, +sprint, +language-content, +Avoid commit conflicts

#215: et-ui-1188388-215.patch queued for re-testing.

webchick’s picture

Assigned: webchick » Unassigned
Status: Needs review » Needs work

Ok! Sorry about the delay, here is the help text that YesCT and I came up with tonight at BADCamp.

Main changes are around formatting, largely, but we did catch a couple of bugs with the text which are now fixed, so this should be accurate based on the current state of the patch.

Another follow-up: Let's please rename this module to just "Translation" after. ;) (This was YesCT's suggestion and I think it's a great one.)

About

The Entity Translation module allows you to create and manage translations for your Drupal site content. You can specify which elements need to be translated at the content-type level for content items and comments, at the vocabulary level for taxonomy terms, and at the site level for user accounts. Other modules may provide additional elements that can be translated. For more information, see the online handbook entry for Entity Translation.

Uses

Enabling translation

Before you can translate content, there must be at least two non-system languages added on the <a href="admin/sssss">languages admnistration</a> page.

Before you can translate content <a href="">add another language</a> so that at least two languages are enabled.

After adding languages, enable translation for any content you wish to translate:

<ul>
<li><strong>Content types</strong>: Enable translation of <a href="">content types</a>, by clicking edit next to the appropriate type, and under Language settings, uncheck <em>Hide language selector</em> and check <em>Enable translation</em>. This allows selecting the language as part of creating or editing the content.</li>
</li><strong>Comments</strong>: switch to the Comment settings in the content type edit page and check Enable translation.</li>
<li><strong>Taxonomy terms</strong>: Enable translation of <a href="">Taxonomy</a> terms by editing the Vocabulary and checking <em>Enable translation</em> under Terms language.</li>
<li><strong>User accounts</strong>: Enable translation of user accounts in the <a href="">account settings page</a>, by checking <em>Enable translation</em> under Language settings.</li>
</ul>

Finally, under the <em>Manage fields</em> tab, <em>edit</em> each field you wish to be translatable, and enable or disable translation under <em>Global settings</em>.

Translating content

After enabling translation you can create a new piece of content, or edit existing content and assign it a language. Once this is done you will see a Translations tab or link that will give you access to an overview of the translation status for the current content. From there, you can add translations and edit or delete existing translations. This process is similar for every translatable element on your site, such as taxonomy terms, comments or user accounts.

Changing source language

When there are two or more possible source languages, selecting a <em>Source language</em> will repopulate the form using the specified source's values. For example,  French is much closer to Spanish than to Chinese, so changing the French translation's source language to Spanish can assist translators.

Maintaining translations

If editing content in one language requires that translated versions also be updated to reflect the change, use the Flag other translations as outdated check box to mark the translations as outdated and in need of revision.

Translation permissions

The Entity Translation module makes a basic set of permissions available. Additional <a href="">permissions</a> are made available after translation is enabled for each content type.


webchick’s picture

Status: Needs work » Needs review

Cross-posting with myself. :P

YesCT’s picture

I'm working on #201 (I checked with webchick and she verified that the @param, etc needs to be done and #219

plach’s picture

@YestCT:

I may have missing something but the lastest patch should already introduce @params everywhere they are needed (i.e. excluding hook and interface implementations where they are well-known and already documented).

plach’s picture

Assigned: Unassigned » plach

Working on the help.

plach’s picture

Ok, here is the help from #219. I changed only the last two words :)

The Entity Translation module makes a basic set of permissions available. Additional <a href="">permissions</a> are made available after translation is enabled for each content type.

became

The Entity Translation module makes a basic set of permissions available. Additional <a href="">permissions</a> are made available after translation is enabled for each translatable element.

since permissions have entity type and not content type granularity.

Anyone here bold enough to mark this RTBC again?

plach’s picture

And this is /me failing at life :(

YesCT’s picture

Assigned: plach » Unassigned
FileSize
6.94 KB
120.92 KB

This is just the help changes. Mostly the ones from #219

YesCT’s picture

+++ b/core/modules/translation_entity/translation_entity.moduleundefined
@@ -22,16 +22,24 @@ function translation_entity_help($path, $arg) {
-      $output .= '<dd>' . t('Enabling the Entity Translation module makes a basic set of permissions available. Additional <a href="@permissions">permissions</a> are made available after translation is enabled for each type of content.', array('@permissions' => url('admin/people/permissions#module-translation_entity'))) . '</dd>';
+      $output .= '<dd>' . t('The Entity Translation module makes a basic set of permissions available. Additional <a href="@permissions">permissions</a> are made available after translation is enabled for each type of content.', array('@permissions' => url('admin/people/permissions#module-translation_entity'))) . '</dd>';

I left "each type of content" instead of the change that webchick and I thought we would want, because: it's not after translation is enabled on each "Content type" it's after. It's after it's enabled on nodes, but also after enabled for taxonomy, etc so those are "types of content" instead of "Content types". :P

In general, I think these instructions are accurate and good enough for now. There can be a follow up later if needed to fix capitalization or grammer.

YesCT’s picture

crud. cross posts.

YesCT’s picture

Status: Needs review » Needs work

This is the list of functions (See #201.)

+++ b/core/modules/comment/comment.moduleundefined
@@ -1106,10 +1116,33 @@ function comment_form_node_type_form_alter(&$form, $form_state) {
+ * Comment translation settings submit handler.
+ */
+function comment_translation_configuration_element_submit($form, &$form_state) {

+++ b/core/modules/system/tests/modules/entity_test/entity_test.moduleundefined
@@ -69,8 +72,8 @@ function entity_test_add() {
  * Menu callback: displays the 'Edit existing entity_test' form.
  */
...
+function entity_test_edit(EntityTest $entity) {

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermTranslationController.phpundefined
@@ -0,0 +1,38 @@
+   * Submit handler for the save action.
+   */
+  function entityFormSave(array $form, array &$form_state) {

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/EntityTranslationController.phpundefined
@@ -0,0 +1,400 @@
+   * Process callback that handles entity form shared elements.
+   */
+  public function entityFormSharedElements($element) {
...
+   * Entity builder method.
+   */
+  public function entityFormEntityBuild($entity_type, $entity, $form, &$form_state) {
...
+   * Submit handler for the source language change.
+   */
+  public function entityFormSourceChange($form, &$form_state) {
...
+   * Submit handler for the entity deletion.
+   */
+  function entityFormDelete($form, &$form_state) {
...
+   * Submit handler for the entity translation deletion.
+   */
+  function entityFormDeleteTranslation($form, &$form_state) {

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Tests/EntityTranslationUITest.phpundefined
@@ -0,0 +1,191 @@
+   * Returns an edit array containing the values to be posted.
+   */
+  protected function getEditValues($values, $langcode, $new = FALSE) {

+++ b/core/modules/translation_entity/translation_entity.admin.incundefined
@@ -0,0 +1,252 @@
+ * Returns the confirmation form for changing field translatability.
+ */
+function translation_entity_translatable_form(array $form, array &$form_state, $field_name) {
...
+ * Submit handler for the field settings form.
+ *
+ * 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_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.
+ */
+function translation_entity_translatable_form_submit(array $form, array $form_state) {
...
+ * Toggles translatability of the given field.
+ *
+ * This is called from a batch operation, but should only run once per field.
+ */
+function translation_entity_translatable_switch($translatable, $field_name) {
...
+ * Converts field data to or from LANGUAGE_NOT_SPECIFIED.
+ */
+function translation_entity_translatable_batch($translatable, $field_name, &$context) {
...
+ * Stores the given field translations.
+ */
+function _translation_entity_update_field($entity_type, $entity, $field_name) {
...
+ * Checks the exit status of the batch operation.
+ */
+function translation_entity_translatable_batch_done($success, $results, $operations) {

+++ b/core/modules/translation_entity/translation_entity.moduleundefined
@@ -0,0 +1,692 @@
+ * Process handler for the language_configuration form element.
+ */
+function translation_entity_language_configuration_element_process($element, array &$form_state, array &$form) {
...
+ * Configuration element validation handler.
+ *
+ * Checks whether translation can be enabled: if language is set to one of the
+ * special languages and language selector is not hidden, translation cannot be
+ * enabled.
+ */
+function translation_entity_language_configuration_element_validate($element, array &$form_state, array $form) {
...
+ * Configuration form submit handler.
+ *
+ * Stores the entity translation settings.
+ */
+function translation_entity_language_configuration_element_submit(array $form, array &$form_state) {

+++ b/core/modules/translation_entity/translation_entity.pages.incundefined
@@ -0,0 +1,272 @@
+ * Form builder for the translation deletion confirmation.
+ */
+function translation_entity_delete_confirm(array $form, array $form_state, EntityInterface $entity, Language $language) {
...
+ * Submit handler for the translation deletion confirmation.
+ */
+function translation_entity_delete_confirm_submit(array $form, array &$form_state) {

+++ b/core/modules/user/lib/Drupal/user/ProfileTranslationController.phpundefined
@@ -0,0 +1,38 @@
+   * Submit handler for the save action.
+   */
+  function entityFormSave(array $form, array &$form_state) {

need @param, some need @return too.

bforchhammer’s picture

Assigned: Unassigned » plach
Status: Needs work » Needs review

I read through parts of the patch (#225) and stumbled on a few documentation issues (typos etc.). Nothing major, but should be easy to fix... :)

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/EntityTranslationControllerInterface.php
@@ -0,0 +1,188 @@
+ * otherwise the 'menu_view_path' and 'menu_edit_path' key must be defined. If

keyS (plural)?

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/EntityTranslationControllerInterface.php
@@ -0,0 +1,188 @@
+ *   function mymodule_entity_info_alter(array &info) {

Missing $ in front of variable name

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/EntityTranslationControllerInterface.php
@@ -0,0 +1,188 @@
+ *       'menu_path_widlcard' => '%my_entity_loader',

menu_path_wildcard - l and d swapped.

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Tests/EntityTranslationUITest.php
@@ -0,0 +1,191 @@
+        // @todo Remove this workaround when field default language is correctly
+        // handled.

Would it be a good idea to add links to the respective follow-up issues for @todos in the code? (There are 28 @todo statements in the code overall).

+++ b/core/modules/translation_entity/translation_entity.admin.inc
@@ -0,0 +1,252 @@
+      // We need a two-steps approach while updating field translations: given

two-step (no s)

+++ b/core/modules/translation_entity/translation_entity.module
@@ -0,0 +1,692 @@
+      $output .= '<ul><li>' . t('<strong>Content types</strong>: Enable translation of <a href="@content_types_url">content types</a>, by clicking edit next to the appropriate type, and under Language settings, uncheck <em>Hide language selector</em> and check <em>Enable translation</em>. This allows selecting the language as part of creating or editing the content.', array('@content_types_url' => url('admin/structure/types'))) . '</li>';
+      $output .= '<li>' . t('<strong>Comments</strong>: switch to the Comment settings in the content type edit page and check Enable translation.</li>');
+      $output .= '<li>' . t('<strong>Taxonomy terms</strong>: Enable translation of <a href="@vocabularies_url">taxonomy</a> terms by editing the Vocabulary and checking <em>Enable translation</em> under Terms language.', array('@vocabularies_url' => url('admin/structure/taxonomy'))) . '</li>';
+      $output .= '<li>' . t('<strong>User accounts</strong>: Enable translation of user accounts in the <a href="@account_url">account settings page</a>, by checking <em>Enable translation</em> under Language settings.', array('@account_url' => url('admin/config/people/accounts'))) . '</li></ul>';
+      $output .= '<p>' . t('Finally, under the <em>Manage fields</em> tab, <em>edit</em> each field you wish to be translatable, and enable or disable translation under <em>Global settings</em>.') . '</p></dd>';

Use of em-tags is a bit inconsistent in this part. For example "Enable translation" is not always emphasized; similar for names of tabs or sections, e.g. "Language settings". Why is "edit" emphasized in the last bit?

+++ b/core/modules/translation_entity/translation_entity.module
@@ -0,0 +1,692 @@
+      $output .= '<dd>' . t('When there are two or more possible source languages, selecting a <em>Source language</em> will repopulate the form using the specified source\'s values. For example,  French is much closer to Spanish than to Chinese, so changing the French translation\'s source language to Spanish can assist translators.') . '</dd>';

Double space after "For example,"

+++ b/core/modules/translation_entity/translation_entity.module
@@ -0,0 +1,692 @@
+      $output .= '<dd>' . t('If editing content in one language requires that translated versions also be updated to reflect the change, use the Flag other translations as outdated check box to mark the translations as outdated and in need of revision.') . '</dd>';

Maybe emphasize checkbox label to make the sentence easier to read?

+++ b/core/modules/user/lib/Drupal/user/ProfileTranslationController.php
@@ -0,0 +1,38 @@
+      // since the curret URL would be preserved and we would try to add a

missing "n" in "current"

bforchhammer’s picture

Sorry, cross-post.

YesCT’s picture

Assigned: plach » Unassigned
Status: Needs review » Needs work
FileSize
4.75 KB
120.89 KB

Here is a patch to deal with the help text cross post. (so #226 can be ignored)

YesCT’s picture

OK. I re-re-re-read 1354 and it's under forms, that is says @param and @ return is not needed for form submit handlers, but it says should have @see. verified by fago and xjm.

plach is busy or a bit (few hours) @bforchhammer had some good catches.

I'm going to work on a new patch for the @see and other little stuff.

YesCT’s picture

this encorporates #230 and a sample of adding @see to submit handler doc stuff.
I'll work on the rest of the docs next.

YesCT’s picture

+++ b/core/modules/comment/comment.moduleundefined
@@ -1126,7 +1126,13 @@ function comment_form_node_type_form_alter(&$form, $form_state) {
 /**
- * Comment translation settings submit handler.
+ * Form submission handler for node_type_form().
+ *
+ * This handles the comment translation settings added by
+ * comment_form_node_type_form_alter().
+ *
+ * @see comment_form_node_type_form_alter()
+ * @see node_type_form()
  */

Here is a go at docs for submit handlers. tim.plunkett helped me with this.

Tor Arne Thune’s picture

Looks good, but there is no need for the @see to node_type_form(), as the summary "Form submission handler for node_type_form()" takes care of that. It's a bit hard to read from 1354 that this is the the way to do it, but the example under "Documenting form-generating functions" explains it better than the sentence further below in the same section: "You may add @see directives to link to correlating validation and submission handlers, and vice-versa."

The API module used on api.drupal.org creates a link to node_type_form() in the summary, so the @see would be superfluous.

Gábor Hojtsy’s picture

@webchick, @YesCT: RE "Let's please rename this module to just "Translation" after." I think this is indeed best to discuss elsewhere. It can easily and quickly become confusing (eg. this module does not translate lots of things, eg. the user interface of core). I think this is an age old discussion, so anyway, could be done later :)

Gábor Hojtsy’s picture

Issue tags: +feature freeze

Add feature freeze tag.

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary since remaining tasks now includes rerolling for EFQ v2

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Issue tags: -feature freeze

Sending in for a testbot tour.

Gábor Hojtsy’s picture

Issue tags: +feature freeze

Cross-posted myself, that is hardcode. .'–/

YesCT’s picture

docs stuff done with help from Gabor.
Also an interdiff back to plach's last patch for the sandbox. Plach has something to add too after incorporating this.

This needs testing and a look at the code, if that looks alright -> RTBC.

plach’s picture

plach’s picture

FileSize
123.29 KB

ehm :)

YesCT’s picture

The test bot is green.
Commit blockers are done (webchick looked at the help, the docs are done)
I've been manually testing and everything looks great.... except. Well, could someone please try and translate a user?

YesCT’s picture

Issue summary: View changes

Updated credits

falcon03’s picture

Did someone review this UI for its accessibility?

If not, I could open a critical follow-up issue to get this done.

YesCT’s picture

A checkout of D8 from around Oct 30 and a patch from around then work for user translation. Here are the steps to setting up that that I did:
sudo rm -r sites
git checkout sites
git checkout 8.x
git log
git checkout a598aa4654e21c
git apply --index et-ui-1188388-197.patch
git status
drush -y si --account-name=admin --account-pass=admin --db-url="mysql://root:root@localhost/d8-patch" --site-name=d8-patch-et-ui-1188388-197

With a current pull from D8 and the most recent patch the behavior I'm seeing is that after enabling translation in the user account settings, adding a text field, and enabling translation of that text field: I do get a translations tab on a user. But the title is n/a for both languages in the translation tab, and none of the languages are bold (to indicate which is *the* language of *the* user). [Edit: #133/2 show same. So that much has been around for a while.] If I edit the user, put something in the text field and then save the user, then there is an entry on the translations tab. And another strange thing: if I then add a translation of that user, the add translation page/form only shows the field that is translatable. It does not show the fields that are shared across languages. Also weird: if I go ahead and put something in the field and save it, I return to the edit page for the user (instead of viewing the user) and the translations tab does not reflect a translation existing.

YesCT’s picture

@falcon3 We have a ui review done for now, as noted in the issue summary (under remaining tasks, ui review is done). There are several follow up issues and remaining tasks that will address some things that ui reviews have brought up. So we dont need a ui review right now, but we will once we start tackling the follow up issues.

However, it would help if you could try and translate a user and see if you can duplicate what I described in #246

YesCT’s picture

If I make a new user (after translation is all set up for user accounts an add a field and enable translation on that field), then the translation tab on that user starts better.
et-ui-243-newuser-transtab-2012-11-02_0728.png

But when clicking on add to add a translation from the translation tab, I see just the translatable field and the notice:

Notice: Undefined index: en in translation_entity_prepare_translation() (line 231 of core/modules/translation_entity/translation_entity.pages.inc).

It does not show the fields that are shared across languages. Also weird: if I go ahead and put something in the field and save it, I return to the edit page for the user (instead of viewing the user) with the notice:
Notice: Undefined index: en in translation_entity_prepare_translation() (line 231 of core/modules/translation_entity/translation_entity.pages.inc).
and the translations tab does not reflect a translation existing.

things are a little better if I edit the user and fill in some value in the translatable field (didn't do that when creating the user because the translatable field was not shown on the create user form).
et-ui-243-s02-2012-11-02_0736.png
If there is some value in the translatable field for the user, then the notices go away when adding a translation, but it's still just the translatable field shown on the add translation form
et-ui-243-s03-justTransFieldOnForm-2012-11-02_0747.png
and there is no new translation added.

penyaskito’s picture

Assigned: Unassigned » penyaskito

Reproduced every problem commented at #246 and #248.

URL redirect after translation is an easy one, just check Drupal\user\ProfileTranslationController::entityFormSave.

Looking at why the rest of the user fields are not there.

penyaskito’s picture

The problem with having only the fields at the entity translation is because the operation that entity_form_controller gets is "default". However, the $entity_info->default_operation is currently 'profile', but that is not taken into account.

penyaskito’s picture

Fixed the redirect and the fields shown at the form.

plach’s picture

Assigned: Unassigned » plach

Working on improved tests.

Berdir’s picture

The code looks very clean and well documented. Much better than when I last checked :)

+++ b/core/lib/Drupal/Core/Entity/EntityManager.phpundefined
@@ -59,6 +59,10 @@
  * - render_controller_class: The name of the class that is used to render the
  *   entities. Defaults to Drupal\Core\Entity\EntityRenderController.
+ * - translation_controller_class: (optional) The name of the translation
+ *   controller class that should be used to handle the translation process.
+ *   See Drupal\translation_entity\EntityTranslationControllerInterface for more

I think extending another hook/info/discovery thingy is a bit problematic, we can cheat and extend the original documentation, but contrib modules would not be able to do that. Don't have a better solution.

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/EntityTranslationController.phpundefined
@@ -0,0 +1,421 @@
+class EntityTranslationController implements EntityTranslationControllerInterface {
+
+  protected $entityType;
+  protected $entityInfo;

These should have a simple docblock.

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/EntityTranslationControllerInterface.phpundefined
@@ -0,0 +1,188 @@
+ * This is how entity info would look for a module defining a new translatable
+ * entity type:
+ * @code
+ *   function mymodule_entity_info_alter(array &$info) {
+ *     $info['myentity'] += array(
+ *       'menu_base_path' => 'mymodule/myentity/%my_entity_loader',
+ *       'menu_path_wildcard' => '%my_entity_loader',
+ *       'translation_controller_class' => 'Drupal\mymodule\MyEntityTranslationController',
+ *       'translation' => array(

Wondering how much of this is really necessary given that parts of this have also been added to EntityManager. Can we maybe add a @see for that class?

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/EntityTranslationControllerInterface.phpundefined
@@ -0,0 +1,188 @@
+   * Returns the base path for the current entity.
+   *
+   * @param Drupal\Core\Entity\EntityInterface $entity
+   *   The entity to the path should refer to.

fully qualified types should now start with a \.

Not blocking IMHO, but a search & replace on "@param Drupal" and "@return Drupal" when you do a re-roll anyway should find most of them.

If @file should include the namespace is not officially decided yet, see http://drupal.org/node/1487760#comment-6681222. So we should probably just leave it as it is for now.

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/EntityTranslationControllerInterface.phpundefined
@@ -0,0 +1,188 @@
+  /**
+   * Checks if the user can perform the given operation on the wrapped entity.
+   *
+   * @param Drupal\Core\Entity\EntityInterface $entity
+   *   The entity access should be checked for.
+   * @param string $op
+   *   The operation to be performed. Possible values are:
+   *   - "view"
+   *   - "update"
+   *   - "delete"
+   *   - "create"
+   *
+   * @return
+   *   TRUE if the user is allowed to perform the given operation, FALSE
+   *   otherwise.
+   */
+  public function getAccess(EntityInterface $entity, $op);

#1696660: Add an entity access API for single entity access is relatively close to being commited, was RTBC already once or twice. Is there already a follow-up to replace this with that?

+++ b/core/modules/translation_entity/translation_entity.admin.incundefined
@@ -0,0 +1,262 @@
+ *   Field machine name.
+ */
+function translation_entity_translatable_batch($translatable, $field_name, &$context) {
+  $entity_types = array();

That's quite a crazy batch function. I'm wondering if it would be easier to follow to do if we'd call it once for each entity type.

Can be a follow-up refactoring. See also #1797526: Make batch a wrapper around the queue system.

Berdir’s picture

Issue summary: View changes

Updated issue summary. took out a mention to verifying help by webchick that was under the documentation remaining tasks, and that task is done.

YesCT’s picture

Status: Needs review » Needs work

just updating the status since plach is actively working on a new patch with the tests. :)

YesCT’s picture

Issue summary: View changes

Updated issue summary.

YesCT’s picture

steps to reproduce

  • Enable entity translation
  • Add a language
  • Add a user
  • Enable translation for users (user account settings)
  • Add a text field to users
  • Enable translation on the text field
  • Add a translation of the user

Gives:
Notice: Undefined index: en in translation_entity_prepare_translation() (line 222 of core/modules/translation_entity/translation_entity.pages.inc).
(same notice just code moved around so it's a different line number)

YesCT’s picture

steps to reproduce on something that is not a user: page content type

  • Create a page. Fill out required fields (just title, not body)
  • Edit page content type. Enable translation. Uncheck hide language select.
  • Edit page content type. Edit body. Enable translation on field.
  • Edit page content. Go to translation tab. Add a translation

Get the notice:

Notice: Undefined index: en in translation_entity_prepare_translation() (line 222 of core/modules/translation_entity/translation_entity.pages.inc).

This should make thinking about the error easier.
Also note if go past the notice and save the translation, the translation is not created.

YesCT’s picture

@plach the patch with the tests did not apply to the clone of the sandbox. So if you can push your changes to the sandbox, or send me a new patch that applies to git clone --recursive --branch 8.x-et-ui-1188388-plach http://git.drupal.org/sandbox/plach/1719670.git d8mi___entity_translation_ui
that would be super [edit: you could push to another branch if you want to keep that one clean]

plach’s picture

Here we go!

Here's a new patch fixing all the small buggy stuff identified above plus #253 and a couple of bonus. It also provides a translation test case for every supported entity. They all extend a base test class and slightly tweak its behavior to adapt it to the various entity-specific cases. I didn't use compatibility mode to ensure everything works also with NG entities.

Attached you can find:

  • the main patch
  • a patch without @penyaskito's fix (should fail user tests)
  • a global interdiff
  • an interdiff with only code for the new tests

@Berdir:

I think extending another hook/info/discovery thingy is a bit problematic, we can cheat and extend the original documentation, but contrib modules would not be able to do that. Don't have a better solution.

If I were to do this in contrib right now I'd just use hook_info_alter(), I guess. However since this would make providing defaults harder we should consider investigating a way to augment plugin annotations before altering them.

#1696660: Add an entity access API for single entity access is relatively close to being commited, was RTBC already once or twice. Is there already a follow-up to replace this with that?

Yes, see the OP.

That's quite a crazy batch function. I'm wondering if it would be easier to follow to do if we'd call it once for each entity type.

That code will all go away when Field API language behavior mirrors Entity Field language one.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Changes look good. Although the testbot feedback did not came back to this page, clicking the "view details" for the nofix and patch shows the original fail and the fix passing proper. Extra thanks for all the new tests! Super huge appreciation for your dedication to this issue.

YesCT’s picture

I reviewed the new stuff. The new tests are great, very readable. I checked them for api docs, and basic code readability. I also manually test most things and translation of users, terms, nodes, comments is working.

The bot agrees it's good.

plach’s picture

@committers:

Please refer to the credits section in the OP for the commit message :)

catch’s picture

Status: Reviewed & tested by the community » Needs work

I don't have time to properly review this at the moment, but Gabor pinged me about it so I've taken a quick look through. Seems like webchick's already looked like this a fair bit.

A few things I noticed that looked a bit odd. The only one that's not minor is the hook_menu()/hook_menu_alter() stuff since I can't understand what it's doing at all. Leaving RTBC so Angie sees it, and we can open follow-ups for this stuff if she's happy committing (I'm not able to see if all her feedback was dealt with at the moment so won't commit this evening).

+++ b/core/modules/comment/comment.moduleundefined
@@ -1009,6 +1009,16 @@ function comment_links(Comment $comment, Node $node) {
+  // Add translations link for translation-enabled comment bundles.
+  if (module_exists('translation_entity') && translation_entity_translate_access($comment)) {
+    $links['comment-translations'] = array(
+      'title' => t('translations'),
+      'href' => 'comment/' . $comment->id() . '/translations',
+      'html' => TRUE,
+    );

I'd expect to see a separate check for whether comments (or the comment bundle most likely) are translation enabled. Why rely on the access callback?

+++ b/core/modules/node/node.jsundefined
@@ -42,6 +42,21 @@ Drupal.behaviors.nodeFieldsetSummaries = {
+
+    $context.find('fieldset.node-translation-options').drupalSetSummary(function (context) {
+      var translate;
+      var $checkbox = $context.find('.form-item-translation-translate input');
+
+      if ($checkbox.size()) {
+        translate = $checkbox.is(':checked') ? Drupal.t('Needs to be updated') : Drupal.t('Does not need to be updated');
+      }
+      else {
+        $checkbox = $context.find('.form-item-translation-retranslate input');
+        translate = $checkbox.is(':checked') ? Drupal.t('Flag other translations as outdated') : Drupal.t('Do not flag other translations as outdated');
+      }
+
+      return translate;
+    });
   }

This feels like something the entity translation module could provide a function for? Or is it very, very specific to node module?

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermTranslationController.phpundefined
@@ -0,0 +1,42 @@
+  function entityFormSave(array $form, array &$form_state) {
+    if ($this->getSourceLangcode($form_state)) {
+      $entity = translation_entity_form_controller($form_state)->getEntity($form_state);
+      // We need a redirect here, otherwise we would get an access denied page
+      // since the curret URL would be preserved and we would try to add a
+      // translation for a language that already has a translation.
+      $form_state['redirect'] = $this->getEditPath($entity);

s/curret/current

+++ b/core/modules/translation_entity/translation_entity.moduleundefined
@@ -0,0 +1,702 @@
+/**
+ * Implements hook_menu_alter().
+ */
+function translation_entity_menu_alter(array &$items) {
+  // Some menu loaders in the item paths might have been altered: we need to
+  // replace any menu loader with a plain % to check if base paths are still
+  // compatible.
+  $paths = array();
+  $regex = '|%[^/]+|';
+  foreach ($items as $path => $item) {
+    $path = preg_replace($regex, '%', $path);
+    $paths[$path] = $path;

I don't understand this at all. Are we checking that the translation paths dynamically defined in hook_menu() are still valid after hook_menu_alter() has run? If that's the case, why not just do the dynamic paths in hook_menu_alter() in the first place. Additionally if a contrib module alters a router item, they're expected to fix the fallout if they break stuff, there's no need for core to worry about that.

+++ b/core/modules/translation_entity/translation_entity.moduleundefined
@@ -0,0 +1,702 @@
+      if (!isset($paths[preg_replace($regex, '%', $path)])) {
+        drupal_set_message(t('The entities of type %entity_type do not define a valid base path: it will not be possible to translate them.', array('%entity_type' => $info['label'])), 'warning');

this should be trigger_error(). If an end-user triggers a menu_rebuild() they'll have no idea what this means.

+++ b/core/modules/translation_entity/translation_entity.moduleundefined
@@ -0,0 +1,702 @@
+  $result = db_select('translation_entity', 'te')
+    ->fields('te', array())
+    ->condition('te.entity_type', $entity_type)
+    ->condition('te.entity_id', array_keys($entities))

This should be just db_query(), not db_select() - it's not a dynamic query.

Sorry this is so incomplete and spotty, I might be able to look a bit more tomorrow unless I'm beaten to it. Overall I'd really like to see this go in so we can continue towards the removal of Translation module.

catch’s picture

Status: Needs work » Reviewed & tested by the community

dreditor status change...

catch’s picture

Issue summary: View changes

Updated issue summary remaining tasks to say tests being added (to cover user translation)

plach’s picture

@catch:

Thanks for the review!

I'd expect to see a separate check for whether comments (or the comment bundle most likely) are translation enabled. Why rely on the access callback?

Because it's also checking permissions, it's exactly what determines whether the user will get an access denied or not when accessing the link path. IMO it makes sense that it governs the link visibility. Seems like a common pattern to me or am I missing something?

I don't understand this at all. Are we checking that the translation paths dynamically defined in hook_menu() are still valid after hook_menu_alter() has run? If that's the case, why not just do the dynamic paths in hook_menu_alter() in the first place. Additionally if a contrib module alters a router item, they're expected to fix the fallout if they break stuff, there's no need for core to worry about that.

Well, this is has been straightly ported from D7. I agree we could avoid worrying about alterations, but declaring items in the menu alter hook caused a lot of pain in D7 because of missing keys added by the menu system only to items returned by hook_menu() implementations. Moreover some module could see the items in place and some other couldn't depending on the hook execution order.

I'd be ok to remove the part messing with menu loaders and just leave the essentials in place, however I think this will be totally revamped by whatever we come up with in #1783964: Allow entity types to provide menu items.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

COMMITTED AND PUSHED TO EIGHT DOT FREAKING EX!!! YEAH!!!

sun’s picture

Status: Fixed » Needs review

Aloha :)

I wasn't able to review the entire code, and I had to apply it locally to make sense of it.

In overall terms, it is exceptional to see how much work went into this issue. Given this massive amount of contribution, that inherently puts some pressure on reviewers. And I am indeed inclined to get this in rather earlier than later, so as to make an end to this epic issue. However, a couple of aspects of the implementation do not really look very clean and well designed, and after (briefly) reviewing the code, I still did not understand the main architecture of how the UI works (technically).

These two sides leave me in a terrible state, because on one hand, I see @plach and a handful of people rocking this issue and trying hard and harder to achieve a corner-stone for Drupal's long-term success, which will have a serious impact on its adoption, especially in Europe and other places of the world. On the other hand, however, the current state of the code definitely leads to a fair amount of confidence that this implementation will get "in the way" for many other core contributors who are working on completely different things. I'd be more than happy to say to everyone, "well, you have to eat that cake, deal with it", but to be honest, I fear some major breakage down the line.

Facing this scenario, the first thing I wondered was whether it would be possible to scale down the size and impact of this initial patch? For example (not sure whether this makes sense), only introducing entity_translation.module on its own, without wide-scale integration for all other core entities, and only proving that it works within its own tests?

Second, I also wondered how many follow-up tasks (as in: features) will actually depend on this initial patch? As in: If there are none, or not many, then we could possibly try to polish this some more until feature freeze, so as to produce less headaches for other core contributors until then? (Tweaks and polishing of this newly introduced Entity Translation UI feature itself would of course be exempt from feature freeze; that's what the phase until code freeze is for.)

.............SNIP.............

@webchick just pushed the button. Heh.

.............SNIP.............

Erm. Regardless of that, here's my review:

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -140,6 +144,16 @@
+ * - menu_base_path: (optional) The base menu router path to which the entity
+ *   administration user interface responds. It can be used to generate UI
+ *   links and to attach additional router items to the entity UI in a generic
+ *   fashion.
+ * - menu_view_path: (optional) The menu router path to be used to view the
+ *   entity.
+ * - menu_edit_path: (optional) The menu router path to be used to edit the
+ *   entity.
+ * - menu_path_wildcard: (optional) A string identifying the menu loader in the
+ *   router path.

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/EntityTranslationControllerInterface.php
@@ -0,0 +1,190 @@
+ * To make Entity Translation automatically support an entity type some keys
+ * may need to be defined, but none of them is required unless the entity path
+ * is different from ENTITY_TYPE/%ENTITY_TYPE (e.g. taxonomy/term/1), in which
+ * case at least the 'menu_base_path' key must be defined. This is used to
+ * determine the view and edit paths if they follow the standard path patterns.
+ * Otherwise the 'menu_view_path' and 'menu_edit_path' keys must be defined. If
+ * an entity type is enabled for translation and no menu path key is defined,
+ * the following defaults will be assumed:
+ * - menu_base_path: ENTITY_TYPE/%ENTITY_TYPE
+ * - menu_view_path: ENTITY_TYPE/%ENTITY_TYPE
+ * - menu_edit_path: ENTITY_TYPE/%ENTITY_TYPE/edit
+ * The menu base path is also used to reliably alter menu router information to
+ * provide the translation overview page for any entity.
+ * If the entity uses a menu loader different from %ENTITY_TYPE also the 'menu
+ * path wildcard' info key needs to be defined.

The addition of these entity info properties is in direct conflict with #1332058: Need a way to distinguish "public/final" URIs for entities from admin and redirected ones and #1783964: Allow entity types to provide menu items.

TBH, I do not understand the properties being proposed here.

However, scanning through the patch, it actually doesn't look like they are used anywhere (except for test entities), the core entities seem to specify the translation_controller_class property only — so perhaps we're in luck and can remove them from this patch without too much hazzle?

FWIW, for now, EntityListController simply leverages $entity->uri() and assumes that to be the entity's primary "view URI", and the default list controller also assumes that /edit and /delete are registered child routes below $entity->uri().

So before introducing these entity info properties (which appear to be conceptually wrong to me), I'd rather go with similar assumptions in the default translation controller, and require entity-specific controllers to override the appropriate (+ separated) methods, if their URI pattern is different for any reason.

+++ b/core/modules/comment/comment.module
@@ -1009,6 +1009,16 @@ function comment_links(Comment $comment, Node $node) {
       $links['comment-forbidden']['html'] = TRUE;
...
+      'title' => t('translations'),
...
+      'html' => TRUE,

It appears from the patch context that the existing comment links are specifying 'html' TRUE already, but that's not really a reason to continue the bogus pattern ;) There's no HTML involved in this link title, so 'html' should be FALSE (or just simply omitted). :)

+++ b/core/modules/node/lib/Drupal/node/NodeTranslationController.php
@@ -0,0 +1,50 @@
+  protected function entityFormTitle(EntityInterface $entity) {
+    $type_name = node_get_type_label($entity);
+    return t('<em>Edit @type</em> @title', array('@type' => $type_name, '@title' => $entity->label()));
+  }

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/EntityTranslationController.php
@@ -0,0 +1,433 @@
+  public function entityFormAlter(array &$form, array &$form_state, EntityInterface $entity) {
...
+      $title = $this->entityFormTitle($entity);
+      // When editing the original values display just the entity label.
+      if ($form_langcode != $entity->language()->langcode) {
+        $t_args = array('%language' => $languages[$form_langcode]->name, '%title' => $entity->label());
+        $title = empty($source_langcode) ? $title . ' [' . t('%language translation', $t_args) . ']' : t('Create %language translation of %title', $t_args);
+      }
+      drupal_set_title($title, PASS_THROUGH);

I do not understand why we need a entityFormTitle() method on EntityTranslationController, if that title gets overridden in 80% of all cases anyway, and can be easily auto-generated from $entity in the first place? I think we can remove that method.

Second, but minor: The auto-generated title composed of the returned $title is not a translatable string. ;)

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/EntityTranslationController.php
@@ -0,0 +1,433 @@
+  /**
+   * Implements EntityTranslationControllerInterface::getAccess().
+   */
+  public function getAccess(EntityInterface $entity, $op) {
+    return TRUE;
+  }

Normally, everything related to access should default to FALSE.

+++ b/core/modules/translation_entity/translation_entity.install
@@ -0,0 +1,71 @@
+function translation_entity_install() {

+++ b/core/modules/translation_entity/translation_entity.module
@@ -0,0 +1,702 @@
+function translation_entity_entity_info_alter(array &$entity_info) {
...
+function translation_entity_menu() {
...
+  foreach (entity_get_info() as $entity_type => $info) {
...
+function translation_entity_menu_alter(array &$items) {
...
+  foreach (entity_get_info() as $entity_type => $info) {

There are a lot of hidden dependencies going on here. At minimum, the hook_install() implementation should set a higher module weight via module_set_weight(), so that the entity_translation.module's hooks are executed after other modules. A reasonable weight might be 10.

(Btw, I have absolutely no idea how we're going to deal with these kind of race conditions in the new kernel/service/container world, so there's a lot of fun ahead of us...)

+++ b/core/modules/translation_entity/translation_entity.install
@@ -0,0 +1,71 @@
+function translation_entity_enable() {
...
+  drupal_set_message($message, 'warning');

1) This hook_enable() implementation outputs a message after enabling — but does not seem to prevent that from happening when installing Drupal (with the module).

2) The message is a first-time installation message, but is re-displayed whenever the module is (re-)enabled.

3) Why is this positive message a warning? Warnings are for... well, warnings. :)

4) The message reads a bit long. I still haven't actually read it, which is a good indicator that it is way too long. ;)

+++ b/core/modules/translation_entity/translation_entity.module
@@ -0,0 +1,702 @@
+function translation_entity_language_types_info_alter(array &$language_types) {
+  unset($language_types[LANGUAGE_TYPE_CONTENT]['fixed']);
+}

Putting on my innocent developer hat:
I do not understand what this alter hook is doing and why it is legitimate to unset language type info in the system like that. This definitely needs explanation in an inline comment.

+++ b/core/modules/translation_entity/translation_entity.module
@@ -0,0 +1,702 @@
+      $entity_position = count(explode('/', $path)) - 1;
+      $keys = array_flip(array('theme_callback', 'theme_arguments', 'access_callback', 'access_arguments', 'load_arguments'));
+      $menu_info = array_intersect_key($info['translation']['translation_entity'], $keys) + array('file' => 'translation_entity.pages.inc');
...
+      foreach ($menu_info as $key => $value) {
+        $item[str_replace('_', ' ', $key)] = $value;
+      }
...
+  $regex = '|%[^/]+|';
+  foreach ($items as $path => $item) {
+    $path = preg_replace($regex, '%', $path);
+    $paths[$path] = $path;
+  }
...
+      if (!isset($paths[preg_replace($regex, '%', $path)])) {
+        drupal_set_message(t('The entities of type %entity_type do not define a valid base path: it will not be possible to translate them.', array('%entity_type' => $info['label'])), 'warning');

This menu system futzing looks very concerning. Not only from a performance perspective, but also from an implementation perspective. I need to study the code some more to understand what it actually tries to achieve, and whether that couldn't be achieved in an easier way.

Second, we definitely do not want to output a message whenever the menu/router is rebuilt. This is rather hook_requirements()/status report material.

+++ b/core/modules/translation_entity/translation_entity.module
@@ -0,0 +1,702 @@
+/**
+ * Returns the key name used to store the configuration item.
+ *
+ * Based on the entity type and bundle, the variables used to store the
+ * configuration will have a common root name.
...
+ * @return string
+ *   The key name of the configuration item.
...
+function translation_entity_get_config_key($entity_type, $bundle, $setting) {
+  $entity_type = preg_replace('/[^0-9a-zA-Z_]/', "_", $entity_type);
+  $bundle = preg_replace('/[^0-9a-zA-Z_]/', "_", $bundle);
+  return $entity_type . '.' . $bundle . '.translation_entity.' . $setting;
+}

Wowza. This contains a ton of configuration system + config entity/bundle assumptions that are actually not enforced anywhere (and shouldn't be).

Is there any reason why this cannot use the entity info and check for the "config_prefix" key contained in there?

Hm. Perhaps I'm also mistaken and this doesn't actually make any assumptions about config object/file names, but only generates a key name that is internal to entity_translation.module...?

+++ b/core/modules/translation_entity/translation_entity.module
@@ -0,0 +1,702 @@
+function translation_entity_form_field_ui_field_edit_form_alter(array &$form, array &$form_state, $form_id) {
...
+      '#prefix' => '<div class="translatable"><label>' . $label . '</label>',
...
+      '#prefix' => '<label>' . $label . '</label>',
+      '#type' => 'checkbox',

I do not understand why this form alter uses completely custom HTML markup instead of the appropriate form element #types.

Both of them should be wrapped in a #type 'item' instead of the manual HTML + label futzing. :)

Schnitzel’s picture

tstoeckler’s picture

Thanks for providing the video and screens. Cool beans!

plach’s picture

Status: Needs review » Fixed

I opened #1831530: Entity translation UI in core (part 2) to address the issues raised by @catch and @sun since this one is totally unmanageable: it takes one minute just to render it on my phone.

plach’s picture

Issue summary: View changes

Updated issue summary to reflect remaining task to do tests is done!

plach’s picture

Issue tags: +entity translation

tagging

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue summary: View changes

Updated issue summary.

Gábor Hojtsy’s picture

Issue tags: -sprint

Moving off the sprint then. Thanks all, it was a heroic effort.

plach’s picture

Component: entity system » translation_entity.module
Issue tags: -entity translation
Gábor Hojtsy’s picture

@Lukas: please comment on the (part 2) issue linked above!

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary.

Status: Fixed » Closed (fixed)

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

webchick’s picture

Retroactively tagging. ;)

plach’s picture

Issue tags: +Celebration video

Not exactly :)

alexpott’s picture

Issue tags: -Avoid commit conflicts

Cleaning up tags

Gábor Hojtsy’s picture

Component: translation_entity.module » content_translation.module

This never had a change notice. Yup. Now it has. https://drupal.org/node/2028009 Please edit away as you see fit.

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary.

batigolix’s picture

In #2091479: Update hook_help for content_translation module we review the hook_help text that has its origin in this issue.
Linking them together as related.