Part of #1971384: [META] Convert page callbacks to controllers

For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.

to test:

  1. install (in english)
  2. install language module and interface translation module
  3. visit admin/reports/translations
  4. add a couple languages
  5. visit admin/reports/translations again

also,

  1. try installing in another language, and then
  2. visit admin/reports/translations
  3. add a couple languages
  4. visit admin/reports/translations again
CommentFileSizeAuthor
#61 locale-1978926-61.patch23.59 KBtim.plunkett
#61 interdiff.txt1.65 KBtim.plunkett
#59 interdiff.txt8.02 KBkim.pepper
#59 1978926-local-translation-status-59.patch22.14 KBkim.pepper
#56 interdiff.txt11.05 KBkim.pepper
#56 1978926-local-translation-status-56.patch29.5 KBkim.pepper
#52 locale-update_status_form-1978926-48-52.txt1.07 KBvictor-shelepen
#52 locale-update_status_form-1978926-52.patch22.24 KBvictor-shelepen
#48 interdiff-47-48.txt887 bytesYesCT
#48 locale-update_status_form-1978926-48-just42withtestandremovedexportwrapper-withfix.patch22.31 KBYesCT
#48 interdiff-48-fix.txt741 bytesYesCT
#48 locale-update_status_form-1978926-48-just42withtestandremovedexportwrapper.patch22.33 KBYesCT
#48 interdiff-42-48-testandremovedexportwrapper.txt1.46 KBYesCT
#1 locale_translation_status_form_controller-1978926-1.patch7.76 KBPancho
#2 locale_translation_status_form_controller-1978926-2.patch14.93 KBPancho
#5 interdiff-1978926-2-5.txt11.84 KBPancho
#5 locale_translation_status_form_controller-1978926-5.patch19.14 KBPancho
#10 drupal-locale_translation_status_form_controller-1978926-10.patch19.42 KBh3rj4n
#11 drupal-locale_translation_status_form_controller-1978926-11.patch19.46 KBh3rj4n
#18 interdiff.txt16.98 KBdisasm
#18 drupal8.locale-module.1978926-18.patch22.26 KBdisasm
#22 interdiff.txt7.33 KBLuxian
#22 drupal.locale.update_form_status-1978926-22.patch22.39 KBLuxian
#26 locale_improved.patch13.15 KBneetu morwani
#28 locale-update_status_form-1978926-28.patch22.29 KBvictor-shelepen
#28 locale-update_status_form-1978926-22-28.txt814 bytesvictor-shelepen
#30 locale-update_status_form-1978926-30.patch22.26 KBvictor-shelepen
#30 locale-update_status_form-1978926-28-30.txt1.44 KBvictor-shelepen
#33 locale-update_status_form-1978926-33.patch22.26 KBvictor-shelepen
#33 locale-update_status_form-1978926-30-33.txt832 bytesvictor-shelepen
#34 locale-update_status_form-1978926-34.patch22.28 KBYesCT
#40 locale-update_status_form-1978926-34-40.txt1.38 KBvictor-shelepen
#40 locale-update_status_form-1978926-40.patch22.18 KBvictor-shelepen
#42 translationupdatestatus.png192.5 KBYesCT
#42 withpatchnolanguages.png138.3 KBYesCT
#42 interdiff-1978926-40-42.txt582 bytesYesCT
#42 locale-update_status_form-1978926-42.patch22.2 KBYesCT
#46 locale-update_status_form-1978926-46.patch21.52 KBvictor-shelepen
#46 locale-update_status_form-1978926-42-46.txt2.43 KBvictor-shelepen
#47 locale-update_status_form-1978926-47.patch22.87 KBvictor-shelepen
#47 locale-update_status_form-1978926-42-46.txt2.43 KBvictor-shelepen
#47 locale-update_status_form-1978926-46-47.txt1.81 KBvictor-shelepen
interdiff-42-48-testandremovedexportwrapper.patch1.46 KBYesCT
locale-update_status_form-1978926-48-just42withtestandremovedexportwrapper.patch22.33 KBYesCT
interdiff-48-fix.txt741 bytesYesCT
locale-update_status_form-1978926-48-just42withtestandremovedexportwrapper-withfix.patch22.31 KBYesCT
interdiff-47-48.txt887 bytesYesCT
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Pancho’s picture

Status: Active » Needs review
Issue tags: +D8MI, +WSCCI-conversion
FileSize
7.76 KB

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

Pancho’s picture

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

Pancho’s picture

Assigned: Unassigned » Pancho

Status: Needs review » Needs work

The last submitted patch, locale_translation_status_form_controller-1978926-2.patch, failed testing.

Pancho’s picture

Status: Needs work » Needs review
FileSize
11.84 KB
19.14 KB

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.

Status: Needs review » Needs work
Issue tags: -D8MI, -FormInterface, -WSCCI-conversion

The last submitted patch, locale_translation_status_form_controller-1978926-5.patch, failed testing.

Pancho’s picture

Status: Needs work » Needs review

#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].

