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
Convert below form callback to new form builder.
_form_test_checkbox
_form_test_disabled_elements (requires more significant changes)
_form_test_input_forgery
_form_test_tableselect_colspan_form
_form_test_tableselect_empty_form
_form_test_tableselect_js_select_form
_form_test_tableselect_multiple_false_form
_form_test_tableselect_multiple_true_form
_form_test_vertical_tabs_form
For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.
Comments
Comment #1
nano_monkey CreditAttribution: nano_monkey commentedComment #2
nano_monkey CreditAttribution: nano_monkey commentedHere's a patch that converts the first 3 on the list - needs testing.
Comment #3
nano_monkey CreditAttribution: nano_monkey commentedComment #5
nano_monkey CreditAttribution: nano_monkey commentedNew patch - fixing hidden: false in module .info.yml file
Comment #6
nano_monkey CreditAttribution: nano_monkey commentedDiff file for the 2 patches submitted today
Comment #7
disasm CreditAttribution: disasm commentedThis is looking good. Here's some minor changes:
change to $this->t(
Just return the JsonResponse. Don't send/exit.
Comment #8
disasm CreditAttribution: disasm commentedaddressing my comments in #7.
Comment #9
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 #10
dawehnerThere is no code removed from the patch ... this feels wrong.
This change is confusing.
We maybe should put title on there?
Comment #11
disasm CreditAttribution: disasm commentedI didn't catch that in my review. Lets hack some code away! Titles are restored to the routes as well, although I don't think there very meaningful (they're all "Form test".
Comment #12.0
(not verified) CreditAttribution: commentedupdating form item list.
Comment #13
piyuesh23 CreditAttribution: piyuesh23 at QED42 commentedPatch re-rolled.
Comment #14
piyuesh23 CreditAttribution: piyuesh23 commentedPatch re-rolled.
Comment #16
piyuesh23 CreditAttribution: piyuesh23 at QED42 commentedUploading another re-rolled patch
Comment #18
InternetDevels CreditAttribution: InternetDevels commentedComment #19
ianthomas_uksimple reroll
Comment #20
ianthomas_uk#18 was not helpful without further explaination or even an interdiff. It's quite hard to read from just diffing the patches, but mostly it is converting some more forms, so that's good.
But it also restored use of
_form_test_submit_values_json()
for no aparent reason and dropped the conversion of_form_test_disabled_elements()
(which I agree with, since it looks like that test needs more serious changes)In #7 disasm said that FormTestInputForgeryForm could just return the JsonResponse object instead of calling send() and exit, but doing this results in the page being rendered in HTML. Is the submit handler therefore totally redundant for FormTestInputForgeryForm?
I'm uploading a new patch that stops using
_form_test_submit_values_json()
and removes some more legacy code. I've included the send() and exit code in FormTestCheckboxForm but not in FormTestInputForgeryForm, as per #7.Comment #21
vijaycs85etc...
Why not under 'Form' ?
missing docblock
Comment #22
jackbravo CreditAttribution: jackbravo commentedRe-roll of the patch.
@vijaycs85, it is still missing documentation of the classes yet, the new test form classes are not in the Form subdirectory just because other classes where already there also, but no other special reason.
Comment #23
jackbravo CreditAttribution: jackbravo commentedAre these tests even being run? Or how do I run them?
When I run "drush test-run" I don't see any of the tests that I modified that live on module/system/tests:
- core/modules/system/tests/modules/form_test/src/FormTestCheckboxForm.php
but only other tests that live on module/system/src like:
- core/modules/system/src/Tests/Form/CheckboxTest.php
Am I looking on the wrong place?
Comment #24
jvandyk CreditAttribution: jvandyk commentedComment #25
xjm22: drupal-convert_to_form_builder_in_form_test-2078867-22.patch queued for re-testing.
Comment #26
xjm@jackbravo, Thanks for your help on this issue!
The classes in the patch are not automated tests themselves. They are "fake" content implementing core functionality, and they are used by test classes to test that functionality. You can figure out where the test forms are used by looking in the routing file in the test module (and the patch) and then seeing what tests use those routes. Maybe a starting point:
Hope this helps!
Comment #27
jackbravo CreditAttribution: jackbravo commentedOk, gotcha. Thanks @xjm.
I think must of these tests can be activated using
drush test-run "Drupal\system\Tests\Form\FormTest"
I'm getting some fails now on my local dev. I'm gonna change status to send the patch to the test bot again just to verify.
Comment #28
jackbravo CreditAttribution: jackbravo commentedSending last patch to the testbot.
Comment #30
jackbravo CreditAttribution: jackbravo commentedActually, the command that ran perfectly from drush was:
There could be some cleanup now, like some routes are form_test while others are form-test. I can add this to the patch.
What else is missing?
Comment #31
jackbravo CreditAttribution: jackbravo commentedOk, I see what's missing. I'll try to migrate other functions to the new module.
Also here is the new patch with the standarized test-form path instead of test_form.
Comment #36
tim.plunkettHere's a reroll.
Comment #37
tim.plunkettHow is this different from #2030165: Convert form_test_* functions to classes?
Comment #38
tim.plunkettOkay this is just the underscore-prefixed ones.
This one is done, might as well get it done now before starting in on the other issue.
As I just rerolled, I feel comfortable RTBCing this. Good work everyone!
Comment #39
webchickCommitted and pushed to 8.x. Thanks!
Comment #41
jackbravo CreditAttribution: jackbravo commentedI think this still had a lot to go. More test cases need to be migrated. But that can be continued on #2030165: Convert form_test_* functions to classes so I wont reopen this.