Updated: Comment #99
Problem/Motivation
Follow up for #1498674: Refactor node properties to multilingual. Node base fields are translatable in storage but not configurable in the UI.
Steps to Test
From Kristen Pol in #30 (plus updates).
- Reset 8.x
- Apply the latest patch
- Install Drupal in English
- Enable language and content translation modules
- Add a language
- Go to content language settings page (admin/config/regional/content-language) Home > Administration > Configuration > Regional and language
- For Custom Language Settings, check content, and then check translatable for article.
- Notice checkbox for title (not to be confused with the title of the image field).
Proposed resolution
Show on the content language settings page also non-configurable fields having the translatable
property set to true in their definition. Content Translation uses this information to tell whether the field supports translation and alters the field definition based on the actual translatability status stored in its own config settings. For instance:
- The node title definitions says it is translatable
- The content language settings page shows it as untranslatable, because CT has no settings stored for it yet
- Node title is marked as translatable in the UI
- Content Translation alters the field definition to reflect that (actually nothing changes)
- Node title is marked as untranslatable in the UI
- Content Translation alters the field definition to reflect that (the field definition is switched to untranslatable)
The title field is used as a proof of concept here, other node fields will be handled in a follow-up.
Remaining tasks
Write a patchAdd test coverageReviewsMake the issue summary proposed resolution accurate
User interface changes
Properties will show up alongside configurable fields in node translation configuration.
Before (from #46):
After (from #46):
Without the patch, the admin content listing has a row for each translation and the original source node title is shown for all. With the patch, the translated title is shown for translation, gotten from the database which now saves the translated title for each translation.
After (from #46):
API changes
An exception is thrown when trying to make entity id, uuid, revision id, bundle and language fields translatable.
In #38 on 6/27, @Dries states,
As this discussed with the D8MI team, this is part of 4 critical issues for D8MI:
. So this is an approved API change at #43
Comment | File | Size | Author |
---|---|---|---|
#30 | d8mi-prop-trans-content-config-all-lang.png | 167.65 KB | Kristen Pol |
#30 | d8mi-prop-trans-content-config-no-props.png | 47.71 KB | Kristen Pol |
#46 | 2004626screen9.png | 54.57 KB | andymartha |
#46 | 2004626screen8.png | 31.65 KB | andymartha |
#46 | 2004626screen7.png | 58.64 KB | andymartha |
Comments
Comment #1
Gábor HojtsyRetitle to be more specific, add sprint tag.
Comment #2
PanchoLeveraging the new Entity Field API. The tag hopefully bring additional visibility to this one.
Comment #3
plachWorking a bit on this.
Comment #4
plachHere is an initial patch. Keep in mind that although the configuration screen works correctly, node translation support is stilly buggy due to the lack of #1810370: Entity Translation API improvements. AAMOF you'll be able to enter translated values (you can check the DB), but node form and visualization will still always present (non-configurable) field values in the original language.
Comment #6
andypostthis makes all fields translatable so no ability to prevent field translatable
Comment #7
plachI dont' see why: the patch handling of non-configurable fields simply ignores fields that are maked as non translatable in the definition, hence there will never be a config setting for those fields. Non-configurable fields that are marked as translatable can have translation enabled/disabled from the UI.
Comment #8
plachI'll work on another issue tonight. Feel free to work on this.
Comment #9
yched CreditAttribution: yched commentedDoes this mean the 'translatable' bool property on "Field API" configurable fields would/should be stored by translation_entity.module in a CMI record of its own, rather than as part of the $field config entity currently ?
Comment #10
plachYes, currently this is the situation. I already thought this might need some kind of cleanup as I don't really like the current special-casing of Field API fields: #2018685: Remove field_is_translatable() seems the right place to do that clean-up :)
Once we are done with the Field NG conversion, we could make all Field API fields translatable by default and store any alteration to their translatability status in the ET settings only. That is we would extend the patch behavior for non-configurable fields to all fields.
Comment #11
yched CreditAttribution: yched commentedSounds like a sensible approach, +1.
Comment #12
plachYesterday I tried this combined with #1810370: Entity Translation API improvements but it still fails, because we need also the node form parts of #1939994: Complete conversion of nodes to the new Entity Field API and #2019055: Switch from field-level language fallback to entity-level language fallback for everything to work correctly.
This will also be heavily affected by #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets, as it turned out in #2018685: Remove field_is_translatable(). If we want to commit this now, I am afraid we need to revert node changes and reduce the scope to the ET module alone. If we want to see translated titles we need to postpone this to the issues above :(
Comment #13
Gábor HojtsyHow can we fix (and get committed) all these issues in two weeks?
Comment #14
Gábor HojtsyComment #15
plachWell, I hope #1810370: Entity Translation API improvements and #2019055: Switch from field-level language fallback to entity-level language fallback will be ready this week. But in no way the node conversion will be complete in two weeks, I'd guess.
That's why I'd suggest to rescope this to the test entity for now. Once the conversion is over, enabling the node properties on the UI (or the ones of any other entity type) will be automatic by simply setting their field definitions to multilingual.
Comment #16
plach#1950632: Create a FieldDefinitionInterface and use it for formatters and widgets has been committed, btw.
Comment #17
Gábor HojtsyIf we only solve this for the test entity, it will still be an API change for nodes, no? Nodes are our most important entities in core, so if we say we have an API freeze and then freely change node behaviour, that is not too honest.
Comment #18
plachI don't see this as an API change: the whole functionality would be in place. The only change would be in node field definitions, which as part of the NG conversion should be allowed to be done after code freeze, if I didn't misunderstand http://buytaert.net/reducing-risk-in-the-drupal-8-release-schedule.
Anyway, the only viable alternative I see is fixing tests and committing this as-is, although translatable titles won't work because of the incomplete node NG conversion.
Comment #19
Gábor Hojtsy"as part of the NG conversion should be allowed to be done after code freeze" sounds like even more things API changing to be done after a freeze? Anyway, sounds like there is no point in this issue in itself, let's work on the others.
Comment #20
plachI really think we should try and get the non-node changes in ASAP, just in case. I don't see the risk honestly. If we are'nt allowed to make node field definitions translatable afterwards, too bad, but the situation will be way more complex if we won't have this in.
Comment #21
Gábor HojtsyComment #22
plachOk, rescoped the issue. I reverted the node-related changes. I'll work on test coverage tonight.
Comment #24
plachThis should be green.
Comment #25
Kristen Pol#24: et-nc_field_config-2004626-24.patch queued for re-testing.
Comment #26
Kristen PolI got an error when patching so I'm running tests again. I tried patching two times and same result for both.
Comment #28
plachHere is a reroll. Test coverage still todo.
Comment #29
Kristen Pol@plach - Thanks for the reroll... I might be not configuring things correctly but I see "all languages" next to all my properties and the properties don't show up on the content language settings page for me to enable for translation. Am I missing a step?
Comment #30
Kristen PolHere's what I did:
Comment #31
plach@Kristen:
This is just the API part now. As written above making node properties translatable is not working properly because the node form and visualization code are not fully converted to NG yet. The patch provides only the infrastructral changes needed to make those translatable once the NG conversion is complete.
If you wish to manually test this you can change the base field definitions in the node storage controller and make them translatable (add a
'translatable' => TRUE entry
). See the earlier patches for details.Comment #32
Kristen PolAh... thanks for the explanation... as for:
I'm not seeing anything obvious in the comments about how to do this. I don't know what the "node storage controller" is ;)
Comment #33
plachlol, silly me :)
You have to change this file manually as described in #31.
Comment #34
Kristen PolHmm... tried that and cleared the cache multiple times but the properties still are changing for both languages upon node editing (and still say "all languages"). I'll leave it up to someone more familiar with this issue to test.
Comment #35
plach#28: et-nc_field_config-2004626-28.patch queued for re-testing.
Comment #36
Gábor HojtsyComment #37
PanchoPlain reroll of #28 after #2024867: Rename translation_entity to content_translation.
Comment #38
Dries CreditAttribution: Dries commentedAs this discussed with the D8MI team, this is part of 4 critical issues for D8MI:
As the product owner, I consider this to be critical for Drupal 8. Not being able to translate node titles is bad. Not having this in core would be a regression from Drupal 7.
Comment #39
plach#37: et-nc_field_config-2004626-37.patch queued for re-testing.
Comment #40
Gábor Hojtsy@plach: what do you think should still be done here?
Comment #41
plachI think we are just missing some test coveragel I'll check later tonight.
Comment #42
plach#37: et-nc_field_config-2004626-37.patch queued for re-testing.
Comment #43
Gábor HojtsyAdding tags as per #38.
Comment #44
Gábor Hojtsy#37: et-nc_field_config-2004626-37.patch queued for re-testing.
Comment #45
andymartha CreditAttribution: andymartha commentedI am working on a review message + issue summary now; hold on...
Comment #46
andymartha CreditAttribution: andymartha commentedAfter applying the patch et-nc_field_config-2004626-37.patch by Pancho in #37 to a Drupal 8 fresh install 8/16 , here are my findings via screenshots.
1) drupal head 8/16/13 does not contain settings for translatable node titles at admin/config/regional/content-language
2) without patch et-nc_field_config-2004626-37.patch, following steps at #30 , the title is still an "all languages" field and there is an error message shown as "invalid argument supplied for foreach() ...
3) without patch, the translated node shows a translated body without translated node title
4) after applying patch, the error message goes away in #2 but the node edit form title field still displays "all languages"
5) before/after applying patch, editing a translated version of the saved node for me produces a WSOD.
PHP Fatal error: Call to a member function bundle() on a non-object in /Applications/MAMP/htdocs/drupal/core/modules/node/node.module on line 136
This error is being addressed in #1831846: Help block is broken with language path prefixes
6) after applying patch, creating a new node, then creating a translation of that node, it shows the translated title in drupal_set_message save message and saved in the database in table node_field_data
7) after applying patch, Drupal DOES contain settings for translatable node titles at admin/config/regional/content-language (this is a change to point 1)
8) the content page at admin/content shows the translated titles of the nodes. Good. But, for example, node/1 shows up as Node 1 and es/node/1 shows up as Cosa Uno, but both link to the original translation...ie...node/1
9) viewing the translated node still shows the original language node title at es/node/3 instead of translated title, but it seems that that may not be this issue's problem.
I'm not sure I understand the actual goal of this issue totally yet but as @plach says in #31
Comment #46.0
andymartha CreditAttribution: andymartha commentedUpdate
Comment #46.1
andymartha CreditAttribution: andymartha commentedfrom the mwds sprint on technical debt
Comment #46.2
YesCT CreditAttribution: YesCT commenteda bit of formatting.
Comment #46.3
YesCT CreditAttribution: YesCT commentedtried to update resolution, added more issues that have come up as related
Comment #46.4
YesCT CreditAttribution: YesCT commentedmore formatting.
Comment #46.5
andymartha CreditAttribution: andymartha commentedafter discussing with YesCT, decided to clarify UI changes section.
Comment #46.6
YesCT CreditAttribution: YesCT commentedwasn't really a UI change, saving to the db. but the content listing is a ui change.
Comment #46.7
YesCT CreditAttribution: YesCT commentedput ui change screenshots in summary
Comment #46.8
YesCT CreditAttribution: YesCT commentedputting class name in text
Comment #46.9
YesCT CreditAttribution: YesCT commentedadded remaining task to still update proposed resolution.
Comment #47
Berdir#37: et-nc_field_config-2004626-37.patch queued for re-testing.
Comment #49
vijaycs85Re-rolling #28
Comment #51
vijaycs85Fixing test fails in #49
Comment #52
rteijeiro CreditAttribution: rteijeiro commentedI'm going to review this one :)
Comment #53
rteijeiro CreditAttribution: rteijeiro commentedPatch applied and followed the steps to reproduce but it's still not working.
Comment #54
pfrenssenIf it is not working and the tests are green, then we have a problem with our test coverage.
Comment #55
vijaycs85The patch in #51 is green, but it is not doing what it suppose to. We don't have the title field (#8 in steps to test).
Comment #56
vijaycs85The patch in #51 is green, but it is not doing what it suppose to. We don't have the title field (#8 in steps to test).
Comment #57
kfritscheAdding a very simple test which just tries to enable the title in the content translation settings page.
This fails if not exists.
Added a test-only patch for this, which has to fail.
Second attached patch is the patch from #51 with the test and a temporary fix to make title translatable like discussed with vijaycs85.
Also did a manual testing on the patch.
* Title appears now in the settings form.
* Title is always translatable - setting doesn't change this
This is because content_translation_entity_field_info_alter won't be called as title is no field. So the settings is never really called.
Also I'm not sure if the syncing would work, as it is no field and so it will be not synced.
But the main intention of the ticket to make it possible to configure the properties is working and we now have a small test for it.
Comment #59
kfritscheSomething seems still not working with the comment subjects.
Removed comment subject as translatable to get the tests passing.
Comment #60
vijaycs85Great work @kfritsche, can you post a test-only patch as well please?
Comment #61
kfritsche@vijaycs85: See #57 first attachment there is the test only patch.
As you can see in the details there, this is failing with another error than the second "real" patch.
Patch in #59 just fixed this new error.
Comment #62
andypostprobably this broken because we are going to drop the way we translate fields, see @plach comments in content translation form controller issue
Comment #63
plach@andypost:
That's a bit misleading :)
To clarify for people here, I just said we should drop the current code to migrate data when switching field translatability, as it will hopefully become unnecessary. See #2076445: Make sure language codes for original field values always match entity language regardless of field translatability.
Comment #64
penyaskitoOnly the title config is translatable at the moment, and there are a lot more, but can be handled as follow-ups.
When I mark the title for translation, the title is translation on the UI. However, hen I save a translation, the translation is not shown, but the source, (the process ends successfully, but after editing a node the title is shown in the target language.
Clearing cache does not help to fix the problem.
Comment #65
Gábor HojtsyI'm not entirely clear on this. Is the title saved and loaded back properly in the form? Is it only not applied on the display? Is the title saved at the end to the db?
Comment #66
Gábor Hojtsy@penyaskito: So as for displaying the proper node title for the translation, #2019055: Switch from field-level language fallback to entity-level language fallback is the issue that generally solves that for fields. This issue would be the configuration so the right form element shows and its saved to the right place (and loaded back on the next form). If the title is not rendered properly at the end, it is still fine for this patch.
We need to ensure here that the form saves to DB and edit again in that language works and original language edit also works and is still the original value. Tests for this would be essential too :)
Comment #67
penyaskitoThis works. The title is saved to node_field_data correctly, and it is loaded correctly in the edit form.
AFAIK, ContentTranslationUITest::assertBasicTranslation is testing this in a generic way, but there is no explicit test for the title. I would say that still needs tests.
Comment #68
plachI'm working on improving test coverage, patch coming soon.
Comment #69
plachOk, cleaned-up some stuff and added test coverage for node title translation and display. The latter is done by checking the output of the translation overview page since, as Gabor was pointing out above, display will need to be fixed after #2019055: Switch from field-level language fallback to entity-level language fallback goes in. We should open a follow-up to make all node base fields are translatable (just change their definitions) and ensure node titles appear in the correct language everywhere.
Comment #70
kfritscheMinor spelling -> Fixed it
Setting it to RTBC as couple of people reviewed it and as Gabor mentioned in #66 this is just so that the title field is available. For the part mentioned by penyaskito in #64 we have another issues.
Also did a final manual check. The title appears now correctly in the content language settings page.
Comment #71
kfritscheRTBC when testbots agree once more. (Should be just a comment change)
Comment #72
alexpottPatch no longer applies.
Comment #73
penyaskitoRerolled.
Comment #74
plachThanks!
Comment #75
fagoI cannot follow the reasoning here. We have translatable node fields, so why shouldn't they be marked as translatable?
Comment #76
plachWe are making the node title translatable just as a proof of concept. To make every node property translatable we might need to adapt some code to ensure everything works correctly in a multilingual context, which is out of scope here, we need a dedicated issue for that.
Comment #77
fagoStill, I cannot follow the reasoning of the code cited in #75. Why would we dis-allow base fields to be translatable? If the node title is translatable, we need a way to mark it as translatable - but the patch removes this?
Comment #78
plachThe patch ensures that only
entity_id
,revision_id
,bundle
,uuid
andlangcode
fields are never made translatable. Maybe the$base_fields
variable name is misleading.Then it just makes field definitions default to untranslatable, that is we require fields to be explicitly set to translatable. Aside from title, which is already made translatable, we will define node fields as translatable in a follow-up.
Comment #79
plach@fago:
I honestly do not understand your concerns in turn :)
Since this has been stuck way too much in the past months, can we please unblock this situation quickly?
Comment #80
fagoI see, thanks. Yeah "base fields" is really misleading - it's just about "entity keys" then.
But still, why do we need the special case for those? Content translation should not try to enable translation for them in the first place, so if we need to special case it why not do it on the content translation side?
If we want the entity system to forbid translation being active on those fields, it shouldn't silently change it around but throw an exception to tell developers that something is wrong and won't work as defined.
> Since this has been stuck way too much in the past months, can we please unblock this situation quickly?
yeah, sry for that. Feel free to ping me on irc/skype/mail regarding to topic to sort it out asap.
Comment #81
plachOk, this works for me either. I'll provide a new patch ASAP.
Comment #82
plachHere is the exception plus test coverage.
Comment #84
plachNo failures here, trying a reroll.
Comment #85
Gábor Hojtsy@fago: ok, what do you think?
Comment #86
fagoYeah, the interdiff looks good - thanks. However, looking at the patch size I guess the latest re-roll includes something else also?
Comment #87
plachSorry, I was on the wrong branch.
We actually have a new failure due to #2106349: Comment translation overview has broken local tasks, which completely breaks the comment translation overview. We need either to postpone this on that one or provide a stop gap fix and skip testing the comment translation overview.
Since for the purpose of this issue having nodes test-covered is enough, the attached patch implements the latter.
Comment #88
Gábor HojtsyYeah I don't think this is committable as-is due to the comment workaround. Let's fix that. I posted a patch there that needs a slight update I think to apply/pass :)
Comment #89
Gábor HojtsyComment #90
plachOTOH if we commited this, it would be way easier to provide test coverage for the other issue.
Comment #91
plachcrosspost
Comment #92
Gábor Hojtsy#2106349: Comment translation overview has broken local tasks landed. Let's reroll this without the workaround and then get it in :)
Comment #93
plachOn it
Comment #94
plachRerolled and removed the workaround :)
Comment #95
Gábor HojtsyBack to RTBC as per #86. Although the latest interdiff does not include removal of the doTest... changed from the previous patch, I manually verified they are not there anymore either.
Comment #96
swentel CreditAttribution: swentel commentednitpick, typo in 'condfigurable'. Don't want to hold up on the commit for that though, can maybe cleaned up during that :)
Comment #98
plachFixed a merge issue and the image field sync test, which was not behaving correctly after the changes introduced here. Hopefully we should be green again now.
Comment #99
Gábor HojtsyThe typo from #96 was also fixed :) Re-rtbc :)
Comment #99.0
Gábor Hojtsyabout duplicates in content list.
Comment #99.1
plachUpdated issue summary
Comment #99.2
plachUpdated issue summary.
Comment #99.3
plachUpdated issue summary.
Comment #100
plachComment #100.0
plachUpdated issue summary.
Comment #101
plachJust a reroll
Comment #102
YesCT CreditAttribution: YesCT commenteddo we want issues opened for these?
Comment #103
plachNot sure about that, those lines would probably need to be converted in the issue actually unfiying field definitions.
Comment #104
YesCT CreditAttribution: YesCT commentedis that #2047229: Make use of classes for entity field and data definitions ?
Comment #105
plachYes, I did not look at that one yet. If the Field API field definitions are removed then it will need to convert also the code we have here, otherwise follow-up :)
Comment #106
catchI was going to ask if we need issues for those as well, but if we already have an issue that's fine.
Committed/pushed to 8.x, thanks!
Comment #107
catchOr this..
Comment #108
Gábor HojtsyLooks like the next big issue is #2019055: Switch from field-level language fallback to entity-level language fallback which would let us work on fixing the *display* of the translations to actually work, which it does not do yet. What is blocking making other node properties configurable to be translatable?
Also I'm not sure I can meaningfully write a change notice for this, so hope plach can do it :)
Comment #109
plachNot much to say on the API side, changes here are mostly in the Content Translation code and UI, but AFAIK we do not provide HEAD 2 HEAD change notices so I didn't mention those:
https://drupal.org/node/2111871
Comment #110
plachStrictly speaking, nothing. I created #2111887: Regression: Only title (of base fields) on nodes is marked as translatable for that. However it would be probably better to have #2019055: Switch from field-level language fallback to entity-level language fallback in first.
Comment #111
fagoI'm wondering why we need a change notice here at all? "Field definitions for entity keys and language cannot be made translatable" is not new compared to D7 - they've never been field-translatable?
Comment #112
Gábor HojtsyIMHO the significant change in this issue is rather that if you define a base field in your baseFieldDefinitions() as 'translatable' => TRUE, then it will be offered up for translatability configuration and properly participate in translation forms (if configured translatable) and stored translated proper. That is what was not here before this issue :)
The exception, yeah, well, that is also there.
Comment #113
Gábor HojtsyNote that I found #2112303: Random extra text around translatability configuration is confusing while reviewing what this patch changed, but that is not related to what this patch changed. Just very confusing.
Comment #114
YesCT CreditAttribution: YesCT commentedyeah, I tried out #112 and got the revision log to show up in the language settings page.
Comment #115
plachI am not sure this is exactly change notice material, at least according to the idea I had of those, however I added the information suggested in #112.
Comment #116
plachChanges from Gabor look good to me, I updated the change notice to clarify that this applies to any field, not just base fields.
Comment #117
Gábor HojtsyI made some further changes as pointed out by @plach, making sure there is cross-reference for needing to make the schema multilingual as well as that the property does not itself become translatable with this definition, it just informs the system it can be made translatable. Full change notice at https://drupal.org/node/2111871. I think this should be good now.
Comment #118
plachTo answer to #102: if the @todos are not cleaned-up in the unification issue we will deal with those in #2018685: Remove field_is_translatable().
Comment #119.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #120
klonos...restoring tags eaten by the tag monster.
While at it, also moved related issues to their dedicated metadata section.
Comment #121
klonos