Combines the following issues:

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.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it's updating the code to the new API/code
Unfrozen changes Unfrozen because it's a change in automated tests
Disruption Not disruptive because it only moves procedural code to class methods
CommentFileSizeAuthor
#25 interdiff-21-25.txt1.99 KBvalthebald
#25 2066557-25.patch9.63 KBvalthebald
#22 2066557-21.patch9.48 KBvalthebald
#15 convert_system_module_s-2066557-15.patch13.42 KBmparker17
#11 system-module.2066557-11-a.patch12.71 KBchakrapani
#5 drupal8.system-module.2066557-5.patch10.87 KBmparker17
#5 drupal8.system-module.2066557-5-a.patch11.08 KBmparker17
#5 interdiff.txt2.11 KBmparker17
#1 drupal8.system-module.2066557-1-a.patch11.06 KBmparker17
#1 drupal8.system-module.2066557-1.patch10.84 KBmparker17
#1 inter-interdiff.txt1.49 KBmparker17
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-4
CreditAttribution: mparker17 commented
FileSize
inter-interdiff.txt1.49 KB
drupal8.system-module.2066557-1.patch10.84 KB
drupal8.system-module.2066557-1-a.patch11.06 KB

Okay. After some adventures with fatal errors, I've created two patches.

Turns out the original test code made the assumption that it would only run inside it's test harness. Since my method for testing was "enable the hidden module with Drush and hit the menu paths", the result of $query->execute() would be null and the call to ->fetchCol() would subsequently fatal error.

The -1 patch is a straight conversion. The -1-a improves the test so it doesn't fatal error if the query returns NULL. If the -1-a patch passes all tests, that one should be preferred. If it fails, then just go with the -1.

inter-interdiff.txt shows the differences between two patches.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-4
CreditAttribution: mparker17 commented
Status: Active » Needs review
mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-4
CreditAttribution: mparker17 commented
Assigned: mparker17 » Unassigned

Status: Needs review » Needs work

The last submitted patch, drupal8.system-module.2066557-1.patch, failed testing.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-4
CreditAttribution: mparker17 commented
Status: Needs work » Needs review
Issue tags: +WSCCI-conversion
FileSize
interdiff.txt2.11 KB
drupal8.system-module.2066557-5-a.patch11.08 KB
drupal8.system-module.2066557-5.patch10.87 KB

Try these.

The -5 versus -5-a is the same as above.

