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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nano_monkey’s picture

Assigned: Unassigned » nano_monkey
nano_monkey’s picture

Here's a patch that converts the first 3 on the list - needs testing.

nano_monkey’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 2078867-first--3--forms--converted-2.patch, failed testing.

nano_monkey’s picture

Status: Needs work » Needs review
FileSize
13.46 KB

New patch - fixing hidden: false in module .info.yml file

nano_monkey’s picture

FileSize
451 bytes

Diff file for the 2 patches submitted today

disasm’s picture

Status: Needs review » Needs work

This is looking good. Here's some minor changes:

  1. +++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/FormTestCheckboxForm.php
    @@ -0,0 +1,106 @@
    +      '#value' => t('Submit')
    
    +++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/FormTestDisabledElementsForm.php
    @@ -0,0 +1,239 @@
    +      '#value' => t('Submit'),
    
    +++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/FormTestInputForgery.php
    @@ -0,0 +1,60 @@
    +      '#value' => t('Submit'),
    

    change to $this->t(

  2. +++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/FormTestDisabledElementsForm.php
    @@ -0,0 +1,239 @@
    +    $response = new JsonResponse($form_state['values']);
    +
    +    $response->send();
    +
    +    exit;
    
    +++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/FormTestInputForgery.php
    @@ -0,0 +1,60 @@
    +    $response = new JsonResponse($form_state['values']);
    +
    +    $response->send();
    +
    +    exit;
    

    Just return the JsonResponse. Don't send/exit.

disasm’s picture

Assigned: nano_monkey » Unassigned
Status: Needs work » Needs review
FileSize
2.29 KB
13.44 KB

addressing my comments in #7.

xjm’s picture

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

dawehner’s picture

Status: Needs review » Needs work

There is no code removed from the patch ... this feels wrong.

  1. +++ w/core/modules/system/tests/modules/form_test/form_test.module
    @@ -221,15 +214,7 @@ function form_test_menu() {
    -  $items['form-test/input-forgery'] = array(
    +  $items['form-test/input-forgery-old'] = array(
    

    This change is confusing.

  2. +++ w/core/modules/system/tests/modules/form_test/form_test.routing.yml
    @@ -67,3 +67,24 @@ form_test.autocomplete_2:
    +
    +form_test.checkbox:
    +  pattern: '/form-test/checkbox'
    +  defaults:
    +    _form: '\Drupal\form_test\FormTestCheckboxForm'
    +  requirements:
    +    _access: 'TRUE'
    +
    +form_test.disabled_elements:
    +  pattern: '/form-test/disabled-elements'
    +  defaults:
    +    _form: '\Drupal\form_test\FormTestDisabledElementsForm'
    +  requirements:
    +    _access: 'TRUE'
    +
    +form_test.input_forgery:
    +  pattern: '/form-test/input-forgery'
    +  defaults:
    +    _form: '\Drupal\form_test\FormTestInputForgery'
    +  requirements:
    +    _access: 'TRUE'
    

    We maybe should put title on there?

disasm’s picture

Status: Needs work » Needs review
FileSize
10.2 KB
22.16 KB

I 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".

The last submitted patch, drupal8.form_test.2078867-11.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

updating form item list.

piyuesh23’s picture

Issue summary: View changes
FileSize
21.54 KB

Patch re-rolled.

piyuesh23’s picture

Status: Needs work » Needs review

Patch re-rolled.

Status: Needs review » Needs work

The last submitted patch, 13: drupal8-form-test-2078867-13.patch, failed testing.

piyuesh23’s picture

Status: Needs work » Needs review
FileSize
21.56 KB

Uploading another re-rolled patch

Status: Needs review » Needs work

The last submitted patch, 16: drupal8.form-test-2078867-16.patch, failed testing.

InternetDevels’s picture

ianthomas_uk’s picture

simple reroll

ianthomas_uk’s picture

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

vijaycs85’s picture

  1. +++ b/core/modules/system/tests/modules/form_test/form_test.routing.yml
    @@ -119,7 +119,7 @@ form_test.pattern:
    -    _content: '\Drupal\form_test\Form\FormTestForm::testTableSelectCheckboxes'
    +    _form: '\Drupal\form_test\FormTestTableSelectMultipleTrueForm'
    
    @@ -127,7 +127,7 @@ form_test.tableselect_checkboxes:
    +    _form: '\Drupal\form_test\FormTestTableSelectMultipleFalseForm'
    

    etc...
    Why not under 'Form' ?

  2. +++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/FormTestCheckboxForm.php
    @@ -0,0 +1,98 @@
    +class FormTestCheckboxForm extends FormBase {
    

    missing docblock

jackbravo’s picture

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

jackbravo’s picture

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

jvandyk’s picture

Title: Covert all form menu items to new form builder in form_test module » Convert all form menu items to new form builder in form_test module
xjm’s picture

xjm’s picture

@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:

[tesla-3:drupal | Wed 21:40:07] $ grep -r "form_test/" * 
core/modules/system/src/Tests/Form/ElementsLabelsTest.php:    $this->drupalGet('form_test/form-labels');
core/modules/system/src/Tests/Form/ElementsTableSelectTest.php:    $this->drupalGet('form_test/tableselect/multiple-true');
core/modules/system/src/Tests/Form/ElementsTableSelectTest.php:    $this->drupalGet('form_test/tableselect/multiple-false');
core/modules/system/src/Tests/Form/ElementsTableSelectTest.php:    $this->drupalGet('form_test/tableselect/colspan');
core/modules/system/src/Tests/Form/ElementsTableSelectTest.php:    $this->drupalGet('form_test/tableselect/empty-text');
core/modules/system/src/Tests/Form/ElementsTableSelectTest.php:    $this->drupalPostForm('form_test/tableselect/multiple-true', $edit, 'Submit');
core/modules/system/src/Tests/Form/ElementsTableSelectTest.php:    $this->drupalPostForm('form_test/tableselect/multiple-true', $edit, 'Submit');
core/modules/system/src/Tests/Form/ElementsTableSelectTest.php:    $this->drupalPostForm('form_test/tableselect/multiple-false', $edit, 'Submit');
core/modules/system/src/Tests/Form/ElementsTableSelectTest.php:    $this->drupalGet('form_test/tableselect/advanced-select/multiple-true-default');
core/modules/system/src/Tests/Form/ElementsTableSelectTest.php:    $this->drupalGet('form_test/tableselect/advanced-select/multiple-true-no-advanced-select');
core/modules/system/src/Tests/Form/ElementsTableSelectTest.php:    $this->drupalGet('form_test/tableselect/advanced-select/multiple-false-default');
core/modules/system/src/Tests/Form/ElementsTableSelectTest.php:    $this->drupalGet('form_test/tableselect/advanced-select/multiple-false-advanced-select');
core/modules/system/src/Tests/Form/ElementsVerticalTabsTest.php:    $this->drupalGet('form_test/vertical-tabs');
core/modules/system/src/Tests/Form/ElementsVerticalTabsTest.php:    $this->drupalGet('form_test/vertical-tabs');
core/modules/system/src/Tests/Form/ElementsVerticalTabsTest.php:    $this->drupalGet('form_test/vertical-tabs');
core/modules/system/src/Tests/Form/StateValuesCleanAdvancedTest.php:    $this->drupalPostForm('form_test/form-state-values-clean-advanced', $edit, t('Submit'));
core/modules/system/src/Tests/Form/StateValuesCleanTest.php:    $values = Json::decode($this->drupalPostForm('form_test/form-state-values-clean', array(), t('Submit')));
core/modules/system/src/Tests/Form/StorageTest.php:    $this->drupalGet('form_test/form-storage');
core/modules/system/src/Tests/Form/StorageTest.php:    $this->drupalGet('form_test/form-storage', array('query' => array('cache' => 1)));
core/modules/system/src/Tests/Form/StorageTest.php:    $this->drupalPostForm('form_test/form-storage', array('title' => '', 'value' => 'value_is_set'), 'Continue submit');
core/modules/system/src/Tests/Form/StorageTest.php:    $this->drupalGet('form_test/form-storage', array('query' => array('cache' => 1)));

Hope this helps!

jackbravo’s picture

Status: Needs review » Needs work

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

jackbravo’s picture

Status: Needs work » Needs review

Sending last patch to the testbot.

jackbravo’s picture

Actually, the command that ran perfectly from drush was:

sudo -u www-data drush -l drupal8.axai.org test-run "Drupal\system\Tests\Form\FormTest"

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?

jackbravo’s picture

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

Status: Needs review » Needs work

The last submitted patch, 31: drupal-convert_to_form_builder_in_form_test-2078867-31.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 31: drupal-convert_to_form_builder_in_form_test-2078867-31.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
28.28 KB

Here's a reroll.

tim.plunkett’s picture

tim.plunkett’s picture

Title: Convert all form menu items to new form builder in form_test module » Convert _form_test_* functions to classes
Status: Needs review » Reviewed & tested by the community

Okay 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!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

  • webchick committed f905406 on 8.x
    Issue #2078867 by tim.plunkett, jackbravo, ianthomas_uk, InternetDevels...
jackbravo’s picture

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

Status: Fixed » Closed (fixed)

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