Duplicate Page Entry in View All Pages when generating a Menu

[garrett-eclipse]

Hello,

Opening here for developer input. Original support topic;
​https://wordpress.org/support/topic/is-there-a-reason-two-homepages-show-in-menu-editor?replies=2#post-8787393

As @girlieworks mentioned on the support topic WordPress adds an extra checkbox to the top of the page list for the Page that is set as your Static Front Page when you're creating a menu.

In this list it appears as duplicate with the first being prefixed with 'Home:'.

This caused confusion initially for myself as I was setting up a small site where all pages were to be part of the navigation, so when I was building the menu I went to Pages > View All > Select All before adding, this resulting in my Homepage being in the menu twice. Easy enough to remove, but as it wasn't expected hadn't originally noticed the duplicate.

Is that behaviour correct, or should the original entry for the Home page be filtered when the Static Front Page entry is added to the list?

Thank you

[SergeyBiryukov]

Previously: #13213, #25410.

[mdgl]

I agree this is rather confusing. In my case there is no visual indication as to why the additional reference to "Home" has been added to the list of pages (because my front page is also called "Home" - see comment:11:ticket:25410). The customizer, on the other hand adds an additional reference to "Home" but this time it is identified as a custom link rather than a page link, which is at least clearer. Both of these features still persist with the "home" and "front page" confusion, however as the link being added is actually a reference to the "front page" (often the site home) rather than the WP concept of "home".

[garrett-eclipse]

Suppress duplicate front_page menu item

[garrett-eclipse]

I've posted an initial loop to suppress the duplicate home entry and would appreciate your review @SergeyBiryukov

[garrett-eclipse]

Second iteration to affect posts_page and privacy_page as well

[garrett-eclipse]

Reviewing this further I thought the implementation would benefit from expanding to include page_for_posts and wp_page_for_privacy_policy as found in _post_states();
​https://github.com/WordPress/WordPress/blob/56c162fbc9867f923862f64f1b4570d885f1ff03/wp-admin/includes/template.php#L1908-L1920

Posted a new concept.

Sadly I had to create a get_post_status method instead of just using _post_states as it just echos and I needed to append. Was that the right approach? Or should I have updated it to simply use a param default $echo = true.

Also am going to revise I believe to use the posts__not_in array on the original query rather than running a foreach to unset posts.

But thoughts on current direction appreciated.

[garrett-eclipse]

Update using postnot_in, and $echo on _post_status

[garrett-eclipse]

Update append of _post_status after the esc_html to preserve the span

[garrett-eclipse]

Full Sample

[garrett-eclipse]

I've just put out another revision as [37782.5.diff]

This version uses the postnot_in argument on the WP_Query. But I had to do a check when the only pages returned are 'important' as so would be suppressed form the query making pagination fail, in this case I drop the suppress and prepend and just rely on the WP_Query results.

I've also updated the _post_status to support an $echo param.

And attached is a screen from my test environment.

Feedback and direction appreciated

[garrett-eclipse]

Adding doc details to post_states functions

[garrett-eclipse]

Added a privacy focus as it would be a small win for the Privacy team to expose the Privacy Policy page so users directed to add the page to their menu find it easily.

[peterwilsoncc]

Moving to the 5.1 milestone due to the WordPress 5.0 focus on the new
editor (Gutenberg).

[pento]

This patch needs testing and review.

[birgire]

37782-test.diff​ is a first draft for wp_nav_menu_item_post_type_meta_box() tests used by the meta boxes on the /wp-admin/nav-menus.php admin page.

Contains the following test cases:

• The function should contain no items without pages
• The function should include a page only once, when viewing all pages.
• The function should include the front page only once, when viewing all pages.

[birgire]

ps: another approach might be to avoid duplications when we click on "Select All" (javascript) or when we view (PHP):

/wp-admin/nav-menus.php?page-tab=all&selectall=1#posttype-page

The PHP case is here:

https://core.trac.wordpress.org/browser/tags/5.0.3/src/wp-admin/includes/nav-menu.php#L552

