Background:
This issue is part of the task to update/create the hook_help texts of the modules for Drupal 8:
#1908570: [meta] Update or create hook_help() texts for D8 core modules

Tasks:
a) [done, patch available] write the hook_help function

b) review d.o. docs at https://drupal.org/documentation/modules/config_translation

c) (novice) Final manual testing:
1. Apply the patch, install drupal, enable configuration translation. (try it with and without content translation enabled, try it with and without an additional language)
2. Go to admin/help.
3. Click on the help page for this module (Config Translation).
4. Verify that the help page is OK:
- Verify that all the links work
- Verify that all mentions of pages/text/permissions within the UI match what is seen in the UI
- Verify that the formatting is OK.

Files: 
CommentFileSizeAuthor
#54 interdiff.2161801.51.54.txt2.67 KBYesCT
#54 2161801-config_translation_help-54.patch4.54 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,840 pass(es).
[ View ]
#51 interdiff.2161801.47.50.txt2.43 KBYesCT
#51 2161801-config_translation_help-50.patch4.31 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,481 pass(es).
[ View ]
#47 2161801-diff-44-47.txt2.3 KBvijaycs85
#47 2161801-config_translation_help-47.patch4.05 KBvijaycs85
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,020 pass(es).
[ View ]
#44 2161801-diff-42-44.txt4.43 KBvijaycs85
#44 2161801-config_translation_help-44.patch4.1 KBvijaycs85
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,023 pass(es).
[ View ]

Comments

jhodgdon’s picture

Status:Needs work» Active

"needs work" status means that there is a patch that needs work. "active" means an issue where work has not started yet.

ifrik’s picture

working on it during DevDays

ifrik’s picture

batigolix’s picture

D.O. page created

https://drupal.org/documentation/modules/config_translation

The copy is a copy of the current help text. It needs refinement and update later after we finish the help text

batigolix’s picture

Assigned:Unassigned» batigolix

going to try to make an attempt to see if it is possible to update the current help to the standards

ifrik’s picture

StatusFileSize
new3.75 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,828 pass(es).
[ View ]

Here's my initial re-write. I've specifically added the translation of date formats to the Uses, because it's an unexpected hidden gem, that you can actually set different date formats as part of the translation.

jhodgdon’s picture

Status:Active» Needs work

Looks pretty good! A few typos:

a) 1st Uses bullet:

all configuration text that can be translate,...

translate => translated

b) Last Uses bullet:

set a language-specific  <em>PHP date format</em>

Extra space here.

c) There is also an extra blank line after the last Uses bullet. Should only be one.

Other than that, I think this reads very clearly -- good work!

ifrik’s picture

Status:Needs work» Needs review
StatusFileSize
new2.72 KB
new3.74 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,809 pass(es).
[ View ]

Thanks, all changed.

jhodgdon’s picture

Looks good, thanks!

I think this is ready for a manual test:
- Verify that all the links work
- Verify that all mentions of pages/text/permissions within the UI match what is seen in the UI
- Verify that the formatting is OK.

batigolix’s picture

Assigned:batigolix» Unassigned

unassigning

batigolix’s picture

Status:Needs review» Needs work
Parent issue:» #1908570: [meta] Update or create hook_help() texts for D8 core modules

i reviewed

a) about formatting:there is a missing closing after t('Translating date formats')
$output .= '<dt>' . t('Translating date formats');

b) about links: the link to the Content Translation help text is broken if the module is disabled. Do we have a guideline how to deal with such situations?
All other links work properly

c): about references to UI: all OK

d) Does "Translating date formats" deserve its own section? it is configuration text after all

e) should we mention the term "string" somewhere?

f) is it necessary to explain what the other translation modules do? is a link not enough?

ifrik’s picture

StatusFileSize
new3.82 KB
new3.75 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,826 pass(es).
[ View ]

Thanks, I fixed the missing closing and the link to the content translation module.
I've taken the reference to the Configuration Manager out because that's unnecessary and not strictly correct.

I added the the explanation about what is translatable by the Config translation because it's not really obvious what's what - and why you might not find what you look for in this translation module.

About strings: The interface translation has a seach field where the label talks of strings, so that's why I added it there. But in the config translation "strings" are not mentioned at all. So I don't think we need it.

Date format: I didn't expect something like "j. m. Y." to show up as translatable. Possibly because I don't perceive that as text as such, but as a configuration that is different in different languages. Since there's only one other Use, I would prefer to keep it here.

ifrik’s picture

Status:Needs work» Needs review
batigolix’s picture

