Make Book navigation translatable

Hey @wadmiraal - are you likely to have time to circle back to this one?

Strike me it's a good idea if it's not too much work!

Which version of Drupal 8 can make Book navigation translatable?

Thank you!

I checked this again with Drupal 8.0.1 and it does not work.
It would be very good if this could handle in standard.

Thank you.

Here is a patch that addresses the list of child pages and the bread crumb titles.

Here is a patch that addresses the list of child pages and the bread crumb titles.
Now the links point to the correct language.

I have have confirmed that this patch works for me. Unsure if testing is needed before this can be applied.

Just to be specific this fixes the breadcrumb, Book Navigation block as well as the (lower) link menu "next" and "previous" links.

#10 appears to work fine for me as well

The code looks reasonable, I'd say, but we really need test coverage to be able to sure it will be working in the future.

#10 works fine. Thank you, ErikWegner.

#10, links are translated, but still point to a different language pages (in book tree section).

Patch including tests for:

1. Pager links (next, previous)
2. Breadcrumbs
3. Outline

Code for test reformatted

Reformatted the code

Nice test coverage!

+++ b/core/modules/book/tests/src/Functional/BookI18NTest.php
@@ -0,0 +1,422 @@
+class BookI18NTest extends BrowserTestBase {

I'd just have named it BookTranslationTest, one less abbreviation people would have to know :)

That's a good point. Here is the patch with the class renamed.

So the patch seems to be doing it's job fine.

On thing though which is important: it's entirely possible a node is not translated in a certain language, so should we then ignore this in the tree when we're showing the navigation in a different language. It's a requirement for the site I'm working on right now to hide it, so maybe this could be a setting on the block and use that as a default?

To visualize the problem. In english (default language) we have this tree

- installation (node 1)
- mac (node 2)
- linux (node 3)
- windows (node 4).

But in dutch, only the installation node is translated, the rest is not. Yet in the navigation tree in the dutch, we see the 3 children which don't make sense to be displayed there.

This behavior (a bug imho) can also be seen in the traversal links at the bottom.

Note, I'd personally consider this as a bug report (maybe even major), as multillingual book navigations are basically useless right now.

Improved intendation.

But I don't know why, I am not able to create interdiff files

#53 is doing the job... many thanks for this!!!!
Note that in "Detection and selection" page, if I check "User" to have the settings like the screencapture, the block is not translated even if I click on another language on the language switcher block...

See #35.

#26 (and #29 which is same as #26 but with improved indentation) work fine for me but I noticed that one issue is still there.
Along with the issues which are reported, there is an another issue which you can reproduce like this:

• Configure Book content type for translation
• Enable a second language
• Create a book outline in English
• Translate each node in the outline
• Navigate the /admin/structure/book "Edit order and titles" in the second language
• Edit the titles and click "Save book pages"

You will notice that the titles are saved for the English language not for the second language, which is incorrect.

Before #26 patch the Title fields in /admin/structure/book "Edit order and titles" were always loaded with the English translation.
After #26 patch, when visiting the "Edit order and titles" page, the Title fields now get loaded correctly but if you edit the Title fields and save them, the titles are saved for the default-language/English not the currently selected second language (just like without the patch). For solving this issue here is the updated patch which is same as #26 (#29 too) with additional work which addresses the above issue.

The names/titles of the books are not correctly translated in /admin/structure/book. They are set to English/default-language in all cases.

Steps to reproduce:

• Configure Book content type for translation
• Enable a second language
• Create a book outline in English
• Translate each node in the outline including the book root page
• Switch language to second language
• Navigate to path /admin/structure/book

You will notice that although you have selected the second language, the book titles are displayed in English.

This patch addresses to resolves this issue too. After applying this patch you will see that the book titles are displayed in the correct translation.

@sakhan: I am interested in a use case where the structure of a book is different for several languages. Could you please describe one and explain the benefits? Maybe it could be an option for each book to choose whether the structure is the same for all languages?

I've tested via simplytest.me and followed the steps to reproduce that are in #36. However, it doesn't appear to be working. I'm still seeing the book titles displayed in English. I'm pretty sure I've followed the steps correctly, but perhaps someone else could make it work.

@Daniel_Rose Most probably your Interface text language detection settings are not properly configured. Please visit admin/config/regional/language/detection and choose a suitable detection method and then try again. I have tested on my end that if I don't set an interface language detection method then the books titles are shown in the default language.

@sakhan, no problem. I'll give it another try. Thanks for the feedback. :)

@Daniel_Rose Please login to https://dubbr.ply.st/ as admin/admin and then visit https://dubbr.ply.st/admin/structure/book AND https://dubbr.ply.st/ur/admin/structure/book, hopefully you will notice the difference. The other language used is Urdu(ur).

@sakhan, I definitely notice there. I guess I didn't correctly setup the language translation. My apologies and thank you very much for your feedback and help. :)

Shall we mark this as RTBC since it's working?

Hello,
I got few notices when applying #36

patching file core/modules/book/src/BookBreadcrumbBuilder.php
patching file core/modules/book/src/BookManager.php
patching file core/modules/book/src/Form/BookAdminEditForm.php
Hunk #1 succeeded at 100 (offset 2 lines).
Hunk #2 succeeded at 117 (offset 2 lines).
patching file core/modules/book/tests/src/Functional/BookTranslationTest.php

Patch for Drupal 8.4.x working here too. Thanks a lot.

The Drupal\book\BookBreadcrumbBuilder class implements a service, so using \Drupal::languageManager() in its methods is wrong.

Using service for language manager. Adding translation to html export.

As class property, the code should use $this->languageManager, not $this->language_manager, in the same way it's used $this->viewBuilder.

Patch: Test fix, code style

Typo in test

There is just something more that I think needs be changed.

       // Note- access checking is already performed when building the tree.
       if ($node = $this->nodeStorage->load($data['link']['nid'])) {
+
+        // HERE
+        $langcode = $this->languageManager->getCurrentLanguage()->getId();
+        if ($node->isTranslatable()) {
+          if ($node->hasTranslation($langcode)) {
+            $node = $node->getTranslation($langcode);
+          }
+        }

It's not clear what that HERE means. If it is referring to the previous comment, then I would rather move that comment.

       if ($node = $this->nodeStorage->load($data['link']['nid'])) {
         // Access checking is already performed when building the tree.
         $langcode = $this->languageManager->getCurrentLanguage()->getId();
         if ($node->isTranslatable()) {
           if ($node->hasTranslation($langcode)) {
             $node = $node->getTranslation($langcode);
           }
         }

Note- is not using the correct punctuation. I would just leave that out, since every code comment is a note about the code.

It seems the comment is explaining why the code doesn't verify the users have permission to access the node. In that case, I would rather change the code like this.

       // Access checking is not necessary here as it is already performed when building the tree.
       if ($node = $this->nodeStorage->load($data['link']['nid'])) {
         $langcode = $this->languageManager->getCurrentLanguage()->getId();
         if ($node->isTranslatable()) {
           if ($node->hasTranslation($langcode)) {
             $node = $node->getTranslation($langcode);
           }
         }

Should not the code verify the users have access to the translation node? The existing code (i.e. the code before the patch) is not getting the translated nodes, so it is also not checking the users have access to the translation nodes.
Probably that comment should be removed, and the code verify the users have access to the translation node.

I was also checking if the following code could be simplified, since it is used more than once.

  if ($node->isTranslatable()) {
    if ($node->hasTranslation($langcode)) {
      $node = $node->getTranslation($langcode);
    }
  }

The only simplification I could get is the following one, which replaces two if() statements with one.

  if ($node->isTranslatable() && $node->hasTranslation($langcode)) {
    $node = $node->getTranslation($langcode);
  }

$node->isTranslatable() is necessary because it checks there is more than a language added to the site, and we don't want to show a translation that was added when the site had two or more languages, after the site is back to having a single language.

That // HERE was an old marker where to insert the new code. Just forgot to remove that.

For those if-conditions: I prefer to have them as separate statements for legibility and creating diffs.

Btw: is it possible to have access to a node in one language but not in the other?

Since the translation is a different node, users could have access to a node, but not the other one, even if the latter is the translation of the previous.

Translation is not a different node in most cases, each language is a set of field values (doc). The published status is also shared across all languages.

They are two different node objects with the same ID, but different properties (e.g. author, creation date, active language). As such, a module implementing a content access hook could deny access to the translation. In fact, there are core tests verifying that.

See NodeAccessLanguageTest::testNodeAccess().

  \Drupal::state()->set('node_access_test_secret_catalan', 1);
  $node_public_ca = $this->drupalCreateNode([
    'body' => [
      [
        
      ],
    ],
    'langcode' => 'ca',
    'private' => FALSE,
  ]);
  $this->assertTrue($node_public_ca->language()->getId() == 'ca', 'Node created as Catalan.');
  
  // Tests that access is granted if requested with no language.
  $this->assertNodeAccess($expected_node_access, $node_public_no_language, $web_user);
  $this->assertNodeAccess($expected_node_access_no_access, $node_public_ca, $web_user);
  
  // Tests that Hungarian node is still accessible.
  $this->assertNodeAccess($expected_node_access, $node_public_hu, $web_user);
  $this->assertNodeAccess($expected_node_access, $node_public_hu->getTranslation('hu'), $web_user);
  
  // Tests that Catalan is still not accessible.
  $this->assertNodeAccess($expected_node_access_no_access, $node_public_ca->getTranslation('ca'), $web_user);

The code acting behind the scene is in node_access_test_node_access().

  $secret_catalan = \Drupal::state()->get('node_access_test_secret_catalan') ?: 0;
  if ($secret_catalan && $node->language()->getId() == 'ca') {
    
    // Make all Catalan content secret.
    return AccessResult::forbidden()->setCacheMaxAge(0);
  }

Thank you for the example, that shows what can be possible.

From my point of view, the patch we have developed so far makes at least these three assumptions:

• Consistent hierarchy: a book page has always the same parent and the same children in all languages.
• Consistent access: a book page is either accessible to the user or not, no matter which language is selected.
• Partial translation: not every book page has a translation. If a translation exists, it is selected in favour of the base language version. If no translation exists, the untranslated version is used.

From my end user perspective, the issues with untranslated links and nodes can be solved easily by applying this patch. But if any of the above points is not valid, then it needs a broader approach to the problem. I am thinking of the book module's storage structure and all internals, there are many places that might need a new concept.

Perhaps it is also possible to fix this issue with the given patch and address any further concerns in a new issue that is handled by a more skilled person?

Using the default language, and without this patch, the Book module gives me the links only for the nodes to which I have access. If I cannot access a book page, I don't see the link to that page.

This patch introduces a difference in the behavior of the module. It could be classified as bug, since it gives me a link to a node to which I could not have access.

As side note, also the published status is translatable, which means a translation could be published, and another translation not.

Hi,
I am not a techie guy so May I ask for your advice if I can use the patch #56 as stated for Drupal 8.6 for my Drupal 8.5? Or, should use the patch #47? Thanks a lot.

Installing 56 on 8.5.2 gave me white screen error. "encountered error"

Currently this issue makes book navigation useless in multilingual sites. It displays only original content. And there are problems with breadcrumbs.

Here is the error I receive.

drupal __construct() must implement interface Drupal\Core\Language\LanguageManagerInterface, none given, called in /home/xxxxxx/public_html/core/lib/Drupal/Component/DependencyInjection/Container.php on line 266 in Drupal\book\BookBreadcrumbBuilder->__construct() (line 52 of core/modules/book/src/BookBreadcrumbBuilder.php).

After applying #56, you have to flush the caches (e.g. drush cr).

#56 drupal 8.5.2
English and custom language.
Thanks @erikwegner

Clearing the cache didn't work in 8.5.2. I've hidden the book navigation by css. Unfortunately that was the only viable option.

@leventdal
How did you clear the caches?
Do you get another error message now?

@ErikWegner

I cleared cache from the performance page.
I didn't try it again with 8.5.3

+++ b/core/modules/book/book.services.yml
@@ -1,18 +1,18 @@
   book.breadcrumb:
     class: Drupal\book\BookBreadcrumbBuilder
-    arguments: ['@entity.manager', '@current_user']
+    arguments: ['@entity.manager', '@current_user', '@language_manager']
...
   book.manager:
     class: Drupal\book\BookManager
-    arguments: ['@entity.manager', '@string_translation', '@config.factory', '@book.outline_storage', '@renderer']
+    arguments: ['@entity.manager', '@string_translation', '@config.factory', '@book.outline_storage', '@renderer', '@language_manager']
...
   book.export:
     class: Drupal\book\BookExport
-    arguments: ['@entity.manager', '@book.manager', '@renderer']
+    arguments: ['@entity.manager', '@book.manager', '@language_manager']

+++ b/core/modules/book/src/BookBreadcrumbBuilder.php
@@ -38,10 +46,13 @@ class BookBreadcrumbBuilder implements BreadcrumbBuilderInterface {
-  public function __construct(EntityManagerInterface $entity_manager, AccountInterface $account) {
+  public function __construct(EntityManagerInterface $entity_manager, AccountInterface $account, LanguageManagerInterface $language_manager) {
     $this->nodeStorage = $entity_manager->getStorage('node');
     $this->account = $account;
+    $this->languageManager = $language_manager;
   }

+++ b/core/modules/book/src/BookExport.php
@@ -40,11 +48,14 @@ class BookExport {
+   * @param \Drupal\Core\Language\LanguageManagerInterface $language_manager
+   *   The language manager.
    */
-  public function __construct(EntityManagerInterface $entityManager, BookManagerInterface $book_manager) {
+  public function __construct(EntityManagerInterface $entityManager, BookManagerInterface $book_manager, LanguageManagerInterface $language_manager) {
     $this->nodeStorage = $entityManager->getStorage('node');
     $this->viewBuilder = $entityManager->getViewBuilder('node');
     $this->bookManager = $book_manager;
+    $this->languageManager = $language_manager;

Potentially other users have subclasses this class. Given that I think you should make the constructor argument optional aka. $this->languageManager = $language_manager ?: \Drupal::service('language_manager')

Note: This would have prevented the problem described by #64

@dawehner Are you sure that a subclass is causing the exception? But that is actually a valid concern: how do/should subclasses handle dependencies (especially changes of constructor arguments) of their parent classes?

Patch #56 works perfectly for me in drupal 8.6 dev in the following link site as example.

http://www.ergophthalmology.com

Hello.
Patch #56 works, but I caught one more problem. When we're trying to edit book outline (change book item parent for example) - the options in "Parent item" are not translated and is being displayed in base language. I've done some investigation and solved that (more or less). Also added a test for this. Maybe it will be helpful for someone.

#75 works perfectly, thank you!

I have entered the code #75 manually and have got the white screen with the message (cache was cleared before it):

"The website encountered an unexpected error. Please try again later.
Recoverable fatal error: Argument 3 passed to Drupal\book\BookBreadcrumbBuilder::__construct() must implement interface Drupal\Core\Language\LanguageManagerInterface, none given, called in /home/useraccount/mysite.org/core/lib/Drupal/Component/DependencyInjection/Container.php on line 266 and defined in Drupal\book\BookBreadcrumbBuilder->__construct() (line 52 of core/modules/book/src/BookBreadcrumbBuilder.php)."

My site version is drupal 8.6.4
Server hoster: PHP Version 5.6.35
-------------------------
Could you suggest any solution, please? (I am an ordinary user using ready-made solutions.)

Please try to clear the cache after applying the patch.

@ErikWegner
Unfortunately, I had no chance to do it. Any menu navigation led to this message.

I try to change the code, so that it works without clearing the cache.

@ErikWegner
Thanks for your support. I once again entered all the code manually. This time I previously opened the Clear Cache page. After entering the code and running the cache, I got a white screen without any messages. When I manually moved to the root of the site, everything works well, and the translation of the book is displayed correctly.
But whenever I hit ../development/performance/Clear cache I get a white screen without messages again.

Status Report informs that there are no errors. If I Run Cron - everything is OK. Recent log messages page - no any messages after Clear Cache. I optimized the database, cleared the browser's cookies, went out and went back to my site, but after Clear Cache I always get a white screen without messages.

Additionally, I inform you that initially I had settings ... Configuration> Regional and language> Russian as default. Then I decided to put finally the English as Default. After that, I entered code #75.
Perhaps an error due to the pre-change of the default language of the site?

If dependency injection does not know about additional required service, retrieve service from global object. This should handle the case when clearing the cache is not possible and avoid a WSOD.

@ErikWegner
Hello, Erik. Unfortunately, the new changes did not fix the WSOD error. When I restored the original files of the directory Book, the cache cleaning worked correctly.

#82 applied patch for drupal 8.6.4 - working.

I also applied patch #82, which appears to be working. Hoping this functionality can be committed to core asap.

However, one thing it does is that if I am on a French page, the book outline block still displays any English pages in the outline that were not translated. I believe this is intentional, but for what I need, it should only list the pages in the outline that are of the same language as the current page. Same with the next/previous pages at the bottom of the page.

Not sure if this is relevant; but in the event you want to see both unpublished pages and have them show translated then this patch could be applied after the patch in [26552] - which I would assume / hope will be rolled in at some point.

< @@ -999,6 +1028,12 @@ public function bookLinkTranslate(&$link) {
<
<      // For performance, don't localize a link the user can't access.
<      if ($link['access']) {
---
> @@ -1005,6 +1034,12 @@ public function bookLinkTranslate(&$link) {
>          $node = $this->entityManager->getStorage('node')
>            ->load($link['nid']);
>        }

@ErikWegner
Haveing a new Drupal update the patch #82 is working without any problem. Thank you.

#86 and #82 doesn't apply on 8.7

patching file core/modules/book/book.services.yml
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file core/modules/book/book.services.yml.rej
patching file core/modules/book/src/BookBreadcrumbBuilder.php
Hunk #1 FAILED at 5.
Hunk #2 succeeded at 31 (offset -1 lines).
Hunk #3 FAILED at 39.
Hunk #4 succeeded at 68 (offset -4 lines).
2 out of 4 hunks FAILED -- saving rejects to file core/modules/book/src/BookBreadcrumbBuilder.php.rej
patching file core/modules/book/src/BookExport.php
Hunk #1 FAILED at 3.
Hunk #2 succeeded at 33 (offset -1 lines).
Hunk #3 FAILED at 41.
Hunk #4 succeeded at 102 (offset -4 lines).
2 out of 4 hunks FAILED -- saving rejects to file core/modules/book/src/BookExport.php.rej
patching file core/modules/book/src/BookManager.php
Hunk #1 succeeded at 7 with fuzz 2 (offset 1 line).
Hunk #2 FAILED at 67.
Hunk #3 FAILED at 108.
Hunk #4 succeeded at 433 with fuzz 2 (offset 4 lines).
Hunk #5 succeeded at 483 (offset 4 lines).
Hunk #6 succeeded at 564 (offset 4 lines).
Hunk #7 FAILED at 591.
Hunk #8 succeeded at 674 (offset -1 lines).
Hunk #9 succeeded at 1023 with fuzz 1 (offset -1 lines).
3 out of 9 hunks FAILED -- saving rejects to file core/modules/book/src/BookManager.php.rej
patching file core/modules/book/src/Form/BookAdminEditForm.php
patching file core/modules/book/tests/src/Unit/BookManagerTest.php
Hunk #1 succeeded at 40 (offset 1 line).
Hunk #2 succeeded at 63 (offset 1 line).

Patch #86 at Drupal 8.6.13 produces error, when I try to open a translated book: "The website encountered an unexpected error. Please try again later."
Patch #82 is OK.

reroll of #86 patch

There is an exception raised in a test.

Drupal\Tests\system\Functional\Update\UpdatePathTestBaseFilledTest::testUpdatedSite
Exception: Error: Call to a member function isTranslatable() on null
Drupal\book\BookManager->bookLinkTranslate()

Patch #75 is working like charme for Drupal 8.7.x if you change the "EntityManagerInterface $entityManager" to "EntityTypeManagerInterface $entity_type_manager"

The other patches are fixing the export issue, but not the book navigation issue.

@JD_1
I tried your solution for Drupal 8.7.3, it doesn't work, any option. [The website encountered an unexpected error. Please try again later.]
So, finally, do we have any working patch?

@godwing

I will send you my modified files tomorror, if you like.

This is working perfectly fine on our 8.7.3 installation.

Attached you will find modified files for drupal 8.7.3, based on Patch 75

@JD_1
Thank you, Daniel! It began to work, really, like a miracle. I replaced the files with yours. In my inexperienced view there are no errors, but you can look at the published book.

@JD_1 Thank you for the files, I created a patch from them for 8.7.3.

Does this fixes the translation of the book tree as well? The one which shows below the content on bartik theme at least?

Yes

@JD_1 and everyone involved, you are my heroes, thanks to all of you.

The patch works like a charm as far as I could test in the past few minutes.

This needs to be included in the next core update :D

KR
Mauricio

#101 Working for 8.7.3 for me. Thanks.

@mauriciomedeiros
I agree, it's time to include it into the next core update.

#101 working beautifully on 8.7.4 for me.

What is the process for this to be included in the core? Can the community push for it in any way? :D

Apparently it is still not there...

Also, has anyone tested #101 on 8.7.5?

KR
Mauricio Medeiros

@mauriciomedeiros It will be included if somebody provides a correct patch. So far, the last patches failed, or failed to apply.

I am so much frustrated to see that still I can't work on my some projects just because they require book module :(
I am afraid that even if I use some recent working patch that may fail in the next release of the Drupal 8. Why Drupal core maintainers don't take their action to solve this problem once for all as they may know more about how to solve this issue.

I am waiting for the working multilingual book module for 2 years as you might have noticed from my #35 and #36 patches. With my heavy heart I can say Drupal is losing my interest and loyalty day by day.
On the top of the drupal.org page, the first menu entry says "Why Drupal?" it should be "Why NOT Drupal?"!

#101 Working for 8.7.10 for me. Thanks.

#101 patch helps to display right language version. But it shows page from another language even if page is not translated yet:

Needs re-rolling for 8.8

Re-rolled #101 for 8.8.x

The tests fails because of the following error.

Drupal\Tests\book\Unit\BookManagerTest::testGetBookParents with data set #1 (array(11, 12), array(11, 1, 11), array(2, 11, 12, 0, 0, 0, 0, 0, 0, 0))
Drupal\Core\DependencyInjection\ContainerNotInitializedException: \Drupal::$container is not initialized yet. \Drupal::setContainer() must be called with a real container.

It is repeated for each data set the test is using.

(The other error is test runner returned a non-zero error code.)

Fix for the failing tests.
Still needs work though as we need a change record.

Haven't checked the patch itself, but here are some code style comments:

1. +++ b/core/modules/book/book.services.yml
   @@ -36,4 +36,4 @@ services:
   \ No newline at end of file
   

   there should be newline in the end of file

2. +++ b/core/modules/book/src/BookBreadcrumbBuilder.php
   @@ -87,4 +104,4 @@ public function build(RouteMatchInterface $route_match) {
   \ No newline at end of file
   

   No newline

3. +++ b/core/modules/book/src/BookExport.php
   @@ -141,4 +159,4 @@ protected function bookNodeExport(NodeInterface $node, $children = '') {
   \ No newline at end of file
   

   No newline

Have implemented the changes suggested in #124

Now the newlines are two. I would suggest to read what https://www.drupal.org/pift-ci-job/1506753 says about coding standards by clicking on 7 coding standards messages.

(This is the right patch.)

Still two remaining coding standards issues to fix:
https://www.drupal.org/pift-ci-job/1506832

And needs a change record.

Please can you remember to include an interdiff when uploading new patches, so it's easier to review the changes between versions. Thanks!

Fixing the 2 coding standard issues mentioned in #131

Also, since the services are getting a new dependency, the class constructors should use code similar to the one used from CssCollectionOptimizer::__construct().

public function __construct(AssetCollectionGrouperInterface $grouper, AssetOptimizerInterface $optimizer, AssetDumperInterface $dumper, StateInterface $state, FileSystemInterface $file_system = NULL) {
  $this->grouper = $grouper;
  $this->optimizer = $optimizer;
  $this->dumper = $dumper;
  $this->state = $state;
  if (!$file_system) {
    @trigger_error('The file_system service must be passed to CssCollectionOptimizer::__construct(), it is required before Drupal 9.0.0. See https://www.drupal.org/node/3006851.', E_USER_DEPRECATED);
    $file_system = \Drupal::service('file_system');
  }
  $this->fileSystem = $file_system;
}

As reference, this is the issue adding that code: #2244513: Move the unmanaged file APIs to the file_system service (file.inc).

Rerolled patch from #132 for Drupal 8.7.x

Any solid solution so far?

I tried to apply this to latest stable (8.8.3) and had two rejections, rerolled. This is because of the change from entity manager to entity type manager.

Moving this to Needs Review.

There isn't yet any BC code as described in comment #134.

@kiamlaluno Can you please discuss in detail what else is needed to solve this issue? I see many of us are just struggling to solve this issue but lacking some expertise so is it possible from your side to contribute to solve this issue once for all please?
One other way I can think of is to give monetary support to the issue resolver to expedite the process.

See comment #134. I quoted the code of the constructor of a class implementing a service for which the parameters have been changed. The constructor has a new parameter with a NULL default value, and it triggers a E_USER_DEPRECATED error when the default value is used for that parameter.

  if (!$file_system) {
    @trigger_error('The file_system service must be passed to CssCollectionOptimizer::__construct(), it is required before Drupal 9.0.0. See https://www.drupal.org/node/3006851.', E_USER_DEPRECATED);
    $file_system = \Drupal::service('file_system');
  }
  $this->fileSystem = $file_system;

Similar code needs to be used for any service which get a parameter that it didn't get in older versions.

Drupal 9 will be released soon but yet the book module lack very basic multilingual support :(

patch #138 failed to apply so i rerolled patch.

@kiamlaluno wrt point in comment #143. can you please provide link for language_manager service where it deprecated ?

@swatichouhan012 The language_manager service isn't deprecated.
Comment #142 is saying what code needs to be added when new arguments are added to a service, such as in the case of the language_manager service which is added to the book.manager service.

The patch from comment #144 does not work properly. Although the menu title is displayed correctly, the link to the pages is not correct if you use a prefix (like /de or /en in the URL)

Patch 2470896_138.patch is working for me.

#144 and #138 are working fine for me with Drupal 8.9.x-dev.

Drupal 9 is about to be released soon but still book module is not translatable. So many developers couldn't transform their ideas into realities due to this missing translation feature.

Re-rolled up to 9.1
Added notice from #142 and new 'Change records'

Patch from comment #150 working for me with drupal 9.

The deprecation messages need to be updated: If the code is changed in Drupal 9.1.x, the messages cannot say Drupal 8.9.

@kiamlaluno thank you for review.
You warning make sense, so I changed it to 9.1

It's seems good to go, to me.

Finally we have a patch that at least works with the upcoming version of Drupal that's Drupal 9! I tested it very extensively and it doesn't fail for me.

#144 works for me except that it doesn't render translation's URL but origin's URL.

#156 is working perfectly! Thank you so much

1. +++ b/core/modules/book/src/BookBreadcrumbBuilder.php
   @@ -68,11 +83,17 @@ public function build(RouteMatchInterface $route_match) {
   +          if ($parent_book->isTranslatable()) {
   +            if ($parent_book->hasTranslation($langcode)) {
   
   +++ b/core/modules/book/src/BookExport.php
   @@ -102,11 +117,18 @@ public function bookExportHtml(NodeInterface $node) {
   +        if ($node->isTranslatable()) {
   +          if ($node->hasTranslation($langcode)) {
   
   +++ b/core/modules/book/src/BookManager.php
   @@ -109,12 +122,24 @@ protected function loadBooks() {
   +          if ($nodes[$nid]->isTranslatable()) {
   +            if ($nodes[$nid]->hasTranslation($langcode)) {
   
   +++ b/core/modules/book/src/Form/BookAdminEditForm.php
   @@ -116,6 +129,11 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +            if ($node->isTranslatable()) {
   +              if ($node->hasTranslation($langcode)) {
   

   This can be if ($parent->isTranslatable() && $parent_book->hasTranslatatioon($langcde)_ {

2. +++ b/core/modules/book/src/BookManager.php
   @@ -109,12 +122,24 @@ protected function loadBooks() {
   +      // Load nodes with proper translation.
   +      $langcode = $this->languageManager->getCurrentLanguage()->getId();
   +      foreach ($nodes as $nid => $node) {
   +        if ($node->isTranslatable() && $node->hasTranslation($langcode)) {
   +          $nodes[$nid] = $node->getTranslation($langcode);
   +        }
   +      }
    
          // @todo: use route name for links, not system path.
          foreach ($book_links as $link) {
            $nid = $link['nid'];
            if (isset($nodes[$nid]) && $nodes[$nid]->status) {
   +          if ($nodes[$nid]->isTranslatable()) {
   +            if ($nodes[$nid]->hasTranslation($langcode)) {
   +              $nodes[$nid] = $nodes[$nid]->getTranslation($langcode);
   +            }
   +          }
   

   This looks duplicated I think only one of these code hunks is necessary.

3. We need to add automated test coverage here. So we can prove we've fixed the bug and that we don't break it in the future. The onther test we're changing so far is an unit test but we've not added test coverage of the bug we're fixing here.

4. +++ b/core/modules/book/book.services.yml
   @@ -36,4 +36,4 @@ services:
   -    lazy: true
   +    lazy: true
   \ No newline at end of file
   

   This change is out-of-scope - and I think incorrect.

It also needs re-roll, since the patch failed to apply.

Rerolled patch for 9.1.x. Still need to address #158 and #159.

Addressing 1-2 of #158; 4 is already fixed.

Still needs tests.

(Patches aren't testing, if the status isn't changed to Needs review.)

No longer requires Reroll.

Setting status back to Needs Work for someone to address #158.3 - still needs tests.

#163 is working fine and has not the mentioned #158.3 issue.

Added a kernel test. There's no interdiff: BookMultilingualTest is new, that's all. getAllBooks(), bookTreeAllData() and getTableOfContents() are covered.

D'oh I always forget that @group because I run only the single file with phpunit. While at it, I have added asserts to make sure the test actually hits the code paths we want it to hit and another to make sure the current language switcher works. It's possible we want to move the current language switcher to a trait in a separate patch, it looks like it could be very useful for other such tests as well.

Removing Needs test issue tag, this has good coverage now.

+++ b/core/modules/book/src/BookBreadcrumbBuilder.php
@@ -38,10 +46,13 @@ class BookBreadcrumbBuilder implements BreadcrumbBuilderInterface {
+    $this->languageManager = $language_manager ?: \Drupal::service('language_manager');

+++ b/core/modules/book/src/BookExport.php
@@ -40,11 +48,14 @@ class BookExport {
+    $this->languageManager = $language_manager ?: \Drupal::service('language_manager');

+++ b/core/modules/book/src/BookManager.php
@@ -81,12 +89,13 @@ class BookManager implements BookManagerInterface {
+    $this->languageManager = $language_manager ?: \Drupal::service('language_manager');

+++ b/core/modules/book/src/Form/BookAdminEditForm.php
@@ -41,10 +49,13 @@ class BookAdminEditForm extends FormBase {
+    $this->languageManager = $language_manager ?: \Drupal::service('language_manager');

Seems like the request of the deprecation message for the new arguments in constructors are not taken care of yet (see #142).

Happy to RTBC afterwards as looks like this has been tested several times by several people and the latest patch is adding tests but not changing functional code.

I have the messages added and also more tests added but please hold for now: I am having a chat with various maintainers whether the translation to the interface language is correct or not. I feel it's not and it should use content language and the EntityRepository::getTranslationFromContext method should be used. Please wait a few days while we sort this out. If I am not back in a week feel free to continue without me... but I will be.

There's no interdiff here because it'd be bigger than the patch as almost every line touched by the patch has been changed.

1. +++ b/core/modules/book/src/BookBreadcrumbBuilder.php
   @@ -38,10 +46,17 @@ class BookBreadcrumbBuilder implements BreadcrumbBuilderInterface {
   +   *   The entity repository service.
       */
   -  public function __construct(EntityTypeManagerInterface $entity_type_manager, AccountInterface $account) {
   +  public function __construct(EntityTypeManagerInterface $entity_type_manager, AccountInterface $account, EntityRepositoryInterface $entity_repository) {
        $this->nodeStorage = $entity_type_manager->getStorage('node');
        $this->account = $account;
   +    if (!$entity_repository) {
   +      @trigger_error('The entity.repository service must be passed to ' . __NAMESPACE__ . '\BookBreadcrumbBuilder::__construct(). It was added in drupal:9.1.0 and will be required before drupal:10.0.0.', E_USER_DEPRECATED);
   +      $entity_repository = \Drupal::service('entity.repository');
   +    }
   +    $this->entityRepository = $entity_repository;
      }
   

   this only works if you add = NULL to the argument, as the type otherwise doesn't allow NULL as a valid value.

2. +++ b/core/modules/book/src/BookManager.php
   @@ -80,13 +97,27 @@ class BookManager implements BookManagerInterface {
        $this->renderer = $renderer;
   +    if (!$language_manager) {
   +      @trigger_error('The language_manager service must be passed to ' . __NAMESPACE__ . '\BookManager::__construct(). It was added in drupal:9.1.0 and will be required before drupal:10.0.0.', E_USER_DEPRECATED);
   +      $language_manager = \Drupal::service('language_manager');
   +    }
   +    $this->currentContentLangcode = $language_manager->getCurrentLanguage(LanguageInterface::TYPE_CONTENT);
   

   I'd recommend to store the language manager, not the current language. that can in theory change during the request.

3. +++ b/core/modules/book/src/BookManager.php
   @@ -666,14 +700,13 @@ protected function bookTreeBuild($bid, array $parameters = []) {
          sort($parameters['expanded']);
        }
   -    $tree_cid = 'book-links:' . $bid . ':tree-data:' . $language_interface->getId() . ':' . hash('sha256', serialize($parameters));
   +    $tree_cid = implode(':', ['book-links', $bid, 'tree-data', $this->currentContentLangcode, hash('sha256', serialize($parameters))]);
    
        // If we do not have this tree in the static cache, check {cache_data}.
   

   Interesting. Is there any chance the interface language _also_ affects what we cache here? I suppose not as we can expect all links to be nodes and that's all we translate?

4. +++ b/core/modules/book/src/BookManager.php
   @@ -1015,7 +1048,7 @@ public function bookLinkTranslate(&$link) {
          }
   -      // The node label will be the value for the current user's language.
   +      $node = $this->entityRepository->getTranslationFromContext($node);
          $link['title'] = $node->label();
   

   ha, interesting case of "I wish to make the API work as I'd like it to by writing a comment saying it is so" :)

5. +++ b/core/modules/book/tests/src/Kernel/BookMultilingualTest.php
   @@ -0,0 +1,211 @@
   +  /**
   +   * Tests various book manager methods return correct translations.
   +   *
   +   * #@dataProvider langcodesProvider
   +   */
   +  public function xtestMultilingualBookManager($langcode) {
   +    $this->setCurrentLanguage($langcode);
   +    /** @var \Drupal\book\BookManagerInterface $bm */
   

   accidentally commented out?

   I'm not a huge fan of data providers in anything but unit tests FWIW. This will do the setup() twice for each combination.

1. Added NULLs. (I already had a few , now we have more.)
2. I'd recommend to store the language manager -- done. (Also fixes the test failures -- I stored the current language when I wanted the current langcode.)
3. . Is there any chance the interface language _also_ affects what we cache here? I suppose not as we can expect all links to be nodes and that's all we translate? -- that's my understanding too. We do not do anything interface-y.
4. accidentally commented out? -- fixed. Data providers are necessary because BookManager::$books stores translated nodes and it's not possible to reset it. I suppose we could reset the entire service but based on our chat this is cleaner.

+++ b/core/modules/book/tests/src/Kernel/BookMultilingualTest.php
@@ -93,7 +93,7 @@
    *
    * #@dataProvider langcodesProvider
    */
-  public function xtestMultilingualBookManager($langcode) {
+  public function testMultilingualBookManager($langcode) {
     $this->setCurrentLanguage($langcode);

the data provider is still commented out with a #, I suppose that caused an error otherwise.

Alright, one more then.

Hopefully the last review.

1. +++ b/core/modules/book/tests/src/Kernel/BookMultilingualTest.php
   @@ -0,0 +1,211 @@
   +   *
   +   * #@dataProvider langcodesProvider
   +   */
   +  public function testMultilingualBookBreadcrumbBuilder($langcode) {
   

   another #, but it seems to run and work anyway? we should remove that though.

2. +++ b/core/modules/book/tests/src/Kernel/BookMultilingualTest.php
   @@ -0,0 +1,211 @@
   +    // Make sure the below parts of the code is actually tested.
   +    if ($top) {
   +      $this->assertTrue($below_tested);
   +    }
   

   several parts of the test seem to be basically testing/asserting itself, also the negotiation below. a bit unusual I suppose but doesn't hurt.

3. +++ b/core/modules/book/tests/src/Kernel/BookMultilingualTest.php
   @@ -0,0 +1,211 @@
   +   * @param $langcode
   +   *   The langcode.
   

   type missing here.

4. +++ b/core/modules/book/tests/src/Kernel/BookMultilingualTest.php
   @@ -0,0 +1,211 @@
   +   * @param $langcode
   +   *   The langcode.
   +   */
   +  protected function setCurrentLanguage($langcode) {
   +    /** @var \Drupal\language\ConfigurableLanguageManagerInterface $lm */
   +    $lm = $this->container->get('language_manager');
   +    $negotiator = $this->prophesize(LanguageNegotiatorInterface::class);
   +    foreach ($lm->getLanguages() as $candidate_langcode => $candidate) {
   +      $type = $candidate_langcode === $langcode ? LanguageInterface::TYPE_CONTENT : LanguageInterface::TYPE_INTERFACE;
   +      $negotiator->initializeType($type)->willReturn([$type => $candidate]);
   +    }
   +    $lm->setNegotiator($negotiator->reveal());
   +    $this->assertSame($lm->getCurrentLanguage(LanguageInterface::TYPE_CONTENT)->getId(), $langcode);
   +    $this->assertNotSame($lm->getCurrentLanguage(LanguageInterface::TYPE_INTERFACE)->getId(), $langcode);
   +  }
   +
   

   also type missing on $langcode @param.

   I know it's long, but we should use $language_manager and not $lm per our coding standards

Fixed as requested.

+++ b/core/modules/book/tests/src/Kernel/BookMultilingualTest.php
@@ -192,20 +192,20 @@
    *   The langcode.
    */
   protected function setCurrentLanguage($langcode) {
-    /** @var \Drupal\language\ConfigurableLanguageManagerInterface $lm */
-    $lm = $this->container->get('language_manager');
+    /** @var \Drupal\language\ConfigurableLanguageManagerInterface $language_manager  */
+    $language_manager  = $this->container->get('language_manager');
     $negotiator = $this->prophesize(LanguageNegotiatorInterface::class);
-    foreach ($lm->getLanguages() as $candidate_langcode => $candidate) {
+    foreach ($language_manager ->getLanguages() as $candidate_langcode => $candidate) {

the search & replace added an extra space in several places here.

Feeling bad to set back to needs work because of that, so fixing that and RTBC'ing anyway, I think that's fine as it's just spaces and an extra comma that phpstorm phpcs also complained about.

Thanks for this, a couple of observations / questions - setting to needs review for feedback

1. +++ b/core/modules/book/tests/src/Kernel/BookMultilingualTest.php
   @@ -0,0 +1,211 @@
   +  /**
   +   * Modules to enable.
   +   *
   +   * @var array
   +   */
   +  protected static $modules = [
   

   ubernit: we can use inheritdoc here

2. +++ b/core/modules/book/tests/src/Kernel/BookMultilingualTest.php
   @@ -0,0 +1,211 @@
   +  public function testMultilingualBookManager($langcode) {
   ...
   +  public function testMultilingualBookBreadcrumbBuilder($langcode) {
   ...
   +  protected function recurseTree(array $tree, $langcode, $top = TRUE) {
   ...
   +  protected function assertBookLinkIsCorrectlyTranslated(array $link, $langcode) {
   ...
   +  protected function setCurrentLanguage($langcode) {
   

   we can and should typehint this as string for new code in my opinion

3. +++ b/core/modules/book/tests/src/Kernel/BookMultilingualTest.php
   @@ -0,0 +1,211 @@
   +    foreach ($bbb->build($route_match)->getLinks() as $link) {
   +      $parameters = $link->getUrl()->getRouteParameters();
   +      if (isset($parameters['node'])) {
   +        $nid = $parameters['node'];
   +        $node = Node::load($nid);
   +        $this->assertSame($node->getTranslation($langcode)->label(), $link->getText());
   +        $found = TRUE;
   +      }
   +    }
   +    $this->assertTrue($found);
   

   should we be more explicit here and assert the expected parent crumbs too? e.g. we're just asserting that one of the crumbs matches the child item (#2) but shouldn't we be also testing that the book parent appears in the correct language?

   I.e. rather than looping over all links to find one, assert that we have a matching set of titles in the correct language for all of the items?

We're also missing deprecation tests here to assert the expected deprecations are raised if the constructor args are missing

Addressing pointers 180.1 and 180.2

> We're also missing deprecation tests here to assert the expected deprecations are raised if the constructor args are missing

We usually don't add tests for that unless there is special complexity, I think it's fine to skip that.

The patch in #182 does fix the the points, #180.1 and .2.

However, 180.3 still needs to be addressed. Setting to NW.

This patch addresses 180.3

In the file

Kernel/BookMultilingualTest.php,

the test sets up two books for testing.

I revised the setUp function so that two books are each nine pages deep.

The test
testMultilingualBookBreadcrumbBuilder

--loads the deepest page of one of the books
--constructs the breadcrumb for that page
--tests that each element in the breadcrumb shows the correct translation

Attached is interdiff for 182-185

My apologies.

Thank you so much for taking this on.

This comment "// The node label will be the value for the current user's language." does not seem correct to me. The logic is complex, let's just call it the "current language".

Made the changes requested in #187. Please Review

Fixing drupal code standard.

Fixing an error in a comment, sorry.

I have tried this patch and translated books are largely working, many thanks to everyone that has contributed to this patch. I am seeing an issue where my base language is En and the translation language is Fr - when I view a book node in Fr the book navigation block shows the link titles in Fr (properly) but they are actually linked to the En node url. I managed to (hack?) fix it like this in BookManager.php:

  protected function buildItems(array $tree) {
    
      if ($data['link']['in_active_trail']) {
        $element['in_active_trail'] = TRUE;
      }

+      $language = $this->languageManager->getCurrentLanguage(LanguageInterface::TYPE_CONTENT);
      // Allow book-specific theme overrides.
      $element['attributes'] = new Attribute();
      $element['title'] = $data['link']['title'];
      $node = $this->entityTypeManager->getStorage('node')->load($data['link']['nid']);
-      $element['url'] = $node->toUrl();
+      $element['url'] = $node->toUrl('canonical', ['language' => $language]);
      $element['localized_options'] = !empty($data['link']['localized_options']) ? $data['link']['localized_options'] : [];
      $element['localized_options']['set_active_class'] = TRUE;
      $element['below'] = $data['below'] ? $this->buildItems($data['below']) : [];
      $element['original_link'] = $data['link'];

I am wondering (a) if anyone else is seeing this issue and (b) should I potentially open a related issue in order to not muddy up this one? Cheers.

@joel_osc

I can confirm this (https://www.drupal.org/project/drupal/issues/2470896#comment-13522365). This is the reason I am using patch #163

@joel_osc Thanks, I think it'd be great if you could add it to this patch with an appropriate test, I feel it might belong here.

Set back to needs work.

I'll do the job.

Adding the fix pointed on comment #192.

Thanks for adding it but don't you think it needs a test, too?

Ignore this patch, it's just because my client is on 8.9.x still

this is patch 156 rerolled for 8.9.x
sorry no interdiff

ignore this patch, it is 156 rerolled for D8.9.7

just because my client is on 8.9.x still on 8.9.7

this is patch 156 rerolled for 8.9.7
sorry no interdiff

Set back to needs work because we need to write tests for patch #197