and here the JS case:

https://core.trac.wordpress.org/browser/tags/5.0.3/src/wp-admin/js/nav-menu.js#L1085

[garrett-eclipse]

Thanks @birgire I'll test out the tests when I get a chance here.

As to the alternate approach, yes we could deal with duplicates during the selection but with my proposed approach there'd be no need as it removes the duplicates from the list you're selecting from. As well those two approaches don't resolve the original issue which was the source list has two entries for home which is what confused the user.

All the best

[garrett-eclipse]

Fixed CS issues and versioned the get_post_states function, and merged tests into single diff

[garrett-eclipse]

Thanks for the tests @birgire

I've updated the 37782.7.diff patch merging in your tests and fixing a few CS issues and setting get_post_states function to 5.2.0.

Running the tests I'm getting two failures;

There were 2 failures:

1) Tests_Menu_WpNavMenuItemPostTypeMetaBox::test_should_contain_no_items_without_pages
Failed asserting that '	<div id="posttype-page" class="posttypediv">\n
		<ul id="posttype-page-tabs" class="posttype-tabs add-menu-item-tabs">\n
			<li  class="tabs">\n
				<a class="nav-tab-link" data-type="tabs-panel-posttype-page-most-recent" href="#tabs-panel-posttype-page-most-recent">\n
					Most Recent				</a>\n
			</li>\n
			<li >\n
				<a class="nav-tab-link" data-type="page-all" href="#page-all">\n
					View All				</a>\n
			</li>\n
			<li >\n
				<a class="nav-tab-link" data-type="tabs-panel-posttype-page-search" href="#tabs-panel-posttype-page-search">\n
					Search				</a>\n
			</li>\n
		</ul><!-- .posttype-tabs -->\n
\n
		<div id="tabs-panel-posttype-page-most-recent" class="tabs-panel tabs-panel-active">\n
			<ul id="pagechecklist-most-recent" class="categorychecklist form-no-clear">\n
							</ul>\n
		</div><!-- /.tabs-panel -->\n
\n
		<div class="tabs-panel tabs-panel-inactive" id="tabs-panel-posttype-page-search">\n
						<p class="quick-search-wrap">\n
				<label for="quick-search-posttype-page" class="screen-reader-text">Search</label>\n
				<input type="search" class="quick-search" value="" name="quick-search-posttype-page" id="quick-search-posttype-page" />\n
				<span class="spinner"></span>\n
				<input type="submit" name="submit" id="submit-quick-search-posttype-page" class="button button-small quick-search-submit hide-if-js" value="Search"  />			</p>\n
\n
			<ul id="page-search-checklist" data-wp-lists="list:page" class="categorychecklist form-no-clear">\n
						</ul>\n
		</div><!-- /.tabs-panel -->\n
\n
		<div id="page-all" class="tabs-panel tabs-panel-view-all tabs-panel-inactive">\n
						<ul id="pagechecklist" data-wp-lists="list:page" class="categorychecklist form-no-clear">\n
							</ul>\n
					</div><!-- /.tabs-panel -->\n
\n
		<p class="button-controls wp-clearfix">\n
			<span class="list-controls">\n
				<a href="\n
				?page-tab=all&#038;selectall=1				#posttype-page" class="select-all aria-button-if-js">Select All</a>\n
			</span>\n
\n
			<span class="add-to-menu">\n
				<input type="submit" class="button submit-add-to-menu right" value="Add to Menu" name="add-post-type-menu-item" id="submit-posttype-page" />\n
				<span class="spinner"></span>\n
			</span>\n
		</p>\n
\n
	</div><!-- /.posttypediv -->\n
	' contains "No items".

/Users/garretthyder/WordPress/37782-suppress_duplicate_home/tests/phpunit/tests/menu/wpNavMenuItemPostTypeMetaBox.php:40

