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.
Commit message:
Issue #1987668 by damiankloip, jibran, h3rj4n, mparker17: Convert config_test() module to routes.
Comment | File | Size | Author |
---|---|---|---|
#25 | 1987668-25.patch | 12.68 KB | jibran |
#25 | interdiff.txt | 1.07 KB | jibran |
#21 | 1987668-21.patch | 12.29 KB | jibran |
#21 | interdiff.txt | 2.11 KB | jibran |
#20 | 1987668-20.patch | 14.39 KB | jibran |
Comments
Comment #1
damiankloip CreditAttribution: damiankloip commentedDoing this now
Comment #2
damiankloip CreditAttribution: damiankloip commentedSeems easier to do this module in one issue...
Here is a first attempt.
Comment #3
damiankloip CreditAttribution: damiankloip commentedWe can use the _entity_form for adding.
Comment #4
tim.plunkettWhat about
Comment #5
tim.plunkettOh you closed those two (with no link? ouch!) but @h3rj4n (mrtime on IRC) has worked pretty hard on that other one...
Comment #6
damiankloip CreditAttribution: damiankloip commentedOops, totally missed that a patch had been posted on the add_page issue, sorry @h3rj4n.
I have updated the issues summary to make sure @h3rj4n also gets commit credit for the conversion.
Comment #7
damiankloip CreditAttribution: damiankloip commentedI spoke to Tim about this, Let's add the default local task back in. There is another issues to fight that one out.
Comment #8
damiankloip CreditAttribution: damiankloip commentedMissed a couple of assertions that needed to be reverted.
Comment #9
tim.plunkettBlocks #1908756: Separate Action Links (MENU_LOCAL_ACTION) from hook_menu()
Comment #10
dawehnerJust to be consistent ... these should be ordered properly.
Maybe i'm stupid, but do you actually use the entity manager at all?
I don't think we should still refer to page callbacks.
Missing "\"
Use String::format instead.
... needs some comment.
Needs doc.
Is there a reason why we set a default value? According to submitForm() we expect it to be valid.
Comment #11
damiankloip CreditAttribution: damiankloip commentedThanks dawehner!
Comment #13
dawehnerI guess that's another issue with the missing /edit.
Comment #14
damiankloip CreditAttribution: damiankloip commentedNo, I think the main issue is that I was still implementing ControllerInterface, but removed the create method as we don;t need it anymore :)
Alas, if we don't specify a default value we cannot add an additional parameter, as we are extending buildForm(array $form, array &$form_state). So that has to be back in the patch.
Comment #16
dawehnerMost of the time we specify the symfony uses after the drupal ones.
We should document the new parameter $config_test in here.
Still uses format_string
Comment #17
damiankloip CreditAttribution: damiankloip commentedThanks Daniel.
Comment #18
jibranAnd this breaks my heart :( #1945444: Convert confirm_form() in config_test.module to the new form interface and convert route
Comment #19
jibranI am marking #1945444: Convert confirm_form() in config_test.module to the new form interface and convert route as duplicate and I think only this is remaing
This should typehint with ConfigTestInterface, but it seems I forgot that one in #1391694: Use type-hinting for entity-parameters from #1945444-26: Convert confirm_form() in config_test.module to the new form interface and convert route point 1.
Comment #20
jibran\Drupal\config_test\Form\DeleteForm
to\Drupal\config_test\Form\ConfigTestDeleteForm
Comment #21
jibranForgot to remove
\Drupal\config_test\Form\DeleteForm
in last patch.Comment #22
kim.pepper#21: 1987668-21.patch queued for re-testing.
Comment #23
jibran#1987662: Convert config_test_add_page() to a new style controller is another duplicate
Comment #23.0
jibranUpdated issue summary.
Comment #23.1
tim.plunkettupdated commit message
Comment #24
tim.plunkettAdded a commit message to the issue summary
Comment #25
jibranImplemented
hook_local_actions()
.Comment #26
dawehnerLooks great all in all.
Comment #27
alexpottwe've already used the drupal_set_title() in edit() we could do the same in the other controller methods and the buildForm method of the confirmForm and then hook_menu is history... :)
Comment #28
alexpottIgnore me... let's wait till we have a proper solution for page titling.
Comment #29
alexpottCommitted 8355142 and pushed to 8.x. Thanks!
Comment #30.0
(not verified) CreditAttribution: commentedUpdated issue summary.