I'm not sure if this will become obsolete if/when #2015149: Replace dblog recent log entries with a view gets done but here it is anyway.

When sorting the dblog by the User, an error is thrown:

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'u.name' in 'order clause': SELECT w.wid AS wid, w.uid AS uid, w.severity AS severity, w.type AS type, w.timestamp AS timestamp, w.message AS message, w.variables AS variables, w.link AS link FROM {watchdog} w ORDER BY u.name ASC LIMIT 50 OFFSET 0; Array ( ) in Drupal\dblog\Controller\DbLogController->overview() (line 169 of /var/www/core/modules/dblog/src/Controller/DbLogController.php).

I believe this happens because the ordering is done in the query by u.name a column which is not joined into the dblog query. Patch coming.

CommentFileSizeAuthor
#12 dblog-fix-2344103-12.patch2.19 KBupchuk
#12 dblog-test-2344103-12.patch1.38 KBupchuk
#8 dblog-fix-2344103-8.patch3.24 KBupchuk
#7 dblog-test-2344103-7.patch2.43 KBupchuk
#2 dblog-2344103-3.patch834 bytesupchuk

Comments

upchuk’s picture

upchuk commented
Issue summary: View changes
upchuk’s picture

upchuk commented
StatusFileSize
new dblog-2344103-3.patch834 bytes

Here's a patch that joins the user_data_field table in order to properly sort by the username.

upchuk’s picture

upchuk commented
Status: Active » Needs review
xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +Needs tests

Nice find!

The next step will be to add an automated test for this bug:
https://drupal.org/contributor-tasks/write-tests

xjm’s picture

xjm
she/her
Primary language English
commented

Providing an EXPLAIN for the new query could be helpful information too.

upchuk’s picture

upchuk commented

I believe this is what you meant. Running the new query with EXPLAIN:

+----+-------------+-------+------+---------------+---------+---------+---------------+------+-------------+
| id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra |
+----+-------------+-------+------+---------------+---------+---------+---------------+------+-------------+
| 1 | SIMPLE | w | ALL | NULL | NULL | NULL | NULL | 261 | |
| 1 | SIMPLE | ufd | ref | PRIMARY | PRIMARY | 4 | VAGRANT.w.uid | 1 | Using index |
+----+-------------+-------+------+---------------+---------+---------+---------------+------+-------------+

upchuk’s picture

upchuk commented
StatusFileSize
new dblog-test-2344103-7.patch2.43 KB

I wrote a test for the bug by creating a separate test class. I believe though that it may be incorporated into the existing DbLogTest class. I'm not sure how the procedure is though...

Attaching the test patch (that should fail) and the test + fix patch (that should, hopefully, pass).

D

upchuk’s picture

upchuk commented
StatusFileSize
new dblog-fix-2344103-8.patch3.24 KB

And here is the patch with the test + fix.

The last submitted patch, 7: dblog-test-2344103-7.patch, failed testing.

upchuk’s picture

upchuk commented
Issue tags: -Needs tests
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work 
  1. +++ b/core/modules/dblog/src/Tests/DBLogUserSortTest.php
@@ -0,0 +1,86 @@
+   * Tests the Database Logging functionality.

    Please make this description similar to DbLogTest::testfilter():

    Tests the database log sort functionality at admin/reports/dblog. 

  2. +++ b/core/modules/dblog/src/Tests/DBLogUserSortTest.php
@@ -0,0 +1,86 @@
+   * First, it logs in an admin user and then tests the database logging reports
+   * table by sorting in both directions and by all sortable headers.

    We generally don't write these kinds of comments for test methods. We only provide a single-line description. 

  3. +++ b/core/modules/dblog/src/Tests/DBLogUserSortTest.php
@@ -0,0 +1,86 @@
+  function testDbLogSort() {

    I think you can move this test into DbLogTest — your setUp() method does almost exactly the same, so no need for a seperate web test. 

  4. +++ b/core/modules/dblog/src/Tests/DBLogUserSortTest.php
@@ -0,0 +1,86 @@
+
...
+
...
+
...
+

    Extraneous newlines at the beginning and end of function bodies. 

  5. +++ b/core/modules/dblog/src/Tests/DBLogUserSortTest.php
@@ -0,0 +1,86 @@
+    $orders = array('Date', 'Type', 'User');
+    $sorts = array('asc', 'desc');

    Hrm, shouldn't these variable names be the other way around? :) 

  6. +++ b/core/modules/dblog/src/Tests/DBLogUserSortTest.php
@@ -0,0 +1,86 @@
+        $this->verifySort(200, $sort, $order);
...
+   *   (optional) HTTP response code. Defaults to 200.
...
+  public function verifySort($response = 200, $sort = 'asc', $order = 'Date') {

    You abstracted the expected response code, but it's always 200. Let's just remove this parameter and hardcode it to 200. This is a testing helper function; no need to abstract here unless absolutely necessary :) 

  7. +++ b/core/modules/dblog/src/Tests/DBLogUserSortTest.php
@@ -0,0 +1,86 @@
+   *
...
+   *

    … also no newlines between @param doxygen statements.

upchuk’s picture

upchuk commented
Status: Needs work » Needs review
StatusFileSize
new dblog-test-2344103-12.patch1.38 KB
new dblog-fix-2344103-12.patch2.19 KB

Hey Wim, thanks for reviewing.

I addressed most of the issues (apart from those that are obsolete with the move of the test to the existing DBLog one).

As for the variable names, they are given by the query params needed in the sorting (sort=asc&order=User) :)

The last submitted patch, 12: dblog-test-2344103-12.patch, failed testing.

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
commented
Status: Needs review » Reviewed & tested by the community

Tested and verified that it fixes the issue.

Only thing I could think of was the variable names as pointed out by @Wim, but it makes sense to call them the same as the query parameters I guess, so that's probably another issue ;)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 5ad6a5e and pushed to 8.0.x. Thanks!

  • alexpott committed 5ad6a5e on 8.0.x 
    Issue #2344103 by Upchuk: Fixed DBLog sort by user is broken.

Status: Fixed » Closed (fixed)

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