As per #1216094: We call too many things 'language', clean that up we are converting schemas and parallel APIs to use 'langcode' instead of 'language' where a language code is involved. The use of 'language' inconsistently for language objects and language codes in Drupal 7 keeps throwing people off the cliff and makes understanding the API much harder.

This issue is about node.module needing the same conversion to move from $node->language to $node->langcode. Will need updates and upgrade tests but first of all it would be a well focused search and replace wherever node language is involved. The tricky part is that field language is a different story, this should focus on node language to keep scope limited.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler’s picture

Assigned: Unassigned » tstoeckler

Taking this one for now.

tstoeckler’s picture

Status: Active » Needs review
FileSize
28.63 KB

All right.
This is the first try.
It passes node tests and at time of creation half of comment tests locally.
So I want to see what other parts might fail. Translation still fails I think (also still running), but I think that should be self-contained enough for a first round of a complete run-through.

I have not started on upgrade tests.

tstoeckler’s picture

So here's what I think the upgrade tests should do:
1. Create nodes with LANGUAGE_NONE
2. Create nodes with something other than that
3. Create a translation set with translation.module
And then in D8 assert that each of those still work like in D7.
Anything else?

tstoeckler’s picture

The latest patch in #1410096: Convert comment language code schema to langcode reminded that there is this little thing called node revisions...
We should test those as well.

Status: Needs review » Needs work

The last submitted patch, 1439686-2-node-language-langcode.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review

Aha! For whichever reason node_revision doesn't have a language column. That is weird, but less work for me :)

tstoeckler’s picture

All right, this should be almost finished.
- On my system AjaxMultiFormTestCase fails, and I didn't figure out why. If the bot has the problem someone would need to investigate that.
- I did the bulk of the work with the upgrade tests, but I need to run now, so I couldn't finish them. If you apply the patch and run the 'Language upgrade tests' you will see that I drupalGet() node/40/translate which is a translation overview table. I don't assert anythingon that yet, but we probably want to assert that the Correct languages are listed and that the links to the translations are correct.

tstoeckler’s picture

Assigned: tstoeckler » Unassigned

Unassigning for now, although if someone reviews this, I will probably pick it up in the next few days.

fubhy’s picture

