Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

#2091691: Convert test non-form page callbacks to routes and controllers has an insane amount of fixes, and also touches a lot of the same code. Might be worth starting from there or waiting a day or two

disasm’s picture

Status: Active » Needs review
FileSize
48.92 KB

first pass converting just form_test module (which has the bulk of the callbacks). Want to see if this is green before adding the others.

tim.plunkett’s picture

tim.plunkett’s picture

FileSize
8.22 KB
52.78 KB

We don't want or need FileInclusionTest anymore, its obsolete (the only way to fix the tests would be to manually module_load_include(), which defeats the purpose).

The rest of the changes were mostly fixes to called form IDs or bad route paths.

tim.plunkett’s picture

FileSize
3.41 KB
53.6 KB

Okay this should be green, carry on with the rest of the conversions.

disasm’s picture

Assigned: disasm » Unassigned
FileSize
3.39 KB
56.99 KB

language_elements_test module converted. Unassigning since timplunkett said he might have some time to work on this.

tim.plunkett’s picture

FileSize
75.19 KB
21.83 KB

Here's the rest. We'll see if it passes...

tim.plunkett’s picture

FileSize
78.12 KB
2.93 KB

Just another case of a test directly accessing a MENU_DEFAULT_LOCAL_TASK

dawehner’s picture

  1. +++ b/core/modules/file/tests/file_module_test/file_module_test.module
    @@ -10,27 +10,13 @@
    -  $items['file/test'] = array(
    -    'title' => 'Managed file test',
    -    'page callback' => 'drupal_get_form',
    -    'page arguments' => array('file_module_test_form'),
    -    'access arguments' => array('access content'),
    -  );
    

    Wait, this isn't a menu callback so why is this removed entirely?

  2. +++ b/core/modules/system/lib/Drupal/system/Tests/Batch/ProcessingTest.php
    @@ -46,34 +46,34 @@ function testBatchNoForm() {
    -    $this->drupalPostForm('batch-test/simple', $edit, 'Submit');
    +    $this->drupalPostForm('batch-test', $edit, 'Submit');
    ...
    -    $this->drupalPostForm('batch-test/simple', $edit, 'Submit');
    +    $this->drupalPostForm('batch-test', $edit, 'Submit');
    
    +++ b/core/modules/system/tests/modules/batch_test/batch_test.module
    @@ -13,9 +13,7 @@ function batch_test_menu() {
       $items['batch-test/simple'] = array(
    
    +++ b/core/modules/system/tests/modules/batch_test/batch_test.routing.yml
    @@ -31,6 +31,30 @@ batch_test.no_form:
    +batch_test.test_form:
    +  path: '/batch-test'
    

    I try to figure out why this change of the path was needed, as this adds additional hunks.

  3. +++ b/core/modules/system/tests/modules/form_test/form_test.module
    @@ -2101,8 +1846,10 @@ function form_test_programmatic_form_submit($form, &$form_state) {
    +function form_test_clicked_button($form, &$form_state, $first, $second, $third) {
    
    @@ -2119,7 +1866,7 @@ function form_test_clicked_button($form, &$form_state) {
    -  $args = array_slice(arg(), 2);
    +  $args = array($first, $second, $third);
    

    I like this tiny change!

tim.plunkett’s picture

1) It's a test, and the menu link is never used. I think when it was written it should have been type => MENU_CALLBACK, the test author just skipped it

2) batch-test/simple is the MENU_DEFAULT_LOCAL_TASK. It's like how node/{node}/view is no longer valid, or admin/structure/types/manage/{node_type}/edit.

3) Yes! Tiny wins.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you for the explanation!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

I only saw two things that stuck out to me in my review:

+++ /dev/null
@@ -1,47 +0,0 @@
-class FileInclusionTest extends WebTestBase {

Where did this test go?

Basically, we don't need it anymore because we no longer support loading forms from separate .inc files in favour of class loading.

+++ b/core/modules/system/tests/modules/form_test/form_test.module
@@ -2101,8 +1846,10 @@ function form_test_programmatic_form_submit($form, &$form_state) {
-function form_test_clicked_button($form, &$form_state) {
+function form_test_clicked_button($form, &$form_state, $first, $second, $third) {

I actually find this pretty bizarre, but it does indeed clean up the code below. Since it's a deprecated function anyway, not really worth bothering about atm.

Committed and pushed to 8.x. Thanks!

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