Convert dblog_build_filter_query() to DBTNG

The patch looks perfect, but I fear we don't have any tests for that yet. Why not adding some?

There was quite a bit of discussion on IRC that led to the creation of this patch. I'd like to see a walkthrough about the changes that were made given here so that this mentoring isn't lost.

A few minor things..

+    ->setHeader($header);
     $result = $query->execute();
-  }

the second line is wrongly indented.

-      'where' => "w.type = ':s'",
+      'where' => 'w.type',

Maybe we should rename where to field, to reflect the value change..

 /**
  * Build query for dblog administration filters based on session.
  */
-function dblog_build_filter_query() {
+function dblog_build_filter_query($query) {

You need to add documentation for $query, have a look at other functions with parameters for examples. And it might make sense to replace "Build query.." with "Extend query object.. " or something like that.

Another possibility to resolve this.. #450666: Filter DB Extender

re #2, I wrote a patch to test this and it's working - #276591: TestingParty08: dblog.admin.inc. The test I wrote fails because of this bug (which I take to mean the test is doing its job). I expect that if both this patch and the other are applied then things should be happy but I haven't tried that yet.

So to summarize, before patching, filtering on type for dblog doesn't work - it shows 0 results. Some of this patch was apparently committed already, so I re-rolled it and merged in #276591: TestingParty08: dblog.admin.inc so there will be tests on the filter now.

  * Build query for dblog administration filters based on session.
  */
-function dblog_build_filter_query() {
+function dblog_build_filter_query($query) {

This still needs a @param line in the api docs and the interface should be added to the function definition, like this:

function dblog_build_filter_query(SelectQueryInterface $query)

This is to enforce that it can only be called with a proper $query object.

+    $this->assertResponse(200);

AFAIK we don't explicitly test this usually. If an error happens on the site, it should be displayed as a exception automatically anyway.

Other than that, the patch looks good.

Includes the revisions noted in #9.

I just rolled this again and got the same patch. I call shenanigans. Re-test.

More shenanigans...

Sorry, we're testing out some new testing slaves this evening.

- Let's not abbreviate $severity to $sev. Let's write $severity.

- We don't usually glue words together; filterinclude should be filter-include. Ditto for filterexclude.

Makes revisions requested in #15

Doesn't pass coder review.

Now adds revisions from #15 + code style fixes.

-      'hostname' => substr($log_entry['ip'], 0, 128),
+      'hostname' => drupal_substr($log_entry['ip'], 0, 128),

Do not fix code style stuff unless you touch the code anyway. See http://webchick.net/please-stop-eating-baby-kittens. Note that neither ipv4 nor ipv6 will ever contain non-ascii characters, so that does not make sense. It might for others, but not in this patch....

Fixes #20. The failures in #19 don't make sense to me. Investigating.

#21 seems to resolve #19 on my local environment.

Coding style needs more work still. For example: // be sure the ... should start with capital B and end with a point. Also need line break before code comments.

Almost there!

Fixes #23 comments.

Oh here it is, bumping the issue :)

I think this should even be critical since the dblog filter form is not usable right now and this *really* needs to be converted to DBTNG.

Patch probably only needs a re-roll, will do this soon...

Ok, it seems that the original bug has been fixed, so this only needs to be converted to DBTNG, similiar to other build filter query functions.

This isn't critical then...

Attaching a updated patch and also add a multiple types condition test to the existing testFilter test instead of the above test function.

forgot the patch?

Ups...

The patch makes some comment and documentation tweaks which while good are patch creep.

If we're going to creep this patch, I'd say that building half the query in one function and half in another, and at the other end of the file to boot, is a bit of a WTF -- how about we fix that?

#29: 445482_PDOException_reroll.patch queued for re-testing.

Hm. I could remove the documentation fixes, but they are in the same function and minor - just two changed lines and no text changes.

Not sure about removing the function. We have similiar functions for node and user filter forms and it doesn't hurt to have them.

Surprisingly doesn't apply anymore. :)

A reroll of the reroll by Berdir attached. Only one instance of creep removed, but going to have it run through testing first to see if the functionality is even up to snuff before messing more with it.

+++ b/core/modules/dblog/dblog.admin.incundefined
@@ -63,7 +59,7 @@ function dblog_overview() {
       ),
-      // Attributes for tr
+      // Attributes for table row.
       'class' => array(drupal_html_class('dblog-' . $dblog->type), $classes[$dblog->severity]),

Fixing/Improving multiple things is usually discouraged, because it could possibly result in unecessary conflicts and makes patches harder to review and get committed. http://webchick.net/please-stop-eating-baby-kittens is a good read, in case you don't know it yet.

@Berdir: Quoting myself here:

Only one instance of creep removed, but going to have it run through testing [...] before messing more with it.

This was just a simple re-roll of your code to the latest HEAD of the 8.x branch. Please do feel free to take it over again. It's touching a lot of core stuff I'm not very familiar with, so I'm not sure how much I'm going to get involved with it.

Already fixed here: #1977254: Convert dblog_overview() to a Controller