Updated: Comment #25
Problem/Motivation
ConfirmFormBase has a getCancelPath() method, which points to what is usually some kind of parent for the form. This requires a URL string pointing to the router path - i.e. admin/config/foobar
However ConfirmFormBase itself is designed to be used as a route controller - and those, by design are path independent (as will their 'parent' be when those are converted. So really I'd expect this method to provide arguments to pass to the generator as opposed to an actual path - then if the parent gets moved somewhere, that'll work too.
Proposed resolution
Use the new route capabilities of '#type' => 'link',
and introduce a getCancelRoute()
method to replace getCancelPath()
.
Remaining tasks
Get blockers in
User interface changes
N/A
API changes
ConfirmFormInterface::getCancelPath() is deprecated and slated for removal pre-8.0
Related Issues
- #1863906: [PP-1] Replace content revision table with a view
- #1974210: Convert admin/structure/forum to a Controller
- #1986606: Convert the comments administration screen to a view
- #1987778: Convert node_show() and node_page_view() to a new style controller
- #1987802: Convert path_admin_overview() to a new style controller
- #1987814: Convert system_admin_menu_block_page() to a new style controller
- #1987818: Convert system_date_format_language_overview_page() to a new style controller
- #2021161: Replace the fallback node listing with a list controller
- #2082071: Convert language_negotiation_configure_browser_form to a Controller
Comment | File | Size | Author |
---|---|---|---|
#35 | drupal8.forms-system.1963394-35.patch | 47.56 KB | jibran |
#35 | interdiff.txt | 772 bytes | jibran |
#31 | confirm-routes-1963394-31.patch | 47.57 KB | tim.plunkett |
#29 | interdiff.txt | 813 bytes | tim.plunkett |
#29 | confirm-routes-1963394-29.patch | 48.55 KB | tim.plunkett |
Comments
Comment #1
catchComment #2
tim.plunkettComment #3
jibranTagging.
Comment #4
jibrancross post.
Comment #5
Crell CreditAttribution: Crell commentedOh drat. That's completely right, but will be annoying to change now that conversions have started.
Comment #6
tim.plunkettSince this is used for #type => link, this is blocked on #2047619: Add a link generator service for route-based links
Comment #7
effulgentsia CreditAttribution: effulgentsia commentedComment #8
tim.plunkettThis defers to a route name if present, but leaves in paths for now (not all of the paths used as the cancel link are converted yet).
I've converted two for now, one regular and one with parameters.
Oh how I long for traits...
Also, #2073813: Add a UrlGenerator helper to FormBase and ControllerBase will let us remove the \Drupal::urlGenerator() calls.
Comment #9
dawehnerI would like to see a @deprecated on getCancelPath and suggest people to use getCancelRoute
Comment #10
dawehnerNote: the url generator generates a path with "/" at the front, so this won't work. Instead of using the url generator here you should better use the new functionality of #type link to pass in route_name and route_parameters
Better point to generateFromRoute
Comment #11
tim.plunkettThe amount of duplicated code here is depressing.
Comment #12
dawehnerWhat blocks us from having a helper object which does just that? Composition/reuse of existing code without inheritance is not bad.
Comment #13
tim.plunkettVery good point!
Also wrote a unit test for the helper.
Comment #14
dawehnerLet's also one @group Drupal, so you can run just them.
In general I am not sure about the getCancelRoute method name, but getCancelRouteInformation sounds odd.
Comment #15
tim.plunkettI think in the context of this class, getCancelRouteInformation() is overly verbose. There is no confusion about whether you should return a Route object or not, I think the current name is fine.
Added the @group, and removed some obsolete use statements.
Comment #16
dawehnerThank you
Comment #17
webchickIck. I don't think it makes sense to have both of these methods. We should pick one and use it everywhere. If anything needs to use the path, it should be converted in this patch IMO.
Comment #18
dawehnerThe problem is that we don't have routes everywhere yet, so what about marking getCancelPath as depricated as recommend to use getCancelRoute if possible ...
Oh see the patch already does that.
Once everything is converted we can remove it.
Comment #19
webchickMy concern is specifically around these "base" classes defining both. That's the primary way that module developers are going to grasp the D8 APIs. Yes, if you use a sophisticated IDE like PHPStorm, {@inheritdoc} will fill in the blanks, but if you don't, it won't. It looks like you need to implement both methods, since both are public.
Tim and I discussed marking them more explicitly deprecated in the base classes, and that would address my concern.
Comment #20
dawehnerDoes that mean that you suggest to even keep this functionality for a long time? It feels wrong to keep getCancelPath() until the release.
Comment #21
webchickNo, I want to see it removed before release. Which... turns this normal bug into a critical task which... again I'm wondering if it's better to just postpone it on route conversions. :(
Comment #22
tim.plunkettThere are 9 blockers:
(EDIT: moved to the issue summary)
I'd much rather get this in, and fix it in those 9 issues...
Comment #24
tim.plunkettThis is your brain. This is your brain on Durpal.
Comment #25
tim.plunkett7 conversions remain.
Found a fun bug in assertUrl() in the process.
It turns out that neither Drupal nor any web browser care about the difference between these two:
But simpletest did care.
Comment #26
effulgentsia CreditAttribution: effulgentsia commentedTo address #19, what if we remove getCancelPath() from the interface, and remove the above code block. Then, for each form that still needs to use a path, it can override buildForm(), call parent::buildForm(), then do something like:
Comment #27
webchickOh, that is lovely.
Comment #28
tim.plunkettBrilliant idea, thanks @effulgentsia.
I think #2073813: Add a UrlGenerator helper to FormBase and ControllerBase is about to go in (and I had to include a Drupal:: call with a @todo for that), but I wanted to post this before I went to lunch to see what the bot thinks.
Comment #29
tim.plunkettYay, that went in. We should be done here now.
Comment #30
dawehnerOne general question: this pattern will potentially appear a lot, so returning route name, parameters and potentially options for the link,
so do we really want to force people to specify 'route_name', route_parameters all the time and not use something simpler like:
Even then you still have a fallback ...
Should we just use $this->entity->uri() instead? But yeah this is temporary so I don't care.
Comment #31
tim.plunkettI debated skipping the key names, but it just seemed messier and more confusing.
For both 1+2, I'd rather just stick with the path as-is until the conversion happens.
Straight reroll.
Comment #32
jibranI think it is ready to fly.
TBH I don't like this but I am happy that we are removing a WTF situation of keeping geCancelPath.
Comment #33
webchickAwesome work!
Going to leave this at RTBC for a couple of days for comment, esp. from other WSCCI-ans and/or core maintainers, but IMO this looks great, and doesn't leave us in an unstable state regarding D8's APIs.
Comment #34
alexpott#2077513: Refactor ControllerBase to be more consistent with FormBase has changed getUrlGenerator to urlGenerator
Comment #35
jibranThis is a trivial change so back to RTBC.
Comment #36
webchickAll right then! :)
Committed and pushed to 8.x. Thanks!
Will need a change notification on this.
Comment #37
tim.plunkettI updated https://drupal.org/node/1945416 which has the details of the conversion, but also opened https://drupal.org/node/2089281 to notify people of the change.
Comment #38.0
(not verified) CreditAttribution: commentedupdated