It is clear from #1096340: Stale language types/negotation info after enabling/disabling modules that the language negotiation API is not properly documented. We have a good reference in http://drupal.org/update/modules/6/7#language_negotiation, but it is farily hidden. We can use it as a foundation to write an initial doc block in language.inc.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

/me loves this issue

plach’s picture

Component: language system » documentation
jhodgdon’s picture

Where exactly are you thinking this documentation should go? Should it be a Topic/Group, or just a @file header in language.inc? If it's a Topic, which functions should be included in it (so they would have a link back to the topic)?

plach’s picture

I don't know which the current guidelines are: almost any function belonging to the language negotiation API is in language.inc, but core implementations are in locale.module and locale.inc, while the UI is in locale.admin.inc, so perhaps we should be using a topic to group them together.

jhodgdon’s picture

Actually, something that long should probably just go into a drupal.org doc page inside
http://drupal.org/developing/api
somewhere.

Then appropriate functions can be linked there, or the @file header for language.inc can link there.

Regarding making a topic/group - normally we don't group UI functions and API functions together in a group... And we already have at least 4 language-related topics on
http://api.drupal.org/api/drupal/groups/7
Does this information (or link) belong in one of them?

Gábor Hojtsy’s picture

@jhodgdon: yes, locale module is one of the most prolific in using groups for functions, which others don't seem to actually use. I think this need to be revisited in Drupal 8. We don't need to have these groups if nobody else uses them, its just inconsistent and confusing in that case. Although I think it would be useful as a practice all across the board. (Locale's overuse of these groups is well demonstrated by the language addition API and the predefined language API (1 function each :D)).

Anyway, http://api.drupal.org/api/drupal/includes--locale.inc/group/locale-langu... is not really complete in terms of the language selection API, so I'm not sure we have an existing group.

chx’s picture

Title: Improve language negotiation API documentation » language negotiation API is undocumented
Category: task » bug
Priority: Normal » Major

In my opinion when bootstrap.inc constants are a WTF (what the heck is a language type, anyways) that goes way beyond a task.

Gábor Hojtsy’s picture

Title: language negotiation API is undocumented » Improve language negotiation API documentation
Category: bug » task
Priority: Major » Normal

Based on discussion with @chx, who was very pissed off with the missing documentation, I've created this figure to help him better understand. He still says we need textual explanation which I very much understand. However, this drawing can be freely used in d.o docs or wherever. I think/hope it does help understand the architecture.

plach’s picture

Issue tags: +Needs backport to D7
Gábor Hojtsy’s picture

Title: Improve language negotiation API documentation » Language negotiation is undocumented
Category: task » bug
Priority: Normal » Major

Crosspost. Also, here is the google doc URL, I can give edit access to people who want: https://docs.google.com/drawings/d/174N_BZAWv2urowMZGZsdikpWicCfUfa1d2eX...

sun’s picture

Priority: Major » Normal

Sorry, but demoting from major. Missing documentation should not hold up D8 initiatives.

jhodgdon’s picture

So... Where do we want to put this documentation -- on api.drupal.org somewhere (presumably in a group/topic -- what should it be called, and what functions/constants should it include?), or on drupal.org, or both?

Gábor Hojtsy’s picture

@jhodgdon: I think both would be ideal. I don't think we have a doc section where we explain the D7 language system in detail (or at all). There is extensive docs on the localization API but I don't know if we have any for the general language system.

plach’s picture

I don't think we have a doc section where we explain the D7 language system in detail (or at all)

AFAIK, nothing neither on a.d.o nor on d.o. except for the module conversion page linked in the OP.

jhodgdon’s picture

OK. We should probably file a separate issue in the Documentation project to write some d.o docs for this, which should go under http://drupal.org/developing/api somewhere. That issue should be tagged "developer".

For this issue (the api.drupal.org piece), can someone write something up briefly (in text, no graphics) -- doesn't have to be in perfect English -- we can edit -- and maybe indicate which functions should be included in the group/topic?

Gábor Hojtsy’s picture

@jhodgdon: do you need it in phpdoc form? I think plach's update guide doc is a very good start for that.

jhodgdon’s picture

Oh yeah, I forgot about that:
http://drupal.org/update/modules/6/7#language_negotiation

No, that's fine. And I think it has enough functions linked that I (or someone else) can at least make the start of a patch.

Gábor Hojtsy’s picture

Issue tags: +Documentation, +D8MI

Tagging

jhodgdon’s picture

Issue tags: -Documentation

We don't generally use the "documentation" tag in the Docs team for Core issues. This patch is in the documentation component, which is good enough - shouldn't need a docs tag as well?

Gábor Hojtsy’s picture

@jdohgdon: I've added the tag so we can link to aspects of the D8MI issues. Such as D8MI combined with "locale-split", or "API cleanup" or "documentation". Not all documentation issues will be in the core queue or in the documentation component but could be part of the D8MI initiative. The only way to do cross-project/component listings of issues is to use tags. (It also makes it consistent to slide and dice the issue list, you always filter down or up via tags). Is this interfering with your overviews of issues in other ways counter to your processes?

jhodgdon’s picture

Issue tags: +Documentation

I see, no problem! I was just following the tag guidelines, which say not to have unnecessary tags, and have a list of approved tags (and normally, documentation is not approved for core issues according to that list).

Gábor Hojtsy’s picture

Issue tags: +language-base

Tagging for base language system.

Gábor Hojtsy’s picture

Issue tags: +negotiation

Tagging for language negotiation too.

Anonymous’s picture

Assigned: Unassigned »
Gábor Hojtsy’s picture

Issue tags: +sprint

Tagging for current sprint :)

