Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Convert these page callbacks to new-style Controllers, using the instructions on http://drupal.org/node/1800686
Comment | File | Size | Author |
---|---|---|---|
#34 | ban-1944910-34.patch | 14.33 KB | tim.plunkett |
#34 | interdiff.txt | 3.6 KB | tim.plunkett |
#31 | interdiff.txt | 10.18 KB | tim.plunkett |
#31 | ban-1944910-31.patch | 14.33 KB | tim.plunkett |
#21 | ban-1944910-21.patch | 12.47 KB | barraponto |
Comments
Comment #1
tim.plunkettWorking on this, since it will be the first new example of #1938600: Add a FormInterface replacement for confirm_form() as well.
Comment #2
tim.plunkettOkay, so I clearly am either missing something with how routes work with breadcrumbs and menu trees, or this is busted.
Here's two versions of the patch, one as I thought it should be, and one that is hacky but works.
Comment #3
xjmThe parameter is optional, but not marked as such. What happens when it's empty?
Also, the docs say
$ban_ip_id
is an int, but you have a thing called$ban_ip
that defaults to an empty string, but the protected property$banIP
is an empty array...Comment #4
Crell CreditAttribution: Crell commentedYou don't have a default for ban_ip listed here, but the buildForm() method lists it as optional.
For routing purposes, default values in the signature of the controller mean diddlysquat. The routing system looks only at the route definition. Really, optional parameters in the controller signature itself are just a confusion at this point. (It has to do that, because that's how /foo/bar and /foo/bar/{baz} can be differentiated before the controllers are known.)
Comment #5
tim.plunkettComment #6
tim.plunkett#2: ban-1944910-2.patch queued for re-testing.
Comment #7
tim.plunkettThis is blocked on #1959122: Optional params are not possible in routes
Comment #8
tim.plunkettOkay, just to test the patch from the other issue, here's the failing patch from #2 combined with it.
Comment #9
tim.plunkettThat went in, reroll.
Comment #10
tim.plunkettRerolled, and injected all the things!
Comment #12
tim.plunkettDidn't rebase all the way.
Comment #14
tim.plunkettUGH I keep doing that. Wrong class name.
Comment #15
dawehnerI guess it's a string stored in the DB, right?
Comment #16
jibranBump and fixed #15.
Comment #17
tim.plunkettI think @dawehner meant the opposite.
Comment #18
ParisLiakos CreditAttribution: ParisLiakos commentedno need to inject request, add it as an argument to buildForm() and then store it in $form_state so it can be reused in validator
see http://drupal.org/node/1972260 as example
wont work. either old style, Overrides... or no additional comment. it is being parsed only when alone
new BanDelete
Comment #19
tim.plunkettAbout the inheritdoc, quoting @dawehner:
Comment #20
barraponto CreditAttribution: barraponto commentedRerolled, fixed third issue mentioned in #18. The second issue seems not to be an issue at all and the first one... well I'm clueless.
Comment #21
barraponto CreditAttribution: barraponto commentedMy bad, core doesn't use format-patch...
Comment #22
kim.pepperInvoking the bot.
Comment #23
dawehnerJust in general I'm wondering whether it is good to move direct database calls back to form instead of having some kind of CRUD api.
Comment #24
kim.pepperGood idea @dawehner. There is an BanIpManager already that does db calls. Let's move it there.
Can we move this to BanIpManager::findAll()
Move this to BanIpManager::isBanned($ip)
Move this to BanIpManager::addIp($ip)
Move to BanIpManager::findById()
Move to BanIpManager::delete($iid)
Comment #25
jibran#21: ban-1944910-20.patch queued for re-testing.
Comment #26
Crell CreditAttribution: Crell commentedThe request should not be a constructor parameter. Instead, specify Request $request as an optional parameter to buildForm() and it will get passed in by the routing system there. You can then save it to a property if you need it for the validate and submit methods.
Agreed with adding a separate service object for handling the DB interaction. I'd suggest BanIpRepository as the name.
Comment #27
tim.plunkettpassing $request to buildForm() is blocked on #1998166: Use the controller resolver to inject parameters into HtmlFormController
Comment #28
kim.pepperCan we use
\Drupal::request()
and mark it as a follow-up to pass it in when #1998166: Use the controller resolver to inject parameters into HtmlFormController is in?@Crell BanIpRepository, eh? @msonnabaum would be so proud. :-)
Comment #29
kim.pepper@Crell. Just re-read the issue and we already have BanIpManager service that is doing db lookups. Suggest we use that, and file a separate issue to rename it to BanIpRepository.
Comment #30
Crell CreditAttribution: Crell commentedAgreed on #29.
Comment #31
tim.plunkettI went with banIP() and unbanIP() instead of addIP() and delete(), it seemed more intuitive.
Comment #32
kim.pepperI thought the naming convention would make this banIp() rather than banIP()?
Comment #33
kim.pepperIbid
Comment #34
tim.plunkettUgh I hate that standard. As @jhodgdon would say, "Ego, Id, Superego"
Comment #36
tim.plunkett#34: ban-1944910-34.patch queued for re-testing.
Comment #37
tim.plunkettComment #38
Crell CreditAttribution: Crell commentedTim's all about the nearly-total module rewrites lately. :-)
Comment #39
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks.
Comment #40
moshe weitzman CreditAttribution: moshe weitzman commentedFYI, our review of this patch led us to suggest removing one docs line from all .twig files - #2013094: [policy adopted, patch needed] Stop saying '@see template_preprocess()' in every twig file
Comment #41.0
(not verified) CreditAttribution: commentedFixing link