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

Files: 
CommentFileSizeAuthor
#74 1990544-74.patch47.46 KBfubhy
PASSED: [[SimpleTest]]: [MySQL] 57,305 pass(es).
[ View ]
#70 1990544-70.patch47.51 KBfubhy
PASSED: [[SimpleTest]]: [MySQL] 56,993 pass(es).
[ View ]
#70 interdiff.txt4.83 KBfubhy
#67 1990544-67.patch47.56 KBfubhy
PASSED: [[SimpleTest]]: [MySQL] 57,136 pass(es).
[ View ]
#65 1990544-65.patch49.5 KBfubhy
PASSED: [[SimpleTest]]: [MySQL] 56,960 pass(es).
[ View ]
#65 interdiff.txt3.01 KBfubhy
#63 1990544-63.patch49.21 KBfubhy
PASSED: [[SimpleTest]]: [MySQL] 56,534 pass(es).
[ View ]
#61 1990544-61.patch58.42 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] 56,996 pass(es), 55 fail(s), and 5 exception(s).
[ View ]
#61 interdiff.txt5.17 KBfubhy
#57 1990544-57.patch49.34 KBfubhy
PASSED: [[SimpleTest]]: [MySQL] 57,253 pass(es).
[ View ]
#55 interdiff.txt783 bytesdawehner
#55 drupal-1990544-54.patch49.35 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,066 pass(es), 7 fail(s), and 1 exception(s).
[ View ]
#52 1990544-52.patch48.58 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] 56,489 pass(es), 12 fail(s), and 4 exception(s).
[ View ]
#52 interdiff.txt7.05 KBfubhy
#50 1990544-50.patch48.55 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] 56,724 pass(es), 5 fail(s), and 3 exception(s).
[ View ]
#46 1990544-46.patch49.57 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] 56,973 pass(es), 13 fail(s), and 6 exception(s).
[ View ]
#45 1990544-45.patch67.87 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] 56,632 pass(es), 7 fail(s), and 3 exception(s).
[ View ]
#45 1990544-45.patch67.87 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] 56,812 pass(es), 7 fail(s), and 3 exception(s).
[ View ]
#43 1990544-43.patch43.08 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] 57,445 pass(es), 575 fail(s), and 38 exception(s).
[ View ]
#40 1990544-40.patch61.97 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]
#38 1990544-37.patch61.16 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]
#36 1990544-36.patch53.12 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]
#32 1990544-32.patch53.73 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]
#30 1990544-28.patch52.99 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#28 1990544-28.patch41.03 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]
#26 drupal-system_module_to_controller-1990544-26.patch36.49 KBh3rj4n
FAILED: [[SimpleTest]]: [MySQL] 57,369 pass(es), 589 fail(s), and 68 exception(s).
[ View ]
#18 interdiff-1990544-16-17.txt4.79 KBPancho
#17 system_modules_form_controller-1990544-17.patch36.51 KBPancho
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#17 system_modules_form_controller-1990544-17.patch36.51 KBPancho
FAILED: [[SimpleTest]]: [MySQL] 56,051 pass(es), 14 fail(s), and 3 exception(s).
[ View ]
#16 system_modules_form_controller-1990544-16.patch35.91 KBPancho
PASSED: [[SimpleTest]]: [MySQL] 56,349 pass(es).
[ View ]
#8 system_modules_form_controller-1990544-8.patch35.77 KBPancho
PASSED: [[SimpleTest]]: [MySQL] 55,651 pass(es).
[ View ]
#8 interdiff.txt793 bytesPancho
#3 system_modules_form_controller-1990544-3.patch36.01 KBPancho
PASSED: [[SimpleTest]]: [MySQL] 55,598 pass(es).
[ View ]
#1 system_modules_form_controller-1990544-1.patch36.02 KBPancho
FAILED: [[SimpleTest]]: [MySQL] 55,007 pass(es), 593 fail(s), and 64 exception(s).
[ View ]

Comments

Assigned:Unassigned» Pancho
Status:Active» Needs review
StatusFileSize
new36.02 KB
FAILED: [[SimpleTest]]: [MySQL] 55,007 pass(es), 593 fail(s), and 64 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new36.01 KB
PASSED: [[SimpleTest]]: [MySQL] 55,598 pass(es).
[ View ]

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.

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.

