WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#12566 closed defect (bug) (fixed)

Menu editor link (URL) disappears (and other menu weirdness)

Reported by: Dickie Owned by: ptahdunbar
Milestone: 3.0 Priority: high
Severity: blocker Version: 3.0
Component: Menus Keywords: accepted
Focuses: Cc:

Description

The new menu editor in 3.0 trunk (is great... and I'm glad to see it, but...) it's broken.
If you add an new menu item from the Categories or Pages, then after saving, the URL is now blank, making it link to nowhere. (it was there before saving)
This does not always seem to be the case, and some items do seem to retain their URL. I have not managed to understand under what circumstances they remain.

I also am having problems with nested pages/categories in the menu editor. I had created a menu with a Category parent at the root of the menu with each of its children underneath it.
After saving and editing some other items, all the categories became a child of the preceding category (i.e. in a linked list syleee)

As great as this feature is (and I really would like to use it) it is completely broken at the moment.

Attachments (1)

nav-menus-custom-url-save.diff (840 bytes) - added by yoavf 5 years ago.
Fix bug where custom urls weren't saved in the menu editor

Download all attachments as: .zip

Change History (35)

comment:1 @Dickie5 years ago

I have also noticed another issue,

Create a menu with a selection of pages (4 or more)
Save
Delete the top item and leave the mouse where it is while the Ajax fires.
Delete the now new top item. (wait to delete)
Now delete the new top item... the item below will now be deleted. It is not not possible to delete the top item, until you save your changes.
(Note this will work at any position, not just the top)

comment:2 @Dickie5 years ago

I've just been looking at which URLs remain, and if you delete all menus and start again, by default the normal menu structure will appear. These URLs will remain, but any added items subsequently will not retain their URLs.

comment:3 @yoavf5 years ago

Probably related #12545

comment:4 @ryan5 years ago

  • Resolution set to fixed
  • Status changed from new to closed

(In [13651]) Menu editor fixes. Props yoavf. fixes #12545 #12566

comment:5 @Dickie5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Sorry, but just tested with latest trunk and it's still the same... (pulled via svn up) The URL portion of added menu items still goes blank after saving changes.
I also cleared the cache on my browser just to be sure.

I also got the linked menu items again.

Recipe.
Start with an auto generated menu from 5 or so pages
Added a category parent and all (7 children)
I then manually had to move all children to be the child of the parent (as by default they are all flat, Note: this also needs fixing)
I then moved the parent up the menu a couple of places.
Then saved.
After the same the menu is screwed up. Some of the child items are now at the root. Some of then are children of each other.

Reopened

comment:6 @voyagerfan57615 years ago

  • Cc WordPress@… added

comment:7 @Dickie5 years ago

Having just tried that recipe again, it did not "quite" do as described, but by just moving parent menu item (with children attached) around and saving you will soon uncover the change in parentage issue.

Also the delete bug mentioned above is still an issue. I can raise another ticket for this if it helps.

@yoavf5 years ago

Fix bug where custom urls weren't saved in the menu editor

comment:8 @yoavf5 years ago

Confirming urls are not saved - the above patch fixes this for custom menu items.

However, it seems that it's intentional for non-custom menu items.
The URLs for non-custom should be displayed and maybe 'disabled' by default.

comment:9 @ProDevStudio5 years ago

Confirming that this patch has NOT worked.

I have a page that I am linking to it show the URL when added to the menu but once I have pressed the "Save All Changes" and go back to it, there is no URL.

comment:10 @yoavf5 years ago

@ProDevStudio Is that a custom page? that patch only fixes custom items, not regular WP pages

comment:11 @automattor5 years ago

(In [13685]) Always use lowercase strings or menu type. Don't double save custom link menu items (remove the AJAX save). see #12566

comment:12 @Dickie5 years ago

That change has fixed the loss of URL bug, but there are still a few remaining issues.

I am trying to create a menu made up of 6 pages and one parent category with 8 children.
If I add the parent and children they all are placed "flat" so have to be moved into the correct hierarchy (this needs changing).
But upon saving the changes are not recognised.

If you move the parent category, then the hierarchy gets completely screwed.

The deleting individual items bug (see previous comment) is still there.

Update...
I HAVE managed to get it to work for me temporarily, but in a very long winded way. This may help in debugging the issue.
Add the category and children... save... move them to the correct place... save, move them "back" to the correct place... save... move them "BACK AGAIN" to the correct place... save.
Now they will finally be in the right place.

comment:13 @mikeschinkel5 years ago

  • Component changed from General to Menus
  • Severity changed from normal to major

I've been spending the past 4+ hours chasing my tail trying to solve the problem of where a menu item for a Page doesn't get a correct URL. I'm running into several things. First, somehow menu_type in postmeta gets 'Page' instead of 'page' and thus $type=='page' does not work as it should. I've tried to track down the source of this and it appears that line 228 on /wp-includes/nav-menu.php sets:

$menu_item->append = _x('Page', 'menu nav item type'); 

I don't at all understand the use for _x() here because it seems that $menu_item->append is only ever used on line 201 of the same file for the item code (why does something that is used internally need to be translated?):

$item .= '<input type="hidden" class="item-type" value="'. esc_attr( $menu_item->append ) .'" />';

IF that's it you can also find similar on lines 244 and 251, respectively:

$menu_item->append = _x('Category', 'menu nav item type');
$menu_item->append = _x('Custom', 'menu nav item type');

However I also found there were errors in the .js files. Most important I found was that item-dbid is not moved over from the queue to the list of menus. I found that wp_update_queue() doesn't get the value for dbid and the append to jQuery('#queue') doesn't have it included. (Question: wouldn't it be better for wp_update_queue() to accept an object rather than ~10 named parameters? As is it is very fragile for future modifications.)

BTW, in (at least) two places there are trailing commas in object definition syntax PHP style that don't work for JS:

-- Line 114 - /wp-admin/js/nav-menu-default-items.dev.js
-- Line 254 - /wp-admin/js/nav-menu-dynamic-functions.dev.js

Of course when I "fix" the problems in the .js files they menu item stops saving so I give up. I've now spent 4 hours trying to trace this down and I guess I just don't understand the logic behind it enough to fix it. (I tend to like code that is more encapsulated than this so I am having a hard time following it.)

Beyond that you find that line 128 in /wp-admin/nav-menus.php produces an "undefined" object_id for a "Page" type:

$object_id = isset( $_POST['item-postmenu'.$k] )? $_POST['item-postmenu'.$k] : 0;

I am just mentally exhausted because I really need to be doing something else today (which this bug was keeping me from doing) and I can't focus on it any more today. I hope this helps whoever wrote the menus. I could really use this getting fixed sooner than later. :-)

Thanks in advance.

comment:14 @mikeschinkel5 years ago

  • Cc mikeschinkel@… added

comment:15 @mikeschinkel5 years ago

  • Severity changed from major to blocker

comment:16 @ptahdunbar5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

Fixed in r13704

comment:17 @Dickie5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Thanks for the work in trying to resolve this, but sorry guys, it's not fixed.

I created a new menu (as the old one had lost its setting since the update)
Added my 6 pages
Added the Category parent with the 8 children (all end up flat)
Moved the child categories to under the parent, and saved.
All children are now children of each other. It is also now not possible to get them to stick as just having 1 ancestor.

We now have some new bugs as well
All pages in the menu do not use the permalinks they appear as

http://www.example.com/?post_type=page&p=43

Categories seem OK and use the permalink structure e.g.

http://www.example.com/category/members/glass-members/

Also now all links in the menu now open link in new window, despite the fact that the setting says open in "current" tab. For info the target of the link is set to "_none" it should either be "_self" or not at all.

The Category list on the right varies from showing the category hierarchy, to being flat, depending on when you add the menu items.

On the plus side the deleting bug noted previously is now fixed. ;)

