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.
Comment | File | Size | Author |
---|---|---|---|
#89 | language-negotiation-docs-1156576-89.patch | 20.23 KB | Albert Volkman |
#89 | interdiff.txt | 7.54 KB | Albert Volkman |
#85 | language-negotiation-docs-1156576-85.patch | 19.41 KB | Albert Volkman |
#85 | interdiff.txt | 8.62 KB | Albert Volkman |
#83 | language-negotiation-docs-1156576-83.patch | 14.74 KB | Albert Volkman |
Comments
Comment #1
sun/me loves this issue
Comment #2
plachComment #3
jhodgdonWhere 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)?
Comment #4
plachI 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.
Comment #5
jhodgdonActually, 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?
Comment #6
Gábor Hojtsy@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.
Comment #7
chx CreditAttribution: chx commentedIn my opinion when bootstrap.inc constants are a WTF (what the heck is a language type, anyways) that goes way beyond a task.
Comment #8
Gábor HojtsyBased 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.
Comment #9
plachComment #10
Gábor HojtsyCrosspost. Also, here is the google doc URL, I can give edit access to people who want: https://docs.google.com/drawings/d/174N_BZAWv2urowMZGZsdikpWicCfUfa1d2eX...
Comment #11
sunSorry, but demoting from major. Missing documentation should not hold up D8 initiatives.
Comment #12
jhodgdonSo... 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?
Comment #13
Gábor Hojtsy@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.
Comment #14
plachAFAIK, nothing neither on a.d.o nor on d.o. except for the module conversion page linked in the OP.
Comment #15
jhodgdonOK. 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?
Comment #16
Gábor Hojtsy@jhodgdon: do you need it in phpdoc form? I think plach's update guide doc is a very good start for that.
Comment #17
jhodgdonOh 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.
Comment #18
Gábor HojtsyTagging
Comment #19
jhodgdonWe 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?
Comment #20
Gábor Hojtsy@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?
Comment #21
jhodgdonI 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).
Comment #22
Gábor HojtsyTagging for base language system.
Comment #23
Gábor HojtsyTagging for language negotiation too.
Comment #24
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #25
Gábor HojtsyTagging for current sprint :)
Comment #26
pixeliteI'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.
Comment #27
Anonymous (not verified) CreditAttribution: Anonymous commentedThe developer documentation is here: http://drupal.org/node/1497272
Comment #28
Gábor HojtsyStill needs to be integrated in phpdoc too to some degree.
Comment #29
pixeliteI reviewed the developer documentation. Ryan, do you want to integrate into the phpdoc?
Comment #30
Anonymous (not verified) CreditAttribution: Anonymous commentedD8 patch to languages.inc to include reference to documentation and to group functions into API docs page.
Comment #31
Anonymous (not verified) CreditAttribution: Anonymous commentedSorry, I mistakenly used an older version of that file to roll the patch. I'll re-roll with just the reference...
Comment #32
Anonymous (not verified) CreditAttribution: Anonymous commentedHere we go... starting to get the hang of this. :)
Comment #33
Anonymous (not verified) CreditAttribution: Anonymous commentedBackport for D7!
Comment #34
jhodgdonPlease 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.
Comment #35
Anonymous (not verified) CreditAttribution: Anonymous commentedThank 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).
Comment #36
Kristen PolLooks good to me. I applied the patch with no issue and read through the documentation.
Comment #37
Gábor HojtsyAs 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.
Comment #38
Anonymous (not verified) CreditAttribution: Anonymous commentedI 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?).
Comment #39
Anonymous (not verified) CreditAttribution: Anonymous commented... and I see my console was set to 82 characters *facepalm*.
Comment #40
Anonymous (not verified) CreditAttribution: Anonymous commentedRerolled.
Comment #41
Kristen PolLooks okay to me. I applied the patch with no issue and looked through the documentation.
Comment #42
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedMissing period.
Extra space in the beginning of the second sentence. Removing this will also allow "implementation" to move up one line.
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.
Missing period.
Missing periods.
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 :)
Should it be "language provider definitions"?
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.
Comment #43
Gábor HojtsyComment #44
jhodgdonAlso, when you make lists, do not put a blank line between the line before the list and the list, as was done here:
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:
You need to format them differently, as narratives and paragraphs.
Sections like this:
should be formatted more like:
And the grammar in this whole section is a mess.
Comment #45
Anonymous (not verified) CreditAttribution: Anonymous commentedThanks for the feedback everyone. I made the requested changes in #42 and #44. By removing the headings the documentation reads a lot better.
Comment #46
Kristen PolI 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!):
"describes" => "describe"
"allows" => "allow"
"associated to URLs" => "associated with URLs"
These could be fixed in the d.o documentation as well.
Comment #47
jhodgdonA couple of other things also need to be fixed:
a)
- 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)
Can we call this language_negotiation please?
c)
- That line "Language Negotiation API" should be a sentence. See http://drupal.org/node/1354#groups
d)
This sentence uses "for" and "on" in two parallel clauses. Use the same preposition for both.
e)
- Don't refer to a particular Drupal version. Just document the version that this documentation is in. Same goes for this text below:
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)
Remove the blank line here. This should be part of the previous paragraph/list.
g)
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)
Get rid of the blank line here, and don't put in < ?php inside @code. Same applies to the other @code blocks.
i)
This example should probably use mymodule_ not language_negotiation_ in the sample function name.
j)
- Punctuation: "... settings; i.e., every language..."
Comment #48
plachATM the correct way to do this is:
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.
Comment #49
Gábor Hojtsy@Ryan Weal: are you still working on this?
Comment #50
Anonymous (not verified) CreditAttribution: Anonymous commentedHey 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.
Comment #51
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #52
Gábor HojtsyLooks good to me :)
Comment #53
webchickWhile this is eligible for D7 backport, it has jhodgdon's fingerprints all over it so I am planning to leave for her.
Comment #54
jhodgdonI'm still not comfortable committing this patch, in several areas (some are minor/typographical, some are content issues):
a)
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)
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)
- 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)
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:
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:
Then lower down, there's another that starts with:
This seems to convey much the same information, and/or to contradict what's above (it doesn't specify "configurable").
f)
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)
This paragraph seems like it belongs in the hook documentation, not here.
h)
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.
Comment #55
Anonymous (not verified) CreditAttribution: Anonymous commentedI'll block off some time on the weekend to work through the edits. I'm swamped with work for the next few days.
Comment #56
jhodgdonComment #57
Anonymous (not verified) CreditAttribution: Anonymous commentedPatch 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.
Comment #58
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #59
jhodgdonMUCH better! Now we can get down to small things I'd suggest fixing:
a)
Let's just say "modules" here rather than "contributed modules" (presumably someone could do a "custom" module too).
b)
will now have -> has
c)
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)
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:
Just use spaces.
e)
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:
if -> whether
id -> ID [id = ego, supergo, id. ID = identifier]
if -> whether
in language.api.php:
... non-configurable and will always... -> non-configurable, so it will always
indexed array -> associative array
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
That comma needs to be either a ; or a . (to start a new sentence with "See").
h)
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?
j)
Probably hook_language_negotiation_info() and hook_language_negotiation_info_alter() should have @see references to each other?
Comment #60
Gábor HojtsyRyan: 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.
Comment #61
Anonymous (not verified) CreditAttribution: Anonymous commentedYes, 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.
Comment #62
Anonymous (not verified) CreditAttribution: Anonymous commentedPatch 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.
Comment #63
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #64
Gábor HojtsyLooks 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.
Comment #65
Gábor HojtsyPutting back on board.
Comment #66
jhodgdonI 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? :)
Comment #67
plachSounds good!
Comment #68
Anonymous (not verified) CreditAttribution: Anonymous commented+1 jhodgdon! Thanks. I'll shift my brain over to other areas that need docs now. :)
Comment #69
jhodgdonHere'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?
Comment #70
Gábor HojtsyLooks good. I only found this typo, otherwise should be good to go in.
"but the they can"
Comment #71
plach@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 :)
Comment #72
jhodgdonOK, waiting for next review...
Comment #73
plachAre 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?
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:
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.
Should we link the Field language API documentation page here?
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."
I'd replace "methods" with "language negotiation settings" or just "settings" here.
I'd use just "URL method" here.
This should be updated with D8 code:
language_switcher_url()
5 days to next Drupal core point release.
Comment #74
jhodgdonre #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!
Comment #75
plachThere were a couple of minor things left. I went ahead and fixed them myself. Now this looks RTBC to me.
These lines still need to be updated to D8.
Actually negotiation settings are alterable by hook implementations.
5 days to next Drupal core point release.
Comment #76
jhodgdonExcellent! Any further reviews? Tentatively RTBC, but I'll wait until tomorrow to commit it in case anyone else has suggestions.
Comment #77
jhodgdonThanks! 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?
Comment #78
jhodgdonJust 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!
Comment #79
Anonymous (not verified) CreditAttribution: Anonymous commentedI have some code sprinting time I can use for this next week.
Comment #80
Gábor HojtsyMoving off of Drupal 8 multilingual focus issues, remaining work does not apply to Drupal 8 anymore.
Comment #81
Albert Volkman CreditAttribution: Albert Volkman commentedFirst take at a D7 port. I replaced instances of "method" with "provider" as it seemed appropriate.
Comment #82
jhodgdonThe 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:
(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:
c) There are a couple of places where the terminology for "language negotiation provider" is not being consistently used:
- language.inc
Should be "language negotation provider" not "language provider".
- language.api.php - hook_language_negotiation_info() doc:
method -> provider
- language.api.php - hook_language_types_info() doc:
method -> provider
- hook_language_negotiation_info_alter():
language provider -> language negotiation provider
d) Something is missing from this sentence in hook_language_negotation_info() doc:
Comment #83
Albert Volkman CreditAttribution: Albert Volkman commentedFixed c-e. Interdiff'd against #81.
Comment #84
jhodgdonThanks! 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.
Comment #85
Albert Volkman CreditAttribution: Albert Volkman commentedI think I got the rest of c. Interdiff'd against #81 again.
Comment #86
jhodgdonThat looks good -- I think almost all the terminology is correct now.
I did still see these problems in the patch:
a)
The first line of any function doc block needs to be squeezed down to 80 characters or less somehow. Same here:
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)
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.
Comment #87
jhodgdonI 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.
Comment #88
YesCT CreditAttribution: YesCT commentedThe 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
Comment #89
Albert Volkman CreditAttribution: Albert Volkman commentedIssues 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.
Comment #90
YesCT CreditAttribution: YesCT commentedthanks!
Comment #91
jhodgdonLooks good -- I'll get this committed sometime soon.
Comment #92
jhodgdonTHANKS to everyone who helped out on this issue! Committed to 7.x.