Convert this page callback to a new-style Controller, using the instructions on http://drupal.org/node/1800686

Files: 
CommentFileSizeAuthor
#41 Drupal8-Convert-dblog_overview-to-a-controller-1977254-41.patch13.96 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 55,627 pass(es).
[ View ]
#41 interdiff.txt1.41 KBParisLiakos
#39 Drupal8-Convert-dblog_overview-to-a-controller-1977254-39.patch13.86 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 55,696 pass(es).
[ View ]
#39 interdiff.txt1.19 KBParisLiakos
#37 Drupal8-Convert-dblog_overview-to-a-controller-1977254-37.patch13.79 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 55,605 pass(es).
[ View ]
#35 Drupal8-Convert-dblog_overview-to-a-controller-1977254-35.patch57.56 KBgreyrhino
PASSED: [[SimpleTest]]: [MySQL] 55,965 pass(es).
[ View ]
#35 interdiff.txt50.08 KBgreyrhino
#33 Drupal8-Convert-dblog_overview-to-a-controller-1977254-33.patch12.28 KBgreyrhino
PASSED: [[SimpleTest]]: [MySQL] 55,761 pass(es).
[ View ]
#33 interdiff.txt1.82 KBgreyrhino
#29 Drupal8-Convert-dblog_overview-to-a-controller-1977254-29.patch12.17 KBgreyrhino
PASSED: [[SimpleTest]]: [MySQL] 55,523 pass(es).
[ View ]
#29 interdiff.txt501 bytesgreyrhino
#18 Drupal8-Convert-dblog_overview-to-a-controller-1977254-18.patch12.23 KBgreyrhino
PASSED: [[SimpleTest]]: [MySQL] 55,487 pass(es).
[ View ]
#18 interdiff.txt1.47 KBgreyrhino
#17 Drupal8-Convert-dblog_overview-to-a-controller-1977254-17.patch12.31 KBgreyrhino
PASSED: [[SimpleTest]]: [MySQL] 55,554 pass(es).
[ View ]
#15 Drupal8-Convert-dblog_overview-to-a-controller-1977254-15.patch11.47 KBgreyrhino
PASSED: [[SimpleTest]]: [MySQL] 55,401 pass(es).
[ View ]
#13 Drupal8-Convert-dblog_overview-to-a-controller-1977254-13.patch7.06 KBgreyrhino
PASSED: [[SimpleTest]]: [MySQL] 55,741 pass(es).
[ View ]
#9 Drupal8-Convert-dblog_overview-to-a-controller-1977254-9.patch7 KBcbiggins
FAILED: [[SimpleTest]]: [MySQL] 55,395 pass(es), 4 fail(s), and 43 exception(s).
[ View ]
#9 interdiff.txt9.44 KBcbiggins
#4 Drupal8-Convert-dblog_overview-to-a-controller-1977254-4.patch14.3 KBcbiggins
FAILED: [[SimpleTest]]: [MySQL] 55,196 pass(es), 4 fail(s), and 86 exception(s).
[ View ]
#2 Drupal8-Convert-dblog_overview-to-a-controller-1977254-2.patch6.89 KBcbiggins
PASSED: [[SimpleTest]]: [MySQL] 55,279 pass(es).
[ View ]

Comments

Issue tags:+WSCCI-conversion

Adding tag

Status:Active» Needs review
StatusFileSize
new6.89 KB
PASSED: [[SimpleTest]]: [MySQL] 55,279 pass(es).
[ View ]

Patch attached.

Some first observations, mostly just nit-picks

