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
#37 drupal-1987686-37.patch10.6 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,706 pass(es).
[ View ]
#33 drupal-dblog-event-1987686-33.patch5.23 KBkmoll
FAILED: [[SimpleTest]]: [MySQL] 57,079 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#23 1987686-dblog_event-controller-23.patch7.56 KBnick_daffodil
PASSED: [[SimpleTest]]: [MySQL] 56,383 pass(es).
[ View ]
#21 1987686-dblog_event-controller-21.patch7.58 KBnick_daffodil
PASSED: [[SimpleTest]]: [MySQL] 56,259 pass(es).
[ View ]
#19 1987686-dblog_event-controller-19.patch4.21 KBnick_daffodil
PASSED: [[SimpleTest]]: [MySQL] 58,082 pass(es).
[ View ]
#16 1987686-dblog_event-controller-16.patch7.18 KBstella
PASSED: [[SimpleTest]]: [MySQL] 58,541 pass(es).
[ View ]
#11 1987686-dblog_event-controller-11.patch7.41 KBstella
PASSED: [[SimpleTest]]: [MySQL] 58,033 pass(es).
[ View ]
#11 1987686-9-11-interdiff.txt2.91 KBstella
#9 1987686-dblog_event-controller-9.patch6.58 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 58,447 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#4 dblog-event-rounter-4.patch7.04 KBsidharthap
FAILED: [[SimpleTest]]: [MySQL] 55,760 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#2 dblog-event-rounter.patch7.12 KBmarcingy
PASSED: [[SimpleTest]]: [MySQL] 55,450 pass(es).
[ View ]

Comments

Assigned:Unassigned» marcingy

Status:Active» Needs review
StatusFileSize
new7.12 KB
PASSED: [[SimpleTest]]: [MySQL] 55,450 pass(es).
[ View ]

Status:Needs review» Needs work

thanks, this looks mostly good, some notices:

+++ b/core/modules/dblog/lib/Drupal/dblog/Controller/DBLogController.phpundefined
@@ -0,0 +1,112 @@
+use Drupal\Core\Extension\ModuleHandlerInterface;

i think this is not needed right?

