![Announcement icon Announcement icon](https://www.drupal.org/files/styles/grid-2-2x-square/public/announcements/drupalcon-hexagon-icon-280X280px_2.png?itok=Oxf_7ZRC)
In order to convert the dblog listing to views let's first add a views integration.
Comment | File | Size | Author |
---|---|---|---|
#74 | vdc-dblog-1874534-27.png | 289.59 KB | R.Hendel |
#73 | dblog-1874534-73.patch | 15.69 KB | dawehner |
#73 | interdiff.txt | 1.2 KB | dawehner |
#66 | vdc-1874534-66.patch | 16.2 KB | dawehner |
#66 | interdiff.txt | 2.87 KB | dawehner |
![Acquia logo](/files/ads/acquia-issue-sponsorship.png)
Comments
Comment #1
dawehnerHere is a first version.
Comment #3
damiankloip CreditAttribution: damiankloip commentedJust 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?
Comment #4
damiankloip CreditAttribution: damiankloip commentedOh, and an interdiff. Sorry.
Comment #5
dawehnerOn the longrun we should think about making click sortable the default.
Is there any reason $variables doesn't exist?
Comment #6
damiankloip CreditAttribution: damiankloip commentedAh, 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.
Comment #7
dawehnerWhat about casting it to an array then?
Comment #8
damiankloip CreditAttribution: damiankloip commentedYeah, why not!
Comment #9
damiankloip CreditAttribution: damiankloip commentedHere is that casting change, I think we are looking good now.
Comment #10
dawehnerIn order to write proper tests, we need #1893730: Provide an in-memory mock implementation of Path\AliasManager for tests and update.php
Comment #12
damiankloip CreditAttribution: damiankloip commentedHere 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 :)
Comment #13
damiankloip CreditAttribution: damiankloip commentedMade 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?
Comment #14
dawehnerJust some small points. I'm happy to write tests later.
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.
I really thing we should not enable system, as it hides the needed tables in the future.
What about putting that onto the handler?
Comment #15
damiankloip CreditAttribution: damiankloip commentedI 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'.
Comment #16
dawehnerSo here are some tests.
Comment #17
Psikik CreditAttribution: Psikik commentedHere's a re-rolled patch
Comment #19
damiankloip CreditAttribution: damiankloip commented#17: drupal-vdc-dblog-1874534-17.patch queued for re-testing.
Comment #21
dawehner#17: drupal-vdc-dblog-1874534-17.patch queued for re-testing.
Comment #23
dawehner#17: drupal-vdc-dblog-1874534-17.patch queued for re-testing.
Comment #25
dawehnerLet's fix the notice.
Comment #26
dawehnerNo tests needed anymore
Comment #27
Psikik CreditAttribution: Psikik commentedAdded in the replacement recent log entries view.
Comment #29
tim.plunkett#27: vdc-dblog-1874534-27.patch queued for re-testing.
Comment #30
dawehnerThanks 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?
Comment #31
dawehner@Psikik
So yeah, please fill a follow up issue, sorry.
Removed the view from #27
Comment #32
klonos...#2015149: Replace dblog recent log entries with a view ;)
Comment #34
jibranWhat info file? I dono any info file in D8. :D
Comment #35
dawehnerHa, good point.
Comment #36
jibranSome minors.
1. #1973312: Move drupal_map_assoc to a MapArray Utility Component :)
2. {@inheritdoc}
Comment #38
dawehnerComment #39
jibranI am so sorry about that.
:P
Comment #40
dawehnerHehe.
Comment #41
jibranWe 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.
Comment #42
dawehner#40: vdc-1874534-40.patch queued for re-testing.
Comment #44
olli CreditAttribution: olli commentedReroll.
Comment #45
dawehnerThanks for the rerole.
While changing this use String::format and Xss::filterAdmin.
Comment #46
olli CreditAttribution: olli commentedThanks!
Comment #48
olli CreditAttribution: olli commented#46: vdc-1874534-46.patch queued for re-testing.
Comment #49
R.Hendel CreditAttribution: R.Hendel commentedPatch 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:
In views-backend you can add every property as single field:
On views-page you find'll your fields displayed:
Comment #50
dawehner+1 from the codeside, as the rest of nitpicks got fixed.
Comment #51
damiankloip CreditAttribution: damiankloip commentedThe patch is looking great now, this is the only minor thing I can spot. Let's add public here.
Comment #52
olli CreditAttribution: olli commentedThanks, let's do that.
Comment #53
damiankloip CreditAttribution: damiankloip commentedGreat.
Comment #54
R.Hendel CreditAttribution: R.Hendel commentedI 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:
... and see WID descending:
So in my opinion it's all working now.
Comment #55
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #56
tim.plunkett#52: vdc-1874534-52.patch queued for re-testing.
Comment #57
xjm#52: vdc-1874534-52.patch queued for re-testing.
Comment #58
webchickHm. 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.
What is an "Unformatted list of Search"? (Meh, I guess files do the same thing, so this wasn't particularly introduced here, but ick.)
When I choose those options along with a page display, the preview box below shows me the following error:
... 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:
Does this represent an API change? Or just a "more betterer" way to say the same thing?
Comment #59
dawehnerThanks 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.
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.
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.
Comment #60
dawehnerTests for the search problem are added somewhere else.
Comment #61
damiankloip CreditAttribution: damiankloip commentedThis looks like a fair compromise, and yes, no_ui is a good idea for search!
Comment #62
alesr CreditAttribution: alesr commented1. 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.
Comment #63
webchickCool, 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.
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:
Seems like we need to fix that. :)
Looking at the code, only thing I had a question about was...
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.
Comment #64
webchickAlso 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.
Comment #65
tim.plunkettThe list is unsorted, so the natural order is by module provider (as it is with all plugins):
We can introduce a sort in a follow-up.
Comment #66
dawehnerAdditional issue for the weight: #2071189: Allow to order views wizard entries
Fixed the php errors.
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.
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.
Yeah this is just a typo.
Comment #67
dawehnerI 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:
Comment #68
damiankloip CreditAttribution: damiankloip commentedYes, I tried the exact same thing earlier. I have an almost identical I diff. Didn't work either!
Comment #69
damiankloip CreditAttribution: damiankloip commentedI 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?
Comment #70
dawehnerOpened a followup: #2079699: Make it easier to change the default style plugin per wizard
Comment #71
damiankloip CreditAttribution: damiankloip commented#66: vdc-1874534-66.patch queued for re-testing.
Comment #73
dawehnerThere we go.
Comment #74
R.Hendel CreditAttribution: R.Hendel commentedI 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)
So in my opionion everything works fine :-)
Comment #75
damiankloip CreditAttribution: damiankloip commentedGreat! +1 on that from a code perspective. Now we have a followup for the wizard issue 'n' all.
Comment #76
webchickAwesome, this looks/works great now! :D Nothing further to complain about.
Committed and pushed to 8.x. Thanks!
Comment #77
jibranThis is an api addition so deserves change notice.
Comment #78
ParisLiakos CreditAttribution: ParisLiakos commentedseriously?
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.
Comment #79
jibranif @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. :)
Comment #80
ParisLiakos CreditAttribution: ParisLiakos commentedno 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
Comment #81
klonosBesides, 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 ;)
Comment #83
dawehnerAdded the follow up, which existed before ...