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
#67 locale-import_controller-1978918-67.patch12.61 KBlikin
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,861 pass(es).
[ View ]

Comments

Pancho’s picture

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.

Pancho’s picture

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.

Pancho’s picture

StatusFileSize
new2.3 KB

Here's the correct interdiff for #3.

Pancho’s picture

Issue tags:+D8MI, +WSCCI-conversion

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

dawehner’s picture

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

kim.pepper’s picture

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.

Pancho’s picture

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

vijaycs85’s picture

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

Pancho’s picture

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.

vijaycs85’s picture

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.

jair’s picture

Issue tags:+Needs reroll
xjm’s picture

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

disasm’s picture

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.

disasm’s picture

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.

disasm’s picture

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.

disasm’s picture

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.

jibran’s picture

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.

Gábor Hojtsy’s picture

Issue tags:+language-ui
Luxian’s picture

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.

Luxian’s picture

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.

Luxian’s picture

Status:Needs work» Needs review

Forgot to set status.

vijaycs85’s picture

@Luxian, it is empty patch file :)

Luxian’s picture

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.

likin’s picture

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

likin’s picture

Status:Needs work» Needs review
mcrittenden’s picture

Issue tags:-Needs reroll

Tags

likin’s picture

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

pfrenssen’s picture

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.

Luxian’s picture

-- wrong: please delete --

Luxian’s picture

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

Luxian’s picture

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.

Luxian’s picture

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

Luxian’s picture

Status:Needs work» Needs review
kbasarab’s picture

Status:Needs review» Needs work

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

likin’s picture

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.

andypost’s picture

  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

likin’s picture

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

  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'),

    ?!

likin’s picture

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.

likin’s picture

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

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

I'm sad this does not apply anymore :(

likin’s picture

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.

likin’s picture

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

0 bytes

Please upload a patch

likin’s picture

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

  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.

likin’s picture

StatusFileSize
new15.81 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,045 pass(es).
[ View ]
new2.9 KB

1,2,3 - Resolved.
4 . It seems a deprecated function. There is not a mark that it will become deprecated. https://api.drupal.org/api/drupal/core!modules!language!language.admin.i...
5. Yes. It works without that code.
6,7 - Resolved.

vijaycs85’s picture

StatusFileSize
new12.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,043 pass(es).
[ View ]
new5.22 KB

More updates:

1. Removed the temporary callback for import at core/modules/locale/lib/Drupal/locale/Form/LocaleForm.php
2. Removed procedural form definition and it's submit handlers.

vijaycs85’s picture

StatusFileSize
new12.48 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,588 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new1.07 KB

Adding docblock for constructor.

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

Thank you. All additional function calls can be done later.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 61: 1978918-locale_import_form-61.patch, failed testing.

penyaskito’s picture

penyaskito’s picture

Status:Needs work» Reviewed & tested by the community

Back to RTBC, the bot will turn back to need works if it fails. There has been some error with them, not related with the patch.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 61: 1978918-locale_import_form-61.patch, failed testing.

likin’s picture

Status:Needs work» Needs review
StatusFileSize
new12.61 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,861 pass(es).
[ View ]
new3.29 KB

To fix the last test I've moved the validation logic from submitForm to validationForm.

LogicException: Form errors cannot be set after form validation has finished.

andypost’s picture

Status:Needs review» Reviewed & tested by the community

Great

penyaskito’s picture

Issue tags:-Needs reroll

@likin w00t, thanks! +1 RTBC.
Removed "Needs reroll" tag.

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed 4b172c7 and pushed to 8.x. Thanks!

+++ b/core/modules/locale/lib/Drupal/locale/Form/ImportForm.php
@@ -0,0 +1,197 @@
+    form_load_include($form_state, 'inc', 'language', 'language.admin');
...
+      $language_options = language_admin_predefined_list();
...
+        $this->t('Languages not yet added') => language_admin_predefined_list()

Can we get an follow up to move language_admin_predefined_list() - this was mentioned in #58

  • Commit 4b172c7 on 8.x by alexpott:
    Issue #1978918 by likin, Luxian, vijaycs85, Pancho, disasm: Convert...

Status:Fixed» Closed (fixed)

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