Part of #1971384: [META] Convert page callbacks to controllers

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

Files: 
CommentFileSizeAuthor
#29 wscci-1987624-29.patch9.58 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,748 pass(es).
[ View ]
#29 interdiff.txt1.02 KBdawehner
#24 drupal8.batch_test.1987624-24.patch9.48 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 59,132 pass(es).
[ View ]
#22 drupal8.batch_test.1987624-22.patch152.35 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 58,979 pass(es).
[ View ]
#22 interdiff.txt3.1 KBdisasm
#16 drupal8.system-module.1987624-16.patch8.68 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 59,113 pass(es).
[ View ]
#16 interdiff.txt3.19 KBdisasm
#12 batch_test-1987624-12.patch8.41 KBpiyuesh23
FAILED: [[SimpleTest]]: [MySQL] 58,364 pass(es), 48 fail(s), and 11 exception(s).
[ View ]
#12 interdiff.txt3.95 KBpiyuesh23
#8 batch_test-1987624-8.patch8.75 KBpiyuesh23
FAILED: [[SimpleTest]]: [MySQL] 58,375 pass(es), 48 fail(s), and 11 exception(s).
[ View ]
#8 interdiff.txt2.45 KBpiyuesh23
#6 batch_test-1987624-6.patch8.64 KBrobeano
PASSED: [[SimpleTest]]: [MySQL] 58,012 pass(es).
[ View ]
#6 interdiff-1987624-4-6.txt4.77 KBrobeano
#4 batch_test-1987624-4.patch8.69 KBrobeano
FAILED: [[SimpleTest]]: [MySQL] 58,092 pass(es), 6 fail(s), and 0 exception(s).
[ View ]

Comments

