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.
Updated: Comment #N
Problem/Motivation
Converting all form callbacks that haven't been converted yet to form controllers
Proposed resolution
create FormBase for each form, return drupal_get_form() of the old form callback in buildForm()
Remaining tasks
User interface changes
API changes
Related Issues
Follow-up from #1971384: [META] Convert page callbacks to controllers.
Comment | File | Size | Author |
---|---|---|---|
#27 | form-non-test-2089671-27.patch | 46.61 KB | tim.plunkett |
#27 | interdiff.txt | 6.7 KB | tim.plunkett |
#25 | form-non-test-2089671-25.patch | 40.39 KB | tim.plunkett |
#22 | drupal8.forms-system.2089671-22.patch | 47.58 KB | disasm |
#22 | interdiff.txt | 6.73 KB | disasm |
Comments
Comment #1
disasm CreditAttribution: disasm commentedassigning to myself to get this started. Will unassign before leaving the country in two days.
Comment #2
disasm CreditAttribution: disasm commentedpath module
Comment #3
tim.plunkettThis issue should be just for
'page callback' => 'drupal_get_form'
.Comment #4
tim.plunkettAnd you might want to just call drupal_get_form in a vanilla Controller, and skip FormBase completely for now.
Comment #5
disasm CreditAttribution: disasm commentedFirst pass at this. Most of this is untested, so expecting some failures. I know there's one in book I was getting locally. Shortcut module still needs access checkers (access: TRUE currently).
Comment #6
disasm CreditAttribution: disasm commentedhelps if I upload the right patch...
Comment #7
tim.plunkettWhy is this needed?
I don't know of many other places we use KILL.
Should be StaticAccessCheckInterface
use getRequirement() instead
If you type hint this, it should work just fine.
the ALL is overkill
StaticAccessCheckInterface
Comment #8
disasm CreditAttribution: disasm commentedAddresses all but #6. Shortcut doesn't have access checks written yet, and I'm planning on combining a mix of access checks and permission checks for those possibly, so leaving the _access_checks: 'ALL' there for now.
I'm not sure if this will work with #1 removed, but we'll see. Last time I tried to access an upcasted argument in an access checker without a param converter specified, it failed pretty bad, but that was a while ago. That might be functioning now.
Comment #10
disasm CreditAttribution: disasm commentedmost of those failures are from pattern -> path in routing.yml. Here's a reroll on that. Also fixing applies -> appliesTo since we're using Static access checker.
Comment #12
effulgentsia CreditAttribution: effulgentsia commentedHow long is "for now"? If it's just to get this patch passing first, then convert from _content to _form still in this issue prior to commit, then ok. But is there a reason to commit form routes to HEAD that incorrectly use _content instead of _form?
Comment #13
tim.plunkettBy for now, I mean, commit this as is (once it's green). The goal here is to remove all page callbacks as quickly as possible, not to kill drupal_get_form().
At least that was my understanding of Phase 1 of these new conversions. Properly chopping up the magically-named validate and submit callbacks and making them methods on a class and handing getFormID should be part of Phase 2 (especially if we do decide to go ahead with #2089593: Have FormBase auto-generate form IDs, so you don't need to implement getFormID() on all forms)
Comment #14
disasm CreditAttribution: disasm commentedFixes content translation tests. That means the single failure in book.module and fixing shortcut module are all that remain.
@effulgentsia This is a crappy hack, we know that. There's a few reasons for doing it this way:
Comment #15
disasm CreditAttribution: disasm commentedfixes book. Needed _access_checks -> _access_mode. Also, renamed it something that looks more like an access checker in the route.
Comment #16
disasm CreditAttribution: disasm commentedComment #17
effulgentsia CreditAttribution: effulgentsia commentedI agree with most of the reasoning in #13 and #14.
I don't mind this crappy hack at all.
But I'm concerned about this existing in HEAD for any longer than absolutely necessary. I think contrib developers will already be struggling a bit to understand the differences between _controller, _content, _form, and _entity_form without us giving them incorrect and contradictory examples.
But I don't want to derail this issue. Would you instead be ok with me rolling a patch after this one gets committed that immediately converts these to _form, and therefore, the classes to extend FormBase, and have wrapper methods for validateForm() and submitForm() that call back to the procedural ones, and getting that follow up patch in quickly, before doing the full phase 2?
My suggestion wouldn't derail that, would it? You would still be able to replace the temporary form classes with the real ones from those patches.
Comment #18
disasm CreditAttribution: disasm commentedI'm fine with the suggestion in #17 as long as it doesn't put a hold on doing proper conversions like this set of issues is doing.
Comment #19
tim.plunkettOverall, looks good! Some of those access checks look ugly as hell, but content_translation has bizarre needs.
Missing blank line
Here, and elsewhere, missing a blank line
Missing a
return static::DENY;
at the end.Comment #20
tim.plunkettOh, also. The temporary methods should have @todo, and the callbacks you wrap should be marked @deprecated
Comment #21
disasm CreditAttribution: disasm commentedaddressing #19 and #20.
Comment #22
disasm CreditAttribution: disasm commentedtypo on deprecated.
Comment #24
disasm CreditAttribution: disasm commented#22: drupal8.forms-system.2089671-22.patch queued for re-testing.
Comment #25
tim.plunkett@disasm went on vacation. Rerolling after the non-test non-form one went in.
Comment #27
tim.plunkettMessed up the content_translation merge, and fixed a bug with user/reset
Comment #28
jibranI think it is good to go. I am unable to find anything objectionable in the patch.
I just RTBC #2063401: Replace the default _access_checks(access mode) with ALL instead of ANY
Comment #29
alexpottCommitted 523c8b6 and pushed to 8.x. Thanks!