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
Comment | File | Size | Author |
---|---|---|---|
#74 | 1990544-74.patch | 47.46 KB | fubhy |
#70 | 1990544-70.patch | 47.51 KB | fubhy |
#70 | interdiff.txt | 4.83 KB | fubhy |
#67 | 1990544-67.patch | 47.56 KB | fubhy |
#65 | 1990544-65.patch | 49.5 KB | fubhy |
Comments
Comment #1
PanchoHere'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:
I tested it and everything seemed to work fine, but let's see what the testbot says...
Comment #3
PanchoWow, 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
protectedprivate, 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.
Comment #5
Pancho1 fail is much better... :)
Don't know what the failed EntityTranslationUITest might have to do with it, though. Going to retest tomorrow.
Comment #6
andypost@Pancho when rolling new patches please provide interdiff
Comment #7
Pancho#3: system_modules_form_controller-1990544-3.patch queued for re-testing.
Comment #8
PanchoRerolled removing the unneeded ControllerInterface.
Comment #9
PanchoComment #11
Pancho#8: system_modules_form_controller-1990544-8.patch queued for re-testing.
[edit] Funny, why does FilePrivateTest suddenly fail? Retesting it.
Comment #12
ParisLiakos CreditAttribution: ParisLiakos commentedlets inject the module_handler service and store it to $this->moduleHandler
check_plain should be String::checkPlain
drupal_ucfirst should be Unicode::ucfirst()
use the module_handler service instead of module_invoke
same for implements
Comment #13
PanchoThanks for your review and the suggestions!
I'll be rolling that into a new patch, therefore setting back to CNW.
Comment #14
Pancho@Paris:
Is that something we generally want to do now?
I'd then roll it into my conversion patches in the other issues as well.
Comment #15
ParisLiakos CreditAttribution: ParisLiakos commentedyes. 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
Comment #16
PanchoStraight 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?
Or can we just do a:
Comment #17
PanchoNew version comprising the comments in #12.
Some feedback regarding the questions in #16 would be awesome.
You see anything else?
Comment #18
PanchoSorry, 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:
Comment #20
ParisLiakos CreditAttribution: ParisLiakos commentedinstantianing a new Form is just fine i think
Comment #21
PanchoThe 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....
Comment #22
ParisLiakos CreditAttribution: ParisLiakos commenteduh...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:/
Comment #23
ParisLiakos CreditAttribution: ParisLiakos commenteda 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
Comment #24
PanchoBeen 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.
Comment #25
tim.plunkettThis blocks removal of MENU_LOCAL_ACTION.
Comment #26
h3rj4n CreditAttribution: h3rj4n commentedTaking over this issue because of inactivity.
I've recreated the patch so that it can be committed on the new HEAD of Drupal.
Comment #28
fubhy CreditAttribution: fubhy commentedI 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?
Comment #30
fubhy CreditAttribution: fubhy commentedOops, uploaded the wrong patch.
Comment #32
fubhy CreditAttribution: fubhy commentedComment #34
fubhy CreditAttribution: fubhy commented#32: 1990544-32.patch queued for re-testing.
Comment #36
fubhy CreditAttribution: fubhy commentedComment #38
fubhy CreditAttribution: fubhy commentedComment #40
fubhy CreditAttribution: fubhy commentedComment #42
tim.plunkettNot 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.
Comment #43
fubhy CreditAttribution: fubhy commentedAgreed. 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.
Comment #45
fubhy CreditAttribution: fubhy commentedChaising 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.
Comment #46
fubhy CreditAttribution: fubhy commentedOh dear, better read your patch before uploading it.... Forgot to remove my debugging code.
Comment #47
fubhy CreditAttribution: fubhy commentedComment #49
fubhy CreditAttribution: fubhy commentedNo idea what those last fails are about. Will take a look on monday unless someone beats me to it.
Comment #50
fubhy CreditAttribution: fubhy commentedFixing some of the fails...
Comment #52
fubhy CreditAttribution: fubhy commentedI 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...
Comment #53
dawehnerLet's inject the request from here, if possible.
Should we use a url generator now?
Comment #54
fubhy CreditAttribution: fubhy commentedI 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
Comment #55
dawehnerThis works.
Comment #57
fubhy CreditAttribution: fubhy commentedThis should be green now.
Comment #59
fubhy CreditAttribution: fubhy commented#57: 1990544-57.patch queued for re-testing.
Comment #60
tim.plunkettAh, 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.
In most other places, we put this in buildForm(), even if we need to assign it for later.
I understand why we do this, but ouch.
I know in other issues I've seen $this->t->translate()...
Is it really worth using that function? It's simple enough I'd vote for it being in a closure inline.
Can this be made to use #theme?
Comment #61
fubhy CreditAttribution: fubhy commentedThanks for the good reviews
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.
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.
Comment #63
fubhy CreditAttribution: fubhy commentedWhoops, I sneaked in some files from another patch.
Comment #64
dawehnerCan we please add a follow up to have a dedicated test for this behavior?
It seems easier to read it if is at the top.
String::checkPlain is the key now.
Comment #65
fubhy CreditAttribution: fubhy commentedFixed the stuff mentioned in #64
Comment #66
dawehnerWhere is the follow up?
This also changes the current behavior, but I agree that this is a bug.
I seriously just hate changes like that.
Comment #67
fubhy CreditAttribution: fubhy commentedAgreed. Probably better to stick with it. Happened as a by-product while I was doing the refactoring (which was more than necessary).
Yes...
Comment #68
fubhy CreditAttribution: fubhy commentedFollow-up here: #2039731: Add test for cleaning up connection upon serialization
Comment #69
dawehnerThank you very much.
Comment #70
fubhy CreditAttribution: fubhy commentedRe-roll and some minor changes according to @alexpott's review.
Comment #71
catchInterdiff looks good - back to RTBC and will commit later if no more objections.
Comment #72
catchCommitted/pushed to 8.x, thanks!
Comment #73
catchWhoops. Typed before actually committing - this no longer applies.
Comment #74
fubhy CreditAttribution: fubhy commentedRe-roll
Comment #75
amateescu CreditAttribution: amateescu commentedIt applies now :)
Comment #76
alexpottCommitted 8d693ad and pushed to 8.x. Thanks!
Comment #77
bowersox CreditAttribution: bowersox commentedIt 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.
Comment #78
tim.plunkettThis issue had a bad merge of #1893072: Use details element on module page, which led to #2053285: Module listing doesn't hide empty package grouping fieldsets when filtering. as a regression.
Comment #79.0
(not verified) CreditAttribution: commentedUpdated issue summary.