For some book paths - with 3 parents and a child, it cuts off the direct parent. Or, for a parent with multiple children, the menu traversal seems to get stuck at the top child instead of the right child.
I have a book with the following leaves: 1/2/3 and 1/2/4/5.
I updated the alias for 1/2/4/5 - and it ended up with the wrong 1/2/5.
On further examination, found that editing any book page results in the same path - the top-most children are sent back - which is 1/2/3 in this case, even if the real path should be 1/7 or 1/8/9 etc.

In one example:
The breadcrumb shows 1 - 2 - 4 for a child with title 5, but the bookpath returned is 1/2 instead of 1/2/4.
Editing the child page also shows the correct book outline, and as mentioned, breadcrumbs on the page are also correct - so the book.module is getting this right.
I did some simple debugging - inserted print statements in _menu_titles.
It seems that the menu traversal is incorrect - it seems to be getting the top-most child of every parent. So if a parent has multiple children, the bookpath returned is incorrect - at least in the case I traced.
In the above case, the printouts in the _menu_titles function emit 1 then 2 then 3 and then book_token_values cuts off the last element, so bookpath becomes 1/2 instead of 1/2/3.
In this case, _menu_titles should have got 1, 2, 4, and then 5.
Somehow, the menu traversal went from 1 to 2 to 3, instead of from 1 to 2 to 4.

CommentFileSizeAuthor
#6 258326_token_bookpath_hierarchy_broken_deep.patch3.42 KBgreggles
#2 token_node.inc_.patch2.6 KBbwooster47
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bwooster47’s picture

bwooster47 CreditAttribution: bwooster47 commented

Looked at the code in token_node.module, compared it to book.module.

I believe now that the _menu_titles(...) function cannot be correct - there is only one way the loop traverses children and that is by this line:
$item = array_shift($item['below']);

Basically, it looks at
($item['below'])['below'])['below'])['below']) (with multiple array_shifts in front)
i.e., will pick the first item from the 'below' list, and then pick the first item from its 'below' list.
But, in a menu, there can be multiple items in the ['below'] array for a single item. Each has to be looked into.
The book module handles this by using the ['in_active_trail'] tag to pick the right 'below' item, which is only set for displaying nodes, so cannot be used by token module.

Both the menupath and bookpath of token module cannot work based on the above issue.

If the above analysis is correct, possible solutions:
1) do a full tree search - which means recursion, and back-tracking to find the correct path from top node to the bottom node.
2) $node->book is a array with data in ['p1'], ['p2'], etc - these look like id's of nodes in the menu path for that $node. There are only 9 of these, so this could be used to handle 9-deep book nodes, which might work for most people using bookpath.
Not sure if this would work for menupath, that may require the full tree search.
3) I tried changing _menu_titles call to menu_tree_all_data and pass it $node->book as the second argument. This resulted in no change, the explanation for menu_tree_all_data seemed to suggest that giving a second argument would give its path to root, which is actually what we are looking for. But the return value from passing $node->book or NULL was the same. So that leaves solution 1 or 2 above.

Would like to hear comments, one way or the other, thoughts, alternate solutions, etc?

bwooster47’s picture

bwooster47 CreditAttribution: bwooster47 commented
FileSize
token_node.inc_.patch2.6 KB

I've tested the attached patch for bookpath - and it works fine at least at my test site. If this fix seems fine, I hope this fix gets into the dev branch as soon as possible!

