Part of #1971384: [META] Convert page callbacks to controllers and sub-set of #2078867: Convert _form_test_* functions to classes
Convert below form callback to new form builder.
form_test_alter_form
form_test_button_class
form_test_checkboxes_radios
form_test_checkboxes_zero
form_test_clicked_button
form_test_color
form_test_email
form_test_empty_select
form_test_form_rebuild_preserve_values_form
form_test_form_state_values_clean_advanced_form
form_test_form_state_values_clean_form
form_test_group_container
form_test_group_details
form_test_group_fieldset
form_test_group_vertical_tabs
form_test_language_select
form_test_limit_validation_errors_form
form_test_load_include_custom
form_test_load_include_menu
form_test_number
form_test_pattern_form
form_test_placeholder_test
form_test_range
form_test_range_invalid
form_test_redirect
form_test_required_attribute
form_test_select
form_test_state_persist
form_test_storage_form
form_test_url #2030165: Convert form_test_url() to a Controller Assigned to: rteijeiro
form_test_validate_form
form_test_validate_required_form
form_test_validate_required_form_no_title
For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.
Comment | File | Size | Author |
---|---|---|---|
#50 | form_test-2030165-50-interdiff.txt | 706 bytes | Berdir |
#50 | form_test-2030165-50.patch | 164.43 KB | Berdir |
#46 | interdiff.txt | 990 bytes | tim.plunkett |
#46 | form_test-2030165-46.patch | 164.36 KB | tim.plunkett |
#40 | form_test-2030165-40.patch | 103 KB | tim.plunkett |
Comments
Comment #1
juampynr CreditAttribution: juampynr commentedClosing as per #1988802: [META] Rewrite test modules in system to provide better unit testing.
Comment #2
vijaycs85Comment #3
vijaycs85Comment #4
mrded CreditAttribution: mrded commentedComment #5
vijaycs85@mrded, are you still working on this?
Comment #6
mrded CreditAttribution: mrded commented@vijaycs85 yes, it's quite a lot to do
Comment #7
YesCT CreditAttribution: YesCT commentedMrded, quite a lot of these are assigned to you. I would suggest just keeping one and post the work you have so far. It does not have to work or be perfect. Lets get it out where it can be seen and get some feedback on the approach and the details.
Comment #8
mrded CreditAttribution: mrded commentedComment #8.0
mrded CreditAttribution: mrded commentedupdating issue with new list of functions
Comment #9
tim.plunkettThere is a very good chance we don't want to convert most of these, as they should become phpunit tests.
I have an example somewhere, I'll post it when I get back to my laptop.
Comment #10
tim.plunkettUm, I thought I posted back here again.
Anyway, see #1987716: Provide a FormTestBase class to allow PHPUnit form tests for an example.
Comment #11
dipen chaudhary CreditAttribution: dipen chaudhary commented@tim.plunkett
How does one recognise which ones should be phpunit tests? I was hoping to pick one of these, but now confused. Happy to work on either phpunit conversion or controller conversion, but limited knowledge on deciding which one should be which? Any pointers?
Comment #12
tkuldeep17 CreditAttribution: tkuldeep17 commentedFor converting all these menu item into Form object, I am thinking to follow this approach
Drupal\system\Tests\Form\
So I am submitting patch for
Drupal\system\Tests\Form\ValidationTest.php
test case. Following menu items have been converted into Form object.form_test_limit_validation_errors_form
form_test_validate_form
form_test_validate_required_form
form_test_pattern_form
Comment #13
tkuldeep17 CreditAttribution: tkuldeep17 commentedso I am submitting patch..
Comment #17
vijaycs85Fixing a minor issue made on #15
Comment #20
tkuldeep17 CreditAttribution: tkuldeep17 commentedAlmost I have all converted menu items into Form object. Tested on my local, these are two test cases which are failing from
Drupal\system\Tests\Form\ElementsTableSelectTest
.. and not able to find out cause..So please help me here..
Comment #21
tkuldeep17 CreditAttribution: tkuldeep17 commentedSubmitting updated patch..
Comment #23
tkuldeep17 CreditAttribution: tkuldeep17 commentedFixing #21.
Comment #24
tkuldeep17 CreditAttribution: tkuldeep17 commented@vijaycs85
Regarding Name Convention: Currently if form_id was
form_test_clicked_button
it changed to Form object\Drupal\form_test\Form\FormTestClickedButton
but in some cases form_id was too long like
form_test_form_rebuild_preserve_values_form
changed to\Drupal\form_test\Form\TestRebuildPreservation
.So at somewhere Name of class generating Form is following form_id, somewhere not..
I think there should be consistency in Form naming convention.
Should I change name of class or not.?
Comment #25
tkuldeep17 CreditAttribution: tkuldeep17 commentedEven after converting these menu Items into Form Object, still there are lot of menus, which has to be converted.
And also Test case classes is using Simpletest not Unittest. I am getting confusion, it will remain as Simpletest or it will be changed to Unittest.
Comment #26
ianthomas_ukIn #9/#10, @tim.plunkett suggested that we should be replacing these tests with PHPUnit tests, rather than just shuffling round the existing tests, which is what #12-25 have been doing.
But I've seen concern from @sun on IRC that the PHPUnit tests mock too much, so are not really testing what they claim to be. I can certainly understand this view point, e.g. looking at #1987716-40: Provide a FormTestBase class to allow PHPUnit form tests there is a test testUniqueHtmlId(), but the desired behaviour is being mocked elsewhere in the patch (
TestFormBuilder::drupalHtmlId()
) so isn't that really just testing the mocking code.I think we need further thought from someone familiar with PHPUnit before we continue with this work.
Comment #27
tim.plunkettIt depends on what is being tested. We have no full replacement for hook_element_info in PHPUnit, so anything testing the form types themselves would definitely need to be DUTB.
But in the interest of progress, porting these directly would be fine.
Comment #28
xjm23: convert_menuitems_to_form_object-2030165-23.patch queued for re-testing.
Comment #30
xjmComment #31
tim.plunkettAttempted reroll, but going to work on this also.
Comment #33
tim.plunkettSo how is this different from #2078867: Convert _form_test_* functions to classes?
I'm worried that this needs to be done from scratch again :(
Comment #34
jackbravo CreditAttribution: jackbravo commentedIt is probably a different subset of the forms. But it would be better to have them on just one issue I guess. Otherwise the first issue that gets in will make the work for the other one harder (a re-roll).
Well, that is my guess. But maybe there are good reasons to separate them.
Comment #35
sunMost of this seems to be covered by #2078867: Convert _form_test_* functions to classes already? We probably need an issue title + summary update here.
Nevertheless, fixing the typo in the issue title. :P
Comment #36
tim.plunkett#2078867: Convert _form_test_* functions to classes went in. That just handled the ones prefixed with an underscore.
Comment #37
jackbravo CreditAttribution: jackbravo commentedMore tests needed to be migrated on the other issue. So that work will need to be completed here now I guess.
Comment #38
tim.plunkettWorking on this from scratch. This is just a start, but I want to make sure I'm on the right track while I do more.
Comment #40
tim.plunkettHere's a bit more.
Comment #42
tim.plunkett12 left for tomorrow, this should pass in the meantime.
Comment #44
tim.plunkettThis should be all of them!
Comment #46
tim.plunkettMissed a spot.
Comment #47
scottrigbyComment #48
scottrigbyThis looks great, and should go in so procedural forms can die the quickest death possible.
BTW on a code standards note: I think for large procedural to class conversions like this, it's silly to hold it up for the 2 spaces some comment lines might go over 80 chars (later if someone wants to open a cleanup task for things like this, it might actually be fun to write a script to do it across many files).
Comment #49
alexpottComment #50
BerdirThis conflicted because of a small docblock change, back to RTBC. Manual diff of the patch shows that it's identical except of that change, so back to RTBC.
Comment #51
scottrigbyEDIT: changed comment. Thanks! #50 works.
Comment #52
webchickOk, awesome work. Getting this in while it's hot!
Committed and pushed to 8.x. Thanks!
Comment #54
scottrigbySweet :)