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

Assigned:Unassigned» nano_monkey

StatusFileSize
new13.9 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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

Status:Active» Needs review

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new13.46 KB
PASSED: [[SimpleTest]]: [MySQL] 58,085 pass(es).
[ View ]

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

StatusFileSize
new451 bytes

Diff file for the 2 patches submitted today

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.

Assigned:nano_monkey» Unassigned
Status:Needs work» Needs review
StatusFileSize
new2.29 KB
new13.44 KB
PASSED: [[SimpleTest]]: [MySQL] 58,585 pass(es).
[ View ]

addressing my comments in #7.

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.

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?

Status:Needs work» Needs review
StatusFileSize
new10.2 KB
new22.16 KB
FAILED: [[SimpleTest]]: [MySQL] 59,097 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

Issue summary:View changes

updating form item list.

Issue summary:View changes
StatusFileSize
new21.54 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8-form-test-2078867-13.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Patch re-rolled.

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.

Status:Needs work» Needs review
StatusFileSize
new21.56 KB
FAILED: [[SimpleTest]]: [MySQL] 59,821 pass(es), 10 fail(s), and 189 exception(s).
[ View ]

Uploading another re-rolled patch

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new23.22 KB
PASSED: [[SimpleTest]]: [MySQL] 63,688 pass(es).
[ View ]

StatusFileSize
new23.22 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,383 pass(es).
[ View ]

simple reroll

Issue summary:View changes
StatusFileSize
new6.32 KB
new26.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,423 pass(es).
[ View ]

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

  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