Problem/Motivation
- We need a reliable way to tell whether an entity type can be multilingual: a non-multilingual entity will never have multilingual support. For instance its SQL storage controller won't create a data table. This will be useful for ET to determine which bundles can be enabled for translation (see the next item). We could even provide simplified controllers based on this.
- We need a reliable way to tell whether a bundle is translatable or not.
- We should not require ET to be enabled to retrieve the information above.
- We should avoid calling
entity_get_bundles()
fromtranslation_entity_entity_info_alter()
.
Proposed resolution
- Add a
'translatable'
boolean entity key. - Add a
'translatable'
entity key to the bundle info and add also a helper methodEntityInterface::isTranslatable()
so that the key does not need to be actually populated. - Stop calling
entity_get_bundles()
while altering the entity info: ET should create the translation overview route for each translatable entity type, no matter if it has any bundle enabled for translation yet. The tab visibility should be determined by access control as usual.
Remaining tasks
Discuss whether entities should default to translatable or notRTBC- Determine a strategy to backport this
- Write a patch
- Review, test, RTBC
User interface changes
None
API changes
- The field translation handler concept has been removed in favor of a
'translatable'
flag in the entity info. - A new
'translatable'
flag has been added to the bundle info, determining whether the bundle is enabled for translation.
Follow-ups
#1945348: Call decorated storage methods in ViewsUI explicitly
Original report by Dave Reid
For example, if I enable content translation (either via translation.module or a contrib module) for article node types but not page nodes, those modules should be altering hook_entity_info() for $info['node']['bundles']['article'] to indicate that translating is enabled.
Something like:
/**
* Implements hook_entity_info_alter().
*/
function translation_entity_info_alter(&$info) {
foreach ($info['node']['bundles'] as $type => &$bundle) {
if (translation_supported_type($type)) {
// Mark that translation.module is providing localization for this content type.
$bundle['translation']['translation'] = TRUE;
}
}
}
Would also be nice to have a isTranslatable() method in the default Entity class that we can call as well (and an entity_is_translatable($entity_type, $entity) API addition for D7).
Comments
Comment #1
Dave ReidThis would assist the Pathauto contrib project help provide URL aliasing in an entity-generic fashion.
Comment #2
YesCT CreditAttribution: YesCT commentedWe might have an api like that now...
Maybe it was added on the way to #1810386: Create workflow to setup multilingual for entity types, bundles and fields, or that might show it in use as an example that would be useful.
Comment #3
plachYes, we totally need something like this, also to remove lots of crufty
module_exists()
spread around core atm. Will try to work on this as soon as I have a minute.Comment #4
plachComing from #111715-90: Convert node/content types into configuration.
@Berdir:
Yes, this sounds as a good pro. Do you think we should follow the
entity_get_info()
pattern and cache the result of ahook_translation_entity_info()
+ alter invocation?This was introduced as a of way to lock out field translatablity when we weren't sure what ET would look like in D7 and whether it would actually work. In D8 I see no real point in providing a way to declare multiple field translation handlers. AAMOF I already planned to remove all that cruft. OTOH that was a cool way for modules to avoid explicitly checking for the presence of ET. I guess we need to retain some level of information at entity system level, that is something that can be reliably checked even when ET is disabled.
Comment #5
sun+1. Closely related: #1904880: Translation entity module does not provide any API
Comment #6
plachI was thinking about a plan to address this.
Goals
entity_get_info()
fromtranslation_entity_entity_info_alter()
.Proposed solution
'multilingual support'
boolean entity key.'translatable'
entity key holding an array of boolean values keyed by bundle machine name. We can add also a helper methodEntityInterface::isTranslatable()
so that the key does not need to be actually populated.hook_entity_translation_info()
+ alter.If we go this way we can remove all the field translation handler cruft from the entity info, that is the
'translation'
key. As a first step, while we are not done with the Entity Field API conversion yet, we can just makefield_is_translatable()
depend on the'translatable'
entity info key. Then we can just drop that part and simply check the'translatable'
field info key.Thoughts?
Comment #7
tstoecklerInstead of a translatable key on the entity info that is an array keyed by bundle names, I think we should be using the bundle info directly. Specifically, I think we should add a "translatable" key to hook_entity_bundle_info(). We could make the EntityManager be smart and make the default of that key depend on the "multilingual support" key of the entity (altough A. dependence of the bundle info on the entity info is exactly what hook_entity_bundle_info() tries to avoid, so that might actually do more harm than good and B. shouldn't that key be called "translation Support" for consistency (or the bundle key be called "multilingual")?).
Comment #8
plachThis solution sounds sensible to me but how would we deal with bundless (tm) entities, such as
User
?Not sure about this: it would mean the by default nodes would be translatable on any D8 site, but since ET is not enabled in the standard profile, this would be quite inconsistent. Actually in my mind the key would be populated by ET by implementing
hook_entity_info_alter()
and checking its config values.Yep, "multilingual" sounds better to my ears, but all the Entity Translation API right now is using the "Translation" term hence "Translation support" is probably better, although it may be unclear which is the difference between the two keys.
Comment #9
tstoecklerWell "bundless" :-) entities get a default bundle that is named after the entity type. So I think if the entity type has declared multilingual => TRUE (as the top-level entity type property) it would make sense to make the bundle translatable. If multitlingual => FALSE we don't make it translatable. Thoughts?
Comment #10
plachYes, this just works :)
As I was saying above I think this may cause an inconsistent behavior: unless I'm missing something, if we go this way, as soon as you install the standard profile and call
$node->isTranslatable()
you will get TRUE. But this would clash with the fact that ET is not enabled. Instead if we default to FALSE and alter the bundle key based on the ET config settings, we will have that the bundle is marked as translatable exactly when ET will allow to translate it.Comment #11
plachComment #12
plachGoing to work on this tonight.
Comment #13
plachHere is patch implementing #6. The only thing I left out is migrating the entity translation info to a dedicated info hook, since it is likely to disappear altogether when #1839516: Introduce entity operation providers is done. However I removed the
entity_get_bundles()
call from thetranslation_entity_entity_info_alter()
, which should unblock #111715: Convert node/content types into configuration.Comment #15
plachThis one should fix most failures.
Comment #17
plachNow tests should pass.
Comment #18
BerdirIt's a bit unfortunate that this needs to be set explicitly everywhere now. Can we maybe default to TRUE if it is fieldable or something like that...
Otherwise, this looks great to me.
Comment #18.0
plachAdded issue summary
Comment #19
plach@Berdir:
Not sure about this, the translatable flag might have heavy implications, for instance it might trigger the creation of the data table. Will ask @Gabor to chime in.
Edit: Added an issue summary.
Comment #19.0
plachUpdated issue summary.
Comment #19.1
plachUpdated issue summary.
Comment #19.2
plachUpdated issue summary.
Comment #19.3
plachUpdated issue summary.
Comment #20
Gábor Hojtsy@Berdir: it does not need to be set everywhere now. For example, this patch makes contact content entities non-translatable as well I believe, which is a great side-fix for the problem that they show up for multilingual configuration, since you cannot currently tell if an entity can be configured for translation. I agree with @plach that it is best to leave the decision possibility to entity implementations, sites using their own entity types should get the possibility to opt out of translation support for their entities even if they use fields.
Comment #21
plachOf course :)
Comment #22
YesCT CreditAttribution: YesCT commentedI did a standards and comment review and the code looks clean.
In the issue summary, there is a remaining task: Discuss whether entities should default to translatable or not
This patch sets
translatable = TRUE
in 7 plugins
and translatable = FALSE
in none.
CustomBlock
Comment
Node
(Test Entity)
(Test Entity Revisions)
Term
User
Next steps
What questions do we still need to ask and answer to finish off this issue? (re: Discuss whether entities should default to translatable or not)
It sounds like this patch is proposing that by default entities *should* default to explicitly stating either translatable or not.Do we need to ask which entities or which bundles should default to translatable?
After writing this comment, and re-reading the question, the issue summary and the patch, I change my mind. I think this patch makes translatable false for everything by default, and sets true for some (see the plugin list). And I think the discussion has taken place and the decision is that entities should default to not translatable unless explicitly set to true because we cannot make an algorithmic guess that things should be translatable. See #19.
here is a way to enableTranslation (in a test)
--
the point?
this function is for the content translation module to determine if a bundle is translatable? And also this patch makes a way for config to store that?
returns TRUE if an bundle is translatable, or if any bundle in an entity is translatable.
But I dont see where this patch is setting defaults, for example #21
why a special case for entity view mode?
this method is the point? and this works without content translation module being enabled?
Ah, ha. So everything else is false, other than the stuff in here where the plugin is set to true?
This seems to be describing that, but I dont see in the code where it is done.
Summary
AFAIK, the remaining tasks are done. And my questions are non-blocking.
And a change notice with examples will help clarify things.
Comment #23
BerdirAgreed, this looks good to me as well, let's get it in so that we can unblock the node type conversion.
Comment #24
xjmIf this blocks the node type conversion it should be the same priority we just bumped that one to. (Haven't reviewed the patch, just bumping it.)
Comment #25
YesCT CreditAttribution: YesCT commentedI think #111715: Convert node/content types into configuration is what @Berdir mentioned is waiting on this issue
Comment #26
YesCT CreditAttribution: YesCT commentedComment #27
xjmUh. What? Magic! Comment on this line might be good.
Comment #28
webchickLet's get one more round of updates here for the most recent comments.
Also, I'm not sure this is backportable to D7..?
Comment #29
Berdir@xjm: That's the same pattern as dozens of existing methods in that class, I don't think it needs a special comment.
Comment #30
Gábor HojtsyAs per #29 and above :)
Comment #31
xjmIt needs something. :P We just spent 15 minutes in IRC debating WTF it does. Maybe a followup for the whole class?
There's a reason I am NOT in MAINTAINERS.txt for Views UI. ;)
Comment #32
BerdirFollow-up sounds good to me. This patch just applies an existing pattern in that class.
What it does it pass the call through to __call() which forwards any method call to $this->storage.
What I would suggest is to call $this->storage directly instead of doing that indirection which just slows things down. The same way that EntityBCDecorator does it, which means simply $this->storage->isTranslatable() in this case or $this->storage->setNewRevision($value) for an example with arguments. The additional human-overhead of doing this once is not big but it helps both with performance and it's better to understand (which is not often the case :)). You had no trouble to understand the same method in EntityBCDecorator I assume?
Comment #33
tim.plunkettThat approach is from http://stackoverflow.com/a/4966752/614351, it ties in with the standard __call() pattern used in all decorators in core:
Where $this->storage is the decorated object.
Comment #34
YesCT CreditAttribution: YesCT commentedin #22 I asked/wondered
and
So here I answer my own question:
so this reads it out of config, and if it's missing or false, then translatable is false. If it's true in config, then it's true, and then the array described in
gets it's translatable value. (which is false unless the plugin set it to be true)
the documentation that there is a translatable element in the array is in the hook.
but would that happen only when the content translation module (translation_entity) module is enabled?
is $enabled if the bundle is enabled, or if translation is enabled on the bundle?
Comment #35
YesCT CreditAttribution: YesCT commentedhere is the follow-up for #27 to #33.
#1945348: Call decorated storage methods in ViewsUI explicitly
Comment #35.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary.
Comment #36
plach@webchick:
The current solution is certainly not backportable. Not sure whether the original one proposed by @Dave Reid is.
@YesCT:
Not sure I understand the question, but I have the feeling there is a bit of confusion about the implemented solution: we have two keys:
'translatable'
key in the entity plugin info, which states whether the entity type supports translation or is inherently language-unaware (the latter is the default).'translatable'
key in the bundle info, which states whether that specific bundle has translation support enabled (by default it doesn't). As you pointed out above, ET alters the bundle info based on its configuration. If it is disabled other modules providing translation support could kick in and say whether a bundle is translatable or not.I'm attaching a new patch, since I realized some field documentation was not updated. As a bonus I added a small clarification on the config_test changes.
Comment #37
xjmYep, still RTBC assuming it goes green.
Comment #38
webchickOk, looks good. About to board a plane, so hopefully this will work without any dramas. ;)
Committed and pushed to 8.x. Thanks!
This needs a change notice methinks.
Comment #39
plachHere it is: http://drupal.org/node/1945906.
Moving to the D7 queue and demoting back to normal. Leaving the status to active since we don't have a backportable patch.
Comment #39.0
plachadded follow-up
Comment #40
Gábor Hojtsy(Change notice looks good to me.)
Comment #40.0
Gábor HojtsyUpdated issue summary.