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.

Remaining tasks

None.

Files: 
CommentFileSizeAuthor
#131 language-1978916-131.patch8.59 KBgoogletorp
FAILED: [[SimpleTest]]: [MySQL] 58,562 pass(es), 0 fail(s), and 4 exception(s).
[ View ]
#131 interdiff.txt5.42 KBgoogletorp
#128 interdiff-patches.124-128.interdiff.txt1.18 KBpenyaskito
#128 language-1978916-128.patch4.23 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] 58,428 pass(es), 25 fail(s), and 635 exception(s).
[ View ]
#124 language-1978916-124.patch4.19 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch language-1978916-124.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#111 interdiff.txt6.83 KBtim.plunkett
#111 locale-1978916-111.patch75.19 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,714 pass(es).
[ View ]
#109 locale-1978916-109.patch74.97 KBLetharion
PASSED: [[SimpleTest]]: [MySQL] 58,713 pass(es).
[ View ]
#106 locale-1978916-106.patch74.96 KBLetharion
PASSED: [[SimpleTest]]: [MySQL] 58,539 pass(es).
[ View ]
#105 locale-1978916-105.patch69.11 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,436 pass(es).
[ View ]
#105 interdiff.txt3.17 KBtim.plunkett
#103 drupal8.locale-module.1978916-103.patch69.08 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,560 pass(es), 13 fail(s), and 419 exception(s).
[ View ]
#103 interdiff.txt942 bytesdisasm
#100 drupal8.locale-module.1978916-100.patch69.07 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,085 pass(es), 13 fail(s), and 419 exception(s).
[ View ]
#100 interdiff.txt7.23 KBdisasm
#97 locale-1978916-96.patch69.08 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,496 pass(es), 32 fail(s), and 8 exception(s).
[ View ]
#97 interdiff.txt7.52 KBtim.plunkett
#96 drupal8.locale-module.1978916-96.patch67.58 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 53,628 pass(es), 398 fail(s), and 400 exception(s).
[ View ]
#96 interdiff.txt1.97 KBdisasm
#94 drupal8.locale-module.1978916-94.patch67.47 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/locale/lib/Drupal/locale/Form/TranslateFormBase.php.
[ View ]
#94 interdiff.txt3 KBdisasm
#91 drupal8.locale-module.1978916-91.patch68.22 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 57,722 pass(es), 214 fail(s), and 145 exception(s).
[ View ]
#91 interdiff.txt5.56 KBdisasm
#88 drupal8.locale-module.1978916-88.patch67.36 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,050 pass(es), 225 fail(s), and 169 exception(s).
[ View ]
#88 interdiff.txt21.19 KBdisasm
#87 drupal8.locale-module.1978916-87.patch72.24 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,096 pass(es), 161 fail(s), and 305 exception(s).
[ View ]
#80 locale-convert_locale_translate_page-1978916-77.patch72.49 KBlikin
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch locale-convert_locale_translate_page-1978916-77.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#80 interdiff-1978916-75-77.txt12.68 KBlikin
#77 locale-convert_locale_translate_page-1978916-75.patch71.67 KBlikin
FAILED: [[SimpleTest]]: [MySQL] 57,868 pass(es), 13 fail(s), and 804 exception(s).
[ View ]
#77 interdiff-1978916-75-77.txt12.68 KBlikin
#75 locale-convert_locale_translate_page-1978916-75.patch71.67 KBlikin
FAILED: [[SimpleTest]]: [MySQL] 58,055 pass(es), 13 fail(s), and 804 exception(s).
[ View ]
#75 interdiff-1978916-66-1978916-75.txt6.72 KBlikin
#66 locale-convert_locale_translate_page-1978916-66.patch70.36 KBlikin
PASSED: [[SimpleTest]]: [MySQL] 57,585 pass(es).
[ View ]
#66 interdiff-1978916-61-1978916-66.txt8.99 KBlikin
#61 locale-convert_locale_translate_page-1978916-61.patch70.21 KBlikin
PASSED: [[SimpleTest]]: [MySQL] 57,597 pass(es).
[ View ]
#61 1978916-interdiff-59-61.txt17.91 KBlikin
#59 1978916-locale-convert_locale_translate_page-59.patch51.83 KBlikin
FAILED: [[SimpleTest]]: [MySQL] 57,176 pass(es), 230 fail(s), and 5 exception(s).
[ View ]
#59 interdiff-1978916#55-1978916#59.txt1.39 KBlikin
#56 interdiff-52-55.txt1.65 KBlikin
#55 1978916-locale-convert_locale_translate_page-55.patch51.79 KBlikin
FAILED: [[SimpleTest]]: [MySQL] 57,412 pass(es), 261 fail(s), and 233 exception(s).
[ View ]
#55 1978916-locale-convert_locale_translate_page-55.patch51.79 KBlikin
FAILED: [[SimpleTest]]: [MySQL] 57,413 pass(es), 260 fail(s), and 233 exception(s).
[ View ]
#52 1978916-locale-convert_locale_translate_page-52.patch51.83 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] 57,305 pass(es), 342 fail(s), and 110 exception(s).
[ View ]
#52 interdiff-51-52.txt5.2 KBYesCT
#51 1978916-locale-convert_locale_translate_page-51.patch50.79 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] 57,292 pass(es), 342 fail(s), and 112 exception(s).
[ View ]
#51 interdiff-49-51.txt7.97 KBYesCT
#49 1978916-locale-convert_locale_translate_page-49.patch50.47 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] 57,249 pass(es), 307 fail(s), and 340 exception(s).
[ View ]
#49 interdiff-47-49.txt1.8 KBYesCT
#47 1978916-locale-convert_locale_translate_page-47.patch49.73 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] 57,332 pass(es), 269 fail(s), and 382 exception(s).
[ View ]
#47 interdiff-45-47.txt3.72 KBYesCT
#45 1978916-locale-convert_locale_translate_page-45.patch49.63 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] 57,344 pass(es), 266 fail(s), and 387 exception(s).
[ View ]
#45 interdiff-44-45.txt3.35 KBYesCT
#44 1978916-locale-convert_locale_translate_page-44.patch52.97 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1978916-locale-convert_locale_translate_page-44.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#44 interdiff-39-44.txt7.57 KBYesCT
#43 diff-locale_translate_edit_form-TranslateEditForm-buildForm.txt3.33 KBYesCT
#43 diff-locale_translate_edit_form_validate-TranslateEditForm-validateForm.txt811 bytesYesCT
#43 diff-locale_translate_edit_form_submit-TranslateEditForm-submitForm.txt1.53 KBYesCT
#43 diff-locale_translate_filter_form-TranslateFilterForm-buildForm.txt492 bytesYesCT
#43 diff-locale_translate_filter_form_submit-TranslateFilterForm-submitForm.txt395 bytesYesCT
#43 diff-locale_translate_filter_values-TranslateFormBase-translateFilterValues.txt690 bytesYesCT
#43 diff-locale_translate_filter_load_strings-TranslateFormBase-translateFilterLoadStrings.txt502 bytesYesCT
#43 diff-locale_translate_filters-TranslateFormBase-translateFilters.txt80 bytesYesCT
#39 1978916-locale-convert_locale_translate_page-39.patch52.49 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 57,350 pass(es), 266 fail(s), and 382 exception(s).
[ View ]
#39 1978916-diff-33-39.txt11.54 KBvijaycs85
#33 1978916-locale-convert_locale_translate_page-33.patch47.46 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 57,382 pass(es), 263 fail(s), and 426 exception(s).
[ View ]
#27 locale-convert_locale_translate_page-1978916-27.patch47.46 KBlikin
FAILED: [[SimpleTest]]: [MySQL] 57,394 pass(es), 263 fail(s), and 426 exception(s).
[ View ]
#27 interdiff.txt12.33 KBlikin
#24 locale-convert_locale_translate_page-1978916-24.patch63.58 KBlikin
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch locale-convert_locale_translate_page-1978916-24.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#21 locale-convert_locale_translate_page-1978916-21.patch5.84 KBlikin
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch locale-convert_locale_translate_page-1978916-21.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#20 locale-convert_locale_translate_page-1978916-20.patch63.46 KBlikin
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch locale-convert_locale_translate_page-1978916-20.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#15 1978916-convert_locale_translate_page.patch62.48 KBayelet_Cr
FAILED: [[SimpleTest]]: [MySQL] 54,796 pass(es), 177 fail(s), and 611 exception(s).
[ View ]
#13 1978916-convert_locale_translate_page.patch62.48 KBayelet_Cr
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/locale/lib/Drupal/locale/Form/TranslateFormBase.php.
[ View ]
#8 1978916-convert_locale_translate_page.patch35.66 KBayelet_Cr
PASSED: [[SimpleTest]]: [MySQL] 55,912 pass(es).
[ View ]
#5 1978916-convert_locale_translate_page.patch35.59 KBayelet_Cr
FAILED: [[SimpleTest]]: [MySQL] 55,874 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#3 1978916-convert_locale_translate_page.patch35.58 KBayelet_Cr
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1978916-convert_locale_translate_page.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Assigned:Unassigned» ayelet_Cr