pixelite’s picture

Status: Active » Needs review

I've updated the documentation (http://drupal.org/node/1497176) to include more information about the most common use case for language negotiation and to include information about ordering the detection methods.

Anonymous’s picture

The developer documentation is here: http://drupal.org/node/1497272

Gábor Hojtsy’s picture

Still needs to be integrated in phpdoc too to some degree.

pixelite’s picture

Status: Needs review » Needs work

I reviewed the developer documentation. Ryan, do you want to integrate into the phpdoc?

Anonymous’s picture

D8 patch to languages.inc to include reference to documentation and to group functions into API docs page.

Anonymous’s picture

Sorry, I mistakenly used an older version of that file to roll the patch. I'll re-roll with just the reference...

Anonymous’s picture

Here we go... starting to get the hang of this. :)

Anonymous’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs work » Needs review
FileSize
1.21 KB

Backport for D7!

jhodgdon’s picture

Version: 7.x-dev » 8.x-dev

Please do not change the version of an issue from 8.x to 7.x until 8.x is done.
Read
http://drupal.org/node/1319154#multiple-versions

Then please upload your 8.x patch again so it can be tested properly. Thanks.

Anonymous’s picture

Thank you for bringing that to my attention. Re-attaching the patch (different comment number but same file to avoid potential problems with the test bot).

Kristen Pol’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me. I applied the patch with no issue and read through the documentation.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

As per our discussion in #1232120: Improve documentation of field multiple language system it seems like we want to have a more extended summary text so people have an offline version of the API docs as well.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
7.25 KB

I brought the full text of the article into the grouping so it will appear on the API pages and within in the file if you are reading it offline. I kept the URLs to the version with graphics. I changed the headers on the code snipplets so as not to mess up your syntax highlighting in your editor (does this make sense?).

Anonymous’s picture

... and I see my console was set to 82 characters *facepalm*.

Anonymous’s picture

Rerolled.

Kristen Pol’s picture

Status: Needs review » Reviewed & tested by the community

Looks okay to me. I applied the patch with no issue and looked through the documentation.

Tor Arne Thune’s picture

Status: Needs work » Reviewed & tested by the community
+++ b/core/includes/language.incundefined
@@ -2,7 +2,13 @@
+ * Language Negotiation API

Missing period.

+++ b/core/includes/language.incundefined
@@ -2,7 +2,13 @@
+ * This file manages the negotiation of languages for users visiting your Drupal
+ * site.  The API is used by the locale module as the primary core
+ * implementation as well as any contrib modules.

Extra space in the beginning of the second sentence. Removing this will also allow "implementation" to move up one line.

+++ b/core/includes/language.incundefined
@@ -11,6 +17,153 @@
+ * @defgroup languages-negotiation Language Negotiation API
+ * functionality

Should (and can) be on one line and must contain a group identifier name in the form of a single-word identifier ([a-zA-Z_][a-zA-Z0-9_]*) so it should probably be languages_negotiation. See Documenting Topics/Groups.

+++ b/core/includes/language.incundefined
@@ -11,6 +17,153 @@
+ * Language Negotiation API

Missing period.

+++ b/core/includes/language.incundefined
@@ -11,6 +17,153 @@
+ * - Language types, which describes the possible types of translatable content
+ * - Language providers, which allows Drupal to detect which language it should
+ *   serve to the user (note that language providers are called language methods
+ *   in Drupal 8)

Missing periods.

+++ b/core/includes/language.incundefined
@@ -11,6 +17,153 @@
+ * Interface language
+ *     It is the page's main language. It is used to present translated user
+ *     interface elements such as titles, labels, help text, and messages.
+ * Content language
+ *     This is used to choose in which language to display content that is
+ *     available in more than one language (see the multilingual capabilities of
+ *     the new Field API for details).
+ * URL language
+ *     This is the language associated to URLs. When generating a URL, this
+ *     value will be used by url() as a default if no explicit preference is
+ *     provided.

Should be indented by only 2 spaces. Also, the description for Interface language should probably start with "This is the", like the other descriptions, but I am no English expert, so leaving up to you :)

+++ b/core/includes/language.incundefined
@@ -11,6 +17,153 @@
+ * hook_language_negotiation_info() and language providers definitions can be

Should it be "language provider definitions"?