menupath is not tested - that is still all blank. I could not find any way to make it enter this if statement:
if ($node->menu['mlid']) {
Story nodes, book nodes, blog nodes, etc don't have a menu object, so not sure how to test that. The above if statement is probably too restrictive, since every node has a breadcrumb, and I believe that the menupath is a breadcrumb representation?

Anyway, for fixing _menu_titles function, it now takes the fully loaded menu_link object as a parameter, and then uses the p columns - the p%d values in the menu parents list to do a correct traversal of the tree, going across each sibling, and then going deep as needed.

dan_aka_jack’s picture

dan_aka_jack CreditAttribution: dan_aka_jack commented
Status: Active » Needs review

I've installed bwooser47's patch and it works for me. Good work - thanks! I can now use pathauto to correctly create paths such as book-name/chapter-name/page-title.

dan_aka_jack’s picture

dan_aka_jack CreditAttribution: dan_aka_jack commented

I should add that I'm not really qualified to assess the quality of the code. The patch works for me and hasn't caused any problems that I've noticed. I've marked this issue as "code needs review" because it'd be great if at least one other person could test the patch.

mediamash’s picture

mediamash CreditAttribution: mediamash commented

is this patch going to be submitted?

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented
FileSize
258326_token_bookpath_hierarchy_broken_deep.patch3.42 KB

@mediamash - the patch is submitted. What is needs is reviews from users confirming that it looks good and works properly - then it will be applied to the code. Here is what I consider to be a decent review:

So, I think we can agree that 5.x is the proper behavior. I did some tests in 5.x because I didn't entirely understand the appropriate behavior.

1. Pathauto pattern for book pages: [bookpath-raw]/[title-raw]
2. Create a top level book with the title "top level book" which generates the alias "top-level-book"
3. Create a child to that with the title "middle level book" which generates the alias "top-level-book/middle-level-book"
4. Create a child to that with the title "bottom level book" which generates the alias "top-level-book/middle-level-book/bottom-level-book"
5. Create a second child to the middle level book with the title "second bottom" which generates the alias "top-level-book/middle-level-book/second-bottom"
6. Create a child to the second bottom book with the title "super bottom" which generates the alias "top-level-book/middle-level-book/second-bottom/super-bottom"

That all seems as expected to me.

So, for 6.x the same test shows:
1. same
2. same
3. same
4. same
5. same
6. Same except the alias generated is: top-level-book/middle-level-book/super-bottom

So...#6 is the bug, right?

I applied the patch and the alias in #6 was then generated so that it matches what Pathauto created for 5.x.

However, there are some code style and functional problems.

First, comments should be whole sentences with caps and punctuation. Also, variables should ideally be words and not abbreviations (at least when feasible). It removes the use of $menu_name but doesn't remove the assignment of $menu_name from book_token_values. For top level books that are a book, there was a warning about an invalid index in the $trail[0].

There were also a bunch of PHP notices when a book is submitted and the "Book" is set to "<none>". I added a more robust conditional check to the top of book_token_values to try and help that.

It was also lacking some comments.

So...I think this is looking pretty close to being ready now. Can someone else review my patch, please?

mariagwyn’s picture

mariagwyn CreditAttribution: mariagwyn commented

I was having a problem with pathauto and URL aliases in BOTH new book nodes as well as bulk updates. It just wasn't registering [bookpath] or [book] or [bookpath-raw]...no variation. I installed this dev version, and the bulk update did exactly what it was supposed to do. So, seems to work on my setup (new 6.3 install).

Yea! Thanks.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented
Status: Needs review » Fixed

Thanks, mariagwyn!

Also, thanks to bwooster47 for your work on this. I committed it (somewhat accidentally) along with #241288: Coding Standards.

mErilainen’s picture

mErilainen CreditAttribution: mErilainen commented

Doesn't work for me. I have Drupal 6.3 and dev versions of token and pathauto, but when I try to create a sub-page for any book, [bookpath-raw] token is completely ignored. So every sub-page has path to root.

Is there something else what I need to setup? I know that all modules are not yet working with Drupal6, but it's a bit annoying that this doesn't work in D6 when it's working fine in D5. Anyway, good work as always, have to manage with D5 for some time more.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented

could you please state specifically what you did, specifically what happened, and what you would like to happen?

The way you have described it now doesn't help me to understand the problem nor what needs to be fixed.

mErilainen’s picture

mErilainen CreditAttribution: mErilainen commented

I have the same problem as described in this issue. But now I realized that this doesn't apply to the admin account, only for other accounts. Is there any setting which might affect this?

Using Drupal 6.3 and the latest dev versions of Token and Pathauto, I try to create book with hierarchy and matching URLs.
For book nodes I have this setting: [ogname-raw]/[bookpath-raw]/[title-raw]. But when I create a testbook and subpages for it, none of the subpages have any parent in their URL.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Fixed » Closed (fixed)

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