Status:Needs review» Reviewed & tested by the community

Ok, sounds cool to me.

I reviewed the patch from #12 and all seems Okay

jhodgdon’s picture

Status:Reviewed & tested by the community» Needs work

I was just going to commit this, but I gave it one more final read-through...

In the first sentence, it includes "blocks" as "configuration text". I don't really think this is a great example, because Custom Blocks are Content, not Configuration. So... Can we just take this out of the list of examples?

Also... Are taxonomy vocabularies config? Terms are Content I think... not sure about vocabularies.

jhodgdon’s picture

Another minor fix: end of the first Uses point:

The translated text can contain tokens, just as the original text.

just as ==> like

And could we also fix this in the case 'admin/config/regional/config-translation' item:

This page lists all configuration items on your site which have translatable text

which ==> that

Thanks!

ifrik’s picture

Status:Needs work» Needs review
StatusFileSize
new4.14 KB
new4.14 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,870 pass(es).
[ View ]

taken up comments.

Vocabularies are listed on the configuration translation page, so they are config, while the terms are content.
The fact that that all looks a bit confusing was the reason why I added them in the first place :)

batigolix’s picture

Assigned:Unassigned» batigolix

i take this

jhodgdon’s picture

Status:Needs review» Needs work

I think you uploaded the patch in place of the interdiff in #17, but that's OK.

So ... I guess I didn't notice this before, but there are a couple of proofreading issues in the About section:

a) Punctuation: should be:
... allows you to translate configuration text; for example, the site name, ...

b) Also, in the part about the other modules, it should say "... and Interface Translation modules, which ..." ("comma which" in place of "that").

c) Also, is locale.module (interface translation) required for Config Translation? If not, we should probably check to see if it is enabled when making that link?

batigolix’s picture

I could pick this up but I need the patch (that was missing from #17). Assigning to Ifrik

jhodgdon’s picture

I think the patch is there in #17, but the interdiff was missing (the patch was uploaded in its place, twice)?

batigolix’s picture

ah okay. and assigning to someone else is not possible ...

ill give this a shot

ifrik’s picture

StatusFileSize
new4.87 KB

Here's the missing interdiff between #12 and 17.
Looks like I forgot to apply the patch myself after I had blasted my D8 install for a fresh start.

batigolix’s picture

Assigned:batigolix» Unassigned
StatusFileSize
new4.1 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,985 pass(es).
[ View ]
new2.45 KB

Patch addresses 19# and introduced a common introduction (for all language modules) as proposed in #2091479: Update hook_help for content_translation module comment #30

a) done
b) now replaced by common intro
c) config translation depends on locale so the check is not necessary

batigolix’s picture

Status:Needs work» Needs review

oops, cross posting confusion. anyhow: the patch in #24 is review-ready

jhodgdon’s picture

Status:Needs review» Needs work

This patch looks good to me, except we still need a comma after "for example" in the About. See #19a.

ifrik’s picture

Status:Needs work» Needs review
StatusFileSize
new2.49 KB
new4.1 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,015 pass(es).
[ View ]

one comma added :)

jhodgdon’s picture

Issue summary:View changes

Great, ready for a manual test, issue summary updated accordingly.

jhodgdon’s picture

Issue summary:View changes
Gábor Hojtsy’s picture

Status:Needs review» Needs work