+++ b/core/modules/dblog/dblog.moduleundefined
@@ -42,8 +42,7 @@ function dblog_menu() {
     'file' => 'dblog.admin.inc',

Safe to remove

+++ b/core/modules/dblog/dblog.routing.ymlundefined
@@ -0,0 +1,6 @@
\ No newline at end of file

Needs a blank line at end of the file

+++ b/core/modules/dblog/lib/Drupal/dblog/Controller/DBLogController.phpundefined
@@ -0,0 +1,197 @@
+   * @var Drupal\Core\Database\Connection
...
+   * @var Drupal\Core\Extension\ModuleHandler

needs leading \

+++ b/core/modules/dblog/lib/Drupal/dblog/Controller/DBLogController.phpundefined
@@ -0,0 +1,197 @@
+   * TODO: object type?

Can go?

+++ b/core/modules/dblog/lib/Drupal/dblog/Controller/DBLogController.phpundefined
@@ -0,0 +1,197 @@
+  public function __construct($database, $moduleHandler) {

These need to be type hinted 'Connection' and 'ModuleHandler'. Also $moduleHandler should be $module_handler in the arguments.

+++ b/core/modules/dblog/lib/Drupal/dblog/Controller/DBLogController.phpundefined
@@ -0,0 +1,197 @@
+   * Page callback: Displays a listing of database log messages.

We seem to be removing the 'Page callback:'

+++ b/core/modules/dblog/lib/Drupal/dblog/Controller/DBLogController.phpundefined
@@ -0,0 +1,197 @@
+   * @see dblog_menu()

We've been removing the references to hook_menu() too

StatusFileSize
new14.3 KB
FAILED: [[SimpleTest]]: [MySQL] 55,196 pass(es), 4 fail(s), and 86 exception(s).
[ View ]

Updated with larowlans changes.

Status:Needs review» Needs work

The last submitted patch, Drupal8-Convert-dblog_overview-to-a-controller-1977254-4.patch, failed testing.

Looking good!

+++ b/Drupal8-Convert-dblog_overview-to-a-controller-1977254-2.patchundefined
@@ -0,0 +1,230 @@
++use Drupal\Core\ControllerInterface;

You need to add "use Drupal\Core\Database\Connection"

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

use Drupal\Core\Database\Connection

Also, please post an interdiff.txt when making changes between patches, so we can track what has changed. See http://drupal.org/documentation/git/interdiff

diff --git a/Drupal8-Convert-dblog_overview-to-a-controller-1977254-2.patch b/Drupal8-Convert-dblog_overview-to-a-controller-1977254-2.patch

You accidentally added a patch within your patch.

+++ b/core/modules/dblog/lib/Drupal/dblog/Controller/DBLogController.phpundefined
@@ -0,0 +1,194 @@
+  public function __construct(Connection $database, ModuleHandler $module_handler) {

Should program to interfaces instead of class implementations. This should be ModuleHandlerInterface instead.

+++ b/core/modules/dblog/lib/Drupal/dblog/Controller/DBLogController.phpundefined
@@ -0,0 +1,194 @@
+    $query->fields('w', array('wid', 'uid', 'severity', 'type',
+                        'timestamp', 'message', 'variables', 'link'))

Indentation of array over 80 chars should follow standards. See http://drupal.org/coding-standards#array

+++ b/core/modules/dblog/lib/Drupal/dblog/Controller/DBLogController.phpundefined
@@ -0,0 +1,194 @@
+  }

Need a spacing newline before class closing bracket

StatusFileSize
new9.44 KB
new7 KB
FAILED: [[SimpleTest]]: [MySQL] 55,395 pass(es), 4 fail(s), and 43 exception(s).
[ View ]

Thanks kimb0. Not sure why my sniffer didn't comment on those style issues. I had some issues with larowlan's sniffer mentioning issues that mine didn't. How embarrassment.

You accidentally added a patch within your patch.

Heh. git add . failed me it seems. :)

Patch and interdiff attached.

Almost there.

+++ b/core/modules/dblog/lib/Drupal/dblog/Controller/DBLogController.phpundefined
@@ -0,0 +1,204 @@
+  public function __construct(Connection $database, ModuleHandlerInterface $module_handler) {

Still need to 'use' ModuleHandlerInterface.

Status:Needs work» Needs review

Summoning the bot.

Status:Needs review» Needs work

The last submitted patch, Drupal8-Convert-dblog_overview-to-a-controller-1977254-9.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new7.06 KB
PASSED: [[SimpleTest]]: [MySQL] 55,741 pass(es).
[ View ]

Hi, just made the amends as per comment #10.

Passes tests locally so hopefully the bot see's it the same way.

We can remove dblog_overview() function and you'll need to update the comment in Drupal\dblog\Tests\DBLogTest that references this function.

StatusFileSize
new11.47 KB
PASSED: [[SimpleTest]]: [MySQL] 55,401 pass(es).
[ View ]

Thanks - have updated the patch as per comment #14.

+++ b/core/modules/dblog/lib/Drupal/dblog/Controller/DBLogController.phpundefined
@@ -0,0 +1,205 @@
+ * Contains \Drupal\dblog\Controller\DBLogController.
+++ b/core/modules/dblog/lib/Drupal/dblog/Tests/DBLogTest.phpundefined
@@ -575,7 +575,7 @@ protected function getTypeCount(array $types) {
-    // Reversed array from dblog_overview().
+    // Array of dblog logging levels.

Should be // Reverse array from \Drupal\dblog\Controller\DBLogController::overview.

But actually what might be nice here is declare this array as the return value of a public static on the controller and then call it from here...

StatusFileSize
new12.31 KB
PASSED: [[SimpleTest]]: [MySQL] 55,554 pass(es).
[ View ]

Updated patch as per comment #16.

StatusFileSize
new1.47 KB
new12.23 KB
PASSED: [[SimpleTest]]: [MySQL] 55,487 pass(es).
[ View ]

Re-updated as per comment #16 to remove unnecessary comment and rename the getLogLevelsClassMap method.

Also including an interdiff this time.

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

The last submitted patch, Drupal8-Convert-dblog_overview-to-a-controller-1977254-18.patch, failed testing.

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

Hey greyrhino, you might want to take notice of the 'assigned' field in future eh. Taking over on somebody elses patch is a bit poor form, especially as it wasn't a long time between review and update (ie, issue was far from dead.)

Assigned:cbiggins» Unassigned
Status:Needs review» Reviewed & tested by the community

I think this is ready, great job @cbiggins and @greyrhino

Hi cbiggins - really sorry about this!

I was on a Drupal sprint weekend and as it was my first time it was suggested that I finish off this one as it was nearly there and would give me a good understanding of the Symfony routing changes.

Fully understand why you are annoyed about it and next time I will take note of the assigned field to avoid this happening on my next patch.

Its all good mate!

I spoke to Alex today and he told me the situation.

Between us we got it done. :)

Status:Reviewed & tested by the community» Needs work

Patch needs a re-roll :)

And whilst you are at it...

+++ b/core/modules/dblog/lib/Drupal/dblog/Tests/DBLogTest.phpundefined
@@ -9,6 +9,7 @@
use Drupal\simpletest\WebTestBase;
use SimpleXMLElement;
+use Drupal\dblog\Controller\DBLogController;

Should be alphabetical...

I was just about to do this, but it might be handy and helpful for greyrhino to re-roll?

Happy to make this change - but to confirm is the re-roll just rebuilding the patch file (with an interdiff) to include the alphabetical listing change shown above?

Just wanted to check as you say 'Whilst you are at it' as though there might be more to a re-roll than just that.

@greyrhino the patch no longer applies to head... so a re-roll is to sort that out... see https://drupal.org/patch/reroll

Status:Needs work» Needs review
StatusFileSize
new501 bytes
new12.17 KB
PASSED: [[SimpleTest]]: [MySQL] 55,523 pass(es).
[ View ]

Hi Alex/CBiggins,

There seemed to be nothing wrong with the old patch file - it patched cleanly against a clean checkout of 8.x and all dblog tests passed - so I've made the amend for alphabetical import listing and rebuilt the patch with an interdiff.

Is this all that you would usually do for a re-roll - if the tests come back with nothing?

Cheers

James

Looks good to me. Test passed, so it applies cleanly.

Will wait for somebody else to RTBC.

Status:Needs review» Needs work

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

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,215 @@
+   * Constructs a DBLogController object.
+   */
+  public function __construct(Connection $database, ModuleHandlerInterface $module_handler) {

Needs documentation for the parameters

+++ b/core/modules/dblog/lib/Drupal/dblog/Controller/DBLogController.phpundefined
@@ -0,0 +1,215 @@
+      WATCHDOG_DEBUG     => 'dblog-debug',
+      WATCHDOG_INFO      => 'dblog-info',
+      WATCHDOG_NOTICE    => 'dblog-notice',
+      WATCHDOG_WARNING   => 'dblog-warning',
+      WATCHDOG_ERROR     => 'dblog-error',
+      WATCHDOG_CRITICAL  => 'dblog-critical',
+      WATCHDOG_ALERT     => 'dblog-alert',
+      WATCHDOG_EMERGENCY => 'dblog-emergency',

lets fix the weird spacing here. just one space needed

+++ b/core/modules/dblog/lib/Drupal/dblog/Controller/DBLogController.phpundefined
@@ -0,0 +1,215 @@
+    $query = $this->database->select('watchdog', 'w')
+            ->extend('Drupal\Core\Database\Query\PagerSelectExtender')
+            ->extend('Drupal\Core\Database\Query\TableSortExtender');

also weird identation.
should be two spaces

Sorry, just seen this - will pick it up hopefully tonight.

Status:Needs work» Needs review
StatusFileSize
new1.82 KB
new12.28 KB
PASSED: [[SimpleTest]]: [MySQL] 55,761 pass(es).
[ View ]

Attached is a new patch file with interdiff.

The only thing that I haven't changed from comment #31 is the class name - the reason for this is that there are existing classes in 8.x that are named this way - so with DBLog in their name rather than DbLog:

core/modules/rest/lib/Drupal/rest/Plugin/rest/resource/DBLogResource.php
core/modules/rest/lib/Drupal/rest/Tests/DBLogTest.php

If it is a must fix then I am happy to make the changes to this patch.

core/modules/rest/lib/Drupal/rest/Plugin/rest/resource/DBLogResource.php
core/modules/rest/lib/Drupal/rest/Tests/DBLogTest.php

those names are wrong then, we should fix them some time, but at least let the new classes have the correct name in the dblog module.
more people will check there than the rest module, if they are wondering about how to name something:)

StatusFileSize
new50.08 KB
new57.56 KB
PASSED: [[SimpleTest]]: [MySQL] 55,965 pass(es).
[ View ]

I've updated the patch to include the name change for controller class and test class.

cool, thanks! but
please include -C -M in your git diff arguments to keep the patch size down.

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new13.79 KB
PASSED: [[SimpleTest]]: [MySQL] 55,605 pass(es).
[ View ]

+++ b/core/modules/dblog/lib/Drupal/dblog/Controller/DbLogController.phpundefined
@@ -0,0 +1,220 @@
+/**

Missing empty line ...

+++ b/core/modules/dblog/lib/Drupal/dblog/Controller/DbLogController.phpundefined
@@ -0,0 +1,220 @@
+  public function overview() {

Needs @return

+++ b/core/modules/dblog/lib/Drupal/dblog/Tests/DbLogTest.phpundefined
@@ -2,18 +2,19 @@
+ * Definition of Drupal\dblog\Tests\DbLogTest.

Contains

StatusFileSize
new1.19 KB
new13.86 KB
PASSED: [[SimpleTest]]: [MySQL] 55,696 pass(es).
[ View ]

ah, thanks totally missed those:)

Status:Reviewed & tested by the community» Needs review

I would like to give it a manual try but i'm to tired to trust myself now.

+++ b/core/modules/dblog/lib/Drupal/dblog/Controller/DbLogController.phpundefined
@@ -0,0 +1,224 @@
+          $log_text = truncate_utf8(filter_xss($message, array()), 56, TRUE, TRUE);
...
+          filter_xss($dblog->link),

Dismissed some prodecurals which could call the classes directly.

StatusFileSize
new1.41 KB
new13.96 KB
PASSED: [[SimpleTest]]: [MySQL] 55,627 pass(es).
[ View ]

oh you are right, how did i miss that..no alternative for filter_xss though
fixed some other doc stuff as well

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

The last submitted patch, Drupal8-Convert-dblog_overview-to-a-controller-1977254-41.patch, failed testing.

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

Status:Needs review» Reviewed & tested by the community

Played a bit with the dblog listing and both filtering and clearing still worked.

Status:Reviewed & tested by the community» Fixed

Committed fdfad94 and pushed to 8.x. Thanks!

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