Deprecate, remove and replace \Drupal\Tests\book\Functional\BookTestTrait::generateOutlinePattern

FIxed CCF of #2.
Please review.

There is only one use of generateOutlinePattern() ?

Since this is test-only code, I don't understand why it's deprecated instead of just removed.

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.

Reviewing the code and webassert changes look good and correct deprecation call looks correct.

LGTM.

Thanks for working on this! There are just some minor issues with the local variable naming.

1. +++ b/core/modules/book/tests/src/Functional/BookTestTrait.php
   @@ -88,37 +86,34 @@ public function createBook($edit = []) {
   -    // $number does not use drupal_static as it should not be reset
   -    // since it uniquely identifies each call to checkBookNode().
   -    static $number = 0;
   

   Yech, glad we are getting rid of that.

2. +++ b/core/modules/book/tests/src/Functional/BookTestTrait.php
   @@ -88,37 +86,34 @@ public function createBook($edit = []) {
   +      $book_nav = $this->getSession()->getPage()->find('css', sprintf('nav[aria-labelledby="book-label-%s"] ul', $this->book->id()));
   +      $this->assertNotNull($book_nav);
   +      $links = $book_nav->findAll('css', 'a');
   

   I think it should probably be $book_navigation. Even though it's passing cspell, we generally don't use abbreviations in variable names.

3. +++ b/core/modules/book/tests/src/Functional/BookTestTrait.php
   @@ -88,37 +86,34 @@ public function createBook($edit = []) {
   +      $previousEl = $this->assertSession()->elementExists('named_exact', ['link', 'Go to previous page']);
   +      $this->assertEquals($previous->toUrl()->toString(), $previousEl->getAttribute('href'));
   

   $previousEl is again using an abbreviation, and furthermore breaking the "don't mix camel and snake case" rule. It should be $previous_element.

4. +++ b/core/modules/book/tests/src/Functional/BookTestTrait.php
   @@ -88,37 +86,34 @@ public function createBook($edit = []) {
   +      $upEl = $this->assertSession()->elementExists('named_exact', ['link', 'Go to parent page']);
   +      $this->assertEquals($up->toUrl()->toString(), $upEl->getAttribute('href'));
   

   In the same vein, this should be $parent_element.

5. +++ b/core/modules/book/tests/src/Functional/BookTestTrait.php
   @@ -88,37 +86,34 @@ public function createBook($edit = []) {
   +      $nextEl = $this->assertSession()->elementExists('named_exact', ['link', 'Go to next page']);
   +      $this->assertEquals($next->toUrl()->toString(), $nextEl->getAttribute('href'));
   

   And $next_element.

Updated patch in #7 with the nitpicks pointed out in #9. Also fixed phpcs warnings w.r.t to the patch changes only.

Thanks!

Shouldn't we test the added Deprecation somewhere?

Confirmed #11 addresses the points in #9

@Spokje I see the deprecation at the bottom for the generateOutlinePattern function.

Thanks @smustgrave, apparently I didn't scroll all the way down... _facepalm_

Looks like a random JS test failure, requeued test to make sure.

+++ b/core/modules/book/tests/src/Functional/BookTestTrait.php
@@ -88,37 +86,43 @@ public function createBook($edit = []) {
+      $book_navigation = $this->getSession()->getPage()->find('css', sprintf('nav[aria-labelledby="book-label-%s"] ul', $this->book->id()));
+      $this->assertNotNull($book_navigation);
+      $links = $book_navigation->findAll('css', 'a');
+      $this->assertCount(count($nodes), $links);
+      foreach ($nodes as $delta => $node) {
+        $link = $links[$delta];
+        $this->assertEquals($node->label(), $link->getText());
+        $this->assertEquals($node->toUrl()->toString(), $link->getAttribute('href'));

I compared this with the existing method, which is:

  public function generateOutlinePattern($nodes) {
    $outline = '';
    foreach ($nodes as $node) {
      $outline .= '(node\/' . $node->id() . ')(.*?)(' . $node->label() . ')(.*?)';
    }
    return '/<nav role="navigation" aria-labelledby="book-label-' . $this->book->id() . '"(.*?)<ul(.*?)' . $outline . '<\/ul>/s';
  }

The only test coverage we're removing is that of testing the markup. And we shouldn't test the markup, because the markup is the theme's business.

I also verified that the fail in https://www.drupal.org/pift-ci-job/2573719 was in an unrelated test (we should always watch for exactly what JS random fails are when we are updating a JS test).

Spokje's earlier feedback is unaddressed:

Shouldn't we test the added Deprecation somewhere?

@smustgrave replied:

@Spokje I see the deprecation at the bottom for the generateOutlinePattern function.

@Spokje was asking about test coverage for the new deprecation, not the deprecation itself. Bit of miscommunication there. However, we don't necessarily need to add test coverage for deprecations in internal APIs. The policy states:

Test traits and abstract base classes should generally use deprecation where possible rather than breaking backwards compatibility, but they are still considered an internal API and may change if necessary.

I especially think we don't need test coverage for the deprecation of a very funky and situationally specific method that is used only once in core for what is already a low-usage module, so here is release management signoff for not having test coverage of that. 🪄

Committed to 10.1.x only since it contains a deprecation. Thanks! Although, I hope we'll be deprecating the whole module for D11... Also published the change record.