In order to convert the dblog listing to views let's first add a views integration.

CommentFileSizeAuthor
#74 vdc-dblog-1874534-27.png289.59 KBR.Hendel
#73 dblog-1874534-73.patch15.69 KBdawehner
#73 interdiff.txt1.2 KBdawehner
#66 vdc-1874534-66.patch16.2 KBdawehner
#66 interdiff.txt2.87 KBdawehner
#63 Screen Shot 2013-08-21 at 12.14.41 AM.png20.4 KBwebchick
#59 watchdog.png11.73 KBdawehner
#59 interdiff.txt1.42 KBdawehner
#59 vdc-1874534-59.patch16.13 KBdawehner
#58 Screen Shot 2013-08-03 at 11.43.20 PM.png23.46 KBwebchick
#58 Screen Shot 2013-08-03 at 11.46.05 PM.png17.55 KBwebchick
#54 1874534-54-asc.png75.89 KBR.Hendel
#54 1874534-54-desc.png99.08 KBR.Hendel
#52 vdc-1874534-52.patch14.66 KBolli
#52 interdiff.txt550 bytesolli
#49 1874534-01.png38.15 KBR.Hendel
#49 1874534-02.png153.25 KBR.Hendel
#49 1874534-03.png159.7 KBR.Hendel
#46 vdc-1874534-46.patch14.65 KBolli
#46 interdiff.txt1.23 KBolli
#44 vdc-1874534-44.patch14.58 KBolli
#44 interdiff.txt1.95 KBolli
#40 vdc-1874534-40.patch14.58 KBdawehner
#40 interdiff.txt516 bytesdawehner
#38 vdc-1874534-38.patch14.58 KBdawehner
#38 interdiff.txt1.08 KBdawehner
#35 drupal-1874534-35.patch14.51 KBdawehner
#31 vdc-watchdog-1874534-31.patch14.75 KBdawehner
#27 vdc-dblog-1874534-27.patch31.36 KBPsikik
#25 drupal-1874534-25.patch14.75 KBdawehner
#25 interdiff.txt616 bytesdawehner
#17 drupal-vdc-dblog-1874534-17.patch14.7 KBPsikik
#16 drupal-1874534-16.patch16.21 KBdawehner
#16 interdiff.txt5.1 KBdawehner
#15 1874534-15.patch15.24 KBdamiankloip
#15 interdiff.txt2.47 KBdamiankloip
#13 1874534-13.patch15.16 KBdamiankloip
#13 interdiff.txt3.61 KBdamiankloip
#12 1874534-12.patch13.15 KBdamiankloip
#12 interdiff.txt5.38 KBdamiankloip
#10 drupal-1874534-10.patch13.4 KBdawehner
#10 interdiff.txt2.8 KBdawehner
#9 1874534-9.patch13.75 KBdamiankloip
#9 interdiff.txt705 bytesdamiankloip
#4 interdiff.txt4.09 KBdamiankloip
#3 drupal-1874534-3.patch13.77 KBdamiankloip
#1 drupal-1874534-1.patch13.36 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
13.36 KB

Here is a first version.

Status: Needs review » Needs work

The last submitted patch, drupal-1874534-1.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
13.77 KB

Just fixed a few things. As for testing, we both have issues that have Integration tests, I haven't seen the ones you wrote yet, but I think we should catch up and work out what is a good idea as the bear minimum that these tests should generally cover for module data integrations?

damiankloip’s picture

FileSize
4.09 KB

Oh, and an interdiff. Sorry.

dawehner’s picture

