This is a follow-up in response to #1188388-108: Entity translation UI in core comment 105 and comment 108
Problem
Currently in the D7 version of ET we have the capability of marking each translation as individually published or not and providing additional translation metadata. We may want to retain this feature also in D8.
Proposed resolution
We will add this metadata to any entity as additional multilingual properties. At the moment only nodes natively provide this kind of information, which will be multilingual as soon as #1498674: Refactor node properties to multilingual. The idea is that nodes override the default translation metadata values with their native multilingual metadata. In a follow-up we could explore the possibility of extending these metadata to any entity regardless of translation.
Remaining tasks
Define which strategy we want to implement.- @catch requests more reviews
- Reach the RTBC status.
User interface changes
UI changes need to be described.
API changes
Are there API changes/additions that would affect module, install profile, and theme developers?
Background/Original Issue
cut and pasted from et ui part 2 issue summary to preserve knowledge:
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?
Comment | File | Size | Author |
---|---|---|---|
#65 | et-metadata-1807800-65.patch | 38.34 KB | plach |
#60 | et-metadata-1807800-60.patch | 38.33 KB | plach |
#56 | et-metadata-1807800-56.patch | 38.34 KB | plach |
#54 | et-metadata-1807800-54.interdiff.do_not_test.patch | 9.82 KB | plach |
#54 | et-metadata-1807800-54.patch | 40.11 KB | plach |
Comments
Comment #1
plachComment #1.0
plachUpdated issue summary. correct the direct link to comment 108
Comment #1.1
plachUpdated issue summary
Comment #1.2
plachUpdated issue.
Comment #2
YesCT CreditAttribution: YesCT commentedThis one is tagged with feature freeze but has no patch yet. @plach, did you have something started, or should anyone jump in?
Comment #3
plachI'm planning to post a patch for this during the upcoming weekend. @bforchhammer will help us with code reviews and coding, if needed.
Comment #4
plachThe attached patch takes care of the issue (it's a port of the D7 code) and adds also support for authoring information in addition to the translation status. In core nodes are the only entity type already supporting these properties hence in this case the property values are inherited from the node ones and are not exposed on the entity form.
Tests missing.
Comment #5
plachFixed a couple of issues with the previous patch.
Comment #6
plachBetter title
Comment #7
attiks CreditAttribution: attiks commentedThis looks good to me, leaving at NR so we can get some more feedback.
Comment #8
YesCT CreditAttribution: YesCT commentedwhy not just make this an if?
Comment #9
plachI find it more readable this way.
Comment #10
bforchhammer CreditAttribution: bforchhammer commentedHaven't tested it properly yet, but looks good overall. A few comments...
Can we make the variable name (array key) more clear by leaving the "re" prefix in the name of the array key as in 'retranslate', or possibly rename it to 'outdated'?
I think this is equivalent to:
I'm guessing this will change once #1807776: Support both simple and editorial workflows for translating entities is done? (Add edit permissions? Move logic into
getTranslationAccess($entity, $op)
?)This may be a stupid question, but why are we cloning the entity here?
This seems to be a "type conversion" of values from boolean to integer (not a "reordering"). Is this really necessary? I would have expected the DB layer to take care of simple type conversions :)
Comment #11
plachThanks! This should take care of #10 and add test coverage:
Yep, totally.
Because we are unsetting field values to trigger language fallback. We don't want them to be lost if the entity is saved afterwards :)
Yes, it's necessary: without it you'll get DB exceptions all the time. It's actually a reordering because the values are inserted into the array in the order expected by the schema.
Comment #12
bforchhammer CreditAttribution: bforchhammer commentedI just tested this again and this looks pretty good. RTBC as far as I'm concerned :)
typo
I stumbled on that error when testing some other patch and created a new issue for it: #1852394: Fatal error: Call to undefined function path_delete().
Comment #13
plachTypo fixed. Anyone daring to RTBC this?
Comment #15
bforchhammer CreditAttribution: bforchhammer commentedHere we go.
Short summary: patch adds
two new translation properties: publication status and translation author4 translation properties: status, author, creation date and modification date. For nodes, these inherit from the respective existing node properties. All properties are covered well in tests. :)Comment #16
plach#13: et-metadata-1807800-13.patch queued for re-testing.
Comment #17
plachActually 4 translation properties: status, author, creation date and modification date :)
Comment #18
plachComment #19
catchThe description here is a bit confusing. If you're in the translation UI and you really wanted to unpublish the entire node in the end, how do you get from here to there assuming you have the permissions?
I'm a bit confused about how this gets saved. Isn't it blocked on #1498674: Refactor node properties to multilingual?
OK so this is where it gets saved without the multilingual patch, but this seems a bit confusing - won't we end up with a field for 'author' and another field for 'translation author' then?
Comment #20
plach@catch:
This feedback makes me think the issue summary could use some improvement :)
What I'm proposing is that we provide these additional translation metadata to support editorial workflows for translation. At the moment only nodes natively provide this kind of information, which will be multilingual as soon as #1498674: Refactor node properties to multilingual is done. The idea is that nodes override the default translation metadata values with their native multilingual metadata. In a follow-up we could explore the possibility of extending these metadata to any entity regardless of translation.
For nodes there will be no way to "unpublish" the node altogether, it will have a per-language status. AAMOF this part of the UI is hidden for nodes, which only have the form elements for the native metadata.
At the moment this behavior is semibroken, because the translation metadata for nodes is actually stored in a multilingual fashion, but it is overridden with the native metadata which is not multilingual yet, hence it is reset each time the node is saved.
With the upcoming feature freeze I thought this was a reasonable approach, since the ML nodes patch is still blocked on other refactorings, however now we might want to postpone this to wait to get a correct behavior. OTOH the code here should start working correctly without any other required change as soon as the ML nodes patch is committed, so we may want to go on and commit this.
I don't know what kind of work is needed here. Could you clarify, please?
Comment #21
YesCT CreditAttribution: YesCT commentedComment #21.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary preserve et ui part 2 issue summary knowledge
Comment #22
plachSetting back to RTBC as there is a (remote) possibility that #20 actually solved the concerns raised by @catch. This is just to ensure the issue doesn't get out of his radar.
I'm totally available to rework anything requiring it but I need more information.
Comment #23
catchOK I still don't get it.
Let's just take the uid field:
Is this:
1. {node}.uid but allowing a translation to refer to a different author?
2. The uid of the person who actually added this translation?
Similar problem to #1528028: Add tests for reverting revisions where revision_uid and uid differ where the column is ambiguous.
Comment #24
plachIn the current proposal
{node}.uid
(let's suppose this is already ML) and{translation_entity}.uid
hold the same (duplicated) information, meaning that the translation they refer to has been authored by{user}.uid
.Any other entity will have this information stored only in the
{translation_entity}
table.This implies that the UI for any entity except nodes will have the form elements letting the user enter the translation author, while nodes will have only their usual author widget, just varying by language.
Comment #25
plachTrying again :)
Comment #26
catchWhat happens if you want to track the translation authors for nodes in addition to translating the author?
Comment #27
plachIn this case we are storing multilingual user references, I am not sure I get what you mean with "translating the author" :)
Comment #28
plachLet me add that there is already a planned follow-up (#1810370: Entity Translation API improvements) that might turn the data we are adding here into true multilingual properties of the entity they are attached to. The alternative plan, which is the one I like more right now, is making the records in the
translation_entity
table actual (non-translatable) entities. This would help a lot with per-language hooks and single-entity access control.Comment #29
catchIt means that in one case it's a "multilingual use reference", and in another case it's "metadata about the author of a translation". I still think it's too odd that exactly the same information means two different things. Marking CNR again in the hope of more reviews.
Comment #30
plachI don't think they mean two different things: they both are metadata about the author of the content in the particular language they concern, on the contrary I see them as exactly the same information. However I'll ask Gabor to chime-in here.
Comment #31
Gábor Hojtsy@catch: once the node property multilingual issue is resolved, we could track authors per language. Those would be the node authors, not the revision authors to relate to the similar issue you cited. (The node.uid is neither the user who created the node, it is the user who the creator wanted to credit the node to, right? :) So the translation uid has the same meaning as the node uid, it is the translator user who was credited for this translation.
I did not look at the internals of this patch, but @plach noted above that once that node issue is resolved, this patch would have no effect on the behaviour there and simulate that behaviour for other entities. Let's take taxonomy terms. Those do not have status or authors, so if we'd keep it like it is, their translations would equally be "published" right away. This issue attempts to solve that problem by introducing status for translations only.
This looks like generally useful for entities which do not have status by themselves. In core, I think that is taxonomy terms and vocabularies only, and vocabularies are going away from being content entities. This could however apply to other contrib entity types too. An alternative is to say all entities are required to provide status and status should be a multilingual property. Another alternative is to say those entities that in themselves do not support publication status should not have support for translation workflows where the translation might be unpublished or hack around the system in contrib some other way. That would keep the separate handling of status-enabled and status-less entities on the translation level.
Hope this helps.
Comment #31.0
Gábor HojtsyUpdated issue summary
Comment #32
plach@catch:
It would really help if we could get this committed or marked back 'needs work' with some indication on the things to fix.
Comment #33
plachrerolled
Comment #34
Gábor Hojtsy@catch: any updated feedback based on the explanation would be very welcome! Thanks a ton!
Comment #35
Gábor HojtsyAlso bringing on D8MI sprint board.
Comment #36
YesCT CreditAttribution: YesCT commented#33: et-metadata-1807800-33.patch queued for re-testing.
Comment #38
catchRight, one of these is along the lines I was thinking, but I'm not sure which is the best option, however since this confuses me I'd not be surprised if it confuses others too.
I'd really like to get a review from someone here who's 1. not me, 2. not plach or Gabor, just to see if it's just me who thinks this is odd. Assigning to fago for a bit but doesn't have to be fago exclusively.
Comment #39
plach#33: et-metadata-1807800-33.patch queued for re-testing.
Comment #41
fagohm, I think that this is ok. Having a "multilingual user reference" just says that you store a user reference per language - the meaning we put into it is left to us. Thus, in the multi-lingual node author case we would have the meaning of "the author of the node in a certain language". That makes sense to me.
Yes, this is an interesting question to figure out. Personally I think that this kind of editing metadata should not be stored inside the entity, but outside of the edited entities. I'd only store it with the entity if it's to be considered as part of the entity, e.g. to a node ("document") the author and the creation date is clearly crucial information and part of the entity although it can be considered metadata. It's more than the CMS having metadata of who created/edited when/what. That this information is displayed with nodes is a good indicator for the importance of that information and I agree with Gabor that it's a valid and important use case to be able to specify custom information for that (what might differ with the reality). Thus, an editor could go and post an article specifying a custom creation and author information while the CMS should still track who really created/edited when/what.
Thus, who created/edited when/what is metadata we want to keep generally, thus should be stored in its own (simple) non-translatable entity as plach suggests.
But then, I see two use-case this entity should fullfill:
a) A simple way to lookup editing metadata, e.g. when was this term last edited?
b) Provide a log of who edited when/what.
For a) I think we should have a similar table as created by this patch, but as its own simple (untranslatable, unrevisionable?) entity as plach suggests. We can make looking-up this information easy by e.g. adding a computed-field, so you could access it via $entity->metadata->created or so, while the editing-metadata entity gets lazy-loaded.
b) reminds me a bit of message module: A separate logging entity with customizable storage-fields and an easy way to generate a human-readable version of the log. While it would be nice to have something extensible like the message module API inside core, it's probably to much to aim for now. A more straight-forward (but unfortunately not extensible) alternative could be just using revisions on the editing-metadata entity.
However, what imho does not 100% fit into the editing-metadata concept is translation metadata like "is the translation published" or "is it outdated"?
First off, imo the entity in default revision should only contain up2date information - having an outdated value in there would be weird as the API does not support the concept of having outdated values - so developers would not care.
Thus, if the translation is so out of date such that it shouldn't be used/displayed anymore, it should be removed in the revision which adds new information that out-dates the translation. Still, another revision can be prepared that comes with a new updated translation for the given language - with our possibility to track non-default forward revisions this should be solvable in contrib.
If the translation still should be shown/used, but needs another check due to the update - then this is information specific to the translation workflow. Thus, it shouldn't be part of the general editing-metadata by default, but information that the translation workflow should care to track ( - it could still extend the editing-metadata entity type?) That way a contrib translation workflow module could do something completely different.
Summary of my 2 cents:
- We should move editing metadata to its own entity. (We already talked about that at the wscci web service format sprint in Paris last May).
- Imo publishing status is something different and should be handled by entity type, and/or added by contribs. It makes sense to track a publishing status for nodes, but it doesn't for e.g. flag entities or message log entities (greetings from the message module). Probably it makes sense to keep that stored with the entity.
- Translation workflow metadata is different to general editing metadata and differs by the translation workflow used. Storage should be up to the translation module in use.
Comment #42
Gábor Hojtsy@fago: sounds like a conceptually sound solution however it is a significant development effort. Do you think this is solvable in the remaining month before feature freeze? Would be good to see cross-opinions from others :) @catch? :)
Comment #43
fagoI think so, however I won't be able to push it myself as I'll be busy with remaining NG-conversions for a while :( Maybe plach or someone else would like to take over the lead here?
Comment #44
YesCT CreditAttribution: YesCT commentedDo we want to open a new issue for that, and postpone this to see if we can get it done the new way?
Or... update the issue summary and remaining tasks to clarify we are taking a new approach here. (since we have a limited number of fagos and plachs :) , any clarification and detail in next steps will help)
Comment #45
plachHonestly I'm a bit confused too now, so I'd like to rehash all of this to set a clear roadmap taking deadlines into account. I'll get in touch ASAP.
Comment #46
YesCT CreditAttribution: YesCT commentedthis was mentioned in response to the user testing issue because this will add items to the translate vertical tab
Budapest Usability Testing Results
http://groups.drupal.org/node/271918
Comment #47
plachRerolled
Comment #49
plachFixed a couple of issues with BC entities. Now tests should be passing again.
[Comment moved to #52]
Comment #51
plachNow for real :)
Comment #52
plachReverted debug leftover. The interdiff is against #49.
Comment #54
plach#1751606: Move published status checkbox next to "Save" went in meanwhile...
It's not clear to me whether according to #41 we can get this patch back to RTBC or we should refactor it. Personally I'd go with this as-is and refactor it once the needed API is in place. I'm afraid that if we wait for a complete API to be there, this won't be ready by the end of the feature completion phase.
Comment #55
plachTypo to be fixed on the next reroll.
Comment #56
plachRerolled after #1807776: Support both simple and editorial workflows for translating entities went in.
Comment #57
plachThere was an IRC meeting today where we decided how to proceed with #1810370: Entity Translation API improvements. With the current proposal entity translation metadata would be regular multilingual entity fields living on the canonical data table.
In this scenario entities (node!) could alter their field definitions to skip any duplicate translation metadata. This way we'd avoid the confusion this issue has been generating so far.
That said I respectfully ask to mark this back to RTBC and commit it as-is. We will remove duplicate properties as soon as the NG conversion is over and we can proceed with the issue above.
Comment #58
YesCT CreditAttribution: YesCT commented#56: et-metadata-1807800-56.patch queued for re-testing.
Comment #60
plachRerolled
Comment #62
plach#60: et-metadata-1807800-60.patch queued for re-testing.
Comment #64
plachThe bot failure is caused by a core bug which is being exposed by the new tests introduced here. We need #1899994-35: Disentangle language initialization from path resolution to be committed to make tests pass again.
Comment #65
plach#1899994: Disentangle language initialization from path resolution went it, here is a reroll. Spoke with @catch in IRC and he said he might be ok with #57. Can we get this back to RTBC so he can have the final call?
Comment #66
Gábor HojtsyI think people above agree that this is an interim step and would be refactored later if possible. Otherwise the code looks as good as it can get AFAIS.
Comment #67
YesCT CreditAttribution: YesCT commented#65: et-metadata-1807800-65.patch queued for re-testing.
Comment #68
catchOK I went ahead and committed this. Tagging with revisit-before-release so we can check if things are back by the time that's done.
There's no new indexes added with the new columns. I can see there's no new queries added either except writing to the table but do we have an idea what queries are likely to be run and if so could some generic indexes that'd cover those be added?
Comment #69
YesCT CreditAttribution: YesCT commentedRevisit tag did not stick. Trying again.
Comment #70
plachHere is the change notice: http://drupal.org/node/1916780.
Comment #71
plachRemoving from sprint, thank you all!
Comment #72
plachCreated #1916790: Convert translation metadata into regular entity fields as follow-up.
Comment #73.0
(not verified) CreditAttribution: commentedclarify that more reviews are needed
Comment #74
catchUntagging this, the entity schema generation has tidied all of this up.