+++ b/core/includes/language.incundefined
@@ -431,3 +584,7 @@ function language_fallback_get_candidates($type = LANGUAGE_TYPE_CONTENT) {
+ * @} End of "languages-negotiation"

Should be changed to "languages_negotiation".

Thanks for documenting this. I know I have scratched my head more than once trying to understand language negotiation.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work
jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

Also, when you make lists, do not put a blank line between the line before the list and the list, as was done here:

+ * The language negotiation API is based on two major concepts:
+ *
+ * - Language types, which describes the possible types of translatable content

And list items need to end in ".". See
http://drupal.org/node/1354#lists

Actually, that whole section... You can't just take docs that are formatted with H2 and H3 headers on drupal.org and put them in like this:

+ *
+ * Defaults provided by Drupal
+ *
+ * Language Types
+ *

You need to format them differently, as narratives and paragraphs.

Sections like this:

+ * Interface language
+ *     It is the page's main language. It is used to present translated user
+ *     interface elements such as titles, labels, help text, and messages.
+ * Content language
+ *     This is used to choose in which language to display content that is
+ *     available in more than one language (see the multilingual capabilities of
+ *     the new Field API for details).
+ * URL language
+ *     This is the language associated to URLs. When generating a URL, this
+ *     value will be used by url() as a default if no explicit preference is
+ *     provided.

should be formatted more like:

- Interface language: The page's main language. ...

And the grammar in this whole section is a mess.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
6.93 KB

Thanks for the feedback everyone. I made the requested changes in #42 and #44. By removing the headings the documentation reads a lot better.

Kristen Pol’s picture

Status: Needs review » Needs work

I have reviewed the patch and verified that it has all the requested changes from #42 and #44. In re-reading the text, I found 3 very small grammar-related changes that you might want to include (sorry I didn't catch these last time!):

+++ b/core/includes/language.inc
@@ -11,6 +17,132 @@
+ * - Language types, which describes the possible types of translatable content.

"describes" => "describe"

+++ b/core/includes/language.inc
@@ -11,6 +17,132 @@
+ * - Language providers, which allows Drupal to detect which language it should
+ *   serve to the user (note that language providers are called language methods
+ *   in Drupal 8).

"allows" => "allow"

+++ b/core/includes/language.inc
@@ -11,6 +17,132 @@
+ * - URL language: This is the language associated to URLs. When generating a
+ *   URL, this value will be used by url() as a default if no explicit
+ *   preference is provided.

"associated to URLs" => "associated with URLs"

These could be fixed in the d.o documentation as well.

jhodgdon’s picture

A couple of other things also need to be fixed:

a)

+++ b/core/includes/language.inc
@@ -2,7 +2,13 @@
 
 /**
  * @file
- * Multiple language handling functionality.
+ * Language Negotiation API.
+ *
+ * This file manages the negotiation of languages for users visiting your Drupal
+ * site. The API is used by the locale module as the primary core implementation
+ * as well as any contrib modules.
+ *
+ * @see http://drupal.org/node/1497272
  */

- I don't think that the description paragraph "This file ..." actually gives us much useful information in its present form. We don't need to tell people about "your Drupal site", and we don't need to tell readers where a Drupal API is used. And it doesn't tell me what "negotiation of languages" means, so it doesn't really have any information that is not already in the title line "Language Negotiation API". Let's just leave that paragraph out.
- Language Negotiation -> Language negotiation [that also applies to the @defgroup that follows]

b)

+ * @defgroup languages_negotiation

Can we call this language_negotiation please?

