\Drupal\Tests\system\Functional\Menu\AssertBreadcrumbTrait::assertBreadcrumbParts is vulnerable to false positives

Thanks for the patch. These are nitpicks of the comments that could be applied.

1. +++ b/core/modules/system/tests/src/Functional/Menu/BreadcrumbTest.php
   @@ -381,4 +382,44 @@ public function testBreadCrumbs() {
   +      // No trail should fail.
   ...
   +      // Incorrect trail should fail.
   

   Nitpick: This wording seems unclear. "No trail should fail" sounds to me like there is no trail that should fail. Perhaps a slight change like:

   If there is no trail, this assert should fail.

   Or

   If there is no trail, this should fail.

2. +++ b/core/modules/system/tests/src/Functional/Menu/BreadcrumbTest.php
   @@ -381,4 +382,44 @@ public function testBreadCrumbs() {
   +      // No trail should fail.
   ...
   +      $this->fail('Breadcrumb assertion should fail with empty trail.');
   +    }
   +    catch (\PHPUnit_Framework_ExpectationFailedException $e) {
   +      $this->assertTrue(TRUE, 'Breadcrumb assertion should fail with empty trail.');
   

   Nitpick: Since this is used twice and is used for comparison, consider pulling out string into a variable.

3. +++ b/core/modules/system/tests/src/Functional/Menu/BreadcrumbTest.php
   @@ -381,4 +382,44 @@ public function testBreadCrumbs() {
   +      $this->assertTrue(TRUE, 'Breadcrumb assertion should fail with empty trail.');
   ...
   +      $this->fail('Breadcrumb assertion should fail with incorrect trail.');
   +    }
   +    catch (\PHPUnit_Framework_ExpectationFailedException $e) {
   +      $this->assertTrue(TRUE, 'Breadcrumb assertion should fail with incorrect trail.');
   

   Nitpick: Since this text is used twice and is used for comparison, consider pulling out string into a variable.

4. +++ b/core/modules/system/tests/src/Functional/Menu/BreadcrumbTest.php
   @@ -381,4 +382,44 @@ public function testBreadCrumbs() {
   +      // Incorrect trail should fail.
   

   Nitpick: If the other phrasing is changed above, consider updating this one as well:

   If the trail is incorrect, this assert should fail.

   Or

   If the trail is incorrect, this should fail.

5. +++ b/core/modules/system/tests/src/Functional/Menu/BreadcrumbTest.php
   @@ -381,4 +382,44 @@ public function testBreadCrumbs() {
   +    // No trail should pass as there is no breadcrumb.
   

   Nitpick: Similar to previous suggestion, perhaps:

   If there is no trail, this should pass as there is no breadcrumb.

   Or, use a wording similar to the next comment:

   There is no breadcrumb, so testing without a trail should pass.

6. +++ b/core/modules/system/tests/src/Functional/Menu/BreadcrumbTest.php
   @@ -381,4 +382,44 @@ public function testBreadCrumbs() {
   +      $this->fail('Breadcrumb assertion should fail when breadcrumb block deleted and there is a trail.');
   +    }
   +    catch (\PHPUnit_Framework_ExpectationFailedException $e) {
   +      $this->assertTrue(TRUE, 'Breadcrumb assertion should fail when breadcrumb block deleted and there is a trail.');
   

   Nitpick: Since this text is used twice and is used for comparison, consider pulling out string into a variable.

I'm not sure how to test this (since it's only for testing code) but one of my notes from previous comment wasn't saved in dreditor so adding here:

+++ b/core/modules/system/tests/src/Functional/Menu/BreadcrumbTest.php
@@ -381,4 +382,44 @@ public function testBreadCrumbs() {
+    // Ensure the test trait works as expected.
+    $home = ['' => 'Home'];
+    $trail = $home + ['menu-test' => t('Menu test root')];

Nitpick: For those who don't know about the test routes, consider changes comment to something like:

Ensure the test trait works as expected using menu_test routes.

Wow. You are fast. :) Ok, I'm going to review the changes in a couple hours (going for a walk). Thanks!

Thanks for the updates. I reviewed the interdiff and the changes look good to me. I see the failure in the tests (screenshot attached). Should I be doing anything else to test this?

I'll mark RTBC but please see the last comment.

Should we be doing the same in the deprecated \Drupal\system\Tests\Menu\AssertBreadcrumbTrait ?

+++ b/core/modules/system/tests/src/Functional/Menu/AssertBreadcrumbTrait.php
@@ -60,6 +60,10 @@ protected function assertBreadcrumbParts($trail) {
+      $pass = FALSE;

why not just $this->fail() here instead of continuing?

1. Refactoring assertBreadcrumbParts().
Check in a loop if both (trait and breadcrumb) are not empty.
Fail if there is no breadcrumb and we have a trail or we have breadcrumb but trait is empty.

protected function assertBreadcrumbParts($trail) {
  // Compare paths with actual breadcrumb.
  $parts = $this->getBreadcrumbParts();
  $pass = TRUE;

  if (!empty($trail) && !empty($parts)) {
    foreach ($trail as $path => $title) {
      if ($path == '') {
        $url = Url::fromRoute('<front>')->toString();
      }
      elseif ($path[0] != '/') {
        $url = Url::fromUri('base:' . $path)->toString();
      }
      else {
        $url = $path;
      }
      $part = array_shift($parts);
      $pass = ($pass && $part['href'] === $url && $part['text'] === Html::escape($title));
    }
    // No parts must be left, or an expected "Home" will always pass.
    $pass = ($pass && empty($parts));
  }
  else if (!empty($trail) && empty($parts) || empty($trail) && !empty($parts)) {
    $pass = FALSE;
  }

  $this->assertTrue($pass, format_string('Breadcrumb %parts found on @path.', [
    '%parts' => implode(' » ', $trail),
    '@path' => $this->getUrl(),
  ]));
}

2. IMO this comments is not necessary.

--- a/core/modules/system/tests/src/Functional/Menu/AssertBreadcrumbTrait.php
+++ b/core/modules/system/tests/src/Functional/Menu/AssertBreadcrumbTrait.php
@@ -64,10 +64,6 @@ protected function assertBreadcrumbParts($trail) {
     // this test would go into an infinite loop, so we need to check that too.
     while ($trail && !empty($parts)) {
       foreach ($trail as $path => $title) {
-        // If the path is empty, generate the path from the <front> route.  If
-        // the path does not start with a leading slash, then run it through
-        // Url::fromUri('base:')->toString() to get the correct base
-        // prepended.
         if ($path == '') {
           $url = Url::fromRoute('<front>')->toString();
         }
         elseif ($path[0] != '/') {
           $url = Url::fromUri('base:' . $path)->toString();
         }

#15

+++ b/core/modules/system/tests/src/Functional/Menu/AssertBreadcrumbTrait.php
@@ -60,6 +60,10 @@ protected function assertBreadcrumbParts($trail) {
     // Compare paths with actual breadcrumb.
     $parts = $this->getBreadcrumbParts();
     $pass = TRUE;
+    // Fail if there is no breadcrumb and we have a trail.
+    if (!empty($trail) && empty($parts)) {
+      $pass = FALSE;
+    }

// There may be more than one breadcrumb on the page. If $trail is empty
// this test would go into an infinite loop, so we need to check that too.
while ($trail && !empty($parts)) {
  foreach ($trail as $path => $title) {

    (...)

    $part = array_shift($parts);
    $pass = ($pass && $part['href'] === $url && $part['text'] === Html::escape($title));
  }
}
// No parts must be left, or an expected "Home" will always pass.
$pass = ($pass && empty($parts));

I think the while loop is more difficult to analyze. If empty($trail) && !empty($parts) loop will not execute so $pass will be also FALSE. It can be written more simply:

#14

else if (!empty($trail) && empty($parts) || empty($trail) && !empty($parts)) {
  $pass = FALSE;
}

mixing a refactor with a bug fix is not how to do things

Considering that #15 solving a problem so RTBC. I will add a new tusk to refactor while loop after fix this issue.

Committed aba8c85 and pushed to 8.7.x. Thanks!

Also I think we should be leaving deprecated code alone. Otherwise we have to add a test for this and there is no test for the old trait atm.

sounds good

we could but then we'd have to duplicate the later message and message consistency is nice

not sure that is a reason on its own - we have local variables, but don't care enough to push the issue - let's just get this in

I added the issue in order to replace the while loop to conditional statement (#14.1).

#3030403: Improve the readability of the code in assertBreadcrumbParts