Support from Acquia helps fund testing for Drupal Acquia logo

Comments

shameemkm’s picture

Issue summary: View changes
tkuldeep17’s picture

I have converted batch_test_chained_form, batch_test_multistep_form,
batch_test_simple_form
into Controller. For this I changed batch_test.routing .yml file. used _form parameter instead of _content in router.

tkuldeep17’s picture

I am attaching patch which contains controller of these menu items..

dawehner’s picture

+++ b/core/modules/system/tests/modules/batch_test/lib/Drupal/batch_test/Form/BatchTestMultiStepForm.php
@@ -0,0 +1,67 @@
+    // This will only be effective on the last step.
+    $form_state['redirect'] = 'batch-test/redirect';

+++ b/core/modules/system/tests/modules/batch_test/lib/Drupal/batch_test/Form/BatchTestSimpleForm.php
@@ -0,0 +1,59 @@
+    $form_state['redirect'] = 'batch-test/redirect';

We could use 'redirect_route' here instead

tim.plunkett’s picture

Thanks for the patch!
Next patch, can you mark the issue as "needs review"?

  1. +++ b/core/modules/system/tests/modules/batch_test/batch_test.module
    @@ -70,133 +70,10 @@ function batch_test_menu() {
    -  $form_state['redirect_route']['route_name'] = 'batch_test.redirect';
    ...
    -  $form_state['redirect_route']['route_name'] = 'batch_test.redirect';
    
    +++ b/core/modules/system/tests/modules/batch_test/lib/Drupal/batch_test/Form/BatchTestMultiStepForm.php
    @@ -0,0 +1,67 @@
    +    $form_state['redirect'] = 'batch-test/redirect';
    
    +++ b/core/modules/system/tests/modules/batch_test/lib/Drupal/batch_test/Form/BatchTestSimpleForm.php
    @@ -0,0 +1,59 @@
    +    $form_state['redirect'] = 'batch-test/redirect';
    

    These should be redirect_route still

  2. +++ b/core/modules/system/tests/modules/batch_test/lib/Drupal/batch_test/Form/BatchTestChainedForm.php
    @@ -0,0 +1,56 @@
    +  public function submitForm(array &$form, array &$form_state) {
    ...
    +  public function buildForm(array $form, array &$form_state) {
    
    +++ b/core/modules/system/tests/modules/batch_test/lib/Drupal/batch_test/Form/BatchTestMultiStepForm.php
    @@ -0,0 +1,67 @@
    +  public function submitForm(array &$form, array &$form_state) {
    ...
    +  public function buildForm(array $form, array &$form_state) {
    
    +++ b/core/modules/system/tests/modules/batch_test/lib/Drupal/batch_test/Form/BatchTestSimpleForm.php
    @@ -0,0 +1,59 @@
    +  public function submitForm(array &$form, array &$form_state) {
    ...
    +    $form['batch'] = array(
    

    Our convention has been to put buildForm before submitForm

  3. +++ b/core/modules/system/tests/modules/batch_test/lib/Drupal/batch_test/Form/BatchTestChainedForm.php
    @@ -0,0 +1,56 @@
    +      #default_value' => 1,
    

    This is missing a '

  4. +++ b/core/modules/system/tests/modules/batch_test/lib/Drupal/batch_test/Form/BatchTestChainedForm.php
    @@ -0,0 +1,56 @@
    +    $form['#submit'] = array(
    +    'batch_test_chained_form_submit_1',
    +    'batch_test_chained_form_submit_2',
    +    'batch_test_chained_form_submit_3',
    +    'batch_test_chained_form_submit_4',
    +  );
    +
    +  return $form;
    

    These are indented wrong, but also the functions should be converted to methods on this class.

tkuldeep17’s picture

Thanks dawehner, tim.plunkett for reviewing patch..

  • I have done form redirect from redirect_route.
  • Order of submitForm and buildForm corrected.

  • @tim.plunkett When I tried to add these submit handlers into Class, drupal was not recognizing methods. So could you please help me how to convert these function into methods.

    tkuldeep17’s picture

    Status: Active » Needs review
    FileSize
    11.95 KB

    Submitting updated patch..

    Status: Needs review » Needs work

    The last submitted patch, 7: convert_batch_test_callbacks_to_controllers-2132477-6.patch, failed testing.

    tkuldeep17’s picture

    Sorry, First I had to test it locally, but now I have tested it locally, all test cases are passing. So again I am submitting updated patch..

    tkuldeep17’s picture

    Status: Needs work » Needs review
    FileSize
    13.86 KB

    Submitting new patch. This patch is now locally tested, all test cases are passing..

    tkuldeep17’s picture

    Updated patch.. Now I have converted

    +++ b/core/modules/system/tests/modules/batch_test/lib/Drupal/batch_test/Form/BatchTestChainedForm.php
    
    +    $form['#submit'] = array(
    +    'batch_test_chained_form_submit_1',
    +    'batch_test_chained_form_submit_2',
    +    'batch_test_chained_form_submit_3',
    +    'batch_test_chained_form_submit_4',
    +  );

    into

    +++ b/core/modules/system/tests/modules/batch_test/lib/Drupal/batch_test/Form/BatchTestChainedForm.php
    
    $form['#submit'] = array(
    +      'Drupal\batch_test\Form\BatchTestChainedForm::batchTestChainedFormSubmit1',
    +      'Drupal\batch_test\Form\BatchTestChainedForm::batchTestChainedFormSubmit2',
    +      'Drupal\batch_test\Form\BatchTestChainedForm::batchTestChainedFormSubmit3',
    +      'Drupal\batch_test\Form\BatchTestChainedForm::batchTestChainedFormSubmit4',
    +  );
    tkuldeep17’s picture

    For creating Form object implemented FormInterface instead of

    extending

    FormBase,
    As here we only need very simple form, not required dependencyInjection. it was just creating Form with additional functionality which was not required.

    And also converted batch_test_mock_form into Form Object Drupal\batch_test\Form\BatchTestMockForm.
    as it was not listed in issue, but I thought this must also converted :-)

    Status: Needs review » Needs work

    The last submitted patch, 12: convert_batch_test_callbacks_to_controllers-2132477-12.patch, failed testing.

    tkuldeep17’s picture

    Sorry, I was wrong, We have to create form using extending Drupal\Core\Form\FormBase . and also when I converting batch_test_mock_form into Form Object Drupal\batch_test\Form\BatchTestMockForm. Test cases are failing, why it is happening not able to find our reason.. If any one, then please share here..

    So please consider my previous patch https://drupal.org/comment/8222485#comment-8222485

    RoSk0’s picture

    Title: convert batch_test callbacks to controllers » Convert batch_test forms to controllers
    Parent issue: » #1971384: [META] Convert page callbacks to controllers
    tim.plunkett’s picture

    Status: Needs work » Needs review
    FileSize
    16.98 KB

    We should use FormBase.

    tim.plunkett’s picture

    Issue tags: +FormInterface
    jibran’s picture

    Status: Needs review » Reviewed & tested by the community

    Unable to find anything objectionable so RTBC.

    Status: Reviewed & tested by the community » Needs work

    The last submitted patch, 16: batch-2132477-16.patch, failed testing.

    tim.plunkett’s picture

    Status: Needs work » Reviewed & tested by the community

    EntityCrudHookTest random fail.

    alexpott’s picture

    Status: Reviewed & tested by the community » Fixed

    Committed 37e61b1 and pushed to 8.x. Thanks!

    • alexpott committed 37e61b1 on 8.x
      Issue #2132477 by tkuldeep17, tim.plunkett | shameemkm: Convert...

    Status: Fixed » Closed (fixed)

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