c)

 /**
+ * @defgroup languages_negotiation Language Negotiation API functionality
+ * @{
+ * Language Negotiation API.

- That line "Language Negotiation API" should be a sentence. See http://drupal.org/node/1354#groups

d)

+ * It is possible to customize the language negotiation process both for how we
+ * detect the user's language and also on what type of data they are requesting.

This sentence uses "for" and "on" in two parallel clauses. Use the same preposition for both.

e)

+ * - Language providers, which allows Drupal to detect which language it should
+ *   serve to the user (note that language providers are called language methods
+ *   in Drupal 8).

- Don't refer to a particular Drupal version. Just document the version that this documentation is in. Same goes for this text below:

+ * Starting in Drupal 7, the language API allows contributed modules to define

And it also applies to some other text about Drupal 6/7 below.
- Why call it "language providers" at all in this doc if it is called "language methods"? This doc is for Drupal 8 and is going into the API code.

f)

+ *
+ * Different language types often share the same values, but the they can have
+ * independent values if needed.

Remove the blank line here. This should be part of the previous paragraph/list.

g)

+ * A language type may be configurable or fixed. A configurable language type
+ * appears in the Configuration > Regional and language > Languages > Detection
+ * and selection page, where the language providers for that language type can
+ * be configured. There are also fixed language types that have predetermined
+ * (module-defined) negotiation settings and, thus, do not appear in the
+ * configuration page. Here is a code snippet that makes the content language
+ * (which by default inherits the interface language's values) configurable:

I don't think referring to specific path for admin is appropriate in API docs. Just say it can be configured by the user.

h)

+ * (which by default inherits the interface language's values) configurable:
+ *
+ * @code
+ * <?php

Get rid of the blank line here, and don't put in < ?php inside @code. Same applies to the other @code blocks.

i)

+ * function language_negotiation_language_types_info_alter(&$language_types) {

This example should probably use mymodule_ not language_negotiation_ in the sample function name.

j)

+ * Every language type can have different language negotiation settings, i.e.
+ * every language type can have a different set of language detection methods,
+ * or providers, assigned to it.

- Punctuation: "... settings; i.e., every language..."

plach’s picture

+++ b/core/includes/language.inc
@@ -11,6 +17,132 @@
+ *   $language_types[LANGUAGE_TYPE_CONTENT] = array(
+ *     'name' => t('Content'),
+ *     'description' => t('If a piece of content is available in multiple
+ * languages, the one matching the <em>content</em> language will be used.'),
+ *   );

ATM the correct way to do this is:

unset($language_types[LANGUAGE_TYPE_CONTENT]['fixed']);

I already fixed the online docs.

Also the figure ebedded in the online docs and in #8 has an error: the balloon saying that content and URL language inherit form UI language is incorrect, actually the URL language always uses the URL language provider and only if there is no language information in the URL it falls back to the current UI language.

-1 days to next Drupal core point release.

Gábor Hojtsy’s picture

@Ryan Weal: are you still working on this?

Anonymous’s picture

Hey everyone, sorry for dropping off on this. I'm finally back home from #drupalcon after three weeks of travel. I've made the requested changes in #46, #47 and #48. Thanks all for your amazing attention to detail.

Anonymous’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Assigned: » jhodgdon
Status: Needs review » Reviewed & tested by the community

Looks good to me :)

webchick’s picture

While this is eligible for D7 backport, it has jhodgdon's fingerprints all over it so I am planning to leave for her.

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

I'm still not comfortable committing this patch, in several areas (some are minor/typographical, some are content issues):

a)

+ * - Language types, which describe the possible types of translatable content.
...
+ * Drupal core defines three built-in language types:
+ * - Interface language: This is the page's main language. It is used to present
...
+ * - Content language: This is used to choose in which language to display
...

The first line here talks about "types of translatable content", but then it turns out that only one of the types is "content" (the other two being interface and URL). Since "content" has such a specific meaning in Drupal, I think we need to use a different word in the first line, especially since one of the types is "content". Perhaps "describe the types of translatable text" would be better? [I also think the word "possible" should be left out]

b)

+ * Different language types often share the same values, but the they can have
+ * independent values if needed.

What are the "values" here? Maybe this is trying to say that the language for the different language types is often the same for the same page request?

c)

+ * Core includes the following methods:

- This should say "language methods".
- It should also either say "Drupal core" or the language type paragraphs should just say "Core" instead of "Drupal core", for parallel construction. I don't care which. :)

d)

+ * In Drupal 6.x, there is only one language type, named just language. During
+ * language initialization the selected language negotiation settings are used
+ * to determine its value. In Drupal 7.x, the same process happens for each
+ * defined language type, see drupal_language_initialize() for details.

This is Drupal 8 API documentation, not an on-line Drupal.org page. So don't talk about Drupal 6 or 7. This applies here too:

+ * invoked. This way the concept of fallback is generalized and untied from the
+ * fixed path prefix > user preference > browser settings > default language
+ * scheme used in Drupal 6.x.

e) In general, there is a lot of redundancy in this, and it seems to go in circles. For instance, there's a paragraph that starts with:

+ * A language type may be configurable or fixed. A configurable language type
+ * can be adjusted in the user interface where the language methods for that
+ * language type can be configured. There are also fixed language types that

Then lower down, there's another that starts with:

+ * Every language type can have different language negotiation settings; i.e.,
+ * every language type can have a different set of language detection methods,
+ * or providers, assigned to it.

This seems to convey much the same information, and/or to contradict what's above (it doesn't specify "configurable").

f)

+ * Language providers are simple callback functions that implement a particular
+ * logic to return a language code. For instance, the URL language provider
...

It seems like language providers are a third concept that should be discussed along with language types and methods above? Or is it really the same as language methods? If so, use consistent terminology.

g)

+ * Language provider definitions may include two more callbacks besides the
+ * language provider itself:
+ * - If the language provider can take advantage of a language switcher block,
+ *   the switcher callback will allow it to return the language switch links
+ *   that suit its logic, see locale_language_switcher_url() for an example.
+ * - If the language provider needs to rewrite URLs, it can specify an
+ *   url_rewrite callback which will provide the rewriting logic.

This paragraph seems like it belongs in the hook documentation, not here.

h)

+ * It is possible to customize the language negotiation process both for how we
+ * detect the user's language and also for what type of data they are
+ * requesting.

Here, the word "we" is used to refer to Drupal. Elsewhere, it's "Drupal". I think it should be "Drupal" throughout.

i) hook_langauge_negotiation_info()'s one-line description is "Allow modules to define their own language negotiation methods.", and it refers to "language negotiation" throughout. The hooks and this new topic should use the same terminology.

j) The hooks related to this topic need @ingroup references to this new topic.

Anonymous’s picture

I'll block off some time on the weekend to work through the edits. I'm swamped with work for the next few days.

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Anonymous’s picture

Assigned: Unassigned »
FileSize
16.63 KB

Patch to address issues raised in #54. This patch affects one additional file (core/modules/system/language.api.php) to account for the hook definitions.

There is a lot of terminology cleanup here: language providers [d7 term] are now all referred to as language methods [d8]. I also changed a number of references to "language negotiation methods" to be simply "language methods" for consistency. One array in hook_language_negotiation_info() was changed (name, description indexes) to reflect this as well.

Some paragraphs were reworked for clarity and to avoid duplication and/or circular discussion.

I have also updated the example code blocks which had obvious errors. They have been updated to match corresponding D8 examples in the hook definitions and function names were renamed to "mymodule" from "language_negotiation" to avoid any confusion with core modules.

Anonymous’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

MUCH better! Now we can get down to small things I'd suggest fixing:

a)

+ * The language API allows contributed modules to define additional language

Let's just say "modules" here rather than "contributed modules" (presumably someone could do a "custom" module too).

b)

+ * Every configurable language type will now have its own (independent) language
+ * configuration. If two language types are configured the same way, their

will now have -> has

c)

+ * configuration. If two language types are configured the same way, their
+ * language switcher blocks will be functionally identical and will act on
+ * both language types.

This confused me for two reasons.

1) Suddenly we're talking about language switcher blocks (which haven't been mentioned before) I think there needs to be a sentence saying "For language types configured to use URL language selection as an option, the XYZ module will define a language switcher block, which can be enabled on the Blocks configuration page." or something like that (presumably with accurate information there instead of what I just made up as a guess.

2) The other point of confusion for me in this sentence is about "functionally identical and will act on both language types". Does that mean there will be two really identical language switcher blocks? Then maybe say that rather than 'functionally identical'... or otherwise explain what that means.

I think that language switcher blocks just switch the URLs, right? In which case, if you add that sentence I suggested above about the blocks, you can say something like "Although each language type that uses the URL method will have a language switcher blocks, they will all be functionally identical". Or something to that effect.

d)

+ *   $negotiation_info[LOCALE_LANGUAGE_NEGOTIATION_URL]['callbacks']
+ * ['negotiation'] = 'mymodule_from_url';
+ *   $negotiation_info[LOCALE_LANGUAGE_NEGOTIATION_URL]['file'] =
+ * drupal_get_path('module', 'mymodule') . '/mymodule.module';

I think that there is some strange character (maybe a tab?) between the * and the $ on these two lines, which I can see when I open the patch in emacs, but not when I look at it on the web. I'm not sure what it is, but if it's real then it needs to be replaced with ordinary spaces.

Also, probably it would be better to not wrap the code lines here, especially in the middle of $a[][][]. You are allowed to violate the usual 80-character limit inside @code. :) If you do want to wrap them, then the subsequent lines should be indented an extra two spaces.

Actually, the next part of the code block has these strange characters too:

+ *   // Use the core URL language method to get a valid language code.
+ *   require DRUPAL_ROOT . '/includes/locale.inc';
...

Just use spaces.

e)

+ *
+ * @link http://drupal.org/node/1497272 Language Negotiation API @endlink
+ */