Since I made the same change to both -5 and -5-a, the interdiff between -1 versus -5 and -1-a versus -5-a is the same, I've included only one interdiff.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
+++ b/core/modules/system/tests/modules/database_test/lib/Drupal/database_test/Controller/DatabaseTestJsonController.php
@@ -0,0 +1,149 @@
+class DatabaseTestJsonController {
...
+    $table_name = db_query_temporary('SELECT age FROM {test}', array());
...
+      'row_count' => db_select($table_name)->countQuery()->execute()->fetchField(),
...
+    $query = db_select('test', 't');
...
+    $query = db_select('test_task', 't');
...
+    $query = db_select('test_task', 't');
...
+    $query = db_select('test_task', 't');
+    $query

All this db_select calls could be replaced directly by an injected db connection, so for example extending ControllerBase would just work.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Needs work 
+++ b/core/modules/system/tests/modules/database_test/lib/Drupal/database_test/Controller/DatabaseTestJsonController.php
@@ -0,0 +1,149 @@
+class DatabaseTestJsonController {
...
+    $table_name = db_query_temporary('SELECT age FROM {test}', array());
...
+      'row_count' => db_select($table_name)->countQuery()->execute()->fetchField(),
...
+    $query = db_select('test', 't');
...
+    $query = db_select('test_task', 't');
...
+    $query = db_select('test_task', 't');
...
+    $query = db_select('test_task', 't');
+    $query

All this db_select calls could be replaced directly by an injected db connection, so for example extending ControllerBase would just work.

sushyl’s picture

sushyl
Location Pune
CreditAttribution: sushyl commented
Assigned: Unassigned » sushyl
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

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.

sushyl’s picture

sushyl
Location Pune
CreditAttribution: sushyl commented
Assigned: sushyl » Unassigned

Unassigned, Couldn't spare enough time to look into it.

chakrapani’s picture

chakrapani CreditAttribution: chakrapani commented
Issue summary: View changes
Status: Needs work » Needs review
FileSize
system-module.2066557-11-a.patch12.71 KB

There has been some changes to the core and some of the changes from the earlier patches have been already committed(eg: removal of hook_menu).
Re-rolling the patch against latest head based on #5-a.patch.
comments from #6/#7 are not considered yet in the current patch.
The patch passed the tests when run locally.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

11: system-module.2066557-11-a.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 11: system-module.2066557-11-a.patch, failed testing.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue tags: +Needs reroll
mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-4
CreditAttribution: mparker17 commented
FileSize
convert_system_module_s-2066557-15.patch13.42 KB

Straight re-roll of #11.

I got a merge conflict...

CONFLICT (rename/delete): core/modules/system/tests/modules/database_test/src/Controller/DatabaseTestController.php deleted in 2066557-11 and renamed in HEAD. Version HEAD of core/modules/system/tests/modules/database_test/src/Controller/DatabaseTestController.php left in tree.

... so I resolved it with git rm core/modules/system/tests/modules/database_test/src/Controller/DatabaseTestController.php

Note the patch name has changed since #11 to whatever Dreditor recommended today.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-4
CreditAttribution: mparker17 commented
Status: Needs work » Needs review
Issue tags: -Needs reroll

Status: Needs review » Needs work

The last submitted patch, 15: convert_system_module_s-2066557-15.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented 
+++ b/core/modules/system/tests/modules/database_test/database_test.routing.yml
--- /dev/null
+++ b/core/modules/system/tests/modules/database_test/lib/Drupal/database_test/Controller/DatabaseTestJsonController.php

The reason it did conflict is that the other class moved to src/, and this has not yet been updated for that.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-4
CreditAttribution: mparker17 commented
Issue tags: +Needs PSR-4
Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Issue tags: +Needs reroll
valthebald’s picture

valthebald
Location Sofia
CreditAttribution: valthebald commented
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs PSR-4, -Needs reroll +SprintWeekend2015
valthebald’s picture

valthebald
Location Sofia
CreditAttribution: valthebald commented
FileSize
2066557-21.patch9.48 KB
valthebald’s picture

valthebald
Location Sofia
CreditAttribution: valthebald commented

Last patch just moves functions to Drupal\database_test\Controller\DatabaseTestController

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Status: Needs review » Needs work

Thanks, @valthebald!

Just some coding standards stuff:

  1. +++ b/core/modules/system/tests/modules/database_test/src/Controller/DatabaseTestController.php
@@ -6,6 +6,7 @@
 namespace Drupal\database_test\Controller;
+use Symfony\Component\HttpFoundation\JsonResponse;

    Needs a blank line between namespace and use. 

  2. +++ b/core/modules/system/tests/modules/database_test/src/Controller/DatabaseTestController.php
@@ -13,38 +14,136 @@
+   * @return JsonResponse
    */
   public function dbQueryTemporary() {

    All of these should be fully qualified like: @return \Symfony\Component\HttpFoundation\JsonResponse

valthebald’s picture

valthebald
Location Sofia
CreditAttribution: valthebald commented
Status: Needs work » Needs review
FileSize
2066557-25.patch9.63 KB
interdiff-21-25.txt1.99 KB

Here we go

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Status: Needs review » Reviewed & tested by the community

Super awesome. :-)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed ed7f9d9 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

diff --git a/core/modules/system/tests/modules/database_test/src/Controller/DatabaseTestController.php b/core/modules/system/tests/modules/database_test/src/Controller/DatabaseTestController.php
index 9805fb6..83ed8ad 100644
--- a/core/modules/system/tests/modules/database_test/src/Controller/DatabaseTestController.php
+++ b/core/modules/system/tests/modules/database_test/src/Controller/DatabaseTestController.php
@@ -21,7 +21,7 @@ class DatabaseTestController {
    * separate menu callback request; After this request is done, the temporary
    * table should automatically dropped.
    *
-   * @return Symfony\Component\HttpFoundation\JsonResponse
+   * @return \Symfony\Component\HttpFoundation\JsonResponse
    */
   public function dbQueryTemporary() {
     $table_name = db_query_temporary('SELECT age FROM {test}', array());
@@ -37,7 +37,7 @@ public function dbQueryTemporary() {
    * This function does care about the page GET parameter, as set by the
    * simpletest HTTP call.
    *
-   * @return Symfony\Component\HttpFoundation\JsonResponse
+   * @return \Symfony\Component\HttpFoundation\JsonResponse
    */
   public function pagerQueryEven($limit) {
     $query = db_select('test', 't');
@@ -63,7 +63,7 @@ public function pagerQueryEven($limit) {
    * This function does care about the page GET parameter, as set by the
    * simpletest HTTP call.
    *
-   * @return Symfony\Component\HttpFoundation\JsonResponse
+   * @return \Symfony\Component\HttpFoundation\JsonResponse
    */
   public function pagerQueryOdd($limit) {
     $query = db_select('test_task', 't');
@@ -89,7 +89,7 @@ public function pagerQueryOdd($limit) {
    * This function does care about the page GET parameter, as set by the
    * simpletest HTTP call.
    *
-   * @return Symfony\Component\HttpFoundation\JsonResponse
+   * @return \Symfony\Component\HttpFoundation\JsonResponse
    */
   public function testTablesort() {
     $header = array(
@@ -121,7 +121,7 @@ public function testTablesort() {
    * This function does care about the page GET parameter, as set by the
    * simpletest HTTP call.
    *
-   * @return Symfony\Component\HttpFoundation\JsonResponse
+   * @return \Symfony\Component\HttpFoundation\JsonResponse
    */
   public function testTablesortFirst() {
     $header = array(

Need to have a leading slash - fixed on commit.

  • alexpott committed ed7f9d9 on 8.0.x 
    Issue #2066557 by mparker17, valthebald, chakrapani: Convert system....

Status: Fixed » Closed (fixed)

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