Review hook_help for the language module

admin/config/regional/settings seems to be in core/modules/system/system.routing.yml.

Here's a catalog of what the Language module actually does/provides (from a UI perspective):
- Managing languages (add/delete/edit individual languages and an Overview page at admin/config/regional/language -- route language.admin_overview)
- Language detection/selection settings (admin/config/regional/language/detection -- route language.negotiation -- and quite a few sub-pages for configuring individual selection/detection methods)
- Content language settings page (admin/config/regional/content-language -- route language.content_settings_page)
- Language switcher block

So, we should make sure all of these are covered in this help text, and nothing else.

Some other thoughts on this latest patch:

a)

The Language module allows you to maintain a list of languages used on your site...

I think we should not say that you are "maintaining a list of languages". That is kind of weak -- and why would anyone care about maintaining a list? What it really allows you to do is configure the languages used on your site.

b) Make sure the pages referred to in the help are the same as the actual page titles. Most have "Sentence case" capitalization, I think, not "Title Case". For instance, I saw "... Languages List page..." in the help, but the page is actually just called "Languages". And then later in that paragraph, you refer to the language being "shown in the Languages List" -- I don't think that needs capitalization at all.

c)

... language code, name and direction ...

Needs comma after "name".

d)

... and a tick box to display language selectors. ...

Tick box is not the right term. I think it is a set of checkboxes?

e)

... is displayed on the content creation pages. ...

No "the" needed here.

f)

... for any supported content element of your site (for example for content types or user profiles). 

This should be "entity" not "content element", and I don't think we refer to anything as "user profiles" any more?

g) Be careful about URLs that go into other modules. For instance, the Block module is no longer required, so if you want to include a Block module URL, you have to check whether it is installed (and also you should say in the text something like "... if the Block module is installed..."). There are examples of this in the Menu module help.

h) I don't understand why the help text recommends not changing the default site language.

i)

... provides several methods  by which the site detects in which language to display interface text ...

This is a bit awkward... In English, it is actually preferable to say "which language to display interface text in" rather than putting the "in" first. And the "by which" part would be better as "provides several methods for detecting..."

j) There are some extra spaces here and there, like:

...specified for each language.	'

Thanks!

Looking better!

A few notes:

1. Uses #1

If the <a href="!interface">Interface Translation module</a> is installed, then interface translation for this language is automatically downloaded as well.

The truth of this statement depends on how you have the Interface Translation module configured, at least according to your latest locale help patch (I think you can turn this on/off, right?). So... not sure what to say here...

2. Uses - Content language

...customize the language settings for any supported content entity of your site...

of ==> on

3. Same Uses bullet

Customizing content languages settings

We have a UI text guideline that says to use the word "configure" rather than "setting". So this header should probably be changed?
https://drupal.org/node/604342

4. Language switcher block uses

array('!blocks' => \Drupal::url('block.admin_display')

This is not going to work. If the Block module is not installed it will cause an error. See my earlier comment -- need to do what is done in the Menu module help code to check if Block is installed.

5. Please get rid of "by which" throughout.

6. Extra spaces: " specifing de for German would result..."

7.

+      $output .= '<li>' . t('<em>Account administration pages</em> sets the interface language based on the language that is set for the specific user on their user page.') . '</li>';

Is this right? It seems like this is "user" setting documentation?

Thanks, looks good! A few typos and minor fixes:

a) in 'Setting administration languages'

method is enable on the

enable => enabled

b) This "administration languages" Uses item seems a bit out of place. It is currently before the "Selection and Detection" stuff is even described, and before any other mention that people can even set a language for themselves. I like the wording, but... maybe it should be moved a bit? Not sure where... Or maybe it just needs a little more explanation?

c) Detection and Selection section:

...page provides several methods to detect in which language interface text is displayed.

===>
page provides several methods for deciding which language should be used for displaying interface text.
(does that sound better? It does to me)

d) Same section - You might want to mention that you choose methods and order them, and the first method that provides an answer is selected?

e) Session item in this section:

...determines the interface language for a request or session parameter.

for => from?

f) same item

...use of <em>de</em> within the <em>language</em> parameter.

within => as?

g) Administration item within that section:

and the interface text language then follows that configuration.

This area doesn't explicitly say that it only applies to administration pages?

Thanks! But I think this still needs a bit of work, sorry...

a)

If the Blocks module is enabled

The module is called "Block", not "Blocks" now.

b) I'm still having trouble with this part:

+      $output .= '<dt>' . t('Setting administration languages') . '</dt>';
+      $output .= '<dd>' . t('If the <em>Account administration pages</em> method is enabled on the <a href="!detection">Detection and selection</a> page, then each administrator can set a second language on their profile page to use for site administration.', array('!detection' => \Drupal::url('language.negotiation'))) . '</dd>';

So, this appears before the section on Detection and Selection, and it refers to a "second language", when as far as I can see, the "first language" hasn't been explained.