This should probably be preceded by "For more information, see", because as it is, it's just a paragraph with text "Language Negotiation API" and no context.

f) There are a couple of other minor standard grammar/doc things that are not necessarily your responsibility to fix (they came from the previous code), but which (if you are touching these lines in the file) would be nice to clean up also.

In language.inc:

 /**
- * Checks if a language negotiation method is enabled for a language type.
+ * Checks if a language method is enabled for a language type.

if -> whether

- *   An array of language negotiation method weights keyed by method id.
+ *   An array of language method weights keyed by method id.

id -> ID [id = ego, supergo, id. ID = identifier]

-      // Check if the language negotiation method is defined and has the right
-      // type.
+      // Check if the language method is defined and has the right type.
 

if -> whether

in language.api.php:

+ *   - "fixed": A fixed array of language method identifiers to use to
+ *     initialize this language. Defining this key makes the language type
  *     non-configurable and will always use the specified methods in the given
  *     priority order.

... non-configurable and will always... -> non-configurable, so it will always

- *   identifier key. The language negotiation method definition is an indexed
- *   array that may contain the following key-value pairs:
+ *   An array of language method definitions. Each method has an identifier
+ *   key. The language method definition is an indexed array that may contain
+ *   the following key-value pairs:

indexed array -> associative array

+ *   the following key-value pairs:
  *   - "types": An array of allowed language types. If a language negotiation
  *     method does not specify which language types it should be used with, it

This list in the docs for hook_language_negotiation_info() could be cleaned up (remove the "") as per our list specs:
http://drupal.org/node/1354#lists

g) in language.api.php

+ *   the switcher callback will allow it to return the language switch links
+ *   that suit its logic, see locale_language_switcher_url() for an example.

That comma needs to be either a ; or a . (to start a new sentence with "See").

h)

+ * - If the language method needs to rewrite URLs, it can specify an
+ *   url_rewrite callback which will provide the rewriting logic.

an url_... -> a url_...

i) Actually, in the hook_language_negotiation_info() documentation, shouldn't this whole added part be incorporated into the @return section?

+ * Language method definitions may include two more callbacks besides the
+ * language method itself:
+ * - If the language method can take advantage of a language switcher block,
+ *   the switcher callback will allow it to return the language switch links
+ *   that suit its logic, see locale_language_switcher_url() for an example.
+ * - If the language method needs to rewrite URLs, it can specify an
+ *   url_rewrite callback which will provide the rewriting logic.

j)

+ *
+ * @ingroup language_negotiation
  */
 function hook_language_negotiation_info_alter(array &$negotiation_info) {

Probably hook_language_negotiation_info() and hook_language_negotiation_info_alter() should have @see references to each other?

Gábor Hojtsy’s picture

Issue tags: -sprint

Ryan: are you still working on this? I think its would be prudent to remove the sprint tag since there is no ongoing work on this :/ I'd like to use this tag for stuff that is actively worked on and people can help out / review.

Anonymous’s picture

Yes, working through a this one now. My schedule is finally back on track so I should be getting some drupal.org weekends happening again too.

Anonymous’s picture

Patch updated per comments in #59. I replaced the spaces that were problematic in emacs but I can't replicate it here so hopefully it is better now. I'm on a Unicode system (using vim) and occasionally I type an à at the end of the line which often needs *two* backspaces to overwrite... could have been that.

Anonymous’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks better to me. This has already been open and debated for a year. If the current doc proposal is not a significant improvement / it would not get committed we should spend our time elsewhere.

Gábor Hojtsy’s picture

Issue tags: +sprint

Putting back on board.

jhodgdon’s picture

Assigned: » jhodgdon

I will review this today or tomorrow, and if I find something else that I think needs fixing, I'll make a new patch rather than asking Ryan to do another round. If it's all good, I'll just commit it; otherwise, I'll submit my new patch for review by the multilingual team (with an interdiff or list of things I changed). How's that for a plan? :)

plach’s picture

Sounds good!

Anonymous’s picture

+1 jhodgdon! Thanks. I'll shift my brain over to other areas that need docs now. :)

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
19.07 KB

Here's a new patch. I rearranged and rewrote the language negotiation group/topic, as well as the hook docs. I also made a couple of grammar/punctuation/standards corrections... since much of the patch is new, I didn't think an interdiff was too useful... I think it just needs a review from scratch. Thoughts?

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Looks good. I only found this typo, otherwise should be good to go in.

+++ b/core/includes/language.incundefined
@@ -11,7 +13,91 @@
+ * Every language type can have a different set of language methods assigned to
+ * it. Different language types often share the same methods, but the they can

"but the they can"

plach’s picture

@jhodgdon:

I have a couple of additional remarks, you might want to wait for me to complete my review before working on a new patch :)

