Problem/Motivation
Ok, just had a great call with Gábor, plach, and Bojhan where we walked through the current UI, proposed UI, and the D7 UI of Entity Translation module. Here were our findings:
Follow-ups we encountered while testing:
- (normal task) "Translation is not supported if language is always one of: Not specified, Not applicable, Multiple" reword to: "....
Proposed resolution
(current) proposed rewording in #23
Remaining tasks
- Done in #14: Reporduce and get a screenshot of the message.
- Done: Reword message
- Done in #22: Follow-up to prevent the need for an error message at all. #1834266: Force site builders to make only valid choices when configuring entity default language with translation enabled
- Provide a patch (@Lukas ?)
- Review
Original report by webchick
This is a follow-up coming from the meta issue #1188388: Entity translation UI in core
Comment | File | Size | Author |
---|---|---|---|
#58 | translation_is_not-1831636-58.patch | 1.07 KB | Manuel Garcia |
#50 | translation_is_not-1831636-1.patch | 1.05 KB | alansaviolobo |
#44 | before-2013-02-09_0133.png | 111.89 KB | YesCT |
#42 | 2013-02-09_0129.png | 125.67 KB | YesCT |
#35 | drush-selection_errormsg_translation_entity-1831636-35.patch | 1021 bytes | claudinec |
Comments
Comment #1
plachComment #2
YesCT CreditAttribution: YesCT commentedLukas, Thank you for opening this issues, it's a help.
For anyone:
Next steps:
To figure out what to do about this issue, get the issue summary template from http://drupal.org/node/1155816
Find the relevant comment from the epic et-ui issue that asked for this follow-up and bring over the quotes so we have background context for what to think about (use blockquote html tag for them).
Comment #3
YesCT CreditAttribution: YesCT commentedI did a:
grep -R "language is always one of" *
And found
More next steps:
get a screen shot
write steps to reproduce (steps to arrive at the screen)
Comment #4
YesCT CreditAttribution: YesCT commentedMore next steps:
Edit the issue summary at the (new) main issue #1831530: Entity translation UI in core (part 2) to link to this issue using [ # 999999 ] pattern (no spaces in the pattern)
make a comment on #1831530: Entity translation UI in core (part 2) saying this followup was created and link to this issue
Comment #5
plachComment #5.0
Lukas von BlarerUpdated issue summary.
Comment #6
moe4715 CreditAttribution: moe4715 commentedI found a way to reproduce the problem (s. attachment).
1. Enable the "Language" and "Content Translation" module
2. Go to the page http://hostname/admin/structure/types/manage/article
3. Click the "Language settings" tab on the right hand side
4. Choose " - Not specified - " as the "Defalult language" and check all checkboxes below
5. Click "Save content type" and the message will appear at the page top
Comment #7
moe4715 CreditAttribution: moe4715 commentedWe would change the sentence "Translation is not supported if language is always one of: Not specified, Not applicable, Multiple" to "Translation is not supported if the default language is "Not specified", "Not applicable" or "Multiple" and the "Hide language selection" option is checked".
Comment #8
moe4715 CreditAttribution: moe4715 commentedI created a patch with the aforementioned text message that replaces the current error message.
Comment #10
Lukas von Blarer#8: 1831636-selection-errormessage-corrected.patch queued for re-testing.
Comment #11
plachThis should be fixed in the Entity Translation module, the Content Translation module is deprecated and will be removed.
Comment #12
plachComment #13
ancamp CreditAttribution: ancamp commentedI created a patch where i changed the error message in the Entity Translation module.
Comment #14
YesCT CreditAttribution: YesCT commentedSteps to reproduce (based of of @moe4715 in #6. Thanks!):
Message with the patch:
Here it is in context
Here is what it said before the patch
Review
The new message is clearer and more accurate with the mention that this happens when the language selector is hidden too.
It needs a period at the end of the message.
Comment #15
YesCT CreditAttribution: YesCT commentedAfter a second look, I think "is one of" makes it grammatically correct (because we cannot put an or in the list of languages).
I added that and the period.
Here is the new message:
Comment #16
Gábor HojtsyLooks like a great improvement!
Comment #17
webchickWe (Gábor, YesCT, and I) showed this error message to a few different people, and they found it a bit confusing. We discussed it and think it would be better, rather than displaying all of the possible wrong choices someone could make, that instead it just said some language to the effect of "Invalid choice selected. Can't use %choice with translation." We are working on better wording right now. :)
Comment #18
YesCT CreditAttribution: YesCT commentedThis is better because it explains why and what the next step is.
Needs to pick a word better word for real that means the opposite of the system languages.
What about "specific"?
Comment #19
YesCT CreditAttribution: YesCT commentedMight still be too long.
Comment #20
Lukas von BlarerWouldn't this be better:
"Hide language selection" is not compatible with content with %choice. Either pick a specific language or do not hide the language selection.
Comment #21
YesCT CreditAttribution: YesCT commentedComment #22
YesCT CreditAttribution: YesCT commented#1834266: Force site builders to make only valid choices when configuring entity default language with translation enabled is a follow up. It should not block this issue.
Comment #23
YesCT CreditAttribution: YesCT commentedThis message merges @Lukas's suggestion from #20 with #19 and takes the approach of specifically saying what was wrong (but not all the wrong choices) and then separately saying how to fix it. So it might address the concern @webchick and others had from #17
@Lukas want to update the patch? :)
Comment #23.0
YesCT CreditAttribution: YesCT commentedProviding summary
Comment #24
Lukas von BlarerWhy do you write the message in two lines? Should it be presented like that in one message?
Comment #25
YesCT CreditAttribution: YesCT commentedI wrote it in two lines because I assumed no one would read all the way to the end of a long line.
And the second line tells the person the action they can take.
I wonder if there is another pattern in core we can look at.
Comment #26
Lukas von BlarerYes, this would be interesting to know. I agree with you that this is more understandable like this. But I have never seen messages like this in Drupal. What should we do?
Comment #27
YesCT CreditAttribution: YesCT commented@Lukas make a new patch for the idea in #23.
Post an interdiff too.
Do manual testing and get a screenshot of the error.
Then we can get a review. :)
Comment #28
kbasarab CreditAttribution: kbasarab commentedUpdated patch based on #23.
Interdiff with #15.
Comment #29
Gábor HojtsyThanks! I think except the two line breaks, this looks good. We don't do multi-component error messages like this elsewhere.
Comment #30
claudinec CreditAttribution: claudinec commentedUpdated patch in #28 with line-breaks removed.
Comment #31
jlscott CreditAttribution: jlscott commentedThanks for the revised patch @claudinec. I have tested and confirm that it does remove the unnecessary breaks.
Is there another patch that needs to be applied to change the wording used in the error message so that it uses the same terminology as the fields in the form, i.e. refering to "Default lanaguage" rather than "language" and "Show language selector" rather then "Hide language selection".
See attached screenshot.
Comment #32
chrischinchilla CreditAttribution: chrischinchilla commentedHi, tried the steps in #14 to reproduce this error and I don't get the message. Not sure if that's a problem, but just saying.
Comment #33
claudinec CreditAttribution: claudinec commentedNew patch with fixes suggested by @jlscott in #31.
Comment #35
claudinec CreditAttribution: claudinec commentedThis one should pass.
Comment #36
claudinec CreditAttribution: claudinec commentedComment #38
webchick#35: drush-selection_errormsg_translation_entity-1831636-35.patch queued for re-testing.
Comment #39
xjmIt looks like this hunk was introduced accidentally, and is also the reason the patch wasn't applying. The patch in #35 removes it.
The text change looks correct to me!
I verified that the patch applies and does not introduce any syntax errors.
Comment #40
webchickLive from the code sprint at DrupalCon Sydney! Committed and pushed to 8.x. :D YAY!
Comment #41
Gábor HojtsyUhm, maybe the updating of the text for the current chechbox name was done in a hurry in #33. Now it says the show language selector feature is not compatible with some of the language choices, but the truth is the contrary. *Unchecking* that checkbox creates the incompatible state.
Comment #42
YesCT CreditAttribution: YesCT commentedhere is what core looks like right now:
Comment #43
Gábor HojtsyRight, so we need the hunk that @xjm identified as accidental too, or we don't get the language name either :) Recategorizing as bug.
Comment #44
YesCT CreditAttribution: YesCT commentedbefore the commit, with no patch, it looked like:
Comment #45
YesCT CreditAttribution: YesCT commentedsince #1853720: Hide language selection option is backwards got in
The second phrase should probably say: Either pick a specific language or show the language selector.
the first part is trickier... if we want to talk hiding the language selector not being compatible (instead of translation not being compatible)...
"Show language selector" disabled is not compatibile with translating content that...
or
Not showing the language selector is not compatible with translating content that...
(ack. lots of negating statements)
How about:
Hiding language selection is incompatible with translating content that ...
It needs to say ... with translating content that has default language: Not specified.
[edited to include the default language wording]
Comment #46
YesCT CreditAttribution: YesCT commentedI really like the improvement of using "default language" instead of "is always". :)
Consolidating previous comment, I suggest:
Hiding the language selection is incompatible with translating content that has default language: Not specified. Either pick a specific language or show the language selector.
Comment #47
YesCT CreditAttribution: YesCT commentedcross-posted, restoring category
Comment #48
Gábor HojtsyComment #49
jair CreditAttribution: jair commentedComment #49.0
jair CreditAttribution: jair commentedUpdated issue summary remaining tasks to reflect background done, re-wording done and next step is making a patch.
Comment #50
alansaviolobo CreditAttribution: alansaviolobo commentedbased on the patch in #35
Comment #51
alansaviolobo CreditAttribution: alansaviolobo commentedComment #58
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedRerolled and keeping the $name variable passed in to
FormState::setErrorByName()
.Comment #68
pameeela CreditAttribution: pameeela commentedThis was committed (live at DrupalCon Sydney!) in #40 and then re-opened for further changes that were never made.
I can't really figure out what exactly should have been done, but it's been 8 years, so I think we are best off setting this back to Fixed and opening a new issue if there is anything that needs to be addressed.