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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vijaycs85’s picture

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.

ayelet_Cr’s picture

Status: Closed (won't fix) » Active
mparker17’s picture

Assigned: Unassigned » mparker17

I'll help!

mparker17’s picture

Assigned: mparker17 » Unassigned
Status: Active » Needs work
FileSize
8.11 KB

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

mparker17’s picture

Status: Needs work » Needs review
FileSize
1.51 KB
8 KB

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

disasm’s picture

Status: Needs review » Reviewed & tested by the community

reviewed this, and looks good. Marking RTBC.

alexpott’s picture

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
mparker17’s picture

Status: Needs work » Needs review
FileSize
1.19 KB
8.1 KB

Try this...

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

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

mparker17’s picture

Status: Needs work » Needs review
Issue tags: +WSCCI-conversion
xjm’s picture

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.

disasm’s picture

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?

rdickert’s picture

Status: Needs work » Needs review
FileSize
5.68 KB

Reroll

Status: Needs review » Needs work

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

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
7.24 KB
vijaycs85’s picture

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

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
1.16 KB
14.26 KB

Valid fails. Missed few use statements.

dawehner’s picture

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.

vijaycs85’s picture

vijaycs85’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
14.37 KB
14.37 KB

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()

vijaycs85’s picture

FileSize
1.36 KB

Wrong diff file in #22. use this.

alexpott’s picture

Status: Needs review » Needs work

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

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
664 bytes
14.35 KB

more review update.

dawehner’s picture

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

vijaycs85’s picture

Here is the update...

Crell’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Crell’s picture

Status: Reviewed & tested by the community » Needs work

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

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
14.95 KB

Re-rolling...

dawehner’s picture

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.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
14.92 KB
1.2 KB

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

dawehner’s picture

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.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
14.31 KB
1.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).

YesCT’s picture

Issue tags: +Needs reroll
star-szr’s picture

Status: Needs review » Needs work
dawehner’s picture

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

vijaycs85’s picture

Crell’s picture

+++ 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. :-)

vijaycs85’s picture

let's get this in then...

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Bot can complain.

catch’s picture

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.