API page: https://api.drupal.org/api/drupal/core%21modules%21book%21src%21BookMana...

In the Parameters section:

bool $new: Is this a new book.

Should be:

bool $new: Is this a new link.

That is, "book" should be "link".

CommentFileSizeAuthor
#16 2985265-16.patch767 bytesayushmishra206
#16 interdiff_6-16.txt807 bytesayushmishra206
#14 2985165_after_patch.png28.31 KBAbhijith S
#6 2985165-6.patch755 bytesAkashKumar07
#3 save-book-link-docs-2985165.patch524 bytesnbaosullivan

Issue fork drupal-2985165

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mathieso created an issue. See original summary.

valthebald’s picture

valthebald
Location Sofia
CreditAttribution: valthebald at Dropsolid commented
Title: Bad docs in BookManager::saveBookLink » Bad docs in BookManagerInterface::saveBookLink
Issue tags: -developer +API Documentation, +Novice, +Amsterdam2019

Looks like an easy fix

nbaosullivan’s picture

nbaosullivan CreditAttribution: nbaosullivan commented
FileSize
save-book-link-docs-2985165.patch524 bytes

Updated docs as suggested.

nbaosullivan’s picture

nbaosullivan CreditAttribution: nbaosullivan commented
Version: 8.2.x-dev » 8.9.x-dev
Status: Active » Needs review
Sutharsan’s picture

Sutharsan CreditAttribution: Sutharsan at LimoenGroen commented
Status: Needs review » Needs work

Although I agree with this change, $new is about the link, the docblock is still confusing. The function description is about a book entry, while the method updates/saves a link to a book entry. Further, the return value of the method is the saved link data. "The book data of that node" is confusing and possibly incorrect.

I suggest to extend the scope of the issue to fix all documentation of this method. If we do, please update the issue summary too.

AkashKumar07’s picture

AkashKumar07 CreditAttribution: AkashKumar07 at OpenSense Labs commented
Status: Needs work » Needs review
FileSize
2985165-6.patch755 bytes

I have made this changes, as per the suggestion. Please let me know, if any other changes has to be done.

GrandMasterJude’s picture

GrandMasterJude CreditAttribution: GrandMasterJude as a volunteer and commented

Reviewed the changes in 2985165-6.patch, changes look fine. Good job all.

valthebald’s picture

valthebald
Location Sofia
CreditAttribution: valthebald at Dropsolid commented 
  1. +++ b/core/modules/book/src/BookManagerInterface.php
@@ -183,15 +183,15 @@ public function getAllBooks();
+   * Saves/Updates link of a single book entry.

    There's hardly any difference between "Saves" and "Saves/Updates" (I would even say the latter is more confusing) 

  2. +++ b/core/modules/book/src/BookManagerInterface.php
@@ -183,15 +183,15 @@ public function getAllBooks();
+   *   The saved link data of the book.

    Of what book? We have only link data in $link parameter.
    Maybe worth commenting that $link array also has information about the book? (Probably adding @see reference)

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

michaellenahan’s picture

michaellenahan CreditAttribution: michaellenahan at Hubert Burda Media commented
Issue summary: View changes
Issue tags: -Amsterdam2019 +Europe2020
Novice issue reserved for the Mentored Contribution during the Contribution Day at Europe2020. After the 11th December 2020, this issue returns to being open to all. Thanks
michaellenahan’s picture

michaellenahan CreditAttribution: michaellenahan at Hubert Burda Media commented
Issue tags: +Amsterdam2019
michaellenahan’s picture

michaellenahan CreditAttribution: michaellenahan at Hubert Burda Media commented
Issue summary: View changes
Issue tags: -Europe2020
Abhijith S’s picture

Abhijith S CreditAttribution: Abhijith S as a volunteer and at Zyxware Technologies commented
FileSize
2985165_after_patch.png28.31 KB

Applied patch #6 on 9.2.x and it works fine.

after

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Needs work

Agree with #8, Saves/Updates is confusing.

ayushmishra206’s picture

ayushmishra206 CreditAttribution: ayushmishra206 at OpenSense Labs commented
Status: Needs work » Needs review
Issue tags: -Novice, -Amsterdam2019
FileSize
interdiff_6-16.txt807 bytes
2985265-16.patch767 bytes

Made the changes suggested. in #8. Please review.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

vikashsoni’s picture

vikashsoni CreditAttribution: vikashsoni as a volunteer and at Zyxware Technologies commented

Applied patch #6 on 9.3.x and it works fine and look good for me
Thanks for the patch

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Liam Morland made their first commit to this issue’s fork.

Liam Morland’s picture

Liam Morland
Primary language English
Location Ontario, CA 🇨🇦
CreditAttribution: Liam Morland at OpenPlus commented
Title: Bad docs in BookManagerInterface::saveBookLink » Improve documentation for BookManagerInterface::saveBookLink()
Version: 9.5.x-dev » 10.1.x-dev

I have added all three patches to an issue fork and made an additional commit to improve the documentation for the $link param.

Liam Morland opened merge request !2603

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Reviewed & tested by the community

The documentation change makes sense to me.

valthebald’s picture

valthebald
Location Sofia
CreditAttribution: valthebald at Dropsolid commented
Status: Reviewed & tested by the community » Needs review

I would slightly change the wording
"Saves link of a single book entry" => "Saves a link of a single book entry" or "Saves a single book entry link"
"The saved link array has information about the book." => "The book entry link information"

Liam Morland’s picture

Liam Morland
Primary language English
Location Ontario, CA 🇨🇦
CreditAttribution: Liam Morland at OpenPlus commented

I have rebased the merge request and implemented the suggestions from #25.

Liam Morland’s picture

Liam Morland
Primary language English
Location Ontario, CA 🇨🇦
CreditAttribution: Liam Morland at OpenPlus commented

Rebased

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Reviewed & tested by the community

Can confirm the changes in #25 have been addressed

Hiding all the files to avoid confusion.

Liam Morland’s picture

Liam Morland
Primary language English
Location Ontario, CA 🇨🇦
CreditAttribution: Liam Morland at OpenPlus commented
Status: Reviewed & tested by the community » Needs review

I made the changes suggested by the maintainer.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

This was very helpful personally for reviewing documentation tickets.

Changes look like they were addressed and all threads are closed.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 090e898157 to 10.1.x and f57cba65f9 to 10.0.x and 9813a1cc64 to 9.5.x. Thanks!

  • longwave committed f57cba65 on 10.0.x 
    Issue #2985165 by Liam Morland, ayushmishra206, nbaosullivan,...

  • longwave committed 090e8981 on 10.1.x 
    Issue #2985165 by Liam Morland, ayushmishra206, nbaosullivan,...

  • longwave committed 9813a1cc on 9.5.x 
    Issue #2985165 by Liam Morland, ayushmishra206, nbaosullivan,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.