Problem/Motivation

Per @Dries manually testing at #67893-37: add way to filter messages on severity, not just type, the breadcrumb is wrong when viewing dblog event details. It doesn't include a link to the parent path, since the menu structure of the dblog paths is inconsistent and the details pages currently live at admin/reports/event/%. So admin/reports/dblog (the parent from the UI) isn't actually the parent to the menu system, and the breadcrumbs get confused.

Steps to reproduce

  1. Install Drupal 7.
  2. Enable dblog module.
  3. Visit /admin/reports/dblog
  4. Click on the details of an event.
  5. The breadcrumbs can get you back to "Reports" but not "Recent log messages".

Proposed resolution

Change the path of the dblog details page to admin/reports/dblog/% so it can be a regular menu item and the breadcrumbs will All Just Work(tm).

Remaining tasks

  1. Decide on BC implications and if we need a redirect from admin/reports/event/% to admin/reports/dblog/%
  2. Decide if we need/want to add test coverage of dblog breadcrumbs in here.
  3. Reviews / refinements.
  4. RTBC.
  5. Commit.

User interface changes

  1. Path to dblog detail pages changes from admin/reports/event/% to admin/reports/dblog/%
  2. Breadcrumbs now contain the parent page (the 'Recent log messages' page at admin/reports/dblog):

Before

D7 dblog details page before the patch applied

After

D7 dblog details page after the patch applied, shows full breadcrumbs

API changes

None.

Data model changes

None.

Release notes snippet

TBD.

Original report by @dww

as reported by Dries at http://drupal.org/node/67893#comment-233462 (comment #37) the breadcrumb is wrong when viewing dblog event details. i originally thought this was due to the split from watchdog into dblog and syslog, but that's not the case. really, it's a problem of how the menu item is being defined (MENU_CALLBACK items don't get breadcrumbs, and this menu item really isn't just a menu callback), and the path we've been using for viewing event details (/admin/logs/event/X). so, there are 3 possible solutions:

  1. just remove the MENU_CALLBACK from the menu item. this leaves the breadcrumb as: Home > Administer > Logs > Details. this is really a trivial fix, but the resulting breadcrumb is arguably not quite right, since the only way to navigate to the per-event item is via admin/logs/dblog ("Recent log entries").
  2. remove the MENU_CALLBACK, and keep the same path (/admin/logs/event/X) but manually set the breadcrumbs in dblog_event() to be more accurate. menu_set_location() is just a no-op in 6.x at this point, so the only way to get this working at all is via drupal_set_breadcrumb(). in other circles, i've come to believe that menu_set_location() is evil, anyway, so i'm happy to just use drupal_set_breadcrumb(). however, it still seems a little silly to use a different path and then manually force the breadcrumbs back into line.
  3. change the path to match the actual navigation structure (/admin/logs/dblog/X), and remove MENU_CALLBACK, then the breadcrumbs work automatically.

personally, i'd prefer #3, but i'm providing patches for each approach so the community and core maintainers can decide which one they think is best (and, since i was stuck on a plane all day, so i had no good way to get input from other people, and got all 3 working, anyway). ;)

CommentFileSizeAuthor
#29 After--patch--pic.png39.48 KBvikashsoni
#29 Before--patch--pic.png41.88 KBvikashsoni
#27 after_139015.png12.35 KBguilhermevp
#27 before_139015.png11.36 KBguilhermevp
#25 139015-25.after_.png55.79 KBdww
#25 139015-25.before.png55.93 KBdww
#25 139015.23_25.interdiff.txt911 bytesdww
#25 139015-25.patch4.03 KBdww
#23 139015-23.patch4.04 KBdww
#21 dblog-event-breadcrumb-139015_21.patch3.39 KBpancho
#17 dblog-event-breadcrumb-139015.patch2.66 KBdavyvdb
#3 dblog_breadcrumb_path.patch.txt1007 bytesdww
#2 dblog_breadcrumb_set_bread.patch.txt1.04 KBdww
#1 dblog_breadcrumb_not_callback.patch.txt341 bytesdww

Comments

dww’s picture

dww
we/he/they
commented
StatusFileSize
new dblog_breadcrumb_not_callback.patch.txt341 bytes

this is the most trivial fix. just remove "MENU_CALLBACK" from the menu definition and it mostly works. however, breadcrumbs become:
Home > Administer > Logs > Details

dww’s picture

