The attached patch moves all Node-related code from Locale to Node, because it belongs there. It is split off from #375397: Make Node module optional.

Files: 
CommentFileSizeAuthor
#70 decouple-locale-from-node-540294-70.patch17.48 KBclemens.tolboom
PASSED: [[SimpleTest]]: [MySQL] 34,334 pass(es).
[ View ]
#66 decouple-locale-from-node-540294-66.patch17.47 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 34,333 pass(es).
[ View ]
#61 decouple-locale-from-node-540294-61.patch21.15 KBclemens.tolboom
PASSED: [[SimpleTest]]: [MySQL] 34,259 pass(es).
[ View ]
#59 decouple-locale-from-node-540294-59.patch20.46 KBclemens.tolboom
FAILED: [[SimpleTest]]: [MySQL] 34,264 pass(es), 9 fail(s), and 0 exception(s).
[ View ]
#58 upgrade-workflow.png6.22 KBclemens.tolboom
#46 decouple-locale-from-node-46.patch15.22 KBclemens.tolboom
PASSED: [[SimpleTest]]: [MySQL] 34,062 pass(es).
[ View ]
#43 decouple-locale-from-node-43.patch14.6 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 34,024 pass(es).
[ View ]
#39 decouple-locale-from-node-39.patch14.54 KBclemens.tolboom
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch decouple-locale-from-node-39.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#38 decouple-locale-from-node-38.patch14.54 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 33,915 pass(es).
[ View ]
#35 decouple-locale-from-node-35.patch14.37 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 33,322 pass(es).
[ View ]
#33 decouple-locale-from-node-33.patch12.78 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 33,131 pass(es), 195 fail(s), and 317 exception(es).
[ View ]
#28 decouple-locale-from-node-28.patch14.22 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 33,804 pass(es).
[ View ]
#26 decouple-locale-from-node-26.patch13.98 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 33,788 pass(es), 9 fail(s), and 3 exception(es).
[ View ]
#24 decouple-locale-from-node-24.patch14.92 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 33,656 pass(es), 118 fail(s), and 172 exception(es).
[ View ]
#20 decouple-node-and-locale.patch14.68 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 33,511 pass(es), 130 fail(s), and 180 exception(es).
[ View ]
#9 node_decouple_locale_05.patch20.88 KBXano
Failed: 11793 passes, 26 fails, 10 exceptions
[ View ]
#7 node_decouple_locale_04.patch20.84 KBXano
Failed: 11821 passes, 27 fails, 10 exceptions
[ View ]
#5 node_decouple_locale_03.patch8.19 KBXano
Failed: 11813 passes, 40 fails, 10 exceptions
[ View ]
#2 node_decouple_locale_02.patch8.11 KBXano
Failed: 11307 passes, 738 fails, 179 exceptions
[ View ]
node_decouple_locale_01.patch8.23 KBXano
Failed: Failed to install HEAD.
[ View ]

Comments

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new8.11 KB
Failed: 11307 passes, 738 fails, 179 exceptions
[ View ]

Schema doesn't yet need to be (un)installed in this patch.

Status:Needs review» Needs work

The last submitted patch failed testing.