comment:18 @ryan5 years ago

(In [13714]) Use get_permalink() instead of get_post_permalink(). Limit to published post types. see #11817 #12566

comment:19 @masonjames5 years ago

In [13712] I'm still seeing the navigation issue with child items (After saving and editing some other items, all the categories became a child of the preceding category).

comment:20 @yoavf5 years ago

Something similar is happening to me:

In my case I have a bunch of pages.
Once of the pages (X) has two children (A, and B).

When saving a menu where A and B are both 1st level child of X, Page 'B' always become a child of A.

comment:21 @ptahdunbar5 years ago

  • Keywords accepted added
  • Milestone changed from Unassigned to 3.0
  • Owner set to ptahdunbar
  • Status changed from reopened to assigned

Just to clear things up as this ticket is addressing multiple issues:

  • pages should use their correct permalinks
  • bugs in nav menu parent/child relationships

comment:22 @graynotgrey5 years ago

The following code segment in the nav-menu-template.php starting at line 126 pertains to the indention of the child menus.

		// Indent children
		$last_item = ( count( $menu_items ) == $menu_item->menu_order );
		if ( $last_item || $current_parent != $menu_items[$key + 1]->post_parent ) {
			if ( $last_item || in_array( $menu_items[$key + 1]->post_parent, $parent_stack ) ) {
				$items .= '</li>';
				while ( !empty( $parent_stack ) && ($last_item || $menu_items[$key + 1]->post_parent != $current_parent ) ) {
					$items .= '</ul></li>';
					$current_parent = array_pop( $parent_stack );
				}
			} else {
				array_push( $parent_stack, $current_parent );
				$current_parent = $menu_item->ID;
				$items .= '<ul class="sub-menu">';
			}
		} else {
			$items .= '</li>';
		}

