Add helpful exception message in CustomAccessCheck

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).

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);
  }

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

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

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 :)

Yeah, that makes sense to me. Corrected :)

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

Seems like unrelated

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

Re-rolled the patch against Drupal 8.6.x-dev.

Also replaced the static object creation in the test class with mocking and added some comments to the class so that it was clear what was going on.

Uploading a new version of the patch file with the file suffix of ".patch" in order to trigger the Drupal unit testing bot.

@philipnorton42 - patches just want a .patch extension, not .patch.txt, otherwise the test bots don't pick it up. (EDIT: snap, we both posted in the same minute.)

Can you upload a patch without the .txt extension, increment the comment number in the patch name, and set it back to needs-review? Then the test-bots will do their thing.

Also, it would be good to provide an interdiff between this and the earlier patch makes it easy to see how it is progressing. So we're looking for an interdiff-2405241-17-26.txt, say.

Review of patch 25:

1. The new exception sounds like it's going to be more useful, I like it.
2. + $controllerResolver = $this->getMockBuilder(\Drupal\Core\DependencyInjection\ClassResolverInterface::class)->getMock();
   We already have a use \Drupal\Core\DependencyInjection\ClassResolverInterface;, so do we need to pass the fully-qualified name here?
3. + $this->controllerResolver = $this->getMockBuilder(\Drupal\Core\Controller\ControllerResolver::class)
   We already have a use \Drupal\Core\Controller\ControllerResolver;, so do we need to pass the fully-qualified name here?
4. +use Symfony\Bridge\PsrHttpMessage\Factory\DiactorosFactory;
   Where's this being used? I don't see it being used by any of the new code added by this patch.
5. +use Symfony\Component\DependencyInjection\ContainerBuilder;
   Where's this being used? I don't see it being used by any of the new code added by this patch.
6. NonExistantController::nonexistantmethod
   This is a real nitpick, but shouldn't this be NonExistentController::nonExistentMethod - spelling "existent", and camel-case for method name.

7. +    // Set that we are to expect an exception
   +    $this->setExpectedException(\BadMethodCallException::class);

   The comment is a bit redundant, we can understand this from the method name.

The test only checks what exception class we get. Should we check the error message formatting too?

Thanks very much for the guidance Andrew, really appreciate it!

I'd spotted the .patch thing literally minutes before you commented on it, but I've sorted that as well.

Potentially. Using UndefinedFunctionFatalErrorHandler::handleError looks like a nice approach, but we would need to move the symfony/debug library from require-dev into require in the composer.json file. Would that have implications in other parts of Drupal?

I can see only one instance of this component actually being used in Drupal and that is in the testing class EntityReferenceSettingsTest.

credited @dawehner and @andrewmacpherson for reviews.

Committed 7c1de1e and pushed to 8.6.x. Thanks!

diff --git a/core/tests/Drupal/Tests/Core/Access/CustomAccessCheckTest.php b/core/tests/Drupal/Tests/Core/Access/CustomAccessCheckTest.php
index 53c05d65be..aeb7f791bb 100644
--- a/core/tests/Drupal/Tests/Core/Access/CustomAccessCheckTest.php
+++ b/core/tests/Drupal/Tests/Core/Access/CustomAccessCheckTest.php
@@ -7,7 +7,6 @@
 
 namespace Drupal\Tests\Core\Access;
 
-use Drupal\Core\Access\AccessArgumentsResolverFactoryInterface;
 use Drupal\Core\Access\AccessResult;
 use Drupal\Core\Access\CustomAccessCheck;
 use Drupal\Core\Controller\ControllerResolver;
@@ -112,8 +111,8 @@ public function testAccess() {
     $this->assertEquals(AccessResult::allowed(), $this->accessChecker->access($route, $route_match, $account));
   }
 
-  /*
-   * Test the access method exception for invalid access callbacks.
+  /**
+   * Tests the access method exception for invalid access callbacks.
    */
   public function testAccessException() {
     // Create two mocks for the ControllerResolver constructor.
@@ -130,15 +129,16 @@ public function testAccessException() {
     $this->accessChecker = new CustomAccessCheck($this->controllerResolver, $this->argumentsResolverFactory);
 
     // Add a route with a _custom_access route that doesn't exist.
-    $route = new Route('/test-route', array(), array('_custom_access' => '\Drupal\Tests\Core\Access\NonExistantController::nonExistantMethod'));
+    $route = new Route('/test-route', [], ['_custom_access' => '\Drupal\Tests\Core\Access\NonExistentController::nonExistentMethod']);
     $route_match = $this->getMock(RouteMatchInterface::class);
     $account = $this->getMock(AccountInterface::class);
 
-    $this->setExpectedException(\BadMethodCallException::class, 'The "\Drupal\Tests\Core\Access\NonExistantController::nonExistantMethod" method is not callable as a _custom_access callback in route "/test-route"');
+    $this->setExpectedException(\BadMethodCallException::class, 'The "\Drupal\Tests\Core\Access\NonExistentController::nonExistentMethod" method is not callable as a _custom_access callback in route "/test-route"');
 
     // Run the access check.
     $this->accessChecker->access($route, $route_match, $account);
   }
+
 }
 
 class TestController {

Coding standards fixed on commit.