Posted by Lukas von Blarer on November 4, 2012 at 6:17pm
15 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | translation_entity.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | D8MI, language-content, Usability |
Issue Summary
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
Comments
#1
#2
Lukas, 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).
#3
I did a:
grep -R "language is always one of" *
And found
/*** Checks if translation can be enabled.
*
* If language is set to one of the special languages
* and language selector is not hidden, translation cannot be enabled.
*/
function translation_node_type_language_translation_enabled_validate($element, &$form_state, $form) {
if (language_is_locked($form_state['values']['language_configuration']['langcode']) && $form_state['values']['language_configuration']['language_hidden'] && $form_state['values']['node_type_language_translation_enabled']) {
foreach (language_list(LANGUAGE_LOCKED) as $language) {
$locked_languages[] = $language->name;
}
form_set_error('node_type_language_translation_enabled', t('Translation is not supported if language is always one of: @locked_languages', array('@locked_languages' => implode(", ", $locked_languages))));
}
}
More next steps:
get a screen shot
write steps to reproduce (steps to arrive at the screen)
#4
More 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
#5
#6
I 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
#7
We 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".
#8
I created a patch with the aforementioned text message that replaces the current error message.
#9
The last submitted patch, 1831636-selection-errormessage-corrected.patch, failed testing.
#10
#8: 1831636-selection-errormessage-corrected.patch queued for re-testing.
#11
This should be fixed in the Entity Translation module, the Content Translation module is deprecated and will be removed.
#12
#13
I created a patch where i changed the error message in the Entity Translation module.
#14
Steps to reproduce (based of of @moe4715 in #6. Thanks!):
Message with the patch:
Translation is not supported if the default language is Not specified, Not applicable, Multiple and the "Hide language selection" option is checkedHere it is in context
Here is what it said before the patch
Translation is not supported if language is always one of: Not specified, Not applicable, MultipleReview
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.
#15
After 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:
Translation is not supported if the default language is one of: Not specified, Not applicable, Multiple and the "Hide language selection" option is checked.#16
Looks like a great improvement!
#17
We (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. :)
#18
%choice is a system language. Translation is only compatible with real languages, when "Hide language selection" option is checked.
Either pick a real language or do not hide the language selection.
This 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"?
#19
With the "Hide language selection" setting, translation is only compatible with specific languages.
Either pick a specific language or do not hide the language selection.
Might still be too long.
Translation is not compatible with content with %choice.
Either pick a specific language or do not hide the language selection.
#20
Wouldn'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.#21
#22
#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.
#23
"Hide language selection" is not compatible with translating content that has language: %choice.
Either do not hide the language selection or pick a specific language.
This 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? :)
#24
Why do you write the message in two lines? Should it be presented like that in one message?
#25
I 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.
#26
Yes, 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?
#27
@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. :)
#28
Updated patch based on #23.
Interdiff with #15.
#29
Thanks! I think except the two line breaks, this looks good. We don't do multi-component error messages like this elsewhere.
#30
Updated patch in #28 with line-breaks removed.
#31
Thanks 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.
#32
Hi, 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.
#33
New patch with fixes suggested by @jlscott in #31.
#34
The last submitted patch, drush-selection_errormsg_translation_entity-1831636-33.patch, failed testing.
#35
This one should pass.
#36
#37
The last submitted patch, drush-selection_errormsg_translation_entity-1831636-35.patch, failed testing.
#38
#35: drush-selection_errormsg_translation_entity-1831636-35.patch queued for re-testing.
#39
+++ b/core/modules/translation_entity/translation_entity.moduleundefined@@ -842,12 +842,12 @@ function translation_entity_language_configuration_element_validate($element, ar
- $locked_languages[] = $language->name;
+ $locked_languages[$language->langcode] = $language->name;
It 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.
#40
Live from the code sprint at DrupalCon Sydney! Committed and pushed to 8.x. :D YAY!
#41
Uhm, 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.
#42
here is what core looks like right now:
#43
Right, so we need the hunk that @xjm identified as accidental too, or we don't get the language name either :) Recategorizing as bug.
#44
before the commit, with no patch, it looked like:
#45
since #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]
#46
I 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.
#47
cross-posted, restoring category