Looks good, but some things I found:

  1. +++ b/core/modules/config_translation/config_translation.module
    @@ -17,16 +17,18 @@ function config_translation_help($path) {
    +      $output .= '<dd>' . t('The <a href="!translation-page">Configuration translation</a> page shows a list of all configuration text that can be translated, either as indidvidual items or as lists. After you click on <em>Translate</em>, you are provided with a list of all languages. You can <em>add</em> a translation for a specific language, but also <em>edit</em> the text in the site\'s default language. For some configuration text items (for example for the site information), the specific translation page can also be accessed directly from their configuration pages. Like the original text, the translated text can contain tokens.', array('!translation-page' => \Drupal::url('config_translation.mapper_list'))) . '</dd>';

    - The original can only be edited with proper permission, that would be good to add. That is not really provided by the config translation module, we just link the relevant page in.

    - Most configuration items have a local translate tab, not only some :)

    - It is not very likely that tokens appear in configuration text, although there may be in views text, date formats, etc. But much less likely compared to interface text from locale module. So maybe the reference to that is a bit too strong here.

  2. +++ b/core/modules/config_translation/config_translation.module
    @@ -17,16 +17,18 @@ function config_translation_help($path) {
    +      $output .= '<dt>' . t('Translating date formats') . '</dt>';
    +      $output .= '<dd>' . t('You can choose to translate date formats on the <a href="!translation-page">Configuration translation</a> page. This allows you not only to translate the label text, but also to set a language-specific <em>PHP date format</em>.', array('!translation-page' => \Drupal::url('config_translation.mapper_list'))) . '</dd>';

    Why did you highlight date formats specifically? From the point of view of the module these are the same as input formats or node types or contact categories, not special. Is that due to the PHP format for dates that is somewhat unique?

Gábor Hojtsy’s picture

Issue tags:+D8MI, +language-config

Tagging with D8MI topic tags.

ifrik’s picture

2.

Why did you highlight date formats specifically? From the point of view of the module these are the same as input formats or node types or contact categories, not special. Is that due to the PHP format for dates that is somewhat unique?

From a site builders point of view a bit of PHP format appears rather different then a bit of text, and it's not really intuative that you could use different date format by translating the format instead of configuring several different ones on the "Date & Time" configuration page.
And since there aren't any other uses anyway, I would like to keep it to point users to something that isn't obvious.

1. I'll take that up.

jhodgdon’s picture

We just had a change to hook_help, on this issue: #2183113: Update hook_help signature to use route_name instead of path.

Here is the change record: https://drupal.org/node/2250345

This patch will need a reroll for this change.

fran seva’s picture

Status:Needs work» Needs review
StatusFileSize
new4.11 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,576 pass(es).
[ View ]

Attach the Re-rolling patch.

jhodgdon’s picture

Status:Needs review» Needs work

Still needs work. See #31/#32. Reroll is fine, thanks!

fran seva’s picture

Assigned:Unassigned» fran seva
Issue tags:+DrupalCampSpain
fran seva’s picture

@jhodgdon @Gábor Hojtsy I have some doubts related with #30 , specially with point 1 and the proposal about the uses of token.
I think the text may be like this: "Like the original text, which can only be edited with proper permission, the translated text can contain tokens."
But at this point I'm not sure if we should not use token and replace the sentence for something like: "The translated text can be translated as the original text. The original can only be edited with proper permission." or "The translated text can be translated as the original text and this cant only be edited with proper permission."

Gábor Hojtsy’s picture

My concern was with the tokens. It is much more likely that the text will NOT contain tokens that it will. It MAY contain tokens rarely IMHO.

fran seva’s picture

@Gábor Hojtsy thanks!! I'm going to make a proposal text XD

fran seva’s picture

Status:Needs work» Needs review
StatusFileSize
new4.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,661 pass(es).
[ View ]
new2.44 KB

My proposal for Translating Configuration Text section is:

The Configuration translation page shows a list of all configuration text that can be translated, either as indidvidual items or as lists. After you click on Translate, you are provided with a list of all languages. You can add a translation for a specific language, but also edit the text in the site's default language. For some configuration text items (for example for the site information), the specific translation page can also be accessed directly from their configuration pages. The translated text can be translated as the original text and may contain tokens but can only be edited with proper permission.

Attach the patch and the interdiff

fran seva’s picture

Patch has a misspelling. I'm on it :(

fran seva’s picture

StatusFileSize
new4.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,689 pass(es).
[ View ]
new2.3 KB

Fix misspelling.
Attach patch and interdiff.

Gábor Hojtsy’s picture

Status:Needs review» Needs work

How did

Like the original text, the translated text can contain tokens.

become

The translated text can be translated as the original text and may contain tokens but can only be edited with proper permission.

? The first part of that sentence does not make sense, as I said above tokens are highly unlikely and those who can access this page can edit the translation, so not sure why to mention proper permissions. If you don't have such permissions, then access to this page is not possible.

vijaycs85’s picture

StatusFileSize
new4.1 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,023 pass(es).
[ View ]
new4.43 KB

Here is an update that tries to address #43

vijaycs85’s picture

Status:Needs work» Needs review
jhodgdon’s picture

Actually, I am not sure why we should even mention "The origin and translated text may contain tokens." ? I don't see any discussion above about why this was added, and I don't really think it is all that relevant to the help here. As Gabor said, most of the time it doesn't contain tokens, an even if it could... why should the Config Translation module help mention this fact?

Other than that, this looks fine to me. Gabor, any thoughts on the tokens text?

vijaycs85’s picture

StatusFileSize
new4.05 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,020 pass(es).
[ View ]
new2.3 KB

Ok, let's remove token specific text.

jhodgdon’s picture

This looks good to me! I think we should run it through a manual test again:
- Verify that all the links work
- Verify that all mentions of pages/text/permissions within the UI match what is seen in the UI
- Verify that the formatting is OK.

And Gábor may also have a review comment...

YesCT’s picture

Issue summary:View changes
+++ b/core/modules/config_translation/config_translation.module
@@ -18,16 +18,18 @@ function config_translation_help($route_name, Request $request) {
-      $output .= '<p>' . t('The Configuration Translation module allows configurations to be translated into different languages. Views, your site name, contact module categories, vocabularies, menus, blocks, and so on are all stored within the unified configuration system and can be translated with this module. Content, such as nodes, taxonomy terms, custom blocks, and so on are translatable with the Content Translation module in Drupal core, while the built-in user interface (such as registration forms, content submission and administration interfaces) are translated with the Interface Translation module. Use these three modules effectively together to translate your whole site to different languages.') . '</p>';
+      $output .= '<p>' . t('The Configuration Translation module allows you to translate configuration text; for example, the site name, vocabularies, menus, or date formats. Together with the modules <a href="!language">Language</a>, <a href="!content-translation">Content Translation</a>, and <a href="!locale">Interface Translation</a>, it allows you to build multilingual websites. For more information, see the <a href="!doc_url">online documentation for the Configuration Translation module</a>.', array('!doc_url' => 'https://drupal.org/documentation/modules/config_translation', '!config' => \Drupal::url('help.page', array('name' => 'config')), '!language' => \Drupal::url('help.page', array('name' => 'language')), '!locale' => \Drupal::url('help.page', array('name' => 'locale')), '!content-translation' => (\Drupal::moduleHandler()->moduleExists('content_translation')) ? \Drupal::url('help.page', array('name' => 'content_translation')) : '#')) . '</p>';

comment #12 added in the module exists check for content translation.

But this is confusing IMO because when the module is not installed (enabled) the link goes back to this config help page. Maybe it shouldn't be a link at all? How do other hook helps? deal with this?

[edit]
content translation uses moduleexists in the same way:

$output .= '<p>' . t('The Content Translation module allows you to translate content, comments, custom blocks, taxonomy terms, users and other <a href="!entity_help">content entities</a>. Together with the modules <a href="!language">Language</a>, <a href="!config-trans">Configuration Translation</a>, and <a href="!locale">Interface Translation</a>, it allows you to build multilingual websites. For more information, see <a href="!translation-entity">the online documentation for the Content Translation module</a>.', array('!locale' => (\Drupal::moduleHandler()->moduleExists('locale')) ? \Drupal::url('help.page', array('name' => 'locale')) : '#', '!config-trans' => (\Drupal::moduleHandler()->moduleExists('config_translation')) ? \Drupal::url('help.page', array('name' => 'config_translation')) : '#', '!language' => \Drupal::url('help.page', array('name' => 'language')), '!translation-entity' => 'https://drupal.org/documentation/modules/translation', '!entity_help' => \Drupal::url('help.page', array('name' => 'entity')))) . '</p>';

to link to config translation.

So if we want to do anything fancy for that, lets do something in a separate issue for both.

[edit: added issue] #2286405: Deal with links to uninstalled module help by adding if installed to hook_helps (that use if moduleExists)

jhodgdon’s picture

We're using the same pattern (link either going to other module help or to # if the module isn't enabled) in a LOT of the hook_help() now. The problem is that we want the entire help text to be translated as one string, so all we can easily do is use t() substitution to put in the URL... Well I guess we could substitute in the start/end A tag as well, but it gets a bit messy.

I guess in most of the other help text in this situation, we've had something in the paragraph saying something like "... if the XYZ module is enabled..." which would give you a clue about why the link doesn't go anywhere. So maybe we should consider for both of these language-related modules, putting in "(if enabled)" ?

YesCT’s picture

Issue summary:View changes
Issue tags:-Needs manual testing, -Novice+Needs followup
StatusFileSize
new4.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,481 pass(es).
[ View ]
new2.43 KB

I reviewed this.
I read the whole patch, and
applied it and tried it on an installed site.
I did all the actions it describes to make sure that the help text described the actual way things work.

--------

1)
Without another language installed, the translate links do not have actions to add a translation. Lets do something similar to content translation and tell people they have to add another language.

