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.
Comment | File | Size | Author |
---|---|---|---|
#54 | interdiff.2161801.51.54.txt | 2.67 KB | YesCT |
#54 | 2161801-config_translation_help-54.patch | 4.54 KB | YesCT |
#51 | interdiff.2161801.47.50.txt | 2.43 KB | YesCT |
#51 | 2161801-config_translation_help-50.patch | 4.31 KB | YesCT |
#47 | 2161801-diff-44-47.txt | 2.3 KB | vijaycs85 |
Comments
Comment #1
jhodgdon"needs work" status means that there is a patch that needs work. "active" means an issue where work has not started yet.
Comment #2
ifrikworking on it during DevDays
Comment #3
ifrikThe d.o page https://drupal.org/documentation/modules/config_translation doesn't exist yet.
Comment #4
batigolixD.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
Comment #5
batigolixgoing to try to make an attempt to see if it is possible to update the current help to the standards
Comment #6
ifrikHere'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.
Comment #7
jhodgdonLooks pretty good! A few typos:
a) 1st Uses bullet:
translate => translated
b) Last Uses bullet:
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!
Comment #8
ifrikThanks, all changed.
Comment #9
jhodgdonLooks 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.
Comment #10
batigolixunassigning
Comment #11
batigolixi 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?
Comment #12
ifrikThanks, 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.
Comment #13
ifrikComment #14
batigolixOk, sounds cool to me.
I reviewed the patch from #12 and all seems Okay
Comment #15
jhodgdonI 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.
Comment #16
jhodgdonAnother minor fix: end of the first Uses point:
just as ==> like
And could we also fix this in the case 'admin/config/regional/config-translation' item:
which ==> that
Thanks!
Comment #17
ifriktaken 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 :)
Comment #18
batigolixi take this
Comment #19
jhodgdonI 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?
Comment #20
batigolixI could pick this up but I need the patch (that was missing from #17). Assigning to Ifrik
Comment #21
jhodgdonI think the patch is there in #17, but the interdiff was missing (the patch was uploaded in its place, twice)?
Comment #22
batigolixah okay. and assigning to someone else is not possible ...
ill give this a shot
Comment #23
ifrikHere'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.
Comment #24
batigolixPatch 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
Comment #25
batigolixoops, cross posting confusion. anyhow: the patch in #24 is review-ready
Comment #26
jhodgdonThis patch looks good to me, except we still need a comma after "for example" in the About. See #19a.
Comment #27
ifrikone comma added :)
Comment #28
jhodgdonGreat, ready for a manual test, issue summary updated accordingly.
Comment #29
jhodgdonComment #30
Gábor HojtsyLooks good, but some things I found:
- 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.
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?
Comment #31
Gábor HojtsyTagging with D8MI topic tags.
Comment #32
ifrik2.
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.
Comment #33
jhodgdonWe 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.
Comment #34
fran seva CreditAttribution: fran seva commentedAttach the Re-rolling patch.
Comment #35
jhodgdonStill needs work. See #31/#32. Reroll is fine, thanks!
Comment #36
fran seva CreditAttribution: fran seva commentedComment #37
fran seva CreditAttribution: fran seva commented@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."
Comment #38
Gábor HojtsyMy 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.
Comment #39
fran seva CreditAttribution: fran seva commented@Gábor Hojtsy thanks!! I'm going to make a proposal text XD
Comment #40
fran seva CreditAttribution: fran seva commentedMy 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
Comment #41
fran seva CreditAttribution: fran seva commentedPatch has a misspelling. I'm on it :(
Comment #42
fran seva CreditAttribution: fran seva commentedFix misspelling.
Attach patch and interdiff.
Comment #43
Gábor HojtsyHow did
become
? 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.
Comment #44
vijaycs85Here is an update that tries to address #43
Comment #45
vijaycs85Comment #46
jhodgdonActually, 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?
Comment #47
vijaycs85Ok, let's remove token specific text.
Comment #48
jhodgdonThis 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...
Comment #49
YesCT CreditAttribution: YesCT commentedcomment #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:
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)
Comment #50
jhodgdonWe'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)" ?
Comment #51
YesCT CreditAttribution: YesCT commentedI 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.
Comment #52
jhodgdonThanks 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!
Comment #53
Gábor HojtsyThis 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.
Comment #54
YesCT CreditAttribution: YesCT commenteddid #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. :)
Comment #55
jhodgdonLooks good to me! I don't think we need a new manual test. I did verify the name of the permission.
Comment #56
webchickCommitted and pushed to 8.x. Thanks!
Comment #58
hass CreditAttribution: hass commentedWhy are we no longer using
@url
for urls?Comment #59
jhodgdonhass: Check out the docs for format_string(). @strings are run through sanitization that is not necessary (or even a good idea) for URLs.