Updated: Comment #1
Problem/Motivation
Multilingual support for node base fields at storage level was added in #1498674: Refactor node properties to multilingual, but there is still no way to enter multilingual values from the UI.
Proposed resolution
After the changes introduced in #2004626: Make non-configurable field translation settings available in the content language settings, we just need to mark node base fields as translatable in ther definitions and verify that everything works as expected.
Remaining tasks
- Fix the UI (labels of form elements) by renaming one-off elements according to field names and traversing form structure for title label attachment
- Add test coverage
- Reviews
User interface changes
All translatable node base fields appear in the content language settings page.
API changes
None
Related Issues
#1498674: Refactor node properties to multilingual
#2004626: Make non-configurable field translation settings available in the content language settings
#2019055: Switch from field-level language fallback to entity-level language fallback
Comment | File | Size | Author |
---|---|---|---|
#90 | 2111887-90-head-broken.patch | 892 bytes | ianthomas_uk |
#83 | regresion-2111887-81.interdiff.txt | 6.02 KB | kfritsche |
#83 | regresion-2111887-81.patch | 21.01 KB | kfritsche |
#80 | regresion-2111887-79-1.patch | 22.2 KB | kfritsche |
#61 | language-node_property-2111887-61.patch | 20.09 KB | Gábor Hojtsy |
Comments
Comment #1
plachFor tests to be meaningful it would probably make sense to postpone this on #2019055: Switch from field-level language fallback to entity-level language fallback.
Comment #2
plachtagging
Comment #2.0
plachUpdated issue summary
Comment #3
Gábor HojtsyBump priority since this is a regression from Drupal 7.
Comment #4
Gábor Hojtsy#2019055: Switch from field-level language fallback to entity-level language fallback landed as well, so all dependencies are in.
Comment #5
segi CreditAttribution: segi commentedI started to solve the issue and made a patch for it. In patch made translatable uid, status, created, changed, promote, sticky, log properties. It seems to me it works.
I tested with two languages English and Hungarian.
A found an issue, the "ALL LANGUAGES" title every time appears near the settings label, see on the picture.
Comment #6
plachThanks for working on this!
We have an issue to improve the behavior of those labels: #1498724: Introduce a #multilingual key (or similar) for form element. We probably can address this over there.
Comment #8
Gábor Hojtsy@plach: I don't think this would be committable with the UI saying it does not work :D Do you propose we postpone this or at least not commit this before #1498724: Introduce a #multilingual key (or similar) for form element?
Comment #9
plachWell, that's not exactly the case: most widget labels would work, some would be broken. I wouldn't postpone this on that one. They should be workable in parallel.
Comment #10
plachWe need manual testing here, as some of the queries converted in #1498674: Refactor node properties to multilingual might not work as expect with multilingual values. From the OP:
Comment #11
Gábor Hojtsy@plach: re #9: Well, looks like the screenshot demonstrates the author, publishing info, etc. don't have the widget label right :/ All say they apply to all languages even though the configuration is fully multilingual on the screenshot.
Comment #12
plachYes, but what I am saying is that working on the code here is not blocked on the UX issue :)
I agree we may want postpone committing this one to #1498724: Introduce a #multilingual key (or similar) for form element.
Comment #13
kfritscheFixed the failures from the last test.
Problem here was that as the status is now translatable, we have to set the published set to the translation too, otherwise the translation was unpublished. Which lead to the errors.
This already proofed that we have a test for the status. Also looking at the other tests, we already had tests for title, created and uid. In the doTestAuthoringInfo Test I added sticky and promote so that we have tests for all base fields.
Also I had to add langcode to getFormSubmitAction method, as the status is now translatable and can differ from the entity.
Also did a manual test:
Node edit form of original node (after translation was done, to check if all values are still correct):
Node edit form of translated node (after translation was done, to check if all values are still correct):
Node view of original node (after translation was done, to check if all values are still correct):
Node view of translated node (after translation was done, to check if all values are still correct):
Seems to work fine for me now.
Comment #15
kfritscheUsed at two places the wrong langcode. When adding a translation the values of the source language entity will be used so I have to use the source language there for the submit message.
Should work now.
Comment #17
Gábor Hojtsy15: language-node_property-2111887-15.patch queued for re-testing.
Comment #18
fagoPatch looks good, a few remarks:
Is there a reason for the flipping, could it just be random?
Shouldn't be in there.
Maybe it could just say:
// Publish and promote the translation to frontpage.
Comment #19
kfritscheFixed 1, 2 and 3 ;)
I changed the flip now to random. I wanted to just get sure there that these are different for sure for each language for my internal testing. Changed this too.
Comment #20
Gábor HojtsySo this patch looks great now. I looked at how to solve the UI problem where it says "(all languages)" erroneously and **that is a whole mess**. The form items are nested in all kinds of different ways, so the code where it tries to match up the form[key] with the name of the item totally does not match. The #multilingual form element array key is already used extensively, so the title of #1498724: Introduce a #multilingual key (or similar) for form element is misleading at least... We need to figure out how to match up the node base fields to their form elements.... which sounds like would be the domain of #1988612: Apply formatters and widgets to rendered entity base fields, starting with node.title in fact...
Comment #21
fagoThanks, this looks ready to me. The UI problem is dealt with over at #1498724: Introduce a #multilingual key (or similar) for form element, or where?
Comment #22
plachThe problem, as I see it, is that now the multilingual key is an after-thought. We need every widget to be "aware" of its multilingual status, meaning, for instance, that fieldsets would just be neutral and display no label.
Comment #23
Gábor HojtsyYeah but base fields don't have widgets, and not even after #1988612: Apply formatters and widgets to rendered entity base fields, starting with node.title (at least for anything beyond title). So we need an interim solution if we want this to land and find all the bugs related to this :) Like the search and entity reference bugs we found based on the title translatability already. I think its important to get this in as soon as possible, however, with the UI as-is I think its hard to argue this should land as-is. :(
Comment #24
plachI am not talking about field widgets (fieldsets aren't), I am talking about raw form elements.
@21: yep :)
Comment #25
Gábor HojtsyWell, the problem now is promote is in $form['options']['promote'], uid is edited in $form['author']['name'], etc. There is really no relation of item names or their position in the form structure to base fields. Whatever we do to fieldsets is not going to help us match up items with elements.
Comment #26
plachThen we need a custom solution to be implemented here and to be removed once we have base field widgets.
Comment #27
effulgentsia CreditAttribution: effulgentsia commentedComing here from #2010930: [META] Apply formatters and widgets to rendered entity base fields (all besides node.title), and only reading #23 and #25, I see three options for how to move forward on this then:
1) Allow this patch in despite the UI problem.
2) Work on a subset of #2010930: [META] Apply formatters and widgets to rendered entity base fields (all besides node.title) in this issue (or a spin-off issue that this issue becomes postponed on). By subset I mean: only for those fields that this issue requires, and not changing any field types (e.g., not changing node.created from int to date), but rather creating temporary widgets/formatters to replicate existing behavior, just as a way to unblock this issue, and then cleaning that all up in #2010930: [META] Apply formatters and widgets to rendered entity base fields (all besides node.title).
3) Postpone this issue on #2010930: [META] Apply formatters and widgets to rendered entity base fields (all besides node.title), but that might be a while.
Comment #28
effulgentsia CreditAttribution: effulgentsia commentedBased on #2010930-20: [META] Apply formatters and widgets to rendered entity base fields (all besides node.title), I actually think we should proceed here by renaming $form['author']['name'] to $form['author']['uid'] and similar for all other mismatched field elements, because seems like that's all that's needed to unblock this issue, and it will end up being needed by the work in that issue anyway. Would the people working on this issue prefer to do that in this issue, or a spin-off?
Comment #29
Gábor HojtsyI can hardly imagine a core committer committing this issue without the UI being honest about how it works. So I think we need to rename the elements and do the traversing of the form to put in the right labels (and write tests for that :D). Updated the issue summary for that.
Comment #30
Gábor HojtsyRerolled quickly for the recent changes (in Node.php). Anybody can/want to help with the base field form element renames?
Comment #32
Gábor HojtsyThe search test should not assume specific order of results. Fixed and it works finding the same nodes (but possibly in a different order). Passes green for me now :)
Comment #33
Gábor HojtsyThe more I tried to restructure the form, the more evident it became that doing it here would be prohibitive and counter-intuitive. We should not mix up the translatability related changes with changing the whole form structure. Therefore I opened #2177469: Move node base widgets to the top level of the form and marking this postponed. Sadly :/
Comment #34
Gábor HojtsyComment #35
webchickOk, #2177469: Move node base widgets to the top level of the form is in now, that should make this unblocked.
Comment #36
Gábor Hojtsy32: language-node_property-2111887-32.patch queued for re-testing.
Comment #38
Gábor HojtsyRerolled so it applies again. Let's see this one.
Comment #40
Gábor HojtsyOne more thing got committed in the meantime.
Comment #42
Gábor HojtsygetAuthorId() is now getOwnerId() following #2078387: Add an EntityOwnerInterface.
Comment #43
tstoecklerThis is broken. Instead of doing the comparison (==) manually you should pass the two things that are equal as separate arguments.
Similar problem: Here assertTrue() should be used.
Comment #44
YesCT CreditAttribution: YesCT commentedyep.
https://api.drupal.org/api/drupal/core%21modules%21simpletest%21lib%21Dr...
https://api.drupal.org/api/drupal/core%21modules%21simpletest%21lib%21Dr...
I'll do this.
Comment #45
YesCT CreditAttribution: YesCT commented1. why didn't those fail?
2. they still pass.
needs review for the bot,
but needs work:
3. the language settings vertical tab on the content type (article) edit is empty with the patch!
4. And we might want to come up with an order for the base fields (used to be properties)
Order is:
User ID
Article Publishing status
Article Created
Article Changed
Article Promote
Article Sticky
Article Log
5. Now, the only place to set the base field language and translation settings is on the content language and translation overview page (other fields can have their language set, or enable translation on each field settings page accessible from the content type manage fields pages.
Is that ok?
I think so.
Comment #46
YesCT CreditAttribution: YesCT commentedComment #47
Gábor Hojtsy@YesCT: 1/2: they passed because assertEqual(TRUE, "some text") will pass, since "some text" is also TRUE.
3: can you check the #language_select field added? it may not be adding the right group now.
4: The order comes from the base field definitions. We should define them in an order that makes sense IMHO and/or introduce weights. But then defining them in an order that does not equal weights would be strange, so we should define them in the order that makes sense :D
5. Right, base fields dont have settings pages on their own. I think this is fine. Not in scope here IMHO.
Comment #48
Gábor HojtsyQuick straight reroll since it did not apply.
Comment #49
Gábor Hojtsy@YesCT: I also tried different ways to reproduce the bug with the article content type editing, but could not. The language settings show up fine. Even cross-checked after adjusting language settings in different ways.
Looked into applying labels properly. Now that #2177469: Move node base widgets to the top level of the form landed, the base fields have form elements at the top of the form, so the code in content_translation_form_alter() automagically applies :) Yay. We only need to make details elements exempt from the label marking and then individual items will get proper labels. Such as in this case I configured author to be translatable but not dates and promoted to be translatable but not sticky. I think its probably safe to argue that this may be an artificial situation, but not sure how to handle this, the fields are individual not maintained in the API as field-groups, so we need to support settings on them one by one, we cannot enforce a group of fields to have the same setting. So the helpful note will be on the individual field.
I also fixed the base field labels, so User ID became Author and Promote became Promoted to be aligned with the others.
I've been looking into what to do with the submit button (which is to some degree status based), but that is very custom, not trivial to put into this system. Still pondering there :/
Comment #50
Gábor HojtsySo the top items are #type item from the seven.theme file's form alter, not less! Since these are essentially the same as the other top items, we cannot put them into the form as top items either, they already have widgets with that name, duh. The 'published' item also would need this title addition, but that is special cased in the theme too. Hum. It should use an item title probably so we can alter it consistently. Still to figure this out, it does not seem to be trivial.
In this update I covered the buttons on the form. We need some kind of treatment for them, but not sure about this extra long text :/
Comment #51
webchickHm. I can't either, but probably worth tagging to see if any of the UX folks have better ideas.
Comment #53
Gábor HojtsyThe button labels in tests need an update as well. Will not fix all fails, but will fix quite some.
Comment #55
Gábor HojtsyMore button text changes needed in tests due to the button text change added above.
Comment #57
Gábor HojtsyTranslatability has other conditions that we need to check based on which the suffix may not be present at all. Heh.
Comment #59
dclavain CreditAttribution: dclavain commentedFixed error php.
Comment #61
Gábor HojtsyRerolled so it applies following changes to node changed and created field type changes.
Comment #63
Gábor HojtsyFixed the test, so it uses the right conditions (same as in runtime), which required $langcode to be passed further down. Also in some cases the langcode was not the proper one sent. Finally, add some comments to make clearer how this works. The term and user translation tests (both of which were failing before) work fine locally, let's see the full suite.
Comment #65
Gábor HojtsyI have issues running NodeTranslationUITest locally, so that will be interesting... :/ Anyone with insights. If not soon, I think I will experiment with some alternate ways to run this test locally tomorrow. I could run the other tests that were failing before, but now this is the only one...
Comment #66
BerdirThat test has lots of methods and each of them runs thrugh the lengthy set-up process. If you didn't try that yet, I'd suggest to disable (by renaming testBla() to dTestBla() all tests out except a single one that is failing and then just try that.
Comment #67
BerdirHad a look at this, the problem is that the test where it decides how the submit button is labelled, he sees 'it' as unpublished, but when the testcode itself runs, it is published. So they don't match.
I'm not sure why, I guess there's some sort of initialization going on that sets the default status when a new translation is added?
Comment #68
jsbalseraI'll work on this, I'm in the Multilingual table.
Comment #69
vasi1186 CreditAttribution: vasi1186 commentedThe issue is in the NodeTranslationUITest::getFormSubmitAction(). I guess that the logic there does not match the one in NodeFormController::actions(). In the later one, the text on the buttons depends on two variables (if the node is new and/or published). In the test function, we check only if the entity is published.
And more specific, this code in the test class:
returns false, so the button will have the text: "Save and keep unpublished"
In the form controller class, the $node->isPublished() seems to be true. So, the buttons will have "Save and unpublish" and "Save and keep published" texts. No one matches the one from the test class...
This raises a question: is there an issue in the controller, or should we change the test class?
Comment #70
BerdirThe logic is the same IMHO but the state of the entity is not. @fago is sitting front if me (is laptop is at least), I'll ask him.
Comment #71
vasi1186 CreditAttribution: vasi1186 commentedAttached the code that should fix the tests.
@Berdir, @fago: can you please also have a look on it (especially the interdiff).
Comment #73
vasi1186 CreditAttribution: vasi1186 commentedok, let's try again. The interdiff should be the same.
Comment #74
kfritschePatch seems reasonable for me.
Tested it manually and the form is fine, but the labels aren't working at all, like mentioned in #49.
But currently it seems, like its the problem of the field definition.
What I did:
* Fresh install in German with patch
* Enabled content translation and added a language
* Only set body translatable for basic pages and everything (also the new base fields) for articles
* Created a article and basic page
* Created a translation for the article and the basic page
* Noticed similar behavior like in #49 (last saved, author has "all languages" and fields which shouldn't be translatable don't have "all languages")
* After saving the translations, I was able to have different values in the title, even that I disallowed for basic pages
Comment #75
kfritscheComment #76
kfritscheSetting back to needs review, as the goal of the issue is working and everything seems good.
The issue I posted is tackled in #2217543: Cannot translate title of Basic Page node if other content types are not translatable and #2143291: Clarify handling of field translatability.
Not sure if we really have to postpone this now, as what this issue is about is working and tests are green. [me want this in finally...]
Comment #77
plachYep, I don't think there is a real need to postpone this issue to those referenced in #76 either.
Looking at this just now, review coming soon.
Comment #78
plachThis is looking good to me, only some easy fix missing:
We don't special-case entity-defining modules in CT: this code should definitely move to
NodeTranslationController::entityFormAlter()
.This implementation should move to
NodeTranslationUITest
as it is node-specific. The base-class implementation should just return an empty string.
We should probably change the description accordingly
I am not sure about this change, it implies that the search results order can be different depending on the translation language. We should check with Gabor, who worked on the multilingual search issue, I think.
Comment #79
plachForgot about this:
This description needs to be somehow reworded to have it on just one line. If that's not enough we should have a brief description, a blank line and then a longer description taking as many lines as needed.
Comment #80
kfritscheRerolled and fixed the issues noted by plach.
Except the one with the search results. I ran the test locally a couple of times and it seems to switch the order from time to time. Will ask Gabor at D8MI meeting later about it.
Comment #81
plachThanks, there's still an issue with the current patch:
As I said, this code block should live in
Drupal\node\NodeTranslationController::entityFormAlter()
, we don't want to pollute the CT code base with entity-specific stuff.Comment #83
kfritscheAh now I got it.
Moved the status part to the node translation and added a comment to the multilingual search part after talking to Gabor. Also moved the rest to the part it was again.
Comment #84
plachAwesome, I think this is good to go now!
Comment #85
webchickSince Gábor's been very involved here, it'd be nice to see him sign off on this RTBC.
Comment #86
plachTotally agreed, I wanted to do that too but I must have been too excited and I forgot about that :)
Comment #87
Gábor HojtsyThe changes look good. Especially where the form suffix handling is moved to the node handler instead of the general content translation handler. We even discussed the search changes in person.
Comment #88
alexpottCommitted 1ad4630 and pushed to 8.x. Thanks!
Fixed the use of
t()
during commit.Comment #90
ianthomas_ukWhich seems to have broken HEAD. This patch just reverses that change.
Comment #91
webchickHm. No idea why that would be the case, but committed and pushed #90 to 8.x in the hope that fixes testbot. :)
Comment #94
webchickTestbot trying to apply patch that's already committed to HEAD. Resetting status.
FWIW though, bizarrely, that did seem to fix the problem. https://qa.drupal.org/8.x-status is showing a green board now. So marking as "needs followup" because we should figure out a way to apply that patch in a way that doesn't break HEAD, but don't need to do so in a critical task.
Comment #95
BerdirWell, not very surprising, this is a translation controller, which doesn't have the t() or any other of those fancy methods :)
Comment #96
webchickOh, well there you go. ;)
Comment #97
Gábor HojtsyYay! Thanks all for your help!