Convert the form builder system_modules() 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.

Follow-up: #2039731: Add test for cleaning up connection upon serialization

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Pancho’s picture

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

Here's the conversion patch.
The confirm form is a bit entangled with the form builder resp. controller. So I left out just the actual confirm form which is already covered by the patch in #1946454: Convert all confirm_form() in system.module and system.admin.inc to the new form interface and convert route. They might want to streamline that, but in this issue we should stay in scope.

Note that the duo of a normal menu item and a default local task pointing to the same function seems to produce rather unpredictable results.
Turned out that this is the exact only way to have everything work with all menu items, tasks, breadcrumbs, and the correct title:

  $items['admin/modules'] = array(
    'title' => 'Extend',
    'description' => 'Add and enable modules to extend site functionality.',
    'route_name' => 'system_modules_list',
    'weight' => -2,
  );
  $items['admin/modules/list'] = array(
    'title' => 'List',
    'access arguments' => array('administer modules'),
    'type' => MENU_DEFAULT_LOCAL_TASK,
  );
system_modules_list:
  pattern: 'admin/modules'
  defaults:
    _form: 'Drupal\system\Form\ModulesListForm'
  requirements:
    _permission: 'administer modules'

I tested it and everything seemed to work fine, but let's see what the testbot says...

Status: Needs review » Needs work

The last submitted patch, system_modules_form_controller-1990544-1.patch, failed testing.

Pancho’s picture

Status: Needs work » Needs review
FileSize
36.01 KB

Wow, 593 failed tests for the uninstall task? I'd say that is pretty much overcovered.
Need to add 'page callback' => 'drupal_get_form' there as it isn't inherited anymore.

Also, camel-cased build_row() restricting visibility to protected private, removed unnecessary property and fixed some docs.
This code can use significant refactoring, but that's out of scope.
So let's see again, now.

The last submitted patch, system_modules_form_controller-1990544-3.patch, failed testing.

Pancho’s picture

Status: Needs review » Needs work

1 fail is much better... :)
Don't know what the failed EntityTranslationUITest might have to do with it, though. Going to retest tomorrow.

andypost’s picture

@Pancho when rolling new patches please provide interdiff

Pancho’s picture

Status: Needs work » Needs review
Pancho’s picture

Rerolled removing the unneeded ControllerInterface.

Pancho’s picture

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

The last submitted patch, system_modules_form_controller-1990544-8.patch, failed testing.

Pancho’s picture

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

#8: system_modules_form_controller-1990544-8.patch queued for re-testing.

[edit] Funny, why does FilePrivateTest suddenly fail? Retesting it.

ParisLiakos’s picture

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

lets inject the module_handler service and store it to $this->moduleHandler

+++ b/core/modules/system/lib/Drupal/system/Form/ModulesListForm.phpundefined
@@ -0,0 +1,451 @@
+    $distribution_name = check_plain(drupal_install_profile_distribution_name());

check_plain should be String::checkPlain

+++ b/core/modules/system/lib/Drupal/system/Form/ModulesListForm.phpundefined
@@ -0,0 +1,451 @@
+          $extra['requires'][$requires] = t('@module (<span class="admin-missing">missing</span>)', array('@module' => drupal_ucfirst($requires)));

drupal_ucfirst should be Unicode::ucfirst()

