TF #4: Translatable fields UI
plach - August 3, 2009 - 21:09
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | language system |
| Category: | task |
| Priority: | critical |
| Assigned: | plach |
| Status: | needs review |
| Issue tags: | Exception code freeze, i18n sprint, Needs usability review, translatable fields |
Description
Locale and Translation need a deep refactoring in order to take advantage of #367595: Translatable fields, moreover we need to decide if node translation and field translation should coexist and if not find an acceptable upgrade path for D6 translation sets.

#1
See http://drupal.org/node/367595#comment-1884540 for details.
Edit: see http://drupal.org/node/367595#comment-1851754 for the patch design.
#2
Re-rolled patch, so that the testing bot will notice it.
#3
Hmm, that didn't work. Re-re-rolled.
#4
The last submitted patch failed testing.
#5
The first patch depends on #367595: Translatable fields, it won't pass the tests before that one is committed.
See #367595-183: Translatable fields for a patch including all the needed code.
#6
subscribing.
#7
Here are some screenshots of the patch applied along with its dependencies: tf-view-* images show the separation between UI language and content language, tf-config-* show the language configuration, tf-translation-overview, tf-edit-node and tf-edit-translation show how the transaltion workflow is handled.
#8
Excellent work. This patch illustrates the power of field-based translation and the relative ease of implementing an admin UI for the backend infrastructure achieved in #367595: Translatable fields.
While I briefly read over the code, this patch review is based on a demo site and focuses on the translation design and interface.
Rather than imagining a translation UI from scratch, the patch takes the approach of emulating what we already have in the translation module. I think this is the right approach. No doubt through a lengthy process we could evaluate and refine our translation interface, possibly completely redesigning it, but here isn't the place to do so. This patch wisely sticks closely to what we have.
Patch summary
The patch implements two new modules and a new locale include file.
The first module, entity_translation, provides a set of API functions for handling records related to the existence of translations of entities. E.g., when a node is translated, a record is created noting the entity type (node), the language of the translation, etc.
The second module is field_translation, parallel to translation module. Like translation, field_translation provides a tab on the node page alongside "Edit" for managing translations of the node. Here the tab is "Field translation" and, as with translation module, is shown when a node has a designated language. The field translation page for a node shows enabled content languages and links for creating new translations or editing existing ones.
The language of the original node is designated the "source" translation. Clicking on a link provides a form for editing a translation.
Comments, questions, and issues
Relation to translation.module. Implementing this in a new module separates the two models of translation (field-based and node-based). However, the interaction of the two models is something we need to think through. Unless we explicitly prevent it - which is not done in the current patch, and is in any case probably not a good idea - both models may coexist on a given site.
This coexistence will be potentially confusing both to site admins and to end users. Particularly confusing would be the coexistence of both models for a given content type. Is there any scenario where we would want both models to coexist for the same content type? I.e., a translation set, each member of which can have fields that are translated - back? - into multiple languages? This would be a mess. It seems like we should prevent the coexistence of both models for a given content type.
How? The cleanest way I can think of is to test the language_content_type setting. If it's TRANSLATION_ENABLED, we don't offer the Field translation tab, leaving translation to translation.module.
What fields to show on field translation edit form? Currently, the field translation edit form shows a mix of translatable and non-translatable fields. I found this confusing, particularly as there is no visual clue as to which fields are translatable. My inclination was to translate all fields on the form, but doing so produced an odd outcome: some changes were made to the original node (e.g., body), others to the translation.
The simplest and I think most intuitive solution would be to show only translatable fields on the translation form.
Theming. Currently we are in the default theme rather than the admin one when viewing lists of existing translations and when creating/editing a translation. Presumably we want to be in the admin theme.
Meaning of "source". The term "source" is used in two different ways for field translation. First, the language of the node is termed the "source" language. To change this source language, we can edit the node and change its language. (I noticed that the available languages are filtered to ensure that we can't change the node to the language of one of the existing translations. Good call.)
However, "source" is also used in a second sense, to describe the language that a translation is being created from (e.g., from English to French). This "source" can be changed on the translation overview page. At least, we probably need to use different terms for these two different concepts. But I don't know if I have a good suggestion. Maybe instead of "Source translation language" we could put "Create new translations from:".
Default text when creating a translation. I expected to find the existing source text given when I went to create a new translation (as is the case when creating a translation with translation module), but instead the field is empty. This looks like a bug. In translation module, hook_nodeapi_prepare_translation() is invoked to allow modules to provide such default values, which in some cases will differ from the source language's values. It looks like we need to invoke this hook.
Losing tabs. When creating or editing a translation, we lose our tab (local task) context.
Reusability for non-nodes. It's great to see the abstract entity_translation methods, ready to be applied to other entities. But does it merit a distinct module? Do we have any other pure API modules in core? Looking to field_translation, it's currently limited to nodes, but is that a necessary limit? My initial feeling is, no, field_translation should eventually enable translation of any fieldable entity. And so it seems to me that entity_translation could be rolled into field_translation rather than being a separate module.
Flag translations as outdated. This is a bit complicated. The current patch implements a flag in the node edit form, but not on the translation edit forms. This works if we assume the node language is the original source. But IIRC translation module offers this flag in all languages. That is, when editing the French translation of a node created in English, one can choose to flag existing translations - including the English original - as outdated. Do we need the same here? In practice this would be a switch on the translation form.
Another minor point is that the vertical tab should show the current status of the field and update when the field is updated. Suggested text: "Don't flag translations as outdated" and "Flag translations as outdated".
Overall, this patch is a great draft in the right direction and shows the strength of the great foundational work in #367595: Translatable fields.
#9
Thanks for the great review, nedjo! This patch started as a proof of concept work so I deliberately didn't address some "minor" aspects, if, as it seems, we are on the right way I'll soon have fixed all the open issues you described.
language_content_typesetting: "enabled" would enable field translation while "enabled, with translation" would enable node translation and exclude field translation.$source_language == ''the other translations have a non-empty source language. We might label the the original node translation as "original" instead of "source" and keep the current source language definition.hook_nodeapi_prepare_translation, just mess a little bit withfield_attach_form.#10
I'm *out* of this discussion (even though it's great to see it happen) ;-)
Just a quick note: "Having untranslatable field values on the field translation form might be useful, to avoid undesired modifications we might display them as readonly "
Tricky, because readonly means different things for each widget. That means we need to require every widget for every field type to support a readonly state. If we can avoid this, it's better IMO.
#11
Hm, a bit lost here :-) Can you summarize the problem ?
#12
@yched:
#10: sorry, I did not make myself understood, I meant using
field_attach_viewfor untranslatable fields instead offield_attach_form.#11: nedjo was righlty pointing out that most of the current field-based node translation could be generalized to support any fieldable entity translation, this way we could join the Entity Translation API and the Entity Translation UI into a single module.
#13
Got that part, but what's the kind of stuff that is specific to nodes and would need to be abstracted out "leveraging #488542: Tie Field UI pages to any fieldable entity" ?
#14
Well, there is a form alter which is needed to filter out existing translations from the language selector in the node form and to add some info needed in the subsequent
hook_node_updateimplementation. Moreover most of the node api hooks are implemented:hook_node_delete,hook_node_insert,hook_node_load,hook_node_update, but probably these could be replaced by theirhook_field_attach_xequivalents.I am a bit scared by the need of attaching the UI to multiple URLs, this is the part I'd mainly need support on.
#15
I implemented nedjo's suggestions from #8 except 1 and 7, I also updated the demo site
#16
the patch
#17
re #14: yes, hook_field_attach_x() should be fine for hook_node_x() equivalents. We don't have any actual use cases for them currently, so using them would provide a good safe-check ;-)
Also, note that a form_alter() should be able to recognize any form with fields by checking for the presence of $form['#fields'].
If there are other, specific needs, I guess it's possible to enrich the 'translatable' section in hook_fieldable_info().
#18
Side note: the link in #15 is wrong :-)
#19
Yesterday I was a little bit on hurry, AAMOF there were a couple of small regressions :)
I fixed them in the attached patch (the whole needed code is also provided if you wish to test it on your box), the demo has been updated accordingly.
Here is a small description of the changes in #15/16:
I reworked the translation overview to display more information about source languages and (hopefully) made some small usability improvement on the source language selector.
One little thing about this: when unistalling the Entity Translation API module the translation table is dropped but the field translations are kept. This way if it gets re-enabled the translations have to be recreated but when creating a new translation the existing field translations are loaded again as suggested values.
If we decide not to extend the UI to all the fieldable entities I think there are only of couple of issues left: handling node revisions which I'll do ASAP and taking into account the translation status (published/unpublished) which is waiting for the content language negotiation to be defined. The permission checkings also need a careful review.
While working on this I realized there are some node components which will need special attention to have fully-translatable nodes:
node.titlecolumn in the node table but this should serve only as a cache for performance purposes (a pseudo-materialized view approach) and obviously would be untranslatable, only the main language would fit in.#20
Node title I can't see being converted to a field this release (although it should be eventually). However when displaying the node title in various places, it ought to be possible to replace it with the contents of a translated field via contrib - so even if we can't deal with that in core, it ought to be possible to fix.
I can see use cases for a language column on comments regardless of field translation - sites are often listing recent comments regardless of node associations, and they could use that to filter on current language. It's also possible we'll eventually make comments into a field (so removing comment->nid and allowing a comment thread to be attached to terms or users) - at which point translatable comment fields looks like a possibility.
Taxonomy - currently we have a 'term reference' field, but haven't converted all the taxonomy_node_* to use fields yet. When that's done, it's likely that similar to node.title, it'd be easier to swap out term->name with the contents of a translated field - since we'd be overriding a formatter instead of hard-coded term->name everywhere.
#21
I forgot:
#22
Well, more generally, in an approach where translated nodes are not "different" nodes, any nodeapi addition loses translatability.
#23
@yched: yes, that's true, the problem is what additions can be achieved through field api and what not, that's one reason for which I changed my mind about the old Translation module.
Edit: however each module I have seen making additions to nodes had to face at least the need of synchronizing its values between translations, so this is not a real "shift of responsibility" to use quicksketch's definition.
#24
In the attached patch I implemented a couple of suggestions I received from Bojhan: mainly moving the source language selector in the translation page. I also started a minimum of work to support #282191-16: TF #1: Allow different interface language for the same path.
#25
@yched, "any nodeapi addition loses translatability"
Yes. I think it's okay though. This is after all "field translation". As with many other features offered by Field API but not by ad-hoc nodeapi additions, the choice for module developers becomes, if you want it translatable via field translation, make it a field.
#26
subscribe
#27
nodeapi additions not being translatable is a strength of this approach - with module like nodequeue, voting API/fivestar, flag and others, you have to add a whole extra layer of support for 'translation sets' as a distinct unique entity apart from nodes to get them to work on a single piece of content as a single piece of content. This was a major part of the CivicActions/Sony internationalisation project, and dealing with tnids was the most difficult part of that project, since it requires every single nodeapi module to refactor itself if site builders are going to have any kind of option. With translatable fields, assuming more and more modules move towards field API for attaching themselves to things (quite possible to do even if you want to maintain your own specialized storage), we'd need only a conversion to a field, not a lot of special-cased code for translation sets.
For fields themselves, the i18nsync module exists in contrib, for the sole purpose of copying field values between translation sets for when they need to be duplicated, this patch also partially grew out of #132075: Attach fields to translation sets. which tried to solve the same issue with Field API in core (although a bit differently).
Since we don't remove tnid handling (not yet anyway), this gives site builders some options on how to do things for nodes, not to mention the benefits for translation of non-nodes, when we can finally stop abusing the {locale} table with all the user interface and performance implications that has.
#28
subscribing
#29
.
#30
One detail I'm concerned about is that users setting up their site will be lost trying to choose between "Translation" module and "Field translation". We can't offer any help before the modules are enabled, so we've got only a short module description to try to convey the difference. I don't think we have any other similar case in core where we have alternate versions of comparable functionality that users can choose between.
I wonder if we could/should roll this into translation module rather than introducing a new module. It would be a bit more complicated. I guess we'd need a way to enable multi-node translation, field translation, or both. At least we could offer some reasonable help to guide admins in making this choice.
#31
#367595: Translatable fields was committed, so setting this to needs review so test bot can have a crack at it.
#32
Tagging.
#33
#34
subscribing
#35
#36
The issue has been split in two: #565480: TF #2: Multilingual field handling is the part of this patch absolutely needed to complete multilingual field handling in core. Here I'll post the UI patch ASAP.
#37
add tag
#38
@nedjo:
If we succeed in making the field translation UI available for eny entity, people will have to choose between enabling a functionality which enables translations for any field and the plain old node translation.
IMO these are not mutually exclusive. I keep having in mind a couple of examples:
We might describe the use cases above in the help. I think we could turn Translation into a simple stub module which just augments the multilingual options for the node types with the following values:
On submit Translation might install/enable one or both of the following modules: Node Translation (
translation_node, the old Translation module) and Field Translation (translation_field, the current field translation API/UI).My main doubt is about the local task titles: perhaps we might have a first local task named Translate and a couple of second level local tasks labeled Field and Node.
Surely we need Bojhan and Yoroy's feedback on this.
#39
I was thinking something similar, except that, instead of further modules, the two implementations would be in include files that are loaded as needed. But, on second thought, that could quickly get messy, e.g., when each needs separate hook implementations. We might be best keeping the current patch structure, introducing a new module, and just focus on making the module descriptions and help as clear as we can.
#40
Bojhan asked some screenshots to better understand the problem discussed in #30, #38 and #39.
My point is: field-based entity translation should become the right way to translate content so I'd prefer to use the "Translate" label for it, IMO it's more correct. How do we call the old node translation? Do we mention the translation set in any way? IMO we should make clear that with node translation we actually create new nodes. This should be reflected also by the module name, possibly, and by the module description.
catch suggested the possibility of moving the old node translation in contrib: I'd like this solution, but it won't solve the problem above.
#41
how about "page translation" (node based) and "content translation" (field based) or something similar.
#42
Here is my first attempt to make this work with any fieldable entity. Needs lots of polishing but works :)
@alexanderpas: I'm really no UX expert, we have seen that in TF1 ;)
#43
the right one
#44
This one should be ready on the coding side. Needs some documentation. I'll provide it ASAP.
#45
OK, now we should be ready to go!
#46
EntityTranslationHandler should be named EntityTranslationHandlerInterface. All interfaces should have an Interface suffix.
You should also not force-include files that contain only classes. Classes and interfaces will be autoloaded by PHP automatically.
No time right now to review other stuff, but catch pointed me here and that's what jumped out at me.
#47
OK I read through this. Can't really comment on OOP conventions but this looks somewhat similar to how entity loading does it, which got in already.
I have two main concerns here from just reading through:
1. The patch relies on etid (an internal ID for an entity type) - this is a concept currently only known to field_sql_storage.module. If we can think of this use-case for using it, it's likely contrib modules will too, so this requires possibly factoring the etid code from field_sql_storage.module upwards into field.module somewhere so that other modules can access it without making assumptions. This would be neither a schema nor API change (possibly an API additional though). Although frankly the only time I've seen etid used, was when a join on that table caused temp tables and filesorts in field_attach_query(), so I'm not at all convinced there's a benefit to having it at all.
2. The patch defines a new hook, hook_translation_info() - which merges it's results via hook_entity_info_alter(). Part of the plan for hook_entity_info() was to have a single routing hook for all info about an entity. I'm not sure at the moment whether declaring separate hooks and merging the data is a great and amazing way to avoid 2000 line hook implementations and keep things readable, or if it's unnecessarily adding hooks when technically one is enough. So just bringing it up as a point to consider.
I found a few code style/docs issues. This isn't exhaustive since I was a bit distracted by irc while looking through.
+++ modules/entity_translation/entity_translation.handler.inc 1 Jan 1970 00:00:00 -0000@@ -0,0 +1,427 @@
+ * Fix the language of the original field values.
+ *
+ * Ensure that the original translation language matches the language assigned
+ * to the original field values.
I'd rather this was "resolve" than fix - it's post-hoc cleanup rather than actually fixing a bug isn't it?
+++ modules/entity_translation/entity_translation.handler.inc 1 Jan 1970 00:00:00 -0000@@ -0,0 +1,427 @@
+ /**
+ * Return the entity type identifier.
+ */
+ protected function getEtid() {
+ // @todo: Consider avoiding the use of a field SQL storage function to
+ // identify the entity: we might have different storage engines.
+ return _field_sql_storage_etid($this->entityType);
+ }
Why do we need the etid here? As far as I know that should be completely internal to field_sql_storage.module
+++ modules/entity_translation/entity_translation.module 1 Jan 1970 00:00:00 -0000@@ -0,0 +1,360 @@
+/* Helper functions */
No need for these.
+++ modules/entity_translation/entity_translation.node.inc 1 Jan 1970 00:00:00 -0000@@ -0,0 +1,181 @@
+ * Implement hook_form_alter().
hook_form_FORM_ID_alter()
+++ modules/entity_translation/entity_translation.node.inc 1 Jan 1970 00:00:00 -0000@@ -0,0 +1,181 @@
+/**
+ * Implement hook_node_load().
+ */
+function entity_translation_node_load($nodes) {
+ // @todo: Multiple load.
+ foreach ($nodes as $node) {
+ entity_translation_get_handler('node', $node)->load();
+ }
+}
Why hook_node_load() here, but no other hook_$entity_load() anywhere else? It looks like the supporting code has been added for nodes, but not yet for everything else. In which case that's probably OK since none of that is an API change to tidy up later.
I'm on crack. Are you, too?
#48
Summary of an IRC discussion tonight:
- This still needs some work, and is not going to be RTBC by the deadline for API/feature freeze.
- While this could live in contrib now that the remaining TF patches were committed, we've seen with Field API that people don't really test out the features without a UI to bang on.
- It also seems fairly tragic for us to have this much more robust field-level translation system and no way for anyone to actually use it without turning to contrib, particularly since Drupal 6 was "the internationalization release".
- While the issue doesn't reflect it (grrr! :P), Bojhan did give this a UX review earlier on IRC. He pointed out some issues that have since been resolved and said he could live with the rest. (apparently)
- This patch is basically going to render the old one-node-per-language way of doing translations moot, in favour of field-level translation. This basically creates a double-UI which is incredibly confusing. One way suggested to circumvent this was to move the old node-based translation to contrib. IMO though, core should provide an upgrade path from the old way to the new way. This is dependent on CCK having an upgrade path in core as well (which as of now is vaporware)
I think Dries and I need to have a discussion as to the fate of this patch. Whether we extend the exception for it as a "release blocker', whether to include it under the banner of "UI clean up", or whether to hold the line and chuck it to contrib to deal with.
Please continue to work on it in the meantime; it certainly won't hurt anything.
#49
This is one of the UI's I just haven't been able to follow/review. Bojhan has been working with plach on it, but didn't comment here.
Two remarks:
1.FWIW, quickly scanned the screenshots in #40 and they don't seem to scary. Interface text very important here, needs work
2. trategically I might prefer to add a UI for another use case with field-level complexity interactions. Just so that core has this problem to solve, not contrib. If smallcore + ui layer is the way for D8, then translatable fields UI is one for the core ui layer to provide patterns for, not contrib.
As such, for UX future this is worthwhile to try and get committable.
#50
The last submitted patch failed testing.
#51
#52
The last submitted patch failed testing.
#53
The attached patch implements the suggestions from #46-#47. It also adds a configuration page where choose on which fieldable entities enable field translation as it makes translation available for entitities not implementing
hook_translation_info().For example for a comment the translation UI is available at the address:
http://example.org/entity/comment/1/entity-translationAs in the case of comments there might not be a usable UI to attach the translation UI to, we might want to implement an additional page (similar to the string translation page) on which enter the entity type and the entity id (maybe through an autocompletion widget) and access the above URL. We absolutely need a UX expert's feedback about this.
#54
@catch:
I totally agree on this.
In this case we need to collect all the translation information before returning from
hook_entity_info_alter(), as we must provide defaults where there are missing values.#55
Many of this is above my head, here are a couple notes from a cursory look at the patch.
+++ modules/entity_translation/entity_translation.handler.inc 1 Jan 1970 00:00:00 -0000@@ -0,0 +1,507 @@
+ * Return TRUE if the entity translation is actually being translated.
"translation being translated" ?
+++ modules/entity_translation/entity_translation.handler.inc 1 Jan 1970 00:00:00 -0000@@ -0,0 +1,507 @@
+ * @param $transation
typo
+++ modules/entity_translation/entity_translation.handler.inc 1 Jan 1970 00:00:00 -0000@@ -0,0 +1,507 @@
+ foreach (field_info_instances($this->entityType, $bundle) as $instance) {
+ $field_name = $instance['field_name'];
+ $field = field_info_field($field_name);
+
+ // do something on $this->entity->{$field_name}
+
+ }
The pattern in field API (more specifically field.attach.inc, which controls field operations) is to use field_invoke() for those "do something on all the fields of an object". Not sure this can easily be applied here, but it's a bit sad to see all those foreach duplicate the work of the field_invoke() iterator.
+++ modules/entity_translation/entity_translation.handler.inc 1 Jan 1970 00:00:00 -0000@@ -0,0 +1,507 @@
+ foreach ($entity->{$field_name} as $lang => $items) {
+ if ($lang != $langcode) {
+ $this->entity->{$field_name}[$lang] = $items;
+ }
+ }
This looks like a no-op to me, but I'm probably missing something.
A comment might be good.
+++ modules/entity_translation/entity_translation.module 1 Jan 1970 00:00:00 -0000@@ -0,0 +1,422 @@
+ 'entity key' => '#node',
Just a note that I think #367006: "Add more" broken without JS / generic handler for Field's "add more" button makes $form['#{entity_type}] standard for entity forms.
+++ modules/entity_translation/entity_translation.module 1 Jan 1970 00:00:00 -0000@@ -0,0 +1,422 @@
+ 'object keys' => array(
+ 'humanReadableId' => 'title',
+ ),
should this be camel cased ? the rest of the translation_info() array uses simple space separated strings as keys.
+++ modules/entity_translation/entity_translation.module 1 Jan 1970 00:00:00 -0000@@ -0,0 +1,422 @@
+ variable_set('entity_translation_edit_form_info', $edit_form_info);
Why is this part stored separately ?
+++ modules/entity_translation/entity_translation.module 1 Jan 1970 00:00:00 -0000@@ -0,0 +1,422 @@
+ 'entity translation' => array(
+ 'title' => t('Translate entities'),
perm machine name seems off - why not just 'translate entities' ?
Side note : do we need separate, per-entity-type permissions ? (as is : common users should be able to translate their own profile info but not actual nodes)
+++ modules/entity_translation/entity_translation.module 1 Jan 1970 00:00:00 -0000@@ -0,0 +1,422 @@
+/**
+ * Implement hook_field_attach_pre_insert().
+ */
+function entity_translation_field_attach_pre_insert($obj_type, $object) {
...
+function entity_translation_field_attach_pre_update($obj_type, $object) {
hook_field_attach_pre_[insert|update]() : those hooks, although misnamed, were originally targeted at 'advanced' storage tricks (like per-bundle storage). Can this use the regular hook_field_attach_[insert|update]() (non-"pre") hooks instead ?
Edit: er, those hooks are currently missing from field_attach_[insert|update]() - well, it's a perfectly valid use case for adding them, then.
I'm on crack. Are you, too?
#56
The last submitted patch failed testing.
#57
This one implements most of yched's suggestions from #55, it also introduces comment filtering and translation authoring info handling (see the next post). Some notes:
I thought a bit about this, to re-use
_field_invoke()(which should becomefield_invoke(), then) we would need some refactoring, mainly the ability to specify a callback, possibily through$options['callback']. AAMOF the current behavior is strictly tied to the assumption that the only pieces of code defining callbacks are: 1) the module implementing the field being processed, 2) the field API itself through thefield_default_*()callbacks.You're quite excused ;) as I used really poor variable names (
$entityvs$this->entity). Changed them.Just subscribed to the issue, I'll stay tuned :)
Because we need a data structure keyed by form id to avoid iterating through the entity info at each form alter.
This one implements per-entity permissions, although I think access control should be mainly focused on entity instances rather than on the entity types.
We need a separate issue here, I suppose.
The module introduced by this patch is probably the first piece of code exploiting the Field API which dose not actually focus on a single entity type. This way it makes evident some needs that might be common for future modules. This is particulary clear in three aspects:
<?php
'node' => array(
'translation' => array(
'entity_translation' => array(
'class' => 'EntityTranslationNodeHandler',
'base path' => 'node/%node',
'edit path' => 'node/%node/edit',
'path wildcard' => '%node',
'alias' => TRUE,
'access callback' => 'entity_translation_node_tab_access',
'access arguments' => array(1),
'edit form' => array(
'form id' => 'node-form',
'entity key' => '#node',
),
),
),
'object keys' => array(
'human readable id' => 'title',
),
?>
should become:
<?php
'node' => array(
'translation' => array(
'entity_translation' => array(
'class' => 'EntityTranslationNodeHandler',
'access callback' => 'entity_translation_node_tab_access',
'access arguments' => array(1),
),
),
'object keys' => array(
'human readable id' => 'title',
'base path' => 'node/%node',
'edit path' => 'node/%node/edit',
'path wildcard' => '%node',
'alias' => TRUE,
'edit form' => array(
'form id' => 'node-form',
'entity key' => '#node',
),
),
?>
and all the
'object keys'above should be defined in the very entity definition and not added later as alterations.hook_field_attach_*()implementations: these hooks are currently the only way to write code that works for any entity, with the exception ofhook_entity_load(); the problem is sometimes one has to callfield_attach_X()in ahook_field_attach_X()implementation and this potentially leads to infinte recursion. The solution here would be havinghook_entity_X()but this not possible for D7 so we have to find a best practice to address this situation, which might become fairly common.field_sql_storagemodule.#58
The last submitted patch failed testing.
#59
really weird, rerolled
#60
FYI, created #622534: Cleanup in hook_field_attach_*().
#61
The current patch fixes a couple of bugs and string issues. As it implements all the features I originally planned for entity translation, I am providing here a summary of the current status. If we exclude some possible Field API cleanup and some usability improvement I think this is ready to go (it is still to be decided if in core or in contrib). As webchick was pointing out in #48, if this has to go in core we still have to find a solution for the old node-based translation issue.
The aim of this patch is providing a way to translate any fieldable entity. The UI design has been derived from the node-based translation UI with some adaptation. AAMOF one of the goals of this patch is emulating all the node-based translation (core) behaviors and replacing it, as providing entity translations as facets of the same entity has been deemed more correct than having different entities for each translation. While discussing this solution we found that also the former approach does have some use cases, so our inclination would be to move the current translation module in contrib and replace it with the field-based entity translation.
The patch introduces a new module,
Entity Translation, which allows users with the appropriate permissions (tf-permissions.png) to translate the fields of any fieldable entity. It provides the concept of entity translation, which you can think of as a view on the entity's field content in a particular language. While creating a new translation all the new field values are assigned the same language, e.g. if you are entering a french translation all the translatable field values are marked as french. Untranslatable fields are left alone. The field values actually displayed depend on the negotiated content language.The patch provides a unified UI for translation, but each entity has its own UI to edit the original field values, e.g. node has the usual node edit form, taxonomy has the term editing form and so on. Everytime a new revision is created for the original field values, a corresponding revision is created for each field translation, this way
revision_ids are always synchronized between field translations.The translation UI is attached to each entity's UI as the Field UI already does. It provides two main pages: the translations overview (tf-translate-overview.png), which lists all the available translations, and the translation page (tf-translate-en-it.png), which let the user insert the field translated values. The user can choose from which language translate, if more than one source translation is available (tf-translate-fr-it.png). While editing a new content the user can choose to mark its translations as outdated (tf-edit-outdate.png), from the translation page it is possibile to mark the translations as updated (tf-translate-en-it.png).
If a translation is not available, i.e. it does not exist or the user has not the permission to access it, there are two possible behaviors: if language fallback is enabled (default behavior) language fallback rules will be applied and fields will displayed in an existing language (tf-view-it-fallback.png). If language fallback is disabled (Locale currently provides no UI for this) a system message is displayed warning the user that the requested translation is not available (tf-view-it-unavailable.png); this behavior works also when displaying multiple entities and is provided through a theme function, so it can be overridden.
To fully achieve the emulation of the node-based translation we made different translation appear as different nodes, when possible. With this in mind several functionalities were implemented:
This patch supports any fieldable entity and allows to choose for which types actually enable translation (tf-config-entity-types.png), but different entities may get a different level of support, depending on whether or not they were fully converted to Field API:
To have this patch committed to core we need to make an upgrade path available. IMO this upgrade path should be optional: people wishing to keep using the node-based translation should be able to do so by simply installing its contrib version; instead people wishing to convert their sites to entity translation could run the upgrade path and then live happy. For this reason I suggest to implement the upgrade path as an additional entity translation submodule which would also take care of the possible broken internal links left by node translations converted to fields, e.g.:
Before the upgrade:
http://example.org/node/1 [english version]http://example.org/fr/node/2 [french version]
After the upgrade:
http://example.org/node/1 [english version]http://example.org/fr/node/1 [french version, a request to /fr/node/2 should be permanently redirected to /fr/node/1]
Usability and design feedbacks are warmly welcome :)
#62
I have some questions mainly related to usability issues:
<?phpif (!$field['translatable']) {
$form[$field_name] = array(
'#markup' => drupal_render($field_view[$field_name]),
// Place the element where it would appear if displayed.
'#weight' => $instance['display']['full']['weight'],
);
}
?>
#63
So, I have actually given this a big review a few weeks ago - going trough a lot of interfaces with plach. I think this is a good state to go from, there are some minor text issues - but I don't see much value in working on those here.
@peximo
2. Seems somewhat over-done, to put these in a vertical tab. These are clearly, and will only be two elements - vertical tabs is not supposed to be used for that.
3. Should see a fix.
4. Could bloat the table?
#64
That would mean we need to require each widget to support a 'disabled' state. A little bit of work, outside the scope of this issue.
I agree, however, that the "test / test" text on translate entity form currently looks confusing. We should probably at least display the field styled with the regular field.tpl.php output and CSS. That would mean moving forward on #612894: API functions for 'display field'.
#65
I was unsure whether we want to add this to core. However.
After working on some other patches (f.e. #301902: Allow more users to see the node admin page), I realized that I have absolutely no clue how the field structure of a translatable field looks like and how to code for it when it is populated with translations. Even after being involved in all these TF patches. That's a major problem. And I have no clue what strange/wrong code we will see in contrib, if contrib developers don't have something to play + understand as well.
Hence, my point is that there is no way around committing this. So we need to get the implementation into RTBC state as soon as possible. UX tweaks can follow afterwards.
Honestly, I didn't expect so much OOP in here. I guess it makes sense, and I really like how the module alters itself into the otherwise untouched entity handling. But it also means that we need Crell or some other OOP masters to review this patch. I will try to review and test this patch more in-depth this weekend.
#66
And here's a (apparently) working upgrade path. It needs feedback on design and implementation (see #61 for details) and more testing but the results are encouraging :)
The rest of the code is identical to the patch in #61.
#67
Reading #61 once again, I'd say that we want to move Translation module into contrib (as node_translation module), and at the same time, make this patch replace translation.module in core. "Entity translation" does not sound like something I'd want to enable as a regular user. But a "Translation" module that allows me to translate fields everywhere - that, I absolutely want to enable. :)
#68
Agreed with sun. The core translation module has very few open issues, and hasn't had many over the whole of the D7 release cycle. It shouldn't require a huge maintenance burden during the release cycle, and if Jose didn't mind, we could move it back to the i18n project maybe?
While we could write an upgrade path for body (and title if translatable titles gets in), doing so for CCK fields relies on the overall CCK to Field API upgrade path, with additional tweaks, so there's a node translation -> translatable entity conversion, CCK -> Field API conversion, then CCK/nodetransation -> Field API/Entity translation patht to wrangle. But keeping node translation in contrib means there's an upgrade path regardless.
#69
yeah, i18n_node sounds even more appropriate. I totally forgot that it originally came out of it.
Created #627734: Take back Translation.module?
@webchick: Would you be ok with this?
#70
Two things bug me in the patch:
function entity_translation_overview() {if ($handler->initOriginalTranslation()) {
field_attach_update()
}
}
I sort of gather this is in order to do some verification / cleanup / preparation, but updating field data on a simple page view sounds scary.
{entity}_save() - field_attach_update() - hook_field_attach_pre_update() - $handler->updateTranslations() - $handler->prepareRevision() - field_attach_load()Load within a save, and that deeply nested, sounds like a recipe for strange effects and hair-pulling debug debug...