+++ b/core/modules/field/modules/text/text.testundefined
@@ -455,7 +455,7 @@ class TextTranslationTestCase extends DrupalWebTestCase {
     $edit = array(
       "title" => $this->randomName(),
-      "language" => 'en',
+      "langcode" => 'en',
       "body[$langcode][0][value]" => $body,

Why does it use double quotes here? (Partly before the patch) - Shouldn't be covered by this patch, I was just curious :).

Other than that it looks okay from a reviewing persepective.

tstoeckler’s picture

@fubhy There really is no reason. Somewhere around this code, there was one array key which used double-quotes to embed a variable, so I guess this was some sort of "consistency", but I really hate stuff like that.
I hope no kittens will be harmed if I change that in the next reroll.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

This all looks very nice, and it even has extensive upgrade tests! Superb! Some likely minor notes:

+++ b/core/modules/locale/locale.moduleundefined
@@ -296,8 +296,8 @@ function locale_field_node_form_submit($form, &$form_state) {
+      if ($field['translatable'] && $previous_language != $node->langcode) {
+        $form_state['values'][$field_name][$node->langcode] = $node->{$field_name}[$previous_language];

$previous_language should be $previous_langcode

+++ b/core/modules/node/node.moduleundefined
@@ -1340,7 +1340,7 @@ function node_view($node, $view_mode = 'full', $langcode = NULL) {
-    '#language' => $langcode,
+    '#langcode' => $langcode,

Does this not have implications on the rendering/field system, and if so are those taken care of?

+++ b/core/modules/node/node.moduleundefined
@@ -2848,10 +2848,8 @@ function node_form_search_form_alter(&$form, $form_state) {
-    foreach (language_list() as $key => $entity) {
-      if ($entity->enabled) {
-        $language_options[$key] = $entity->name;
-      }
+    foreach (language_list(TRUE) as $langcode => $language) {
+      $language_options[$langcode] = $language->name;

Yay, this looks great. Languages are not $entity :) :)

tstoeckler’s picture

Fixes the above.

Does this not have implications on the rendering/field system, and if so are those taken care of?

Well this is definitely part of the API change for implementors of HOOK_preprocess_node or people you call node_view() directly. There are a bunch of those in core but the only one who accesses $variables['language'] is locale_preprocess_node(), which is covered in the patch, as you can see.

As I had noted in #7 I added some more assertions for the upgrade test. For the "tests-only" patch, what I did this time is only remove the update function from the patch. I think that should be more precise proof that the update function is correct.
Either way, though I think this patch is good to go. I also tried this manually created a bunch of languages and translation etc. and everything works. So I would say, if there are no code style issues, this should be RTBC.

tstoeckler’s picture

Status: Needs work » Needs review

Oops, forgot to set needs review.

tstoeckler’s picture

Assigned: Unassigned » tstoeckler

Also, re-assigning. Now that it is basically finished, if something comes up in reviews I guess I'll drive this home so @GáborHojtsy can mark this RTBC.

EDIT: Or anyone else, of course, didn't want to exclude you from the party @fubhy :)

(PS: Sorry for the noise, it's late...)

fubhy’s picture

Status: Needs review » Needs work
+++ b/core/modules/node/node.tokens.incundefined
@@ -50,9 +50,9 @@ function node_token_info() {
+  $node['langcode'] = array(
+    'name' => t("Language code"),
+    'description' => t("The language code of the language the node is written in."),

I would prefer using single quotes here. But that is really a personal thing as I am very strict about the whole single- / double quote argument.

+++ b/core/modules/simpletest/tests/upgrade/drupal-7.language.database.phpundefined
@@ -512,11 +512,11 @@ db_insert('variable')->fields(array(
 ->values(array(
   'name' => 'language_content_type_article',
-  'value' => 's:1:"1";',
+  'value' => 's:1:"2";',

Just a minor thing: Using serialize(2) might increase readability here. Not sure how we handle that elsewhere though.

Other than that it looks really nice and it also looks like it covers everything.

Feel free to directly set it to RTBC (my notes are rather proposals than real issues).

Gábor Hojtsy’s picture

@fubhy: on the serialize() stuff, the dumps are meant to be generatable by a script from directly from the database, so they usually contain much more convoluted structures. I think we need to accept that as a matter of fact.

tstoeckler’s picture

Status: Needs work » Reviewed & tested by the community

Ok, fixed #15.1
Didn't upload a tests-only patch again, I think the point has been proven.
RTBC per #15.

tstoeckler’s picture

Ahhh, sorry!

Dries’s picture

I reviewed this patch and it looks good. However, it needs a re-roll because of either #1416392: Clean up language (types) bootstrap function naming and documentation or #1410096: Convert comment language code schema to langcode going in. Let's ask for a re-test to make sure.

Dries’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +D8MI, +sprint, +langcode, +language-content

The last submitted patch, 1439686-17-node-language-langcode.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
45.1 KB

Well, here's a reroll.
Because the language-comment and language-node upgrade tests are now integrated with one another, I changed some comments to make it more clear, what node is used for what. I guess that could use another look-over so moving to 'needs review' and not straight back to RTBC. Basically this is still the same as #18, though, so...

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good. Has a very comprehensive test suite which verifies not only the langcode rename but that it still works with translation module. Good stuff.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Looks great. Committed to 8.x. Thanks!

Gábor Hojtsy’s picture

Issue tags: -sprint

Change notice posted at http://drupal.org/node/1451962 - removing from sprint.

tstoeckler’s picture

Awesome stuff!
@GáborHojtsy: I added some more examples to the change notice with the theming changes that you noted in #11.2

Gábor Hojtsy’s picture

@tstoeckler: Superb, thanks!

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