Convert locale_translation_status_form to a Controller

Quite straightforward, but I didn't get to testing it that much.
So let's see what the bot says and then please review.

Sorry, the first patch was missing our new class. No other changes.
Here we go again:

New patch:

• Use 'locale_translation_status_form' as FormID, so the form is correctly recognized for theming
• Roll in _locale_translation_status_debug_info() as TranslationStatusForm::createInfoString()
• Swap out the quite complex data shuffling to TranslationStatusForm::prepareUpdateData()
• Remove the unused $projects variable with it's call to locale_translation_get_projects()
• Inject the module handler.
• Some minor optimizations.

Now let's see what the testbot says.
In any case still open for further improvements, optimizations and suggestions.

#5: locale_translation_status_form_controller-1978926-5.patch queued for re-testing.

Previous result: FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].

taking over the issue.

This patch is definitely going to fail but the old one couldn't be applied, so just created a new one.

Ok, I hope this one doesn't fail. Should all be fixed.

Forgot 'needs review' again :S

quick update: The locale module requires the update module to be enabled. Otherwise this module doesn't work. It's about this function locale_translation_build_projects. It works but won't give you the functionality you want.

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.

massive reroll. Built on FormBase. Injecting KeyVaueStoreInterface to get rid of Drupal::state(). Compared functions being replaced with methods and made changes accordingly. Not throwing any errors locally. Lets see what testbot thinks!

Noting that if anything looks odd in the interdiff (like removing locale_translation_manual_status), that isn't really happening. It was a screw-up I made in the reroll somehow that I fixed in my commit (in the reroll I ended up with two locale_translation_manual_status functions, one of which contained the stuff I really wanted to remove.

@disasm Your patch looks really good and I would like to continue your work and try to get all the Locale forms converted. The other forms already have a working patch:

• #1978918: Convert locale_translate_import_form to a Controller
• #1978924: Convert locale_translate_export_form to a Controller

So, I will assign this to me.

This is the patch from #1978926-18: Convert locale_translation_status_form to a Controller re-rolled with conflicts resolved. I have 3 tests failing on my local. I'm not sure how much time I will have in the next 2 days, so if anyone wants to work on it can assign it to himself.

Interdiff might be wrong (not sure if I do them right).

22: drupal.locale.update_form_status-1978926-22.patch queued for re-testing.

Last patch re-rolled. Now the patch applies successfully.

It still applies, tag removed.

was going to review this, but it did not apply.
rerolled.
easy, auto 3-way merge.
no conflicts. (no interdiff)

I'll review this now.

+++ b/core/modules/locale/lib/Drupal/locale/Form/LocaleForm.php
@@ -31,4 +31,14 @@ public function status() {
     return \Drupal::formBuilder()->getForm('locale_translation_status_form');
   }
...
+   * Wraps locale_translate_export_form().
+   *
+   * @todo Remove locale_translate_export_form().
+   */
+  public function export() {
+    module_load_include('bulk.inc', 'locale');
+    return drupal_get_form('locale_translate_export_form');
+  }
+

Bad merge? It's supposed to be removing the other one.

1.

