Convert locale_translate_import_form to a Controller

The conversion patch, tested to some degree, now let's see if our testbot agrees.

Ok, test failed because I made the status message consistent to language_admin_add_form_submit().
Adapting the test, also removing an obsolete '@see' tag and a redundant 'use' statement in locale.bulk.inc
Should be fine now.

[edit:] Please ignore wrong interdiff.

Here's the correct interdiff for #3.

Nice, it basically seems to work. Now I'm open for improvements.
Adding tags.

+++ b/core/modules/locale/lib/Drupal/locale/Form/ImportForm.phpundefined
@@ -0,0 +1,174 @@
+ * {@inheritdoc}

This feels wrong.

+++ b/core/modules/locale/lib/Drupal/locale/Form/ImportForm.phpundefined
@@ -0,0 +1,174 @@
+  /**
+   * {@inheritdoc}
+   */
+  public static function create(ContainerInterface $container) {
+    return new static();
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function __construct() {
+  }

No need for that so far.

#3: locale_translate_import_form_controller-1978918-3.patch queued for re-testing.

Uhh, on retest there suddenly seem to be lots of errors.
I'm gonna check this again today.

Thanks!

Thanks for your work on this issue! Please see #1971384-43: [META] Convert page callbacks to controllers for an update on the routing system conversion process.

Probably won't be RTBC by the 14th, but we'll need a FormBase for this anyways, so here's a go at it. Yes, I know moduleHandler loading files in classes is hacky.

#17: drupal8.locale-module.1978918-17.patch queued for re-testing.

Adding missing use flag. Manual testing of import/export worked fine! This might actually make it...

I'm perplexed by the number of failures here. I added the German Language, made some changes, exported, un-did the changes I made, imported and all my changes were there.

Some minor issues.

1. +++ w/core/modules/locale/lib/Drupal/locale/Form/ImportForm.php
   @@ -0,0 +1,179 @@
   +   * Form constructor for the translation import screen.
   

   @param missing from doc. we can use @inheritdoc here.

2. +++ w/core/modules/locale/lib/Drupal/locale/Form/ImportForm.php
   @@ -0,0 +1,179 @@
   +    form_load_include($form_state, 'inc', 'language', 'language.admin');
   

   Can we create a follow up to convert this to OO if there is not already one and write a @todo with issue link here.

3. +++ w/core/modules/locale/lib/Drupal/locale/Form/ImportForm.php
   @@ -0,0 +1,179 @@
   +        'js' => array(
   +          drupal_get_path('module', 'locale') . '/locale.bulk.js' => array(),
   +        ),
   

   Are we planning to make it a library? We should make it a library and add @todo here with issue link.

@disasm It's been awhile since you work on this. I'm at the Prague sprint and going to work on this now.

I think this is part 2 from the process described in: A faster process for Drupal 8 routing conversions

Did the following:

• re-base against lates 8.x
• fixed the errors (submitForm function used 'langcode' instead of 'id' and entity->save() failed)
• clean-up in the LocaleForm (remove function import()) - forgot to put it in diff
• updated a test because message text has changed (easy to spot in interdiff)

I'm not sure if interdiff is correct. All local test pass on my machine, hope test bot will confirm this.

Forgot to set status.

Sorry for the empty patch. This is the right one

Tags

1. +++ b/core/modules/locale/lib/Drupal/locale/Form/ImportForm.php
   @@ -0,0 +1,179 @@
   +use Drupal\Core\Form\FormBase;
   +use Drupal\Core\Language\Language;
   +use Drupal\Core\DependencyInjection\ContainerInjectionInterface;
   +use Drupal\Core\Extension\ModuleHandlerInterface;
   +use Symfony\Component\DependencyInjection\ContainerInterface;
   +
   

   Order these alphabetically.

2. +++ b/core/modules/locale/lib/Drupal/locale/Form/ImportForm.php
   @@ -0,0 +1,179 @@
   +class ImportForm extends FormBase implements ContainerInjectionInterface {
   +  /**
   +   * Module Handler Service.
   

   Leave an empty line between start of class definition and property definition.

3. +++ b/core/modules/locale/lib/Drupal/locale/Form/ImportForm.php
   @@ -0,0 +1,179 @@
   +   * @param $module_handler \Drupal\Core\Extension\ModuleHandlerInterface
   +   */
   

   Write the type first, then the variable name.

4. +++ b/core/modules/locale/lib/Drupal/locale/Form/ImportForm.php
   @@ -0,0 +1,179 @@
   +  /**
   +   * Form constructor for the translation import screen.
   +   *
   +   * @return
   +   *   A form array representing the currently disabled modules.
   +   *
   +   * @ingroup forms
   +   */
   

   I don't think it is necessary to document the return value for form builders. Everybody is supposed to know what this returns.

5. +++ b/core/modules/locale/lib/Drupal/locale/Form/ImportForm.php
   @@ -0,0 +1,179 @@
   +  }
   +}
   

   Put a blank line before the brace that closes a class.

For the rest this looks fine to me. There are a lot of function calls left to procedural code but for none of them I know whether there are better alternatives available already.

