Make WordPress Core

Opened 8 years ago

Closed 5 years ago

Last modified 4 years ago

#37782 closed defect (bug) (fixed)

Duplicate Page Entry in View All Pages when generating a Menu

Reported by: garrett-eclipse's profile garrett-eclipse Owned by: garrett-eclipse's profile garrett-eclipse
Milestone: 5.3 Priority: normal
Severity: normal Version: 3.0
Component: Menus Keywords: has-patch has-unit-tests early dev-feedback
Focuses: ui, administration, privacy Cc:

Description

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

Attachments (20)

37782.diff (875 bytes) - added by garrett-eclipse 6 years ago.
Suppress duplicate front_page menu item
37782.2.diff (4.6 KB) - added by garrett-eclipse 6 years ago.
Second iteration to affect posts_page and privacy_page as well
37782.4.diff (7.6 KB) - added by garrett-eclipse 6 years ago.
Update using postnot_in, and $echo on _post_status
37782.5.diff (7.9 KB) - added by garrett-eclipse 6 years ago.
Update append of _post_status after the esc_html to preserve the span
Screen Shot 2018-09-25 at 12.49.52 AM.png (56.1 KB) - added by garrett-eclipse 6 years ago.
Full Sample
37782.6.diff (8.0 KB) - added by garrett-eclipse 6 years ago.
Adding doc details to post_states functions
37782-test.diff (3.8 KB) - added by birgire 5 years ago.
37782.7.diff (12.7 KB) - added by garrett-eclipse 5 years ago.
Fixed CS issues and versioned the get_post_states function, and merged tests into single diff
37782.8.diff (12.9 KB) - added by garrett-eclipse 5 years ago.
Updated tests that were failing
37782.9.diff (8.9 KB) - added by garrett-eclipse 5 years ago.
Fixed issue identified in test_should_contain_no_items_without_pages to re-introduce No Items
37782.10.diff (9.1 KB) - added by xkon 5 years ago.
Capture d’écran 2019-04-04 à 11.20.02.png (39.7 KB) - added by audrasjb 5 years ago.
Works great on my side (fr_FR Locale)
37782.11.diff (12.7 KB) - added by garrett-eclipse 5 years ago.
Re-introduces missing unit tests to approved patch
37782.12.diff (9.0 KB) - added by garrett-eclipse 5 years ago.
Minor update to address phpDocs and CS. No code change, but still retested to ensure no issues
37782.13.diff (12.9 KB) - added by garrett-eclipse 5 years ago.
Re-introduced unit tests that got dropped in v12 due to me not running svn add.
37782.14.diff (12.7 KB) - added by garrett-eclipse 5 years ago.
Additional Cleanup as suggested by @Birgire.
37782.15.diff (12.7 KB) - added by garrett-eclipse 5 years ago.
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
37782.16.diff (13.9 KB) - added by garrett-eclipse 5 years ago.
Refresh for 5.3.0.
Screen Shot 2019-09-18 at 10.29.26 PM.png (41.8 KB) - added by garrett-eclipse 5 years ago.
Test result successful illustrating all three special page names and state is left unaffected by the change
Screen Shot 2019-09-18 at 10.37.18 PM.png (50.5 KB) - added by garrett-eclipse 5 years ago.
Test result of Appearance > Menu to show special page labels as well as no duplicate 'Home' link (original issue)

Download all attachments as: .zip

Change History (65)

#1 @SergeyBiryukov
8 years ago

  • Component changed from General to Menus

#3 @swissspidy
8 years ago

  • Version changed from 4.6 to 3.0

#4 @mdgl
8 years ago

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".

Last edited 8 years ago by mdgl (previous) (diff)

@garrett-eclipse
6 years ago

Suppress duplicate front_page menu item

#5 @garrett-eclipse
6 years ago

  • Keywords has-patch needs-testing added
  • Owner set to garrett-eclipse
  • Status changed from new to accepted

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

@garrett-eclipse
6 years ago

Second iteration to affect posts_page and privacy_page as well

#6 @garrett-eclipse
6 years ago

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 postsnot_in array on the original query rather than running a foreach to unset posts.

But thoughts on current direction appreciated.