Status: Needs review » Needs work
Issue tags: +D8MI, +FormInterface, +WSCCI-conversion

The last submitted patch, locale_translation_status_form_controller-1978926-5.patch, failed testing.

h3rj4n’s picture

Assigned: Pancho » h3rj4n

taking over the issue.

h3rj4n’s picture

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

h3rj4n’s picture

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

h3rj4n’s picture

Status: Needs work » Needs review

Forgot 'needs review' again :S

Status: Needs review » Needs work
h3rj4n’s picture

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.

jair’s picture

Issue tags: +Needs reroll
xjm’s picture

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.

Gábor Hojtsy’s picture

Issue tags: +language-ui
disasm’s picture

Assigned: h3rj4n » disasm
Status: Needs work » Needs review
FileSize
16.98 KB
22.26 KB

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!

disasm’s picture

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.

Status: Needs review » Needs work

The last submitted patch, drupal8.locale-module.1978926-18.patch, failed testing.

Luxian’s picture

Assigned: disasm » Luxian

@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:

So, I will assign this to me.

Luxian’s picture

Status: Needs work » Needs review
FileSize
7.33 KB
22.39 KB

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).

The last submitted patch, drupal.locale.update_form_status-1978926-22.patch, failed testing.

artem.ua’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 22: drupal.locale.update_form_status-1978926-22.patch, failed testing.

neetu morwani’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
13.15 KB

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

Status: Needs review » Needs work

The last submitted patch, 26: locale_improved.patch, failed testing.

victor-shelepen’s picture

Status: Needs work » Needs review
FileSize
22.29 KB
814 bytes

Status: Needs review » Needs work

The last submitted patch, 28: locale-update_status_form-1978926-28.patch, failed testing.

victor-shelepen’s picture

Assigned: Luxian » Unassigned
Status: Needs work » Needs review
FileSize
22.26 KB
1.44 KB

Status: Needs review » Needs work

The last submitted patch, 30: locale-update_status_form-1978926-30.patch, failed testing.

ehegedus’s picture

Issue tags: -Needs reroll

It still applies, tag removed.

victor-shelepen’s picture

Status: Needs work » Needs review
FileSize
22.26 KB
832 bytes
-        if (!isset($project_info->type)) {
+        if (empty($project_info->type)) {

if it is a class. It always have to have an attribute.

YesCT’s picture

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.

tim.plunkett’s picture

Status: Needs review » Needs work
+++ 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.

victor-shelepen’s picture

@tim.plunkett This is from
https://drupal.org/node/1978924. It is a pitty that it has been closed automaticaly due to no activity. We need an authorized person to solve such problems. There are a lot rtbc tasks.

victor-shelepen’s picture

Status: Needs work » Needs review

After this issue and the issue https://drupal.org/node/1978918 are commited we need remove a file that realizes \Drupal\locale\Form\LocaleForm because its unuseless. So we need commit at the first.

victor-shelepen’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 34: locale-update_status_form-1978926-34.patch, failed testing.

victor-shelepen’s picture

victor-shelepen’s picture

Status: Needs work » Needs review

I've corrected variable definitions.

YesCT’s picture

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

Status: Needs review » Needs work

The last submitted patch, 42: locale-update_status_form-1978926-42.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review
victor-shelepen’s picture

victor-shelepen’s picture

I've corrected the dunctionality according to snapshots #42, removed extra code according to #35. The test is defined for but it does not cover on fully Drupal\locale\Tests\LocaleUpdateTest::testUpdateCheckStatus()

I tried

// Case when contributed modules are absent.
$this->drupalGet('admin/reports/translations');
$this->assertText(t('Missing translations for one project'));

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

victor-shelepen’s picture

YesCT’s picture

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.

victor-shelepen’s picture

The patch from the comment 47 has passed tests succussfuly. So It was a temporary fail. We can close the task #2264027: Refactoring locale_translation_get_status. I hope this task is ready to be commited. :) Tasks appear these modify the logic. So we need finish the convertion and continue the logic modification later.

penyaskito’s picture

+++ 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.

victor-shelepen’s picture

It is modified according to #51

rodrigoaguilera’s picture

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

rodrigoaguilera’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 52: locale-update_status_form-1978926-52.patch, failed testing.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
29.5 KB
11.05 KB

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.

kim.pepper’s picture

tim.plunkett’s picture

+++ 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.

kim.pepper’s picture

Sorry! Got a bit excited with reformatting.

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

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Excellent, thanks!

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.65 KB
23.59 KB

Actually, too soon. Deleted some stale code.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

Gábor Hojtsy’s picture

Issue tags: +sprint

Yay, thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 61: locale-1978926-61.patch, failed testing.

Status: Needs work » Needs review
tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

EntityCrudHookTest has been randomly failing all day :(

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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.

  • alexpott committed 8b45d21 on 8.x
    Issue #1978926 by likin, YesCT, Pancho, kim.pepper, h3rj4n, tim.plunkett...
Gábor Hojtsy’s picture

Issue tags: -sprint

Yay, thanks!

Status: Fixed » Closed (fixed)

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