2) Tests_Menu_WpNavMenuItemPostTypeMetaBox::test_should_contain_front_page_only_once_when_viewing_all
Failed asserting that '<div id="page-all" class="tabs-panel tabs-panel-view-all tabs-panel-inactive">					<ul id="pagechecklist" data-wp-lists="list:page" class="categorychecklist form-no-clear">				<li><label class="menu-item-title"><input type="checkbox" class="menu-item-checkbox" name="menu-item[-7][menu-item-object-id]" value="1307" /> My Test Page &mdash; <span class='post-state'>Front Page</span></label><input type="hidden" class="menu-item-db-id" name="menu-item[-7][menu-item-db-id]" value="0" /><input type="hidden" class="menu-item-object" name="menu-item[-7][menu-item-object]" value="page" /><input type="hidden" class="menu-item-parent-id" name="menu-item[-7][menu-item-parent-id]" value="0" /><input type="hidden" class="menu-item-type" name="menu-item[-7][menu-item-type]" value="post_type" /><input type="hidden" class="menu-item-title" name="menu-item[-7][menu-item-title]" value="My Test Page" /><input type="hidden" class="menu-item-url" name="menu-item[-7][menu-item-url]" value="http://example.org/" /><input type="hidden" class="menu-item-target" name="menu-item[-7][menu-item-target]" value="" /><input type="hidden" class="menu-item-attr_title" name="menu-item[-7][menu-item-attr_title]" value="" /><input type="hidden" class="menu-item-classes" name="menu-item[-7][menu-item-classes]" value="" /><input type="hidden" class="menu-item-xfn" name="menu-item[-7][menu-item-xfn]" value="" /></li>		</ul>					</div>' contains "> Home: My Test Page</label>".

/Users/garretthyder/WordPress/37782-suppress_duplicate_home/tests/phpunit/tests/menu/wpNavMenuItemPostTypeMetaBox.php:118

Would you mind taking a look.

The test_should_contain_no_items_without_pages test is checking for 'No items' but when there's no pages the list is simply empty and doesn't contain the 'No items' text.

The test_should_contain_front_page_only_once_when_viewing_all fails as the labelling changed.

[garrett-eclipse]

Updated tests that were failing

[garrett-eclipse]

I spoke too soon, I was able to quickly fix the tests and uploaded 37782.8.diff

[garrett-eclipse]

Fixed issue identified in test_should_contain_no_items_without_pages to re-introduce No Items

[garrett-eclipse]

After some manual testing I realized that the issue originally identified in test_should_contain_no_items_without_pages was a valid issue so updated the patch to usesuppress_page_ids instead of important_pages in the check for No Items as important_pages always contains at minimum the Home object placeholder menu item.
37782.9.diff addresses this issue and reverts the test_should_contain_no_items_without_pages test to check for 'No items'

[xkon]

37782.9.diff​ was applying fine and manual testing looked good on my end, thanks @garrett-eclipse and @birgire !

In 37782.10.diff​ I've made some really minor CS fixes that I've spotted, hopefully I didn't miss anything else.

[audrasjb]

Works great on my side (fr_FR Locale)

[audrasjb]

Hi and thanks for all the work on that ticket!

I tested 37782.10.diff and the patch applies cleanly on my side.

Nothing has changed on the customizer but the customizer doesn't show duplicate items so it looks fine on my side.

Let's mark that ticket as commit so it can land in 5.2. I think we are ready now :-)

[audrasjb]

Oops @xkon can you please re-add unit-tests that were here in https://core.trac.wordpress.org/attachment/ticket/37782/37782.8.diff ?

[garrett-eclipse]

Re-introduces missing unit tests to approved patch

[garrett-eclipse]

Thanks @xkon for the testing and CS update.

