Use RouteMatch instead of request attributes in book navigation block

Let's keep this a 8.3.x only fix, so we don't have to deal with 100% strict BC policies.

+++ b/core/modules/book/src/Cache/BookNavigationCacheContext.php
diff --git a/core/modules/book/src/Plugin/Block/BookNavigationBlock.php b/core/modules/book/src/Plugin/Block/BookNavigationBlock.php
index 1a68622..d2c43cd 100644

index 1a68622..d2c43cd 100644
--- a/core/modules/book/src/Plugin/Block/BookNavigationBlock.php

--- a/core/modules/book/src/Plugin/Block/BookNavigationBlock.php
+++ b/core/modules/book/src/Plugin/Block/BookNavigationBlock.php

+++ b/core/modules/book/src/Plugin/Block/BookNavigationBlock.php
+++ b/core/modules/book/src/Plugin/Block/BookNavigationBlock.php
@@ -8,12 +8,13 @@

@@ -8,12 +8,13 @@
 namespace Drupal\book\Plugin\Block;
 

Could we not rewrite this block to use context as in block context? Then this block could be also used for usecases where the node is provided by something else in page_manager/panels. I guess this could be totally be done in a follow up.

I don't think #7 is really a followup - if we're changing how the block gets its book node let's get it right first time.

I don't think #7 is really a followup - if we're changing how the block gets its book node let's get it right first time.

Well, the problem with #7 is that it requires a big update path, and it will break instances of the block in page_manager we cannot update.

@dawehner ah ok - then a followup it is.

Still, the patch needs re-rolling.

Sure

Patch updated to 9.1-dev

Do we need test coverage for it?

BookTest already has coverage for the block, as this is a refactor/bug fix I don't think we need additional coverage.

Tentatively marking RTBC. Unsure if we have to do the deprecation dance in the constructors here or not. I could only find one case in contrib and it looks like the module is using the service incorrectly anyway, as it just duplicates the existing core service twice:

https://git.drupalcode.org/project/book_blocks/-/blob/8.x.-1.x/book_bloc...

@longwave given thats a stable module I think we should either fix it first or account for BC here by removing the typehint and doing a deprecation dance.

Something like:

  /**
   * Constructs a new BookNavigationCacheContext service.
   *
   * @param \Drupal\Core\Routing\RouteMatchInterface $route_match
   *   The current route match.
   */
  public function __construct($route_match) {
    if (!$route_match instanceof RouteMatchInterface) {
      @trigger_error("@todo", E_USER_DEPRECATED);
      $route_match = \Drupal::routeMatch();
    }
    $this->routeMatch = $route_match;
  }

Made the changes suggested in #30. Please review.

1. +++ b/core/modules/book/src/Cache/BookNavigationCacheContext.php
   @@ -25,20 +26,24 @@ class BookNavigationCacheContext implements CacheContextInterface, ContainerAwar
   -    $this->requestStack = $request_stack;
   

   Oh we also should use the \Drupal\Core\DependencyInjection\DeprecatedServicePropertyTrait in order to provide BC for this. See \Drupal\Core\Installer\Form\SiteConfigureForm::$deprecatedProperties for an example of how that works.

2. +++ b/core/modules/book/src/Cache/BookNavigationCacheContext.php
   @@ -25,20 +26,24 @@ class BookNavigationCacheContext implements CacheContextInterface, ContainerAwar
   +      @trigger_error("@todo", E_USER_DEPRECATED);
   

   @todo was a placeholder :) - see other @trigger_error's in constructors for an example of what to do. I don't think this requires a CR because the @trigger_error message will describe exactly what to do.

Updated the patch as changes mentioned in #32. Please review.

+++ b/core/modules/book/src/Cache/BookNavigationCacheContext.php
@@ -23,22 +25,32 @@
+      @trigger_error("Passing the request_stack service to ' . __METHOD__ . '() is deprecated in drupal:9.2.0 and will be removed before drupal:10.0.0. The parameter should be instance of \Drupal\Core\Routing\RouteMatchInterface instead", E_USER_DEPRECATED);

A word is missing here - this should read "The parameter should be an instance of..." or could just be "Pass an instance of...". The sentence should also end in a full stop.

Also, we should add a deprecation test for this.

Updated the patch as changes suggested by @longwave in #35.

Missed the full stop in patch #37. Updated the patch as changes suggested by @longwave in #35.

Thanks, the message looks good now.

+++ b/core/modules/book/tests/src/Functional/BookTest.php
@@ -97,6 +98,17 @@ protected function setUp(): void {
+    $bookNavigationCacheContext = new BookNavigationCacheContext($requestStack);

We will get the unused variable police on this line sooner or later, we should assert something simple about the result I think. Also the variable names should be snake_case not camelCase.

I'll do the fix

Added changes pointed on comment #39.

$requestStack doesn't follow our variable naming either, sorry!

Updated the patch as suggested in #42.

This all looks good now.

Committed/pushed to 9.2.x, thanks!