Problem/Motivation
Noticed during #1945226-38: Add language selector on menus that there should be a way to have things on the language settings wizard/workflow page: admin/config/regional/content-language without having to fake that they are translatable. The language settings page was added in #1810386: Create workflow to setup multilingual for entity types, bundles and fields.
in core/modules/language/language.module in function language_entity_supporte()
if (!empty($info['fieldable']) && !empty($info['translatable'])) {
$supported[$entity_type] = $entity_type;
}
'fieldable' and 'translatable' can be set in the @EntityType (was plugin) annotation.
Things that are not fieldable or not translatable might still want to have language support and be on the settings page.
Proposed resolution
TBD
Remaining tasks
- Evaluate how the configuration page for language settings (admin/config/regional/content-language) is assembled
- ?
User interface changes
No UI changes anticipated.
API changes
TBD
Follow-up issues
- #27 is being done in #1968970: Standardize module-provided entity info documentation and clean-up the @EntityType annotation
- #1983928: translatable, translation enabled, multilingual, language-aware, language-alterable
Related issues
- #1945226: Add language selector on menus
- #1810386: Create workflow to setup multilingual for entity types, bundles and fields
- #1836086: [meta] Entity Translation UI improvements
- #1446382: Need a reliable way to determine if a specific bundle for an entity type is translatable (fixed in 8.x. it's open for backport to 7.x)
- #1983948: document EntityType annotation public $translation
Comment | File | Size | Author |
---|---|---|---|
#53 | drupal8.translation-entity.module.1977784-53.patch | 10.55 KB | shnark |
#53 | interdiff-48-53.txt | 2.96 KB | shnark |
#48 | drupal8.translation-entity.module.1977784-48.patch | 10.43 KB | YesCT |
#48 | interdiff-44-48.txt | 2.44 KB | YesCT |
#44 | drupal8.translation-entity.module.1977784-44.patch | 7.99 KB | YesCT |
Comments
Comment #1
YesCT CreditAttribution: YesCT commentedin core/modules/language/language.module
is used to decide what to list.
In core/modules/language/language.admin.inc
Comment #2
Gábor HojtsyIs that used for anything else but this screen? If not, we should rename it to something more specific and use it here. If it is used for other things, we should look if we break things if we change it :) Also this is actually for content language, tagging for that :)
Comment #3
Gábor Hojtsy#1945226: Add language selector on menus is also postponed on this one then. :) Putting on sprint to figure out.
Also looking at the code, this seems to be the only use of this helper function, so I think we should rename and modify. I don't really know what should be the criteria in the method, but neither translatability neither fieldability should be a requirement. The screen is used first to set up language settings, which should work even if the entity is not fieldable or translatable. The translatability and fieldability (or actually the presence of fields configured on the bundle) should be the condition to add a checkbox for translation of the bundle. That would also solve our current bug where you can try to make the terms translatable, even though they don't have any fields configured by default, and then it spits back an error at you telling you should not have clicked that checkbox. Then why it is there? :D
So this would make this screen more flexible, apply to menus and taxonomy terms without any need for further development on them, and they would receive the translation checkboxes automatically as they are built out in their respective issues (when entity ng conversion and property multilingual variance done for them).
Comment #4
YesCT CreditAttribution: YesCT commentedI'll take a few stabs at this.
Here is a first bit, just taking out the helper and seeing what breaks.
Comment #5
YesCT CreditAttribution: YesCT commentedComment #6
YesCT CreditAttribution: YesCT commentedThis lists everything.
Eventually, we should hide contact messages. Maybe it would be best to put the helper back, and use *it* to filter out the contact messages.
Everything (admin/config/regional/content-language):
Block
Breakpoint
Breakpoint group
Comment
Contact category
Contact message
Custom Block
Custom block type
Editor
Entity display
Field
Field instance
File
Text format
Image style
Menu link
Content
Shortcut set
Menu
Taxonomy term
Taxonomy vocabulary
Tour
Role
User
View
Previously: it only showed:
Comment
Custom Block
Content
Taxonomy term
User
Comment #7
Gábor HojtsyOk, well, hum, we definitely should NOT display any config entities here, such as contact category, image style, entity display, editor, etc. Over half of what t the "everything" list shows. Then we should be down AFAIS to "File", "Menu link" and "Contact message" being the only new items. File and contact message does not support being assigned a language even, let alone being multilingual (by design, not an issue, its fine for us :), so we need to figure out a way to filter them out. Maybe we need to add a 'has_language' key on entity info like 'has_title' and set to TRUE only for the entities where we have language?
So something like:
1. Filter out the config entities.
2. Filter out the content entities that will have no language.
These sound like better filters vs. the fieldability and translatability filter, given that we want to display content entities that are not translatable and not fieldable but can take a language and are content entities.
Comment #8
YesCT CreditAttribution: YesCT commentedLooking for entities with language support...
I started with those that were already translatable
grep -R "* translatable = TRUE," *
then looked for things that have a language select.
grep -R "'#type' => 'language_select'," *
Menus had it added http://drupal.org/node/916388#comment-7014556
but it's not per bundle, and we are doing that in #1945226: Add language selector on menus and that can add the language_supported = TRUE.
Have not filtered out config yet. Just using the new supported annotation key. Called it language_supported instead of languageable (which is the pattern for fieldable and translatable).
Now the list is:
Comment
Custom Block
Content
Taxonomy term
Taxonomy vocabulary
View
No interdiff, since didn't use the previous changes and went back to clean 8.x to do this.
Comment #9
Gábor HojtsyI'd default this to TRUE, since we built in a language property to entities in general. However, it does not make sense for all entities (eg. files and contact messages), which is why we introduce this property :) So I think it should be on by default and then we need less to undo it. We can leave it on for all config entities, all of them maintain their langcode thanks to a few recent patches :)
Comment #10
YesCT CreditAttribution: YesCT commentedoops. mis-spelled language.
I'll fix that.
Maybe a nice easy novice follow up later for:
core/includes/install.core.inc: // langauges available at http://localize.drupal.org.
core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.php: * Tests automatic translation import when a langauge is enabled.
core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.php: * Tests automatic translation import when a custom langauge is enabled.
core/modules/translation_entity/lib/Drupal/translation_entity/Tests/EntityTranslationSyncImageTest.php: // while the default langauge is used as target.
Comment #11
YesCT CreditAttribution: YesCT commentedOK, with defaulting to TRUE, without marking content FALSE, with filtering out entities with config_prefix, the list is:
Comment
Contact message
Custom Block
File
Menu link
Content
Taxonomy term
User
I'll mark File, Contact message, Menu link FALSE.
I never noticed that they were out of alphabetical order before, since Content is close to Custom Block..
Comment #12
plachGuys, sorry for coming late to the party, but I don't think the direction here is correct: as stated somewhere in #1446382: Need a reliable way to determine if a specific bundle for an entity type is translatable and in the related change notice the
'translatable'
entity key should actually be called'multilingual'
and is used to denote the fact that the entity type is language-aware. Hence I think it's perfectly suited to be used as a way to tell whether an entity type should appear in the content language settings page. IMO the current patch is just duplicating the information conveyed by the'translatable'
entity key.I agree that fieldability should not play a role here, but it was a temporary way to exclude untranslatable stuff until we completed the transition to NG. We can get rid of it I guess.
We should not arbitrarily exclude config entities IMO, as we could still find a way to translate through ET. They should be left out just because they do not declare themselves as language-aware (yet).
I'd definitely retain
language_entity_supported()
because I think it's an useful API function. The fact we have only one use in core does not mean it can be safely thrown away :)Comment #13
Gábor HojtsyYeah, well, translatable is pretty misleading then. How do we distinguish entity types that support language (have language assignment, etc) but are not translatable (like files) from entities that have language assignment and translatable like nodes?
Comment #14
YesCT CreditAttribution: YesCT commentedreminder for myself or others: drush cc all to clear the caches after changes to annotations.
hm. http://api.drupal.org/api/drupal/core%21includes%21entity.inc/function/e...
says @deprecated Use \Drupal\Core\Entity\EntityManager::getDefinitions() directly.
But I dont see that... http://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21E...
I could not figure out how to sort $supported based on the labels in $labels
I thought config would be translated via http://drupal.org/project/config_translation and #1952394: Add configuration translation user interface module in core
renaming translatable to multilingual ...
leaves us still needing a way to know if something is translatable.
Or are you saying that they are *the same*. Anything language aware, translatable/multilingual should show on the language settings page, and also get a translatable checkbox?
For kicks, patch attached. [oops. left a dpm in that was not commented]
Comment #15
plachLanguage-awareness is an information set at entity-type level. Instead translatability is set in the bundle info, as you well know ;) An entity type that supports only assinging a language has the
language_support
/translatable
key set to TRUE (this will also tell the storage controller to create a language-aware schema). A translatable bundle has the'translatable'
bundle info key set to TRUE, and this value can be changed in any moment.I am totally fine with renaming the
'translatable'
entity info key tolanguage_support
ormultilingual
or whatever. I didn't do it in #1446382: Need a reliable way to determine if a specific bundle for an entity type is translatable becuase right now outr APIs are rather inconsistent on this side, AAMOF most stuff that should be labelled as language-aware or multilingual is actually referring to translation/translatability.Comment #16
plachAt least for now we should not default to TRUE. Explicitly opting-in for language support is the filter we are looking for :)
Comment #17
YesCT CreditAttribution: YesCT commentedRe-reading, again, http://drupal.org/node/1945906
An entity-level 'translatable' key in the entity plugin info, which states whether the entity type supports translation or is inherently language-unaware (the default).
=>
An entity-level 'translatable' key in the entity plugin info, which states whether the entity type stores language information (and might have translatable bundles) or is inherently language-unaware (the default).
Comment #18
plachYep, sounds as a sane fix (along with the key name update) when we are done here.
Comment #18.0
plachfix closing html tag typo
Comment #19
YesCT CreditAttribution: YesCT commentedwould it make sense to call it multilingual (or language_supported) on the entity level, but call it translatable on the bundle level?
Or would that be more confusing?
Comment #20
YesCT CreditAttribution: YesCT commented@plach btw, any thoughts on how to sort the second part of the settings page?
Content (node) should come before custom block.
Comment #21
Gábor Hojtsy@plach:
Again, how do you distinguish the case of file entities, which have language assignment but nothing to translate and other entities that have language assignment and have stuff to translate. I assume by language-aware schema you don't mean the langcode column only :)
Comment #22
plachI don't see the difference honestly: translation is added on top of language-awareness. File-enties will have something to translate as soon as you attach a description field to them.
If an entity is language-aware it may become translatable sooner or later. Proactively providing the data table will make it ready for translation in any moment. Instead using separate keys to denote language-awareness and translatablity would let the storage controller skip the data table and prevent translation from being enabled without a migration.
Edit: Moreover even file entity properties might vary by language ;)
Comment #23
Gábor HojtsyI see language assignment and translation as two different use cases. Language awareness is definitely available without the content translation module turned on, so you can assign language to nodes, taxonomy terms, and so on. You seem like consider this a restricted version of translatability, but I consider this metadata on the content. So a file uploaded we should know it is an English book photo or language independent scenery picture or whatnot. Then whether you want/need translation on top of that is a wholly different question. Sites use wholly separate menus at times for different languages or even different nodes for different section of their site or for certain types or whatnot. I'm not sure enforcing translation features even though someone only wanted to assign language information is the best idea we can do.
Anyway, the use case of this issue is menu items and taxonomy terms. Neither have fields by default, and menu items are not even fieldable now. However, they have language assignment and we want them to show up in this table even if they will never be translation capable (eg. if you never going to have a field configured on your term or if we never get around to make menu item properties multilingual). So entities showing up in this table vs. entities where showing the translation checkbox makes sense / will not result in a validation error when attempting to submit the form are not equal sets of entities. We want to resolve that problem, so you don't get a checkbox for entities where you'd get an error back if you attempted to check it but we allow people to configure language assignment of those entities still, because that capability is applicable to them.
So what we are looking for is exactly to distinguish between the two cases which you consider the same :D
Comment #24
YesCT CreditAttribution: YesCT commentedmore playing around.
bundles are not translatable by default.
I dont see translatable specified in a article or page ... annotation (I dont think there is one, I think it's just default config?)
So I made articles translatable, and checked
sites/default/files/config_IEzPosfvp3g2-OAq-XoORh4wT73FIqGSAnaLvdSZygE/active/language.settings.yml
...
nope.. here, translation_entity.settings.yml:
node:
article:
translation_entity:
enabled: '1'
page:
translation_entity:
enabled: '0'
Is that from EntityType.php:
If so, maybe that is the bundle level translatable-ishness?
Comment #26
plachSure.
Nope :)
Yup :)
What I am saying is that the concept of language is closely related to the one of multilingual. If your entity type is language-aware it might easily end-up being in need of multilingual support, although language could be used just to filter listings and stuff like that. My point is that the latter use case is easly denoted by having the (to be renamed)
'translatable'
entity key to TRUE and no'translatable'
bundle key set to TRUE. Bundless (tm) entities have a single bundle entry in their bundle info so we can support the scenario above also in this case.Yes, fieldability was just an interim filter. We can get rid of it now if we want to.
All my work is in the direction of always having the ability of enabling translation on language-aware entity types, so I don't see the need of having two different keys, one saying that an entity-type is language-aware and one saying that the entity-type supports translation. If we want to keep this distinction for now since we are not sure if that goal can be fully accomplished, I can live with that but I think it's providing redundant information that might confuse developers not familar with this matter.
Comment #27
plach(crosspost)
Bundle translatability is disabled by default. ET alters the bundle info based on the configuration provided in the content language settings page.
Aw, we need to remove this.
Comment #28
Gábor Hojtsy@plach: Well, http://api.drupal.org/api/drupal/core%21modules%21translation_entity%21t... does use the translation key as an array still in its current form? I also seen that undocumented @todo thing. If we do use it for something, we may want to document it :)
As for language_support, multilingual or translatable, I think it will look odd for DX to have a name that says it can take multiple languages or is translatable (to languages) when all it does is it indicates it can take a language, but whatever, this issue is concerned for the UI not the API, so we'll look at the API more if others speak up. Until then we can leave the IMHO confusing state intact.
What's more important for this screen then is (a) entities that are not fieldable show up for language configuration (b) only entities that are fieldable AND have fields configured will have a translation checkbox on their bundles in the table. So taxonomy terms would not get a checkbox in the default core install. Looks like you suggest (a) should depend on the 'translatable' property and (b) should I guess depend on fieldability + that the bundle has at least one field defined. So the condition from the existing filter function (before patch) for fieldability should be moved to the checkbox logic and one more condition should be added there to look if the bundle actually has at least one field.
That would make taxonomy still show up but no checkbox until you add a field. Then we can move on to the menu entity issue where we can add the translatable property on the entity (which will not mean it is translatable) and that will make it show up proper in the table but without a checkbox.
I see you are working hard to make all entity types in themselves translatable and it makes sense for almost all core entity types to eventually have a translation checkbox (I'd debate file entities), but its impossible to say all entities in Drupal ever should support translation if they can take language assignment. So this UI should support entities that can take language assignment but not to be translatable. Looks like this solution with the translatable property will force them to a certain translation-supporting schema already, even if the developer does not want to do that, so maybe contrib developers who only want to language-assignment-enable their entities might need to hack around this and do their own thing?! Sounds not very friendly to me, but whatever...
Comment #29
andypostSuppose awesome plan
Also I think language-aware mean that entity has no langcode field in it's definition so requires translation.
Can we make "translatable" property dynamic to allow translation only for entities that have no language definition?
Comment #30
Gábor Hojtsy@andypost: I don't see how translation would be possible if we don't know the base language. Translation assumes we know the language of data :)
Comment #31
YesCT CreditAttribution: YesCT commentedI'm going to leave the translatable called translatable and take another stab at this.
Still looking for hints on sorting... (it's a nit at this point, but I know we'll have to get it eventually).
Comment #32
YesCT CreditAttribution: YesCT commentedthis has trouble if there are no rows of fields.
also I haven't put in a way to check if anything has fields or not.
still needs to sort the tables on the label instead of the entity machine name.
you can test various combinations
by adding the translatable = TRUE annotation for menulinks (this is a bit of a WTH, since it just makes it use the language stuff, not translatable)
then picking custom settings for:
menus (not fieldable... and should not be able to enable translation. in the ui that is marking it translatable)
taxonomy terms: translatable, but has no fields to translate
content: has fields to translate. <- should check the translatable checkbox
=========
Comment #33
YesCT CreditAttribution: YesCT commentedneed to take out the enable translation checkbox from the language content settings widget.
has errors because it's assuming there is a checkbox for all the fields I think.
is checking to see if it has fields using plach's recommended field_info_instances() on the bundle.
Comment #34
YesCT CreditAttribution: YesCT commentedthis takes out the redundant enable translation from the language settings widget.
To do that, I re-ordered the conditions, so both the check on the entity being fieldable and the bundle having fields are next to each other... so if they stay there, they could be combined into one condition.
Still some errors...when reloading the page
============
============
I'll unassign for a bit since I'm going to be driving around for part of the day. :) and give anyone else a chance to try something if they want.
[edit: oops. left the menulink in there... we'll just take it back out in next patch version. it's just for testing.]
Comment #35
YesCT CreditAttribution: YesCT commentedoh, I just took it out, ... and fixed the word wrap (which was really putting it back).
Comment #37
andypostlooks nice
Comment #38
Gábor HojtsyFor sorting the tables, I'd just modify the second foreach() to use $labels instead of $supported. $labels only has items from $supported and is already properly sorted. Also $labels is computed from $supported, so putting the sorting before the other foreach on $supported sounded like more code. While this looses us some unneeded code in fact :)
Comment #39
Gábor HojtsyBehavior and code looks good:
Only found one issue:
Lack of space with if() is a pre-existing code error here, you just indented differently, but still :)
Also notices from test should be fixed. The fail seems to be unrelated.
Comment #40
Gábor HojtsyGo testbot!
Comment #41
andypostThis function is expensive so better to change s/has_fields/fields
Comment #43
YesCT CreditAttribution: YesCT commented1. Fixed the space issue with if( => if (
2a. Only calling field_info_instances once and saving the result to reuse.
2b. I thought maybe we could further re-use like:
But it seems not.
3a. Fixed a misspelling of mutlilingual..
3b. and clarified (?!) translatable annotation meaning.
4a. Got rid of errors by using
if (array_key_exists('translatable', $element[$bundle])) {
I tried !empty , but I think that expected the index to be there, so still got error.
I dont know if this is the way we really want to do it.
4b. Also did something funky inserting class untranslatable because needed something to put in the array so the it had the right number of columns. I'll take advice on the right way to do that.
5. to test: try, reloading page, shift reloading, and also submitting without changing the values.
Comment #44
YesCT CreditAttribution: YesCT commentedre #43 4a: checked again, and looks like !empty is fine.
Comment #46
Gábor HojtsyLooks like still two instances of notices on line 1013 of the module.
Comment #47
YesCT CreditAttribution: YesCT commentedworking on the last exceptions.
Comment #48
YesCT CreditAttribution: YesCT commentedran that one failing test locally, and passes. (Entity Translation UI -> Entity Translation settings)
at first I used !empty again, but here I need to know it it's false, so I had to use isset.
with !empty, when the it's translatable (the checkbox showing) we need to save that translatable is false (translation is not enabled)
there was a small spelling typo of untranslatable:
If the field is untransltable we need to reset the sync settings to
That comment changed anyway as the reset for the sync wasn't really being done if it wasn't translatable, but if it was translatable (if it was isset) but translation was not enabled.
Which brings me back to some of my earlier general confusion from early comments in this issue.
Not the problem of this issue, so it does not need to be fixed here.
#1983928: translatable, translation enabled, multilingual, language-aware, language-alterable
Comment #48.0
YesCT CreditAttribution: YesCT commentednote reliable way to tell if translatable, is fixed in 8.x
Comment #49
YesCT CreditAttribution: YesCT commentedTesting that this works as expected when unchecking the translatable settings after a save is also important to do.
When using the !empty instead of isset, for example, article was left translatable but with no fields translatable when disabling translatable on articles that were previously translatable. It should have had the translatable checkbox not checked. Using isset in the save settings function helped that.
Comment #50
YesCT CreditAttribution: YesCT commented#1983948: document EntityType annotation public $translation for #27
updating issue summary
Comment #51
YesCT CreditAttribution: YesCT commentedchecking fieldable made sense before, when I think the call to field_info_instances was only done if it was fieldable.
Maybe change this to just if ($fields) since that works for not fieldable things.
Or do:
Comment #51.0
YesCT CreditAttribution: YesCT commentedadding follow-up issues
Comment #52
YesCT CreditAttribution: YesCT commentedunassigning to give a chance to do this novice change.
Comment #53
shnark CreditAttribution: shnark commentedI made the changes that YesCT recommended in comment 51.
I had to indent a bunch of stuff, so the interdiff looks weird.
Comment #54
Gábor HojtsyLooked through the code again, tried it out, it all works and looks great. Ready to go IMHO. Once we are over this, we can move back into #1945226: Add language selector on menus and have it show up in this table without it having a translation checkbox which was the goal for now :)
Comment #55
Gábor Hojtsy#53: drupal8.translation-entity.module.1977784-53.patch queued for re-testing.
Comment #56
alexpottCommitted cb4782e and pushed to 8.x. Thanks!
Fixed the spacing on commit...
Comment #57
Gábor HojtsyI've added a site builder focused change notice at http://drupal.org/node/1987978 (including the original issue, which did not have a change notice). Please elaborate if you think we need other change notices. I don't think there is any API change here, we only *fixed* how the content language config page understands certain properties on entities, it had a misleading understanding not in line with the intention of entity config/annotations.
Comment #58
Gábor HojtsyComment #59.0
(not verified) CreditAttribution: commentedactual issue covering follow-up
Comment #60
plach