Status:Active» Closed (won't fix)

Need to rewrite the whole module to make test sync with current test implementation. For more details, please refer: #1988802: [META] Rewrite test modules in system to provide better unit testing.

Status:Closed (won't fix)» Active

Assigned:Unassigned» robeano

Working on this one.

Title:Convert batch_test_nested_drupal_form_submit() to a new style controllerConvert batch_test page callbacks to a new style controller
Status:Active» Needs review
StatusFileSize
new8.69 KB
FAILED: [[SimpleTest]]: [MySQL] 58,092 pass(es), 6 fail(s), and 0 exception(s).
[ View ]

Adding a patch which converts the page callbacks in batch_test module to use the new BatchTestController.

Changes made:

  • replaced hook_menu items with routes in batch_test.routing.yml
  • moved corresponding page callback functions into BatchTestController as methods

This patch covers page callbacks. Forms have not been converted.

This issue now covers the needs of other page callback issues which will be marked as duplicates:

#1987622: Convert batch_test_large_percentage() to a new style controller
#1987626: Convert batch_test_no_form() to a new style controller
#1987628: Convert batch_test_programmatic() to a new style controller
#1987632: Convert batch_test_theme_batch() to a new style controller
#1987630: Convert batch_test_redirect_page() to a new style controller

Status:Needs review» Needs work

The last submitted patch, batch_test-1987624-4.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.77 KB
new8.64 KB
PASSED: [[SimpleTest]]: [MySQL] 58,012 pass(es).
[ View ]

A couple of the controllers needed to support path arguments. The routes have been updated. Also, the controller methods were renamed to simplify things a bit. Ready for a review.

Status:Needs review» Needs work

This is almost RTBC! A couple comments below:

  1. +++ b/core/modules/system/tests/modules/batch_test/batch_test.module
    @@ -41,53 +41,6 @@ function batch_test_menu() {
    -  $items['batch-test/programmatic'] = array(
    -    'title' => 'Programmatic',
    -    'page callback' => 'batch_test_programmatic',
    -    'access callback' => TRUE,
    -    'type' => MENU_LOCAL_TASK,
    -    'weight' => 3,
    -  );
    -  // No form: fire a batch simply by accessing a page.
    -  $items['batch-test/no-form'] = array(
    -    'title' => 'Simple page',
    -    'page callback' => 'batch_test_no_form',
    -    'access callback' => TRUE,
    -    'type' => MENU_LOCAL_TASK,
    -    'weight' => 4,
    -  );
    -  // No form: fire a batch; return > 100% complete
    -  $items['batch-test/large-percentage'] = array(
    -    'title' => 'Simple page with batch over 100% complete',
    -    'page callback' => 'batch_test_large_percentage',
    -    'access callback' => TRUE,
    -    'type' => MENU_LOCAL_TASK,
    -    'weight' => 5,
    -  );
    -  // Tests programmatic form submission within a batch operation.
    -  $items['batch-test/nested-programmatic'] = array(
    -    'title' => 'Nested programmatic',
    -    'page callback' => 'batch_test_nested_drupal_form_submit',
    -    'access callback' => TRUE,
    -    'type' => MENU_LOCAL_TASK,
    -    'weight' => 6,
    -  );
    -  // Landing page to test redirects.
    -  $items['batch-test/redirect'] = array(
    -    'title' => 'Redirect',
    -    'page callback' => 'batch_test_redirect_page',
    -    'access callback' => TRUE,
    -    'type' => MENU_LOCAL_TASK,
    -    'weight' => 7,

    Because these aren't MENU_CALLBACK, the menu items should remain, remove page callback and access callback, and add 'route_name' => 'name_of_route_in_yaml_file'.

  2. +++ b/core/modules/system/tests/modules/batch_test/lib/Drupal/batch_test/Controller/BatchTestController.php
    @@ -0,0 +1,101 @@
    +  }
    +}

    need a blank line between last two closing braces. Also missing a newline at end of file.

Assigned:robeano» piyuesh23
StatusFileSize
new2.45 KB
new8.75 KB
FAILED: [[SimpleTest]]: [MySQL] 58,375 pass(es), 48 fail(s), and 11 exception(s).
[ View ]

Updated the Patch with the information mentioned in the previous comment. Uploading the new patch and interdiff file.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, batch_test-1987624-8.patch, failed testing.

Thanks for picking this up piyuesh23. Note that we aren't doing any major cleanups for test modules (creating services, moving helper methods into controllers, etc... We just need to get tests working again. That being said, there are some minor cleanups that shouldn't take much more than 5 minutes to do. I've listed them below.

  1. +++ b/core/modules/system/tests/modules/batch_test/batch_test.routing.yml
    @@ -0,0 +1,41 @@
    +  pattern: 'batch-test/redirect'
    ...
    +  pattern: 'batch-test/large-percentage'
    ...
    +  pattern: 'batch-test/nested-programmatic/{value}'
    ...
    +  pattern: 'batch-test/no-form'
    ...
    +  pattern: 'batch-test/programmatic/{value}'
    ...
    +  pattern: 'admin/batch-test/test-theme'

    patterns should begin with a leading '/'

  2. +++ b/core/modules/system/tests/modules/batch_test/lib/Drupal/batch_test/Controller/BatchTestController.php
    @@ -0,0 +1,102 @@
    +use Drupal\Core\Controller\ControllerInterface;
    ...
    +class BatchTestController implements ControllerInterface {

    Since we're not injecting anything, lets just make this a plain class and not extend/implement anything.

  3. +++ b/core/modules/system/tests/modules/batch_test/lib/Drupal/batch_test/Controller/BatchTestController.php
    @@ -0,0 +1,102 @@
    +use Symfony\Component\DependencyInjection\ContainerInterface;
    ...
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function create(ContainerInterface $container) {
    +    return new static();
    +  }
    +

    not needed. No services are injected in this controller.

  4. +++ b/core/modules/system/tests/modules/batch_test/lib/Drupal/batch_test/Controller/BatchTestController.php
    @@ -0,0 +1,102 @@
    +   * Menu callback: Redirects successfuly.
    ...
    +   * Menu callback: Submits a form within a batch programmatically.
    ...
    +   * Menu callback: Fires a batch process without a form submission.
    ...
    +   * Menu callback: Submits the 'Chained' form programmatically.
    ...
    +   * Menu callback: Runs a batch for testing theme used on the progress page.

    We don't call controller methods menu callbacks. start with the description, no need for the prefix.

StatusFileSize
new3.95 KB
new8.41 KB
FAILED: [[SimpleTest]]: [MySQL] 58,364 pass(es), 48 fail(s), and 11 exception(s).
[ View ]

@disasm Thanks for reviewing and helping out with this issue. Uploading the fixed patch and interdiff files.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, batch_test-1987624-12.patch, failed testing.

+++ b/core/modules/system/tests/modules/batch_test/batch_test.module
@@ -45,47 +45,41 @@ function batch_test_menu() {
     'title' => 'Programmatic',
...
     'title' => 'Simple page',
...
     'title' => 'Simple page with batch over 100% complete',
...
     'title' => 'Nested programmatic',
...
     'title' => 'Redirect',

Lets use _title property in the routing.yml file.

Status:Needs work» Needs review
StatusFileSize
new3.19 KB
new8.68 KB
PASSED: [[SimpleTest]]: [MySQL] 59,113 pass(es).
[ View ]

Spent some time debugging these test failures, and these are passing locally now. Here's the patch. We needed a default set for value. Also, fixed the titles above, so this should be green and ready to be reviewed. Thanks piyuesh for all your work on this!

Status:Needs review» Needs work
Issue tags:-WSCCI-conversion

The last submitted patch, drupal8.system-module.1987624-16.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+WSCCI-conversion

  1. +++ w/core/modules/system/tests/modules/batch_test/batch_test.module
    @@ -44,48 +44,37 @@ function batch_test_menu() {
       // This item lives under 'admin' so that the page uses the admin theme.
       $items['admin/batch-test/test-theme'] = array(
    -    'page callback' => 'batch_test_theme_batch',
    -    'access callback' => TRUE,
    +    'route_name' => 'batch_test_test_theme',
         'type' => MENU_CALLBACK,
       );

    That one is really just a menu callback, so can be dropped.

  2. +++ w/core/modules/system/tests/modules/batch_test/lib/Drupal/batch_test/Controller/BatchTestController.php
    @@ -0,0 +1,93 @@
    +  /**
    +   * Redirects successfuly.
    +   */
    +  public function testRedirect() {
    ...
    +  /**
    +   * Fires a batch process without a form submission.
    +   */
    ...
    +  /**
    +   * Submits a form within a batch programmatically.
    +   */
    +  public function testNestedDrupalFormSubmit($value = 1) {
    ...
    +  /**
    +   * Fires a batch process without a form submission.
    +   */
    ...
    +  /**
    +   * Submits the 'Chained' form programmatically.
    +   *
    +   * Programmatic form: the page submits the 'Chained' form through
    +   * drupal_form_submit().
    +   */
    ...
    +  /**
    +   * Runs a batch for testing theme used on the progress page.
    +   */

    I guess we should put some @return and some @param on there, just short ones.

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

.

Status:Needs work» Needs review
StatusFileSize
new3.1 KB
new152.35 KB
PASSED: [[SimpleTest]]: [MySQL] 58,979 pass(es).
[ View ]

addresses comments in #19. $value param has no description, because digging into the functions that it's passed to explains about as much as these methods do.

Status:Needs review» Needs work

This diff seems too big.

Status:Needs work» Needs review
StatusFileSize
new9.48 KB
PASSED: [[SimpleTest]]: [MySQL] 59,132 pass(es).
[ View ]

try again. no interdiff, same as above.

Status:Needs review» Needs work
Issue tags:-WSCCI-conversion

The last submitted patch, drupal8.batch_test.1987624-24.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+WSCCI-conversion

#24: drupal8.batch_test.1987624-24.patch queued for re-testing.

I ran a few simple tests as well as enabled content translation for pages. Are there any specific other manual tests that need done? I experienced no errors in my manual testing.

#27 was meant for a different issue.

StatusFileSize
new1.02 KB
new9.58 KB
PASSED: [[SimpleTest]]: [MySQL] 58,748 pass(es).
[ View ]

Just fixing some tiny docs

Status:Needs review» Reviewed & tested by the community

Patch is green and we already have tests for this code so i think it is good to go.

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

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