PathBasedBreadcrumbBuilder should catch all exceptions from routing

I could reproduce this with cURL from the command line:

curl -v --request DELETE --header 'Accept:' --cookie 'SESSf722138df952ff0f3000137055cb16f4=n7gN43952qYn7aPTEPXzMp1n-kUWE05hKH0wQghkA2E' http://drupal-8.localhost/entity/node/2
* Adding handle: conn: 0x1462a80
* Adding handle: send: 0
* Adding handle: recv: 0
* Curl_addHandleToPipeline: length: 1
* - Conn 0 (0x1462a80) send_pipe: 1, recv_pipe: 0
* About to connect() to drupal-8.localhost port 80 (#0)
*   Trying 127.0.0.1...
* Connected to drupal-8.localhost (127.0.0.1) port 80 (#0)
> DELETE /entity/node/2 HTTP/1.1
> User-Agent: curl/7.32.0
> Host: drupal-8.localhost
> Cookie: SESSf722138df952ff0f3000137055cb16f4=n7gN43952qYn7aPTEPXzMp1n-kUWE05hKH0wQghkA2E
> 
< HTTP/1.1 200 OK
< Date: Sat, 25 Jan 2014 21:52:05 GMT
* Server Apache/2.4.6 (Ubuntu) is not blacklisted
< Server: Apache/2.4.6 (Ubuntu)
< X-Powered-By: PHP/5.5.3-1ubuntu2.1
< Vary: Accept-Encoding
< Content-Length: 1215
< Content-Type: text/html

Unfortunately I could not reproduce it with PHP in DeleteTest.php by setting "Accept:" in the cURL options, but I tested with hal_json there, maybe it triggers with json.

After excessive debugging I found PathBasedBreadcrumbBuilder to be guilty. When the error page is rendered for "/entity/node/1" it tries to match the path to "/entity/node". But the route for that path is restricted to POST requests and is filtered out, which triggers a MethodNotAllowedException. That exception is not caught and breaks further page execution.

klausi opened a new pull request for this issue.

After just reviewing the patch and some work this ended with a lot of changes in the actual change + test the full class.

Great, you covered the whole class! I was just too lazy to do that, but this is of course amazing.

1. +++ b/core/modules/system/tests/Drupal/system/Tests/Breadcrumbs/PathBasedBreadcrumbBuilderTest.php
   @@ -0,0 +1,472 @@
   + * Tests machine name controller's transliteration functionality.
   

   Wrong doc sentence, yes this was my fault.

2. +++ b/core/modules/system/tests/Drupal/system/Tests/Breadcrumbs/PathBasedBreadcrumbBuilderTest.php
   @@ -0,0 +1,472 @@
   +   * The title resolver.
   

   Should probably also describe it as "mocked"?

3. +++ b/core/modules/system/tests/Drupal/system/Tests/Breadcrumbs/PathBasedBreadcrumbBuilderTest.php
   @@ -0,0 +1,472 @@
   +   * The current user.
   

   same here.

4. +++ b/core/modules/system/tests/Drupal/system/Tests/Breadcrumbs/PathBasedBreadcrumbBuilderTest.php
   @@ -0,0 +1,472 @@
   +  /**
   +   * Tests that MethodNotAllowedExceptions are caught.
   +   *
   +   * @covers ::build()
   +   * @covers ::getRequestForPath()
   +   */
   +  public function testBuildWithPathMatchingExceptions() {
   

   Since that test case is identical to the 2 other exceptions we should only write this test code once and use a data provider for the different execption types.

5. +++ b/core/modules/system/tests/Drupal/system/Tests/Breadcrumbs/PathBasedBreadcrumbBuilderTest.php
   @@ -0,0 +1,472 @@
   +   * Tests the breadcrumb for an user path.
   

   Should be "a user path". You only use "an" if the word is pronounced with a vocal first.

And we still need to decide if we just want to catch \Exception or a useful parent class instead of all those exception types.

Should be "a user path". You only use "an" if the word is pronounced with a vocal first.

Machines can't execute this logic :p

And we still need to decide if we just want to catch \Exception or a useful parent class instead of all those exception types.

I kind of like your approach given that we still allow to have a lot of the unrelated exceptions going through, which is a good DX in general.

Since that test case is identical to the 2 other exceptions we should only write this test code once and use a data provider for the different execption types.

Good idea!

The test logic looks fine now and makes sense to me. Just some minor remaining nitpicks:

1. +++ b/core/modules/system/lib/Drupal/system/PathBasedBreadcrumbBuilder.php
   @@ -198,6 +197,18 @@ protected function getRequestForPath($path, array $exclude) {
   +   * @return \Drupal\Core\Session\AccountInterface
   

   @return comment missing.

2. +++ b/core/modules/system/tests/Drupal/system/Tests/Breadcrumbs/PathBasedBreadcrumbBuilderTest.php
   @@ -0,0 +1,437 @@
   +   * Tests that MethodNotAllowedExceptions are caught.
   

   Comment is out of date now, sine arbitrary exceptions are tested.

3. +++ b/core/modules/system/tests/Drupal/system/Tests/Breadcrumbs/PathBasedBreadcrumbBuilderTest.php
   @@ -0,0 +1,437 @@
   +  /**
   +   * Provides exception types for testBuildWithException.
   +   *
   +   * @return array
   +   */
   

   Missing doc comment for @return, missing @see doc to point to the test method that uses this data provider.

4. +++ b/core/modules/system/tests/Drupal/system/Tests/Breadcrumbs/PathBasedBreadcrumbBuilderTest.php
   @@ -0,0 +1,437 @@
   +class TestPathBasedBreadcrumbBuilder extends PathBasedBreadcrumbBuilder {
   

   This class should have a doc block what it is used for, i.e. only for testing purposes. I guess it is ok to not document the methods of the class, since they are only used for testing.

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

Great, just 6 clicks.

+++ b/core/modules/system/lib/Drupal/system/PathBasedBreadcrumbBuilder.php
@@ -198,6 +197,19 @@ protected function getRequestForPath($path, array $exclude) {
+
+  /**
+   * Gets the current user.
+   *
+   * @return \Drupal\Core\Session\AccountInterface
+   *   The current user object.
+   */
+  protected function getCurrentUser() {
+    return \Drupal::currentUser();
   }
 

Is there any reason why we don't use proper constructor DI?

Once #9 is resolved I think this is RTBC, but I still don't grok how this situation is coming up in the first place. Protecting against it is nearly free though, so that's fine.

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

Implemented constructor injection for the current user, interdiff: https://github.com/klausi/drupal/commit/e3b675a87ce4a6ecd07fa8b7512962c3...

Note that we still need the TestPathBasedBreadcrumbBuilder class for unit testing, because of the ->t() and ->l() methods. This class really has a bazillion of dependencies.

Great, thank you!

11: 2180425.patch queued for re-testing.

Test fail caused by a PHP segmentation fault on the testbot:

Update dependency ordering 5 passes, 0 fails, 0 exceptions
Segmentation fault
FATAL Drupal\toolbar\Tests\ToolbarAdminMenuTest: test runner returned a non-zero error code (139).
7.x update hooks 15 passes, 0 fails, 0 exceptions

So I just pretend that I have not seen that, retesting now.

It is green again.

11: 2180425.patch queued for re-testing.

Tests passing as expected, back to RTBC.

This is a strange one, but the fix is reasonable and the extra coverage looks great. Committed/pushed to 8.x, thanks!