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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Pancho’s picture

Status: Active » Needs review
FileSize
11.2 KB

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.

dawehner’s picture

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

Pancho’s picture

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.

dawehner’s picture

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.

Pancho’s picture

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?

h3rj4n’s picture

Taking over because of inactivity.

Recreated the patch on the current Drupal HEAD.

h3rj4n’s picture

Status: Needs work » Needs review
fubhy’s picture

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

h3rj4n’s picture

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.

h3rj4n’s picture

Status: Needs work » Needs review
FileSize
18.26 KB

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

h3rj4n’s picture

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.

fubhy’s picture

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!

fubhy’s picture

Assigned: fubhy » Unassigned
FileSize
13.23 KB
19.52 KB
fubhy’s picture

FileSize
19.16 KB

Some final clean-ups and doc fixes.

Status: Needs review » Needs work

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

fubhy’s picture

Status: Needs work » Needs review
FileSize
19.24 KB

Whoops, missing 'use' statement.

Crell’s picture

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

dawehner’s picture

Status: Needs review » Needs work

format_plural is not converted yet.

fubhy’s picture

Status: Needs work » Needs review
FileSize
5.91 KB
19.67 KB

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.

dawehner’s picture

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.

Crell’s picture

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.

fubhy’s picture

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?

dawehner’s picture

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

fubhy’s picture

Yeah, that's what I said :P

fubhy’s picture

Status: Needs work » Needs review
FileSize
5.24 KB
19.59 KB

Okay, addressed the above.

dawehner’s picture

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

fubhy’s picture

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.

fubhy’s picture

FileSize
19.65 KB

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

dawehner’s picture

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.

dawehner’s picture

Issue summary: View changes

minor fix

fubhy’s picture

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.

dawehner’s picture

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

fubhy’s picture

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.

fubhy’s picture

Issue summary: View changes

Updated issue summary.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Okay, adapted the the issue summary.

alexpott’s picture

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

fubhy’s picture

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.

fubhy’s picture

Assigned: fubhy » Unassigned
Status: Needs work » Needs review
FileSize
19.67 KB
4.06 KB
catch’s picture

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

catch’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

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

Anonymous’s picture

Issue summary: View changes

blub