dww
we/he/they
commented
StatusFileSize
new dblog_breadcrumb_set_bread.patch.txt1.04 KB

leave the path for the menu item at /admin/logs/event/X, but manually set more accurate breadcrumbs with drupal_set_breadcrumb(). breadcrumbs are now:
Home > Administer > Logs > Recent log entries > Details

dww’s picture

dww
we/he/they
commented
Status: Active » Needs review
StatusFileSize
new dblog_breadcrumb_path.patch.txt1007 bytes

IMHO, best approach: remove MENU_CALLBACK, but change the path for the dblog event detail page from /admin/logs/event/X to /admin/logs/dblog/X. no manual setting of breadcrumbs, but they're still the most accurate possible:
Home > Administer > Logs > Recent log entries > Details

AmrMostafa’s picture

AmrMostafa commented

breadcrumbs are great with #3, but now 'Details' appear in the menu

dries’s picture

dries commented

This looks a bit awkward. The 'Details' is shown in the breadcrumb and in the title .. is this is menu system bug maybe?

AmrMostafa’s picture

AmrMostafa commented
Status: Needs review » Needs work

I'm do not know the menu system well enough, but I've done the following MENU_CALLBACK test on both HEAD and 5.x and the result came different. In 5.x the breadcrumbs indicates the item parents. While in 6.x, the parents are disregarded.

Regarding the title, can we modify it so instead of 'Details' it would be 'Watchdog entry details' ?

dww’s picture

dww
we/he/they
commented
Status: Needs work » Needs review

@Dries: yes, in D6 menu land, breadcumbs always go all the way up to the current page. look anywhere in D6, and you'll see the same thing. if we don't like that, it's a menu system bug, and we should file a separate issue for that. i suspect it's a trivial 1 line change to the right function, but i'm sure chx could fix it even faster than i could. ;)

@alienbrain: re: the title -- that's just the title of the page itself. if we want a different title, we could change it, but that's sort of orthogonal to what this issue is trying to solve.