Issue tags:+D8MI

Status:Active» Needs review
StatusFileSize
new35.58 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1978916-convert_locale_translate_page.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Status:Needs review» Needs work

The last submitted patch, 1978916-convert_locale_translate_page.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new35.59 KB
FAILED: [[SimpleTest]]: [MySQL] 55,874 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 1978916-convert_locale_translate_page.patch, failed testing.

Issue tags:+language-interface

Short extract from our IRC discussion:

[10:02pm] GaborHojtsy: looks like this is a line lost
[10:02pm] GaborHojtsy: -  // Keep the user on the current pager page.
[10:02pm] GaborHojtsy: -  if (isset($_GET['page'])) {
[10:02pm] GaborHojtsy: -    $form_state['redirect'] = array('admin/config/regional/translate', array('query' => array('page' => $_GET['page'])));
[10:02pm] GaborHojtsy: -  }
[10:02pm] GaborHojtsy: -
[10:02pm] GaborHojtsy: -  if ($updated) {
[10:02pm] GaborHojtsy: -    // Clear cache and refresh configuration and JavaScript translations.
[10:02pm] GaborHojtsy: -    _locale_refresh_translations(array($langcode), $updated);
[10:02pm] GaborHojtsy: -    _locale_refresh_configuration(array($langcode), $updated);
[10:02pm] GaborHojtsy: -  }
[10:02pm] GaborHojtsy: from the part removed
[10:02pm] GaborHojtsy: VS
[10:02pm] GaborHojtsy: +    // Keep the user on the current pager page.
[10:02pm] GaborHojtsy: +    if (isset($_GET['page'])) {
[10:02pm] GaborHojtsy: +      $form_state['redirect'] = array('admin/config/regional/translate', array('query' => array('page' => $_GET['page'])));
[10:02pm] GaborHojtsy: +    }
[10:02pm] GaborHojtsy: +
[10:02pm] GaborHojtsy: +    if ($updated) {
[10:02pm] GaborHojtsy: +      // Clear cache and force refresh of JavaScript translations.
[10:02pm] GaborHojtsy: +      _locale_refresh_translations(array($langcode), $updated);
[10:02pm] GaborHojtsy: +    }
[10:02pm] GaborHojtsy: in the added
[10:02pm] GaborHojtsy: note no refresh configuration called anymore
[10:03pm] GaborHojtsy: (otherwise the same code seems like)
[10:03pm] GaborHojtsy: would be good to check the newly added lines are not missing anything else :)

Status:Needs work» Needs review
StatusFileSize
new35.66 KB
PASSED: [[SimpleTest]]: [MySQL] 55,912 pass(es).
[ View ]

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

The last submitted patch, 1978916-convert_locale_translate_page.patch, failed testing.

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

Status:Needs review» Reviewed & tested by the community

Looks to me like a good straightforward conversion. The routing, classes, etc. look fine. I did not compare the moved lines one by one, assuming based on the prior error we found it was copied over with the new code. Looks good to me!

Status:Reviewed & tested by the community» Needs work

The patch no longer applies. Also here is some feedback.

