Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Convert the form builder system_modules_uninstall()
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. The tempstore is not used, because it also provides a lock, which is not wanted here.
Comment | File | Size | Author |
---|---|---|---|
#37 | interdiff.txt | 4.06 KB | fubhy |
#37 | 1993202-37.patch | 19.67 KB | fubhy |
#29 | 1993202-29.patch | 19.65 KB | fubhy |
#26 | 1993202-26.patch | 19.59 KB | fubhy |
#26 | interdiff.txt | 5.24 KB | fubhy |
Comments
Comment #1
PanchoHere's the patch.
I needed to tweak the entry in system_menu() a bit because it would otherwise inherit old-school page and access callbacks from its parent. Seems like a bug in the new WSCCI implementation, but is okay for now and won't break.
The rest is straightforward, except for the actual confirm form that I left out because it is covered in #1946454: Convert all confirm_form() in system.module and system.admin.inc to the new form interface and convert route.
From my testing it seems to work, so now let's see what the testbot says.
Comment #2
dawehnerI don't think we need a controllerinterface here.
Can't you use $form_state['redirect'] instead of drupal_goto?
Isn't it "route_name" instead of "route name"?
You don't have to set these values if you have defined a route.
Comment #3
PanchoThanks for your review, Daniel!
You're right. Removed.
Sure, we can. I just didn't touch the code more than necessary in this conversion issue.
That's true, leaving out the underscore also lead to 'page callback' and 'access callback' not being unset, and that's why it seemed necessary to explicitely set them:
What I thought was a bug with the new routing system turns out to be my fault.
Hope, everything is fine now.
Comment #4
dawehnerThis looks pretty good so far!
All that is not needed ... I should have made it clear.
One could argue that you actually want to inject the module handler to load this includes, but yeah I don't really care.
Comment #5
PanchoBut in order to inject the module handler service, we do need create() and __construct(), ControllerInterface, ContainerInterface plus ModuleHandlerInterface, don't we?
Comment #6
h3rj4n CreditAttribution: h3rj4n commentedTaking over because of inactivity.
Recreated the patch on the current Drupal HEAD.
Comment #7
h3rj4n CreditAttribution: h3rj4n commentedComment #8
fubhy CreditAttribution: fubhy commentedThat's redundant.
This needs to get removed. We need a separate form controller for the confirm form. Make use of ConfirmFormBase.
That got readded accidently. It's already a proper controller. Probably a re-roll problem.
That got readded accidently. It's already a proper controller. Probably a re-roll problem.
What's that? :)
Comment #9
h3rj4n CreditAttribution: h3rj4n commentedProcessed all the feedback.
Changed the confirmation form to a separate form using a KeyValueInterface.
Comment #11
h3rj4n CreditAttribution: h3rj4n commentedI wasn't able to create an interdiff. This patch should pass all the tests.
Comment #12
h3rj4n CreditAttribution: h3rj4n commentedOk, the previous one didn't contain all the points of #8. They magically appeared some how. Fixed it. Also changed some white space errors.
Comment #13
fubhy CreditAttribution: fubhy commentedVery good job there @h3rj4n. Taking over now while working on the final parts of #1990544: Convert system_modules() to a Controller. Just going to do some small, final adjustments. Good job!
Comment #14
fubhy CreditAttribution: fubhy commentedComment #15
fubhy CreditAttribution: fubhy commentedSome final clean-ups and doc fixes.
Comment #17
fubhy CreditAttribution: fubhy commentedWhoops, missing 'use' statement.
Comment #18
Crell CreditAttribution: Crell commentedWe have generally been only storing the particular store we need, not the entire factory. I don't fully grok the KV system yet so don't quote me on that, but... :-)
This patch is converting translation to an injected object elsewhere. Let's do it here, too.
I think format_plural() now has an OO version, doesn't it?
Otherwise looks good!
Comment #19
dawehnerformat_plural is not converted yet.
Comment #20
fubhy CreditAttribution: fubhy commentedNo OO version for that yet :(.
Whoops, missed that.
Comment #21
dawehnerI really prefer to inject the request object not in the constructor but just via the buildForm method but will, some folks prefer the other way.
Comment #22
Crell CreditAttribution: Crell commentedHm, actually that's a fair point. I'd say it's wrong to do it in the constructor for a controller, or a form being used as a controller. The request is part of the parameters to the call, not the setup of the object.
Comment #23
fubhy CreditAttribution: fubhy commentedYeah but we need it in the submit handler too. We can't get it there. So, what do we do? Write it to $this->request in buildForm?
Comment #24
dawehnerThat is not right. You can access the information in the submit form, if you have stored it in the buildForm method.
Comment #25
fubhy CreditAttribution: fubhy commentedYeah, that's what I said :P
Comment #26
fubhy CreditAttribution: fubhy commentedOkay, addressed the above.
Comment #27
dawehnerIt would be cool to just store it at the beginning of the method.
Comment #28
fubhy CreditAttribution: fubhy commentedWe don't need it in all cases, just if we are going to reach the submit handler.
Comment #29
fubhy CreditAttribution: fubhy commentedRe-roll and moving $this->request to the beginning as daniel requested.
Comment #30
dawehnerThe patch needs a rerole.
Please update the issue summary to explain why we use the the key value store here
and why this could not have been a straight port of the existing code.
Comment #30.0
dawehnerminor fix
Comment #31
fubhy CreditAttribution: fubhy commentedHmm, no?! Still applies cleanly for me.
Updated the issue summary with an explanation of what we are doing here with the key value store.
Comment #32
dawehnerSo this KV is not per user? This seems to be a possible usecase for the tempstore.
Comment #33
fubhy CreditAttribution: fubhy commentedYes, it's per user. It's not a tempstore though because we don't need/want the lock, etc.
It's just an expirable per-user k/v store.
Comment #33.0
fubhy CreditAttribution: fubhy commentedUpdated issue summary.
Comment #34
dawehnerOkay, adapted the the issue summary.
Comment #35
alexpottWe are we not passing in KeyValueStoreExpirableInterface instead of KeyValueExpirableFactory... ie. in create do...
$container->get('keyvalue.expirable')->get('modules_uninstall')
Using t() unnecessarily...
Again why ee are we not passing in KeyValueStoreExpirableInterface instead of KeyValueExpirableFactory... ie. in create do...
$container->get('keyvalue.expirable')->get('modules_uninstall')
Comment #36
fubhy CreditAttribution: fubhy commentedYeah, good point. Will fix that.
Comment #37
fubhy CreditAttribution: fubhy commentedComment #38
catchInterdiffs look good. Moving back to RTBC and will commit a bit later if there's no more objections.
Comment #39
catchComment #40
catchCommitted/pushed to 8.x, thanks!
Comment #41.0
(not verified) CreditAttribution: commentedblub