jhodgdon’s picture

OK, waiting for next review...

plach’s picture

+++ b/core/includes/language.inc
@@ -2,7 +2,9 @@
+ * @see http://drupal.org/node/1497272

@@ -11,7 +13,91 @@
+ * @link http://drupal.org/node/1497272 Language Negotiation API @endlink

Are these links correct? The documentation page there refers to D7 while this patch targets D8. Should we include the link only in the backported patch?

+++ b/core/includes/language.inc
@@ -11,7 +13,91 @@
+ * - Language methods: functions for determining which language to use to

Here and all around: the new docs should be consistent and use the full "language negotiation method" terminology or just "method" where it makes sense; "language method" sounds pretty meaningless to my ears in this context. It only means this to me:

\Drupal\Core\Language\Language::extend()

We tried hard to find a consistent terminology (and we discussed it) in #1222106: Unify language negotiation APIs, declutter terminology: I don't see the point of reverting all that work, especially because we have function names using the "language negotiation method" terminology.

I'd appreciate if all the hunks switching the terminology from "language negotiation method" to "language method" could be reverted and the new docs updated accordingly.

+++ b/core/includes/language.inc
@@ -11,7 +13,91 @@
+ *   @link field Field API @endlink for details).

Should we link the Field language API documentation page here?

+++ b/core/includes/language.inc
@@ -11,7 +13,91 @@
+ * Language types may be configurable or fixed. A configurable language type's
+ * methods can be configured in the user interface. A fixed language type
+ * has predetermined (module-defined) method settings and, thus, does not
+ * appear in the configuration page. Here is a code snippet that makes the