+++ b/core/modules/locale/lib/Drupal/locale/Controller/LocaleController.phpundefined
@@ -0,0 +1,54 @@
+  public function translatePage() {
+
+    return array(
+++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateEditForm.phpundefined
@@ -0,0 +1,232 @@
+  public function buildForm(array $form, array &$form_state, Node $node = NULL) {
+
+    $filter_values = locale_translate_filter_values();

There should be no blank line after the beginning of a method. I didn't bother highlighting them all, but please go through each one.

+++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateEditForm.phpundefined
@@ -0,0 +1,232 @@
+namespace Drupal\locale\Form;
+
+use Drupal\Core\Form\FormInterface;
+use Drupal\locale\SourceString;
+
+class TranslateEditForm implements FormInterface {
...
+  public function buildForm(array $form, array &$form_state, Node $node = NULL) {

Node is not in the use statements, which means this form does not have proper test coverage. This would cause an error.

While fixing it, please typehint with NodeInterface

+++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateEditForm.phpundefined
@@ -0,0 +1,232 @@
+      $plural_formulas = state()->get('locale.translation.plurals') ?: array();

This should have stat injected.

+++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateEditForm.phpundefined
@@ -0,0 +1,232 @@
+            '#markup' => check_plain($source_array[0]),

use Drupal\Component\Utility\String::checkPlain()

+++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateEditForm.phpundefined
@@ -0,0 +1,232 @@
+        $form['actions'] = array('#type' => 'actions');
+        $form['actions']['submit'] = array('#type' => 'submit', '#value' => t('Save translations'));

Please split this out on multiple lines for readability.

+++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateEditForm.phpundefined
@@ -0,0 +1,232 @@
+      _locale_refresh_translations(array($langcode), $updated);
+      _locale_refresh_configuration(array($langcode), $updated);

Could these be moved to methods somewhere?

+++ b/core/modules/locale/locale.moduleundefined
@@ -1355,3 +1353,135 @@ function _locale_rebuild_js($langcode = NULL) {
+function locale_translate_filter_load_strings() {
...
+function locale_translate_filter_values() {
...
+function locale_translate_filters() {

It looks like these are only used by the forms, they should be protected methods.

+++ b/core/modules/locale/locale.moduleundefined
@@ -1355,3 +1353,135 @@ function _locale_rebuild_js($langcode = NULL) {
+      if (isset($_GET[$key])) {
...
+          $filter_values[$key] = $_GET[$key];
...
+      elseif (isset($_SESSION['locale_translate_filter'][$key])) {
...
+        if (!isset($filter['options']) || isset($filter['options'][$_SESSION['locale_translate_filter'][$key]])) {
+          $filter_values[$key] = $_SESSION['locale_translate_filter'][$key];

Once this is moved into the form, it can use the Request object directly.

+++ b/core/modules/locale/locale.routing.ymlundefined
@@ -4,3 +4,17 @@ locale_settings:
+locale_translate_page_tab:
+  pattern: '/admin/config/regional/translate/translate'
+  defaults:
+    _content: 'Drupal\locale\Controller\LocaleController::translatePage'
+  requirements:
+    _permission: 'translate interface'

The consensus so far in #1995620: [policy, no patch] Document how to handle routes for MENU_DEFAULT_LOCAL_TASK has been to *not* define duplicate routes. You should be able to remove this.

Status:Needs work» Needs review
StatusFileSize
new62.48 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/locale/lib/Drupal/locale/Form/TranslateFormBase.php.
[ View ]

Status:Needs review» Needs work

The last submitted patch, 1978916-convert_locale_translate_page.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new62.48 KB
FAILED: [[SimpleTest]]: [MySQL] 54,796 pass(es), 177 fail(s), and 611 exception(s).
[ View ]

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

The last submitted patch, 1978916-convert_locale_translate_page.patch, failed testing.

Status:Needs work» Needs review

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

The last submitted patch, 1978916-convert_locale_translate_page.patch, failed testing.

Issue tags:+Needs reroll

StatusFileSize
new63.46 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch locale-convert_locale_translate_page-1978916-20.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

The rerolled patch #15.

StatusFileSize
new5.84 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch locale-convert_locale_translate_page-1978916-21.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

The rerolled patch is too old. The system does not work with it. I've recovered the code. It works. It needs code review.

* What is better to use: String::checkPlain or check_plaine?
* I've made a

<?php
function translateFilterValues(Request $request) {
 
// @todo vlikin: Need be decoupled.
 
$request = \Drupal::request();
?>

in TranslateFormBase. I see. It should be decoupled. Where is a right place to get it from service?
* Frequently We use
<?php
return \Drupal::service('locale.storage')->getTranslations($conditions, $options);
?>

It is too long. What style should we bend on?

+++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateEditForm.phpundefined
@@ -80,7 +79,7 @@ public function buildForm(array $form, array &$form_state, NodeInterface $node =
-            '#markup' => String::check_plain($source_array[0]),
+            '#markup' => check_plain($source_array[0]),

This is backwards, we should never use check_plain() inside a class.

+++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateEditForm.phpundefined
@@ -183,7 +182,7 @@ public function submitForm(array &$form, array &$form_state) {
+    foreach (\Drupal::service('locale.storage')->getTranslations(array('lid' => $lids, 'language' => $langcode, 'translated' => TRUE)) as $existing_translation_object) {

This can be injected, see the conversion guide linked at the top of the issue

+++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateFormBase.phpundefined
@@ -48,13 +49,16 @@ function translateFilterLoadStrings() {
   function translateFilterValues(Request $request) {
+    // @todo vlikin: Need be decoupled.
+    $request = \Drupal::request();

You already have the $request being passed in.

StatusFileSize
new63.58 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch locale-convert_locale_translate_page-1978916-24.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

The whole patch.

My previous review still stands.

+++ b/core/modules/locale/lib/Drupal/locale/Controller/LocaleController.phpundefined
@@ -0,0 +1,53 @@
+      'filter' => drupal_get_form(new TranslateFilterForm($this->state)),
+      'form' => drupal_get_form(new TranslateEditForm($this->state)),

If the constructors of these forms ever change, we'll have to come back here and do it as well.

Instead, let's make this controller ContainerAware (or at least store $this->container), and rewrite this as

return array(
  'filter' => TranslateFilterForm::create($this->container),
  'form' => TranslateEditForm::create($this->container),
);

And make those two forms implement ControllerInterface, and get the state themselves.

+++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateEditForm.phpundefined
@@ -0,0 +1,240 @@
+  public function buildForm(array $form, array &$form_state, NodeInterface $node = NULL) {
+++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateFilterForm.phpundefined
@@ -0,0 +1,110 @@
+  public function buildForm(array $form, array &$form_state, Node $node = NULL) {

Where is $node ever used?

+++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateEditForm.phpundefined
@@ -0,0 +1,240 @@
+    foreach (\Drupal::service('locale.storage')->getTranslations(array('lid' => $lids, 'language' => $langcode, 'translated' => TRUE)) as $existing_translation_object) {

Once this is converted to ControllerInterface, it can inject this as well.

+++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateEditForm.phpundefined
@@ -0,0 +1,240 @@
+    if (isset($_GET['page'])) {

Use the $request instead

+++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateFilterForm.phpundefined
@@ -0,0 +1,110 @@
+  public function submitForm(array &$form, array &$form_state) {
+    $op = $form_state['values']['op'];
+    $filters = $this->translateFilters();
+    switch ($op) {
+      case t('Filter'):
+        foreach ($filters as $name => $filter) {
+          if (isset($form_state['values'][$name])) {
+            $_SESSION['locale_translate_filter'][$name] = $form_state['values'][$name];
+          }
+        }
+        break;
+
+      case t('Reset'):
+        $_SESSION['locale_translate_filter'] = array();
+        break;
+    }
+
+    $form_state['redirect'] = 'admin/config/regional/translate';

Why not have different submit methods for each button?

Assigned:ayelet_Cr» Unassigned
Issue tags:-Needs reroll

@ayelet_Cr It's been a while, are you ok with us helping here?

We should go through each point of the recent reviews and make sure they are addressed.
We dont have to do it all at once, but if we only address a couple, we should summarize and say what we addressed and what is still left to do.

Also, at this point, if possible, we should be using interdiffs each time we post a new patch.

For instructions on creating an interdiff, see https://drupal.org/documentation/git/interdiff | Microbranching workflow: http://xjm.drupalgardens.com/blog/interdiffs-how-make-them-and-why-they-...

@likin Do you want to try another patch and I can review it? Or do you want to do it the other way around? I can give it a try to do some of the concerns identified. This looks really interesting.

since the patch applies, removing the needs reroll tag. it's still needs work though. :)

StatusFileSize
new12.33 KB
new47.46 KB
FAILED: [[SimpleTest]]: [MySQL] 57,394 pass(es), 263 fail(s), and 426 exception(s).
[ View ]

The reroll process has been done. The notices above have not been applied yet.

Status:Needs work» Needs review

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

The last submitted patch, locale-convert_locale_translate_page-1978916-27.patch, failed testing.

Status:Needs work» Needs review

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

The last submitted patch, locale-convert_locale_translate_page-1978916-27.patch, failed testing.

I see we have a lot of test problems. Those are not related with locale_traslate_page.

Status:Needs work» Needs review
StatusFileSize
new47.46 KB
FAILED: [[SimpleTest]]: [MySQL] 57,382 pass(es), 263 fail(s), and 426 exception(s).
[ View ]

Just a re-roll with current code base and will check what testbot says.

Issue tags:+LONDON_2013_JULY

Status:Needs review» Needs work

The last submitted patch, 1978916-locale-convert_locale_translate_page-33.patch, failed testing.

Status:Needs work» Needs review

@vijaycs85 Attach interdiff.

+++ b/core/modules/locale/lib/Drupal/locale/Controller/LocaleController.phpundefined
@@ -22,6 +27,15 @@ class LocaleController implements ControllerInterface {
+  ¶

Let's remove this unneeded whitespace.

+++ b/core/modules/locale/lib/Drupal/locale/Controller/LocaleController.phpundefined
@@ -69,4 +85,16 @@ public function checkTranslation() {
+   * Page callback: Shows the string search screen.
...
+  }
}

Just to nitpick as well: Let's drop 'Page callback: ' as well as add an empty line before the last to closing brackets at the end of the file.

+++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateEditForm.phpundefined
@@ -0,0 +1,240 @@
+                '#title' => ($i == 0 ? t('Singular form') : format_plural($i, 'First plural form', '@count. plural form')),

t() should be now be done by using an injected translation service.

+++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateEditForm.phpundefined
@@ -0,0 +1,240 @@
+    foreach (\Drupal::service('locale.storage')->getTranslations(array('lid' => $lids, 'language' => $langcode, 'translated' => TRUE)) as $existing_translation_object) {
...
+        $target = isset($existing_translation_objects[$lid]) ? $existing_translation_objects[$lid] : \Drupal::service('locale.storage')->createTranslation(array('lid' => $lid, 'language' => $langcode));

It seems to be that we should probably inject this service into the class instead of directly calling it.

+++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateEditForm.phpundefined
@@ -0,0 +1,240 @@
+    if (isset($_GET['page'])) {
+      $form_state['redirect'] = array('admin/config/regional/translate', array('query' => array('page' => $_GET['page'])));

Instead of using $_GET['query'] you should always try to use the request object

StatusFileSize
new11.54 KB
new52.49 KB
FAILED: [[SimpleTest]]: [MySQL] 57,350 pass(es), 266 fail(s), and 382 exception(s).
[ View ]

Thanks for the review @dawehner. Fixed all items in #38

I'm also looking at this one. I'm going through the previous feedback trying to make sure it was addressed.

+++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateEditForm.phpundefined
@@ -0,0 +1,274 @@
+  public function __construct(KeyValueStoreInterface $state, TranslatorInterface $translator, StringDatabaseStorage $local_storage) {

should this be $locale_storage? maybe $translation_storage?

Status:Needs review» Needs work

The last submitted patch, 1978916-locale-convert_locale_translate_page-39.patch, failed testing.

StatusFileSize
new7.57 KB
new52.97 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1978916-locale-convert_locale_translate_page-44.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

This
adds doc blocks to classes,

uses \FullyQualified class name in comments "If you use a namespace in documentation, always make sure it is a fully-qualified namespace (beginning with a backslash)." https://drupal.org/coding-standards/docs#classes ,

adds missing @param's and @returns

adds the empty line before the close brace of classes (mentioned in #38 and also shown in https://drupal.org/node/1353118 [edit: oh! and specified here: https://drupal.org/node/608152#indenting in the OO standards]

puts new use statements in alphabetical order

takes out some extra newlines

makes oneline summaries less than 80 chars "Lines containing comments (including docblocks) must wrap as close to 80 characters as possible without going over, with a few exceptions (noted in the Tag Reference below)." https://drupal.org/coding-standards/docs#drupal

corrects some spelling and grammar (in new comments).

===

some other fixing coming.

StatusFileSize
new3.35 KB
new49.63 KB
FAILED: [[SimpleTest]]: [MySQL] 57,344 pass(es), 266 fail(s), and 387 exception(s).
[ View ]

+++ b/core/modules/node/lib/Drupal/node/Controller/NodeController.phpundefined
@@ -0,0 +1,98 @@
+<?php
+
+/**
+ * @file
+ * Contains \Drupal\node\Controller\NodeController.
+ */

This snuck into #33

Taking it out.
If it is needed, just let us know. :)

--
more fixes coming.

Status:Needs review» Needs work

The last submitted patch, 1978916-locale-convert_locale_translate_page-45.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.72 KB
new49.73 KB
FAILED: [[SimpleTest]]: [MySQL] 57,332 pass(es), 269 fail(s), and 382 exception(s).
[ View ]
  1. +++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateEditForm.phpundefined
    @@ -0,0 +1,278 @@
    +    $form['langcode'] = array(
    +      '#type' => 'value',
    +      '#value' => $filter_values['langcode'],
    +    );
    ...
    +        $form['actions']['submit'] = array(
    +          '#type' => 'submit',
    +          '#value' => $this->translator->translate('Save translations'));

    arrays when split over multiple lines are usually split with the closing paren on its own line.

    @tim.plunkett asked in #12 for it to be split. Just fixing the formatting here. https://drupal.org/coding-standards#array

  2. +++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateFormBase.phpundefined
    @@ -0,0 +1,166 @@
    +  function translateFilterLoadStrings(Request $request) {
    ...
    +  function translateFilterValues(Request $request) {
    ...
    +  function translateFilters() {

    in #12, tim asked:
    +++ b/core/modules/locale/locale.moduleundefined
    @@ -1355,3 +1353,135 @@ function _locale_rebuild_js($langcode = NULL) {
    +function locale_translate_filter_load_strings() {
    ...
    +function locale_translate_filter_values() {
    ...
    +function locale_translate_filters() {
    It looks like these are only used by the forms, they should be protected methods.

    they are methods now, but not noted as public or protected.

    I made them protected.

    Also, https://drupal.org/node/608152#indenting "All methods and properties of classes must specify their visibility: public, protected, or private."

  3. +++ b/core/modules/locale/locale.pages.incundefined
    @@ -12,460 +12,27 @@
    -  if ($updated) {
    -    // Clear cache and refresh configuration and JavaScript translations.
    -    _locale_refresh_translations(array($langcode), $updated);
    -    _locale_refresh_configuration(array($langcode), $updated);

    in #12, tim asked for:
    +++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateEditForm.phpundefined
    @@ -0,0 +1,232 @@
    + _locale_refresh_translations(array($langcode), $updated);
    + _locale_refresh_configuration(array($langcode), $updated);
    Could these be moved to methods somewhere?

    seems like those lines were moved from locale_translate_edit_form_submit() to TranslateEditForm::submitForm

    I'm not sure that is what was being asked for.

    I think tim is asking to move them from locale.module to methods in a class.

    =======

  4. I was going back through the history here,
    Note #13 doubled in size mostly because of fixing tests... like
    > - $this->drupalGet('admin/config/regional/translate/translate');
    > + $this->drupalGet('admin/config/regional/translate');

    =======

  5. @likin
    the reroll in #20
    did:
    locale_storage()->
    to
    $this->container->get('locale.storage')->

    locale_storage()->
    to
    Drupal::service('locale.storage')->

    language()->langcode
    to
    language()->id

    by:
    < $js_strings = locale_storage()->getStrings(array('type' => 'javascript'));
    ---
    > $js_strings = $this->container->get('locale.storage')->getStrings(array('type' => 'javascript'));
    just context lines change.

    and:
    @@ -12,463 +12,6 @@
    1128c1128
    < - return locale_storage()->getTranslations($conditions, $options);
    ---
    > - return Drupal::service('locale.storage')->getTranslations($conditions, $options);

    etc.

    but it only changed the context and deleted lines,
    it did not then also update those lines in the places those lines were moved to.

    so the return in translateFilterLoadStrings needed to be updated to be
    return Drupal::service('locale.storage')->getTranslations($conditions, $options);
    this is fixed by #39 and is fine now I think.

    Mentioning it here to help future rerolls.

    Also, in my head, rerolls are when a patch does not apply, and only changes made to the patch are those needed because head has changed. Most often rerolls are difficult to make interdiffs for, but it's ok to do so, or to just do a diff between the two patches.

    If changes are needed after a reroll, a separate patch and interdiff are then made. I try not to mix a reroll with improvements.

    Other new patches, I dont call rerolls, I just call them new patches. Those I almost always make interdiffs for.
    =============

  6. in #12 tim asked:
    +++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateEditForm.phpundefined
    @@ -0,0 +1,232 @@
    + '#markup' => check_plain($source_array[0]),

    use Drupal\Component\Utility\String::checkPlain()

    #13 tried to do that but used String::check_plain instead of String::checkPlain
    #21 put it back to the procedural
    #23 pointed out that it should be checkPlain.

    But here I do the use and String::checkPlain again. Let us see if this works this time.

  7. $_SESSION is still used, and I suspect we can use the Request object instead. Not sure yet how to.
  8. in #23 @tim.plunkett asked:
    +++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateEditForm.phpundefined
    @@ -183,7 +182,7 @@ public function submitForm(array &$form, array &$form_state) {
    +    foreach (\Drupal::service('locale.storage')->getTranslations(array('lid' => $lids, 'language' => $langcode, 'translated' => TRUE)) as $existing_translation_object) {

    This can be injected, see the conversion guide linked at the top of the issue

    looking at a previous time I did dependency injection: https://drupal.org/node/1945226#comment-7572819

    ... I'm going to post what I have so far and do the injection in a bit.

Status:Needs review» Needs work

The last submitted patch, 1978916-locale-convert_locale_translate_page-47.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.8 KB
new50.47 KB
FAILED: [[SimpleTest]]: [MySQL] 57,249 pass(es), 307 fail(s), and 340 exception(s).
[ View ]

continuing 8.

Looking at https://drupal.org/node/1953354 I was still confused, but tried anyway.

I blindly guess that the property might want to be localeStorage since it was
\Drupal::service('locale.storage')->whatever...

So we need a property so we can do
$this->localeStorage->whatever...

I found the type that that property should be (with help from @tim.plunkett and @xjm in irc)
by looking for a locale.services.yml file
and looking for locale.storage in there.

it says:

  locale.storage:
    class: Drupal\locale\StringDatabaseStorage
    arguments: ['@database']

The type of the property though should be an interface whenever possible. "if there is no interface it's a bug"
So I opened up StringDatabaseStorage and it has:
class StringDatabaseStorage implements StringStorageInterface {

So, that gives me this part:

  /**
   * The locale storage.
   *
   * @var \Drupal\locale\StringStorageInterface
   */
  protected $localeStorage;

Looking at a previous example of injection, I see I will want a create() (or createInstance() if it is dealing with an EntityControllerInterface since that had a create() already) and __construct()

__construct() should set the property, so do: $this->localeStorage = $somethingPassedInAsAParameter and I know the type of the parameter should be the same type as the property.

So that gets me:

  /**
   * Constructs a new TranslationFormBase object.
   *
   * @param \Drupal\locale\StringStorageInterface $locale_storage
   *   The locale storage.
   */
  public function __construct(StringStorageInterface $locale_storage) {
    $this->localeStorage = $locale_storage;
  }

Since this is not an EntityController, I want a create() method.
Looking at:
abstract class TranslateFormBase implements FormInterface {
I check in FormInterface to see what it has for its create(), but there is no create.
Tim says FormInterface doesn't assume injection.
So use ControllerInterface because ... that's its purpose it only has create() and allows injection.
It has the pattern   public static function create(ContainerInterface $container);

That means I also want to implement that like:
abstract class TranslateFormBase implements ControllerInterface, FormInterface {

and then make a create like:

  /**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container) {
    return new static(
      $container->get('locale.storage')
    );
  }

That new code means new use's also.

Attached is the interdiff for all that.

Status:Needs review» Needs work

The last submitted patch, 1978916-locale-convert_locale_translate_page-49.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new7.97 KB
new50.79 KB
FAILED: [[SimpleTest]]: [MySQL] 57,292 pass(es), 342 fail(s), and 112 exception(s).
[ View ]

oh...

  1. rereading #23
    +++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateEditForm.phpundefined
    @@ -183,7 +182,7 @@ public function submitForm(array &$form, array &$form_state) {
    +    foreach (\Drupal::service('locale.storage')->getTranslations(array('lid' => $lids, 'language' => $langcode, 'translated' => TRUE)) as $existing_translation_object) {

    This can be injected, see the conversion guide linked at the top of the issue

    I see that was talking about TranslateEditForm, and I did the injection in TranslationFormBase.

    In TranslateEditForm it's already now
        foreach ($this->localStorage->getTranslations(array('lid' => $lids, 'language' => $langcode, 'translated' => TRUE)) as $existing_translation_object) {

    *which* makes me think it really should be localeStorage and not localStorage.

    Note, @Crell thought in irc that we should use a better name that localeStorage because of the easy confusion with localStorage. Maybe stringStorage or something (see the question in #41.

    Now I'm unsure if 49 is needed, but if it is, trying to make this work with that....
    I think the TranslateEditForm create() is wrong now... not matching ControllerInterface
    ah, I see looking at #39 I can get the state and translation from the container. Trying that.

  2. Another thing to check is the Node $node = NULL argument in buildForm. It seems to be unused.

    +++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateEditForm.phpundefined
    @@ -0,0 +1,280 @@
    +  public function buildForm(array $form, array &$form_state, NodeInterface $node = NULL) {
    +    $filter_values = $this->translateFilterValues();
    +    $langcode = $filter_values['langcode'];

    tim brought up the same point in #25

    Where is $node ever used?

    . I took those arguments out.

  3. Also, translateFilterValues is expecting $request, but other places it is called are not passing the request in.
    I'm not sure if that means request needs to be set somewhere in TranslateFilterForm...
    We need to pass in $request to translateFilterValues, or handle the @todo.

    +++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateFormBase.phpundefined
    @@ -0,0 +1,195 @@
    +  protected function translateFilterValues(Request $request) {
    +    // @todo vlikin: Need be decoupled.
    +    $request = \Drupal::request();

    If we take out the $request = \Drupal::request(); then we need to make sure to pass in request everywhere translateFilterValues is called.

    +++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateFilterForm.phpundefined
    @@ -0,0 +1,114 @@
    +  public function buildForm(array $form, array &$form_state, Node $node = NULL) {
    +    $filters = $this->translateFilters();
    +    $filter_values = $this->translateFilterValues();

    here.

    +++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateFilterForm.phpundefined
    @@ -0,0 +1,114 @@
    +  public function buildForm(array $form, array &$form_state, Node $node = NULL) {
    +    $filters = $this->translateFilters();
    +    $filter_values = $this->translateFilterValues();

    here also.

    Do we put a $request property on TranslateFormBase?

  4. Getting back to looking at previous reviews... #25

    we did change the constructor, so the first point in #25 is well taken. Making that change.

    --- a/core/modules/locale/lib/Drupal/locale/Controller/LocaleController.php
    +++ b/core/modules/locale/lib/Drupal/locale/Controller/LocaleController.php
    @@ -96,8 +96,8 @@ public function checkTranslation() {
       public function translatePage() {
         $state = $this->container->get('keyvalue')->get('state');
         return array(
    -      'filter' => drupal_get_form(new TranslateFilterForm($state)),
    -      'form' => drupal_get_form(new TranslateEditForm($state, $this->container->get('string_translation'), $this->container->get('locale.storage'))),
    +      'filter' => TranslateFilterForm::create($this->container),
    +      'form' => TranslateEditForm::create($this->container),
         );
       }
  5. From #25
    +++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateFilterForm.phpundefined
    @@ -0,0 +1,110 @@
    +  public function submitForm(array &$form, array &$form_state) {
    +    $op = $form_state['values']['op'];
    +    $filters = $this->translateFilters();
    +    switch ($op) {
    +      case t('Filter'):
    +        foreach ($filters as $name => $filter) {
    +          if (isset($form_state['values'][$name])) {
    +            $_SESSION['locale_translate_filter'][$name] = $form_state['values'][$name];
    +          }
    +        }
    +        break;
    +
    +      case t('Reset'):
    +        $_SESSION['locale_translate_filter'] = array();
    +        break;
    +    }
    +
    +    $form_state['redirect'] = 'admin/config/regional/translate';

    Why not have different submit methods for each button?

    This needs addressed.

  6. The rest of #25 was already addressed.
  7. The points in #38 were addressed (most done during #39)

Next, I will take a look at the diffs in #43

Assigned:YesCT» Unassigned
StatusFileSize
new5.2 KB
new51.83 KB
FAILED: [[SimpleTest]]: [MySQL] 57,305 pass(es), 342 fail(s), and 110 exception(s).
[ View ]

From diff-locale_translate_edit_form-TranslateEditForm-buildForm.txt

<           '#title' => t('Source string (@language)', array('@language' => t('Built-in English'))),
...
>           '#title' => $this->translator->translate('Source string'),
...
<           '#markup' => '<span lang="en">' . check_plain($source_array[0]) . '</span>',
...
>           '#markup' => check_plain($source_array[0]),
...
<           '#prefix' => '<span class="visually-hidden">' . t('Source string (@language)', array('@language' => t('Built-in English'))) . '</span>',
etc.

did we loose this Built-in English on purpose? I dont think so.

Looks like it was put in in #2004684: Improve Accessibility for String Translation UI on June 20.

--------
from diff-locale_translate_filter_form-TranslateFilterForm-buildForm.txt

<     drupal_get_path('module', 'locale') . '/css/locale.admin.css',
...
>     drupal_get_path('module', 'locale') . '/locale.admin.css',

June 7, #1987066: Rename files to match CSS file naming convention

---------
the rest of the diffs look like they have changes we intended.

I'm taking a break from this for a bit. It is available for anyone to take on.

Status:Needs review» Needs work

The last submitted patch, 1978916-locale-convert_locale_translate_page-52.patch, failed testing.

<?php
/**
* Defines a translation edit form.
*/
class TranslateEditForm extends TranslateFormBase implements FormInterface {
?>

Shoud be changed to :

<?php
/**
* Defines a translation edit form.
*/
class TranslateEditForm extends TranslateFormBase {
?>

TranslateFormBase already containes FormInterface.

StatusFileSize
new51.79 KB
FAILED: [[SimpleTest]]: [MySQL] 57,413 pass(es), 260 fail(s), and 233 exception(s).
[ View ]
new51.79 KB
FAILED: [[SimpleTest]]: [MySQL] 57,412 pass(es), 261 fail(s), and 233 exception(s).
[ View ]

The patch is workable. I push it to run tests.

StatusFileSize
new1.65 KB

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 1978916-locale-convert_locale_translate_page-55.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.39 KB
new51.83 KB
FAILED: [[SimpleTest]]: [MySQL] 57,176 pass(es), 230 fail(s), and 5 exception(s).
[ View ]

The form TranslateEditForm has been corrected to pass test successfully. It was a bug that could be defined by manual testing. :)

Status:Needs review» Needs work

The last submitted patch, 1978916-locale-convert_locale_translate_page-59.patch, failed testing.

StatusFileSize
new17.91 KB
new70.21 KB
PASSED: [[SimpleTest]]: [MySQL] 57,597 pass(es).
[ View ]

Status:Needs work» Needs review

+++ b/core/modules/locale/lib/Drupal/locale/Controller/LocaleController.phpundefined
@@ -24,14 +29,26 @@ class LocaleController implements ControllerInterface {
   /**
+   * The container.
+   *
+   * @var \Symfony\Component\DependencyInjection\ContainerInterface
+   */
+  protected $container;
...
+  public function __construct(ModuleHandlerInterface $module_handler, PathBasedGeneratorInterface $url_generator, ContainerInterface $container) {
...
+    $this->container = $container;
@@ -40,7 +57,8 @@ public function __construct(ModuleHandlerInterface $module_handler, PathBasedGen
+      $container
@@ -69,4 +87,16 @@ public function checkTranslation() {
+  }
}
diff --git a/core/modules/locale/lib/Drupal/locale/Form/TranslateEditForm.php b/core/modules/locale/lib/Drupal/locale/Form/TranslateEditForm.php

In order to properly document which classes are containeraware this controller should implement the ContainerAwareInterface

+++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateEditForm.phpundefined
@@ -0,0 +1,292 @@
+    $this->state = $state;
+    $this->translator = $translator;
+    $this->localeStorage = $locale_storage;
+    $this->request = $request;

We could also call parent::__construct to store the variables.

+++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateFilterForm.phpundefined
@@ -0,0 +1,114 @@
+    $op = $form_state['values']['op'];
...
+    switch ($op) {
+      case t('Filter'):

This code was just moved, so that is fine, but isn't there a better way then dealing with t() directly? Afaik there is something like triggered_element

+++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateFormBase.phpundefined
@@ -0,0 +1,203 @@
+    $filter_values = &drupal_static(__FUNCTION__);
...
+    // Get all languages, except English.
+    drupal_static_reset('language_list');

It feels wrong to still have drupal_static here. Maybe we should replace it by a property on the object.

Assigned:Unassigned» likin

In case it helps, the second thing about the parent is like we did in #2045043-17: Field listings operations cannot be altered

StatusFileSize
new8.99 KB
new70.36 KB
PASSED: [[SimpleTest]]: [MySQL] 57,585 pass(es).
[ View ]

The patch for the comment 61 has been corrected according to the comment 63.
I can not understand clear

This code was just moved, so that is fine, but isn't there a better way then dealing with t() directly? Afaik there is something like triggered_element

Status:Needs review» Needs work

@@ -24,14 +27,26 @@ class LocaleController implements ControllerInterface {
+    $this->container = $container;

The full container should never be passed to a ControllerInterface object. If you need a service, extract it in the create() method and inject it that way.

  1. @@ -0,0 +1,229 @@
    +abstract class TranslateFormBase implements ContainerAwareInterface, FormInterface {

    Forms should never be ContainerAware. Inject the services you need explicitly.

  2. @@ -0,0 +1,229 @@
    +  public static function create(ContainerInterface $container) {
    +    $form = new static(
    +      $container->get('locale.storage'),
    +      $container->get('request')
    +    );
    +    $form->setContainer($container);
    +    $renderable_form = drupal_get_form($form);
    +
    +    return $renderable_form;
    +  }

    This is a bug. create() should return the form OBJECT. Not the result of drupal_get_form(). Any create() method that does more than return new static(...) is probably a bug.

  1. @@ -0,0 +1,229 @@
    +    return self::$filter_values = $filter_values;

    1) You almost never want to use self:: in PHP 5.3. Use static:: instead.

    2) That said, this line is totally screwy. An object should (almost) never write to the class, as that becomes a quasi-global and pollutes other instances. If the idea here is caching computed values, save that to a $this-> property, not to self::.

    3) That said, don't bother caching a generated value unless you know it's actually needed. Excessive caching is just as much a problem as insufficient caching, but harder to fix later.

  2. @@ -0,0 +1,229 @@
    +      'title' => t('String contains'),

    The translation service should be injected.

  3. @@ -12,460 +12,27 @@
    +function locale_translation_manual_status() {
    +  module_load_include('compare.inc', 'locale');

    Ick, why are we *adding* a module_load_include()? We should be eliminating those.

@Crell. I agree with you partly. I also agree with Drupal way partly. I see that I should follow it. Your comments are different form the flow of this issues. I need somebody to review an issue and @Crell comments that are related with containers.

For @Crell #64

The full container should never be passed to a ControllerInterface object. If you need a service, extract it in the create() method and inject it that way.

Why not? I need it inject deeper into another objects like in the

/**
   * Shows the string search screen.
   *
   * @see locale_menu()
   */
  public function translatePage() {
    return array(
      'filter' => TranslateFilterForm::create($this->container),
      'form' => TranslateEditForm::create($this->container),
    );
  }

For @Crell #64 1.

Forms should never be ContainerAware. Inject the services you need explicitly.

Why not? ;) We have added some rules - Interface and helpful methods from. It works fine. Interface helps people see why we need it.

For @Crell #64

1) You almost never want to use self:: in PHP 5.3. Use static:: instead.

Maybe. LSB it is the very interesting topic. The feature exists in the scope. Here data should be shared between classes those inherit this class TranslateFormBase. So data is inherited between classes: TranslateFilterForm, TranslateEditForm. drupal_static has been used earlier. But it does not support methods of classes well. So if we use static, data will be saved in the current scopes. So same operations will be processed twice. So it is equal if we do not use this field in the static way. It is almost the same situation when data is saved into a field of a class instance.

It likes a hack way now. I think we need use a common context.

I agree with another comments.

You should still use constructor injection to properly declare your other dependencies, and only use $this->container for those two forms.

And please make the switch from self:: to static::, it's an existing pattern that we've tried to adhere to 100%.

I see it needs to be converted from ContainerInterface to ContainerAware.

No, from ContainerAware to ControllerInterface

Status:Needs work» Needs review
StatusFileSize
new6.72 KB
new71.67 KB
FAILED: [[SimpleTest]]: [MySQL] 58,055 pass(es), 13 fail(s), and 804 exception(s).
[ View ]

Done:

  • ConntainerAware moved to ControllerInterface
  • self:: moved to static::
  • string_translation service has been injected.

Also:

  • module_load_include is used widely. We need clean it later.

Status:Needs review» Needs work

The last submitted patch, locale-convert_locale_translate_page-1978916-75.patch, failed testing.

StatusFileSize
new12.68 KB
new71.67 KB
FAILED: [[SimpleTest]]: [MySQL] 57,868 pass(es), 13 fail(s), and 804 exception(s).
[ View ]

The test has been fixed.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, locale-convert_locale_translate_page-1978916-75.patch, failed testing.

StatusFileSize
new12.68 KB
new72.49 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch locale-convert_locale_translate_page-1978916-77.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Status:Needs work» Needs review

  1. +++ b/core/modules/locale/lib/Drupal/locale/Controller/LocaleController.php
    @@ -7,10 +7,13 @@
    use Drupal\Core\Controller\ControllerInterface;
    -use Drupal\Core\Routing\PathBasedGeneratorInterface;
    use Drupal\Core\Extension\ModuleHandlerInterface;
    +use Drupal\Core\Routing\PathBasedGeneratorInterface;
    @@ -24,14 +27,26 @@ class LocaleController implements ControllerInterface {
    +   * @param \Drupal\Core\Routing\PathBasedGeneratorInterface $url_generator
    ...
    -  public function __construct(ModuleHandlerInterface $module_handler, PathBasedGeneratorInterface $url_generator) {
    +  public function __construct(ModuleHandlerInterface $module_handler, PathBasedGeneratorInterface $url_generator, ContainerInterface $container) {

    As per #2057155: Add a generateFromRoute() method on the url generator to accept options like url(), this should now be Drupal\Core\Routing\UrlGeneratorInterface, no?

  2. +++ b/core/modules/locale/lib/Drupal/locale/Controller/LocaleController.php
    @@ -69,4 +85,16 @@ public function checkTranslation() {
    +   * Shows the string search screen.
    +   *

    I don't think we should have this @see, that hook will go away...

  3. +++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateEditForm.php
    @@ -0,0 +1,308 @@
    +  protected $translationManager;
    +  ¶
    +  /**
    +   * ¶
    +   * Constructs a new TranslateEditForm object.
    +   *
    +   * @param \Drupal\locale\StringStorageInterface $locale_storage
    +   *   The locale storage.
    +   */
    +  ¶
    +  /**
    +   * Constructs a new TranslateEditForm object.
    +   * ¶

    Several extra line-end whitespaces.

  4. +++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateEditForm.php
    @@ -0,0 +1,308 @@
    +            '#title' => $this->translationManager->translate('Source string (@language)', array('@language' => $this->translationManager->translate('Built-in English'))),
    ...
    +            '#title' => $this->translationManager->translate('Singular form'),
    ...
    +            '#prefix' => '<span class="visually-hidden">' . $this->translationManager->translate('Source string (@language)', array('@language' => $this->translationManager->translate('Built-in English'))) . '</span>'
    ...
    +            '#title' => $this->translationManager->translate('Translated string (@language)', array('@language' => $langname)),
    ...
    +                '#title' => ($i == 0 ? $this->translationManager->translate('Singular form') : format_plural($i, 'First plural form', '@count. plural form')),
    ...
    +                '#prefix' => $i == 0 ? ('<span class="visually-hidden">' . $this->translationManager->translate('Translated string (@language)',  array('@language' => $langname)) . '</span>') : '',
    ...
    +              '#title' => $this->translationManager->translate('Singular form'),
    ...
    +              '#title' => $this->translationManager->translate('Plural form'),
    ...
    +          '#value' => $this->translationManager->translate('Save translations'),
    ...
    +          form_set_error("strings][$lid][translations][$key", $this->translationManager->translate('The submitted string contains disallowed HTML: %string', array('%string' => $value)));
    +          form_set_error("translations][$langcode][$key", $this->translationManager->translate('The submitted string contains disallowed HTML: %string', array('%string' => $value)));
    ...
    +    drupal_set_message($this->translationManager->translate('The strings have been saved.'));
    +++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateFormBase.php
    @@ -0,0 +1,232 @@
    +      'title' => $this->translationManager->translate('String contains'),
    ...
    +      'title' => $this->translationManager->translate('Translation language'),
    ...
    +        'all' => $this->translationManager->translate('Both translated and untranslated strings'),
    +        'translated' => $this->translationManager->translate('Only translated strings'),
    +        'untranslated' => $this->translationManager->translate('Only untranslated strings'),
    ...
    +      'title' => $this->translationManager->translate('Translation type'),
    ...
    +        'all' => $this->translationManager->translate('All'),
    +        LOCALE_NOT_CUSTOMIZED => $this->translationManager->translate('Non-customized translation'),
    +        LOCALE_CUSTOMIZED => $this->translationManager->translate('Customized translation'),

    I've seen elsewhere $this->t() is already on containers/forms? Is that the case? Would be much better.

  5. +++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateFilterForm.php
    @@ -0,0 +1,112 @@
    +      '#title' => t('Filter translatable strings'),
    ...
    +      '#value' => t('Filter'),
    ...
    +        '#value' => t('Reset'),

    Some standalone t()s left around. I honestly don't think leaving the others standalone t() would be fine for now IMHO, we don't need to do all the conversions for all the best practices as once... but if we do conversion for something at least do it consistently.

  6. +++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateFormBase.php
    @@ -0,0 +1,232 @@
    +  public static $filter_values = NULL;
    +  ¶
    ...
    +    }
    +    ¶
    +    return static::$filter_values = $filter_values;

    Extra whitespace.

Status:Needs review» Needs work
Issue tags:+D8MI, +WSCCI-conversion, +language-interface, +LONDON_2013_JULY

The last submitted patch, locale-convert_locale_translate_page-1978916-77.patch, failed testing.

Indeed as per #2018411-34: Figure out a nice DX when working with injected translation using $this->t() is the only good way. If the base class for whatever you are using here does not include that method, just go back to t() the function call and ignore trying to do these replacements. We gotta get this in sooner than later :)

Any controller or form can has $this->t() available to it.

StatusFileSize
new72.24 KB
FAILED: [[SimpleTest]]: [MySQL] 58,096 pass(es), 161 fail(s), and 305 exception(s).
[ View ]

reroll

Status:Needs work» Needs review
StatusFileSize
new21.19 KB
new67.36 KB
FAILED: [[SimpleTest]]: [MySQL] 58,050 pass(es), 225 fail(s), and 169 exception(s).
[ View ]

Massive clean-up:

Extended FormBase
Used $this->container in constructs so don't have to replicate create() everywhere
Did regexp search replace t() -> $this->t()
Dig regexp search replace $this->translationManager->translate -> $this->t()

Even though there are a decent number of function calls in these methods, since we're focusing on getting rid of a new router, creating a translation service can be done in another issue IMHO.

  1. +++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateFormBase.php
    @@ -29,73 +24,31 @@
    +    $this->localeStorage = $this->container->get('locale.storage');

    FormBase is different than ControllerBase, it is not ContainerAware. You'll still need public static function create()

  2. +++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateFormBase.php
    @@ -29,73 +24,31 @@
    -  protected function translateFilterLoadStrings(Request $request) {
    -    $filter_values = $this->translateFilterValues($request);
    +  protected function translateFilterLoadStrings() {
    +    $filter_values = $this->translateFilterValues($this->request);
    @@ -146,10 +96,10 @@ protected function translateFilterValues(Request $request, $reset=FALSE) {
    +      if ($this->request->query->has($key)) {
    ...
    +        $value = $this->request->query->get($key);

    It's actually considered bad practice to store $this->request. Just use $this->getRequest() wherever its needed. (You can't trust that getRequest() or setRequest() has been called yet)

  3. +++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateFormBase.php
    @@ -130,13 +83,10 @@ protected function translateFilterLoadStrings(Request $request) {
    +  protected function translateFilterValues($reset=FALSE) {

    $reset = FALSE

Status:Needs review» Needs work

The last submitted patch, drupal8.locale-module.1978916-88.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new5.56 KB
new68.22 KB
FAILED: [[SimpleTest]]: [MySQL] 57,722 pass(es), 214 fail(s), and 145 exception(s).
[ View ]

attached addresses comments in #90.

Status:Needs review» Needs work

The last submitted patch, drupal8.locale-module.1978916-91.patch, failed testing.

  1. +++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateFormBase.php
    @@ -24,6 +25,13 @@
    +  protected $container;
    @@ -36,10 +44,27 @@
    +  public function __construct(ContainerInterface $container) {
    +    $this->container = $container;

    We should not store the container if at all possible. Also how is it even been passed to the constructor?

  2. +++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateFormBase.php
    @@ -36,10 +44,27 @@
         $this->localeStorage = $this->container->get('locale.storage');

    This should be handled in create()

  3. +++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateFormBase.php
    @@ -36,10 +44,27 @@
    +   * Instantiates a new instance of this form.
    +   *
    +   * This is a factory method that returns a new instance of this object. The
    +   * factory should pass any needed dependencies into the constructor of this
    +   * object, but not the container itself. Every call to this method must retur
    +   * a new instance of this object; that is, it may not implement a singleton.
    +   *
    +   * @param \Symfony\Component\DependencyInjection\ContainerInterface $containe
    +   *   The service container this object should use.

    {@inheritdoc}

  4. +++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateFormBase.php
    @@ -36,10 +44,27 @@
    +  public static function create(ContainerInterface $container) {
    +    return new static(
    +      $container
    +    );
    +  }

    A) This should be right below __construct()
    B) We shouldn't pass container, get the services you need.

The request looks great.

Status:Needs work» Needs review
StatusFileSize
new3 KB
new67.47 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/locale/lib/Drupal/locale/Form/TranslateFormBase.php.
[ View ]

Status:Needs review» Needs work

The last submitted patch, drupal8.locale-module.1978916-94.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.97 KB
new67.58 KB
FAILED: [[SimpleTest]]: [MySQL] 53,628 pass(es), 398 fail(s), and 400 exception(s).
[ View ]

One more try for the night. Completely removing constructor in Controller, and fixing errors in base form.

StatusFileSize
new7.52 KB
new69.08 KB
FAILED: [[SimpleTest]]: [MySQL] 57,496 pass(es), 32 fail(s), and 8 exception(s).
[ View ]

3 times I think people have reviewed this and asked for the switch/case to be removed.
I added "@todo Fix this before commit." to make sure that happens.

+++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateFormBase.php
@@ -132,7 +151,7 @@ protected function translateFilterValues($reset = FALSE) {
-    return static::$filter_values = $filter_values;
+    return static::$filterValues = $filter_values;

Also this is weird.

Status:Needs review» Needs work

The last submitted patch, locale-1978916-96.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new7.23 KB
new69.07 KB
FAILED: [[SimpleTest]]: [MySQL] 58,085 pass(es), 13 fail(s), and 419 exception(s).
[ View ]

split the methods, however; not sure where it needs defined that the submit action for the reset button is different. Also, had some changes locally I was working on, and your interdiff didn't apply over them, so I just manually went through the diff and added the bits I didn't have yet.

Status:Needs review» Needs work

The last submitted patch, drupal8.locale-module.1978916-100.patch, failed testing.

+++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateFilterForm.php
@@ -83,29 +83,22 @@ public function buildForm(array $form, array &$form_state) {
+    $filters = $this->translateFilters();
...
     $filters = $this->translateFilters();

Not sure what this does, and its not used in resetForm.

diff --git a/core/modules/locale/lib/Drupal/locale/Form/TranslateFilterForm.php b/core/modules/locale/lib/Drupal/locale/Form/TranslateFilterForm.php
index c6a36f2..e59c77c 100644
--- a/core/modules/locale/lib/Drupal/locale/Form/TranslateFilterForm.php
+++ b/core/modules/locale/lib/Drupal/locale/Form/TranslateFilterForm.php
@@ -74,6 +74,7 @@ public function buildForm(array $form, array &$form_state) {
       $form['filters']['actions']['reset'] = array(
         '#type' => 'submit',
         '#value' => $this->t('Reset'),
+        '#submit' => array(array($this, 'resetForm')),
       );
     }

is how you get the correct submit.

Status:Needs work» Needs review
StatusFileSize
new942 bytes
new69.08 KB
FAILED: [[SimpleTest]]: [MySQL] 58,560 pass(es), 13 fail(s), and 419 exception(s).
[ View ]

lets try this one. Fixing submit handler and removing the extraneous line in resetForm.

Status:Needs review» Needs work

The last submitted patch, drupal8.locale-module.1978916-103.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.17 KB
new69.11 KB
PASSED: [[SimpleTest]]: [MySQL] 58,436 pass(es).
[ View ]

Redoing my work in #97.

Assigned:likin» Unassigned
StatusFileSize
new74.96 KB
PASSED: [[SimpleTest]]: [MySQL] 58,539 pass(es).
[ View ]

No longer applied. Re-rolled on top of the ContainerInjectionInterface change.

+++ b/core/modules/language/lib/Drupal/language/Form/ContentLanguageSettingsForm.php
@@ -0,0 +1,180 @@
+      '#title' => t('Custom language settings'),
...
+      '#value' => t('Save'),

$this->t

Status:Needs review» Needs work

.

Status:Needs work» Needs review
StatusFileSize
new74.97 KB
PASSED: [[SimpleTest]]: [MySQL] 58,713 pass(es).
[ View ]

Fixed #107.

Status:Needs review» Reviewed & tested by the community
Issue tags:+sprint

I did not review each line but spot-checking and looking for the things I complained about earlier, this looks good to go!

Status:Reviewed & tested by the community» Needs review
Issue tags:+Needs followup
StatusFileSize
new75.19 KB
PASSED: [[SimpleTest]]: [MySQL] 58,714 pass(es).
[ View ]
new6.83 KB

Part of LocaleController::checkTranslation() has no test coverage. The last patch uses $this->redirect with a path, while that only works with route name.

Basically, admin/reports/translations/check is never visited when the "Translation source" (translation.use_source) is set to "Local files only".
Please open a follow-up to test that.

In addition, several variables were only defined inside foreach loops, which can cause bugs in certain circumstances.

Finally, FormBase implements ContainerInjectionInterface already, and this patch tried to implement ControllerInterface.

@tim.plunkett: neither '/check', neither "Translation source", neither "use_source" is mentioned in this patch. Not sure what you observed and how is it related this issue?

Not sure what are the missing pieces, your comment is not clear, and the issue summary is only up to date up to #53 with remaining tasks...

The patch in #109 has

-    return new RedirectResponse($this->urlGenerator->generateFromPath('admin/reports/translations', array('absolute' => TRUE)));
+    return $this->redirect($this->urlGenerator()->generateFromPath('admin/reports/translations', array('absolute' => TRUE)));

And that should be an error. generateFromPath() returns a URL, redirect() expects a route name.
Therefore this conversion touches code that has no test coverage.

All right, are we supposed to write test coverage as well while doing conversions? Last time I heard there is a plan B that the procedural functions will just be wrapped in minimal OO wrappers as a fire-sure way to get the conversions "done" sooner, and then we can fiddle forward with all-ecompassing issues. I personally think it would be more respectful of people's time spent on these issues to let conversions that look fine get in vs. holding it up on more perfection, like writing even more test coverage for stuff. Otherwise looks like the other option is this will go on for a while, then get ignored for the fast track crappy solution and we end up with lots of contributor time wasted and a much worse solution at the end. I'm a big fan of moving ahead with smaller steps and not trying to cover everything at once. It is not just more sensible for people so they have some success (this issue has been worked on for almost 5 months) but also more sensible for core progress.

That's why I tagged it "Needs followup", not "Needs tests". I'm not a monster ;)

Status:Needs review» Reviewed & tested by the community

Right, I asked about tasks remaining. So looks like none for this issue.

Issue tags:-Needs followup

Issue summary:View changes

added remaining tasks.

Status:Reviewed & tested by the community» Fixed

Awesome work on this, folks!

Committed and pushed to 8.x. Thanks!

Issue tags:-sprint

Woot, thanks!

I went over this patch but could not find any place ContentLanguageSettingsForm is actually used. I didn't see any discussion on that here, either. Can anyone comment on that?

Status:Fixed» Needs review
StatusFileSize
new4.19 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch language-1978916-124.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Um, yeah, that's weird. I didn't even notice when I was reviewing, that's embarrassing.

Status:Needs review» Needs work
Issue tags:-D8MI, -language-ui, -WSCCI-conversion, -LONDON_2013_JULY

The last submitted patch, language-1978916-124.patch, failed testing.

Status:Needs work» Needs review

#124: language-1978916-124.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+D8MI, +language-ui, +WSCCI-conversion, +LONDON_2013_JULY

The last submitted patch, language-1978916-124.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.23 KB
FAILED: [[SimpleTest]]: [MySQL] 58,428 pass(es), 25 fail(s), and 635 exception(s).
[ View ]
new1.18 KB

Straight reroll.
Interdiff is between patch files with diff -u.

Status:Needs review» Needs work

The last submitted patch, language-1978916-128.patch, failed testing.

+++ b/core/modules/language/language.admin.inc
@@ -342,96 +343,7 @@ function theme_language_negotiation_configure_browser_form_table($variables) {
function language_content_settings_page() {

We have to remove this function as well and move it to controller.

Status:Needs work» Needs review
StatusFileSize
new5.42 KB
new8.59 KB
FAILED: [[SimpleTest]]: [MySQL] 58,562 pass(es), 0 fail(s), and 4 exception(s).
[ View ]

I did some work on this patch cleaning it up some more, changing the router and the test case to use the new configuration based form.

I ran into a problem I couldn't quite solve. It seems somethings language_configuration_element_submit doesn't get $form passed by refference in the batch job. Changing the function signature or what _batch_set_next does seems breaks the config form.

I'm hoping some one can fix the notices.

Setting to needs review to have the tests run.

Status:Needs review» Needs work

The last submitted patch, language-1978916-131.patch, failed testing.

The mess is going from this place https://drupal.org/node/1978916#comment-7865513. It overlaps with another issue https://drupal.org/node/1987726.

We do not need touch language_content_settings_page here.

Status:Needs work» Closed (fixed)

Closing this. This patch was already committed, and all the ContentLanguageSettingsForm stuff is being done in #1987726: Convert language content page related callback to new style controller.

Issue summary:View changes

Update remaining tasks