PHP Notice for custom language with markup in name

#2 looks good to me. Thanks for your work on this patch. Note that the Drupal 8.x patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). When the patch has been rerolled, please set the issue back to "Needs Review."

Tagging as novice for the task of rerolling the Drupal 8.x patch.

If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

Updated for #5 - D8 /core

Here's the failed tests:

Failed to set field native to <br />gnVE5LnNYSZoJCon	Other	locale.test	72	LocaleConfigurationTest->testLanguageConfiguration()	
Found the requested form fields at admin/config/regional/language/add	Other	locale.test	72	LocaleConfigurationTest->testLanguageConfiguration()	
Markup in language name not allowed.	Other	locale.test	73	LocaleConfigurationTest->testLanguageConfiguration()	
Markup in native language name not allowed.	Other	locale.test	74	LocaleConfigurationTest->testLanguageConfiguration()	

#13: Sorry about that; the test-only patch had the "wrong" fail so I retested it. It's fixed now in either case. :)

#9: locale-notice-custom-language-1295048-9.patch queued for re-testing.

So, the code for adding language simply moved to another file. Reuploading patches, first one will fail (due to a notice because wrong form is passed), second one should pass.

Meh, moving the test to the bottom of all tests so it doesn't interfere with the flow there is now.

The patches from #18 no longer applied. Here they are again.

#19: custom_language_validation-1295048-19.patch queued for re-testing.

The patch didn't apply anymore, but let's reroll it (and also remove the t() from the assertion message). Looks RTBC to me once that's taken care of.

Rerolls and removes assertion message t().

Looks like t('The English language has been removed.') was missed in #23. There should be no t() around this assert message.

Oops, good catch Lars. Fixed....

I just noticed that the t() in the drupalPost call was removed in #23. That should be put back. Only t() around assert messages should be removed. Sorry for not noticing it the first time.

The patch in #27 looks like a re-roll of the fail patch in #19. I dont see the "fix"

diff --git a/modules/locale/locale.admin.inc b/modules/locale/locale.admin.inc
index e17ed7c..623f1a4 100644
--- a/modules/locale/locale.admin.inc
+++ b/modules/locale/locale.admin.incundefined
@@ -331,7 +331,7 @@ function locale_languages_add_custom_form_validate($form, &$form_state) {
   if ($form_state['values']['predefined_langcode'] == 'custom') {
     $langcode = $form_state['values']['langcode'];
     // Reuse the editing form validation routine if we add a custom language.
-    locale_languages_edit_form_validate($form, $form_state);
+    locale_languages_edit_form_validate($form['custom_language'], $form_state);
 
     $languages = language_list();

So I think the patch custom_language_validation-1295048-27.patch needs to be renamed custom_language_validation-1295048-XX-fail.patch to indicate this is the patch without the fix, but with the test that is supposed to detect the problem, and that it's expected the testbot will show it fails.

And another patch needs to be added with the fix and the test and that can be named normally, that is the one that should pass. See #2 for an example, of one patch that fails (the one with just the test) and one that passes (the patch with the fix and the test).

Posting interdiffs is a bit tricky, I'd just post an interdiff between the normal patch (with the fix and the test) and the patch in #19. Then maybe post a diff between the regular patch and the fail patch (that will show just what the fix was).

Even after that is done though, I'm worried. It seems like the fail patch is going to pass. So that means either that the fix is not the fix, or the test is not testing what it should... But lets worry about that after seeing the fail and the normal patch first.

Ok so I think I see what happened here. Looks like the "fix' YesCT mentioned is missing is actually already in core. So this leads us back to strictly testing.

Attached I have the same patch as #27. Reverting the fix that was made at some point in core (not sure how to use Git blame to see when that happened) and attach that patch as a -FAIL and then the interdiff showing what I changed to create the fail.

We'll see if these fail/pass test bot as expected.

Ok so I found the issue where this was committed to 8.x:

#1751082: Undefined index: langcode in language_admin_edit_form_validate

Looks like this task is a duplicate of that.

@kbasarab, so the task is just to add the test coverage then? Or are there existing tests in core that also duplicate what's being added here? (If the latter, we can close this issue as a duplicate.)

xjm: Test coverage was added in #1751082: Undefined index: langcode in language_admin_edit_form_validate also so I believe this is a full duplicate of that case.

Test for this specific case from #21 in http://drupal.org/node/1751082#comment-6423144

+    // Test validation of invalid values.
+    $edit = array(
+      'predefined_langcode' => 'custom',
+      'langcode' => 'white space',
+      'name' => '<strong>evil markup</strong>',
+      'direction' => LANGUAGE_LTR,
+    );

Marking as duplicate. Feel free to reopen though if you see something I'm missing.