-- wrong: please delete --

Took me some time, but now the patch is re-rolled and changes from #36 included. I had to do updates to make sure we don't lose the changes done to that form meanwhile (can be seen in interdiff).

Re-roll

41: drupal.locale.update_import_form-1978918-41.patch queued for re-testing.

1. +++ b/core/modules/locale/lib/Drupal/locale/Form/ImportForm.php
   @@ -0,0 +1,184 @@
   +    return 'locale_translate_import';
   

   should be 'locale_translate_import_form' also remove old one

2. +++ b/core/modules/locale/lib/Drupal/locale/Form/ImportForm.php
   @@ -0,0 +1,184 @@
   +    \Drupal::languageManager()->reset();
   

   should be injected like module handler

3. +++ b/core/modules/locale/lib/Drupal/locale/Form/ImportForm.php
   @@ -0,0 +1,184 @@
   +    $languages = language_list();
   

   deprecated, use getLanguages()

4. +++ b/core/modules/locale/lib/Drupal/locale/Form/ImportForm.php
   @@ -0,0 +1,184 @@
   +      '#description' => array(
   +        '#type' => 'file_upload_help',
   

   #theme
   also better to copy/paste a render from original form

5. +++ b/core/modules/locale/lib/Drupal/locale/Form/ImportForm.php
   @@ -0,0 +1,184 @@
   +    return;
   

   is not needed

1. +++ b/core/modules/locale/lib/Drupal/locale/Form/ImportForm.php
   @@ -0,0 +1,191 @@
   +    return 'locale_translate_import_form';
   
   +++ b/core/modules/locale/locale.bulk.inc
   @@ -99,13 +99,80 @@ function locale_translate_import_form($form, &$form_state) {
   +/**
   

   old form should be removed

2. +++ b/core/modules/locale/lib/Drupal/locale/Form/ImportForm.php
   @@ -0,0 +1,191 @@
   +  }
   +}
   

   missing extra blank line at the end of the class

3. +++ b/core/modules/locale/locale.bulk.inc
   @@ -99,13 +99,80 @@ function locale_translate_import_form($form, &$form_state) {
   +    //'#value' => t('Import'),
   

   ?!

I'm sad this does not apply anymore :(

0 bytes

Please upload a patch

1. +++ b/core/modules/locale/lib/Drupal/locale/Form/ImportForm.php
   @@ -0,0 +1,191 @@
   +   * @var \Drupal\Core\Language\LanguageManager
   

   we do have a interface for the language manager.

2. +++ b/core/modules/locale/lib/Drupal/locale/Form/ImportForm.php
   @@ -0,0 +1,191 @@
   +   *
   +   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
   +   */
   +  public function __construct(ModuleHandlerInterface $module_handler, LanguageManager $language_manager) {
   

   Missing docs + language manager interface

3. +++ b/core/modules/locale/lib/Drupal/locale/Form/ImportForm.php
   @@ -0,0 +1,191 @@
   +    $this->languageManager->reset();
   

   it would be kind of cool to have a one liner why we always reset the language manager here.

4. +++ b/core/modules/locale/lib/Drupal/locale/Form/ImportForm.php
   @@ -0,0 +1,191 @@
   +    form_load_include($form_state, 'inc', 'language', 'language.admin');
   ...
   +      $language_options = language_admin_predefined_list();
   

   Do we have a follow up to convert this into a separate service?

5. +++ b/core/modules/locale/lib/Drupal/locale/Form/ImportForm.php
   @@ -0,0 +1,191 @@
   +      '#attached' => array(
   +        'js' => array(
   +          drupal_get_path('module', 'locale') . '/locale.bulk.js' => array(),
   +        ),
   

   Isn't that js file already a library?

6. +++ b/core/modules/locale/lib/Drupal/locale/Form/ImportForm.php
   @@ -0,0 +1,191 @@
   +      $language = language_load($form_state['values']['langcode']);
   

   language load is languageManager->getLanguage

7. +++ b/core/modules/locale/lib/Drupal/locale/Form/ImportForm.php
   @@ -0,0 +1,191 @@
   +        drupal_set_message(t('The language %language has been created.', array('%language' => t($language->name))));
   

   we can use $this->t() here.

Thank you. All additional function calls can be done later.

61: 1978918-locale_import_form-61.patch queued for re-testing.

Back to RTBC, the bot will turn back to need works if it fails. There has been some error with them, not related with the patch.

Great

@likin w00t, thanks! +1 RTBC.
Removed "Needs reroll" tag.

Committed 4b172c7 and pushed to 8.x. Thanks!

+++ b/core/modules/locale/lib/Drupal/locale/Form/ImportForm.php
@@ -0,0 +1,197 @@
+    form_load_include($form_state, 'inc', 'language', 'language.admin');
...
+      $language_options = language_admin_predefined_list();
...
+        $this->t('Languages not yet added') => language_admin_predefined_list()

Can we get an follow up to move language_admin_predefined_list() - this was mentioned in #58

Created #2254495: Move language_admin_predefined_list to ConfigurableLanguageManager