+++ b/core/modules/dblog/dblog.views.incundefined
@@ -69,12 +72,14 @@ function dblog_views_data() {
+      'click sortable' => TRUE,

On the longrun we should think about making click sortable the default.

+++ b/core/modules/dblog/lib/Drupal/dblog/Plugin/views/field/DblogMessage.phpundefined
@@ -67,7 +67,7 @@ function render($values) {
+      return format_string($value, $variables ? $variables : array());

Is there any reason $variables doesn't exist?

damiankloip’s picture

Ah, format_string expects an array as the second arg, if you unserialize nothing or some broken string, you get nothing, so we need to default this to an array.

Yeah, we could even look at making some plugins a default in the data too? click sortable would probably be easier as opt out yes.

dawehner’s picture

What about casting it to an array then?

damiankloip’s picture

Yeah, why not!

damiankloip’s picture

FileSize
705 bytes
13.75 KB

Here is that casting change, I think we are looking good now.

dawehner’s picture

Status: Needs review » Needs work

The last submitted patch, drupal-1874534-10.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
5.38 KB
13.15 KB

Here is a re roll with some recent changes incorporated, like removing click sortable from the views data. I have also fixed the unit tests so they run, I think we can press on here without the issue mentioned in #10 and change later if we need to? Seems like that issue might take a while longer :)

damiankloip’s picture

FileSize
3.61 KB
15.16 KB

Made a few more changes, I also added a DblogOperations handler, to correctly display the link column markup.

Maybe we should abstract this out and make a generic 'Markup' field handler?

dawehner’s picture

Just some small points. I'm happy to write tests later.

+++ b/core/modules/dblog/lib/Drupal/dblog/Plugin/views/field/DblogMessage.phpundefined
@@ -62,12 +63,12 @@ public function buildOptionsForm(&$form, &$form_state) {
+      return $this->sanitizeValue(format_string($value, (array) $variables));

So peter and berdir had some discussion about that in IRC, and I think it should not be escaped. The message strings itself should be safe, and the placeholders exploded properly.

+++ b/core/modules/dblog/lib/Drupal/dblog/Tests/Views/ViewsIntegrationTest.phpundefined
@@ -27,21 +27,20 @@ class ViewsIntegrationTest extends ViewUnitTestBase {
+    $this->enableModules(array('system', 'dblog'));

I really thing we should not enable system, as it hides the needed tables in the future.

+++ b/core/modules/dblog/dblog.views.incundefined
@@ -134,11 +135,11 @@ function dblog_views_data() {
+      'click sortable' => FALSE,

What about putting that onto the handler?

damiankloip’s picture

FileSize
2.47 KB
15.24 KB

I have made those changes, apart from switching the test back to install schema. This was not working when I did and I don;t have time now.

So I think now this patch is basically just 'Needs tests'.

dawehner’s picture

FileSize
5.1 KB
16.21 KB

So here are some tests.

Psikik’s picture

Here's a re-rolled patch

Status: Needs review » Needs work
Issue tags: -Needs tests, -VDC

The last submitted patch, drupal-vdc-dblog-1874534-17.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review

#17: drupal-vdc-dblog-1874534-17.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, drupal-vdc-dblog-1874534-17.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

#17: drupal-vdc-dblog-1874534-17.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, drupal-vdc-dblog-1874534-17.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

#17: drupal-vdc-dblog-1874534-17.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs tests, +VDC

The last submitted patch, drupal-vdc-dblog-1874534-17.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
616 bytes
14.75 KB

Let's fix the notice.

dawehner’s picture

Issue tags: -Needs tests

No tests needed anymore

Psikik’s picture

FileSize
31.36 KB

Added in the replacement recent log entries view.

Status: Needs review » Needs work
Issue tags: -VDC

The last submitted patch, vdc-dblog-1874534-27.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +VDC

#27: vdc-dblog-1874534-27.patch queued for re-testing.

dawehner’s picture

Thanks psikik! I would like to not add the replacement in this issue because there will be a ton of discussion and as result nothing will happen. Can we first get the fine integration in, please?

dawehner’s picture

@Psikik
So yeah, please fill a follow up issue, sorry.

Removed the view from #27

klonos’s picture

Status: Needs review » Needs work

The last submitted patch, vdc-watchdog-1874534-31.patch, failed testing.

jibran’s picture

+++ b/core/modules/dblog/tests/modules/dblog_test_views/dblog_test_views.infoundefined
@@ -0,0 +1,8 @@
+name = Dblog test views
+description = Provides default views for views dblog tests.
+package = Testing
+version = VERSION
+core = 8.x
+dependencies[] = dblog
+dependencies[] = views

What info file? I dono any info file in D8. :D

dawehner’s picture

Status: Needs work » Needs review
FileSize
14.51 KB

Ha, good point.

jibran’s picture

Some minors.

+++ b/core/modules/dblog/dblog.moduleundefined
@@ -134,7 +134,7 @@ function _dblog_get_message_types() {
+  return drupal_map_assoc($types);

1. #1973312: Move drupal_map_assoc to a MapArray Utility Component :)

+++ b/core/modules/dblog/lib/Drupal/dblog/Plugin/views/field/DblogOperations.phpundefined
@@ -0,0 +1,39 @@
+   * Overrides \Drupal\views\Plugin\views\field\FieldPluginBase::FieldPluginBase().

2. {@inheritdoc}

Status: Needs review » Needs work

The last submitted patch, drupal-1874534-35.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.08 KB
14.58 KB
jibran’s picture

+++ b/core/modules/dblog/lib/Drupal/dblog/Tests/Views/ViewsIntegrationTest.phpundefined
@@ -0,0 +1,108 @@
+   * @inheritdoc}

I am so sorry about that.
:P

dawehner’s picture

FileSize
516 bytes
14.58 KB

Hehe.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

We have a follow up #2015149: Replace dblog recent log entries with a view and a green patch with tests. I think it is good to go.

dawehner’s picture

Issue tags: -VDC

#40: vdc-1874534-40.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +VDC

The last submitted patch, vdc-1874534-40.patch, failed testing.

olli’s picture

Status: Needs work » Needs review
FileSize
1.95 KB
14.58 KB

Reroll.

dawehner’s picture

Thanks for the rerole.

+++ b/core/modules/dblog/lib/Drupal/dblog/Tests/Views/ViewsIntegrationTest.phpundefined
@@ -0,0 +1,108 @@
+      $this->assertEqual($view->style_plugin->getField($index, 'message'), format_string($entry['message'], $entry['variables']));
+      $this->assertEqual($view->style_plugin->getField($index, 'link'), filter_xss_admin($entry['link']));

While changing this use String::format and Xss::filterAdmin.

olli’s picture

FileSize
1.23 KB
14.65 KB

Thanks!

Status: Needs review » Needs work
Issue tags: -VDC

The last submitted patch, vdc-1874534-46.patch, failed testing.

olli’s picture

Status: Needs work » Needs review
Issue tags: +VDC

#46: vdc-1874534-46.patch queued for re-testing.

R.Hendel’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
159.7 KB
153.25 KB
38.15 KB

Patch works as expected and very well!

After installing a fresh new drupal and patching it with #46 you find a new option while creating new view:

1874534-01.png

In views-backend you can add every property as single field:

1874534-02.png

On views-page you find'll your fields displayed:

1874534-03.png

dawehner’s picture

+1 from the codeside, as the rest of nitpicks got fixed.

damiankloip’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/dblog/lib/Drupal/dblog/Plugin/views/field/DblogOperations.phpundefined
@@ -0,0 +1,39 @@
+  function clickSortable() {

The patch is looking great now, this is the only minor thing I can spot. Let's add public here.

olli’s picture

Status: Needs work » Needs review
FileSize
550 bytes
14.66 KB

Thanks, let's do that.

damiankloip’s picture

Status: Needs review » Reviewed & tested by the community

Great.

R.Hendel’s picture

FileSize
99.08 KB
75.89 KB

I tested with the same method as described in #49.

This time I choose an table based view and activated "sortable" in all possible columns.
As there are always overall same data in rows, I sorted by WID.

See WID ascending:

1874534-54-asc.png

... and see WID descending:

1874534-54-desc.png

So in my opinion it's all working now.

YesCT’s picture

Issue tags: +RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

tim.plunkett’s picture

Issue tags: -VDC, -RTBC July 1

#52: vdc-1874534-52.patch queued for re-testing.

xjm’s picture

Issue tags: +VDC, +RTBC July 1

#52: vdc-1874534-52.patch queued for re-testing.

webchick’s picture

Category: feature » task
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests
FileSize
17.55 KB
23.46 KB

Hm. Why is "Unsorted" the only option in the wizard? I would expect to see "newest first / oldest first" as options, like I do with files.

After selecting 'watchdog entries' as the type, there are no sorts available

What is an "Unformatted list of Search"? (Meh, I guess files do the same thing, so this wasn't particularly introduced here, but ick.)

You can specify an unformatted list of fields or search

When I choose those options along with a page display, the preview box below shows me the following error:

SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'FROM watchdog watchdog LIMIT 10 OFFSET 0' at line 1: SELECT FROM {watchdog} watchdog LIMIT 10 OFFSET 0; Array ( ) 

... which the tests should be picking up. So needs tests for that.

Presumably, this is because no fields are selected, but under fields it says "The selected style or row format does not utilize fields."

Selecting "Fields" instead of "Search" works fine, so my guess is we need to remove that "Search" option, or else fix it so it doesn't blow up.

Also, this feels more "tasky" to me than "featurey"... it's simply providing views integration for something currently without it. But what's the nature of this hunk:

+++ b/core/modules/dblog/dblog.module
@@ -134,7 +135,7 @@ function _dblog_get_message_types() {
-  return $types;
+  return MapArray::copyValuesToKeys($types);

Does this represent an API change? Or just a "more betterer" way to say the same thing?

dawehner’s picture

Status: Needs work » Needs review
FileSize
16.13 KB
1.42 KB
11.73 KB

Thanks for the great review!

The fix for the first is so simple that I will do it here, see #1853536: Reintroduce Views integration for search.module (not supporting backlinks view) for tests of the search integration.

Selecting "Fields" instead of "Search" works fine, so my guess is we need to remove that "Search" option, or else fix it so it doesn't blow up.

I am not sure, whether I should be honest, but let's just be honest. The search row plugin never worked (at least since drupal 6), so that is why it is currently marked as no_ui...

The patch also adds a new wizard plugin, which allows to define a "created column", so newest first and oldest first automatically appears. In general I prefer iterative progress instead of making it perfect from the beginning.

watchdog.png

Does this represent an API change? Or just a "more betterer" way to say the same thing?

Well, we just need to have the watchdog types by KEY, so why not allow other people to use it as well. This is a underscore function, so internal changes should be allowed.

dawehner’s picture

Issue tags: -Needs tests

Tests for the search problem are added somewhere else.

damiankloip’s picture

Status: Needs review » Reviewed & tested by the community

This looks like a fair compromise, and yes, no_ui is a good idea for search!

alesr’s picture

1. Is it possible to put current Recent log messages in views? Why having views in core if those core tables are not editable :/
Found the issue https://drupal.org/node/2015149

2. Maybe we could set the default format type to "Table" instead of "Unformatted list".
A log in an unformatted list is not what you would want by default.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
20.4 KB

Cool, thanks a lot for fixing my previous feedback. This works much better now. More UI feedback...

Hm. How did "Watchdog entries" end up second in the list here? Seems like it should be alphabetically last.

Screen Shot 2013-08-21 at 12.14.41 AM.png

Oh, wow. This list seems to be in totally random order. Maybe a sub-issue to fix that? In the meantime, can we push it further down the list so it's not right in peoples' faces? This will be one of the least-used type of views, IMO.

Also, let's rename this to "Log messages," IMO. The term "Watchdog" doesn't show up in the UI anywhere as far as I know, so users aren't going to know what this means.

I proceeded to add all fields to a table view. I had the following questions from that:

1) Why are "Location" and "Referer" [sic] not output as links?
2) Why on earth would anyone ever want to add "Variables" to a view? Is there any use case for exposing this? If not, we might want to remove it so that the UI is (slightly) simpler.

And finally, because I am testing creating a log message view, I can see that while doing this in the preview that my log files are getting filled with errors like this:

Strict warning: Declaration of Drupal\dblog\Plugin\views\field\DblogMessage::render() should be compatible with Drupal\views\Plugin\views\field\FieldPluginBase::render(Drupal\views\ResultRow $values) in require() (line 23 of /Users/webchick/Sites/8.x/core/modules/dblog/lib/Drupal/dblog/Plugin/views/field/DblogMessage.php). 

Seems like we need to fix that. :)

Looking at the code, only thing I had a question about was...

+++ b/core/modules/dblog/dblog.views.inc
@@ -0,0 +1,224 @@
+ * @ingroup views_module_ids

I can't find reference to this group anywhere else in core. Is this a typo? Or, if it's meant to start introducing a new @ingroup, we should probably do that in a separate issue.

webchick’s picture

Also if #62.2 is easy to do, I concur that this is what most people would want. Unless this would make it totally unstandard from other view types, in which case nevermind.

tim.plunkett’s picture

The list is unsorted, so the natural order is by module provider (as it is with all plugins):

array (
  'comment' => 'comment',
  'watchdog' => 'dblog',
  'file_managed' => 'file',
  'node' => 'node',
  'node_revision' => 'node',
  'taxonomy_term' => 'taxonomy',
  'users' => 'user',
)

We can introduce a sort in a follow-up.

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.87 KB
16.2 KB

Additional issue for the weight: #2071189: Allow to order views wizard entries

Fixed the php errors.

1) Why are "Location" and "Referer" [sic] not output as links?

I would tell people to use the rewrite functionality of views to produce a link. This is the suggested way to get all the flexibility out of views.

2) Why on earth would anyone ever want to add "Variables" to a view? Is there any use case for exposing this? If not, we might want to remove it so that the UI is (slightly) simpler.

You can configure for example which key should be displayed, which then might be used somewhere else. Another usecase for the variables might be, if you expose the watchdog messages via a view export to other people. Not adding something, even it is possible feels just wrong. The code for serialized data already exists.

I can't find reference to this group anywhere else in core. Is this a typo? Or, if it's meant to start introducing a new @ingroup, we should probably do that in a separate issue.

Yeah this is just a typo.

dawehner’s picture

I tried to change the default value to be a table, though this was not as simple as I thought.
The following code does not work:

  /**
   * {@inheritdoc}
   */
  public function buildForm(array $form, array &$form_state) {
    $form = parent::buildForm($form, $form_state);

    // Change the default style to be table.
    $form['displays']['page']['options']['style']['style_plugin']['#default_value'] = 'table';

    return $form;
  }

  /**
   * {@inheritdoc}
   */
  protected function defaultDisplayOptions() {
    $display_options = parent::defaultDisplayOptions();
    $display_options['style']['type'] = 'table';

    return $display_options;
  }
damiankloip’s picture

Yes, I tried the exact same thing earlier. I have an almost identical I diff. Didn't work either!

damiankloip’s picture

I think we should just get this in. The only issue left is the defaulting to table thing, which tbh is a very small UX improvement. However, now that we have found this issue/bug with the wizard, I think we should tackle it there instead?

dawehner’s picture

damiankloip’s picture

Issue tags: -VDC, -RTBC July 1

#66: vdc-1874534-66.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +VDC, +RTBC July 1

The last submitted patch, vdc-1874534-66.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.2 KB
15.69 KB

There we go.

R.Hendel’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
289.59 KB

I tested patch #73 at simplytest.me and found, that it works as expected:

You can create a view selecting log-entries. If you want to have fields "Location" and "Refererer" displayed as links you can do so by rewriting results. (see screenshot)

vdc-dblog-1874534-27.png

So in my opionion everything works fine :-)

damiankloip’s picture

Great! +1 on that from a code perspective. Now we have a followup for the wizard issue 'n' all.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Awesome, this looks/works great now! :D Nothing further to complain about.

Committed and pushed to 8.x. Thanks!

jibran’s picture

Title: Introduce a dblog / watchdog views integration » Change notice: Introduce a dblog / watchdog views integration
Priority: Normal » Critical
Status: Fixed » Active
Issue tags: +Needs change record

This is an api addition so deserves change notice.

ParisLiakos’s picture

seriously?
we didnt get change notices for other modules views integration and i am having hard time to remember a change notification that is purely an api additon.

jibran’s picture

Title: Change notice: Introduce a dblog / watchdog views integration » Introduce a dblog / watchdog views integration
Priority: Critical » Normal
Status: Active » Fixed
Issue tags: -Needs change record

if @ParisLiakos thinks it doesn't need change notice it is fine by me. I asked @damiankloip, he agreed, before adding the "needs change notification" tag. Sorry for the noise. :)

ParisLiakos’s picture

no problem and sorry if i sounded too harsh, but none similar issue got a change notice, which makes sense imo.
there is no point making the dusty list of "critical for a change notice" issues even bigger

klonos’s picture

Besides, if I've got this right, this issue is part of #1823450: [Meta] Convert core listings to Views so may it will get a mention in the single changelog we create for that one ;)

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

dawehner’s picture

Issue summary: View changes

Added the follow up, which existed before ...