When you look at a single log event page, the breadcrumb should let you go back up a level to the whole log.

However, the last item in the breadcrumb is the 'Reports' admin landing page.

CommentFileSizeAuthor
#15 dblog-breadcrumb-2267053-15.patch2.39 KBDaniel Norton
#11 dblog-breadcrumb-2267053-11.patch2.44 KBiMiksu
#7 dblog-breadcrumb-2267053-7.patch1.71 KBsidharthap
#6 dblog-breadcrumb-2267053-6.patch1.71 KBsidharthap
#3 dblog-breadcrumb-2267053-3.patch493 bytesemclaughlin
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented

This always annoyed me

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Issue tags: +Novice

Yeah in theory this should just be the change in the dblog.routing.yml and the rest will be taken over automatically.

emclaughlin’s picture

emclaughlin CreditAttribution: emclaughlin commented
FileSize
dblog-breadcrumb-2267053-3.patch493 bytes

Does this have the right component? I assume that it's actually for dblog.module, not syslog.module. I've included a patch for dblog.module.

emclaughlin’s picture

emclaughlin CreditAttribution: emclaughlin commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 3: dblog-breadcrumb-2267053-3.patch, failed testing.

sidharthap’s picture

sidharthap
Primary language English
CreditAttribution: sidharthap commented
FileSize
dblog-breadcrumb-2267053-6.patch1.71 KB

changing the paths in test file.

sidharthap’s picture

sidharthap
Primary language English
CreditAttribution: sidharthap commented
Component: syslog.module » dblog.module
Status: Needs work » Needs review
FileSize
dblog-breadcrumb-2267053-7.patch1.71 KB

oops .........

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Nice!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

We should be asserting the breadcrumb is now as expected

iMiksu’s picture

iMiksu
Location Finland
CreditAttribution: iMiksu commented
Issue tags: +drupalcampfi

Three guys from our code sprint checking on this.

-- Miksu

iMiksu’s picture

iMiksu
Location Finland
CreditAttribution: iMiksu commented
Status: Needs work » Needs review
FileSize
dblog-breadcrumb-2267053-11.patch2.44 KB

Here's same patch as in comment 7 with verifying the breadcrumb.

The test approach is similar as in \Drupal\node\Tests\NodeTitleTest\testNodeTitle, but I wasn't able to make this fail when not applying those fixes above.

Nikolay Shapovalov’s picture

Nikolay Shapovalov
Primary language Russian
CreditAttribution: Nikolay Shapovalov commented

It's good for me. Nice patch. Thank you.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
  1. +++ b/core/modules/dblog/lib/Drupal/dblog/Tests/DbLogTest.php
@@ -186,11 +187,23 @@ private function verifyReports($response = 200) {
+  private function verifyBreadcrumbs() {

    let's mark it as protected ... even there is no technical reason. 

  2. +++ b/core/modules/dblog/lib/Drupal/dblog/Tests/DbLogTest.php
@@ -186,11 +187,23 @@ private function verifyReports($response = 200) {
+    $wid = db_query('SELECT MIN(wid) FROM {watchdog}')->fetchField();
+    $this->drupalGet('admin/reports/dblog/event/' . $wid);
+    $xpath = '//nav[@class="breadcrumb"]/ol/li[last()]/a';

    Can we pass along the $wid directly somehow?

Daniel Norton’s picture

Daniel Norton CreditAttribution: Daniel Norton commented
Assigned: Unassigned » Daniel Norton

Reviewing

Daniel Norton’s picture

Daniel Norton CreditAttribution: Daniel Norton commented
FileSize
dblog-breadcrumb-2267053-15.patch2.39 KB

Reroll for PSR-4 only

Daniel Norton’s picture

Daniel Norton CreditAttribution: Daniel Norton commented
Issue tags: -Needs tests, -drupalcampfi
Daniel Norton’s picture

Daniel Norton CreditAttribution: Daniel Norton commented

Re comment, #13, those are good suggestions, but seem to me to be outside of the scope of this particular issue. The newly suggested issue applies to several other places in that same source file and the current patch simply duplicates those other instances.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

I tested this patch in SimpleTest against a non-patched D8 core. This patch is showing the correct breadcrumbs path when viewing a dblog event.

Correct breadcrumbs with patch: Home-> Administration -> Reports -> Recent log messages

Incorrect without patch: Home -> Administration -> Reports

Daniel Norton’s picture

Daniel Norton CreditAttribution: Daniel Norton commented
Assigned: Daniel Norton » Unassigned
Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs review » Reviewed & tested by the community

Verified the tests again (simpletest and phpunit) and marking this RTBC. Works in UI.

Recommended commit message: "Issue #2267053 by sidharthap, Daniel Norton, iMiksu, emclaughlin | joachim: Fixed breadcrumb is wrong on single log event pages."

alexpott’s picture

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

Committed c81a6ef and pushed to 8.x. Thanks!

  • Commit c81a6ef on 8.x by alexpott: 
    Issue #2267053 by sidharthap, Daniel Norton, iMiksu, emclaughlin |...

Status: Fixed » Closed (fixed)

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