Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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
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. |
Comment | File | Size | Author |
---|---|---|---|
#8 | remove_unused-2481383-8_test.png | 97.84 KB | jmolivas |
#8 | interdiff-1-8.txt | 1.28 KB | jmolivas |
#8 | remove_unused-2481383-8.patch | 3.15 KB | jmolivas |
#1 | remove_entity_manager-2481383-1.patch | 3.15 KB | willzyx |
Comments
Comment #1
willzyx CreditAttribution: willzyx commentedComment #2
willzyx CreditAttribution: willzyx commentedTagging
Comment #3
willzyx CreditAttribution: willzyx commentedComment #4
cilefen CreditAttribution: cilefen commentedComment #5
willzyx CreditAttribution: willzyx commentedAdded Beta evaluation
Comment #6
cilefen CreditAttribution: cilefen commentedComment #7
cilefen CreditAttribution: cilefen commentedThis is a tiny change. In reviewing this I:
There are two spaces after a comma in the function signature.
Comment #8
jmolivas CreditAttribution: jmolivas at Blink Reaction (now part of FFW) commentedI remove the two spaces separation and upload a patch, the interdiff and a screenshot of phpunit result
Comment #11
jmolivas CreditAttribution: jmolivas at Blink Reaction (now part of FFW) commentedpatch re-tested and passed this time.
Comment #12
jmolivas CreditAttribution: jmolivas at Blink Reaction (now part of FFW) commentedassigning correct status
Comment #13
cilefen CreditAttribution: cilefen commented@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.
Comment #14
xjmThanks 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:
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.
Comment #15
xjmDidn'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". :)
Comment #16
xjmAnd 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.
Comment #18
cilefen CreditAttribution: cilefen commented@xjm Thank you for your careful attention.