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
#43 1987606-diff-39-43.txt570 bytesvijaycs85
#43 1987606-system-ajax-test-43.patch14.11 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 63,171 pass(es).
[ View ]
#39 system-ajax_test_dialog-1987606-39.patch16.19 KBInternetDevels
PASSED: [[SimpleTest]]: [MySQL] 63,105 pass(es).
[ 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» mparker17

I'll help!

Assigned:mparker17» Unassigned
Status:Active» Needs work
StatusFileSize
new8.11 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

For some reason, my sidebars disappear after applying this patch and flushing caches. I don't know why.

Status:Needs work» Needs review
StatusFileSize
new1.51 KB
new8 KB
PASSED: [[SimpleTest]]: [MySQL] 58,157 pass(es).
[ View ]

Special thanks to @timplunkett for helping me figure out the sidebar bug.

Status:Needs review» Reviewed & tested by the community

reviewed this, and looks good. Marking RTBC.

+++ b/core/modules/system/tests/modules/ajax_test/ajax_test.moduleundefined
@@ -37,8 +37,7 @@ function ajax_test_menu() {
   $items['ajax-test/dialog'] = array(
     'title' => 'AJAX Dialog',
-    'page callback' => 'ajax_test_dialog',
-    'access callback' => TRUE,
+    'route_name' => 'ajax_test_dialog',
   );

I think we can remove the entire menu hook here now that #2032535: Resolve 'title' using the route and render array has landed.

Status:Reviewed & tested by the community» Needs work

Status:Needs work» Needs review
StatusFileSize
new1.19 KB
new8.1 KB
PASSED: [[SimpleTest]]: [MySQL] 58,452 pass(es).
[ View ]

Try this...

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

The last submitted patch, drupal8.system-module.1987606-9.patch, failed testing.

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

#9: drupal8.system-module.1987606-9.patch queued for re-testing.

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

+++ b/core/modules/system/tests/modules/ajax_test/lib/Drupal/ajax_test/AjaxTestController.php
@@ -21,4 +21,95 @@ public function dialogContents() {
+    $build['form'] = drupal_get_form('ajax_test_dialog_form');

Can we create a Form for this and return it instead?

Status:Needs work» Needs review
StatusFileSize
new5.68 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/tests/modules/ajax_test/ajax_test.module.
[ View ]

Reroll

Status:Needs review» Needs work

The last submitted patch, drupal8.system-module.1987606-14.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new7.24 KB
PASSED: [[SimpleTest]]: [MySQL] 59,402 pass(es).
[ View ]

StatusFileSize
new8.3 KB
new14.2 KB
FAILED: [[SimpleTest]]: [MySQL] 59,415 pass(es), 5 fail(s), and 0 exception(s).
[ View ]

@dawehner & @timplunkett advised to convert ajax_test_dialog_form as part of this issue, as it is inside this controller conversion.

1. Created a 'Form' namespace and moved existing formController there
2. Create new formController form DialogForm.
3. Moved ajax callback and its helpers (except ajax_test_dialog_contents() - which is covered in https://drupal.org/node/1987612#comment-7982379).

Status:Needs review» Needs work

The last submitted patch, 1987606-system-ajax-test-17.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.16 KB
new14.26 KB
PASSED: [[SimpleTest]]: [MySQL] 59,050 pass(es).
[ View ]

Valid fails. Missed few use statements.

Status:Needs review» Reviewed & tested by the community

+++ b/core/modules/system/tests/modules/ajax_test/lib/Drupal/ajax_test/Form/AjaxTestDialogForm.php
@@ -94,7 +96,7 @@ public function nonModal(&$form, &$form_state) {
-    $title = $this->t('AJAX Dialog contents');
+    $title = t('AJAX Dialog contents');
     $html = drupal_render($content);

The form test does not yet use the FormBase class, so let's keep this until we use the base class.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new14.37 KB
new14.37 KB
PASSED: [[SimpleTest]]: [MySQL] 59,793 pass(es).
[ View ]

updated the status on #2116145-1: Create a new formBase class (or reuse existing formBase) in test classes and issuing new patch with #$this->t()

StatusFileSize
new1.36 KB

Wrong diff file in #22. use this.

Status:Needs review» Needs work

Re #20 and #21 I don't understand why we just don't use FormBase here

Status:Needs work» Needs review
StatusFileSize
new664 bytes
new14.35 KB
PASSED: [[SimpleTest]]: [MySQL] 59,284 pass(es).
[ View ]

more review update.

+++ b/core/modules/system/tests/modules/ajax_test/lib/Drupal/ajax_test/Controller/AjaxTestController.php
@@ -43,10 +43,96 @@ public function renderError() {
+    $build['form'] = drupal_get_form('Drupal\ajax_test\Form\AjaxTestDialogForm');

drupal_get_form got replaced by the form builder service.

StatusFileSize
new955 bytes
new14.36 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1987606-system-ajax-test-27.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Here is the update...

Issue summary:View changes
Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 27: 1987606-system-ajax-test-27.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new14.95 KB
FAILED: [[SimpleTest]]: [MySQL] 58,017 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Re-rolling...

Status:Needs review» Needs work
  1. +++ b/core/modules/system/tests/modules/ajax_test/lib/Drupal/ajax_test/Form/AjaxTestDialogForm.php
    @@ -0,0 +1,113 @@
    +   * @return AjaxResponse

    Let's use a doc with the full namespace included.

  2. +++ b/core/modules/system/tests/modules/ajax_test/lib/Drupal/ajax_test/Form/AjaxTestDialogForm.php
    @@ -0,0 +1,113 @@
    +    $title = $this->t(String::checkPlain('AJAX Dialog contents'));

    So need for this extra check_plain

  3. +++ b/core/modules/system/tests/modules/ajax_test/lib/Drupal/ajax_test/Form/AjaxTestDialogForm.php
    @@ -0,0 +1,113 @@
    +}
    \ No newline at end of file
    diff --git a/core/modules/system/tests/modules/ajax_test/lib/Drupal/ajax_test/AjaxTestForm.php b/core/modules/system/tests/modules/ajax_test/lib/Drupal/ajax_test/Form/AjaxTestForm.php

    No new endline.

Status:Needs work» Needs review
StatusFileSize
new14.92 KB
FAILED: [[SimpleTest]]: [MySQL] 57,944 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new1.2 KB

Thanks for the review @dawehner. Here is the updates for #32

Status:Needs review» Reviewed & tested by the community

Thank you

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 31: 1987606-system-ajax-test-31.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new14.31 KB
PASSED: [[SimpleTest]]: [MySQL] 58,056 pass(es).
[ View ]
new1.05 KB

Test error: called to undefined function ajax_test_dialog_contents()

ajax_test_dialog_contents() is in use on three different location. So for now leaving it in .module. (locally test case passing).

Issue tags:+Needs reroll

instructions for rerolling: https://drupal.org/contributor-tasks/reroll

Status:Needs review» Needs work

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new16.19 KB
PASSED: [[SimpleTest]]: [MySQL] 63,105 pass(es).
[ View ]

I wonder whether we have an issue to fix the problem with ajax_test_dialog_contents().

+++ b/core/modules/system/tests/modules/ajax_test/ajax_test.routing.yml
@@ -3,6 +3,7 @@ ajax_test.dialog_contents:
   defaults:
     _title: 'AJAX Dialog contents'
     _content: '\Drupal\ajax_test\Controller\AjaxTestController::dialogContents'
+    _title: 'AJAX Dialog contents'

This seems redundant.

Otherwise I didn't see any big red flags, at least given that it's a test module. :-)

Issue tags:+LONDON_2014_JANUARY, +SprintWeekend2014
StatusFileSize
new14.11 KB
PASSED: [[SimpleTest]]: [MySQL] 63,171 pass(es).
[ View ]
new570 bytes

let's get this in then...

Status:Needs review» Reviewed & tested by the community

Bot can complain.

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

Status:Fixed» Closed (fixed)

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