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.
From meta issue: #1971384: [META] Convert page callbacks to controllers
Comment | File | Size | Author |
---|---|---|---|
#53 | 1992606-53.patch | 39.75 KB | pwieck |
#53 | 1992606-46-53-interdiff.txt | 249 bytes | pwieck |
#47 | 1992606-46.patch | 39.74 KB | damiankloip |
#44 | 1992606-44.patch | 39.69 KB | damiankloip |
#43 | 1992606-43.patch | 39.61 KB | damiankloip |
Comments
Comment #1
damiankloip CreditAttribution: damiankloip commentedHere is an initial conversion of the class. I haven't touched hook menu yet, as it iterates through each theme to provide a path/route. Are there any examples in core already of how we plan to implement this with routes?
Comment #2
damiankloip CreditAttribution: damiankloip commentedComment #3
damiankloip CreditAttribution: damiankloip commentedok, added a RouteSubscriber and friends.
Comment #5
damiankloip CreditAttribution: damiankloip commentedThis should improve things a bit.
Comment #7
damiankloip CreditAttribution: damiankloip commented#5: 1992606-5.patch queued for re-testing.
Comment #9
damiankloip CreditAttribution: damiankloip commentedRerolled after #1925660: Convert system's system_config_form() to SystemConfigFormBase
Comment #11
damiankloip CreditAttribution: damiankloip commentedSo the fails are because bartik is enabled, but caches need to be flushed to see the changes in the test env.
Comment #12
tim.plunkettThis doesn't seem right
double space before {
inheritdoc
Regardless of the continuing discussion of what to do with MENU_DEFAULT_LOCAL_TASK, this route_name shouldn't be here.
Comment #14
damiankloip CreditAttribution: damiankloip commentedThanks for the review.
I have just used _permission in the route for the access because the other part of _system_theme_access (system_theme_access) just checks the status of the theme, which we can easily roll into the controller callback. This seems better than having an access checker just for this? Also, only enabled themes will get routes registered, and shouldn't rely on access being denied to hide them.
Fixed t'other things.
Comment #15
damiankloip CreditAttribution: damiankloip commentedIf not, let me know, and I can add an _access_system_theme access checker or something.
Comment #17
damiankloip CreditAttribution: damiankloip commentedNeed to rebuild the router as well are call menu_router_rebuild().
Comment #19
damiankloip CreditAttribution: damiankloip commented#17: 1992606-17.patch queued for re-testing.
Comment #21
damiankloip CreditAttribution: damiankloip commented#17: 1992606-17.patch queued for re-testing.
Comment #22
dawehnerJust use $this->container->get('router_builder')
What is the reason we have to rebuild now and we haven't needed it before?
As talked there is no reason to just create a single route with a parameter.
ModuleHandlerInterface should be used here
Comment #23
damiankloip CreditAttribution: damiankloip commentedOk, let's remove that route subscriber - seems to be pretty obsolete now.
I think we need to have a discussion with some folks about guidelines for access checkers.
Comment #24
damiankloip CreditAttribution: damiankloip commentedForgot to make the ModuleHandlerInterface change. I'll see how the test gets on then change it.
Comment #26
dawehner#23: 1992606-23.patch queued for re-testing.
Comment #28
damiankloip CreditAttribution: damiankloip commentedHopefully this works, I'm bored of this issue.
Comment #30
dawehner#28: 1992606-28.patch queued for re-testing.
Comment #32
damiankloip CreditAttribution: damiankloip commentedOk, so having one route for all the dynamic paths just doesn't work. It will blow up as multiple menu items will have the same route name. I took some inspiration from field UI, and they are using a RouteSubscriber, as I was before.
Here is a new patch with the feedback from #22 but based on the patch in #17, when I had the RouteSubscriber.
Comment #33
damiankloip CreditAttribution: damiankloip commentedComment #35
dawehner#32: 1992606-32.patch queued for re-testing.
Comment #37
damiankloip CreditAttribution: damiankloip commentedThe build args have changed, so the form alter in color.module needs to reflect that.
Comment #38
dawehnerLet's document the interface.
It would be cool to document the ModuleHandler here as well.
While you fix some things anyway: please remove this ugly whitespace.
Comment #39
tim.plunkettPlease revert that last interdiff and include this change instead:
Comment #40
damiankloip CreditAttribution: damiankloip commentedThanks! All those changes make sense.
Comment #41
tim.plunkettThanks, this looks great now.
Comment #42
alexpottNeeds a reroll
Comment #43
damiankloip CreditAttribution: damiankloip commented*sigh* ...
Comment #44
damiankloip CreditAttribution: damiankloip commentedAnd again.
Comment #46
damiankloip CreditAttribution: damiankloip commentedAh, I see what's wrong here.
Comment #47
damiankloip CreditAttribution: damiankloip commentedNeeded a proper reroll after the File entity stuff.
Comment #49
damiankloip CreditAttribution: damiankloip commented#47: 1992606-46.patch queued for re-testing.
Comment #50
tim.plunkettLooks good, thanks.
Comment #51
alexpottNeeds a reroll
Comment #52
pwieck CreditAttribution: pwieck commentedWorking on re-roll now.
Comment #53
pwieck CreditAttribution: pwieck commentedre-rolled.
Comment #54
pwieck CreditAttribution: pwieck commentedremoved tag
Comment #55
tim.plunkettThanks for the reroll!
Comment #56
alexpottCommitted a93de78 and pushed to 8.x. Thanks!