Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

Issue tags: +WSCCI-conversion

Adding tag

CB’s picture

Status: Active » Needs review
FileSize
6.89 KB

Patch attached.

larowlan’s picture

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

CB’s picture

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.

kim.pepper’s picture

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

kim.pepper’s picture

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.

kim.pepper’s picture

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

CB’s picture

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.

kim.pepper’s picture

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.

kim.pepper’s picture

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.

greyrhino’s picture

Status: Needs work » Needs review
FileSize
7.06 KB

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

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

alexpott’s picture

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

greyrhino’s picture

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

alexpott’s picture

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

greyrhino’s picture

Updated patch as per comment #16.

greyrhino’s picture

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.

greyrhino’s picture

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

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

larowlan’s picture

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

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

greyrhino’s picture

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.

CB’s picture

Its all good mate!

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

Between us we got it done. :)

alexpott’s picture

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

CB’s picture

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

greyrhino’s picture

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.

alexpott’s picture

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

greyrhino’s picture

Status: Needs work » Needs review
FileSize
501 bytes
12.17 KB

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

CB’s picture

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

Will wait for somebody else to RTBC.

ParisLiakos’s picture

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

greyrhino’s picture

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

greyrhino’s picture

Status: Needs work » Needs review
FileSize
1.82 KB
12.28 KB

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.

ParisLiakos’s picture

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

greyrhino’s picture

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

ParisLiakos’s picture

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

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
13.79 KB
dawehner’s picture

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

ParisLiakos’s picture

ah, thanks totally missed those:)

dawehner’s picture

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.

ParisLiakos’s picture

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.

ParisLiakos’s picture

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

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

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.