therefore, i think this (#3, in particular) still needs review.

thanks,
-derek

AmrMostafa’s picture

AmrMostafa commented

I think the breadcrumb is fine, except that now 'Details' appears on the menu too. I don't know of a solution or what could be the right menu type/flags to use in order to fix this.

Just thinking out loudly, this patch removes MENU_CALLBACK which defaults the item to MENU_NORMAL_ITEM, hence it appears in the breadcrumb and the menu. But I feel MENU_CALLBACK was the appropriate type here, and it's the menu system fault that MENU_CALLBACK type causes the breadcrumb to loose the parents causing the wrong "Home > Details" breadcrumb.

dries’s picture

dries commented

dww: thanks for pointing out that this is a menu system glitch. Can be fixed later.

I also noticed that 'Details' appears in the menu or on the admin overview page -- something is still wrong.

dries’s picture

dries commented

alienbrain: I think you might be right. However, I just checked a Drupal 5 install, and it also has a breadcrumb problem with the log pages. So maybe the behavior of MENU_CALLBACK is consistent with the old Drupal 5 behavior -- but admittedly b0rked in this use case. Let's call the Menu Man?

dww’s picture

dww
we/he/they
commented

see http://drupal.org/node/139303 about D6 breadcrumbs including the current page.

chx’s picture

chx commented

Yes, breadcrumbs are broken and I will not fix this next week but as said in the other issue i do not want to meddle with menu builder while pwolanin is working on it. And I do not readily have the time anyways.

chx’s picture

chx commented

On another note, #3 is almost the solution but removing MENU_CALLBACK is not needed. Of course, if you do not remove it now then it'll broken but that's just because menu.inc is broken. Not a biggie.

dries’s picture

dries commented

chx: so you advice to put this patch on hold for a couple days (weeks)?

keith.smith’s picture

keith.smith commented
Status: Needs review » Needs work

Unfortunately patch no longer applies cleanly.

# patch -p0 < dblog_breadcrumb_path.patch.txt
patching file modules/dblog/dblog.module
Hunk #1 FAILED at 67.
Hunk #2 succeeded at 165 (offset -15 lines).
1 out of 2 hunks FAILED -- saving rejects to file modules/dblog/dblog.module.rej

AmrMostafa’s picture

AmrMostafa commented
Version: 6.x-dev » 7.x-dev

The issue still applies to HEAD, and patch at #3 doesn't fix it.

davyvdb’s picture

davyvdb commented
Status: Needs work » Needs review
StatusFileSize
new dblog-event-breadcrumb-139015.patch2.66 KB

Status: Needs review » Needs work

The last submitted patch failed testing.

davyvdb’s picture

davyvdb commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

pancho’s picture

pancho
Location UTC+2 🇪🇺 EU
commented
Component: watchdog.module » dblog.module
Assigned: dww » Unassigned
Status: Needs work » Needs review
StatusFileSize
new dblog-event-breadcrumb-139015_21.patch3.39 KB

Rerolled patch against HEAD.
Also, reordered the entries in dblog_menu() for more clarity about what belongs together.
Also, renamed page title from the inexpressive "Details" to "Event details" (However, I'm not sure whether this is still acceptble in 7.x)

Status: Needs review » Needs work

The last submitted patch, dblog-event-breadcrumb-139015_21.patch, failed testing.

dww’s picture

dww
we/he/they
commented
Status: Needs work » Needs review
Issue tags: +Bug Smash Initiative
StatusFileSize
new 139015-23.patch4.04 KB

Re-found this as part of my #BugSmash homework to triage the bug reports I opened. ;) This bug is still present in D7, so I rerolled this. Interdiff is sort of confused. I think I fixed the tests, but let's see. ;)

Status: Needs review » Needs work

The last submitted patch, 23: 139015-23.patch, failed testing. View results

dww’s picture

dww
we/he/they
commented
Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new 139015-25.patch4.03 KB
new 139015.23_25.interdiff.txt911 bytes
new 139015-25.before.png55.93 KB
new 139015-25.after_.png55.79 KB 
+++ b/modules/dblog/dblog.module
@@ -45,6 +45,14 @@ function dblog_menu() {
+    'title' => 'Event details',

@@ -61,13 +70,6 @@ function dblog_menu() {
-    'title' => 'Details',

This is what caused the test failure. I'm not sure we want/need to do that. So I'm reverting the page title change and the test now passes locally.

I'm wondering about the "BC" implications of changing the path for these pages, too. Are folks that wrote things to scrape these pages going to be sad?

Should we add a redirect from admin/reports/event/% to admin/reports/dblog/% ?

Meanwhile, summary update with before/after screenshots.

dww’s picture

dww
we/he/they
commented
Issue summary: View changes

Formatting updates to the summary, shrink the embedded screenshots, and properly link to the original bug report (where Dries was manually testing this). ;)

guilhermevp’s picture

guilhermevp commented
StatusFileSize
new before_139015.png11.36 KB
new after_139015.png12.35 KB

Patch works as intended. I'm also in favor of decision #3. Not moving to RTBC because I believe that at this point it should be a decision made by the maintainers. Just adding to the discussion.

Before:
1

After:
2

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Status: Needs review » Needs work
Issue tags: +Needs change record

Mmm. Good question. The admin/reports/event/% paths are transient, but I am kind-of wary that someone made their own overview and we don't want to break that. So we likely should implement the redirect.

E.g. https://www.drupal.org/project/views_watchdog - I haven't checked whether it includes (hardcoded) URLs to individual menu entries; I just imagine it does and otherwise there may be similar code around that we don't want to break.
(Edit - actually I'm wrong, views_watchdog doesn't have links to the detail page / doesn't want to use Core's detail page. So you can disagree. I'm still wary that in the current D7 install base, people are depending on it, though.)

So, needs:

  • that redirect
  • a code block in DBLogTestCase::verifyReports() to check that path (I am assuming it's easy because drupalGet() resolves redirects... right?)
  • a change record - even if it just says 2 sentences "URLs have changed from X to Y; the old ones still work and redirect"

I'm going to just go ahead and set Needs Work for this. Have added to #3216978: [meta] Priorities for 2021-12-01 release of Drupal 7 so there's, uhm, a non-zero chance this will get into the December release. (The maintainers are trying and hopefully succeeding to get a little extra assistance, by then.)

vikashsoni’s picture

vikashsoni commented
StatusFileSize
new Before--patch--pic.png41.88 KB
new After--patch--pic.png39.48 KB

Applied patch #25 working fine and looks good for me
For ref sharing screenshot...

Status: Needs work » Closed (outdated)

Automatically closed because Drupal 7 security and bugfix support has ended as of 5 January 2025. If the issue verifiably applies to later versions, please reopen with details and update the version.