Updated: Comment #N

Problem/Motivation

Like many other places in core shortcut_preprocess_page() relies on menu_get_item() to work, so on route only pages
the add/remove link right beside the page title does not work.

Proposed resolution

Remaining tasks

User interface changes

API changes

#2078583: menu callbacks removed from hook_menu do not display any menus

Comments

dawehner’s picture

Status: Active » Needs review
StatusFileSize
new1.36 KB

Here is a patch.

pwolanin’s picture

Can you list some example pages where this can be verified? Looking at a local install it seems like all are working in Severn and none in Bartik.

dawehner’s picture

StatusFileSize
new4.71 KB
new3.35 KB

Yes this link is not shown by bartik, so on route only pages this bug will never be visible.

To reproduce the bug, just set seven as default theme and go to the usual router_test/test2 page.

Here is a test for that.

dawehner’s picture

Issue summary: View changes

Updated issue summary.

mr.baileys’s picture

Issue summary: View changes
StatusFileSize
new3.36 KB
  • Re-rolled to keep up with head.
  • Removed the "p"-prefix from other shortcut tests (debugging artifact).
  • Changed assertFalse(!empty()) to assertTrue(empty()) for legibility.
dawehner’s picture

Oh I like those changes! Sadly I can't really RTBC it.

tim.plunkett’s picture

+++ b/core/modules/shortcut/shortcut.module
@@ -373,7 +374,21 @@ function shortcut_preprocess_page(&$variables) {
+    // @todo What should be done on a 404/403 page?
+    $item['access'] = TRUE;

This seems like a problem still.

mr.baileys’s picture

Current behaviour is the same as it is in Drupal 7.x: the "Add to shortcuts" icon/link is shown on access denied, but not on page not found.

It does not make sense to display the icon/link on a 403 page, but this patch does not alter the current behaviour, so I think it is best to tackle this in a separate issue (especially since determining whether or not the page is a 403/404 in shortcut_preprocess_page() seems non-trivial according to #1969270: 403/404 pages: drupal_get_http_header('Status') returns no status code at all)?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
berdir’s picture

Priority: Major » Critical
Issue tags: +beta blocker

Really marking as critical :)

berdir’s picture

+++ b/core/modules/shortcut/shortcut.module
@@ -373,7 +374,21 @@ function shortcut_preprocess_page(&$variables) {
+  if ($request->attributes->has('_legacy')) {
+    // @todo Remove once the old router system got removed.
+    $item = menu_get_item();
+  }

Do we really still need this?

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I can't see _legacy being set anywhere so it does look superflous.

mr.baileys’s picture

Status: Needs work » Needs review
StatusFileSize
new846 bytes
new3.18 KB

"_legacy" was removed in #2106709: Remove legacy router backward compatibility layer

Rerolled without _legacy-support.

berdir’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, thought so, back to RTBC then.

catch’s picture

Status: Reviewed & tested by the community » Needs review
  1. +++ b/core/modules/shortcut/shortcut.module
    @@ -388,7 +389,17 @@ function shortcut_preprocess_page(&$variables) {
    +  if ($route = $request->attributes->get(RouteObjectInterface::ROUTE_NAME)) {
    

    /me shudders but already did that on #2095959: Remove instances of menu_get_object('node'), not the fault of this patch.

  2. +++ b/core/modules/shortcut/shortcut.module
    @@ -388,7 +389,17 @@ function shortcut_preprocess_page(&$variables) {
    +  if (shortcut_set_edit_access() && ($item) && $item['access']) {
    

    Very nitpicky but why not just !empty($item['access'])?

mr.baileys’s picture

StatusFileSize
new3.18 KB
new586 bytes

1. will be addressed as part of #2124749: [meta] Stop using $request->attributes->get(MAGIC_KEY) as a public API
2. Fixed

new patch attached.

berdir’s picture

Status: Needs review » Reviewed & tested by the community

Yes, looks better, RTBC unless testbot disagrees (he better not! ;))

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 15: 2081963-15-shortcut-route-only-pages.patch, failed testing.

alexpott’s picture

The last submitted patch, 15: 2081963-15-shortcut-route-only-pages.patch, failed testing.

catch’s picture

mr.baileys’s picture

Status: Needs work » Reviewed & tested by the community

Passed testing, so back to RTBC

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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