+++ b/core/modules/locale/lib/Drupal/locale/Form/TranslationStatusForm.php
@@ -0,0 +1,298 @@
+ * {@inheritdoc}
+ */
+class TranslationStatusForm extends FormBase {
+  /**

we dont use inheritdoc on classes.

fixing that.

--------notes
at first I tried something like find . -name "*.php" | xargs awk '/inheritdoc/,/function/' > /tmp/results.txt
but could not get that to work, so instead did a bit of manual work and did
ag inheritdoc -A 2 > /tmp/results.txt
then
ag class -A 2 /tmp/results.txt
and looked through those results.
#2250165: Replace fake mocks with actual OpenDialogCommand stubs in AjaxCommandsTest opened to take care of that.
--------end notes

2.
also fixing nit of missing newline after open { of the class.
"Leave an empty line between start of class/interface definition and property/method definition:"
https://drupal.org/node/608152#indenting

3.
either this issue, or a separate issue needs to add tests for this form/table.

because with this patch, the table at admin/reports/translations
should show

but it shows

42: locale-update_status_form-1978926-42.patch queued for re-testing.

I haven't checked for all the forms this effects and tried them all manually. We might still want to do that.

But there was a line in #47 I didn't understand why was added.
So, what I did was take 42, and add the test (from 47, but only part of 47) and take out the export wrapper.
This had 1 fail (agreeing with 42 3.) Good! Uploading that to show the test works (fails).
Then I added the fix from #46.

I talked with @likin in irc, but I still dont understand #46

But it crashes tests because because this page caches some settings.

Maybe #2264027: Refactoring locale_translation_get_status will clarify?

taking the needs tests tag off since the only problem spotted so far, has had a assert added for it.

We might want to keep an eye on this, as in the future, when adding a language, core should be able to get the translation automatically. But we dont need to worry about that here, because now we have a test that will fail when that does begin to work, and that issue can update this test.

+++ b/core/modules/locale/lib/Drupal/locale/Form/TranslationStatusForm.php
@@ -0,0 +1,299 @@
+    $this->moduleHandler->loadInclude('locale', 'translation.inc');
+    $this->moduleHandler->loadInclude('locale', 'compare.inc');

Are these included in any other forms? Should we make those services? I guess old-style includes are not good, but this can be done in follow-up.

Otherwise, looks good. I will proceed with manual testing if noone beats me to do it.

I've done the manual testing according to the issue summary and everything looks alright to me.

I've re-rolled for PSR4 and made some code-style clean ups.

This issue is pretty old, so there may be a few other cleanups needed.

Tests are passing locally.

56: 1978926-local-translation-status-56.patch queued for re-testing.

+++ b/core/modules/locale/src/Tests/LocaleUpdateTest.php
@@ -126,12 +143,12 @@ function testUpdateCheckStatus() {
-  function testUpdateImportSourceRemote() {
+  public function testUpdateImportSourceRemote() {
     $config = \Drupal::config('locale.settings');
 
     // Build the test environment.
     $this->setTranslationFiles();
-    $this-> setCurrentTranslations();
+    $this->setCurrentTranslations();
     $config->set('translation.default_filename', '%project-%version.%language._po');
 
     // Set the update conditions for this test.
@@ -188,12 +205,12 @@ function testUpdateImportSourceRemote() {

@@ -188,12 +205,12 @@ function testUpdateImportSourceRemote() {
    *  - Source: local files only
    *  - Import overwrite: all existing translations
    */
-  function testUpdateImportSourceLocal() {
+  public function testUpdateImportSourceLocal() {
     $config = \Drupal::config('locale.settings');
 
     // Build the test environment.
     $this->setTranslationFiles();
-    $this-> setCurrentTranslations();
+    $this->setCurrentTranslations();
     $config->set('translation.default_filename', '%project-%version.%language._po');
 
     // Set the update conditions for this test.
@@ -242,12 +259,12 @@ function testUpdateImportSourceLocal() {

@@ -242,12 +259,12 @@ function testUpdateImportSourceLocal() {
    *  - Source: remote and local files
    *  - Import overwrite: only overwrite non-customized translations
    */
-  function testUpdateImportModeNonCustomized() {
+  public function testUpdateImportModeNonCustomized() {
     $config = \Drupal::config('locale.settings');
 
     // Build the test environment.
     $this->setTranslationFiles();
-    $this-> setCurrentTranslations();
+    $this->setCurrentTranslations();
     $config->set('translation.default_filename', '%project-%version.%language._po');
 
     // Set the test conditions.
@@ -278,12 +295,12 @@ function testUpdateImportModeNonCustomized() {

@@ -278,12 +295,12 @@ function testUpdateImportModeNonCustomized() {
    *  - Source: remote and local files
    *  - Import overwrite: don't overwrite any existing translation
    */
-  function testUpdateImportModeNone() {
+  public function testUpdateImportModeNone() {
     $config = \Drupal::config('locale.settings');
 
     // Build the test environment.
     $this->setTranslationFiles();
-    $this-> setCurrentTranslations();
+    $this->setCurrentTranslations();
     $config->set('translation.default_filename', '%project-%version.%language._po');
 
     // Set the test conditions.
@@ -310,7 +327,7 @@ function testUpdateImportModeNone() {

@@ -310,7 +327,7 @@ function testUpdateImportModeNone() {
   /**
    * Tests automatic translation import when a module is enabled.
    */
-  function testEnableUninstallModule() {
+  public function testEnableUninstallModule() {
     // Make the hidden test modules look like a normal custom module.
     \Drupal::state()->set('locale.test_system_info_alter', TRUE);
 
@@ -325,7 +342,11 @@ function testEnableUninstallModule() {

@@ -325,7 +342,11 @@ function testEnableUninstallModule() {
 
     // Check if translations have been imported.
     $this->assertRaw(t('One translation file imported. %number translations were added, %update translations were updated and %delete translations were removed.',
-      array('%number' => 7, '%update' => 0, '%delete' => 0)), 'One translation file imported.');
+      array(
+        '%number' => 7,
+        '%update' => 0,
+        '%delete' => 0,
+      )), 'One translation file imported.');
     $this->assertTranslation('Tuesday', 'Dienstag', 'de');
 
     $edit = array(
@@ -348,7 +369,7 @@ function testEnableUninstallModule() {

@@ -348,7 +369,7 @@ function testEnableUninstallModule() {
    * enabled modules and will import them. When a language is removed the system
    * will remove all translations of that langugue from the database.
    */
-  function testEnableLanguage() {
+  public function testEnableLanguage() {
     // Make the hidden test modules look like a normal custom module.
     \Drupal::state()->set('locale.test_system_info_alter', TRUE);
 
@@ -370,7 +391,11 @@ function testEnableLanguage() {

@@ -370,7 +391,11 @@ function testEnableLanguage() {
 
     // Check if the right number of translations are added.
     $this->assertRaw(t('One translation file imported. %number translations were added, %update translations were updated and %delete translations were removed.',
-      array('%number' => 8, '%update' => 0, '%delete' => 0)), 'One language added.');
+      array(
+        '%number' => 8,
+        '%update' => 0,
+        '%delete' => 0,
+      )), 'One language added.');
     $this->assertTranslation('Extraday', 'extra dag', 'nl');
 
     // Check if the language data is added to the database.
@@ -392,7 +417,7 @@ function testEnableLanguage() {

@@ -392,7 +417,7 @@ function testEnableLanguage() {
   /**
    * Tests automatic translation import when a custom langauge is added.
    */
-  function testEnableCustomLanguage() {
+  public function testEnableCustomLanguage() {

Is *any* of this in scope?
Otherwise the rest looks good.

Sorry! Got a bit excited with reformatting.

This reverts code style cleanups @tim.plunkett mentions in #58

Excellent, thanks!

Actually, too soon. Deleted some stale code.

Back to RTBC

Yay, thanks!

EntityCrudHookTest has been randomly failing all day :(

Committed 8b45d21 and pushed to 8.x. Thanks!

diff --git a/core/modules/locale/locale.pages.inc b/core/modules/locale/locale.pages.inc
index 68870ee..82506d7 100644
--- a/core/modules/locale/locale.pages.inc
+++ b/core/modules/locale/locale.pages.inc
@@ -5,7 +5,6 @@
  * Interface translation summary, editing and deletion user interfaces.
  */
 
-use Drupal\Component\Utility\String;
 use Drupal\Core\Render\Element;
 use Drupal\locale\SourceString;
 use Drupal\locale\TranslationString;
@@ -92,6 +91,8 @@ function theme_locale_translate_edit_form_strings($variables) {
  *   An associative array containing:
  *   - updates: The projects which have updates.
  *   - not_found: The projects which updates are not found.
+ *
+ * @see \Drupal\locale\Form\TranslationStatusForm
  */
 function template_preprocess_locale_translation_update_info(&$variables) {
   $details = array();
@@ -148,6 +149,8 @@ function template_preprocess_locale_translation_update_info(&$variables) {
  * @param $variables
  *   An associative array containing:
  *   - last: The timestamp when the site last checked for available updates.
+ *
+ * @see \Drupal\locale\Form\TranslationStatusForm
  */
 function template_preprocess_locale_translation_last_check(&$variables) {
   $last = $variables['last'];
diff --git a/core/modules/locale/src/Form/TranslationStatusForm.php b/core/modules/locale/src/Form/TranslationStatusForm.php
index 74bff5c..a1c7e34 100644
--- a/core/modules/locale/src/Form/TranslationStatusForm.php
+++ b/core/modules/locale/src/Form/TranslationStatusForm.php
@@ -267,7 +267,7 @@ protected function createInfoString($project_info) {
   public function validateForm(array &$form, array &$form_state) {
     // Check if a language has been selected. 'tableselect' doesn't.
     if (!array_filter($form_state['values']['langcodes'])) {
-      form_set_error('', $this->t('Select a language to update.'));
+      $this->setFormError('', $this->t('Select a language to update.'));
     }
   }
 

Minor fixes on commit.

Yay, thanks!