Files: 
CommentFileSizeAuthor
#25 interdiff-21-25.txt1.99 KBvalthebald
#25 2066557-25.patch9.63 KBvalthebald
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,239 pass(es).
[ View ]
#22 2066557-21.patch9.48 KBvalthebald
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,093 pass(es).
[ View ]
#15 convert_system_module_s-2066557-15.patch13.42 KBmparker17
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,698 pass(es), 3 fail(s), and 3 exception(s).
[ View ]

Comments

mparker17’s picture

StatusFileSize
new1.49 KB
new10.84 KB
FAILED: [[SimpleTest]]: [MySQL] 58,085 pass(es), 6 fail(s), and 6 exception(s).
[ View ]
new11.06 KB
FAILED: [[SimpleTest]]: [MySQL] 58,176 pass(es), 6 fail(s), and 6 exception(s).
[ View ]

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

Status:Active» Needs review
mparker17’s picture

Assigned:mparker17» Unassigned

Status:Needs review» Needs work

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

mparker17’s picture

Status:Needs work» Needs review
Issue tags:+WSCCI-conversion
StatusFileSize
new2.11 KB
new11.08 KB
PASSED: [[SimpleTest]]: [MySQL] 58,098 pass(es).
[ View ]
new10.87 KB
PASSED: [[SimpleTest]]: [MySQL] 57,937 pass(es).
[ View ]

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

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

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

Assigned:Unassigned» sushyl
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.

sushyl’s picture

Assigned:sushyl» Unassigned

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

chakrapani’s picture

Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new12.71 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch system-module.2066557-11-a.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

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

Issue tags:+Needs reroll
mparker17’s picture

StatusFileSize
new13.42 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,698 pass(es), 3 fail(s), and 3 exception(s).
[ View ]

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

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

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

Issue tags:+Needs PSR-4
Mile23’s picture

Issue tags:+Needs reroll
valthebald’s picture

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

StatusFileSize
new9.48 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,093 pass(es).
[ View ]
valthebald’s picture

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

Mile23’s picture

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

Status:Needs work» Needs review
StatusFileSize
new9.63 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,239 pass(es).
[ View ]
new1.99 KB

Here we go

Mile23’s picture

Status:Needs review» Reviewed & tested by the community

Super awesome. :-)

alexpott’s picture

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.