Undefined index: langcode in language_admin_edit_form_validate

Just a small thing:

+++ b/core/modules/language/language.admin.incundefined
@@ -313,7 +313,7 @@ function language_admin_add_custom_form_validate($form, &$form_state) {
+    language_admin_edit_form_validate($form['custom_language'], $form_state);

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageCustomLanguageConfigurationTest.phpundefined
@@ -0,0 +1,91 @@
+    $this->assertText('Language code field is required.');
+    $this->assertText('Language name field is required.');

Shouldn't we use t() here? As I've seen, t() is usually used to check the messages in the core tests.

Looks good, found just some really small things after a closer look:

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageCustomLanguageConfigurationTest.phpundefined
@@ -0,0 +1,91 @@
+    $this->assertText(t('!name field is required.', array('!name' => 'Language code')));
+    $this->assertText(t('!name field is required.', array('!name' => 'Language name')));

This is better, but I actually think that the 100% correct version would be to use the t() also on the 'Language code', 'Language name' etc... This is because the core uses the #title attribute, which in our case is t('Language code'), etc...

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageCustomLanguageConfigurationTest.phpundefined
@@ -0,0 +1,91 @@
+    $this->assertRaw(t('%field may only contain characters a-z, underscores, or hyphens.', array('%field' => 'Language code')));
+    $this->assertRaw(t('%field cannot contain any markup.', array('%field' => 'Language name')));

The same as above.

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageCustomLanguageConfigurationTest.phpundefined
@@ -0,0 +1,91 @@
+    $this->assertRaw(t(
+      'The language %language (%langcode) already exists.',
+      array('%language' => t($edit['name']), '%langcode' => t($edit['langcode']))

I think here we do not have to use the t() on $edit['name'] and $edit['langcode'].

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageCustomLanguageConfigurationTest.phpundefined
@@ -0,0 +1,91 @@
+    $this->assertRaw(t('%field may only contain characters a-z, underscores, or hyphens.', array('%field' => 'Language code')));

I think you missed this one :)

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageCustomLanguageConfigurationTest.phpundefined
@@ -0,0 +1,91 @@
+      'The language %language has been created and can now be used.',
+      array('%language' => t($edit['name']))
+    ));

and this one. Here, we should not have t().

Otherwise, everything looks good to be, after uploading the new patch, and the testbot is green, can be RTBC.

Looks good now to me, if green tests, it is RTBC from my side.

I review a patch. I check "User interface translation" - "Import" page (admin/config/regional/translate/import) because you can add new language there. I saw you can not add custom language. So, I can't reproduce this bug there.

The patch looks good for me too.

A few minor things, but enough of them that I don't really feel comfortable editing prior to commit.

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageCustomLanguageConfigurationTest.phpundefined
@@ -0,0 +1,91 @@
+    // User to add and remove language.

"Create user with permissions to add and remove languages."

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageCustomLanguageConfigurationTest.phpundefined
@@ -0,0 +1,91 @@
+    // Test validation on missing values.

That second comment should be moved to just above the assertText() stuff.

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageCustomLanguageConfigurationTest.phpundefined
@@ -0,0 +1,91 @@
+    $this->assertEqual($this->getUrl(), url('admin/config/regional/language/add', array('absolute' => TRUE)), t('Correct page redirection.'));
...
+    $this->assertEqual($this->getUrl(), url('admin/config/regional/language/add', array('absolute' => TRUE)), t('Correct page redirection.'));
...
+    $this->assertEqual($this->getUrl(), url('admin/config/regional/language', array('absolute' => TRUE)), t('Correct page redirection.'));
...
+    $this->assertEqual($this->getUrl(), url('admin/config/regional/language/add', array('absolute' => TRUE)), t('Correct page redirection.'));

Assertion messages should not be wrapped in t(). It should just be 'Correct page redirection' rather than t('Correct page redirection')

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageCustomLanguageConfigurationTest.phpundefined
@@ -0,0 +1,91 @@
+    // Test validation off invalid values.
...
+    // Test validation off existing language values.

This needs to be changed to "of" rather than "off"

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageCustomLanguageConfigurationTest.phpundefined
@@ -0,0 +1,91 @@
+    );
+    // Add the language the first time.

(nit-pick) We should add newline between these.

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageCustomLanguageConfigurationTest.phpundefined
@@ -0,0 +1,91 @@
+    // Add the language a second time.

"...and confirm that this is not allowed." or something, to provide some context as to why you would want to do that. :)

On visual inspection this looks like what I asked for, thanks! There are a couple of minor things like whitespace that I can clean-up on commit.

Moving to RTBC and we'll see what the testbot thinks.

Those radio buttons from what I remember it used to not have a default value for a new language, but I think some patch that was committed recently has changed that. If you check the _language_admin_common_controls() function, you can see that, if a language is not sent as a parameter, then a default Language object is created. The Language class has as the default parameter for the direction the value LANGUAGE_LTR. The radio buttons will (or at least should) always have a default value, and probably this is why you do not get the error message anymore on the page.

So, maybe now it would make sense to test that the selected radio button by default is LANGUAGE_LTR (or even better, to check that the selected radio button is the default direction parameter of the Language class).

After that, and the test bot returns green, can be set to RTBC again.

Well, yes, maybe you're right, probably the test class makes more tests than the ones strictly related to the issue. But, from my point of view, this is not a harmful thing, and is better than having less tests than it should. So, from my side, can be RTBC.

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageCustomLanguageConfigurationTest.phpundefined
@@ -0,0 +1,94 @@
+
+  /**
+   * Information about the test.
+   */
+  public static function getInfo() {
+    return array(
+      'name' => 'Custom Language configuration',
+      'description' => 'Adds and configures custom languages.',
+      'group' => 'Language',
+    );

No need to document the getInfo() method. Otherwise this looks fine to me. The tests for page redirection do look a bit odd but I agree they're not doing any harm.

Committed and pushed to 8.x. Thanks!

Removing from sprint. I don't think this should get a changelog entry or change notice, it is just fixing a bug introduced in D8 :) Thanks all though!