I did that. (staying at needs review to check that).

2.
"You can add a translation for a specific language, but also edit the text in the site's default language."
technically, after adding, we can also edit a translation for a specific language. I think this will be obvious, but if others think it needs to be spelled out... I suggest:
"You can add or edit a translation for a specific language, but also edit the text in the site's default language."
or
"You can add or edit a translation for a specific language. You can also edit the text for the site's default language." [rewording this might be part of 5c?]
I'm totally fine with not addressing this point. But let's get a +1 from someone else on not dealing with it.

3.
The improved date stuff is awesome and different than in d7. Is that why it's separated out here? Ah, as noted in #6. I'm cool with that.

4.
There is no mention of permissions in this help.
However
https://drupal.org/node/632280 "Help text standard (for core and contrib)" mentions permissions.
Strangely, #2091479-22: Update hook_help for content_translation module took out mentions of permissions, and that is the example in the help text guidelines.
Maybe the guidelines need to be updated to say that the idea of permissions ... is engrained in Drupal and doesn't need to be there?
blocks still mention permissions https://api.drupal.org/api/drupal/core%21modules%21block_content%21block...
https://api.drupal.org/api/drupal/core%21modules%21book%21book.module/fu...

So, here, do we think permissions are tricky enough to need an explanation?

Updating the standards/guidelines is a separate issue...

