in menu.inc:

function menu_item_route_access(Route $route, $href, &$map) {
  $request = Request::create('/' . $href);
  $request->attributes->set('system_path', $href);
  // Attempt to match this path to provide a fully built request to the
  // access checker.
  try {
    $request->attributes->add(Drupal::service('router.dynamic')->matchRequest($request));
  }
  catch (NotFoundHttpException $e) {
    return FALSE;
  }

Results in an uncaught exception if you pass in a bad $href that isn't a real system path. On the RequestMatcherInterface that's documented as the exception that will be thrown.

CommentFileSizeAuthor
#27 drupal-2045453-27.patch2.94 KBpwolanin
#27 drupal-2045453-26-27.increment.txt1.09 KBpwolanin
#26 drupal-2045453-26.patch3 KBdawehner
#26 interdiff.txt1.43 KBdawehner
#21 menu-2045453-21.patch2.59 KBtim.plunkett
#21 interdiff.txt2.26 KBtim.plunkett
#7 2045453-7.patch1.87 KBpwolanin
#7 2045453-1-7.increment.txt1.1 KBpwolanin
#1 2045453-1.patch781 bytespwolanin
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Active » Needs review
FileSize
2045453-1.patch781 bytes
Crell’s picture

Crell CreditAttribution: Crell commented
Status: Needs review » Reviewed & tested by the community

This seems reasonable to me.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Sorry, but this needs tests.
I'm curious to see the circumstances when this happens.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

@tim,plunkett - I was seeing this when having issues with the system paths generated by the local task plugins. I was getting an uncaught exception, instead of e.g. a missing tab.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Title: function menu_item_route_access() does not catch a ResourceNotFoundException as expeted from RequestMatcherInterface » function menu_item_route_access() does not catch a ResourceNotFoundException as expected from RequestMatcherInterface

Depending on the circumstances, it might make more sense for the calling code to catch the exception.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Issue tags: +MenuSystemRevamp, +WSCCI

Tagging.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs work » Needs review
FileSize
2045453-1-7.increment.txt1.1 KB
2045453-7.patch1.87 KB

Since the function is currently documented as only returning TRUE or FALSE, we should make such a change in a bigger scope.

This adds a small test case to make sure there's not an exception.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
+++ b/core/includes/menu.incundefined
@@ -975,6 +976,9 @@ function menu_item_route_access(Route $route, $href, &$map) {
+  catch (ResourceNotFoundException $e) {
+    return FALSE;
+  }

Mh, is it okay to throw away the exception message?

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

@Dawehner - I don't think there is any message? If there was what would we do with it?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

For #2044969: Break the old router for contrib it would have made sense to provide a help message for the developer.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Maybe we could at least write a watchdog entry?
Well, having an access denied is kind of a common event.

dawehner’s picture

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

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Needs review

Access denied isn't that common if you're developing, I think a watchdog would be worth adding.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Well, the problem is that this is also triggered when you just have a menu with a link. This would create multiple watchdog entries on every request containing a menu.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

AFAIK this isn't triggered at all in HEAD yet...

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

So you agree that we should add a watchdog entry?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

I wish I had called it _menu_item_route_access().
This wasn't intended to be called willy-nilly.

The test is pretty bogus, since it purposefully calls it with a known broken value.
It doesn't illustrate why that would ever happen in reality.

I personally think this exception should be either uncaught, or at least put into watchdog. Definitely not okay to silently eat the exception.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

@peter
When did you ran into the problem with the exception?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Title: function menu_item_route_access() does not catch a ResourceNotFoundException as expected from RequestMatcherInterface » menu_item_route_access() does not catch ResourceNotFoundException

This is breaking my screen

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

@dawehner - I saw this when developing and the language prefix wasn't stripped from the path. so, I'd agree this other exception would often be a developer error.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
interdiff.txt2.26 KB
menu-2045453-21.patch2.59 KB
pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

@tim.plunkett - do we really want to re-throw rather than logging it? At that point let's just close this issue.

This function is kind of a hack that we should mark deprecated by release time anyhow.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

If this really just happens when you develop then throwing an exception is the best thing you can do in my perspective. Fail as early as possible is great, as you have still the context why things are broken.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

The exception that is thrown gives you no helpful information, so I think catching it and throwing a better one is our best bet.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Issue tags: -Needs tests

This improved exception helped me a bit with #1987762: Convert node_add_page() to a new style controller.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
interdiff.txt1.43 KB
drupal-2045453-26.patch3 KB

What about throwing a better exception in the first place.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
drupal-2045453-26-27.increment.txt1.09 KB
drupal-2045453-27.patch2.94 KB

We should annotate the function, and we no longer need the use statements in menu.inc.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Good catch! It seems fine that this issue is RTBC now.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs review » Reviewed & tested by the community

Oh, I didn't know that we threw it, thought it was upstream! That's much better. Thanks @dawehner and @pwolanin

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Much better!

Committed/pushed to 8.x. Thanks1

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