Convert the form builder system_modules_uninstall() to a new-style Controller, using the instructions in the WSCCI Conversion Guide.

This issue also refactors the existing form to properly leverage ConfirmFormBase as a separate multi-step form step. Previously we only had a single form with a built-in switch for displaying the confirm form (which was bad). We are using a temporary (expirable) key value store entry for accomplishing this multi-step behavior. The tempstore is not used, because it also provides a lock, which is not wanted here.

Files: 
CommentFileSizeAuthor
#37 interdiff.txt4.06 KBfubhy
#37 1993202-37.patch19.67 KBfubhy
PASSED: [[SimpleTest]]: [MySQL] 57,042 pass(es).
[ View ]
#29 1993202-29.patch19.65 KBfubhy
PASSED: [[SimpleTest]]: [MySQL] 56,933 pass(es).
[ View ]
#26 1993202-26.patch19.59 KBfubhy
PASSED: [[SimpleTest]]: [MySQL] 56,986 pass(es).
[ View ]
#26 interdiff.txt5.24 KBfubhy
#20 1993202-20.patch19.67 KBfubhy
PASSED: [[SimpleTest]]: [MySQL] 56,991 pass(es).
[ View ]
#20 interdiff.txt5.91 KBfubhy
#17 1993202-17.patch19.24 KBfubhy
PASSED: [[SimpleTest]]: [MySQL] 56,961 pass(es).
[ View ]
#15 1993202-15.patch19.16 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] 57,026 pass(es), 0 fail(s), and 134 exception(s).
[ View ]
#14 1993202-14.patch19.52 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] 56,955 pass(es), 128 fail(s), and 538 exception(s).
[ View ]
#14 interdiff.txt13.23 KBfubhy
#12 interdiff-1993202.txt2.92 KBh3rj4n
#12 drupal-system_modules_uninstall_form_controller-1993202-12.patch16.88 KBh3rj4n
PASSED: [[SimpleTest]]: [MySQL] 58,078 pass(es).
[ View ]
#11 drupal-system_modules_uninstall_form_controller-1993202-11.patch18.26 KBh3rj4n
PASSED: [[SimpleTest]]: [MySQL] 57,989 pass(es).
[ View ]
#9 drupal-system_modules_uninstall_form_controller-1993202-9.patch16.13 KBh3rj4n
FAILED: [[SimpleTest]]: [MySQL] 56,452 pass(es), 274 fail(s), and 49 exception(s).
[ View ]
#9 interdiff-1993202.txt6.43 KBh3rj4n
#6 drupal-system_modules_uninstall_form_controller-1993202-6.patch13.54 KBh3rj4n
PASSED: [[SimpleTest]]: [MySQL] 57,946 pass(es).
[ View ]
#3 system_modules_uninstall_form_controller-1993202-3.patch10.98 KBPancho
PASSED: [[SimpleTest]]: [MySQL] 56,040 pass(es).
[ View ]
#3 interdiff.txt2 KBPancho
#1 system_modules_uninstall_form_controller-1993202-1.patch11.2 KBPancho
PASSED: [[SimpleTest]]: [MySQL] 55,798 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new11.2 KB
PASSED: [[SimpleTest]]: [MySQL] 55,798 pass(es).
[ View ]

Here's the patch.
I needed to tweak the entry in system_menu() a bit because it would otherwise inherit old-school page and access callbacks from its parent. Seems like a bug in the new WSCCI implementation, but is okay for now and won't break.
The rest is straightforward, except for the actual confirm form that I left out because it is covered in #1946454: Convert all confirm_form() in system.module and system.admin.inc to the new form interface and convert route.
From my testing it seems to work, so now let's see what the testbot says.

