Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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 |
---|---|---|---|
#43 | 1987606-diff-39-43.txt | 570 bytes | vijaycs85 |
#43 | 1987606-system-ajax-test-43.patch | 14.11 KB | vijaycs85 |
#39 | system-ajax_test_dialog-1987606-39.patch | 16.19 KB | InternetDevels |
Comments
Comment #1
vijaycs85Need to rewrite the whole module to make test sync with current test implementation. For more details, please refer: #1988802: [META] Rewrite test modules in system to provide better unit testing.
Comment #2
ayelet_Cr CreditAttribution: ayelet_Cr commentedComment #3
mparker17I'll help!
Comment #4
mparker17For some reason, my sidebars disappear after applying this patch and flushing caches. I don't know why.
Comment #5
mparker17Special thanks to @timplunkett for helping me figure out the sidebar bug.
Comment #6
disasm CreditAttribution: disasm commentedreviewed this, and looks good. Marking RTBC.
Comment #7
alexpottI think we can remove the entire menu hook here now that #2032535: Resolve 'title' using the route and render array has landed.
Comment #8
alexpottComment #9
mparker17Try this...
Comment #11
mparker17#9: drupal8.system-module.1987606-9.patch queued for re-testing.
Comment #12
xjmThanks for your work on this issue! Please see #1971384-43: [META] Convert page callbacks to controllers for an update on the routing system conversion process.
Comment #13
disasm CreditAttribution: disasm commentedCan we create a Form for this and return it instead?
Comment #14
rdickert CreditAttribution: rdickert commentedReroll
Comment #16
vijaycs85Re-rolling again - impact of #2091691: Convert test non-form page callbacks to routes and controllers
Comment #17
vijaycs85@dawehner & @timplunkett advised to convert ajax_test_dialog_form as part of this issue, as it is inside this controller conversion.
1. Created a 'Form' namespace and moved existing formController there
2. Create new formController form DialogForm.
3. Moved ajax callback and its helpers (except ajax_test_dialog_contents() - which is covered in https://drupal.org/node/1987612#comment-7982379).
Comment #19
vijaycs85Valid fails. Missed few use statements.
Comment #20
dawehnerThe form test does not yet use the FormBase class, so let's keep this until we use the base class.
Comment #21
vijaycs85Thanks @dawehner. Created follow up for #20 #2116145: Create a new formBase class (or reuse existing formBase) in test classes
Comment #22
vijaycs85updated the status on #2116145-1: Create a new formBase class (or reuse existing formBase) in test classes and issuing new patch with #$this->t()
Comment #23
vijaycs85Wrong diff file in #22. use this.
Comment #24
alexpottRe #20 and #21 I don't understand why we just don't use FormBase here
Comment #25
vijaycs85more review update.
Comment #26
dawehnerdrupal_get_form got replaced by the form builder service.
Comment #27
vijaycs85Here is the update...
Comment #28
Crell CreditAttribution: Crell commentedComment #29
Crell CreditAttribution: Crell commented27: 1987606-system-ajax-test-27.patch queued for re-testing.
Comment #31
vijaycs85Re-rolling...
Comment #32
dawehnerLet's use a doc with the full namespace included.
So need for this extra check_plain
No new endline.
Comment #33
vijaycs85Thanks for the review @dawehner. Here is the updates for #32
Comment #34
dawehnerThank you
Comment #36
vijaycs85Test error: called to undefined function ajax_test_dialog_contents()
ajax_test_dialog_contents() is in use on three different location. So for now leaving it in .module. (locally test case passing).
Comment #37
YesCT CreditAttribution: YesCT commentedinstructions for rerolling: https://drupal.org/contributor-tasks/reroll
Comment #38
star-szrComment #39
InternetDevels CreditAttribution: InternetDevels commentedComment #40
dawehnerI wonder whether we have an issue to fix the problem with ajax_test_dialog_contents().
Comment #41
vijaycs85@dawehner, you mean #1987612: Convert ajax_test_dialog_contents() to a new style controller
Comment #42
Crell CreditAttribution: Crell commentedThis seems redundant.
Otherwise I didn't see any big red flags, at least given that it's a test module. :-)
Comment #43
vijaycs85let's get this in then...
Comment #44
Crell CreditAttribution: Crell commentedBot can complain.
Comment #45
catchCommitted/pushed to 8.x, thanks!