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_url #2030165: Convert form_test_url() to a Controller Assigned to: rteijeiro

For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.


juampynr’s picture

Status: Active » Closed (won't fix)
vijaycs85’s picture

Title: Convert form_test_url() to a Controller » Covert all form menu items to new form builder in form_test module part 2
vijaycs85’s picture

Assigned: rteijeiro » Unassigned
Status: Closed (won't fix) » Active
mrded’s picture

Assigned: Unassigned » mrded
vijaycs85’s picture

@mrded, are you still working on this?

mrded’s picture

@vijaycs85 yes, it's quite a lot to do

YesCT’s picture

Mrded, 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.

mrded’s picture

Assigned: mrded » Unassigned
mrded’s picture

Issue summary: View changes

updating issue with new list of functions

tim.plunkett’s picture

There 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.

tim.plunkett’s picture

Um, I thought I posted back here again.

Anyway, see #1987716: Provide a FormTestBase class to allow PHPUnit form tests for an example.

dipen chaudhary’s picture


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?

tkuldeep17’s picture

For converting all these menu item into Form object, I am thinking to follow this approach

  • Choose any Testcase in form_test module from Drupal\system\Tests\Form\
  • Convert all the menu items to Form object, then test locally and submit patch.
  • By this way, Testing of Form API will become easy. we have to not run all test cases within Form API.

So I am submitting patch for Drupal\system\Tests\Form\ValidationTest.php test case. Following menu items have been converted into Form object.


tkuldeep17’s picture

Issue summary: View changes
Status: Active » Needs review
15.21 KB
FAILED: [[SimpleTest]]: [MySQL] 58,931 pass(es), 112 fail(s), and 11 exception(s). View

so I am submitting patch..

vijaycs85’s picture

14.85 KB
FAILED: [[SimpleTest]]: [MySQL] 59,164 pass(es), 1 fail(s), and 0 exception(s). View
451 bytes

Fixing a minor issue made on #15

tkuldeep17’s picture

Almost 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..

tkuldeep17’s picture

144.81 KB
FAILED: [[SimpleTest]]: [MySQL] 58,698 pass(es), 1 fail(s), and 0 exception(s). View

Submitting updated patch..

tkuldeep17’s picture

Status: Needs work » Needs review
724 bytes
145.22 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch convert_menuitems_to_form_object-2030165-23.patch. Unable to apply patch. See the log in the details link for more information. View

Fixing #21.

tkuldeep17’s picture


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.?

tkuldeep17’s picture

Even 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.

ianthomas_uk’s picture

In #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.

tim.plunkett’s picture

It 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.

xjm’s picture

xjm’s picture

Issue tags: +Needs reroll
tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett
Status: Needs work » Needs review
Issue tags: -Needs reroll +FormInterface
94.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,810 pass(es), 23 fail(s), and 6 exception(s). View

Attempted reroll, but going to work on this also.

tim.plunkett’s picture

So how is this different from #2078867: Convert _form_test_* functions to classes?
I'm worried that this needs to be done from scratch again :(

jackbravo’s picture

It 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.

sun’s picture

Title: Covert all form menu items to new form builder in form_test module part 2 » Convert all form menu items to new form builder in form_test module part 2

Most 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

tim.plunkett’s picture

Title: Convert all form menu items to new form builder in form_test module part 2 » Convert form_test_* functions to classes

#2078867: Convert _form_test_* functions to classes went in. That just handled the ones prefixed with an underscore.

jackbravo’s picture

More tests needed to be migrated on the other issue. So that work will need to be completed here now I guess.

tim.plunkett’s picture

Status: Needs work » Needs review
60.47 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,051 pass(es), 3 fail(s), and 0 exception(s). View

Working 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.

tim.plunkett’s picture

Status: Needs work » Needs review
103 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,906 pass(es), 10 fail(s), and 4 exception(s). View
33.04 KB

Here's a bit more.

tim.plunkett’s picture

Status: Needs work » Needs review
105.3 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,217 pass(es), 10 fail(s), and 20 exception(s). View

12 left for tomorrow, this should pass in the meantime.

tim.plunkett’s picture

Status: Needs work » Needs review
164.37 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,183 pass(es), 2 fail(s), and 2 exception(s). View

This should be all of them!

Status: Needs review » Needs work

The last submitted patch, 44: form_test-2030165-44.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
164.36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,198 pass(es). View
990 bytes

Missed a spot.

scottrigby’s picture

Assigned: tim.plunkett » scottrigby
scottrigby’s picture

Assigned: scottrigby » Unassigned
Status: Needs review » Reviewed & tested by the community

This 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).

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll
git ac
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  164k  100  164k    0     0  89572      0  0:00:01  0:00:01 --:--:-- 90197
error: patch failed: core/modules/system/tests/modules/form_test/form_test.module:81
error: core/modules/system/tests/modules/form_test/form_test.module: patch does not apply
Berdir’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
164.43 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,212 pass(es). View
706 bytes

This 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.

scottrigby’s picture

EDIT: changed comment. Thanks! #50 works.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, awesome work. Getting this in while it's hot!

Committed and pushed to 8.x. Thanks!

  • webchick committed e008d3a on 8.x
    Issue #2030165 by Berdir, tim.plunkett, vijaycs85, tkuldeep17 |...
scottrigby’s picture

Sweet :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.