#37782 closed defect (bug) (fixed)
Duplicate Page Entry in View All Pages when generating a Menu
Reported by: | garrett-eclipse | Owned by: | 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)
Change History (65)
#4
@
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".
#5
@
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
#6
@
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.
This ticket was mentioned in Slack in #core by garrett-eclipse. View the logs.
6 years ago
#8
@
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
#9
@
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
#14
@
6 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.
6 years ago
This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.
6 years ago
#18
@
6 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.
#19
@
6 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.
6 years ago
#21
@
6 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
@
6 years ago
Fixed CS issues and versioned the get_post_states
function, and merged tests into single diff
#22
@
6 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&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 — <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.
#23
@
6 years ago
I spoke too soon, I was able to quickly fix the tests and uploaded 37782.8.diff
@
6 years ago
Fixed issue identified in test_should_contain_no_items_without_pages to re-introduce No Items
#24
@
6 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.
6 years ago
#26
@
6 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.
#27
@
6 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
@
6 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.
6 years ago
#30
@
6 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.
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
6 years ago
This ticket was mentioned in Slack in #core-privacy by garrett-eclipse. View the logs.
6 years ago
@
6 years ago
Minor update to address phpDocs and CS. No code change, but still retested to ensure no issues
#34
@
6 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.
- Made the
if ( ! empty( $important_pages ) )
into an elseif. - Dropped
$privacy_policy_page_obj
in favour of the pre-existing$privacy_policy_page
- Added @since 2.7.0 to _post_states
- Minor cleanup for structure
All the best
@
6 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
@
6 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
@
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
@
5 years ago
Test result successful illustrating all three special page names and state is left unaffected by the change
@
5 years ago
Test result of Appearance > Menu to show special page labels as well as no duplicate 'Home' link (original issue)
#38
@
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
@
5 years ago
- Keywords dev-feedback added
*didn't mean to remove dev-feedback
, as @pento mentioned this requires committers review now.
Previously: #13213, #25410.