@audrasjb I appreciate the testing and catching the missing tests, that was my fault I'd started with a clean install between diff 8 & 9 and forgot to svn add the new tests file. It's included with the last revisions I'd mentioned in #9.
*Also thanks for noting the Customizer Menu, I did overlook that. I opened a separate ticket (#46829) to address that in a future release.

@birgire thank you for your testing and unit tests. You'd provided some additional feedback and I wanted to address that in order to not leave any loose ends;

• Performance consideration using post__not_in on a large db.
• Could it behave like sticky posts, without modifying the query, where duplications only removed (via PHP) on the first page.

This I assume stems from the WPVIP articlet (​https://vip.wordpress.com/documentation/performance-improvements-by-removing-usage-of-post__not_in/);

Using post__not_in means that the query cache hitrate will often be a lot lower. It can also make the query slower if the exclusion list is large.

The hitrate issue shouldn't be a problem as once pages are set in options they'll be fairly consistent. And with the exclusion list having a max of 3 the list shouldn't be too large to slow down the query.

• Introduces new function get_post_states(). Name too general? Is there any guideline on when to use wp_ function name prefix? Should it be private? Didn't find such a function in wp.org plugin directory though.

There exists get_post_status(), get_post_statuses() and get_post_status_object() (not the same functionality though)

The new function was a way to splice an existing one to serve two needs which I felt better than creating a completely unique functionality for this. The get_* methods in core don't provide the wp_ prefix in most cases. And I expect it can be useful in other places such as Customizer Menu.

As the latest patch just re-introduces the missing unit tests going to move this back to commit but would love a final re-test from interested parties.

[xkon]

Everything went fine on my end! I believe it's good to go ( cc @audrasjb ).

[garrett-eclipse]

Minor update to address phpDocs and CS. No code change, but still retested to ensure no issues

[garrett-eclipse]

Re-introduced unit tests that got dropped in v12 due to me not running svn add.

[garrett-eclipse]

Additional Cleanup as suggested by @Birgire.

[garrett-eclipse]

@birgire provided some great feedback via Slack which I've addressed in 37782.14.diff

The changes don't change functionality but makes the code more concise.

1. Made the if ( ! empty( $important_pages ) ) into an elseif.
2. Dropped $privacy_policy_page_obj in favour of the pre-existing $privacy_policy_page
3. Added @since 2.7.0 to _post_states
4. Minor cleanup for structure

All the best

[garrett-eclipse]

Minor CS fix and updated _post_states to 2.7.0 as it was the filter introduced in 2.8.0 but the function was 2.7.0

[pento]

This patch needs reviewing by a committer, it's some fairly old code to be shuffling around.

With 5.2 RC1 due out today, I'm bumping this ticket to 5.3.

[garrett-eclipse]

Marking this early in hopes to have a committer (potentially @SergeyBiryukov / @pento) review early in the 5.3 process as am hoping to reuse the changed function in #46829.
Also note although the old code is being shuffled around the _post_states function signature stays basically the same with an additional option w/ default to control the echo behaviour. The return, if default is left alone, stays the same so shouldn't have a huge back-compat impact.
Thank you

[garrett-eclipse]

Refresh for 5.3.0.

[garrett-eclipse]

Test result successful illustrating all three special page names and state is left unaffected by the change

[garrett-eclipse]

Test result of Appearance > Menu to show special page labels as well as no duplicate 'Home' link (original issue)

[garrett-eclipse]

Refresh 37782.16.diff to update version and have patch reapply successfully. Unit tests are running properly and provided test results of the area of focus as well as the other place the _post_states function is used to illustrate no reversion or change.

[garrett-eclipse]

*didn't mean to remove dev-feedback, as @pento mentioned this requires committers review now.

[whyisjake]

As part of [46309] this ticket was fixed.

Menus: Duplicate Page Entry in View All Pages when generating a Menu

Simplifies the interface in menu creation.

Fixes #37782

Props garrett-eclipse, mdgl, birgire, xkon, audrasjb, pento, girlieworks

[garrett-eclipse]

Related: #46829
Now that get_post_states is in core I've used it to refresh #46829 and hopefully it can also land in 5.3

*Also flagging this for dev-note as it introduced a new function.

[garrett-eclipse]

Related #49374 (Applies the special page names to their menu accordions once added to the menu)

[desrosj]

As far as I can tell, there was no dev note published for this ticket. I am going to remove the needs-dev-note keyword as the ship has sailed.