error when moving menu links [#162095]

Comment	File	Size	Author
#10	menu_link_move_4.patch	3.39 KB	pwolanin

#5	menu_link_move_3.patch	2.88 KB	pwolanin

#4	menu_link_move_2.patch	2.82 KB	pwolanin

#3	menu_link_move_1.patch	2.44 KB	pwolanin

Comment #1

pwolanin CreditAttribution: pwolanin commented 25 July 2007 at 21:08

Category:

task

» bug

Log in or register to post comments

Comment #2

pwolanin CreditAttribution: pwolanin commented 25 July 2007 at 22:20

Ok, I found the problem - here's an example up the update query:
SET menu_name = '%s', depth = depth + %d, p1 = %d, p2 = %d, p3 = p2, p4 = p3, p5 = p4, p6 = p5

Apparently MySQL is using the new value of p2 to update p3, and then then new value of p3 (now == p2) to update p4, etc. So the arguments need to be written in the opposite order for this case (moving to a deeper level).

Something like:
SET menu_name = '%s', depth = depth + %d, p6 = p5, p5 = p4, p4 = p3, p3 = p2, p2 = %d, p1 = %d

Log in or register to post comments

Comment #3

pwolanin CreditAttribution: pwolanin commented 25 July 2007 at 22:50

Assigned:	Unassigned	» pwolanin
Status:	Active	» Needs review

File	Size
menu_link_move_1.patch	2.44 KB

rewrite of this function - seems to solve the issue. The parameters of the query get reordered as needed.

Log in or register to post comments

Comment #4

pwolanin CreditAttribution: pwolanin commented 26 July 2007 at 02:09

File	Size
menu_link_move_2.patch	2.82 KB

slightly better patch - simplifies the update query so it should use the available index and fixes up doxygen docs.

Log in or register to post comments

Comment #5

pwolanin CreditAttribution: pwolanin commented 26 July 2007 at 02:38

File	Size
menu_link_move_3.patch	2.88 KB

more cleanup from suggestions by Karoly.

Log in or register to post comments

Comment #6

chx CreditAttribution: chx commented 26 July 2007 at 02:54

Status:

Needs review

» Reviewed & tested by the community

Log in or register to post comments

Comment #7

hswong3i CreditAttribution: hswong3i commented 26 July 2007 at 04:05

i check it with latest oracle driver, with menus:

1
  - 1-1
    - 1-1-1
  - 1-2
2

with following tables value:

MLID 	PLID 	P1 	P2 	P3 	P4 	P5 	P6 	LINK_TITLE
209 	0 	209 	0 	0 	0 	0 	0 	1
210 	0 	210 	0 	0 	0 	0 	0 	2
211 	209 	209 	211 	0 	0 	0 	0 	1-1
212 	209 	209 	212 	0 	0 	0 	0 	1-2
213 	211 	209 	211 	213 	0 	0 	0 	1-1-1

after update to:

data becomes:

MLID 	PLID 	P1 	P2 	P3 	P4 	P5 	P6 	LINK_TITLE
209 	0 	209 	0 	0 	0 	0 	0 	1
210 	0 	210 	0 	0 	0 	0 	0 	2
211 	209 	209 	211 	0 	0 	0 	0 	1-1
212 	213 	209 	211 	213 	212 	0 	0 	1-2
213 	211 	209 	211 	213 	0 	0 	0 	1-1-1

i hope it should be fine, even without this patch, in case of oracle :)

Log in or register to post comments

Comment #8

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 26 July 2007 at 08:06

Status:

Reviewed & tested by the community

» Needs work

- "Update the the children" does seem to be a typo
- The last query in the patch could use single quotes instead of double quotes
- I don't see that either chx or hswong3i actually tested the patch; hswong3i specifically mentions the behaviour *before* this patch

Log in or register to post comments

Comment #9

hswong3i CreditAttribution: hswong3i commented 26 July 2007 at 08:59

pwolanin ask me to test the case without patch, as he hope to clarify if it is really required for oracle driver,too ;)

Log in or register to post comments

Comment #10

pwolanin CreditAttribution: pwolanin commented 26 July 2007 at 22:53

Status:

Needs work

» Needs review

File	Size
menu_link_move_4.patch	3.39 KB

fixes doxygen comments.

Log in or register to post comments

Comment #11

pwolanin CreditAttribution: pwolanin commented 27 July 2007 at 00:58

not- chx tested before setting RTBC above.

Also, on a side note, this won't be an issue for SQLite since during updates "All expressions are evaluated before any assignments are made. "

Log in or register to post comments

Comment #12

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 27 July 2007 at 15:14

It would help if chx would not only set the issue to RTBC, but would care to write a few kinds words what he has done and found. If I see a patch set to RTBC without a comment, I don't see a reason not to set it back to CNR immediately.

Log in or register to post comments

Comment #13

eaton CreditAttribution: eaton commented 27 July 2007 at 22:39

I've taken some time to poke through reproducing the bug, and tested the fix, and the changes definitely resolve the issue as I can see it without breaking other stuff. I've taken a look at the code itself and it doesn't appear broken in a visible way, but I will be the first to admit that I don't have a deep understanding of the internals of the menu system yet.

I'll take some time to pore over it in more detail when I'm able to tonight -- without that I won't RTBC, but I can say the bug is reproducable and the patch fixes it reliably.

Log in or register to post comments

Comment #14

pwolanin CreditAttribution: pwolanin commented 28 July 2007 at 02:01

Chad (hunmonk) was kind enough to check for this bug on PostgreSQL, and it seems not to occur there.

Here's the test: move "Create content" (path = 'node/add') and make it a child of "My Account". Run the query "select * from menu_links where link_path like 'node/add%';"

without the patch, the relevant {menu_links} data from his PostgreSQL table (correct p1 .. p6):

menu_name  | mlid | plid |   link_path    | p1 | p2 | p3  | p4 | p5 | p6 |   link_title 
-----------+------+------+----------------+----+----+-----+----+----+----+----------------
navigation |   99 |   10 | node/add/page  | 22 | 10 |  99 |  0 |  0 |  0 | Page         
navigation |  100 |   10 | node/add/story | 22 | 10 | 100 |  0 |  0 |  0 | Story        
navigation |   10 |   22 | node/add       | 22 | 10 |   0 |  0 |  0 |  0 | Create content

without the patch the relevant {menu_links} data in my MySQL 4.1 table, exhibiting the bug:

menu_name  | mlid | plid | link_path      | p1 | p2 | p3 | p4 | p5 | p6 | link_title 
-----------+------+------+----------------+----+----+----+----+----+----+----------------
navigation |   10 |   22 | node/add       | 22 | 10 |  0 |  0 |  0 |  0 | Create content
navigation |   99 |   10 | node/add/page  | 22 | 10 | 10 | 10 | 10 | 10 | Page  
navigation |  100 |   10 | node/add/story | 22 | 10 | 10 | 10 | 10 | 10 | Story