Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marcingy’s picture

Assigned: Unassigned » marcingy
marcingy’s picture

Status: Active » Needs review
FileSize
7.12 KB
ParisLiakos’s picture

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?

sidharthap’s picture

Status: Needs work » Needs review
FileSize
7.04 KB

Status: Needs review » Needs work

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

CB’s picture

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.

ParisLiakos’s picture

Status: Needs work » Postponed
ParisLiakos’s picture

Status: Postponed » Needs work
vijaycs85’s picture

Status: Needs work » Needs review
FileSize
6.58 KB

Re-rolling...

Status: Needs review » Needs work

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

stella’s picture

Status: Needs work » Needs review
FileSize
2.91 KB
7.41 KB

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.

stella’s picture

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

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

stella’s picture

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);
stella’s picture

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

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Cool

alexpott’s picture

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
Anonymous’s picture

Status: Needs work » Needs review
FileSize
4.21 KB

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

ParisLiakos’s picture

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

Anonymous’s picture

Status: Needs work » Needs review
FileSize
7.58 KB

Reroll the patch and removed whitespaces also.Please needs review

ParisLiakos’s picture

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

Anonymous’s picture

Assigned: Unassigned »
FileSize
7.56 KB

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

ParisLiakos’s picture

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.

ParisLiakos’s picture

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

Status: Needs review » Reviewed & tested by the community
YesCT’s picture

Issue tags: +RTBC July 1

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

YesCT’s picture

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

Anonymous’s picture

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

alexpott’s picture

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

kmoll’s picture

Assigned: » kmoll
kmoll’s picture

re rolled against head

kmoll’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

kmoll’s picture

Assigned: kmoll » Unassigned
dawehner’s picture

Status: Needs work » Needs review
FileSize
10.6 KB

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

vijaycs85’s picture

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

dawehner’s picture

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

Crell’s picture

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.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

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