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.

CommentFileSizeAuthor
#67 locale-import_controller-1978918-61-67.txt3.29 KBvictor-shelepen
#67 locale-import_controller-1978918-67.patch12.61 KBvictor-shelepen
#61 1978918-diff-60-61.txt1.07 KBvijaycs85
#61 1978918-locale_import_form-61.patch12.48 KBvijaycs85
#60 1978918-diff-59-60.txt5.22 KBvijaycs85
#60 1978918-locale_import_form-60.patch12.4 KBvijaycs85
#59 locale-import_controller-1978918-57-59.txt2.9 KBvictor-shelepen
#59 locale-import_controller-1978918-59.patch15.81 KBvictor-shelepen
#57 locale-import_controller-1978918-53-57.txt729 bytesvictor-shelepen
#57 locale-import_controller-1978918-57.patch15.86 KBvictor-shelepen
#55 locale-import_controller-1978918-55.patch0 bytesvictor-shelepen
#53 locale-import_controller-1978918-53.patch15.87 KBvictor-shelepen
#51 locale-import_controller-1978918-47-49.txt5.04 KBvictor-shelepen
#51 locale-import_controller-1978918-51.patch15.74 KBvictor-shelepen
#49 locale-import_controller-1978918-47-49.txt5.04 KBvictor-shelepen
#49 locale-import_controller-1978918-49.patch430.14 KBvictor-shelepen
#47 locale-import_controller-1978918-45-47.txt2.46 KBvictor-shelepen
#47 locale-import_controller-1978918-47.patch12.1 KBvictor-shelepen
#45 locale-import_controller-1978918-41-45.txt872 bytesvictor-shelepen
#45 locale-import_controller-1978918-45.patch11.83 KBvictor-shelepen
#41 drupal.locale.update_import_form-1978918-41.patch12.07 KBLuxian
#38 interdiff.txt4.24 KBLuxian
#38 drupal.locale.update_import_form-1978918-37.patch11.97 KBLuxian
#30 drupal.locale.update_import_form-1978918-30.patch13.09 KBLuxian
#30 interdiff.txt2.65 KBLuxian
#27 interdiff.txt1.97 KBLuxian
#27 drupal.locale.update_import_form-1978918-27.patch0 bytesLuxian
#21 drupal8.locale-module.1978918-21.patch11.96 KBdisasm
#21 interdiff.txt562 bytesdisasm
#17 drupal8.locale-module.1978918-17.patch11.91 KBdisasm
#17 interdiff.txt6.12 KBdisasm
#10 1978918-locale_translate_import_form_controller-10.patch16.8 KBvijaycs85
#4 interdiff.txt2.3 KBPancho
#3 locale_translate_import_form_controller-1978918-3.patch12.7 KBPancho
#3 interdiff.txt2 KBPancho
#1 locale_translate_import_form_controller-1978918-1.patch11.5 KBPancho
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Pancho’s picture

Assigned: Unassigned » Pancho
Status: Active » Needs review
FileSize
11.5 KB

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
FileSize
2 KB
12.7 KB

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

FileSize
2.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
FileSize
16.8 KB

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
FileSize
6.12 KB
11.91 KB

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
FileSize
562 bytes
11.96 KB

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
FileSize
0 bytes
1.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
FileSize
2.65 KB
13.09 KB

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.

victor-shelepen’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
victor-shelepen’s picture

Status: Needs work » Needs review
mcrittenden’s picture

Issue tags: -Needs reroll

Tags

victor-shelepen’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

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

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.

victor-shelepen’s picture

Status: Needs work » Needs review
FileSize
11.83 KB
872 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

victor-shelepen’s picture

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

    ?!

victor-shelepen’s picture

Status: Needs review » Needs work

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

victor-shelepen’s picture

Status: Needs work » Needs review
FileSize
15.74 KB
5.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 :(

victor-shelepen’s picture

Status: Needs work » Needs review
FileSize
15.87 KB

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.

victor-shelepen’s picture

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

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.

victor-shelepen’s picture

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

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

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.

victor-shelepen’s picture

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

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...
alexpott’s picture

Status: Fixed » Closed (fixed)

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