This sounds a bit misleading to me, I'd rather use something like the following: "Language types may be configurable or fixed. The language negotiation settings associated to a configurable language type can be explicitly assigned one or more language negotiation methods through the user interface. A fixed language type has predetermined (module-defined) language negotiation settings and, thus, does not appear in the configuration page."

+++ b/core/includes/language.inc
@@ -11,7 +13,91 @@
+ * it. Different language types often share the same methods, but the they can
+ * have independent methods if needed. If two language types are configured the

I'd replace "methods" with "language negotiation settings" or just "settings" here.

+++ b/core/includes/language.inc
@@ -11,7 +13,91 @@
+ * logic to return a language code. For instance, the URL language method

I'd use just "URL method" here.

+++ b/core/includes/language.inc
@@ -11,7 +13,91 @@
+ * function mymodule_language_negotiation_info_alter(&$negotiation_info) {
+ *   // Replace the core function with our own function.
+ *   $negotiation_info[LOCALE_LANGUAGE_NEGOTIATION_URL]['callbacks']['negotiation'] = 'mymodule_from_url';
+ *   $negotiation_info[LOCALE_LANGUAGE_NEGOTIATION_URL]['file'] = drupal_get_path('module', 'mymodule') . '/mymodule.module';
+ * }
+ *
+ * function mymodule_from_url($languages) {
+ *   // Use the core URL language method to get a valid language code.
+ *   require DRUPAL_ROOT . '/includes/locale.inc';
+ *   $langcode = locale_language_from_url($languages);
+ *
+ *   // If we have an administrative path just return the default language.
+ *   if (isset($_GET['q']) && strtok($_GET['q'], '/') == 'admin') {
+ *     return language_default()->language;
+ *   }
+ *   return $langcode;
+ * }
+ * ?>

This should be updated with D8 code:

<?php

function mymodule_language_negotiation_info_alter(&$negotiation_info) {
  // Replace the core function with our own function.
  $negotiation_info[LANGUAGE_NEGOTIATION_URL]['callbacks']['negotiation'] = 'mymodule_from_url';
  $negotiation_info[LANGUAGE_NEGOTIATION_URL]['file'] = drupal_get_path('module', 'mymodule') . '/mymodule.module';
}

function mymodule_from_url($languages) {
  // Use the core URL language negotiation method to get a valid language
  // code.
  module_load_include('language', 'inc', 'language.negotiation');
  $langcode = language_from_url($languages);

  // If we have an administrative path just return the default language.
  $path = request_path();
  if (!empty($path) && strtok($path, '/') == 'admin') {
    return language_default()->langcode;
  }

  return $langcode;
}
?>
+++ b/core/modules/system/language.api.php
@@ -106,31 +107,36 @@ function hook_language_types_info_alter(array &$language_types) {
+ *       language method. See locale_language_switcher_url() for an example.

language_switcher_url()

5 days to next Drupal core point release.

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
15.24 KB

re #73 - I mostly agree with what you wrote... One comment:

D7 vs. D8 links: Most of our Community Documentation pages address several Drupal versions. It is OK for us to link to the D7 page, assuming that if things change in D8 appreciably, that page will be updated with notes on the D8 changes. I believe that is the intention.

Here's a new patch, which hopefully addresses all the concerns above. Thanks for the reviews!

plach’s picture

There were a couple of minor things left. I went ahead and fixed them myself. Now this looks RTBC to me.

+++ b/core/includes/language.inc
@@ -11,10 +13,98 @@
+ *   $negotiation_info[LOCALE_LANGUAGE_NEGOTIATION_URL]['callbacks']['negotiation'] = 'mymodule_from_url';
+ *   $negotiation_info[LOCALE_LANGUAGE_NEGOTIATION_URL]['file'] = drupal_get_path('module', 'mymodule') . '/mymodule.module';

These lines still need to be updated to D8.

+++ b/core/includes/language.inc
@@ -68,8 +158,8 @@ function language_types_info() {
+ * whose language negotiation methods are unchangeable (defined by the module
+ * that defines the language type).

Actually negotiation settings are alterable by hook implementations.

5 days to next Drupal core point release.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Excellent! Any further reviews? Tentatively RTBC, but I'll wait until tomorrow to commit it in case anyone else has suggestions.

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: -Documentation

Thanks! Committed to 8.x.

Ready for port to D7... I am not sure which of the information here is the same in D7 and D8, so hopefully Ryan or someone else who does can make the port?

jhodgdon’s picture

Assigned: jhodgdon » Unassigned

Just realized I am still assigned on this issue. It would be better (I think/hope) if someone more familiar with the Language API changes between D7 and D8 would make the port. (I'd be happy to review/edit once the information is deemed correct, of course.) Thanks!

Anonymous’s picture

Assigned: Unassigned »

I have some code sprinting time I can use for this next week.

Gábor Hojtsy’s picture

Issue tags: -sprint

Moving off of Drupal 8 multilingual focus issues, remaining work does not apply to Drupal 8 anymore.

Albert Volkman’s picture

Status: Patch (to be ported) » Needs review
FileSize
14.48 KB

First take at a D7 port. I replaced instances of "method" with "provider" as it seemed appropriate.

jhodgdon’s picture

Status: Needs review » Needs work

The text in the proposed patch reads well! A few comments:

a) It would be helpful if someone more knowledgeable than I am could read through the patch and make sure it is accurately documenting Drupal 7.x's language capabilities. For instance, looking at locale_language_negotiation_info()'s function body, I'm not sure this is accurate:

+ * Drupal defines the following built-in language negotiation providers:
+ * - URL: Determine the language from the URL (path prefix or domain).
+ * - Session: Determine the language from a request/session parameter.
+ * - User: Follow the user's language preference.
+ * - Browser: Determine the language from the browser's language settings.
+ * - Default language: Use the default site language.

(Actually, looking at language_language_negotiation_info() in D8, was it accurate there either???)

b) I think some of the code snippets and function reference might be D8 instead of D7 too, such as:

+ *   module_load_include('language', 'inc', 'language.negotiation');
+ *       method. See language_switcher_url() for an example.

c) There are a couple of places where the terminology for "language negotiation provider" is not being consistently used:
- language.inc

  * @param $language_providers