====
5.
more *separate* issues,
a) when the list of things to translate is empty... maybe we should not show the list button? [edit: added issue] #2286359: Dont have a list button, if there is no config items to list (for configuration translation)
b) When there is only one language, the translate page ... doesn't tell people to add a language. Maybe we should. Check what content does in that case. [edit: added issue.] #2286367: Give a hint on translation pages, that to translate another language should be added
c) there is no "edit" button on the english when there is no second language. why? (for performance I suspect...? but the language module is enabled so maybe not.) [Fixing this might mean we shouldn't do b) or we word b) carefully.] [edit: added issue] #2286375: Allow translating english config, even if there is no second language
d) the interaction on the date format translation kinda sucks. After updating the example date under for format (which looks like an autocomplete cause of the circle wait thing), I have to click back into the text box for *each character* I want to type, and wait for each character to be updated in the sample. [edit: added issue] #2286385: Improve translating date format, user interaction
e) from 4. update https://drupal.org/node/632280 "Help text standard (for core and contrib)" as to if permissions need to be mentioned (and update the example to use an example that actually references permissions.) [edit: added issue] #2286351: Update help text standard regarding permission

Adding needs follow-up tag, not because they are caused by this issue, but just because I dont want to lose these, and I can't open them right away.

jhodgdon’s picture

Status:Needs review» Needs work
Issue tags:-Needs followup

Thanks a lot! Great review in #51.

To your specific points:

#1 - change looks good to me. Consistency is always good. :)

#2 - I like this rewording: "You can add or edit a translation for a specific language. You can also edit the text for the site's default language." Seems less awkward and more accurate than what is there now.

#4 - We have a lot of help text that does mention specific permissions, and probably some that probably doesn't... Some of the help text just says "with sufficient permission", and some is explicit about "a user with Administer content permission can" etc. So... probably adding them here would be good? On content translation, the permissions are quite complex though (at least I think so), because presumably you would need both translation and the specific entity add/edit permission in order to do the actions? So that might be why they were taken out... I really am not sure that was a conscious decision though. The help for that module was totally overhauled, and the permissions may have been an inadvertent casualty. If you think we need them there, let's open a new issue to add them back in.

#5 - Thanks for the new issues! I think this is not "needs followup" any more because you seem to have opened all the necessary issues?

So... back to needs work for your #2/#4 points. Thanks!

Gábor Hojtsy’s picture

This issue is for config translation, but since you mentioned content translation, the permissions are explained in https://drupal.org/node/1903128, https://drupal.org/node/1903124 and https://drupal.org/node/1776752. Config translation works with a very simple permission structure, you either have permission to translate ALL the config, or you don't. *Editing* of the original config OTOH is bound to all kinds of permissions not in control of config translation.

YesCT’s picture

Assigned:fran seva» Unassigned
Status:Needs work» Needs review
StatusFileSize
new4.54 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,840 pass(es).
[ View ]
new2.67 KB

did #2 and #4.
also changes a singular to plural to make it match the rest of a phrase.

git diff --color-words
is your friend. :)

jhodgdon’s picture

Status:Needs review» Reviewed & tested by the community

Looks good to me! I don't think we need a new manual test. I did verify the name of the permission.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

  • webchick committed 3d36fe5 on 8.x
    Issue #2161801 by YesCT, vijaycs85, fran seva, batigolix, ifrik: Update...
hass’s picture

Why are we no longer using @url for urls?

jhodgdon’s picture

hass: Check out the docs for format_string(). @strings are run through sanitization that is not necessary (or even a good idea) for URLs.

Status:Fixed» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.