@Pancho when rolling new patches please provide interdiff

Status:Needs work» Needs review

StatusFileSize
new793 bytes
new35.77 KB
PASSED: [[SimpleTest]]: [MySQL] 55,651 pass(es).
[ View ]

Rerolled removing the unneeded ControllerInterface.

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

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

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.

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

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.

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

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

StatusFileSize
new35.91 KB
PASSED: [[SimpleTest]]: [MySQL] 56,349 pass(es).
[ View ]

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?

<?php
   
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:
<?php
   
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']);
    }
?>

Status:Needs work» Needs review
StatusFileSize
new36.51 KB
FAILED: [[SimpleTest]]: [MySQL] 56,051 pass(es), 14 fail(s), and 3 exception(s).
[ View ]
new36.51 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

New version comprising the comments in #12.

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

StatusFileSize
new4.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.

instantianing a new Form is just fine i think

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

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:/

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

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.

Issue tags:+MENU_LOCAL_ACTION

This blocks removal of MENU_LOCAL_ACTION.

Assigned:Pancho» h3rj4n
Status:Needs work» Needs review
StatusFileSize
new36.49 KB
FAILED: [[SimpleTest]]: [MySQL] 57,369 pass(es), 589 fail(s), and 68 exception(s).
[ View ]

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.

Assigned:h3rj4n» fubhy
Status:Needs work» Needs review
StatusFileSize
new41.03 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new52.99 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Oops, uploaded the wrong patch.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new53.73 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new53.12 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new61.16 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new61.97 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]

Status:Needs review» Needs work

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

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

Status:Needs work» Needs review
StatusFileSize
new43.08 KB
FAILED: [[SimpleTest]]: [MySQL] 57,445 pass(es), 575 fail(s), and 38 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new67.87 KB
FAILED: [[SimpleTest]]: [MySQL] 56,812 pass(es), 7 fail(s), and 3 exception(s).
[ View ]
new67.87 KB
FAILED: [[SimpleTest]]: [MySQL] 56,632 pass(es), 7 fail(s), and 3 exception(s).
[ View ]

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.

StatusFileSize
new49.57 KB
FAILED: [[SimpleTest]]: [MySQL] 56,973 pass(es), 13 fail(s), and 6 exception(s).
[ View ]

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

Assigned:fubhy» Unassigned

Status:Needs review» Needs work

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

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

Status:Needs work» Needs review
StatusFileSize
new48.55 KB
FAILED: [[SimpleTest]]: [MySQL] 56,724 pass(es), 5 fail(s), and 3 exception(s).
[ View ]

Fixing some of the fails...

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new7.05 KB
new48.58 KB
FAILED: [[SimpleTest]]: [MySQL] 56,489 pass(es), 12 fail(s), and 4 exception(s).
[ View ]

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

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

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

StatusFileSize
new49.35 KB
FAILED: [[SimpleTest]]: [MySQL] 57,066 pass(es), 7 fail(s), and 1 exception(s).
[ View ]
new783 bytes

This works.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new49.34 KB
PASSED: [[SimpleTest]]: [MySQL] 57,253 pass(es).
[ View ]

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.

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

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

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

StatusFileSize
new5.17 KB
new58.42 KB
FAILED: [[SimpleTest]]: [MySQL] 56,996 pass(es), 55 fail(s), and 5 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new49.21 KB
PASSED: [[SimpleTest]]: [MySQL] 56,534 pass(es).
[ View ]

Whoops, I sneaked in some files from another patch.

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

StatusFileSize
new3.01 KB
new49.5 KB
PASSED: [[SimpleTest]]: [MySQL] 56,960 pass(es).
[ View ]

Fixed the stuff mentioned in #64

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.

StatusFileSize
new47.56 KB
PASSED: [[SimpleTest]]: [MySQL] 57,136 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

Thank you very much.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new4.83 KB
new47.51 KB
PASSED: [[SimpleTest]]: [MySQL] 56,993 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

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

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

Status:Fixed» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new47.46 KB
PASSED: [[SimpleTest]]: [MySQL] 57,305 pass(es).
[ View ]

Re-roll

Status:Needs review» Reviewed & tested by the community

It applies now :)

Status:Reviewed & tested by the community» Fixed

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

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.

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

Issue summary:View changes

Updated issue summary.