- *   An array of language provider weights keyed by id.
+ *   An array of language provider weights keyed by provider ID.
  *   @see language_provider_weight()
  */
 function language_negotiation_set($type, $language_providers) {

Should be "language negotation provider" not "language provider".

- language.api.php - hook_language_negotiation_info() doc:

+ * Define language negotiation methods.
...
+ *   - weight: The default weight of the method.
+ *   - name: The translated human-readable name for the method.
+ *   - description: A translated longer description of the method.
+ *   - config: An internal path pointing to the method's configuration page.
+ *   - cache: The value Drupal's page cache should be set to for the current
+ *     method to be invoked.

method -> provider

- language.api.php - hook_language_types_info() doc:

+ *   - fixed: A fixed array of language negotiation method identifiers to use to

method -> provider

- hook_language_negotiation_info_alter():

  * @param $language_providers
  *   Array of language provider definitions.

language provider -> language negotiation provider

d) Something is missing from this sentence in hook_language_negotation_info() doc:

+ *   - types: An array of allowed language types. If a language negotiation
  *     not specify which language types it should be used with, it will be
  *     available for all the configurable language types.
Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
2.87 KB
14.74 KB

Fixed c-e. Interdiff'd against #81.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks! The changes in the interdiff look correct. They didn't take care of all of the terminology items identified in #82(c), but (d) is taken care of. And as a note, #82 a/b [make sure it's D7 not D8] still needs taking care of.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
8.62 KB
19.41 KB

I think I got the rest of c. Interdiff'd against #81 again.

jhodgdon’s picture

Status: Needs review » Needs work

That looks good -- I think almost all the terminology is correct now.

I did still see these problems in the patch:

a)

/**
- * Check if the given language provider is enabled for any configurable language
- * type.
+ * Check if the given language negotiation provider is enabled for any
+ * configurable language type.

The first line of any function doc block needs to be squeezed down to 80 characters or less somehow. Same here:

/**
- * Updates language configuration to remove any language provider that is no longer defined.
+ * Updates language configuration to remove any language negotiation provider
+ * that is no longer defined.

b) Gracious, I hope we didn't put this into the D8 patch... There are a lot of function doc blocks whose first lines are starting with "Return", "Check", etc. instead of "Returns", "Checks", etc. Those need to be fixed for sure! (Note that hooks do not follow this verb pattern though.)

c) (the provider hook in the api.php file)

/**
- * Allow modules to define their own language providers.
+ * Define language negotiation methods.

This one should be "language negotiation providers" I think -- shouldn't say "methods"?

d) The word "method" is still used in place of "provider" at at least 3 places in the patched documentation.

e) And as a note, #82 a/b [make sure it's D7 not D8] still needs taking care of.

jhodgdon’s picture

Version: 7.x-dev » 8.x-dev

I guess we should go back to D8 briefly and make a quick patch to change a couple of verb tenses:
a) language_url_split_prefix() -- Split the given path into prefix and actual path. ==> Splits
b) language_fallback_get_candidates() -- Return the possible fallback languages ordered by language weight. ==> Returns

The rest look OK.

YesCT’s picture

Issue tags: +Novice

The small changes for D8 look novice to me. Adding tag just for that part.

related? #1502816: Add help text explaining the limitations of language negotiation and page caching

Albert Volkman’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs work » Needs review
FileSize
7.54 KB
20.23 KB

Issues in #87 were resolved with #1317626: Clean up API docs for includes directory, files starting with H-M.

Issues in #86 are resolved in this patch. Confirmed that 86b is not exhibited in D8.

YesCT’s picture

Issue tags: -Novice

thanks!

jhodgdon’s picture

Assigned: » jhodgdon
Status: Needs review » Reviewed & tested by the community

Looks good -- I'll get this committed sometime soon.

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Fixed

THANKS to everyone who helped out on this issue! Committed to 7.x.

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