Problem/Motivation

When you write a routing.yml file with a _custom_access check which points to a non existing method/class you get the following message:

InvalidArgumentException: The controller for URI "" is not callable. in Drupal\Core\Controller\ControllerResolver->getControllerFromDefinition() (line 81 of core/lib/Drupal/Core/Controller/ControllerResolver.php).
Drupal\Core\Access\CustomAccessCheck->access(Object, Object, Object)
call_user_func_array(Array, Array)
Drupal\Core\Access\AccessManager->performCheck('access_check.custom', Object)
Drupal\Core\Access\AccessManager->checkAll(Array, Object)
Drupal\Core\Access\AccessManager->check(Object, Object, Object, )
Drupal\Core\Access\AccessManager->checkRequest(Object, Object)
Drupal\Core\Routing\AccessAwareRouter->checkAccess(Object)
Drupal\Core\Routing\AccessAwareRo

Its not helpful

Proposed resolution

CustomAccessCheck should catch the InvalidArgumentException and convert it into a new exception
which explains which route / custom access check exactly is broken

Remaining tasks

User interface changes

API changes

Comments

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

Issue tags: +php-novice

This should be a php-novice task.

philipnorton42’s picture

Assigned: Unassigned » philipnorton42
philipnorton42’s picture

I have added a \BadMethodCallException here, which is still part of the SPL library and seems to make sense in this context. This now produces a the following message when this situation is encountered:

BadMethodCallException: The "\Drupal\mmodule\Controller\MyModuleController::accesscallback" method is not callable as a _custom_access callback in route "/mymodule" in Drupal\Core\Access\CustomAccessCheck->access() (line 67 of /core/lib/Drupal/Core/Access/CustomAccessCheck.php).

philipnorton42’s picture

Assigned: philipnorton42 » Unassigned
Status: Active » Needs review
philipnorton42’s picture

I did try to create a test for this particular exception being thrown, but due to the way in which the objects are mocked the exceptions are not produced when running under the testing environment. As a result I'm not sure this can be tested correctly in this instance.

For what it's worth here is the code I created for this:

  /*
   * Test that the access method throws an exception for invalid access callbacks.
   *
   * @expectedException \BadMethodCallException
   */
  public function testAccessException() {
    $route = new Route('/test-route', array(), array('_custom_access' => '\Drupal\Tests\Core\Access\NonExistantController::nonexistantmethod'));
    $route_match = $this->getMock('Drupal\Core\Routing\RouteMatchInterface');
    $account = $this->getMock('Drupal\Core\Session\AccountInterface');

    $this->accessChecker->access($route, $route_match, $account);
  }
dawehner’s picture

I'm wondering whether we should have a test for that.

philipnorton42’s picture

Just realised that the original patch didn't follow Drupal coding standards. So I'm applying those standards in an updated patch :)

dawehner’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

A small test would still be nice

philipnorton42’s picture

Not a problem. I will put something together soon and post it back :)

philipnorton42’s picture

Assigned: Unassigned » philipnorton42
philipnorton42’s picture

Assigned: philipnorton42 » Unassigned
Status: Needs work » Needs review
FileSize
3.19 KB

Ok, so I've had a go at writing the test for this. In order to properly simulate the conditions of the test I had to create the CustomAccessCheck object (i.e. instead of Mocking it). There is therefore quite a bit of object creation, but at least I was able to Mock some of the objects used in this process.

Attached is the patch containing the code change and the passing test :)

dawehner’s picture

+++ b/core/tests/Drupal/Tests/Core/Access/CustomAccessCheckTest.php
@@ -106,6 +110,28 @@ public function testAccess() {
+
+    $this->setExpectedException('\BadMethodCallException');
+
...
+    $this->accessChecker->access($route, $route_match, $account);

A small suggestion: Move the setExpectedException right before the access call. This makes the overall test easier to read.

philipnorton42’s picture

Yeah, that makes sense to me. Corrected :)

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Cool, thank you!

jibran’s picture

Version: 8.2.x-dev » 8.3.x-dev
FileSize
2.42 KB
3.26 KB

Fixed some cs issues. Moving it to 8.3.x so that it can be back ported after stable release of 8.2.x

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 17: add_helpful_exception-2405241-17.patch, failed testing.

jibran’s picture

Seems like unrelated

testDateRangeField
fail: [Other] Line 204 of core/modules/datetime_range/src/Tests/DateRangeFieldTest.php:
Separator not found on page

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.