+++ b/core/modules/system/lib/Drupal/system/Form/ModulesUninstallForm.phpundefined
@@ -0,0 +1,141 @@
+class ModulesUninstallForm implements FormInterface, ControllerInterface {
...
+  /**
+   * {@inheritdoc}
+   */
+  public static function create(ContainerInterface $container) {
+    return new static();
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function __construct() {

I don't think we need a controllerinterface here.

+++ b/core/modules/system/lib/Drupal/system/Form/ModulesUninstallForm.phpundefined
@@ -0,0 +1,141 @@
+      drupal_goto('admin/modules/uninstall');

Can't you use $form_state['redirect'] instead of drupal_goto?

+++ b/core/modules/system/system.moduleundefined
@@ -771,17 +771,16 @@ function system_menu() {
+    'route name' => 'system_modules_uninstall',
...
+    'route name' => 'system_modules_uninstall_confirm',

Isn't it "route_name" instead of "route name"?

+++ b/core/modules/system/system.moduleundefined
@@ -771,17 +771,16 @@ function system_menu() {
+    'page callback' => 'USES ROUTE',
+    'access callback' => TRUE,

You don't have to set these values if you have defined a route.

StatusFileSize
new2 KB
new10.98 KB
PASSED: [[SimpleTest]]: [MySQL] 56,040 pass(es).
[ View ]

Thanks for your review, Daniel!

I don't think we need a controllerinterface here.

You're right. Removed.

Can't you use $form_state['redirect'] instead of drupal_goto?

Sure, we can. I just didn't touch the code more than necessary in this conversion issue.

Isn't it "route_name" instead of "route name"?

That's true, leaving out the underscore also lead to 'page callback' and 'access callback' not being unset, and that's why it seemed necessary to explicitely set them:

+ 'page callback' => 'USES ROUTE',
+ 'access callback' => TRUE,

What I thought was a bug with the new routing system turns out to be my fault.

Hope, everything is fine now.

This looks pretty good so far!

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

All that is not needed ... I should have made it clear.

+++ b/core/modules/system/lib/Drupal/system/Form/ModulesUninstallForm.phpundefined
@@ -0,0 +1,140 @@
+      \Drupal::moduleHandler()->loadInclude('system', 'inc', 'system.admin');
...
+      \Drupal::moduleHandler()->loadInclude('system', 'inc', 'system.admin');

One could argue that you actually want to inject the module handler to load this includes, but yeah I don't really care.

Status:Needs review» Needs work

But in order to inject the module handler service, we do need create() and __construct(), ControllerInterface, ContainerInterface plus ModuleHandlerInterface, don't we?

Assigned:Pancho» h3rj4n
StatusFileSize
new13.54 KB
PASSED: [[SimpleTest]]: [MySQL] 57,946 pass(es).
[ View ]

Taking over because of inactivity.

Recreated the patch on the current Drupal HEAD.

Status:Needs work» Needs review

+++ b/core/modules/system/lib/Drupal/system/Form/ModulesUninstallForm.phpundefined
@@ -0,0 +1,140 @@
+
+  /**
+   * {@inheritdoc}
+   */
+  public function __construct() {
+  }

That's redundant.

+++ b/core/modules/system/system.admin.incundefined
@@ -764,110 +764,96 @@ function system_modules_submit($form, &$form_state) {
-    }
+function system_modules_uninstall_confirm_form($storage) {
+  // Nothing to build.
+  if (empty($storage)) {
+    return;
   }
-  // Only build the rest of the form if there are any modules available to
-  // uninstall.
-  if (!empty($disabled_modules)) {
-    $profile = drupal_get_profile();
-    uasort($disabled_modules, 'system_sort_modules_by_info_name');
-    $form['uninstall'] = array('#tree' => TRUE);
-    foreach ($disabled_modules as $module) {
-      $module_name = $module->info['name'] ? $module->info['name'] : $module->name;
-      $form['modules'][$module->name]['#module_name'] = $module_name;
-      $form['modules'][$module->name]['name']['#markup'] = $module_name;
-      $form['modules'][$module->name]['description']['#markup'] = t($module->info['description']);
-      $form['uninstall'][$module->name] = array(
-        '#type' => 'checkbox',
-        '#title' => t('Uninstall @module module', array('@module' => $module_name)),
-        '#title_display' => 'invisible',
-      );
-      // All modules which depend on this one must be uninstalled first, before
-      // we can allow this module to be uninstalled. (The installation profile
-      // is excluded from this list.)
-      foreach (array_keys($module->required_by) as $dependent) {
-        if ($dependent != $profile && drupal_get_installed_schema_version($dependent) != SCHEMA_UNINSTALLED) {
-          $dependent_name = isset($all_modules[$dependent]->info['name']) ? $all_modules[$dependent]->info['name'] : $dependent;
-          $form['modules'][$module->name]['#required_by'][] = $dependent_name;
-          $form['uninstall'][$module->name]['#disabled'] = TRUE;
-        }
-      }
-    }
-    $form['actions'] = array('#type' => 'actions');
-    $form['actions']['submit'] = array(
-      '#type' => 'submit',
-      '#value' => t('Uninstall'),
+  // Construct the hidden form elements and list items.
+  foreach (array_filter($storage['uninstall']) as $module => $value) {
+    $info = drupal_parse_info_file(drupal_get_path('module', $module) . '/' . $module . '.info.yml');
+    $uninstall[] = $info['name'];
+    $form['uninstall'][$module] = array('#type' => 'hidden',
+      '#value' => 1,
     );
-    $form['#action'] = url('admin/modules/uninstall/confirm');
-  }
-  else {
-    $form['modules'] = array();
   }
-  return $form;
+  // Display a confirm form if modules have been selected.
+  if (isset($uninstall)) {
+    $form['#confirmed'] = TRUE;
+    $form['uninstall']['#tree'] = TRUE;
+    $form['text'] = array('#markup' => '<p>' . t('The following modules will be completely uninstalled from your site, and <em>all data from these modules will be lost</em>!') . '</p>');
+    $form['modules'] = array('#theme' => 'item_list', '#items' => $uninstall);
+    $form = confirm_form(
+      $form,
+      t('Confirm uninstall'),
+      'admin/modules/uninstall',
+      t('Would you like to continue with uninstalling the above?'),
+      t('Uninstall'),
+      t('Cancel'));
+    return $form;
+  }

This needs to get removed. We need a separate form controller for the confirm form. Make use of ConfirmFormBase.

+++ b/core/modules/system/system.admin.incundefined
@@ -764,110 +764,96 @@ function system_modules_submit($form, &$form_state) {
+function system_status($check = FALSE) {
+  // Load .install files
+  include_once DRUPAL_ROOT . '/core/includes/install.inc';
+  drupal_load_updates();
+
+  // Check run-time requirements and status information.
+  $requirements = module_invoke_all('requirements', 'runtime');
+  usort($requirements, '_system_sort_requirements');
+
+  if ($check) {
+    return drupal_requirements_severity($requirements) == REQUIREMENT_ERROR;
   }
+  // MySQL import might have set the uid of the anonymous user to autoincrement
+  // value. Let's try fixing it. See http://drupal.org/node/204411
+  db_update('users')
+    ->expression('uid', 'uid - uid')
+    ->condition('name', '')
+    ->condition('pass', '')
+    ->condition('status', 0)
+    ->execute();
+  return theme('status_report', array('requirements' => $requirements));
}

That got readded accidently. It's already a proper controller. Probably a re-roll problem.

+++ b/core/modules/system/system.admin.incundefined
@@ -764,110 +764,96 @@ function system_modules_submit($form, &$form_state) {
+function system_run_cron() {
+  // Run cron manually
+  if (drupal_cron_run()) {
+    drupal_set_message(t('Cron ran successfully.'));
   }
   else {
-    $form_state['storage'] = $form_state['values'];
-    $form_state['rebuild'] = TRUE;
+    drupal_set_message(t('Cron run failed.'), 'error');
   }
+
+  drupal_goto('admin/reports/status');

That got readded accidently. It's already a proper controller. Probably a re-roll problem.

+++ b/core/modules/system/system.admin.incundefined
@@ -764,110 +764,96 @@ function system_modules_submit($form, &$form_state) {
+/**
+ * Menu callback: return information about PHP.
+ */
+function system_php() {
+  phpinfo();
+  drupal_exit();
}

What's that? :)

StatusFileSize
new6.43 KB
new16.13 KB
FAILED: [[SimpleTest]]: [MySQL] 56,452 pass(es), 274 fail(s), and 49 exception(s).
[ View ]

Processed all the feedback.

Changed the confirmation form to a separate form using a KeyValueInterface.

Status:Needs review» Needs work

The last submitted patch, drupal-system_modules_uninstall_form_controller-1993202-9.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new18.26 KB
PASSED: [[SimpleTest]]: [MySQL] 57,989 pass(es).
[ View ]

I wasn't able to create an interdiff. This patch should pass all the tests.

StatusFileSize
new16.88 KB
PASSED: [[SimpleTest]]: [MySQL] 58,078 pass(es).
[ View ]
new2.92 KB

Ok, the previous one didn't contain all the points of #8. They magically appeared some how. Fixed it. Also changed some white space errors.

Assigned:h3rj4n» fubhy

Very good job there @h3rj4n. Taking over now while working on the final parts of #1990544: Convert system_modules() to a Controller. Just going to do some small, final adjustments. Good job!

Assigned:fubhy» Unassigned
StatusFileSize
new13.23 KB
new19.52 KB
FAILED: [[SimpleTest]]: [MySQL] 56,955 pass(es), 128 fail(s), and 538 exception(s).
[ View ]

StatusFileSize
new19.16 KB
FAILED: [[SimpleTest]]: [MySQL] 57,026 pass(es), 0 fail(s), and 134 exception(s).
[ View ]

Some final clean-ups and doc fixes.

Status:Needs review» Needs work

The last submitted patch, 1993202-15.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new19.24 KB
PASSED: [[SimpleTest]]: [MySQL] 56,961 pass(es).
[ View ]

Whoops, missing 'use' statement.

+++ b/core/modules/system/lib/Drupal/system/Form/ModulesUninstallConfirmForm.php
@@ -54,31 +124,46 @@ public function getFormID() {
+    $this->modules = $this->keyValueExpirableFactory
+      ->get('modules_uninstall')
+      ->get($account);

We have generally been only storing the particular store we need, not the entire factory. I don't fully grok the KV system yet so don't quote me on that, but... :-)

+++ b/core/modules/system/lib/Drupal/system/Form/ModulesUninstallForm.php
@@ -0,0 +1,173 @@
+        '#title' => t('Uninstall @module module', array('@module' => $name)),

This patch is converting translation to an injected object elsewhere. Let's do it here, too.

+++ b/core/modules/system/system.admin.inc
@@ -1233,11 +1126,11 @@ function theme_system_modules_uninstall($variables) {
+      $disabled_message = format_plural(count($form['modules'][$module]['#dependents']),

I think format_plural() now has an OO version, doesn't it?

Otherwise looks good!

Status:Needs review» Needs work

format_plural is not converted yet.

Status:Needs work» Needs review
StatusFileSize
new5.91 KB
new19.67 KB
PASSED: [[SimpleTest]]: [MySQL] 56,991 pass(es).
[ View ]

I think format_plural() now has an OO version, doesn't it?

No OO version for that yet :(.

This patch is converting translation to an injected object elsewhere. Let's do it here, too.

Whoops, missed that.

Status:Needs review» Reviewed & tested by the community

I really prefer to inject the request object not in the constructor but just via the buildForm method but will, some folks prefer the other way.

Status:Reviewed & tested by the community» Needs work

Hm, actually that's a fair point. I'd say it's wrong to do it in the constructor for a controller, or a form being used as a controller. The request is part of the parameters to the call, not the setup of the object.

Yeah but we need it in the submit handler too. We can't get it there. So, what do we do? Write it to $this->request in buildForm?

That is not right. You can access the information in the submit form, if you have stored it in the buildForm method.

Yeah, that's what I said :P

Status:Needs work» Needs review
StatusFileSize
new5.24 KB
new19.59 KB
PASSED: [[SimpleTest]]: [MySQL] 56,986 pass(es).
[ View ]

Okay, addressed the above.

+++ b/core/modules/system/lib/Drupal/system/Form/ModulesUninstallConfirmForm.phpundefined
@@ -126,9 +122,9 @@ public function getFormID() {
-  public function buildForm(array $form, array &$form_state) {
+  public function buildForm(array $form, array &$form_state, Request $request = NULL) {
     // Retrieve the list of modules from the key value store.
-    $account = $this->request->attributes->get('account')->id();
+    $account = $request->attributes->get('account')->id();
     $this->modules = $this->keyValueExpirable->get($account);
     // Prevent this page from showing when the module list is empty.
@@ -136,6 +132,9 @@ public function buildForm(array $form, array &$form_state) {
@@ -136,6 +132,9 @@ public function buildForm(array $form, array &$form_state) {
       return new RedirectResponse('/admin/modules/uninstall');
     }
+    // Store the request for use in the submit handler.
+    $this->request = $request;
+++ b/core/modules/system/lib/Drupal/system/Form/ModulesUninstallForm.phpundefined
@@ -90,12 +86,15 @@ public function getFormID() {
+  public function buildForm(array $form, array &$form_state, Request $request = NULL) {
     $form['modules'] = array();
     // Make sure the install API is available.
     include_once DRUPAL_ROOT . '/core/includes/install.inc';
+    // Store the request for use in the submit handler.

It would be cool to just store it at the beginning of the method.

It would be cool to just store it at the beginning of the method.

We don't need it in all cases, just if we are going to reach the submit handler.

StatusFileSize
new19.65 KB
PASSED: [[SimpleTest]]: [MySQL] 56,933 pass(es).
[ View ]

Re-roll and moving $this->request to the beginning as daniel requested.

Status:Needs review» Needs work

The patch needs a rerole.

Please update the issue summary to explain why we use the the key value store here
and why this could not have been a straight port of the existing code.

Issue summary:View changes

minor fix

Status:Needs work» Needs review

The patch needs a rerole.

Hmm, no?! Still applies cleanly for me.

Updated the issue summary with an explanation of what we are doing here with the key value store.

So this KV is not per user? This seems to be a possible usecase for the tempstore.

Yes, it's per user. It's not a tempstore though because we don't need/want the lock, etc.

It's just an expirable per-user k/v store.

Issue summary:View changes

Updated issue summary.

Status:Needs review» Reviewed & tested by the community

Okay, adapted the the issue summary.

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/system/lib/Drupal/system/Form/ModulesUninstallConfirmForm.phpundefined
@@ -8,27 +8,94 @@
+  /**
+   * Constructs a ModulesUninstallConfirmForm object.
+   *
+   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
+   *   The module handler.
+   * @param \Drupal\Core\KeyValueStore\KeyValueExpirableFactory $key_value_expirable_factory
+   *   The key value expirable factory.
+   * @param \Drupal\Core\StringTranslation\TranslationManager
+   *   The translation manager.
+   */
+  public function __construct(ModuleHandlerInterface $module_handler, KeyValueExpirableFactory $key_value_expirable_factory, TranslationManager $translation_manager) {
+    $this->moduleHandler = $module_handler;
+    $this->keyValueExpirable = $key_value_expirable_factory->get('modules_uninstall');
+    $this->translationManager = $translation_manager;
+  }

We are we not passing in KeyValueStoreExpirableInterface instead of KeyValueExpirableFactory... ie. in create do... $container->get('keyvalue.expirable')->get('modules_uninstall')

+++ b/core/modules/system/lib/Drupal/system/Form/ModulesUninstallConfirmForm.phpundefined
@@ -79,6 +151,15 @@ public function buildForm(array $form, array &$form_state, $modules = array(), R
+    drupal_set_message(t('The selected modules have been uninstalled.'));

Using t() unnecessarily...

+++ b/core/modules/system/lib/Drupal/system/Form/ModulesUninstallForm.phpundefined
@@ -0,0 +1,174 @@
+  /**
+   * Constructs a ModulesUninstallForm object.
+   *
+   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
+   *   The module handler.
+   * @param \Drupal\Core\KeyValueStore\KeyValueExpirableFactory $key_value_expirable_factory
+   *   The key value expirable factory.
+   * @param \Drupal\Core\StringTranslation\TranslationManager $translation_manager
+   *   The translation manager.
+   */
+  public function __construct(ModuleHandlerInterface $module_handler, KeyValueExpirableFactory $key_value_expirable_factory, TranslationManager $translation_manager) {
+    $this->moduleHandler = $module_handler;
+    $this->keyValueExpirable = $key_value_expirable_factory->get('modules_uninstall');
+    $this->translationManager = $translation_manager;
+  }

Again why ee are we not passing in KeyValueStoreExpirableInterface instead of KeyValueExpirableFactory... ie. in create do... $container->get('keyvalue.expirable')->get('modules_uninstall')

Assigned:Unassigned» fubhy

We are we not passing in KeyValueStoreExpirableInterface instead of KeyValueExpirableFactory... ie. in create do... $container->get('keyvalue.expirable')->get('modules_uninstall')

Yeah, good point. Will fix that.

Assigned:fubhy» Unassigned
Status:Needs work» Needs review
StatusFileSize
new19.67 KB
PASSED: [[SimpleTest]]: [MySQL] 57,042 pass(es).
[ View ]
new4.06 KB

Interdiffs look good. Moving back to RTBC and will commit a bit later if there's no more objections.

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

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

Issue summary:View changes

blub