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.

Files: 
CommentFileSizeAuthor
#57 locale-import_controller-1978918-53-57.txt729 byteslikin
#57 locale-import_controller-1978918-57.patch15.86 KBlikin
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,992 pass(es).
[ View ]

Comments

Assigned:Unassigned» Pancho
Status:Active» Needs review
StatusFileSize
new11.5 KB
FAILED: [[SimpleTest]]: [MySQL] 55,792 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, locale_translate_import_form_controller-1978918-1.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2 KB
new12.7 KB
FAILED: [[SimpleTest]]: [MySQL] 55,656 pass(es), 130 fail(s), and 13 exception(s).
[ View ]

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

[edit:] Please ignore wrong interdiff.

StatusFileSize
new2.3 KB

Here's the correct interdiff for #3.

Issue tags:+D8MI, +WSCCI-conversion

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

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

This feels wrong.

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

No need for that so far.

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

The last submitted patch, locale_translate_import_form_controller-1978918-3.patch, failed testing.

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

Status:Needs work» Needs review
StatusFileSize
new16.8 KB
FAILED: [[SimpleTest]]: [MySQL] 58,056 pass(es), 30 fail(s), and 0 exception(s).
[ View ]

Re-rolling with fixes for #6

Thanks!

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

The last submitted patch, 1978918-locale_translate_import_form_controller-10.patch, failed testing.

Status:Needs work» Needs review

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

The last submitted patch, 1978918-locale_translate_import_form_controller-10.patch, failed testing.

Issue tags:+Needs reroll

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

Status:Needs work» Needs review
StatusFileSize
new6.12 KB
new11.91 KB
FAILED: [[SimpleTest]]: [MySQL] 58,904 pass(es), 139 fail(s), and 53 exception(s).
[ View ]

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

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

The last submitted patch, drupal8.locale-module.1978918-17.patch, failed testing.

Status:Needs work» Needs review

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

The last submitted patch, drupal8.locale-module.1978918-17.patch, failed testing.

Assigned:Pancho» disasm
Status:Needs work» Needs review
StatusFileSize
new562 bytes
new11.96 KB
FAILED: [[SimpleTest]]: [MySQL] 58,588 pass(es), 139 fail(s), and 3 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, drupal8.locale-module.1978918-21.patch, failed testing.

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

Some minor issues.

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

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

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

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

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

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

Issue tags:+language-ui

Assigned:disasm» Luxian

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

Status:Needs review» Needs work
StatusFileSize
new0 bytes
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new1.97 KB

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

Did the following:

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

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

Status:Needs work» Needs review

Forgot to set status.

@Luxian, it is empty patch file :)

Status:Needs work» Needs review
StatusFileSize
new2.65 KB
new13.09 KB
PASSED: [[SimpleTest]]: [MySQL] 58,911 pass(es).
[ View ]

Sorry for the empty patch. This is the right one

Status:Needs review» Needs work

The last submitted patch, drupal.locale.update_import_form-1978918-30.patch, failed testing.

I've applied the patch, checked the wrong test - ViewEditTest localy. It works.

Tests to be run:
- General views edit test (\Drupal\views_ui\Tests\ViewEditTest)
Test run started:
Sunday, September 29, 2013 - 11:36
Test summary
------------
General views edit test 48 passes, 0 fails, 0 exceptions, and 10 debug messages

Status:Needs work» Needs review

Issue tags:-Needs reroll

Tags

It does not need re-rolling. It is applied well.

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

    Order these alphabetically.

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

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

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

    Write the type first, then the variable name.

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

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

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

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

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

-- wrong: please delete --

Issue summary:View changes
StatusFileSize
new11.97 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal.locale.update_import_form-1978918-37.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new4.24 KB

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

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 38: drupal.locale.update_import_form-1978918-37.patch, failed testing.

StatusFileSize
new12.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal.locale.update_import_form-1978918-41.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Re-roll

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 41: drupal.locale.update_import_form-1978918-41.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new11.83 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,602 pass(es).
[ View ]
new872 bytes

Reroll, the theme function moved to the render-able array.

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

    should be 'locale_translate_import_form' also remove old one

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

    should be injected like module handler

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

    deprecated, use getLanguages()

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

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

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

    is not needed

StatusFileSize
new12.1 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,598 pass(es).
[ View ]
new2.46 KB

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

    old form should be removed

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

    missing extra blank line at the end of the class

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

    ?!

StatusFileSize
new430.14 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch locale-import_controller-1978918-49.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new5.04 KB

Status:Needs review» Needs work

The last submitted patch, 49: locale-import_controller-1978918-49.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new15.74 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,682 pass(es).
[ View ]
new5.04 KB

Assigned:Luxian» Unassigned
Status:Needs review» Needs work
Issue tags:+Needs reroll

I'm sad this does not apply anymore :(

Status:Needs work» Needs review
StatusFileSize
new15.87 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,774 pass(es), 164 fail(s), and 76 exception(s).
[ View ]

Patch has been rerolled. Manul testing went not bad. Look problem tests.

Status:Needs review» Needs work

The last submitted patch, 53: locale-import_controller-1978918-53.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new0 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,076 pass(es).
[ View ]

0 bytes

Please upload a patch

StatusFileSize
new15.86 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,992 pass(es).
[ View ]
new729 bytes

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

    we do have a interface for the language manager.

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

    Missing docs + language manager interface

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

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

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

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

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

    Isn't that js file already a library?

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

    language load is languageManager->getLanguage

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

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