Problem/Motivation

Usage of entity manager in Drupal\system\Form\ModulesListForm was removed in #2301317: MenuLinkNG part4: Conversion but the service is still injected

Proposed resolution

Remove the entity manager

User interface changes

none

API changes

none

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because we are removing dead code.
Issue priority Normal because Drupal will work whether or not it is removed.
Prioritized changes The main goal of this issue is removing dead code, and is a prioritized change per @xjm in #14.
Disruption Disruptive for any classes that extend ModuleListForm, which will need to update their constructors.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

willzyx’s picture

willzyx’s picture

Issue tags: +code cleanup

Tagging

willzyx’s picture

Title: Remove entity manager from Drupal\system\Form\ModulesListForm » Remove unused entity.manager service from Drupal\system\Form\ModulesListForm
cilefen’s picture

Status: Needs review » Needs work
Issue tags: +Needs beta evaluation
willzyx’s picture

Issue summary: View changes
Status: Needs work » Needs review

Added Beta evaluation

cilefen’s picture

Issue summary: View changes
cilefen’s picture

Status: Needs review » Needs work

This is a tiny change. In reviewing this I:

  • Looked over the parent issue and read this issue entirely.
  • Checked for inheritors of the class being changed. There are none.
  • Verified there is a test. There is: ModulesListFormWebTest. The test is not comprehensive. But the entityManager was doing absolutely nothing in this class so I am not concerned.
  • Checked that all the dead code is removed: it is.
  • Checked for Drupal coding standards - there is a nitpick-level problem.
+++ b/core/modules/system/src/Form/ModulesListForm.php
@@ -144,12 +133,11 @@ public static function create(ContainerInterface $container) {
-  public function __construct(ModuleHandlerInterface $module_handler, ModuleInstallerInterface $module_installer, KeyValueStoreExpirableInterface $key_value_expirable, AccessManagerInterface $access_manager, EntityManagerInterface $entity_manager, AccountInterface $current_user,  RouteMatchInterface $route_match, TitleResolverInterface $title_resolver, RouteProviderInterface $route_provider, MenuLinkManagerInterface $menu_link_manager) {
+  public function __construct(ModuleHandlerInterface $module_handler, ModuleInstallerInterface $module_installer, KeyValueStoreExpirableInterface $key_value_expirable, AccessManagerInterface $access_manager, AccountInterface $current_user,  RouteMatchInterface $route_match, TitleResolverInterface $title_resolver, RouteProviderInterface $route_provider, MenuLinkManagerInterface $menu_link_manager) {

There are two spaces after a comma in the function signature.

jmolivas’s picture

Status: Needs work » Needs review
FileSize
3.15 KB
1.28 KB
97.84 KB

I remove the two spaces separation and upload a patch, the interdiff and a screenshot of phpunit result

Status: Needs review » Needs work

The last submitted patch, 8: remove_unused-2481383-8.patch, failed testing.

jmolivas’s picture

Status: Needs work » Reviewed & tested by the community

patch re-tested and passed this time.

jmolivas’s picture

Status: Reviewed & tested by the community » Needs review

assigning correct status

cilefen’s picture

Status: Needs review » Reviewed & tested by the community

@jmolivas ModulesListFormWebTest is a Simple Test so phpunit does not execute it.

In addition to what I did in the review in #7, I manually tested the form and found it is working fine. Based on the code standards fix, this is RTBC.

xjm’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs review
Issue tags: -Needs beta evaluation

Thanks all for your work here!

The beta evaluation here isn't entirely complete. It isn't listed as either a prioritized change or unfrozen, which would mean that, by default, it should be postponed. Going through the flowchart for allowed changes during the beta:

  1. It's not a feature request.
  2. It's not unfrozen (it changes more than CSS, markup, tests, strings, migrate).
  3. It's not critical.
  4. It's not a prioritized change (the goal is not to improve usability, accessibility, security, performance, etc. nor fix a bug).
  5. It's not major.
  6. There's no mention on the issue of it reducing fragility (which needs committer signoff). In this case? Yeah, it is. Removing dead, unused, untested code is one of the things that we discussed as a potential reduction to fragility. So I'm in favor of prioritizing this during the beta.

So once someone confirms that it's a prioritized change (in this case, with a committer's signoff), then we evaluate the impact versus the disruption. That section of the beta evaluation could also use some work because we need to consider more than just core. The disruption would be that any extending class (core/contrib/custom code) would now have a broken constructor. So we should ask ourselves, how likely is it that there are alternate module list form implementations out there? It seems like something there could be a usecase to override in contrib and distros. It seems less likely that they exist already. I think that updating the constructor is a small disruption for those modules, so the impact of removing unneeded code outweighs this disruption.

I've updated the beta evaluation to reflect these things. Please do similarly in other issues, especially normal and minor tasks. :) Consider whether cleanups should be postponed.

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Didn't actually need to set the status back, since I took the time to do the beta evaluation myself. In general, though, non-docs normal tasks without the beta evaluation (or with incomplete ones) can be "Needs work". :)

xjm’s picture

Status: Reviewed & tested by the community » Fixed

And so, per #14, this issue is a prioritized change as per https://www.drupal.org/core/beta-changes and its benefits outweigh any disruption. Committed and pushed to 8.0.x. Thanks!

I decided a constructor change for a form builder does not need a change record.

  • xjm committed 5f84c3d on 8.0.x
    Issue #2481383 by jmolivas, willzyx, cilefen: Remove unused entity....
cilefen’s picture

@xjm Thank you for your careful attention.

Status: Fixed » Closed (fixed)

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