Convert dblog_top() to a controller

That's really good so far

+++ b/core/modules/dblog/dblog.moduleundefined
@@ -71,16 +63,12 @@ function dblog_menu() {
-  if (module_exists('search')) {
...
+  $items['admin/reports/search'] = array(
+    'title' => 'Top search phrases',
+    'description' => 'View most popular search phrases.',
+    'route_name' => 'dblog_top',

Don't we still need the search module for this functionality? The problem with that is, that it will appear in the menu, but you will get a 404 at the end.

+++ b/core/modules/dblog/lib/Drupal/dblog/Controller/DBLogController.phpundefined
@@ -0,0 +1,312 @@
+   * Database Service.

Something like "The database connection" is more descriptive.

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

The constructor needs some documentation.

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

Missing @return

+++ b/core/modules/dblog/lib/Drupal/dblog/Controller/DBLogController.phpundefined
@@ -0,0 +1,312 @@
+   * Page callback: Shows the most frequent log messages of a given event type.

That's not really a page callback anymore :)

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

This @see doesn't seem to be really helpful anymore. What do you thin?

+++ b/core/modules/dblog/lib/Drupal/dblog/Tests/DBLogTest.phpundefined
@@ -575,17 +576,7 @@ protected function getTypeCount(array $types) {
+    $map = array_flip(DBLogController::getLogLevelClassMap());

Nice!

lets wait for this #1977254: Convert dblog_overview() to a Controller

#17: convert-dblog_top-to-a-controller-1982954-17.patch queued for re-testing.

Rerolled against the dblog overview push.

+++ b/core/modules/dblog/dblog.routing.ymlundefined
@@ -4,3 +4,11 @@ dblog_overview:
+    type: (access-denied|search|page-not-found).*

this is so cool!

i cant see though dblog_top's removal, is it used anywhere?

You are totally right.

i am not sure why...but after applying this patch, admin/reports misses all those 3 links:( just tested on simplytest.me:/

That's more tricky to solve.

The requirements checking via a regex on parameters is not based on the access manager but on some symfony related regex bits.

Seems like we can reroll at least.

Here's a reroll of the patch in #26.

Here is an updated patch with the reroll as well as changing drupal_set_title to $build['#title'] . Hope I understood this correctly.

Here's an interdiff for the patch in #31.

Fixed a couple bugs in the reroll in #30 and the title fix in #31.

Great job on the reroll mdrummond! Here's some minor coding style modifications that need done.

1. +++ b/core/modules/dblog/dblog.routing.yml
   @@ -4,10 +4,17 @@ dblog_overview:
   -
   

   add this blank line back.

2. +++ b/core/modules/dblog/dblog.routing.yml
   @@ -4,10 +4,17 @@ dblog_overview:
        _permission: 'access site reports'
   +dblog_top:
   

   add blank line between these also.

3. +++ b/core/modules/dblog/dblog.routing.yml
   @@ -4,10 +4,17 @@ dblog_overview:
   \ No newline at end of file
   

   needs new line at end of file

4. +++ b/core/modules/dblog/lib/Drupal/dblog/Controller/DbLogController.php
   @@ -319,4 +320,99 @@ protected function buildFilterQuery() {
   +        $build['#title'] = "Top 'page not found' errors";        ¶
   

   extraneous spaces at end of this line

5. +++ b/core/modules/dblog/lib/Drupal/dblog/Controller/DbLogController.php
   @@ -319,4 +320,99 @@ protected function buildFilterQuery() {
   +      array('data' => t('Count'), 'field' => 'count', 'sort' => 'desc'),
   +      array('data' => t('Message'), 'field' => 'message'),
   ...
   +          $message = t($dblog->message, unserialize($dblog->variables));
   ...
   +      '#empty' => t('No log messages available.'),
   

   Change t( to $this->t(

I made the above changes.

I changed all the t functions to $this->t functions. If that was too aggressive, my apologies.

+++ b/core/modules/dblog/dblog.routing.yml
@@ -17,4 +19,5 @@ dblog_top:
+    ¶

Newline character still isn't correct. Get rid of the spaces. Note that when you have missing newline character at end of file, or extra spaces at ends of lines, git diff should show it in bright red background text.

$this->t() and other changes are perfect.

Got rid of the spaces this time.

Now that the views integration is in it seems resonable to just use views to provide this top listing: #2015149: Replace dblog recent log entries with a view

Patch fixes remaining test failures. As for #42 I have no problems with switching this to a view, but I think that would make more sense to do in a follow-up issue and get this committed before we change our strategy on these conversions.

1. +++ w/core/modules/dblog/dblog.module
   @@ -49,34 +49,24 @@ function dblog_menu() {
    
   -  if (module_exists('search')) {
   -    $items['admin/reports/search'] = array(
   -      'title' => 'Top search phrases',
   -      'description' => 'View most popular search phrases.',
   -      'page callback' => 'dblog_top',
   -      'page arguments' => array('search'),
   -      'access arguments' => array('access site reports'),
   -      'file' => 'dblog.admin.inc',
   -    );
   -  }
   +  $items['admin/reports/search'] = array(
   +    'title' => 'Top search phrases',
   +    'description' => 'View most popular search phrases.',
   +    'route_name' => 'dblog_top',
   +  );
   +
    
      return $items;
   

   mhh, why did you removed the module('search') bit? Doesn't that mean that the link might still appear?

2. +++ w/core/modules/dblog/lib/Drupal/dblog/Controller/DbLogController.php
   @@ -319,4 +320,99 @@ protected function buildFilterQuery() {
   +   * @throws \Symfony\Component\HttpKernel\Exception\NotFoundHttpException
   +   *
   

   Maybe we could write something like: Thrown when watchdog listing for search got accessed but search is not enabled.

addressed comments in #44.
Removed title from hook_menu

Also, renamed top() method to topLogMessages()

Perfect, thank you!

Without the titles in the hook_menu implementation the menu items disappear.

Before patch

After patch

ok, adding titles back in to hook_menu.

#49: drupal8.dblog-module.1982954-49.patch queued for re-testing.

Great!

#49: drupal8.dblog-module.1982954-49.patch queued for re-testing.

Yes, it would make sense to use a view for that, but the other issue is on needs work and we need to get rid of the old router items: #2015149: Replace dblog recent log entries with a view

Here's a fixed patch.

1) Page title inherited from menu
2) Reverted unused changes
3) no reason to have {type} - straight conversion so here's 3 routes

I don't think we need to implement route subscriber to add search route depending on module existence, just hidding of menu item is enough

At least let's not drop the title from the route definition, this is wrong.

It's either commit #56 or fix the test to use a normal quote and not an html escaped quote. When _title is used, the quote isn't converted to &#039, hence why I fixed the test.

I see no reason in title for this routes because we always have menus for them.
And this is a conversion of callback to controller so let's leave menu and titles as they are

I see no reason in title for this routes because we always have menus for them.

That is not true. For things like building the breadcrumb we should better use the title on the route definition.
I guess I mixed up the single quote syntax of yaml, ... feel free to fix it.

See http://yaml.org/spec/current.html#id2534365

Seems '_title' in yml is not passed check_plain()

Your syntax is correct, just the way the title is rendered in hook_menu and in _title is different.

#63: dblog-1982954-62.patch queued for re-testing.

Rerolled the patch since #2089635: Convert non-test non-form page callbacks to routes and controllers ruined the patch that was created.

I kept the naming of the routes that was already created in #2089635: Convert non-test non-form page callbacks to routes and controllers since they seemed to be more consistent with rest of core (dblog.search vs dblog_top_search)

Fixed the above issue, which meant removing the dblog.search as an dynamic route and use the static type which was introduced in the original patch.

+++ b/core/modules/dblog/lib/Drupal/dblog/Controller/DbLogController.php
@@ -320,27 +320,69 @@ protected function buildFilterQuery() {
+      ->extend('Drupal\Core\Database\Query\PagerSelectExtender')
+      ->extend('Drupal\Core\Database\Query\TableSortExtender');

It is \Drupal now.

I did what #72 said and fixed the issue from the test.

I found no nitpicks

Patch no longer applies.

straight reroll. If this is green back to RTBC.

Straight RTBC though I realized that we could improve somehow the information available on on the 403/404 pages. Message is not really the proper table header
as this is always just the path where the error happened.

Daniel can you explain in a follow up issue?

There you go: #2105431: Rename the table headers on the watchdog 403/404 controllers

+1 This is RTBC.

#75: drupal8.dblog-module.1982954-75.patch queued for re-testing.

untag

The with this patch applied the dblog.routing.yml file has a duplicate entry for dblog.search. They need to be combined :)

dblog.search:
  path: '/admin/reports/search'
  defaults:
    _title: "Top search phrases"
    _content: '\Drupal\dblog\Controller\DbLogController::topLogMessages'
    type: 'search'
  requirements:
    _permission: 'access site reports'

dblog.search:
  path: '/admin/reports/search'
  defaults:
    _content: '\Drupal\dblog\Controller\DbLogController::search'
    _title: 'Top search phrases'
  requirements:
    _module_dependencies: 'search'
    _permission: 'access site reports'

Thank you
corrected #82

Here's the interdiff.

#84 the interdiff is twice the size of the patch :) - not so helpful!!!

83: drupal8.dblog-module.1982954-83.patch queued for re-testing.

Patch still appplies, and this is just great clean-up

Committed and pushed to 8.x. Thanks!