Version 0, edited 6 years ago by garrett-eclipse (next)

This ticket was mentioned in Slack in #core by garrett-eclipse. View the logs.


6 years ago

@garrett-eclipse
6 years ago

Update using postnot_in, and $echo on _post_status

@garrett-eclipse
6 years ago

Update append of _post_status after the esc_html to preserve the span

#8 @garrett-eclipse
6 years ago

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

This version uses the post__not_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

Last edited 6 years ago by garrett-eclipse (previous) (diff)

@garrett-eclipse
6 years ago

Adding doc details to post_states functions

#9 @garrett-eclipse
6 years ago

  • Focuses privacy added

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.

This ticket was mentioned in Slack in #core by garrett-eclipse. View the logs.


6 years ago

This ticket was mentioned in Slack in #core by garrett-eclipse. View the logs.


6 years ago

This ticket was mentioned in Slack in #core-privacy by garrett-eclipse. View the logs.


6 years ago

#13 @SergeyBiryukov
6 years ago

  • Milestone changed from Awaiting Review to 5.0

#14 @peterwilsoncc
5 years ago

  • Milestone changed from 5.0 to 5.1

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

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


5 years ago

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


5 years ago

#17 @pento
5 years ago

  • Milestone changed from 5.1 to 5.2

This patch needs testing and review.

#18 @birgire
5 years ago

  • Keywords has-unit-tests added

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
5 years ago

#19 @birgire
5 years ago

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

This ticket was mentioned in Slack in #core-privacy by garrett-eclipse. View the logs.


5 years ago

#21 @garrett-eclipse
5 years ago

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
5 years ago

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

#22 @garrett-eclipse
5 years ago

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
5 years ago

Updated tests that were failing

#23 @garrett-eclipse
5 years ago

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

@garrett-eclipse
5 years ago

Fixed issue identified in test_should_contain_no_items_without_pages to re-introduce No Items

#24 @garrett-eclipse
5 years ago

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'

This ticket was mentioned in Slack in #core-privacy by garrett-eclipse. View the logs.


5 years ago

@xkon
5 years ago

#26 @xkon
5 years ago

  • Keywords needs-testing removed

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
5 years ago

Works great on my side (fr_FR Locale)

#27 @audrasjb
5 years ago

  • Keywords commit added

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 :-)

#28 @audrasjb
5 years ago

  • Keywords commit removed

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

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


5 years ago

@garrett-eclipse
5 years ago

Re-introduces missing unit tests to approved patch

#30 @garrett-eclipse
5 years ago

  • Keywords commit added

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.

#31 @xkon
5 years ago

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

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


5 years ago

This ticket was mentioned in Slack in #core-privacy by garrett-eclipse. View the logs.


5 years ago

@garrett-eclipse
5 years ago

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

@garrett-eclipse
5 years ago

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

@garrett-eclipse
5 years ago

Additional Cleanup as suggested by @Birgire.

#34 @garrett-eclipse
5 years ago

@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
5 years ago

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

#35 @pento
5 years ago

  • Keywords dev-feedback added
  • Milestone changed from 5.2 to 5.3

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.

#36 @garrett-eclipse
5 years ago

  • Keywords early added

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

This ticket was mentioned in Slack in #core by garrett-eclipse. View the logs.


5 years ago

@garrett-eclipse
5 years ago

Refresh for 5.3.0.

@garrett-eclipse
5 years ago

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

@garrett-eclipse
5 years ago

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

#38 @garrett-eclipse
5 years ago

  • Keywords dev-feedback removed

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.

#39 @garrett-eclipse
5 years ago

  • Keywords dev-feedback added

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

This ticket was mentioned in Slack in #core-privacy by garrett-eclipse. View the logs.


5 years ago

This ticket was mentioned in Slack in #core-committers by whyisjake. View the logs.


5 years ago

#42 @whyisjake
5 years ago

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

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

#43 @garrett-eclipse
4 years ago

  • Keywords needs-dev-note added

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.

#44 @garrett-eclipse
4 years ago

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

#45 @desrosj
4 years ago

  • Keywords commit needs-dev-note removed

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.

Note: See TracTickets for help on using tickets.