+++ b/core/modules/system/lib/Drupal/system/Form/ModulesListForm.phpundefined
@@ -0,0 +1,451 @@
+        if (module_invoke($filename, 'help', "admin/help#$filename", $help_arg)) {

use the module_handler service instead of module_invoke

+++ b/core/modules/system/lib/Drupal/system/Form/ModulesListForm.phpundefined
@@ -0,0 +1,451 @@
+      if ($module->status && user_access('administer permissions') && in_array($filename, module_implements('permission'))) {

same for implements

Pancho’s picture

Status: Needs review » Needs work
Issue tags: +D8MI

Thanks for your review and the suggestions!
I'll be rolling that into a new patch, therefore setting back to CNW.

Pancho’s picture

@Paris:

lets inject the module_handler service and store it to $this->moduleHandler

Is that something we generally want to do now?
I'd then roll it into my conversion patches in the other issues as well.

ParisLiakos’s picture

yes. in general, whenever you can inject stuff, instead of using procedural functions, the better it is later, when you want to unit test something, or lazy loading stuff

You can see here an example on how to inject services using the create() method http://drupal.org/node/1953354

Pancho’s picture

Straight reroll after:
#1946454: Convert all confirm_form() in system.module and system.admin.inc to the new form interface and convert route.

Over there, they called the confirm form 'ModulesInstallConfirmForm', while we're calling the main form 'ModulesListForm' here.
I believe that the two should receive a consistent naming scheme. We could also go for 'ModulesOverviewForm' or simply 'ModulesForm', if we wnat to see these conversions as an opportunity to get a somewhat streamlined naming scheme. But I tend to 'ModulesListForm' and 'ModulesListConfirmForm' because it matches the UI label.

Also, is it necessary to create a new form object here?

    if (!empty($form_state['storage'])) {
      // Contents of confirm form is injected here because already in form
      // building function.
      $confirm_form = new ModulesInstallConfirmForm();
      return $confirm_form->buildForm($form, $form_state, $visible_files, $form_state['storage']);
    }

Or can we just do a:

    if (!empty($form_state['storage'])) {
      // Contents of confirm form is injected here because already in form
      // building function.
      return $this->buildForm($form, $form_state, $visible_files, $form_state['storage']);
    }
Pancho’s picture

Status: Needs work » Needs review
FileSize
36.51 KB
36.51 KB

New version comprising the comments in #12.

Some feedback regarding the questions in #16 would be awesome.
You see anything else?

Pancho’s picture

FileSize
4.79 KB

Sorry, double-posted the patch. Could someone please cancel one of the two testbot runs? THX and sorry...

Now here's the interdiff I intended to add:

Status: Needs review » Needs work

The last submitted patch, system_modules_form_controller-1990544-17.patch, failed testing.

ParisLiakos’s picture

instantianing a new Form is just fine i think

Pancho’s picture

The testbot errors are interesting. They came up with patch #17, because I am now injecting the module handler.
When in the end of ModulesListForm::submitForm(), a pre-install and a post-install module list are compared, drupal_container()->get('module_handler')->getModuleList() in the meantime has updated, while $this->moduleHandler->getModuleList() doesn't:

After an install, drupal_container()->get('module_handler')->getModuleList() would return the correct, say 47 modules, while $this->moduleHandler->getModuleList() still gives the pre-install list of 45 modules.
However, this is - even more interesting - only the case when taking the route via the confirm form. If all dependencies are checked, both alternatives give the same correct results.

So it seems important to note that injecting the module handler isn't necessarily a simple replacement without pitfalls.
If I still want to use it, what would I do to make it work?
I'm trying to figure it out, but maybe someone has an instant idea what's going wrong here....

ParisLiakos’s picture

Issue tags: +Stalking Crell

uh...this is because the kernel is being rebuilt..the module handler service is not being updated after rebuilt:/
this is a side effect we never thought about...
I am tagging it for Crell to check...
@Crell: can you think of a way to upload classes that got injected with ::create() method with the new services?
sigh:/

ParisLiakos’s picture

a quick fix for this issue to move on, would be:
turn this controller to a service..eg, register it and its dependencies in system.services.yml

Pancho’s picture

Been preparing a patch that slightly refactors the code making it more OOP, easier to grasp and probably faster, because some expensive module list rebuilds can be omitted.
However, I'm currently waiting for #1387438: Timeout on enabling modules: make it a batch operation to land, which would also remove the module list comparison causing us problems.

tim.plunkett’s picture

Issue tags: +MENU_LOCAL_ACTION

This blocks removal of MENU_LOCAL_ACTION.

h3rj4n’s picture

Assigned: Pancho » h3rj4n
Status: Needs work » Needs review
FileSize
36.49 KB

Taking over this issue because of inactivity.

I've recreated the patch so that it can be committed on the new HEAD of Drupal.

Status: Needs review » Needs work

The last submitted patch, drupal-system_module_to_controller-1990544-26.patch, failed testing.

fubhy’s picture

Assigned: h3rj4n » fubhy
Status: Needs work » Needs review
FileSize
41.03 KB

I am sorry, I went a bit wild on this one... I couldn't stand what I saw both in system_modules and the submit handler. Also, manually instantiating and mapping to a separate form controller is _not cool_. This patch is not fully ready yet and very likely to produce some test fails.

Especially the submit handler in the old code hurts my eyes :/. Why do we replicate the module handler and dependency resolution there just to abuse the $install_dependencies/$uninstall_dependencies variables then?

Status: Needs review » Needs work

The last submitted patch, 1990544-28.patch, failed testing.

fubhy’s picture

Status: Needs work » Needs review
FileSize
52.99 KB

Oops, uploaded the wrong patch.

Status: Needs review » Needs work

The last submitted patch, 1990544-28.patch, failed testing.

fubhy’s picture

Status: Needs work » Needs review
FileSize
53.73 KB

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

The last submitted patch, 1990544-32.patch, failed testing.

fubhy’s picture

Status: Needs work » Needs review

#32: 1990544-32.patch queued for re-testing.

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

The last submitted patch, 1990544-32.patch, failed testing.

fubhy’s picture

Status: Needs work » Needs review
FileSize
53.12 KB

Status: Needs review » Needs work

The last submitted patch, 1990544-36.patch, failed testing.

fubhy’s picture

Status: Needs work » Needs review
FileSize
61.16 KB

Status: Needs review » Needs work

The last submitted patch, 1990544-37.patch, failed testing.

fubhy’s picture

Status: Needs work » Needs review
FileSize
61.97 KB

Status: Needs review » Needs work

The last submitted patch, 1990544-40.patch, failed testing.

tim.plunkett’s picture

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentFieldsTest.phpundefined
@@ -70,19 +70,19 @@ function testCommentEnable() {
-    $edit['modules[Core][comment][enable]'] = FALSE;
+    $edit['modules[comment]'] = FALSE;

Not sure I see the point in having to change all of this. Would it be that hard to fix the form structure so we don't have to change these tests?
That seems like half the patch (in size, not complexity).

As far as the new approach, I love the use of tempstore.

fubhy’s picture

Status: Needs work » Needs review
FileSize
43.08 KB

Agreed. Removed that part of the patch that changed the form element names and cleaned it up a little bit more.

We might want to switch to a plain expirable key value store for now as I am not sure about the locking that comes with TempStore although I kinda like the idea of only one person being able to install/unintsall stuff at a time. But that's not happening really anyways as that would just be the case for the confirmation form. So I think a plain keyvalue expirable is maybe better. Will switch to that in the next patch.

Status: Needs review » Needs work

The last submitted patch, 1990544-43.patch, failed testing.

fubhy’s picture

Status: Needs work » Needs review
FileSize
67.87 KB
67.87 KB

Chaising head, adding translation manager, retrieving user context from request object instead of $GLOBALS['user'], fixing some test failures, using KeyValueExpirable instead of TempStore as the locking might be a bit overkill.

fubhy’s picture

FileSize
49.57 KB

Oh dear, better read your patch before uploading it.... Forgot to remove my debugging code.

fubhy’s picture

Assigned: fubhy » Unassigned

Status: Needs review » Needs work

The last submitted patch, 1990544-46.patch, failed testing.

fubhy’s picture

No idea what those last fails are about. Will take a look on monday unless someone beats me to it.

fubhy’s picture

Status: Needs work » Needs review
FileSize
48.55 KB

Fixing some of the fails...

Status: Needs review » Needs work

The last submitted patch, 1990544-50.patch, failed testing.

fubhy’s picture

Status: Needs work » Needs review
FileSize
7.05 KB
48.58 KB

I can't figure out what's wrong with the LocaleUpdateTest thingy... It fails somewhere in batch_process() when attempting to do some language updates. No idea, really...

dawehner’s picture

+++ b/core/modules/system/lib/Drupal/system/Form/ModulesListConfirmForm.phpundefined
@@ -0,0 +1,191 @@
+  public function buildForm(array $form, array &$form_state) {
+    $account = $this->request->attributes->get('account')->id();

Let's inject the request from here, if possible.

+++ b/core/modules/system/lib/Drupal/system/Form/ModulesListConfirmForm.phpundefined
@@ -0,0 +1,191 @@
+      return new RedirectResponse(url($this->getCancelPath(), array('absolute' => TRUE)));

Should we use a url generator now?

fubhy’s picture

Let's inject the request from here, if possible.

I was wondering if we should do that, mhm... We got $this->request anyways because we need it in the submit handler. Not sure what's better tbh. At the end of the day they are both the same anyways :P

dawehner’s picture

FileSize
49.35 KB
783 bytes

This works.

Status: Needs review » Needs work

The last submitted patch, drupal-1990544-54.patch, failed testing.

fubhy’s picture

Status: Needs work » Needs review
FileSize
49.34 KB

This should be green now.

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

The last submitted patch, 1990544-57.patch, failed testing.

fubhy’s picture

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

#57: 1990544-57.patch queued for re-testing.

tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.phpundefined
@@ -114,6 +114,19 @@ public static function open(array &$connection_options = array()) {
+  /**
+   * {@inheritdoc}
+   */
+  public function serialize() {
+    // Cleanup the connection, much like __destruct() does it as well.
+    if ($this->needsCleanup) {
+      $this->nextIdDelete();
+    }
+    $this->needsCleanup = FALSE;
+
+    return parent::serialize();
+  }

Ah, I need this hunk in #1946466: Convert all confirm_form() in user.module and user.pages.inc to the new form interface and convert route as well.

+++ b/core/modules/system/lib/Drupal/system/Form/ModulesListConfirmForm.phpundefined
@@ -0,0 +1,191 @@
+    $this->request = $request;
...
+  public function buildForm(array $form, array &$form_state) {

In most other places, we put this in buildForm(), even if we need to assign it for later.

+++ b/core/modules/system/lib/Drupal/system/Form/ModulesListConfirmForm.phpundefined
@@ -0,0 +1,191 @@
+    return $this->translationManager->translate('Some required modules must be enabled');

I understand why we do this, but ouch.

I know in other issues I've seen $this->t->translate()...

+++ b/core/modules/system/lib/Drupal/system/Form/ModulesListForm.phpundefined
@@ -0,0 +1,457 @@
+    $this->moduleHandler->loadInclude('system', 'inc', 'system.admin');
...
+    uasort($modules, 'system_sort_modules_by_info_name');

Is it really worth using that function? It's simple enough I'd vote for it being in a closure inline.

+++ b/core/modules/system/lib/Drupal/system/Form/ModulesListForm.phpundefined
@@ -0,0 +1,457 @@
+      $row['description']['#markup'] .= theme('system_modules_incompatible', array(

Can this be made to use #theme?

fubhy’s picture

FileSize
5.17 KB
58.42 KB

Thanks for the good reviews

Is it really worth using that function? It's simple enough I'd vote for it being in a closure inline.

I think it does not make much sense to refactor that now. I guess we can clean that up in a follow-up.

Fixed the rest.

I understand why we do this, but ouch.

Agreed. But we did not yet agree on a pattern for that so I did not go any further than this. This would all be much easier if we had traits. Putting a protected t() method in every object is rather weird too.

Status: Needs review » Needs work

The last submitted patch, 1990544-61.patch, failed testing.

fubhy’s picture

Status: Needs work » Needs review
FileSize
49.21 KB

Whoops, I sneaked in some files from another patch.

dawehner’s picture

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.phpundefined
@@ -114,6 +114,19 @@ public static function open(array &$connection_options = array()) {
+  /**
+   * {@inheritdoc}
+   */
+  public function serialize() {
+    // Cleanup the connection, much like __destruct() does it as well.
+    if ($this->needsCleanup) {
+      $this->nextIdDelete();
+    }
+    $this->needsCleanup = FALSE;
+
+    return parent::serialize();
+  }

Can we please add a follow up to have a dedicated test for this behavior?

+++ b/core/modules/system/lib/Drupal/system/Form/ModulesListConfirmForm.phpundefined
@@ -0,0 +1,190 @@
+    // Store the request for use in the submit handler.
+    $this->request = $request;

It seems easier to read it if is at the top.

+++ b/core/modules/system/lib/Drupal/system/Form/ModulesListForm.phpundefined
@@ -0,0 +1,457 @@
+    $distribution = check_plain(drupal_install_profile_distribution_name());

String::checkPlain is the key now.

fubhy’s picture

FileSize
3.01 KB
49.5 KB

Fixed the stuff mentioned in #64

dawehner’s picture

Where is the follow up?

+++ b/core/modules/system/lib/Drupal/system/Tests/Module/DependencyTest.phpundefined
@@ -61,7 +61,7 @@ function testMissingModules() {
-    $this->assertText(t('The @module module is missing, so the following module will be disabled: @depends.', array('@module' => '_missing_dependency', '@depends' => 'system_dependencies_test')), 'The module missing dependencies will be disabled.');
+    $this->assertText(t('The @module module is missing, so the following module will be disabled: @depends.', array('@module' => '_missing_dependency', '@depends' => 'System dependency test')), 'The module missing dependencies will be disabled.');

This also changes the current behavior, but I agree that this is a bug.

+++ b/core/modules/system/system.admin.incundefined
@@ -1145,11 +734,11 @@ function theme_system_modules_details($variables) {
-    $module['#requires'] = array_filter($module['#requires']);
-    $module['#required_by'] = array_filter($module['#required_by']);
+    $module['#dependencies'] = array_filter($module['#dependencies']);
+    $module['#dependents'] = array_filter($module['#dependents']);

I seriously just hate changes like that.

fubhy’s picture

FileSize
47.56 KB

I seriously just hate changes like that.

Agreed. Probably better to stick with it. Happened as a by-product while I was doing the refactoring (which was more than necessary).

This also changes the current behavior, but I agree that this is a bug.

Yes...

fubhy’s picture

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you very much.

fubhy’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
4.83 KB
47.51 KB

Re-roll and some minor changes according to @alexpott's review.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Interdiff looks good - back to RTBC and will commit later if no more objections.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

catch’s picture

Status: Fixed » Needs work

Whoops. Typed before actually committing - this no longer applies.

fubhy’s picture

Status: Needs work » Needs review
FileSize
47.46 KB

Re-roll

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

It applies now :)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 8d693ad and pushed to 8.x. Thanks!

bowersox’s picture

It looks like this patch introduced a CSS class, element-invisible, that was recently renamed for D8.

I've opened a small follow-up issue with a 1-line patch: #2046301: CSS regression on admin/modules: overlapping "Enabled" table header not properly invisible. Review of that would be appreciated.

tim.plunkett’s picture

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.