Revise docblock for BookManager methods

Working on this.

Added @param and @return to the function.

Added new patch

Thanks for the patches and reviews! Sorry for my not reviewing sooner; I've been on vacation...

Anyway, this latest patch still needs some work:

1. +++ b/core/modules/book/src/BookManager.php
   @@ -618,6 +618,26 @@ protected function bookTreeBuild($bid, array $parameters = array()) {
   +   *   - expanded: An array of parent link ids to return only book links that
   

   ids => IDs

2. +++ b/core/modules/book/src/BookManager.php
   @@ -618,6 +618,26 @@ protected function bookTreeBuild($bid, array $parameters = array()) {
   +   *     are children of one of the plids in this list. If empty, the whole
   

   don't use "plids" in docs text. Write out "parent link IDs" instead.

3. +++ b/core/modules/book/src/BookManager.php
   @@ -618,6 +618,26 @@ protected function bookTreeBuild($bid, array $parameters = array()) {
   +   *     outline is built, unless 'only_active_trail' is TRUE.
   

   Well it looks like also max_depth can affect whether the whole outline is built, right?

4. +++ b/core/modules/book/src/BookManager.php
   @@ -618,6 +618,26 @@ protected function bookTreeBuild($bid, array $parameters = array()) {
   +   *   - active_trail: An array of nids, representing the coordinates of the
   

   nids => node IDs

   And ... coordinates??!?

5. +++ b/core/modules/book/src/BookManager.php
   @@ -618,6 +618,26 @@ protected function bookTreeBuild($bid, array $parameters = array()) {
   +   *     trail. This option is ignored, if 'expanded' is non-empty.
   

   remove the comma

6. +++ b/core/modules/book/src/BookManager.php
   @@ -618,6 +618,26 @@ protected function bookTreeBuild($bid, array $parameters = array()) {
   +   *     Defaults to 1, which is the default to build a whole tree for a book.
   

   Redundant wording here. Tighten up.

7. +++ b/core/modules/book/src/BookManager.php
   @@ -618,6 +618,26 @@ protected function bookTreeBuild($bid, array $parameters = array()) {
   +   *   Return array of menu trees.
   

   Remove word "return" here.... and what is an "array of menu trees" anyway? It seems like it is just returning the tree for one book?

8. +++ b/core/modules/book/src/BookManager.php
   @@ -930,6 +950,9 @@ public function bookTreeCheckAccess(&$tree, $node_links = array()) {
   +   * ¶
   

   extra space on line end. Set up your editor to remove these or at least highlight them.

9. +++ b/core/modules/book/src/BookManager.php
   @@ -1010,6 +1033,20 @@ protected function buildBookOutlineData(array $links, array $parents = array(),
   +   * ¶
   

   extra space on line end

10. +++ b/core/modules/book/src/BookManager.php
    @@ -1010,6 +1033,20 @@ protected function buildBookOutlineData(array $links, array $parents = array(),
    +   * ¶
    

    extra space

11. +++ b/core/modules/book/src/BookManager.php
    @@ -1010,6 +1033,20 @@ protected function buildBookOutlineData(array $links, array $parents = array(),
    +   *   Return array of menu trees.
    

    I doubt it is an array of trees, should just be one tree?

12. +++ b/core/modules/book/src/BookManager.php
    @@ -1010,6 +1033,20 @@ protected function buildBookOutlineData(array $links, array $parents = array(),
    +   * ¶
    

    extra space

Hi,
Created patch.

1. +++ b/core/modules/book/src/BookManager.php
   @@ -1009,6 +1032,20 @@ protected function buildBookOutlineData(array $links, array $parents = array(),
   +   * ¶
   

   Extra Space

2. +++ b/core/modules/book/src/BookManager.php
   @@ -1009,6 +1032,20 @@ protected function buildBookOutlineData(array $links, array $parents = array(),
   +   * ¶
   

   Extra Space

3. +++ b/core/modules/book/src/BookManager.php
   @@ -1009,6 +1032,20 @@ protected function buildBookOutlineData(array $links, array $parents = array(),
   +   * ¶
   

   Extra Space

Corrected and applied patch. Please review.

Thanks! Getting better but still needs some work:

1. +++ b/core/modules/book/src/BookManager.php
   @@ -586,15 +586,15 @@ protected function buildItems(array $tree) {
   +   *   - active_trail: An array of node IDs, representing the coordinates of
   

   What are coordinates?

2. +++ b/core/modules/book/src/BookManager.php
   @@ -617,6 +617,26 @@ protected function bookTreeBuild($bid, array $parameters = array()) {
   +   * @param array $parameters
   +   *   (optional) An associative array of build parameters. Possible keys:
   +   *   - expanded: An array of parent link ids to return only book links that
   

   Please copy the corrected documentation from the other method in this patch to this section, instead of making the same mistakes that have already been corrected in the patch (see previous reviews). The lines here and below have several problems that have already been addressed in this patch, and I don't really want to go through them one by one again. Thanks!

3. +++ b/core/modules/book/src/BookManager.php
   @@ -617,6 +617,26 @@ protected function bookTreeBuild($bid, array $parameters = array()) {
   +   *     are children of one of the plids in this list. If empty, the whole
   

   plids => parent link IDs

4. +++ b/core/modules/book/src/BookManager.php
   @@ -992,7 +1015,7 @@ public function bookLinkTranslate(&$link) {
   +   *     'in_active_trail' (TRUE if the link ID was in $parents)
   

   Needs to end in .

5. +++ b/core/modules/book/src/BookManager.php
   @@ -1009,6 +1032,20 @@ protected function buildBookOutlineData(array $links, array $parents = array(),
   +   *   Return a menu tree.
   

   Um, I doubt it... what's a "menu" tree? I think it's returning a book tree?

   Also don't start return value docs with "Return a".

Hi jhodgdon,
Added Patch. For #2 kindly tell if more changes are required

Thanks for the patch! But where's the interdiff file please? In the future, when you attach a new patch on an existing issue that previously had a patch, ALWAYS make an interdiff file.

See https://www.drupal.org/documentation/git/interdiff for instructions.

Anyway, there are STILL the same problems with this patch as in previous reviews, plus a few new ones:

1. +++ b/core/modules/book/src/BookManager.php
   @@ -583,18 +583,18 @@ protected function buildItems(array $tree) {
   +   *   - active_trail: An array of node IDs, representing the currently active
   +   *      book link.
   

   The second line here should be indented 1 space less.

2. +++ b/core/modules/book/src/BookManager.php
   @@ -583,18 +583,18 @@ protected function buildItems(array $tree) {
   +   *     Defaults to 1, which is to build a whole tree for a book.
   

   a book => the book
   a whole tree => the whole tree

3. +++ b/core/modules/book/src/BookManager.php
   @@ -617,6 +617,26 @@ protected function bookTreeBuild($bid, array $parameters = array()) {
   +   *   (optional) An associative array of build parameters. Possible keys:
   +   *   - expanded: An array of parent link ids to return only book links that
   +   *     are children of one of the parent link IDs in this list. If empty,
   +   *     the whole outline is built, unless 'only_active_trail' is TRUE.
   +   *   - active_trail: An array of nids, representing the coordinates of the
   +   *     currently active book link.
   +   *   - only_active_trail: Whether to only return links that are in the active
   +   *     trail. This option is ignored, if 'expanded' is non-empty.
   +   *   - min_depth: The minimum depth of book links in the resulting tree.
   +   *     Defaults to 1, which is the default to build a whole tree for a book.
   +   *   - max_depth: The maximum depth of book links in the resulting tree.
   +   *   - conditions: An associative array of custom database select query
   +   *     condition key/value pairs; see _menu_build_tree() for actual query.
   

   sigh. AGAIN please instead of making the same mistakes that were already mentioned in previous reviews and fixed in the other part of this patch, please COPY this section from the part of the patch that has already been fixed.

   Sorry for shouting, but it is REALLY annoying to have to make the same review comments over and over. Probably the problem is that so many different people are working on this patch and they aren't reading the whole issue. It would be a LOT better if ONE person worked on an issue and kept working on it until it was fixed, rather than new people picking it up all the time and making the same mistakes over and over and over. Please.

4. +++ b/core/modules/book/src/BookManager.php
   @@ -617,6 +617,26 @@ protected function bookTreeBuild($bid, array $parameters = array()) {
   +   *   Tree of a book.
   

   This should say something like:

   An array with links representing the tree structure of the book.

   or maybe check what the other function's @return docs say and copy that?

Thanks @jhodgdon for your valuable comments and suggestions.
Patch attached for review.

1. +++ b/core/modules/book/src/BookManager.php
   @@ -583,18 +583,18 @@ protected function buildItems(array $tree) {
   +   *   The ID of the book that we are building the tree for.
   

   Please don't use we, them, those etc.

2. +++ b/core/modules/book/src/BookManager.php
   @@ -583,18 +583,18 @@ protected function buildItems(array $tree) {
   +   *     Defaults to 1, which is to build a whole tree for a book.
   

   which build a tree for a book.
   How's it ?
   Don't know what jhodgodon says about it.

3. +++ b/core/modules/book/src/BookManager.php
   @@ -617,6 +617,26 @@ protected function bookTreeBuild($bid, array $parameters = array()) {
   +   *   The ID of the book that we are building the tree for.
   

   See 1st one comment.

4. +++ b/core/modules/book/src/BookManager.php
   @@ -617,6 +617,26 @@ protected function bookTreeBuild($bid, array $parameters = array()) {
   +   *     currently active book link.
   

   currently really ?

5. +++ b/core/modules/book/src/BookManager.php
   @@ -617,6 +617,26 @@ protected function bookTreeBuild($bid, array $parameters = array()) {
   +   *     Defaults to 1, which is to build the whole tree for the book.
   

   Please see my second comment.

6. +++ b/core/modules/book/src/BookManager.php
   @@ -617,6 +617,26 @@ protected function bookTreeBuild($bid, array $parameters = array()) {
   +   *   An array with links representing the tree structure of the book.
   

   of the book
   OR
   of a book.
   Use same wording on similar lines.

7. +++ b/core/modules/book/src/BookManager.php
   @@ -929,6 +949,9 @@ public function bookTreeCheckAccess(&$tree, $node_links = array()) {
   +   *   The book tree you wish to operate on.
   

   You wish ???

updated patch attached.Please review.

1. +++ b/core/modules/book/src/BookManager.php
   @@ -618,14 +618,14 @@
   +   *   The ID of the book that are used to build the tree for.
   

   s/are/is

2. +++ b/core/modules/book/src/BookManager.php
   @@ -618,14 +618,14 @@
   +   *     active book link.
   

   Not sure about this.

3. +++ b/core/modules/book/src/BookManager.php
   @@ -635,7 +635,7 @@
   +   *   An array with links representing the tree structure of a book.
   

   use "the book" everywhere.

4. +++ b/core/modules/book/src/BookManager.php
   @@ -951,7 +951,7 @@
   +   *   The book tree which have to operate on.
   

   The book tree to operate on.

Thanks for all the patches and reviews!

But there are *still* problems with this patch:

1. +++ b/core/modules/book/src/BookManager.php
   @@ -583,18 +583,18 @@ protected function buildItems(array $tree) {
   -   *   The Book ID to find links for.
   +   *   The ID of the book that are used to build the tree for.
   

   This is garbled and ungrammatical. The original line is much clearer.

2. +++ b/core/modules/book/src/BookManager.php
   @@ -617,6 +617,26 @@ protected function bookTreeBuild($bid, array $parameters = array()) {
   +   *   The ID of the book that are used to build the tree for.
   

   See above note.

3. +++ b/core/modules/book/src/BookManager.php
   @@ -617,6 +617,26 @@ protected function bookTreeBuild($bid, array $parameters = array()) {
   +   * @param array $parameters
   +   *   (optional) An associative array of build parameters. Possible keys:
   +   *   - expanded: An array of parent link IDs to return only book links that
   +   *     are children of one of the parent link IDs in this list. If empty,
   +   *     the whole outline is built, unless 'only_active_trail' is TRUE.
   +   *   - active_trail: An array of node IDs, representing the coordinates of the
   +   *     active book link.
   +   *   - only_active_trail: Whether to only return links that are in the active
   +   *     trail. This option is ignored if 'expanded' is non-empty.
   +   *   - min_depth: The minimum depth of book links in the resulting tree.
   +   *     Defaults to 1, which is to build the whole tree for the book.
   +   *   - max_depth: The maximum depth of book links in the resulting tree.
   +   *   - conditions: An associative array of custom database select query
   +   *     condition key/value pairs; see _menu_build_tree() for actual query.
   

   Sigh. CAN YOU PLEASE COPY THE FIXED-UP DOCUMENTATION FROM THE OTHER PART OF THE PATCH INTO HERE?????????

   This is only about the 5th time I have asked for this to be done. It is getting very tiresome. Look at the patch. We are putting the same documentation into two places. One of them has been revised and fixed up. The other one was copied from the old version and still has problems. Why is this so difficult to take care of?

4. +++ b/core/modules/book/src/BookManager.php
   @@ -929,6 +949,9 @@ public function bookTreeCheckAccess(&$tree, $node_links = array()) {
   +   *   The book tree which have to operate on.
   

   This is garbled and ungrammatical.

Hi @Jhodgdon Sorry for making you shout here.
In future we will take care the same and ONE person will work on an issue and keep working on it until it is fixed, rather than new people picking it up all the time and making the same mistakes over and over and over.

Please see attached patch and interdiff.

Thank you, and even though I have been very frustrated lately with people ignoring my review comments, I should not have shouted -- so please accept my apology. It is definitely best if a single person assigns themself an issue and leaves it assigned (with only that one person working on it) until the patch is committed, so I would definitely appreciate it if people adopted that workflow!

Anyway, the new patch is almost perfect. One very small thing to fix:

+++ b/core/modules/book/src/BookManager.php
@@ -617,6 +617,26 @@ protected function bookTreeBuild($bid, array $parameters = array()) {
+   *   An array with links representing the tree structure of a book.

a book => the book

[the reason is this is the return docs, so "the" refers to the book that we're making the tree for]

Thanks for motivating us and your patience. I know we all are doing silly mistakes over and over again, i hope it will not be repeated again.
Anyway i uploaded the final patch with interdiff .

Thanks! Looks fine now.

+++ b/core/modules/book/src/BookManager.php
@@ -617,6 +617,26 @@ protected function bookTreeBuild($bid, array $parameters = array()) {
    * @see menu_build_tree()

Let's fix this whilst we are here - menu_build_tree() no longer exists. I think this should be replaced with an @see to BookOutlineStorageInterface::getBookMenuTree()

Good idea. Whoever makes the next patch, remember to put the full namespace on the class when you do this.

Hi alexpott/jhodgdon,
Replaced menu_build_tree(). Kindly suggest if any other changes are required.

Changes that suggested in #32are there in #34. Looks good now.

+++ b/core/modules/book/src/BookManager.php
@@ -884,7 +904,7 @@ protected function updateOriginalParent(array $original) {
+   *   The book link to update. It is passed by reference to setParents().

Sorry I should have noticed this before - it should be self::setParents() and there should be an @see with a fqdn link

Addressed #36 in this patch.
Thanks

I'll give it a go.

I'll give it a go.

Thanks for the new patch! Still not quite right though...

1. +++ b/core/modules/book/src/BookManager.php
   @@ -586,15 +586,15 @@ protected function buildItems(array $tree) {
       *     condition key/value pairs; see _menu_build_tree() for the actual query.
   

   Oh, we should also fix this -- function _menu_build_tree(), as far as I can tell, does not exist. It's most likely a method on this class now?

2. +++ b/core/modules/book/src/BookManager.php
   @@ -617,7 +617,27 @@ protected function bookTreeBuild($bid, array $parameters = array()) {
   +   *     condition key/value pairs; see _menu_build_tree() for the actual query.
   

   Same here.

3. +++ b/core/modules/book/src/BookManager.php
   @@ -884,9 +904,11 @@ protected function updateOriginalParent(array $original) {
   +   *   The book link to update. It is passed by reference to self::setParents().
   

   Um. This doesn't make sense to me. Why are we having a link to the function that is being documented here? (This is the docs header for the setParents() method.)

   So what we should be doing here is just something like:

   The book link to update, passed by reference.

4. +++ b/core/modules/book/src/BookManager.php
   @@ -884,9 +904,11 @@ protected function updateOriginalParent(array $original) {
   +   * @see \Drupal\book\BookManager::setParents()
       */
      protected function setParents(array &$link, array $parent) {
   

   We should not have an @see to the same method that is being documented.

   Sorry for the confusion... most likely Alex didn't realize which method was being documented when he said we wanted a @see added and the above modification.

@jhodgdon,

I have done the changes according to your comments above, I am not able to create the interdiff file from #35. So for

1 & 2)

   *   - conditions: An associative array of custom database select query
   *     condition key/value pairs; see \Drupal::menuTree( ); for the details.

3)

/**
   * Sets the p1 through p9 properties for a book link being saved.
   *
   * @param array $link
   *   The book link to update, passed by reference.

4) removed the @see

Thanks! Still some problems to fix in this patch...

1. +++ b/core/modules/book/src/BookManager.php
   @@ -586,18 +586,18 @@ protected function buildItems(array $tree) {
   -   *     condition key/value pairs; see _menu_build_tree() for the actual query.
   +   *     condition key/value pairs; \Drupal::menuTree( ); for the details.
   

   This line has MANY problems:
   - The word "see" has vanished. We need it.
   - Should not be "the" before "details".
   - The namespace/class are missing for this. There is no Drupal::menuTree() method. It is on some other class.
   - There should not be a space between the ()
   - There should not be a ; after the ()

   Note: there's another line below with similar problems...

2. +++ b/core/modules/book/src/BookManager.php
   @@ -617,7 +617,27 @@ protected function bookTreeBuild($bid, array $parameters = array()) {
   +   *   The Book ID to find links for.
   

   Book should not be capitalized here.

3. +++ b/core/modules/book/src/BookManager.php
   @@ -617,7 +617,27 @@ protected function bookTreeBuild($bid, array $parameters = array()) {
   +   *     condition key/value pairs; see \Drupal::menuTree( ); for the details.
   

   See note above about multiple problems in this line.

Please review this one.

Thanks, but:

1. +++ b/core/modules/book/src/BookManager.php
   @@ -586,18 +586,19 @@ protected function buildItems(array $tree) {
   +   *     condition key/value pairs; see \Drupal\Core\Menu::MenuLinkTree() for
   

   This class/method does not exist, as far as I can tell. Method names never begin with capital letters anyway... Something is wrong.

2. +++ b/core/modules/book/src/BookManager.php
   @@ -617,7 +618,28 @@ protected function bookTreeBuild($bid, array $parameters = array()) {
   +   *     condition key/value pairs; see \Drupal\Core\Menu::MenuLinkTree() for
   

   Same here

Thanks, almost!

1. +++ b/core/modules/book/src/BookManager.php
   @@ -586,18 +586,19 @@ protected function buildItems(array $tree) {
   +   *     condition key/value pairs; see \Drupal\book\BookOutlineStorage::getBookMenuTree()
   

   Thanks for fixing this, but now this documentation line goes over 80 characters. Please rewrap.

2. +++ b/core/modules/book/src/BookManager.php
   @@ -617,7 +618,28 @@ protected function bookTreeBuild($bid, array $parameters = array()) {
   +   *     condition key/value pairs; see \Drupal\book\BookOutlineStorage::getBookMenuTree()
   

   Here too.

@jhodgdon If i am not wrong this issue should be move into 8.1.x branch. What do you think ?

So, this is an API docs issue. It will eventually need to be committed to 8.0, 8.1, and 8.2. Our policy (which is NOT sustainable) has been that such issues are marked 8.0 and then committed to all three at the same time... I think this policy will need to be revised at some point but that is what we're doing at the moment.

Regarding the last patch, looking at the interdiff, the rewrapping still isn't quite right:

+++ b/core/modules/book/src/BookManager.php
@@ -598,7 +598,8 @@
+   *     condition key/value pairs; see
+   *     \Drupal\book\BookOutlineStorage::getBookMenuTree()
    *     for the actual query.

At least some of the words in that last line "for the actual query" can be moved up to the previous line... same in the other doc block in the interdiff too.

Have added a patch for the above comment.

Looks good now, thanks!

Changing version to 8.2.x and adding tags for backport, because this should go into all 3 8.x branches. The "backport" probably just means "apply the same patch to all 3".

Committed/pushed to all three 8.x branches, thanks!