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.

CommentFileSizeAuthor
#131 language-1978916-131.patch8.59 KBgoogletorp
#131 interdiff.txt5.42 KBgoogletorp
#128 interdiff-patches.124-128.interdiff.txt1.18 KBpenyaskito
#128 language-1978916-128.patch4.23 KBpenyaskito
#124 language-1978916-124.patch4.19 KBtim.plunkett
#111 interdiff.txt6.83 KBtim.plunkett
#111 locale-1978916-111.patch75.19 KBtim.plunkett
#109 locale-1978916-109.patch74.97 KBLetharion
#106 locale-1978916-106.patch74.96 KBLetharion
#105 locale-1978916-105.patch69.11 KBtim.plunkett
#105 interdiff.txt3.17 KBtim.plunkett
#103 drupal8.locale-module.1978916-103.patch69.08 KBdisasm
#103 interdiff.txt942 bytesdisasm
#100 drupal8.locale-module.1978916-100.patch69.07 KBdisasm
#100 interdiff.txt7.23 KBdisasm
#97 locale-1978916-96.patch69.08 KBtim.plunkett
#97 interdiff.txt7.52 KBtim.plunkett
#96 drupal8.locale-module.1978916-96.patch67.58 KBdisasm
#96 interdiff.txt1.97 KBdisasm
#94 drupal8.locale-module.1978916-94.patch67.47 KBdisasm
#94 interdiff.txt3 KBdisasm
#91 drupal8.locale-module.1978916-91.patch68.22 KBdisasm
#91 interdiff.txt5.56 KBdisasm
#88 drupal8.locale-module.1978916-88.patch67.36 KBdisasm
#88 interdiff.txt21.19 KBdisasm
#87 drupal8.locale-module.1978916-87.patch72.24 KBdisasm
#80 locale-convert_locale_translate_page-1978916-77.patch72.49 KBvictor-shelepen
#80 interdiff-1978916-75-77.txt12.68 KBvictor-shelepen
#77 locale-convert_locale_translate_page-1978916-75.patch71.67 KBvictor-shelepen
#77 interdiff-1978916-75-77.txt12.68 KBvictor-shelepen
#75 locale-convert_locale_translate_page-1978916-75.patch71.67 KBvictor-shelepen
#75 interdiff-1978916-66-1978916-75.txt6.72 KBvictor-shelepen
#66 locale-convert_locale_translate_page-1978916-66.patch70.36 KBvictor-shelepen
#66 interdiff-1978916-61-1978916-66.txt8.99 KBvictor-shelepen
#61 locale-convert_locale_translate_page-1978916-61.patch70.21 KBvictor-shelepen
#61 1978916-interdiff-59-61.txt17.91 KBvictor-shelepen
#59 1978916-locale-convert_locale_translate_page-59.patch51.83 KBvictor-shelepen
#59 interdiff-1978916#55-1978916#59.txt1.39 KBvictor-shelepen
#56 interdiff-52-55.txt1.65 KBvictor-shelepen
#55 1978916-locale-convert_locale_translate_page-55.patch51.79 KBvictor-shelepen
#55 1978916-locale-convert_locale_translate_page-55.patch51.79 KBvictor-shelepen
#52 1978916-locale-convert_locale_translate_page-52.patch51.83 KBYesCT
#52 interdiff-51-52.txt5.2 KBYesCT
#51 1978916-locale-convert_locale_translate_page-51.patch50.79 KBYesCT
#51 interdiff-49-51.txt7.97 KBYesCT
#49 1978916-locale-convert_locale_translate_page-49.patch50.47 KBYesCT
#49 interdiff-47-49.txt1.8 KBYesCT
#47 1978916-locale-convert_locale_translate_page-47.patch49.73 KBYesCT
#47 interdiff-45-47.txt3.72 KBYesCT
#45 1978916-locale-convert_locale_translate_page-45.patch49.63 KBYesCT
#45 interdiff-44-45.txt3.35 KBYesCT
#44 1978916-locale-convert_locale_translate_page-44.patch52.97 KBYesCT
#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
#39 1978916-diff-33-39.txt11.54 KBvijaycs85
#33 1978916-locale-convert_locale_translate_page-33.patch47.46 KBvijaycs85
#27 locale-convert_locale_translate_page-1978916-27.patch47.46 KBvictor-shelepen
#27 interdiff.txt12.33 KBvictor-shelepen
#24 locale-convert_locale_translate_page-1978916-24.patch63.58 KBvictor-shelepen
#21 locale-convert_locale_translate_page-1978916-21.patch5.84 KBvictor-shelepen
#20 locale-convert_locale_translate_page-1978916-20.patch63.46 KBvictor-shelepen
#15 1978916-convert_locale_translate_page.patch62.48 KBayelet_Cr
#13 1978916-convert_locale_translate_page.patch62.48 KBayelet_Cr
#8 1978916-convert_locale_translate_page.patch35.66 KBayelet_Cr
#5 1978916-convert_locale_translate_page.patch35.59 KBayelet_Cr
#3 1978916-convert_locale_translate_page.patch35.58 KBayelet_Cr
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ayelet_Cr’s picture

Assigned: Unassigned » ayelet_Cr
Gábor Hojtsy’s picture

Issue tags: +D8MI
ayelet_Cr’s picture

Status: Active » Needs review
FileSize
35.58 KB

Status: Needs review » Needs work

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

ayelet_Cr’s picture

Status: Needs work » Needs review
FileSize
35.59 KB

Status: Needs review » Needs work

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

Gábor Hojtsy’s picture

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 :)
ayelet_Cr’s picture

Status: Needs work » Needs review
FileSize
35.66 KB

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

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

Dave.Ingram’s picture