With the help of Firebug I discovered that for some reason, the first option in the If statement is always being skipped over and the <ul class="sub-menu"> in the first else statement is being applied to each sub-item and thus indenting each sub-menu. This is as far as I can take it.

comment:23 @nacin5 years ago

A walker was introduced in [13802]. There's still an edge case that causes a bug, so this is staying open.

comment:24 @Dickie5 years ago

Just tried the new code, and just for information, I know you said there was an edge case bug, but NO items can now be added as children.

comment:25 @Dickie5 years ago

Sorry... Update... I decided to try some more tests, so I deleted the old menu and tried again, it now seems to work...
Thanks

comment:26 @ptahdunbar5 years ago

  • Cc trac@… added
  • Resolution set to fixed
  • Status changed from assigned to closed

Fixed in r13802 :D

comment:27 @Dickie5 years ago

I have only done a small amount of testing, so please don't assume that I was say it was completely sorted... I will do some more over the next few hours.

comment:28 @nacin5 years ago

(In [13803]) Prevent the nav menu walker from walking in circles. Don't allow an object from being a child of itself; if the functionality is desired, a workaround would be custom links. see #11817, see #12566

comment:29 @nacin5 years ago

Last commit was props ptahdunbar, sorry.

That was the edge case I was referring to, fyi.

comment:30 @nacin5 years ago

(In [13804]) Prevent the nav menu walker from walking in circles. Don't allow an object from being a child of itself; if the functionality is desired, a workaround would be custom links. see #11817, see #12566. (Script loader bump for r13802, r13803). props ptahdunbar for the commits.

comment:31 follow-up: @Dickie5 years ago

Looks good to me at the moment... thanks.

I would point out that the drop zone for re-ordering items is very small... and quite difficult to do.

comment:32 in reply to: ↑ 31 @nacin5 years ago

Replying to Dickie:

I would point out that the drop zone for re-ordering items is very small... and quite difficult to do.

I agree, and the last list item could use a drop zone underneath as well. I opened #12675 for this, as this one is closed, and #11817 should probably closed soon as well.

comment:33 follow-up: @pbassham5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Since this update, I haven't been able to get the menu working at all. More info on the beta forms:

http://wordpress.org/support/topic/379231?replies=4

comment:34 in reply to: ↑ 33 @ptahdunbar5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

Replying to pbassham:

Since this update, I haven't been able to get the menu working at all. More info on the beta forms:

http://wordpress.org/support/topic/379231?replies=4

This is a scaling issue and is not related to this ticket, See #12734

Note: See TracTickets for help on using tickets.