Maybe we should replace this with "Setting user languages", and explain that users can select maybe a prefered language and maybe an administration language (both of those settings can apparently be turned off?), and that both of them can be used for detection and selection optionally (see below)? [the "see below" would at least clue people in that the explanation of detection and selection is coming up]

c)

for German  would result 

(in URL under Detection and selection) --> Empty space before "would" here.

d) In Session detection item:

based on the use of <em>de</em> as <em>language</em> parameter

needs "the" after "as" here.

e) User detection item:

follows the user\'s language configuration set on the user\'s profile page.

How about omitting the first "user's"?

f) Administration detection item:

then administration users  can add a second language on their profile page. 

administration => administrative
add => configure
second => ... Again, I don't think it makes sense to say "second" here. What does it say in the UI? Use that text?

g) Selected detection item:

or a specific language as fall-back language

Needs 'the' after 'as'

Um. The patch in #19 lost most of the patch from #17 (most of the help changes are missing)? And the 13-17 interdiff is for some other patch I think?

I'll wait to review until we have the patches/interdiffs you intended here.

OK, that was thorough of you. :)

So I looked through the patch in #21. It looks great!

Do we want to edit the text in the case: section of the hook_help now?
- We shouldn't be using Drupal in there ( case 'admin/config/regional/language/detection/browser')
- Need "the" at the start of this sentence: "Default language can be changed at the Regional settings page." (case 'admin/config/regional/language/detection')
- Last case: "(eg." should be "(for example, " or else "(e.g., "

Didn't see any other problems...

Anyway, this is probably ready for a manual test - updated the issue summary.

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: I am pretty sure ifrik will not mind. She's been working on a lot of patches for these issues in collaboration with a lot of people. Please do!

Yeah, don't worry about an interdiff... if you are rerolling a patch because some underlying code has changed, you cannot really make an interdiff that is useful.

So... This change at the top of your new patch is not good:

+++ b/core/modules/language/language.module
@@ -1,4 +1,4 @@
-<?php
+  <?php

Can you take those two spaces back out please?

You also apparently added an extra lines that is not needed, here:

 
+
     case 'language.admin_overview':

Other than those two spots, the reroll looks OK. Thanks!

Actually, I think the extra spaces before the <?php at the beginning of the file could cause errors. Those spaces would be output at the top of any page that includes the module file, and that is consistent with the test failures on your previous patch. So I think the reroll is correct now. Thanks!

Anyway... Looking at this new patch, I see that between when patch #21 was submitted and now, someone else changed the Language help in this section:

      case 'language.admin_overview':

Patch #31 is removing this line:

-      return '<p>' . t('Reorder the added languages to set their order in the language switcher block and, when editing content, in the list of selectable languages. This ordering does not impact <a href="@detection">detection and selection</a>.', array('@detection' => url('admin/config/regional/language/detection'))) . '</p>';

Patch #21 was removing this line:

-      return '<p>' . t('With multiple languages enabled, registered users may select their preferred language and authors can assign a specific language to content. The selection of what language is used to display page elements is made depending on the detection menthod settings in the <a href="@detection">Detection and Selection</a> tab.', array('@detection' => url('admin/config/regional/language/detection'))) . '</p>';

And both patches are replacing the removed line with this line:

+      return '<p>' . t('With multiple languages enabled, registered users may select their preferred language and authors can assign a specific language to content. The selection of what language is used to display page elements is made depending on the detection menthod settings in the <a href="!detection">Detection and Selection</a> tab.', array('!detection' => \Drupal::url('language.negotiation'))) . '</p>';
 

I think maybe that we should keep the current help that is in Drupal Core for this page, and take this change out of the patch entirely, because it seems like more correct information.

And here were some other comments I made in #22, for the specific-page section of the hook_help:

- We shouldn't be using Drupal in there ( case 'admin/config/regional/language/detection/browser')

- Need "the" at the start of this sentence: "Default language can be changed at the Regional settings page." (case 'admin/config/regional/language/detection')

- Last case: "(eg." should be "(for example, " or else "(e.g., "

Let's see. The text says ... "(primarily text provided by Drupal and modules,..." -- here it should probably just say "primarily text provided by modules". I think that is what you meant?

I think this patch is looking really really good!

I read through the whole thing, and I think it could use just this one small change. In the Uses section about detection and selection:

If the language detection is done by domain name, a URL needs to be specified for each language.

I think it should say "... a domain needs to be specified..." here, not a URL?

Also, this patch still needs a manual test. See issue summary for instructions.

Thanks for the updates and testing! We are in "critical and major commits only" period until after Wednesday when the next Alpha is released. So, this can get committed on Thursday or later.

Agreed with Gabor, let's not wait on the decision on that other issue about "Drupal translation server".

The comments in #52 seem to be a separate issue about whether the translation UI works, not about this patch. Please file a separate issue.

So I think we are back to RTBC here, as this patch has been tested and verified.

Thanks!

Thanks again everyone! Committed to 8.x.