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 v2commit 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
- (critical task) When #1498674: Refactor node properties to multilingual gets in, node properties: node titles, status, etc will be translatable.
- (major task) Global settings page for all translatable entities, alongside the per-entity ones. Follow-up #1810386: Create workflow to setup multilingual for entity types, bundles and fields opened based on #81 Currently the implementation of the entity translatability setting is a bit hacky, we need to clean it up now that #1739928: Generalize language configuration on content types to apply to terms and other entities landed. This might be done here and not part of a follow-up.
- (major task) Auto-enable translatability on every field. Follow-up #1807366: Make fields translatable by default when enabling translation on a bundle opened (based on comment #100)
- (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
- (normal task) "Translation is not supported if language is always one of: Not specified, Not applicable, Multiple" reword to: "...."
- (normal task) Notice: Trying to get property of non-object in node_help (line 142 of core/modules/node/node.module) (For background see #29?, #51, #57, #181.)
(minor task) Code/Comment style review/cleanup. (See #179, #180.)- The are some methods of
EntityTranslationControllerInterface
which should be best located elsewhere:- Follow-up #1810320: Remove EntityTranslationControllerInterface::getAccess() once have entity access opened based on #81 The
getAccess()
method should be removed as soon as we have entity access available: #1696660: Add an entity access API for single entity access, #777578: Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter() - Follow-up #1810330: Remove EntityTranslationControllerInterface::removeTranslation() opened based on #81 The
removeTranslation()
should go into theEntityInterface
as well, since all the data-structure handling methods concerning entity translation are there, the translation controller is focused only on the UI. - Follow-up #1810350: Remove menu_* entity keys and replace them with entity links opened based on #81 The
get*Path()
methods, which provide the paths to be used when building the UI for a particular entity type, should not be translation specific: #1781372: Add an API for listing (configuration) entities, #1783964: Allow entity types to provide menu items
- Follow-up #1810320: Remove EntityTranslationControllerInterface::getAccess() once have entity access opened based on #81 The
- Follow-up #1810370: Entity Translation API improvements opened based on #81 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 inefficient and fragile. The new Entity Field API offers a
getTranslation()
method which returns an object representing a facet of the entity in a particular language. Currently these live only at data structure level and are not persisted. We could have a full-fledgedEntityTranslation
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. - Follow-up #1807800: Add status and authoring information as generic entity translation metadata opened (based on comment #105 and #108) No matter what we decide for the previous point, we should consider whether we should make the publication status a property of the entity translations, and thus making it available for any entity type. Should we 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?
- Follow-up #1807776: Support both simple and editorial workflows for translating entities opened (based on comment #107) 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: Introduce a #multilingual key (or similar) for form element
- Follow-up #1810394: Site configuration with domain based language negotiation results in redirecting authenticated users to a different domain when accessing a content entity route for translation language different from the interface language opened based on #81 and #82 The translation addition as implemented right now is abit tricky: currently we have a dedicated page callback which provides an entity form having as form language the target one. 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.
Instead we need to to exploit the upcominghook_entity_prepare()
hook and set the form language from there. This would allow us to reuse the usual entity edit router path and would make sure we don't couple the entity form controller with the language switch logic implemented by Entity Translation. - Follow-up #1807692: Introduce a column synchronization capability and use it to translate alt and titles through the image field widget opened (based on comment #104)
- Follow-up #1807322: Filter comments taking into account the current content language opened (based on comment #98)
See #19, #81, #82 and #181 for details.
Other related issues belonging to the original roadmap:
- #1498704: Provide multilingual capabilities to the default SQL storage controller
- #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions
- #1498850: Rename the 'translatable' field property
- #1498880: Theme language switcher for seven theme
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.
Comment | File | Size | Author |
---|---|---|---|
#267 | 03.11.12 20:41-Schnappschuss.jpeg | 825.25 KB | Schnitzel |
#267 | 03.11.12 20:40-Schnappschuss.jpeg | 973.84 KB | Schnitzel |
#267 | 03.11.12 20:40-Schnappschuss.jpeg | 1004.78 KB | Schnitzel |
#258 | et-ui-1188388-258.patch | 137.55 KB | plach |
#258 | et-ui-1188388-258.nofix_.patch | 137.55 KB | plach |
Comments
Comment #2
plachI'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.
Comment #3
plachtagging
Comment #4
renat CreditAttribution: renat commentedSubscribe.
Comment #5
zilverdistel CreditAttribution: zilverdistel commentedsubscribe
Comment #6
sunComment #7
Gábor HojtsyTagging for the content handling leg of D8MI.
Comment #8
Gábor HojtsyTagging for sprint tomorrow to get started with and adding META to title, since we want to break this up most likely.
Comment #8.0
Gábor HojtsyFixed the link to entity_translation module
Comment #9
plachUpdated the issue summary.
Comment #9.0
plachAdded issue summary
Comment #10
Gábor HojtsyThe 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.
Comment #11
Gábor HojtsyThe 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.
Comment #11.0
Gábor HojtsyUpdated issue summary.
Comment #11.1
plachUpdated issue summary.
Comment #12
Gábor Hojtsy@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.
Comment #13
plachAs 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...
Comment #14
webchickTagging. :) EXCITING that this is being worked on!!
Comment #15
plachJust to clarify:
They should be identical at the moment.
I'll provide some updated screenshots ASAP.
Comment #16
plachJust 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.
Comment #17
Gábor HojtsyWoot! Looking forward to reviewing the progress :) It would make sense to update the summary as well, eg. no Storage API to build on.
Comment #18
plachJust 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 :)
Comment #19
plachOk, 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:NodeFormController
to the base one, to support the language switcher being available to any entity type.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:
EntityTranslationControllerInterface
that I think would be best located elsewhere:getAccess()
method should be removed as soon as we have entity access available and replaced withEnttyInterface::access()
calls or something along those lines.removeTranslation()
should probably go into theEntityInterface
as well, since all the data-structure handling methods concerning entity translation are there, the translation controller is focused only on the UI.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.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 anEntityInterface::getTranslation($langcode)
method that would return a class implementing theEntityInterface
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-fledgedEntityTranslation
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.entity_get_form()
performed in its context.Here are features not yet ported as I need some feedback:
NodeTranslationController
to adjust the node form to support multilingual menus.I think most of these should be deferred to followups to make the patch reviewable.
Comment #20
plachComment #22
sunLet me introduce to you... ImageMagick
;)
Comment #23
plachThis should fix the failing tests.
Comment #24
plachI think this should be part of the entity system queue.
Comment #26
plachLet's try this one.
Comment #26.0
plachUpdated issue summary.
Comment #26.1
plachUpdated issue summary.
Comment #27
sunComment #28
podarok#26 just installed locally for review
one bug with this patch is duplicated Translation option in content type editing form
Comment #29
podarokchild issue #1785936: Trying to get property of non-object in taxonomy_help()
Comment #30
plach@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.
Comment #31
podarok#30 yup, it is
So without legacy Content translation module #26 looks like RTBC for me
Comment #32
plachThanks a lot for actually testing the patch but I think we need at least some feedback on #19 before actually committing this :)
Comment #33
Gábor HojtsyTagging for D8MI sprint.
Comment #34
plachComment #34.0
plachUpdated issue summary.
Comment #35
plachRerolled after the latest commits.
Comment #37
plachNo failures here on my box.
#35: et-ui-1188388-35.patch queued for re-testing.
Comment #38
fagojust 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:
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.
Comment #39
Gábor HojtsyPostponed on #1696640: Implement API to unify entity properties and fields or there are good things to work on inbetween? Sounds like there might be.
Comment #40
Gábor HojtsyHighten priority as it is required to remove the existing translation module, etc.
Comment #41
YesCT CreditAttribution: YesCT commentedI was checking in on this to redo a ui review I started. I'll hold off based on #38.
Comment #42
plach@YesCT:
Thanks! If you are testing the UI, please, feel free to go on: nothing mentioned in #28 will affect UX.
Comment #43
YesCT CreditAttribution: YesCT commentedOK. Will do.
Comment #44
mtomaizy CreditAttribution: mtomaizy commented#35: et-ui-1188388-35.patch queued for re-testing.
Comment #45
plachWorking 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.
Comment #46
Gábor HojtsyPlease summarise outstanding questions then with the new patch so we can quickly look at those and figure out solutions if needed.
Comment #47
plachOk, I'll update the issue summary. However most of #19 still stands.
Comment #48
YesCT CreditAttribution: YesCT commentedI went through the ui and I have a bunch of comments. I'm writing them up to make them understandable by others.
Comment #49
YesCT CreditAttribution: YesCT commentedFew 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.
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.
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
cannot translate because no translatable fields
edit the content type and go to manage fields to edit the body to enable translation
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)
Expanded it has wording like:
Field translation
This field is shared among the entity translations. --link--Enable translation--endlink--
and looks like:
Once it's enabled it has the wording:
Field translation
Users may translate this field. --link--Disable translation--endlink--
And looks like:
More steps to testing
Now on the translation tab of the content can add a translation
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.
Comment #50
Bojhan CreditAttribution: Bojhan commentedI don't fully get what to review. It feels a little like most internationalization, checkboxes all over :D
Comment #51
YesCT CreditAttribution: YesCT commentedSorting 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).
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.
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.
Comment #52
YesCT CreditAttribution: YesCT commentedImages missing from previous comment...
Comment #53
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedBojhan: I think the attachment in #22 is your best bet :)
Comment #54
YesCT CreditAttribution: YesCT commentedReusing my not quite as organized screenshots from earlier and I dont have notes to exactly reproduce these steps... sorry.
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.
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?!
url Lang
Sometimes the url and the links to click on seemed strange or to not be consistant
Translation Tab Redesign
I think some reordering of the columns could make this translations tab be easier to use.
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.
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.
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.
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.
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.
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.)
Order of settings and fields on comment edit
I suggest moving down the location of the translation setting collapsed field set.
Cannot reorder in the ui.
I have some more comments about... comments. :) To come.
Comment #55
YesCT CreditAttribution: YesCT commented@Bojhan (#50 #53) I added some more things to look at.
Comment #56
YesCT CreditAttribution: YesCT commentedAdding 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).
Comment #57
Gábor Hojtsy@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
Comment #58
Gábor Hojtsy@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.
Comment #59
Bojhan CreditAttribution: Bojhan commentedI 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.
Comment #60
Gábor HojtsyI think that sounds like a good suggestion and combined with YesCT's suggestions can elevate the usability of this patch quite a bit :)
Comment #61
plachCan 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.
Comment #62
YesCT CreditAttribution: YesCT commentedRegarding #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.]
Comment #63
YesCT CreditAttribution: YesCT commented@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.
Comment #64
Bojhan CreditAttribution: Bojhan commented@plach I am generally not really confident i18n followups on usability happen, but sure :)
Comment #65
plach@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 :)
Comment #66
YesCT CreditAttribution: YesCT commented@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.
Comment #67
Gábor Hojtsy@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).
Comment #68
plachDidn't mean to change the status.
Comment #69
plachI 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.
Comment #71
plachLet's try this one.
Comment #72
plachThe 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:
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).
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.
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.
We have a follow-up for this: #1498724: Introduce a #multilingual key (or similar) for form element.
I think simply suggesting a log message in the current interface language might work. Thoughts?
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.
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 :)
Looks like it's tied to URL prefixes, IIRC this happens also without the patch.
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:
Do you mean we would need a central place where enabling translation for all the available entities?
Comment #73
Bojhan CreditAttribution: Bojhan commented@plach Yes, that is what I mean - not just place, workflow :)
Comment #74
plachWould 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
.Comment #76
plachForgot an include...
Comment #78
plachwtf
Comment #79
Gábor HojtsyTried 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.
Comment #80
plachI'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.
Comment #81
Gábor HojtsyThis 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.
Sounds good.
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.
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.
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.
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.
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.
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.
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.
Right, I think this is discussed above.
Don't know how would you make a meaningful UI for that though.
I don't think this belongs in this patch. Let's not blow up the scope.
I think a hook should be provided, which path module reacts to. Definitely not explicit. Sounds like yet another todo for this patch.
This might need to happen in this patch as well :/
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? :)
Comment #82
plachLet 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.
This probably won't be needed as
EntityNG::getTranslation()
returns the entity itself when the passed langcode is the default language.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.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.
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.
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.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.
Yes, but do we want this for any entity type?
I think it can be another option below the one enabling translation.
Again, all of this is already implemented in D7 and should totally be a follow-up :)
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?
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.
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.
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.
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 :)
Comment #82.0
plachUpdated issue summary.
Comment #83
plachI 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.
Comment #83.0
plachUpdated issue summary
Comment #83.1
plachUpdated issue summary.
Comment #84
YesCT CreditAttribution: YesCT commented@plach I'm going to work on opening follow up issues.
Comment #85
YesCT CreditAttribution: YesCT commentedImprovement 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.
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
note: orig language no [language] bracket on edit page title
edit of original content /node/1/edit has no [language]
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?
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)
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
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)
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)
/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?
(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.]
Comment #86
Gábor HojtsyThen 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? :)
Comment #87
YesCT CreditAttribution: YesCT commentedI 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.
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.]
Comment #88
plach@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.
Comment #89
YesCT CreditAttribution: YesCT commentedIn #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)
Comment #90
plach@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.
Comment #91
plachThis is weird. Did you clear the cache after applying the new patch?
Comment #92
YesCT CreditAttribution: YesCT commented@plach regarding #89 and #91 (source language selection not possible sometimes) yep. I double checked the cache is cleared.
Comment #93
YesCT CreditAttribution: YesCT commented@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.
Comment #94
plach#92 looks like a bug in the overlay: it seems some JS is not working properly.
Comment #95
YesCT CreditAttribution: YesCT commented@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.
Comment #96
YesCT CreditAttribution: YesCT commentedin #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:
orig language spanish, english has no translation
using the edit link in the spanish row (editing the spanish version/translation/originalcontent) can change the language to english
now it says that english is the original language but it's not the original language, it's *the* language of the node
Comment #97
plachIt'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.
Comment #98
YesCT CreditAttribution: YesCT commentedin #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?)
Comment #99
YesCT CreditAttribution: YesCT commented@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.)
Comment #100
YesCT CreditAttribution: YesCT commentedDoing 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.
Comment #101
plach@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.
Comment #102
YesCT CreditAttribution: YesCT commented[ 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.)
Comment #103
YesCT CreditAttribution: YesCT commentedTrying 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."
Comment #104
YesCT CreditAttribution: YesCT commentedSummarizing 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]
Comment #105
YesCT CreditAttribution: YesCT commentedTopics: Translation Status, Publication status per entity translation property, Special case for nodes, Fallback for unpublished translation
background
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.
Questions
[edit: @plach, sorry I found these related questions and added them to this comment]
Comment #105.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary.
Comment #106
plachNope, this is one of the major follow-ups. At the moment only nodes have published status support.
Comment #107
YesCT CreditAttribution: YesCT commentedSummarizing 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
Comment #107.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary.
Comment #108
YesCT CreditAttribution: YesCT commentedI 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
Comment #108.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary
Comment #109
YesCT CreditAttribution: YesCT commentedI 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.
Comment #110
YesCT CreditAttribution: YesCT commentedI 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.
translations settings can be positioned on the node manage fields tab
looking at default position of language setting on node edit
moved it on the content type manage fields tab
reload the node edit and see the language select position is changed.
(I like it closer to the title)
check out the manage display tab on content type.
translation (language setting) is by default hidden.
there is no language or translation field in the comment display (and the table border is strange)
adding comment to a node
defaults to detected language (not sure if that's the user interface detected language or the content detected language)
comment has nice translations link added
add a translation of a comment. strange order: translation setting below save button
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.
nice. looking at what a node is like with multiple comments not all translated into the detected language
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.
Comment #111
BerdirLooked 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.
Missing @param and @return documentation.
Class references in docblocks need to be fully qualified. Drupal\Core\Entity\...
Lot's of module_exists() checks added, wondering if there are ways to avoid this.
Isn't node enabled by default?
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.
I think we're not translating language names anymore?
Not sure if this a valid thing to do (the strtolower). Maybe keep it in the original case and rewrite the message a bit?
Replace "@param Entity" with @param Drupal\Core\Entity\Entity"
What are the valid operations? I assume this is partially translation specific and should probably be listed...
I think we can remove the modules directory there. tests/ doesn't contain anything else than test modules anymore...
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?
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...
@param and actual argument mismatch.
types missing.
I guess this would mess up with RTL languages?
Hah, sounds familar :p
Typo. No param documentation.
Comment #112
Bojhan CreditAttribution: Bojhan commented@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.
Comment #112.0
Bojhan CreditAttribution: Bojhan commentedUpdated issue summary. I added links to the follow-up issues that were created.
Comment #112.1
YesCT CreditAttribution: YesCT commentedclarified what the remaining tasks are for this issue and what are follow up issues
Comment #113
YesCT CreditAttribution: YesCT commentedI'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.
Comment #113.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary. clarified the ui review is done and assigned some stuff to plach and some to me.
Comment #114
YesCT CreditAttribution: YesCT commentedI 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.
Comment #114.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary. Added references to the specific follow-up issues
Comment #115
plachThe 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.
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.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.
The test module was empty, I dropped it. We can resurrect it when/if needed.
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.
Comment #116
cosmicdreams CreditAttribution: cosmicdreams commentedin that case, can you put in @todo comments for that future work?
Comment #116.0
cosmicdreams CreditAttribution: cosmicdreams commentedUpdated issue summary to note all follow-ups have been opened.
Comment #117
aspilicious CreditAttribution: aspilicious commentedWe need 3th person verbs for this. And this doens't have any docs.
Toggles
Same
Don't we need some kind of reference in this function? Or does php pass stuff into arrays as reference?
Should start with 3th person verb
It's best to typehint the @param stuff
Comment #118
nod_tag
Comment #119
YesCT CreditAttribution: YesCT commented@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
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]
Comment #120
plachThe attached patch takes care of the latest reviews including @YesCT's ones. Specifically:
Some answers:
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.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.
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.
This patch does not introduce per-language status for nodes. This will come with #1498674: Refactor node properties to multilingual.
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.
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.
The language extra fields is not provided by this patch.
The detected content language.
Language selectors are not provided by this patch.
No documentation has been written yet. We will probably need a page describing how to setup everything.
The current tests should cover more or less all the features implemented here.
@aspilicious:
Thanks for reviewing :)
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.
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.
Comment #121
YesCT CreditAttribution: YesCT commentedneeds review to get the bot to look at it (and the people!) Thanks plach
Comment #122
YesCT CreditAttribution: YesCT commentedNote 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?
Comment #122.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary. took out the remaining task to post the most recent code. done.
Comment #122.1
YesCT CreditAttribution: YesCT commentedUpdated issue summary with information about tests and documentation
Comment #123
plachNo idea of what that needs work stands for.
Comment #124
YesCT CreditAttribution: YesCT commented@plach Sorry, I'm not used to dreditor auto changing status to needs work. Need review is exactly where this should be. :)
Comment #125
plachWhew :)
Comment #126
YesCT CreditAttribution: YesCT commentedI 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.
Comment #127
YesCT CreditAttribution: YesCT commentedGetting 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.
Can see the language select since the hide setting was unchecked on the article content type.
The language select is in the default location.
After creating an article, viewing it has a translations tab along with the view and edit tab.
Shows overview of translations. Right now, nothing is translatable, so cannot add translations.
(moved the language field while at the edit content type page to enable translation on body.)
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.
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.
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.
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)
the hint in the collapsed fieldset has the right value, and the fields in the form have values from the new source.
Can check the box in the translation vertical tab translation setting to flag other translations as needing to be updated.
and showing the edit link
(showing it without my red marks)
has lots of hints
no changing source while editing a translation
can uncheck to indicate it has been updated in the translation setting in the vertical tab. shows wording.
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.
(showing it without my red marks)
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.
This is a little weird, but probably a general language detection issue and not specific to this et-ui.
Comment #127.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary to reflect latest code is posted and update remaining tasks.
Comment #128
plachWonderful. Anyone wishing to RTBC this or push it back to needs work? ;)
Comment #129
Gábor HojtsyI 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?
Comment #130
YesCT CreditAttribution: YesCT commentedsimilar to node, nothing to translate
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"
Comment #131
plach@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.
Comment #132
YesCT CreditAttribution: YesCT commentedVocabulary and Terms
Comment #133
YesCT CreditAttribution: YesCT commentedUsers
Comment #134
YesCT CreditAttribution: YesCT commentedchanging status and updating summary
Comment #134.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary.
Comment #134.1
YesCT CreditAttribution: YesCT commentedUpdated issue summary to reflect screenshots posted and might be blocked on generalize language cmi 1739928
Comment #134.2
plachUpdated issue summary.
Comment #135
cosmicdreams CreditAttribution: cosmicdreams commentedOMG 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.
Comment #136
YesCT CreditAttribution: YesCT commentedOh, 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?
Comment #137
YesCT CreditAttribution: YesCT commented@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.
Comment #137.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary.
Comment #138
YesCT CreditAttribution: YesCT commented#120: et-ui-1188388-120.patch queued for re-testing.
Comment #138.0
plachUpdated issue summary clarifying where the test entity screen is.
Comment #139
plachOn commit please credit also @peximo who did lots of work in the sandbox...
Edit: please refer to the updated credits section in the summary.
Comment #140
plach@YesCT:
Just enable the Testing module, run the Entity Translation UI tests and you will find the pages ready for screenshot in the verbose messages.
Comment #140.0
plachUpdated issue summary.
Comment #140.1
plachUpdated issue summary.
Comment #141
Dries CreditAttribution: Dries commentedA 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.
Comment #142
catchHow 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.
Comment #143
Bojhan CreditAttribution: Bojhan commentedThe 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.
Comment #144
plach@catch:
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.Comment #145
catchI 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.
Comment #146
YesCT CreditAttribution: YesCT commentedNot 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.
Comment #147
webchickI'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. :)
Comment #148
plachThis needs a reroll now that #1739928: Generalize language configuration on content types to apply to terms and other entities got in.
Comment #148.0
plachUpdated issue summary.
Comment #149
YesCT CreditAttribution: YesCT commentedThis 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)
Comment #150
plachWell, 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.
Comment #151
plachHere 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 :)
Comment #153
fagoGreat 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.
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.
is matches. Misses @return.
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().
Comment #154
plachAside 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:
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?
Comment #156
fago>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?
Comment #157
plach#154: et-ui-1188388-154.patch queued for re-testing.
Comment #158
plachThe attached patch addresses #153, except:
Won't this iterate on Field API fields too? Here we are updating plain properties, or am I missing something?
Comment #159
fagoOh, 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.
Comment #160
webchickInitial 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.
Comment #161
plachFirst 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:
We have no entity translation settings at the moment.
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.
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.
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.
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.
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?
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.
In the current UI we have only one operation, however it might make sense to improve consistency.
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).
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.
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.
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.
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.
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.
Agreed. Going to work on that ASAP.
Agreed. There already is a planned follow-up for this: #1807366: Make fields translatable by default when enabling translation on a bundle.
Agreed. When node properties are multilingual (see above).
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.
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.
Comment #161.0
plachUpdated issue summary to reflect 1810386 might not be done as a follow up since 1739928 has landed already
Comment #161.1
plachUpdated issue summary.
Comment #161.2
plachUpdated issue summary
Comment #162
plachHere is a patch providing a new help section for ET, fixing @webchick's "fixable" UX concerns and #159:
Comment #163
plachAbout
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.
Comment #163.0
plachUpdated issue summary.
Comment #163.1
plachUpdated issue summary.
Comment #164
jhodgdonplatch 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!
Comment #165
plach@jhogdon:
Even if the new keys are provided by a module and not by the core entity system?
Comment #166
Bojhan CreditAttribution: Bojhan commentedCould 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.
Comment #167
jhodgdonRE #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.
Comment #168
Gábor Hojtsy#162: et-ui-1188388-162.patch.patch queued for re-testing.
Comment #169
Gábor Hojtsy#1739928: Generalize language configuration on content types to apply to terms and other entities got committed, so this definitely need a reroll.
Comment #170
plachHere it is.
Comment #171
plachReviewing/fixing my own patch :)
Incomplete PHP doc.
This is not needed anymore.
LANGUAGE_NOT_SPECFIED
translation_entity
variable_get(), hm
Remove this
Unnecessary changes.
Comment #172
fagoAs said, patch looks but I think the hook_entity_info docs still need some clarifications:
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.
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.
Comment #173
plachImplemented #172.
Comment #174
YesCT CreditAttribution: YesCT commentedIn #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:
Here is the suggestion from 49:
Oh, and @plach points out in #161
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.)
Comment #175
YesCT CreditAttribution: YesCT commentedI 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)
Comment #175.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary.
Comment #176
YesCT CreditAttribution: YesCT commentedChanges 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.
Comment #177
YesCT CreditAttribution: YesCT commentedBecause 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.
Comment #177.0
plachUpdated issue summary to add #1498674 in with the follow-up's.
Comment #178
plach@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.
Comment #179
fagoAd #173, thanks that looks good to me now.
Minor: It should use our standard for (optional) stuff though, e.g.
menu base path: (optional) The base...
Comment #180
plachGreat, I guess I looked at the wrong code to get inspiration :)
Comment #181
webchickOk, 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: "...."
Comment #181.0
webchickUpdated issue summary.
Comment #181.1
YesCT CreditAttribution: YesCT commentedUpdated issue summary to integrate notes in #181 that resulted from the phone call to identify commit blockers.
Comment #182
YesCT CreditAttribution: YesCT commentedI updated the issue summary.
From the phone call, the commit blocker listed in #181
@plach Can something be implemented to accomplish that without #1498724: Introduce a #multilingual key (or similar) for form element?
Comment #182.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary remaining tasks section.
Comment #183
YesCT CreditAttribution: YesCT commentedIn #181
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.
Comment #184
YesCT CreditAttribution: YesCT commentedFixes 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
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.
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
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.
4: better success message for disabling or enabling translation to address the comment from @webchick in step 13 of #160
"Data successfully processed." changed to "Successfully changed field translation setting." because I think that is more accurate and eliminates the wth? webchick had.
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.)
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).
Comment #185
YesCT CreditAttribution: YesCT commentedDoes
+ $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:
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.
Comment #186
YesCT CreditAttribution: YesCT commentedThis 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.
Comment #188
YesCT CreditAttribution: YesCT commentedOops 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.
Comment #189
YesCT CreditAttribution: YesCT commentedtests passed locally, trying test bot.
Comment #190
YesCT CreditAttribution: YesCT commentedthis 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
Comment #191
plachThis 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.
We still need #1498724: Introduce a #multilingual key (or similar) for form element to fix things reliably.
Comment #191.0
plachUpdated issue summary to track minor follow-up for comment style fix needed from #179
Comment #192
cosmicdreams CreditAttribution: cosmicdreams commentedManually tested, works great
Comment #193
cosmicdreams CreditAttribution: cosmicdreams commentedwhoops didn't mean to RTBC as this needs Webchick's help text still.
Comment #193.0
plachUpdated issue summary.
Comment #193.1
plachUpdated issue summary.
Comment #194
plachThanks! 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.
Comment #195
YesCT CreditAttribution: YesCT commented1: question about test for hidden language select on translations
I have a confusion about the test; there may not be anything wrong.
hidden using #access FALSE
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)
line 140 is the page arguments line.
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.)
Comment #195.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary.
Comment #196
YesCT CreditAttribution: YesCT commentedOn 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.
Comment #197
plachOops. 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.
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.
It's checking that the language selector is not there. No point in specifying a value.
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).Yes, this is by design.
Comment #198
YesCT CreditAttribution: YesCT commentedah. 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?
Comment #199
YesCT CreditAttribution: YesCT commentedhere is a screen shot showing the latest patch from #197 working for the (all languages) addition to field labels
Comment #199.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary with status of commit blocker: webchick review instructions and marking shared fields (all languages) still in progress.
Comment #199.1
YesCT CreditAttribution: YesCT commentedUpdated issue summary to give comment numbers which contain updated screenshots.
Comment #200
YesCT CreditAttribution: YesCT commentedHere are some screenshots of the test entity (for completeness) This all seems to be working fine.
Comment #201
YesCT CreditAttribution: YesCT commentedDoes 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)
Comment #202
YesCT CreditAttribution: YesCT commentedIn #176 I asked about help page guidelines. I found them http://drupal.org/node/632280
Comment #202.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary to reflect "all languages" commit blocker no longer a remaining task.
Comment #202.1
YesCT CreditAttribution: YesCT commentedUpdated issue summary with comment number that has test entity screenshots that result from recent patch
Comment #203
BerdirI'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?
Comment #204
plachThis is clearly an oversight :)
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.
Comment #205
plach@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.
Comment #206
BerdirEFQ v2 is in, so I'm quite sure this needs to be updated, will trigger a re-test.
Comment #207
Berdir#197: et-ui-1188388-197.patch queued for re-testing.
Comment #208.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary to reflect screenshots have been posted to reflect commit blocker fixes.
Comment #209
YesCT CreditAttribution: YesCT commentedHere 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]
Comment #210
plachWill reroll this tonight.
Comment #211
plachThis 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.
Comment #213
YesCT CreditAttribution: YesCT commented#211: et-ui-1188388-211.patch queued for re-testing.
Comment #215
plachI 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.
Comment #216
plachThe EQ issue: #1828790: Entity count query not working with fields shared among different entity types.
Comment #218
webchick#215: et-ui-1188388-215.patch queued for re-testing.
Comment #219
webchickOk! 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.)
Comment #220
webchickCross-posting with myself. :P
Comment #221
YesCT CreditAttribution: YesCT commentedI'm working on #201 (I checked with webchick and she verified that the @param, etc needs to be done and #219
Comment #222
plach@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).
Comment #223
plachWorking on the help.
Comment #224
plachOk, here is the help from #219. I changed only the last two words :)
became
since permissions have entity type and not content type granularity.
Anyone here bold enough to mark this RTBC again?
Comment #225
plachAnd this is /me failing at life :(
Comment #226
YesCT CreditAttribution: YesCT commentedThis is just the help changes. Mostly the ones from #219
Comment #227
YesCT CreditAttribution: YesCT commentedI 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.
Comment #228
YesCT CreditAttribution: YesCT commentedcrud. cross posts.
Comment #229
YesCT CreditAttribution: YesCT commentedThis is the list of functions (See #201.)
need @param, some need @return too.
Comment #230
bforchhammer CreditAttribution: bforchhammer commentedI read through parts of the patch (#225) and stumbled on a few documentation issues (typos etc.). Nothing major, but should be easy to fix... :)
keyS (plural)?
Missing $ in front of variable name
menu_path_wildcard - l and d swapped.
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).
two-step (no s)
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?
Double space after "For example,"
Maybe emphasize checkbox label to make the sentence easier to read?
missing "n" in "current"
Comment #231
bforchhammer CreditAttribution: bforchhammer commentedSorry, cross-post.
Comment #232
YesCT CreditAttribution: YesCT commentedHere is a patch to deal with the help text cross post. (so #226 can be ignored)
Comment #233
YesCT CreditAttribution: YesCT commentedOK. 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.
Comment #234
YesCT CreditAttribution: YesCT commentedthis encorporates #230 and a sample of adding @see to submit handler doc stuff.
I'll work on the rest of the docs next.
Comment #235
YesCT CreditAttribution: YesCT commentedHere is a go at docs for submit handlers. tim.plunkett helped me with this.
Comment #236
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedLooks 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.
Comment #237
Gábor Hojtsy@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 :)
Comment #238
Gábor HojtsyAdd feature freeze tag.
Comment #238.0
Gábor HojtsyUpdated issue summary since remaining tasks now includes rerolling for EFQ v2
Comment #239
Gábor HojtsySending in for a testbot tour.
Comment #240
Gábor HojtsyCross-posted myself, that is hardcode. .'–/
Comment #241
YesCT CreditAttribution: YesCT commenteddocs 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.
Comment #242
plachRerolled after #1828790: Entity count query not working with fields shared among different entity types.
Comment #243
plachehm :)
Comment #244
YesCT CreditAttribution: YesCT commentedThe 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?
Comment #244.0
YesCT CreditAttribution: YesCT commentedUpdated credits
Comment #245
falcon03 CreditAttribution: falcon03 commentedDid someone review this UI for its accessibility?
If not, I could open a critical follow-up issue to get this done.
Comment #246
YesCT CreditAttribution: YesCT commentedA 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.
Comment #247
YesCT CreditAttribution: YesCT commented@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
Comment #248
YesCT CreditAttribution: YesCT commentedIf 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.
But when clicking on add to add a translation from the translation tab, I see just the translatable field and the notice:
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).
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
and there is no new translation added.
Comment #249
penyaskitoReproduced 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.
Comment #250
penyaskitoThe 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.
Comment #251
penyaskitoFixed the redirect and the fields shown at the form.
Comment #252
plachWorking on improved tests.
Comment #253
BerdirThe code looks very clean and well documented. Much better than when I last checked :)
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.
These should have a simple docblock.
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?
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.
#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?
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.
Comment #253.0
BerdirUpdated issue summary. took out a mention to verifying help by webchick that was under the documentation remaining tasks, and that task is done.
Comment #254
YesCT CreditAttribution: YesCT commentedjust updating the status since plach is actively working on a new patch with the tests. :)
Comment #254.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary.
Comment #255
YesCT CreditAttribution: YesCT commentedsteps to reproduce
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)
Comment #256
YesCT CreditAttribution: YesCT commentedsteps to reproduce on something that is not a user: page content type
Get the notice:
This should make thinking about the error easier.
Also note if go past the notice and save the translation, the translation is not created.
Comment #257
YesCT CreditAttribution: YesCT commented@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]
Comment #258
plachHere 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:
@Berdir:
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.Yes, see the OP.
That code will all go away when Field API language behavior mirrors Entity Field language one.
Comment #259
Gábor HojtsyChanges 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.Comment #260
YesCT CreditAttribution: YesCT commentedI 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.
Comment #261
plach@committers:
Please refer to the credits section in the OP for the commit message :)
Comment #262
catchI 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).
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?
This feels like something the entity translation module could provide a function for? Or is it very, very specific to node module?
s/curret/current
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.
this should be trigger_error(). If an end-user triggers a menu_rebuild() they'll have no idea what this means.
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.
Comment #263
catchdreditor status change...
Comment #263.0
catchUpdated issue summary remaining tasks to say tests being added (to cover user translation)
Comment #264
plach@catch:
Thanks for the review!
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?
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.
Comment #265
webchickCOMMITTED AND PUSHED TO EIGHT DOT FREAKING EX!!! YEAH!!!
Comment #266
sunAloha :)
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:
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.
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). :)
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. ;)
Normally, everything related to access should default to FALSE.
There are a lot of hidden dependencies going on here. At minimum, the
hook_install()
implementation should set a higher module weight viamodule_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...)
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. ;)
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.
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.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...?
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. :)
Comment #267
Schnitzel CreditAttribution: Schnitzel commentedcommit video! yay :)
http://www.youtube.com/watch?v=TWtMNLCMdeQ
Comment #268
tstoecklerThanks for providing the video and screens. Cool beans!
Comment #269
plachI 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.
Comment #269.0
plachUpdated issue summary to reflect remaining task to do tests is done!
Comment #270
plachtagging
Comment #270.0
plachUpdated issue summary.
Comment #270.1
plachUpdated issue summary.
Comment #271
Gábor HojtsyMoving off the sprint then. Thanks all, it was a heroic effort.
Comment #272
plachComment #273
Lukas von BlarerI created a few follow-up issues of this one:
#1831636: "Translation is not supported if language is always one of: Not specified, Not applicable, Multiple" reword to: "...."
#1831604: Change dropbutton labels on translations tab to "add translation" / "edit translation" / "delete translation"?
#1831608: Show or hide the "Make field translatable" checkbox on the add field form depending on translatability
Comment #274
Gábor Hojtsy@Lukas: please comment on the (part 2) issue linked above!
Comment #274.0
Gábor HojtsyUpdated issue summary.
Comment #276
webchickRetroactively tagging. ;)
Comment #277
plachNot exactly :)
Comment #278
alexpottCleaning up tags
Comment #279
Gábor HojtsyThis never had a change notice. Yup. Now it has. https://drupal.org/node/2028009 Please edit away as you see fit.
Comment #279.0
Gábor HojtsyUpdated issue summary.
Comment #280
batigolixIn #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.