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.
Part of #1971384: [META] Convert page callbacks to controllers
For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.
Comment | File | Size | Author |
---|---|---|---|
#41 | entity-form-1987692-41.patch | 8.19 KB | tim.plunkett |
#38 | entity-form-1987692-38.patch | 8.25 KB | tim.plunkett |
#34 | entity_get_form-1987692-34.patch | 9.67 KB | tim.plunkett |
#30 | drupal8.menu-system.1987692-30.patch | 9.42 KB | becw |
#30 | drupal8.menu-system.1987692-30.interdiff.txt | 1.24 KB | becw |
Comments
Comment #1
CBComment #2
CBUsing #1978946: Convert comment_edit_page() to a Controller as an example here. (And adding this comment as a reminder to myself.)
Comment #3
amateescu CreditAttribution: amateescu commentedentity_get_form()
is not a form controller, it's just a procedural helper :) And it's going away in #1982980: Move entity_get_form to Drupal\Core\Entity\EntityManager::getForm() anyway..Comment #4
CBYep, been chatting with larowlan about this today.
Comment #5
oenie CreditAttribution: oenie commentedHi,
Since you guys seem to know what's going on on the entity-side of things, could this issue be of the same type:
#1987698 : Convert entity_view() to a new style controller
As with entity_get_form, entity_view is in entity.inc
Any ideas ?
Just looking to see if i can help a hand on conversion issues, preferably clear ones that need to be done :)
Comment #6
amateescu CreditAttribution: amateescu commentedYeah, that's the same situation. Thanks for bringing it up :)
Comment #7
larowlanEven though entity_get_form is going OO
We still need to convert all entries in hook_menu where the page callback is entity_get_form to use _entity_form and routing.
Updating issue title.
Comment #8
CBPatch attached.
Comment #9
CBWoops, fixing statuses.
Comment #11
larowlanneeds leading /
this looks like left-over from another patch?
I think you need to single quote these?
needs a new line
The fails are down to missing include files. eg the cancel form is in the user.pages.inc but we're not loading that now. Those forms should really become their own page callbacks, there is one for the vocabulary one already, not sure about the user cancel.
should be menu_link.default - will fix a swathe of the fails
Comment #12
CBWoops, I started writing Controllers in a typical approach to WSCCI conversion. I forgot to delete it before I created a patch. :)
I had them single quoted but found a whole bunch of instances where they weren't quoted (user.routing.yml for example.) So I assumed that they shouldn't be. Fixed now.
Sweet!
Thanks larowlan!
Comment #13
CBComment #15
CBComment #16
ParisLiakos CreditAttribution: ParisLiakos commentedlooks like a great cleanup:)
lets wait for #1946456: Convert taxonomy_term_confirm_delete() and taxonomy_vocabulary_confirm_delete() to the new form interface thats RTBC so we can get rid of the @todo as well
Comment #17
tim.plunkettComment #18
becw CreditAttribution: becw commentedThe patch in #15 is very out of date. I'm going to steal this issue and re-rolling the patch; here is just the user edit portion. More coming shortly.
Comment #19
becw CreditAttribution: becw commentedThe original patch had routes for aggregator feed, menu link, taxonomy term and vocabulary, and user entity edit forms, but the menu link route updates have already been applied as part of some other issue.
Here's an updated patch that updates the remaining uses of
entity_get_form()
inhook_menu()
.Comment #20
tim.plunkettWow, thanks for jumping on this!
Technically this should be something like "Provides a submit handler for the 'Cancel account' button."
We should make the buildForm store the request, and reuse it here.
Instead of all this to get $account, you can just use
$this->entity
, that pattern was for external code. Also, can we swap the double quotes for single quotes?Comment #22
becw CreditAttribution: becw commentedI've made the changes you suggested, and I also took the opportunity to replace
$GLOBAL['user']
with$this->request->attributes->get('_account');
.Testbot reported some issues too, but I haven't fixed those yet. I'll leave this as "needs work" until I finish addressing those.
Comment #23
becw CreditAttribution: becw commentedThe failing tests are at least in part due to issues with the tests themselves:
Drupal\taxonomy\Tests\TermTest
includes a test that ensures thatentity_get_form()
does not accept additional arguments from the URL. This test isn't relevant anymore since the new routing system doesn't route these paths to the term edit page:Drupal\user\Tests\UserCancelTest
'stestUserCancelInvalid()
seems to have a bug where it doesn't confirm the account cancellation correctly.Additionally, the new request dependency in
ProfielFormController
was missing the "use" statement at the top of the file.Here's an updated patch, with an interdiff to the patch from the re-roll in #19.
Comment #25
becw CreditAttribution: becw commentedThere were some changes to
aggregator.module
in the past few hours. Here's a re-roll, again with an interdiff against the patch in #19.Comment #27
becw CreditAttribution: becw commentedI get different test failures when I run these tests locally, so I'm not quite sure what's going on. I'm going to post the same patch once more, without the mis-named interdiff.
Comment #29
becw CreditAttribution: becw commentedHere's an update that fixes dependency injection now that
FormBase
is in (#2059245: Add a FormBase class containing useful methods).Comment #30
becw CreditAttribution: becw commentedtimplunkett pointed out in IRC that this should use
FormBase::getCurrentUser()
.Comment #31
tim.plunkettThis looks perfect now. Thanks so much @becw!
Comment #32
tim.plunkettNote, this has the same hunk that is being discussed in #1987898-40: Convert user_view_page() to a new style controller
Comment #33
alexpottPatch no longer applies
Comment #34
tim.plunkettStraight reroll.
Comment #35
alexpottPatch no longer applies
Comment #36
alexpottComment #37
xjmPoor @tim.plunkett :)
Comment #38
tim.plunkettO_o
Part of this was done in #2062769: Convert taxonomy edit form to a route.
Comment #39
catch#38: entity-form-1987692-38.patch queued for re-testing.
Comment #41
tim.plunkettPatch context conflict, nothing more.
Comment #42
webchickComitted and pushed to 8.x. Thanks!