Status: Needs work » Needs review
Issue tags: +D8MI, +WSCCI-conversion, +language-interface
Gábor Hojtsy’s picture

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!

tim.plunkett’s picture

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.

ayelet_Cr’s picture

Status: Needs work » Needs review
FileSize
62.48 KB

Status: Needs review » Needs work

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

ayelet_Cr’s picture

Status: Needs work » Needs review
FileSize
62.48 KB

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

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

vijaycs85’s picture

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.

dawehner’s picture

Issue tags: +Needs reroll
victor-shelepen’s picture

The rerolled patch #15.

victor-shelepen’s picture

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

victor-shelepen’s picture

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

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

return \Drupal::service('locale.storage')->getTranslations($conditions, $options);

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

tim.plunkett’s picture

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

victor-shelepen’s picture

The whole patch.

tim.plunkett’s picture

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?

YesCT’s picture

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

victor-shelepen’s picture

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

victor-shelepen’s picture

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.

tim.plunkett’s picture

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.

victor-shelepen’s picture

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

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
47.46 KB

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

vijaycs85’s picture

Issue tags: +LONDON_2013_JULY

Status: Needs review » Needs work

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

vijaycs85’s picture

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

@vijaycs85 Attach interdiff.

dawehner’s picture

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

vijaycs85’s picture

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

YesCT’s picture

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

YesCT’s picture

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

YesCT’s picture

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.

YesCT’s picture

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

YesCT’s picture

Status: Needs work » Needs review
FileSize
3.72 KB
49.73 KB
  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.

YesCT’s picture

Status: Needs work » Needs review
FileSize
1.8 KB
50.47 KB

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.

YesCT’s picture

Status: Needs work » Needs review
FileSize
7.97 KB
50.79 KB

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

YesCT’s picture

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.

victor-shelepen’s picture

/**
 * Defines a translation edit form.
 */
class TranslateEditForm extends TranslateFormBase implements FormInterface {

Shoud be changed to :

/**
 * Defines a translation edit form.
 */
class TranslateEditForm extends TranslateFormBase {

TranslateFormBase already containes FormInterface.

victor-shelepen’s picture

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

victor-shelepen’s picture

FileSize
1.65 KB
victor-shelepen’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

victor-shelepen’s picture

Status: Needs work » Needs review
FileSize
1.39 KB
51.83 KB

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.

victor-shelepen’s picture

victor-shelepen’s picture

Status: Needs work » Needs review
dawehner’s picture

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

victor-shelepen’s picture

Assigned: Unassigned » victor-shelepen
YesCT’s picture

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

victor-shelepen’s picture

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
Crell’s picture

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.

victor-shelepen’s picture

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

victor-shelepen’s picture

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),
    );
  }
victor-shelepen’s picture

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.

victor-shelepen’s picture

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.

tim.plunkett’s picture

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

victor-shelepen’s picture

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

tim.plunkett’s picture

No, from ContainerAware to ControllerInterface

victor-shelepen’s picture

Status: Needs work » Needs review
FileSize
6.72 KB
71.67 KB

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.

victor-shelepen’s picture

victor-shelepen’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

victor-shelepen’s picture

victor-shelepen’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

  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.

Gábor Hojtsy’s picture

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.

Gábor Hojtsy’s picture

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

tim.plunkett’s picture

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

disasm’s picture

reroll

disasm’s picture

Status: Needs work » Needs review
FileSize
21.19 KB
67.36 KB

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.

tim.plunkett’s picture

  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.

disasm’s picture

Status: Needs work » Needs review
FileSize
5.56 KB
68.22 KB

attached addresses comments in #90.

Status: Needs review » Needs work

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

tim.plunkett’s picture

  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.

disasm’s picture

Status: Needs work » Needs review
FileSize
3 KB
67.47 KB

Status: Needs review » Needs work

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

disasm’s picture

Status: Needs work » Needs review
FileSize
1.97 KB
67.58 KB

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

tim.plunkett’s picture

FileSize
7.52 KB
69.08 KB

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.

tim.plunkett’s picture

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

disasm’s picture

Status: Needs work » Needs review
FileSize
7.23 KB
69.07 KB

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.

tim.plunkett’s picture

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

disasm’s picture

Status: Needs work » Needs review
FileSize
942 bytes
69.08 KB

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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
3.17 KB
69.11 KB

Redoing my work in #97.

Letharion’s picture

Assigned: victor-shelepen » Unassigned
FileSize
74.96 KB

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

jibran’s picture

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

$this->t

dawehner’s picture

Status: Needs review » Needs work

.

Letharion’s picture

Status: Needs work » Needs review
FileSize
74.97 KB

Fixed #107.

Gábor Hojtsy’s picture

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!

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs followup
FileSize
75.19 KB
6.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.

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

CaDyMaN’s picture

tim.plunkett’s picture

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.

Gábor Hojtsy’s picture

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.

tim.plunkett’s picture

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

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

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

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

Issue tags: -Needs followup
Gábor Hojtsy’s picture

Issue summary: View changes

added remaining tasks.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Awesome work on this, folks!

Committed and pushed to 8.x. Thanks!

Gábor Hojtsy’s picture

Issue tags: -sprint

Woot, thanks!

tstoeckler’s picture

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?

tim.plunkett’s picture

Status: Fixed » Needs review
FileSize
4.19 KB

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.

penyaskito’s picture

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.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
4.23 KB
1.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.

jibran’s picture

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

googletorp’s picture

Status: Needs work » Needs review
FileSize
5.42 KB
8.59 KB

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.

victor-shelepen’s picture

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

victor-shelepen’s picture

We do not need touch language_content_settings_page here.

disasm’s picture

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.

disasm’s picture

Issue summary: View changes

Update remaining tasks

cilefen’s picture