+++ b/core/modules/dblog/lib/Drupal/dblog/Controller/DBLogController.phpundefined
@@ -0,0 +1,112 @@
+class DBLogController implements ControllerInterface {

i believe the class name should be DbLogController see http://drupal.org/node/608152#naming point 3

+++ b/core/modules/dblog/lib/Drupal/dblog/Controller/DBLogController.phpundefined
@@ -0,0 +1,112 @@
+   *   format expected by drupal_render(); otherwise, an empty string.

the empty string part is no longer true, right?

Status:Needs work» Needs review
StatusFileSize
new7.04 KB
FAILED: [[SimpleTest]]: [MySQL] 55,760 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, dblog-event-rounter-4.patch, failed testing.

Please note also that this needs to work with the changes in #1982954: Convert dblog_top() to a controller and #1977254: Convert dblog_overview() to a Controller.

It might be worth waiting until those two have been committed before working any more on this, otherwise we'll end up with a big dependency mess.

Status:Needs work» Postponed

Status:Postponed» Needs work

Status:Needs work» Needs review
StatusFileSize
new6.58 KB
FAILED: [[SimpleTest]]: [MySQL] 58,447 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Re-rolling...

Status:Needs review» Needs work

The last submitted patch, 1987686-dblog_event-controller-9.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.91 KB
new7.41 KB
PASSED: [[SimpleTest]]: [MySQL] 58,033 pass(es).
[ View ]

Patch changes a few things:

  • Test to check for "DBLog event node was displayed" was failing because it was loading event with id 1 which didn't exist, now loads a valid event id.
  • It was also failing because the page title wasn't set to "Details" so restored a simple hook_menu() item with the title and route_name.
  • Class name case in dblog.routing.yml (was DBLogController rather than DbLogController)

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

The last submitted patch, 1987686-dblog_event-controller-11.patch, failed testing.

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

+++ b/core/modules/dblog/dblog.routing.ymlundefined
@@ -4,3 +4,10 @@ dblog_overview:
+  pattern: 'admin/reports/event/{eventId}'
+++ b/core/modules/dblog/lib/Drupal/dblog/Controller/DbLogController.phpundefined
@@ -191,6 +191,75 @@ public function overview() {
+   * @param int $eventId
...
+  function eventDetails($eventId) {

Just for codestyle reasons let's use $event_id

+++ b/core/modules/dblog/lib/Drupal/dblog/Controller/DbLogController.phpundefined
index e619fff..29276fc 100644
--- a/core/modules/dblog/lib/Drupal/dblog/Tests/DbLogTest.php
+++ b/core/modules/dblog/lib/Drupal/dblog/Tests/DbLogTest.phpundefined
@@ -187,7 +188,8 @@ private function verifyReports($response = 200) {
+    $wid = db_query('SELECT MIN(wid) FROM {watchdog}')->fetchField();

I am a bit confused about the test changes. Doesn't the test ensure that there are logs created for login in a user etc. so the test below should not use the new generated log entry.

The original test was checking for an event with event_id = 1, however that event didn't exist whenever I tested it. That particular test is to ensure that the event details page is working I think, but this code change probably isn't needed as the logging in of the user would generate an entry.

+++ b/core/modules/dblog/lib/Drupal/dblog/Tests/DbLogTest.php
@@ -64,6 +64,7 @@ function setUp() {
   function testDbLog() {
     // Login the admin user.
     $this->drupalLogin($this->big_user);
+    $this->generateLogEntries(1);
     $row_limit = 100;
     $this->verifyRowLimit($row_limit);

StatusFileSize
new7.18 KB
PASSED: [[SimpleTest]]: [MySQL] 58,541 pass(es).
[ View ]

Patch reload to change eventId to event_id, and to remove the unneeded call to generateLogEntries().

Status:Needs review» Reviewed & tested by the community

Cool

Assigned:marcingy» Unassigned
Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/dblog/lib/Drupal/dblog/Controller/DbLogController.phpundefined
@@ -191,6 +191,75 @@ public function overview() {
+          check_plain($dblog->hostname),

use String::checkPlain()

Needs reroll due to #2008990: Replace theme() with drupal_render() in dblog module - we need to make sure we replace all the calls to theme with drupal_render or a render array...

curl https://drupal.org/files/1987686-dblog_event-controller-16.patch | git apply --check
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  7354  100  7354    0     0   3451      0  0:00:02  0:00:02 --:--:--  4199
error: patch failed: core/modules/dblog/dblog.admin.inc:71
error: core/modules/dblog/dblog.admin.inc: patch does not apply

Status:Needs work» Needs review
StatusFileSize
new4.21 KB
PASSED: [[SimpleTest]]: [MySQL] 58,082 pass(es).
[ View ]

Reroll the #16 patch and added the #18 suggestions.Please needs review

Status:Needs review» Needs work

+++ b/core/modules/dblog/dblog.routing.ymlundefined
@@ -4,3 +4,9 @@
+    _permission: 'access site reports'
\ No newline at end of file

needs a newline here

+++ b/core/modules/dblog/lib/Drupal/dblog/Controller/DbLogController.phpundefined
@@ -193,7 +194,76 @@
   }
+   ¶
+  /**
...
+    return $build;
+  }
+  ¶

extra spaces should be removed

+++ b/core/modules/dblog/lib/Drupal/dblog/Controller/DbLogController.phpundefined
@@ -193,7 +194,76 @@
+  function eventDetails($event_id) {

needs public visibility

Status:Needs work» Needs review
StatusFileSize
new7.58 KB
PASSED: [[SimpleTest]]: [MySQL] 56,259 pass(es).
[ View ]

Reroll the patch and removed whitespaces also.Please needs review

looks ready to go. Just one small thing:

+++ b/core/modules/dblog/lib/Drupal/dblog/Controller/DbLogController.phpundefined
@@ -195,6 +196,75 @@
+   * @see dblog_menu()

we remove those now, since controllers dont have much to do with menus

Assigned:Unassigned» nick_daffodil
StatusFileSize
new7.56 KB
PASSED: [[SimpleTest]]: [MySQL] 56,383 pass(es).
[ View ]

Sorry I missed this small thing.Added the #22 suggestions.Please needs review

Status:Needs review» Reviewed & tested by the community

thank you!

Status:Reviewed & tested by the community» Needs work
Issue tags:-WSCCI-conversion

The last submitted patch, 1987686-dblog_event-controller-23.patch, failed testing.

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

Status:Needs review» Reviewed & tested by the community

Issue tags:+RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

@naveenvalecha
It's not required, but it is helpful to make interdiffs.

interdiff?
For instructions on creating an interdiff, see https://drupal.org/documentation/git/interdiff | Microbranching workflow: http://xjm.drupalgardens.com/blog/interdiffs-how-make-them-and-why-they-...

Keep that in mind for future. :)

@YesCT
Thanks for your suggestions regarding the interdiff. I know regarding the interdiff but the #19 patch has the space errors.So that's why I not created the interdiff.

Status:Reviewed & tested by the community» Needs work

Needs a reroll

git applyc https://drupal.org/files/1987686-dblog_event-controller-23.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  7742  100  7742    0     0   7912      0 --:--:-- --:--:-- --:--:--  9689
error: patch failed: core/modules/dblog/dblog.admin.inc:68
error: core/modules/dblog/dblog.admin.inc: patch does not apply
+++ b/core/modules/dblog/lib/Drupal/dblog/Tests/DbLogTest.phpundefined
@@ -187,7 +187,8 @@
-    $this->drupalGet('admin/reports/event/1');
+    $wid = db_query('SELECT MIN(wid) FROM {watchdog}')->fetchField();
+    $this->drupalGet('admin/reports/event/' . $wid);

Like @dawehner I don't get why this change is necessary

Assigned:nick_daffodil» kmoll

StatusFileSize
new5.23 KB
FAILED: [[SimpleTest]]: [MySQL] 57,079 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

re rolled against head

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, drupal-dblog-event-1987686-33.patch, failed testing.

Assigned:kmoll» Unassigned

Status:Needs work» Needs review
StatusFileSize
new10.6 KB
PASSED: [[SimpleTest]]: [MySQL] 57,706 pass(es).
[ View ]

This fixes the failures, removes the old code and fixes some small points.

This patch applies fine and looks good to me. +1 for RTBC.

#37: drupal-1987686-37.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

There's actually quite a bit more refactoring that can/should be done here, but this at least gets the page out of the old router so let's take care of that first, as that's the release-blocking part. :-) Thanks all.

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

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