<?php
+  if (TRUE || variable_get('node_type_language_' . $node->type, 0)) {
?>

I guess that's debugging stuff?

Status:Needs work» Needs review
StatusFileSize
new8.19 KB
Failed: 11813 passes, 40 fails, 10 exceptions
[ View ]

Good one. That was what made at least some of the tests fail.

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new20.84 KB
Failed: 11821 passes, 27 fails, 10 exceptions
[ View ]

Locally all tests pass, except for "Path alias functionality', which causes a 500 error and all I can find is this crash message.

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new20.88 KB
Failed: 11793 passes, 26 fails, 10 exceptions
[ View ]

Hmm, weird. The language element is #edit-language-1 instead of #edit-language in the node add form...

By the way, some of the tests that fail according to t.d.o work fine when doing locally (Path aliases with translated nodes and Translation functionality.

Status:Needs review» Needs work

The last submitted patch failed testing.

A lot of this code should stay in locale.module - locale is already optional, so it's fine for it to form_alter() node where that makes sense - and additionally locale is de-facto installed on less sites than node.

The reason for migrating this code is that the storage of a node's language already happens in Node.module. With this patch all code related to a node's language has moved to Node. Putting it in Locale was an option as well, but that would have required a migration path.

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

Interesting. I agree in Drupal 8 modules will need to contain language support themselves if we consider language support fundamental enough. Does the goal here still apply to Drupal 8? (It will certainly not happen anymore in Drupal 7).

The general goal of making node module optional (which is more about removing circular dependencies than the .info flag) is a good one.

Looking at the patch here, I disagree with my 2009 self - we should move the code from locale to node.module as it does here.

On top of that, we should look at whether some of these features would be better maintained at the entity level (and/or shifted further towards field-level translation).

However having it located in one place in the first place would be a good basis for that.

Yes, we should look at generalizing the node translation set concept to entities overall, that should be a different issue. I have not opened one for that before, now opened #1190454: Abtract translation set concept to work with entities in general. I think we should move to node module as a start, and look at generalizing it as a bigger task.

Issue tags:+D8MI

Let's make this happen in Drupal 8.

Issue tags:+locale-split

Title:Decouple Locale from NodeMove node language settings from Locale to Node module

Status:Needs work» Needs review
StatusFileSize
new14.68 KB
FAILED: [[SimpleTest]]: [MySQL] 33,511 pass(es), 130 fail(s), and 180 exception(es).
[ View ]

Updated patch. This includes all changes but the test moves from Xano's patch. Based on #9. Not tested much. In summary the patch:

- moves content type language config from locale to node module
- moves node language selector from locale to node (and attempts to add a fieldset summary, not sure that works)
- renames the content type language variables to be in the node_* namespace

What's missing?

- upgrade path for the variable rename
- moving tests around

Locale took on field translation functionality as well in the meantime, which I kept there for now, but it has some code which are at the crossroads of node, field and locale module. Ha!

Interested in human and automated test review, so marking needs review (despite some stuff missing).

Status:Needs review» Needs work

db_query("DELETE FROM {variable} WHERE name LIKE 'node_%'");

Should be db_delete().

Are the variable checks enough for this as opposed to module_exists('locale') - can they get set otherwise?

Variable deletion could happen in hook_modules_uninstalled() as for when node stays enabled but locale doesn't.

Are the variable checks enough for this as opposed to module_exists('locale') - can they get set otherwise?

They can get set and then locale disabled, which would whitescreen on locale_language_list('name'). If we modify that to a module_invoke(), that sounds like would present a separate language list if locale is not there, and could result in validation failures (the possible options would not have the selected language, but you cannot change it)?

Status:Needs work» Needs review
StatusFileSize
new14.92 KB
FAILED: [[SimpleTest]]: [MySQL] 33,656 pass(es), 118 fail(s), and 172 exception(es).
[ View ]

Here is a patch rerolled for current /core setup (but the previous patch applied with minor offsets there). Also updated the db_delete() as requested. Looking at the code again, the places where it checks for the variable are either in locale module directly or in translation.module (dependent on locale module). The only place it is checked direct in node.module it is coupled with module_exists('locale'), so this should be complete IMHO. I'm not sure the variables should be deleted if locale module is uninstalled but node is not. The settings are for node module, not for locale module, and when/if locale module is installed again they should be there IMHO.

The upgrade path for the variable rename and moving tests around is still missing, otherwise still needs review.

Status:Needs review» Needs work

The last submitted patch, decouple-locale-from-node-24.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new13.98 KB
FAILED: [[SimpleTest]]: [MySQL] 33,788 pass(es), 9 fail(s), and 3 exception(es).
[ View ]

Instead of trying to rework the UX of specifying languages in node forms, let's postpone that for later. It caused too many problems and would probably cause issues with reviews and getting this in faster anyway. Also fixed the translation module form alter to properly change the field type too. This should pass a lot more tests (if not all).

Status:Needs review» Needs work

The last submitted patch, decouple-locale-from-node-26.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new14.22 KB
PASSED: [[SimpleTest]]: [MySQL] 33,804 pass(es).
[ View ]

The remaining test fail seems to be due to LANGUAGE_NONE not used properly for no language assignment for nodes. Fixed that.
Edit: also removed checking for the removal of the node setting, that is now not removed on locale uninstall since its governed by node as discussed above.

Status:Needs review» Needs work
Issue tags:-Framework Initiative, -D8MI, -locale-split

The last submitted patch, decouple-locale-from-node-28.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Framework Initiative, +D8MI, +locale-split

#28: decouple-locale-from-node-28.patch queued for re-testing.

Ok, now that this passes, @catch any updated feedback that we should be looking at (minus the missing update function). On possibly moving tests, since the tests cover the combination of modules, it can live at either place and can just be with locale module IMHO.

#1074672: Allow language select to be rearranged inside node form is blocked by this and will be rerolled after committing this to 8.x.

StatusFileSize
new12.78 KB
FAILED: [[SimpleTest]]: [MySQL] 33,131 pass(es), 195 fail(s), and 317 exception(es).
[ View ]

Quick reroll for the introduction of language.module and some changes in core. Let's see if this still passes tests. Also with this and #1414314: Make node and path depend on language module only for language support, get rid of locale_language_name, locale module is finally out of the game for language assignment and support for nodes. Let's move this forward.

Status:Needs review» Needs work

The last submitted patch, decouple-locale-from-node-33.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new14.37 KB
PASSED: [[SimpleTest]]: [MySQL] 33,322 pass(es).
[ View ]

Found one more instance of old settings form item ID used to set articles to translatable. Also, some leftover code removed from locale module. Passes at least the locale tests on my computer, let's see the rest.

Issue tags:-locale-split+sprint

Tagging as current focus.

Status:Needs review» Needs work

I cannot apply patch #35

git apply ../decouple-locale-from-node-35.patch
error: patch failed: core/modules/locale/locale.module:1008
error: core/modules/locale/locale.module: patch does not apply

Status:Needs work» Needs review
StatusFileSize
new14.54 KB
PASSED: [[SimpleTest]]: [MySQL] 33,915 pass(es).
[ View ]

Thanks Clemens. Indeed, the code in question moved to comment.module since the last patch was rolled above. Any other feedback?

Status:Needs review» Needs work
StatusFileSize
new14.54 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch decouple-locale-from-node-39.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

There are two competing 'workflow' the alter the node_type forms. module_exist versus node_type_form_alter

<?php
if (module_exists('language')) {
?>

+++ b/core/modules/node/content_types.inc
@@ -187,6 +187,14 @@ function node_type_form($form, &$form_state, $type = NULL) {
+      '#description' => t('Add a language selection field to the editing form, allowing you to select from one of the <a href="!languages">enabled languages</a>. If disabled, new posts are saved with the default language. Existing content will not be affected by changing this option.', array('!languages' => url('admin/international/language'))),
+    );

This description is competing with translation_form_node_type_form_alter

+++ b/core/modules/translation/translation.module
@@ -111,9 +111,10 @@ function translation_permission() {
+  $form['workflow']['node_type_language']['#description'] = t('Add a language selection field to the editing form, allowing you to select from one of the <a href="!languages">enabled languages</a>. You can also turn on translation for this content type, which lets you have content translated to any of the installed languages. If disabled, new posts are saved with the default language. Existing content will not be affected by changing this option.', array('!languages' => url('admin/international/language')));
}

Should both the #description be the same? I guess not but not sure.

It is weird translation is using a node_type_alter ... shouldn't that code be in the node.module?

The links mentioned above should point to admin/config/regional/language.

Attached patch fixes only the link.

Status:Needs work» Needs review

@clemens.tolboom: Yes, the translation module alters the language assignment option, because if you have node + language module, you get the language assignment feature (as explained in node module). If you also enable the translation module, you get language assignment + translation as well, so both options need to be explained. The translation module description has this explanation in the middle to cover that but otherwise looks identical to me to the node module one(?):

You can also turn on translation for this content type, which lets you have content translated to any of the installed languages.

The translation module can use the node_type_form_alter() without checking for language module, because it will depend on language module (currently depends on locale module which depends on language module). So it is already ensured that language module is on, no need to check for it. (Which also means this form element is there and can be modified).

Sidenote: a big drawback of the translation module is that it only supports nodes and that it only works in a node relational way. Our D8 plans include dropping this module altogether and move in the entity_translation contrib module instead (likely under a name something like content_translation). So we don't have any big plans for the translation.module as-is, more like plans to clean up the field translation API and provide a UI for that for all entity types as well as provide a core built-in migration path from the current translation.module. So we should keep translation module intact until that is available, but otherwise not planning any changes there.

Moving to needs review based on that. Any other comments?

Status:Needs review» Needs work

The last submitted patch, decouple-locale-from-node-39.patch, failed testing.

The code looks good to me for a RTBC but I'm only scratching D8MI now :(

Status:Needs work» Needs review
StatusFileSize
new14.6 KB
PASSED: [[SimpleTest]]: [MySQL] 34,024 pass(es).
[ View ]

Ok, well, the above patch did not apply anymore either due to other changes in core. Rerolled. No changes.

Status:Needs review» Needs work

+++ b/core/modules/comment/comment.module
@@ -1841,7 +1841,7 @@ function comment_form($form, &$form_state, $comment) {
-  if (($comment_langcode == LANGUAGE_NONE) && variable_get('language_content_type_' . $node->type, 0)) {
+  if (($comment_langcode == LANGUAGE_NONE) && variable_get('node_type_language_' . $node->type, 0)) {

This needs an update path I think.

@tstoeckler: right. Can you or Clemens help with that? It would be superb! Thanks!

StatusFileSize
new15.22 KB
PASSED: [[SimpleTest]]: [MySQL] 34,062 pass(es).
[ View ]

Added node_update_8001 ... does the testbot test upgrades too?

Status:Needs work» Needs review

@clemens: Thanks! The update looks like what we need.

Testbot would not do update testing in itself. There is a good system for testing these updates though. Simpletest has a tests/upgrade/ directory, which does have an empty and a filled database dump (in PHP form) and respective tests. We started adding some language tests to the filled.test, which we can either keep expanding or refactor to more tests extending each other like Drupal 7 did. Anyway, its normal tests which import a dump and then run the test assertions on the updated dump.

Status:Needs review» Postponed

@clemens: I wrote this doc page based on your excellent questions: http://drupal.org/node/1429136. Also introduced easier to diff componentized language test files in #1357918: Missing update for language_default in language langcode update, so I think we should postpone this on that change, so we can introduce the upgrade path test seamlessly. I hope it will be in quick, so we can move on with this.

Again, excellent upgrade path, thanks.

Issue tags:+language-content

Tagging for the content handling leg of D8MI.

Status:Postponed» Needs work

Ok #1357918: Missing update for language_default in language langcode update landed, so now we can continue adding tests. IMHO the following should do it:

1. Add database dump data for a node type with language support in drupal-7.language.database.php.
2. Add the respective variable with its D7 name there too.
2. Extend the upgrade.language.test to (a) check for the new variable name (b) check the admin page for the content type that the config is still set (c) check the node/add/$type page for the type that it includes a language dropdown.

I think that should be well enough coverage :) With the dump now in its own PHP, it should be easy to diff and submit a patch. @clemens: do you have time to work on this?

Assigned:Xano» Unassigned

Not being worked on by Xano.

Issue tags:+Needs tests

Needs tests.

(just some notes to accomplish the generation of test data)

I'm running the following code as per http://drupal.org/node/1429136 script fit to my dev env

drush sql-drop --yes
drush site-install --yes standard
# Is malfunctioning as it enabled non-core modules from my sites/default/modules
drush pml --core --status="not installed" --pipe | xargs drush en --yes
# Why aren't the scripts in the d7 git checkout?
# Dumping for comparing purposes only
php ../../d8/www/core/scripts/dump-database-d7.sh > ../../d8/www/core/modules/simpletest/tests/upgrade/drupal-7.bare.database.php
php ../../d8/www/core/scripts/generate-d7-content.sh
# This fails due to ?
drush php-script ../../d8/www/core/modules/simpletest/tests/upgrade/drupal-7.language.database.php

Why aren't the scripts located in D7 branch?
Which fails as we need to add a language?

The script ../d8/www/core/modules/simpletest/tests/upgrade/drupal-7.language.database.php is assuming there are no blocks yet. But these are added due to the "Enable all core modules step" some question are:

1. What is our Drupal-7 base line?
- just a site-install?

2. Why should we enable _all_ core modules?
- we should only enable our targeted modules part of our particular test.

3. Adding the 'block' records it the result of enabling the local module so shouldn't we insert a record into the system table too?

I continue with the documentation page now.

Locale module is added to the system table but not installed in the filled db dump (gz). The language dump updates the sytem table entry and adds the schema elements from the module, so in effect installs it. I think it is likely going to be very hard to script data dump diffs for componentized dumps but if we dont componentize it, testing will be hard with intermingled target data. So i think a method with the least pain is to configure a D7 site the way you need and do a dump from that. Then copy parts of that that you need for the test. If idenitfying tose parts is nontrivial, you can make a dump before and after your config and diff those.

What I tried to accomplish today is a working D7 site to check for how to test the data at hand. I think being able to load the testset into a working D7 is key for people to develop upgrade tests.

When doing

git reset --hard
git pull
git apply
drush site-install --yes standard --account-name=admin --account-pass=drupal
drush php-script ../../d8/www/core/modules/simpletest/tests/upgrade/drupal-7.language.database.php

I get
Notice: Undefined property: stdClass::$language in locale_language_url_fallback() (line 311 of /Users/clemens/Sites/drupal/d7/www/includes/locale.inc).

As a logged in user @ http://drupal.d7/node#overlay=admin/config
Notice: Undefined property: stdClass::$language in drupal_lookup_path() (line 77 of /Users/clemens/Sites/drupal/d7/www/includes/path.inc).

Then @ http://drupal.d7/node#overlay=admin/modules
Notice: Undefined property: stdClass::$language in drupal_lookup_path() (line 77 of /Users/clemens/Sites/drupal/d7/www/includes/path.inc).
Notice: Undefined property: stdClass::$language in locale_language_url_fallback() (line 311 of /Users/clemens/Sites/drupal/d7/www/includes/locale.inc).

where 'Content translation' is not enabled. Do we need that too? (Enabling and clearing cache does not remove the notices)

So I'm stuck here puzzling where these warnings come from.

Following along this issues test (loading the filled and the languages db) I cannot load the filled db as the commands.

drush sql-drop --yes
drush php-script ../../d8/www/core/modules/simpletest/tests/upgrade/drupal-7.filled.database.php

fails misarably as drush cannot bootstrap on an empty drupal site.

So stuck again for now.

StatusFileSize
new6.22 KB

To make it more clear I have problems with the red arrow. How to load a dumpfile into a pristine D7 install?

upgrade-workflow.png

Studying

<?php
abstract class UpgradePathTestCase extends DrupalWebTestCase {
...
  protected function
setUp() {
...
   
// Load the database from the portable PHP dump.
    // The files can be gzipped.
   
foreach ($this->databaseDumpFiles as $file) {
      if (
substr($file, -3) == '.gz') {
       
$file = "compress.zlib://$file";
      }
      require
$file;
    }
?>

shows how it is done through the simpletest framework on a D8 codebase.

Am I missing something? Guess not as Gabor said somewhere (in ? http://drupal.org/node/1429136 ?) loading a dump file needs some documentation. But we are getting close :p

For today I stop trying to load the dumps and try to add some tests to this issue.

BTW Did you noticed the error in the image? D7 codebase cannot create it's own dumpfile ... which is said :(

Status:Needs work» Needs review
StatusFileSize
new20.46 KB
FAILED: [[SimpleTest]]: [MySQL] 34,264 pass(es), 9 fail(s), and 0 exception(s).
[ View ]

I only added a new content type 'locale' to drupal-7.language.database.php. Be back on this at the next D8MI sprint @ budapest.

It was trivial to dump but not to extract sensible data out of the dump. I left out i.e. the menu entries added by installing the locale module. But the process we are now getting into place is pretty neat.

Status:Needs review» Needs work

The last submitted patch, decouple-locale-from-node-540294-59.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new21.15 KB
PASSED: [[SimpleTest]]: [MySQL] 34,259 pass(es).
[ View ]

Weird as we missed(?) the poll.test. I should have ran all tests to find that. But that takes ages :(

The poll test were extended very recently, therefore the new issues. You can find that out in the git log. We did not miss them before, they were not there.

@Gábor Please shed some light on #58 and #59 if possible. Pointers to other issues are welcome too.

What I want to know is how to load my D7 install with test data to work on that.

I'm now checking out #1364798: Impossible to generate meaningful diffs of upgrade db dumps to see what that will bring.

@clemens: honestly I never tried loading one of those dump files in Drupal 7 (or the aggregates of them for that matter). I agree it would make the process even more complete, however I also believe that Drupal 8 will include so many changes that all aspects of the dump and the functionality it represents will be exercised at some point. I do believe that loading that dump is not a pre-requisite to extending the dumps or writing tests against the updated Drupal instances based on those dumps. At least the changes we make are very self-contained and limited. So I'm afraid I cannot help you with that part.

Status:Needs review» Needs work

Still needs work as it is missing tests.

Status:Needs work» Needs review
StatusFileSize
new17.47 KB
PASSED: [[SimpleTest]]: [MySQL] 34,333 pass(es).
[ View ]

Here are tests. I don't think we need the whole set of new content type and such that you included with your patch. I decided to not test the same setting three ways as documented in #51 but instead do cross-check with a content type that is not multilingual enabled. Since all that we do is to change one option's internal name, all we should test if the option still works after the update. So I've added that option for the existing article content type and a test as to whether languages show up on that (and that languages don't show up on the page content type).

I think this should be enough test coverage.

Would be good to get this in as soon as possible, so we can work on more interesting things like #258785: Provide more flexible settings for initial language on content types on the codesprint (which is dependent on this landing). (Did not change anything outside of the db dump and test, compared to your last patch).

Status:Needs review» Reviewed & tested by the community

The added tests are so short I hardly noticed them :)

Priority:Normal» Major

Marking major for itself as well as because other important issues depend on it like #258785: Provide more flexible settings for initial language on content types.

Status:Reviewed & tested by the community» Needs review

Sorry for jumping in late. Some minor remarks:

+++ b/core/modules/field/modules/text/text.test
@@ -434,7 +434,7 @@ class TextTranslationTestCase extends DrupalWebTestCase {
+    $edit = array('node_type_language' => 2);
+++ b/core/modules/path/path.test
@@ -260,7 +260,7 @@ class PathLanguageTestCase extends DrupalWebTestCase {
+    variable_set('node_type_language_page', 2);
+++ b/core/modules/poll/poll.test
@@ -843,7 +843,7 @@ class PollTranslateTestCase extends PollTestCase {
+    $edit['node_type_language'] = 2;
+++ b/core/modules/translation/translation.module
@@ -111,9 +111,10 @@ function translation_permission() {
+  $form['workflow']['node_type_language']['#options'] = array(t('Disabled'), t('Enabled'), TRANSLATION_ENABLED => t('Enabled, with translation'));
+++ b/core/modules/translation/translation.test
@@ -38,7 +38,7 @@ class TranslationTestCase extends DrupalWebTestCase {
+    $edit['node_type_language'] = 2;

While touching these, it would really make sense to replace those 2s with the explicit TRANSLATION_ENABLED constant for clarity.

That is, because 2 is not a valid value for Language module's original language select element.

+++ b/core/modules/node/content_types.inc
@@ -187,6 +187,14 @@ function node_type_form($form, &$form_state, $type = NULL) {
+      '#description' => t('Add a language selection field to the editing form, allowing you to select from one of the <a href="!languages">enabled languages</a>. If disabled, new posts are saved with the default language. Existing content will not be affected by changing this option.', array('!languages' => url('admin/config/regional/language'))),

Placeholder tokens for attributes should always be escaped; i.e., @languages-url

+++ b/core/modules/node/node.install
@@ -525,6 +526,22 @@ function node_update_8000() {
+ * Convert existing node type language settings to its new name.

"Rename node type language variable names."

+++ b/core/modules/node/node.pages.inc
@@ -181,6 +181,30 @@ function node_form($form, &$form_state, $node) {
+    $form['language'] = array(
+      '#type' => 'select',
...
+      '#empty_value' => LANGUAGE_NONE,
+      '#empty_option' => t('- None -'),

Specifying #empty_value is sufficient. The default option label for non-required selects is "- None -" already.

Also, by omitting the #empty_option, the label automatically adjusts itself based on whether the select is #required (and thus automatically uses a stronger/clearer "- Select -" if it is)

StatusFileSize
new17.48 KB
PASSED: [[SimpleTest]]: [MySQL] 34,334 pass(es).
[ View ]

Fixed all issues mentioned by @sun.

Status:Needs review» Reviewed & tested by the community

Thanks!

Status:Reviewed & tested by the community» Fixed

This all looks good and I agree that it is the right thing to do. Some nice little clean ups along the way too. Committed to 8.x.

#1074672: Allow language select to be rearranged inside node form is unblocked too. I will reroll the patch there.

Status:Fixed» Closed (fixed)
Issue tags:-Framework Initiative, -D8MI, -language-content

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