Follow-up to: #1188388: Entity translation UI in core. Work is ongoing in the D8MI - Entity Translation sandbox.
Problem
The current Entity Translation API is:
- Inefficient: To determine whether a translation exists, all fields are scanned (each time) to collect all language-specific values.
- Fragile: When invoking
EntityNG::getTranslation($langcode)
a copy of each field value in the given language is attached to the returnedEntityTranslation
object. If there is no value for the given language, fields are initialized with an empty vaule. This way the next timeEntityNG::getTranslationLanguages()
is called,$langcode
erroneously appears as an existing translation. - There is no API to create or delete a translation, and no hooks are fired to allow to react to these events.
- There is no concept of "active language"; i.e., the language of the translation being manipulated. Therefore, the active language has to be passed forward in a way that is bad for DX and testability (see the API Changes section below).
- If the active language is not passed forward, all code falls back to the default language.
- If the active language is passed forward, every line of code dealing with entities needs to retrieve it and explicitly take it into account.
Currently we would need to write something like this:
<?php // Retrieve the active language in some way and put it into $langcode. if ($entity->language()->langcode == $langcode) { // Do something on $entity. $entity->foo->value = 'bar'; } else { $translation = $entity->getTranslation($langcode); // Do something probably identical on $translation. $translation->foo->value = 'baz'; } $entity->save(); ?>
Or simply always do something like:
<?php // Retrieve the active language in some way and put it into $langcode. // Do something on $entity. $entity->getTranslation($langcode)->foo->value = 'baz'; $entity->save(); ?>
- There is no indication of which translations exist for an entity at the storage level, thus we have no table to reliably join on when needing to filter queries per language.
Goal
- Provide a clean way to deal with translations in the Entity Translation API.
- Improve Views integration.
Details
- #1260640: Improve field language API DX already concluded that it is OK to for all code to act on a single language, but the code needs to know which one.
- Code that needs to act on all languages (rare) can use
Entity::getTranslationLanguages()
to retrieve all languages. - The current
EntityNG::getTranslation()
method returns an instance of theEntityTranslation
class which represents a "facet" of the entity in a particular language.
Proposed solution
- Add the following methods:
TranslatableInterface::getOriginal();
TranslatableInterface::hasTranslation($langcode)
TranslatableInterface::addTranslation($langcode, $values = array())
TranslatableInterface::removeTranslation($langcode)
TranslatableInterface::getTranslationStatus()
(created/existing/removed)
- After adding or removing translations, when storing the updated entity, fire these hooks:
hook_entity_translation_create()
hook_entity_translation_delete()
- Remove the
EntityTranslation
class and refactor the EntityNG object ato make it return a shallow clone of itself when a translation object is retrieved throughEntityNG::getTranslation()
(see #30 and #38 for details). The only difference for API consumers is that the value returned byEntityNG::language()
will be different based on the language of the translation object (the active language). When retrieving a translation corresponding to the entity language the entity object itself is returned. To get the entity language we just need to retrieve the original entity object:$translation->getOriginal()->language()
. - Rely on the entity storage to track the existence of translations through a separate data table for each translatable entity type. It must contain a row for each available translation; e.g.:
| entity_id* | langcode* | default_langcode |
This table could also hold translation metadata information, see #1807800: Add status and authoring information as generic entity translation metadata.
Implementation notes
One tricky aspect to address in the current proposal is dealing with translation removal. When removing the translation corresponding to the active language, we need to invalidate the translation object, by setting the translation status to removed. Any invocation of the (magic) accessor methods following a translation removal implies throwing an exception: basically the translation object becomes unusable and a new one needs to be instantiated. Since the need to reuse a translation after removing it should be pretty rare, being strict and not allowing it directly will avoid unexpected behaviors.
As a sidenote there is consensus that language fallback logic does not belong to the entity/transation objects. See #2019055: Switch from field-level language fallback to entity-level language fallback for details.
API changes
Before
See the realted issue: #1807692-43: Introduce a column synchronization capability and use it to translate alt and titles through the image field widget.
function translation_entity_field_attach_presave(EntityInterface $entity) {
if (translation_entity_enabled($entity->entityType(), $entity->bundle())) {
$attributes = drupal_container()->get('request')->attributes;
translation_entity_sync($entity, $attributes->get('working_langcode'), $attributes->get('source_langcode'));
}
}
After
function translation_entity_field_attach_presave(EntityInterface $entity) {
if (translation_entity_enabled($entity->entityType(), $entity->bundle())) {
translation_entity_sync($entity, $entity->language()->langcode, $entity->source->value);
}
}
The EntityTranslation object wraps the entity. This greatly simplifies the Entity API DX, because we introduce the concept of "active language" without introducing a state in the entity object.
Blocking this issue
We are postponed (in #20) on "various NG conversions".
Related issues
- #1188388: Entity translation UI in core
- #1260640: Improve field language API DX
- #1810330: Remove EntityTranslationControllerInterface::removeTranslation()
- #1807800: Add status and authoring information as generic entity translation metadata
- #1807692-43: Introduce a column synchronization capability and use it to translate alt and titles through the image field widget
- #1836086: [meta] Entity Translation UI improvements
- #1916790: Convert translation metadata into regular entity fields
- #1391694: Use type-hinting for entity-parameters
- #2019055: Switch from field-level language fallback to entity-level language fallback
Comment | File | Size | Author |
---|---|---|---|
#132 | et-api-1810370-131.interdiff.do_not_test.patch | 3.14 KB | plach |
#132 | et-api-1810370-131.patch | 100.92 KB | plach |
#128 | et-api-1810370-128.interdiff.do_not_test.patch | 2.95 KB | plach |
#128 | et-api-1810370-128.patch | 100.74 KB | plach |
#126 | et-api-1810370-126.interdiff.do_not_test.patch | 4.4 KB | plach |
Comments
Comment #1
YesCT CreditAttribution: YesCT commentedBackground
Originally in #1188388-19: Entity translation UI in core
plach
And responded in #1188388-81: Entity translation UI in core
Gabor
And responded in #1188388-82: Entity translation UI in core
plach
Comment #2
plachComment #3
plachComment #4
plachComment #5
fago+1 for introducing translation "CRUD hooks. This is what the UI and the concepts do already, so having hooks to respond to it would be reasonable. E.g., you could easily log "translation created".
hook_entity_translation_create()
hook_entity_translation_delete()
I don't think we need hook_entity_translation_insert/update() though, as we can always have $entity being updated there. I'd see use introducing an easy way to determine which fields changed there - by language code? That should make it easier to write code working with translations as you don't have to specifically implement the entity and the translation update hook but handle all cases in one hook implementation working with all translations?
I don't think we need a loading hook either (instead I'd love to consider removing hook_entity_load()).
Comment #5.0
plachUpdated issue summary
Comment #6
plachAfter thinking about this for quite some time and actually trying some experimental code, I felt that the previously proposed approach was not going to work. Hence I updated the issue summary with what I think should be the proper way to address this. I'm also slightly broadening the scope of this issue, since a couple of related problem were previously left out.
Promoting to major since solving this is of fundmental importance for lots of different issues concerning ET.
Comment #6.0
plachUpdated issue summary.
Comment #6.1
plachUpdated issue summary.
Comment #6.2
plachUpdated issue summary.
Comment #6.3
plachUpdated issue summary.
Comment #6.4
plachUpdated issue summary.
Comment #6.5
plachUpdated issue summary.
Comment #7
fagoWe cannot enforce that, we do not even enforce SQL.
Well, you can already pass around the EntityTranslation object, not? So is the only thing concerning to you that code has to differ between $entity and $translation?
I'm not sure about having $translation implementing the EntityInterface. What does $translation->delete() and $translation->save() do? Working on entity-level might be confusing, breaking operations down to translations might have unexpected consequences. Which one have you been thinking of?
Related, we need to start thinking about language fallbacks - mostly for display, but it should be at optionally respected by e.g. entity serialization also. Then the language-fallback rules should be available declarative, such that entity query could be improved to pick it up (e.g. in contrib).
I guess it would make sense to have a Decorator object that applies the language fallback rules, or should it even be in EntityTranslation?
Comment #8
plachWe can enforce it for the SQL database storage, that's all I'm concerned about. Other storage backends will have to find their way to deal with the existence of translations, but that's how a storage-agnostic solution is expected to work, I guess.
Think of an entity having only configurable-fields: strictly-speaking it wouldn't need the data table, but this way we'd have no "official" table to join on to determine the existence of a translation.
Yes, exactly, I don't think this is a small concern. I'd wish that the only difference between an
EntityTranslation
and anEntity
object were the value returned by::language()
. Any other method invocation, includingdelete()
andsave()
would be proxied to the wrapped entity. Entity-handling code should not need to worry about the concept of translation in any way, in fact in this scenario we'd always use an$entity
variable. I don't think there is an alternative approach if we want to have a sane DX this time.I'm seeing the
EntityTranslation
wrapper as an entity facet whose role is just providing the default language to act on, for anything else it would behave as the regularEntity
. To remove a translation I would just do:Yep, this totally makes sense: in D7 language fallback is applied only in the render phase and there is a reusable (to some extent) logic behind it. The one you are outlining above seems the natural evolution of this approach. My only doubt is about the declarative aspect, not sure whether it'll be that easy.
Comment #8.0
plachUpdated issue summary.
Comment #8.1
sunUpdated issue summary.
Comment #8.2
sunUpdated issue summary.
Comment #9
sunI've taken the liberty to completely rewrite the issue summary, in order to understand the complete proposal.
There's only one @todo in the Problem section about the claim of "fragile", which should be filled out.
Aside from that, this proposal makes a lot of sense to me. I almost guess it would have the potential of finally getting rid of the langcode keys in the field properties, so
$entity->fieldname
contains the field items directly. If that is part of the vision, then we should add it as last point of the Proposed solution section.Comment #10
yched CreditAttribution: yched commentedI like the overall proposal.
Off hand, I'd echo @fago's worries about EntityTranslation implementing EntityInterface, and both Entity & EntityTranslation objects being passed through code as $entity.
Feels like there's potential for lots of confusion :
- When exactly can you do $entity->someMethodOfEntityTranslationInterface() if you don't know for sure what you receive ? test instance_of ?
- $entity(_translation)->delete() deletes the whole entity ?
Comment #10.0
yched CreditAttribution: yched commentedUpdated issue summary.
Comment #11
plach@sun:
Thanks for streamlining the summary, I completed the todo and fixed the storage bullet which was inexact. The rest looks great to me :)
I assume you are referring to the internal values since the developer-facing API already got rid of the langcode keys, at least for NG entities. I think retaining the langcode level has a value because it makes easier to deal with non-translatable (shared) fields, whereas having langcode-keyed arrays of field objects would force us to copy a reference of every shared field object for each available language.
IMO the real (huge) pro of this proposal is that we will avoid tons of LoC like the following whenever a piece of code might need to deal with a multilingual entity (potentially everywhere we deal with an entity):
Or simply always do something like:
which woiuld be nothing more than a more verbose and less readable version of the current proposal. Let alone being able to determine the active language, which is something that might be not so easy to do in a clean way as stated in the OP.
@yched:
Honestly I don't see the potential for confusion. What behavior would you expect from the following line?
I expect it to delete an entity and all of its translations with it. I don't need to know nor care whether the active language is
'it'
or'fr'
. The new proposed methods would just act at data-structure level. Instead the new hooks would be fired by the storage controller when storing the updated entity. We can discuss whether when deleting an entity we should invokehook_entity_translation_delete()
(personally I wouldn't), but this is the biggest kind of doubt this approach inspires me. Which is a very little concern if compared to the DX wtf sketched above.If we want to be sure we are dealing with the original values, we juest need to do something like this:
but this is a way less common use case than the ones above. We should be optimizing DX for the most common cases, not the remaining 20%.
Comment #12
sunThanks! That makes this issue (summary) and proposal ready for syndication.
Comment #13
plachTagging for sprint to get more eyes on this.
Comment #14
fagoIndeed, your proposal should work out as long as we pass $translation around as $entity, i.e. never have $translation. That means EntityTranslation should probably be not a translation of the entity, but a just a different facet defaulting to a different language? Not sure, whether we could better clarify that with another name?
However, what happens if you call
$entity->removeTranslation('de')
on the translation of 'de' ? You are working with invalid values then?
Comment #14.0
fagoUpdated issue summary.
Comment #15
plachYes, this is exactly how I think about it when I try to imagine how code dealing with the future API should interpret it.
I was thinking about this too. The translation term is misleading in first place, since you cannot translate a map point, but you can have different points for different languages. I'd be happier if we could use the multilingual term, but this is not a hard requirement :)
The initial proposal refers to the
EntityTranslation
class because I didn't want to introduce too much changes in the current API, but probably we should rename it to convey the proper meaning.When I was playing with this idea, I thought about two possible solutions:
I think it's fair to require code explicitly dealing with translations to be aware of the concept of active language.
Comment #16
yched CreditAttribution: yched commentedOkay - but then I'm wondering how really different or better it is from the approach where we'd add a "current working language" state on the (unique, original) $entity object ?
Comment #17
plachWell, this way the wrapped object could be serialized/cached more reliably, I guess, whereas a stateful one would be more tricky to.
Comment #18
plachMoving to the right queue.
Comment #18.0
plachUpdated issue summary.
Comment #19
plachAn IRC meeting has been held today in #drupal-entity. Partecipants were das-peter, fago, plach, sun and tim.plunkett. Basically we agreed to proceed in the direction outlined in the OP. Some details were fleshed out. You can read the full discussion in the attached IRC log. The issue summary has been updated accordingly.
The decorator class name, currently
EntityLanguageDecorator
, is still open for debate so any suggestion is welcome.Comment #20
plachBefore working on this we probably need to complete the various NG conversions.
Comment #20.0
plachUpdated the issue summary after the IRC meeting
Comment #20.1
plachUpdated issue summary.
Comment #21
plachComment #21.0
plachUpdated issue summary.
Comment #22
msonnabaum CreditAttribution: msonnabaum commentedIs that really the consensus of the community? I completely disagree with it.
I'd like to see some of our entity classes turn into actual domain models, and this issue would prevent that. Forcing code to type hint EntityInterface almost defeats the purpose of type hinting it in the first place. I would consider this limitation a total non-starter.
Comment #22.0
plachUpdated issue summary.
Comment #22.1
plachUpdated issue summary.
Comment #23
plachThis is the consensus reached by the few people that participated to the IRC meeting. I tried to the spread the voice about it but very few showed up unfortunately.
That said, it seems both @tim.plunkett and @fago no longer agree with the conclusion above now.
Comment #24
tim.plunkettCompared to the other suggestions about how to solve this problem, I (at the time) thought a decorator was a reasonable solution. I didn't realize it would cause this interface problem, since we were just talking in the abstract, not with any code.
TL;DR I'm -1 on this now.
Comment #25
plachWell, we can still implement this solution and allow entity-type-specific decorators or just skip them if the entity definition does not specify neither the generic ELD nor an entity-type-specific version. This approach would allow us to use whatver interface we want in type hints.
Comment #26
tim.plunkettCan't we just have a bunch of empty classes like:
If not, the entity_info approach in #25 sounds great. Either seems like it would unblock #1391694: Use type-hinting for entity-parameters.
Comment #27
plachWe can surely go both ways: the former would mean implementing C, the latter would mean B or C depending on which interface the entity class implements. I think ths would be a reasonable compromise, but I could live also with strict C and empty classes/interfaces, as already stated in #1391694: Use type-hinting for entity-parameters. Do you think I didn't explain myself well over there? I didn't realize I was actually blocking it.
Comment #28
Gábor HojtsyHow would the different proposed approaches change the DX as proposed in the OP? I'm afraid I don't have the overview that others might have or that they don't have it either and we are just reflecting on certain particularities, not on what such decisions would entail for the overall DX.
Comment #29
plach@Gabor:
As long as we can use a decorator the API change proposed in the OP, with its evident DX gains, is feasible. The moment we agree (and I never will :) in #1391694: Use type-hinting for entity-parameters that we should use class names instead of interfaces in type-hints we lose all of this.
Comment #29.0
plachImproved issue summary.
Comment #30
Crell CreditAttribution: Crell commentedeffulgentsia explained this thread to me on the REST team call this week, so I *think* I finally understand what is going on here and how it impacts #1391694: Use type-hinting for entity-parameters. :-) From my POV, plach's original proposal (if I understand it) is much better than forcing a wrapper, and not just for the type hinting potential.
In part, it has the potential to be considerably more memory efficient. Consider, an EntityNG is, largely, a wrapper around a bunch of field objects. (If I have my APIs straight.) The difference for different languages is which language we're passing to those fields to retrieve the correct language's data. That means getTranslation() can be implemented as... a shallow clone. Consider the following over-simplified example:
That means if you call $trans = $node->getTranslation('de'), $node and $trans now refer to the same field objects, but different default languages. That means you can use both variables to get/set field values in that language only, but there's still only one data structure in memory. And, moreover, if you call ->save() on one of them then you save... the one copy of the data. So calling save() on ANY of them will work.
The net overhead here is the cost of an object, plus the language string. That's on the order of 50 bytes or less. All of the big data is single-instanced. IMO, that's huge for memory savings *and* for DX, and we get type hinting back because we don't need to have a generic translation wrapping object. The only caveat is that we need to have the $fields variable be an object rather than a basic array, but that should be simple enough.
And yes, I tested that the above does actually work in PHP. :-)
Is my understanding here correct? Does this all make sense?
Comment #31
plach@Crell:
Hey, good to see you here :)
Well, my original proposal is mostly what the OP is talking about. The solution currently implemented, which is closer to yours, was designed by @fago and me when introducing the Entity Field API. I spent much time thinking about the current proposal because also a wrapper has the potential to heavily reduce memory usage, but it does not imply introducing a "state" in the Entity object. AAMOF if I'm not mistaken your solution could be simplified even more if we just added an
activeLanguage
field to the Entity object itself, as I think we don't need different translation objects being instantiated at the same time in 99% of our use cases. This would also avoid losing entity deep-cloning, which we currently have (not sure whether it's needed only by the BC layer). When originally talking about this problem space, @fago strongly felt we should not introduce a state into the entity object, as this might be tricky when, for instance, serializing it. A possible solution for this particular issue is always reverting the active language to the entity language during serialization, but I guess this has the potential to introduce some tricky edge cases.I think you are totally grasping the issue, not sure whether your proposal actually respects the design constraint of "not introducing a state in the Entity object".
Comment #32
Crell CreditAttribution: Crell commentedI believe the serializer only serializes fields. Unless you mean PHP serialize(), in which case we should implement the Serializable interface and state explicitly what to serialize. So a non-field state property would be fine from that POV, as long as it can be adequately restored.
Actually, adding an activeLanguage Field to the Entity would be a terrible idea. :-) For one, it's not part of the stored state of the entity (the entity as it exists in the database has no "language that should be used", AFAIK, as that's a runtime value). For another, that would make it shared between instances and thus defeat the entire purpose of using a shallow copy; we'd need to do a deep copy, and then save() breaks, memory balloons, etc.
Since entities are by definition state, I'm not sure what is meant by "not introducing state". ;-)
Comment #33
plachYep, I meant this. And the last sentence outlines where tricky stuff might be: if we store contextual information in the entity data structure, when we restore it context might have changed. Anyway this was just an example. I think we could find a way to properly deal with this in the 80% of our use cases.
I think I didn't explain myself well, as usual: currently in the D7 Entity API we have a method to explicitly switch the active language, i.e. the language field accessors default to. There would be no clone in this scenario, although the ugliness is exactly in having runtime stuff on the entity object that is not persisted. My only doubt is that you proposal might end-up being close to this scenario as we would have a runtime language value that would not match the stored one.
The runtime stuff you were talking about above :)
Anyway, I think your proposal is way more attractive than the D7-inspired one I was referring to and has certainly evident advantages over the decorator-based one. I guess we could even support both shallow and deep cloning based on a flag, so this would be a non-issue. Overall if @fago is happy with it I think we have a deal :)
Comment #34
Crell CreditAttribution: Crell commentedI don't know when we'd be doing PHP serialize() on an entity object and it not being a bad idea, frankly. :-)
Comment #35
Berdir$form_state['node']? tempstore for views?
Comment #36
Crell CreditAttribution: Crell commentedOh. That. FAPI--
In that case, we *would* want to persist "transient state", wouldn't we?
Comment #37
fagoCorrect, but we have field objects which hold their own data in a single language right now, i.e. we have field objects per language. We do not have a single field object holding all language values.
Exactly - with introducing "state" we were talking about introducing "runtime" information, which would be shared among instances and thus a problem.
So what we've right now is a different Entity and Translation class. If I get this right, you are proposing to use the same class for the translations?
Right now, translation objects do not necessarily have all fields as not all fields are translatable. So we have two modes for dealing with that (strict mode or not). Without strict mode we fallback on default language, what makes all fields accessable. But by default we have strict mode enabled and do not make untranslatable fields available from the translation, such that you can explicitly deal with all translated fields, e.g.
But furthermore I think we should add handling language fallback as we have it during rendering (thanks to the field API) right now. I'd add it as part of the regular API, so that you can do
$translation->field->value
in order to get the value with fallback logic applied. That way it's easy to programmatically access the same values as they are rendered, what I think is rather important to have.So we could simply do that with a decorator object that implements the language fallbacks as necessary - given we have interfaces and can do decorator objects. Similar like we have a separate Translation object/class right now. But without decorator objects we'd have to bake everything into the Entity object, what would result in more complexity there and a bad SoC.
-> Let's better keep that logic separated.
Comment #38
Crell CreditAttribution: Crell commentedI'm trying to remove the decorator entirely, as it breaks things. (Like, any custom methods you add to an entity type, interfaces for entity types, etc.) Decorator-- here.
What I'm proposing is that a "translation" is simply another entity object that has the same persisted state, but different transient state. That is, a given Node object has a property that says what its default language is, whether it should fallback to default language or not, etc. That's all transient state and is never saved.
The persisted state is... fields.
What makes this work is the way clone() works in PHP. When you clone an object, its primitive and array data members clone with it, while its object data members do not, unless you tell them to. Thus:
So "language to return data for", "language fallback logic", etc. we ensure are stored in primitives (like $a). Persisted state (fields) we ensure are stored in objects (like $b). Now:
And now language and defaultHandling are unique between the two objects, but the data is not. That the fields are one object per language is irrelevant. The class just needs to know how to get to the right one via $this->language and $this->defaultHandling... which is needs to know anyway. That gets the translation fallback logic fago mentions in #37. because $node_fr and $node_en have exactly the same API and same data... just different transient state.
Comment #39
plachI am not sure the fallback decorator would need to be passed around as we would do with a translation object (however we implement it), instead I think its use would be confined to very specific tasks, such as entity rendering, that would not need to deal with entity-type-specific stuff. AAMOF by definition the fallback decorator would just deal with field values, thus a generic implementation should work with any entity type. We could still provide entity-type-specific ones through entity info, but this would not be a hard requirement.
If these assumptions are correct, I think translation objects as described by @Crell (cloned entities with different language default) could live together with the fallback decorator envisioned by @fago.
@fago:
This is one of my main goals even if we go for ELDs: I think having different objects (interfaces!) to deal with is a huge wtf in terms of DX, for an example you can see the interdiff in #1498674-216: Refactor node properties to multilingual. I don't think the current approach of having strict mode and objects with just translatable fields on is revealing particularly useful, in turn it brings a very bad DX. I strongly feel we should just provide an entity object with a language default matching the translation language, however we implement it. I think the risk of unintentionally overwriting the value of a shared field is a small concern compared to the additional complexity and DX burden this solution is bringing. In my experience writing D8 code dealing with ML entities is really painful at the moment :(
Comment #40
Crell CreditAttribution: Crell commentedI don't know what "Fallback decorator" you mean. My main concern is type hinting NodeInterface in a method, and getting an actual honest to goodness node object, not something that may or may not be wrapped in some other class that doesn't know that NodeInterface has extra methods. If this decorator is internal to that object, meh. :-) But all of these extra wrappers around the entity object need to go.
I actually think it's better DX that you *do* overrwrite a shared field, because different translations are the same node. The API should reflect that. It shouldn't make it seem like they're different nodes when they really aren't.
Comment #41
fagoI can get behind the idea of re-using the entity object for translation instances I think. crell is right in that we could implement and separate the fallback in an internal helper object also - if we want to. I'm still not convinced that the outlined approach leads to a reasonable simple way to implement it though, but let's think it through. I agree that just having one class makes things simpler and so would be a win.
However - we would have to fix terminology. You have been using the term "default language" as "active translation language", but "default language" is already the term we use for the original language of the entity. We need to come up with a consistent terminology here - like "default language" and "active language" maybe?
Well, I'm not sure about this. If a field is just one object it needs to hold all values in all languages (what it doesn't right now). In order to know with which language to work it needs to have the transient state of the active language also. For that transient state to be different between various translations we need different field object instances. Consequently, we'd have to do a deep clone for translations.
Sounds reasonable. But I guess we'd still need to have strict mode then as well? I guess that could be just be a special fallback logic case.
So here is a question: When a a node has default language of EN and I'd have an entity object with active language DE am I supposed to pass it around as regular entity object? I think that would be the goal and makes sense as it's an instance of the same class. But what if we have language fallback rules active that makes required stuff like $node->status inaccessible because it's not available in a certain language? I guess we'd have to assume that any available translation passes validation, i.e. has all required fields?
Comment #42
YesCT CreditAttribution: YesCT commentedGiven the recent discussion, can we update anything in the issue summary?
Comment #43
YesCT CreditAttribution: YesCT commentedWe are postponed (in #20) on "various NG conversions".
Maybe we should list some.
Comment #43.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary
Comment #44
Crell CreditAttribution: Crell commentedA required property being completely inaccessible in a given language sounds like a configuration bug that should be prevented, IMO.
Comment #45
plachTagging and unpostponing. I am actually working on this in http://drupalcode.org/sandbox/plach/1719670.git/shortlog/refs/heads/8.x-....
Comment #45.0
plachadding why this is postponed.
Comment #46
plachHere is a first draft, let's see how many failures we have.
Comment #48
plachRerolled and fixed some test failures. I successfully completed a standard installation with the patch applied, hence hopefully the failure in #46 was just a bot hiccup.
Comment #50
plachNo idea of what's happening :(
Comment #51
Gábor HojtsyIs the issue summary up to date in terms of the changes proposed?
If the module is named translation_entity, why name these hooks the other way?
Comment #52
plachNot exactly, I am more or less implementing @Crell's suggestion from #30.
Because this is the entity system Entity Translation API, which the
translation_entity
module just builds on.Comment #53
plachI'll try to get in touch with @jthorston or @rfay to track down the testbot failure, meanwhile early reviews would be welcome :)
Comment #54
Gábor Hojtsy#48: et-api-1810370-48.patch queued for re-testing.
Comment #55
Gábor HojtsyI tried installing manually as well locally with this patch and noticed no issues.
Comment #57
plachThis should fix many failures.
Comment #59
YesCT CreditAttribution: YesCT commentedyep these implements changes agree with the use that is in ViewUI.php:
use Drupal\Core\TypedData\TypedDataInterface;
but.. they can also all just be {@inheritdoc}
https://drupal.org/node/1354#inheritdoc
Comment #60
andypostso no update?
Comment #61
plachWe already have
hook_entity_update()
for that, we don't track update per-language at the moment. Do you think there are uses cases for this? In D7 I never needed it so far.Comment #62
fagoI think we missed an issue for language fallback right? So I created one: #2019055: Switch from field-level language fallback to entity-level language fallback
Comment #63
plachWell, no, I was planning to add that in the next iteration.
I'd be tempted to mark #2019055: Switch from field-level language fallback to entity-level language fallback as a duplicate of this one.Let's go on with two issues :)
Comment #64
plachSome more fixes.
Comment #66
plachStraight reroll. Not that many failures here, let's try a new testing round.
Comment #68
plachThis is more like it :)
Here are some more fixes.
Comment #69
plachComment #71
plachBroken bot, I guess.
Comment #73
plachUnless I just broke something else, we should be almost done.
Comment #75
plachThis should hopefully fix the last failures.
Comment #77
Berdir#75: et-api-1810370-75.patch queued for re-testing.
Comment #78
BerdirOne time login link no longer valid assertion failure, sounds like as random as it gets to me.
Comment #79
plachAdded a brand new unit test to provide test coverage for the new stuff. I'm pretty happy with the current patch. Reviews welcome.
Comment #81
plachFixed the node translation failure. The other looks unrelated and here the test is green.
Comment #82
Kristen PolI read the issue summary but I'm unclear as to how to test this patch manually. Write a custom module that calls these new functions? Or, is it only in need of code review? If the latter, than it's beyond me ;)
Comment #83
plach@Kristen Pol:
Thanks for your efforts but, yes, we just need code reviews now :)
Writing a module using the new API just to test it would be pointless IMHO, because we already have dedicated unit tests.
Comment #84
Kristen Pol@plach - Gotcha! That's what I figured :) Hope the code review goes well!!
Comment #85
andypostI think full-namespace is enough in doc-blcok, so hook_entity_translation_insert(EntityInterface $translation)
Also why $translation with entityinterface? Maybe better TranslationEntityInterface?
+1
Maybe @deprecated? but needs href to issue
Suppose more readable
if ($this->translationInit) {
//Avoid..
return;
}
is this mapping really needed? Maybe better to implement Base class?
Comment #86
plachThanks for reviewing!
Well, I couldn't find any directive in the coding standards but the rest of entity.api.php is using full namespaces, hence for consistency I kept those. I think the reason is that that code would work as-is whereas without the full namespace we'd need a
use
statement.The whole point of this issue is unfying the handling of original and translated values and being able to pass a translation object around as if it were an entity object (actually it is). Thus a different interface is the last thing we want :)
Not sure what you are referring to. Can you elaborate?
Yes, it is.
ViewUI
implementsEntityInterface
but shares no ancestry with any other entity object, it's just a decorator around theView
object.Comment #87.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #88
plach#86: et-api-1810370-86.patch queued for re-testing.
Comment #89
plachThe issue summary is now up to date. Again, reviews welcome :)
Comment #89.0
plachUpdated issue summary
Comment #91
plach#86: et-api-1810370-86.patch queued for re-testing.
Bot, be good :(
Comment #93
plachEvery time a different failure, this is crazy :(
Comment #95
plach#86: et-api-1810370-86.patch queued for re-testing.
Comment #96
Crell CreditAttribution: Crell commentedI haven't reviewed the entire patch, but the getTranslation() /__clone() logic from #86 looks good to me. My only comment would be to restructure the code to follow the usual if (empty($list[$key]) { $list[$key] = $whatever; } return $list[$key]; idiom. That way there's only one return statement instead of 2.
Otherwise, nice work, plach!
Comment #97
plachComment #98
BerdirLooks good to me in general, certainly better than the current special EntityTranslation class. Various coding style related feedback below, a bit about method names.
Missing description.
The @return was probably added by an IDE, not necessary.
Should be {@inheritdocs} for this and those below.
Not sure about the name. We already have $entity->original, and it's something completely different. Maybe getDefaultLangage() or getOriginalLanguage()? A bit confusing as it returns the entity, not the language...
That seems like an accidental merge thing.
bool
I think we usually don't shorten variable names, so it should probably be translationInitialized?
Should we do this on __sleep() instead, seems unecessary to serialize it and then throw it away on unserialize?
Can you check how this relates to #1953892: Move language node access logic into NodeAccessController, in terms of language negotiation.
We always execute the first condition, so we re-set it again? Couldn't we just as well simply do a return $this here?
Not sure :)
bool
@inheritdoc
We're currently doing this manually in getTranslation(), right? could we move it above the check, so that we do it always?
I'm not sure what exactly the coding standard says for simple returns like this, but I fear you need to repeat it..
Same.
int.
Don't use Exception/InvalidArgumentException (the second is already here, not sure if you're touching the code that uses it), inline with \Exception.
Given that you document it here, should we also load it be default here?
> 80 char. Maybe just leave the state service part out, that seems like an unecessary implementation detail?
No space between the two.
More that should be @inheritdoc.
Comment #99
fagoGreat work. The overall approach looks good to me.
So strict mode goes away - is that problem somewhere?
This looks like it should be done on __sleep() to prevent that data being serialized.
If this returns the active language, how do I get the original language then? I guess we should have getDefaultLanguage() or so.
I think that can be done just by $field->applyDefaultValue() calls on the objects. If we need the hook, I guess it would make sense to have a separate one or use the same one with a langcode parameter?
I fear that's going to be expensive - we'll need to performance test it. Cannot the field values be removed when the translation is removed and the check be done on object creation only?
Why would I bother with removed translations? To get the changes? Cannot that be derived via $entity->original? Having a $translation that says it's removed is a bit confusing to me.
Having that in addition to addTranslation() seems weird to me, but good to know it will go away.
Comment #100
plachThanks for reviewing! The attached patch should fix almost everything noted above, there might be some failures due to the last merges. Some answers below.
@Berdir:
No, actually it is needed to cast the returned value from
TranslatableInterface
toEntityInterface
.We'd need to provide the list of fields to be serialized. Added a @todo about this, see #2027795: Optimize content entity serialization.
Nope, because by populating the cache we make the reference available to all the translation objects, which wouldn't happen if we just returned.
I agree having both is confusing but I think that
$entity->original
is the culprit actully: it should be called$entity->unchanged
or better$entity->getUnchanged()
. Checked with @Gabor and he agreesgetOriginal()
makes sense as a name.@fago:
Apparently not :)
Yes, we want the hook to be applied to multiple languages. A single hook is better fo DX as people does not need to care about multilingual, and we don't need a language parameter because we have the translation object language :)
Great idea. I was really bugged by the need of exposing the status concept in the public API. I completely got rid of it and used the unchanged entity instead.
PS: The interdiff does not contain the latest merges but there shouldn't be relevant changes.
Comment #101
plachWe need to improve this message :)
Comment #103
plachFailing tests are likely to be fixed (at least in part) by #1822000: Remove Drupal\field_test\Plugin\Entity\Type\TestEntity in favor of EntityTest. Reviews are welcome meanwhile.
Comment #104
plach#100: et-api-1810370-100.patch queued for re-testing.
Comment #106
plachReroll
Comment #108
plachThis should (ought to) be green.
Comment #110
plachThis should fix the last failure
Comment #111
Dries CreditAttribution: Dries commentedAs this discussed with the D8MI team, this is part of 4 critical issues for D8MI:
Comment #112
Berdirfor @xjm to review.
Comment #113
xjmWell, I still don't entirely quite understand why we don't create our own method rather than using
clone
, but @berdir and @plach both indicated it won't work. So, what I'm picturing is something like:and then
I think have a separate protected method would make the code easier to understand. @plach suggested a reason this might not be a good idea but I didn't quite follow, so putting the example here for people to read at least. :)
(NB, the wrapping is not correct in the comments. I typed this in the issue.)
Comment #114
plachI had a final review with @Berdir and @Xjm, the interdiff would be pretty big as we are renaming a method so skipping it. Hopefullly this will still be green.
Comment #115
plachRerolled and updated after #2025991: Introduce hook_entity_prepare_form() to generalize hook_node_prepare().
Comment #117
plachObviously a BC decorator issue...
Comment #118
plachI did some profiling after speaking with @Berdir.
Comment #119
plachRerolled after #1754246: Languages should be configuration entities.
Comment #121
kfritscheRerolled 119 worked but it fails because the patch itself inserts langcode's. I changed the new langcode's now to id (see #1754246: Languages should be configuration entities).
When I didn't missed anything this should go green again.
Comment #123
plachThanks! Another reroll.
Comment #124
plachActually it was just a random fail, uploading a fresh reroll as #121 does not apply anymore.
Comment #125
Berdirhas been.
As discussed, let's change this to a constant.
I'e done some performance tests, and there's no measurable difference. 2.21 [#/sec] (8.x), 2.17 [#/sec] with the patch, with 500 requests on the frontpage with 50 nodes. We have 3000 calls to getUntranslated(), this is the biggest slowdown according to xhprof, 2998 of those are due to the BC decorator and should go away if we remove that. So we are good there.
Comment #126
plachActually since metadata technically is plural I changed another @todo to be consistent :)
Restored public constants, although I don't think this is actually an improvement as we are exposing an implementation detail, which has no place in the public API: if we change the internals of EntityNG, we might be forced to leave those constants there, even if we could remove them, to avoid API changes.
Comment #127
BerdirSooo... @alexpott says this should be static::TRANSLATION_EXISTING.
Comment #128
plachAnd here it is
Comment #129
BerdirOk. We went through this together in detail, did performance checks, detailed reviews of comments and so on. I think this is ready if testbot agrees.
Comment #129.0
BerdirUpdated issue summary.
Comment #130
alexpottI'm not sure about these hook's being fired from the DatabaseStorageController. That couples them with our storage engine and this seems an odd decision given prior refactoring in #1893772: Move entity-type specific storage logic into entity classes...
OTOH (on the other hand) if this should be here...
Missing scope - I think it should be on the interface so all storage controllers support translations...
Comment #131
BerdirDiscussed with @alexpott.
- The notify should be at the same place as the other update hook invocations.
- Move the implementation into the base class, so that a different implementation can easily re-use it, make it protected.
- Check with @chx of that's ok with him and in line with his changes.
Comment #132
plachThanks @alexpott and @berdir for taking some more time to discuss this. Here is a new one moving the translation hooks method in the base class and renaming it for consistency with the other available
invoke*
hooks.chx is not available at the moment, but I checked his sandbox and the
DatabaseStorageControllerNG::save()
method appears identical to me, so this change should be fine. Uploading the patch meanwhile, so the testbot can have a look to it.Comment #133
plachComment #134
BerdirOk, talked with @chx, and he is fine with having this here.
What might happen is that we internally refactor the save method and move more into the base class, but that is something that can be done internally and later on. But it will be called from the storage controller and will stay together with the other entity update hooks.
Therefore, back to RTBC.
Comment #135
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks!
Comment #136
plachAwesome, thanks!
Comment #137
YesCT CreditAttribution: YesCT commentedcool. I was about to post this. It's a follow-up instead. :)
#2031917: Docs follow-up Entity Translation API improvements
Comment #138
plachChange notice here:
https://drupal.org/node/2040323
Comment #139
BerdirLooks great! This is almost too nice for a mere change notice. Want to extract the part that explains how it works now and create a doc page below https://drupal.org/documentation/modules/entity ? (That needs a lot of work and probably a completely separate page for D8 but it can easily be moved around later.
Comment #140
plachI copied the D8 pieces in https://drupal.org/node/2040721.
Comment #141
BerdirUntagging.
Comment #142.0
(not verified) CreditAttribution: commentedSpelling mistake