WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

#33404 closed defect (bug) (worksforme)

Customizer Menus: Search results can have duplicate items

Reported by: valendesigns Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.3
Component: Customize Keywords: needs-patch
Focuses: administration Cc:

Description

The logic in WP_Customize_Nav_Menus::search_available_items_query is off. It's combining the post_type & term queries, which are both set at a limit of 20, but not taking into account that it should only be retuning a total of 20 items and instead returns 40. By limiting both offsets to 10 it appears to solve the double item issue, but then presents a different challenge. If the height of the browser is taller than the initial 20 items and you try to scroll to load more items it doesn't work. So we need to initiate search results for page 2 to fill the space and make scroll work correctly again. The patch I'm attaching does just that.

Attachments (5)

33404.diff (2.2 KB) - added by valendesigns 5 years ago.
menu-item-search-duplicate-terms-loading.png (40.6 KB) - added by valendesigns 5 years ago.
33404.2.diff (3.9 KB) - added by valendesigns 5 years ago.
33404.3.diff (3.9 KB) - added by valendesigns 5 years ago.
33404.4.diff (3.9 KB) - added by valendesigns 5 years ago.

Download all attachments as: .zip

Change History (26)

@valendesigns
5 years ago

#1 @valendesigns
5 years ago

  • Milestone changed from Awaiting Review to 4.3.1
  • Owner set to valendesigns
  • Status changed from new to assigned

#2 @adamsilverstein
5 years ago

Can you describe steps I can take to reproduce the issue, ie. how to set up the data so i can see duplicates?

This would be a good case for a quinit test to go along with the patch.

#3 @valendesigns
5 years ago

  • Keywords needs-patch needs-unit-tests added; has-patch removed

The above image is from Slack. Exactly why this happens is still a bit of a mystery to me. My best guess, magic!

In all seriousness, I didn't consider why when I was trying to fix it, though now that I've walked away and come back this patch does not fix it completely. It does fix a different issue. But duplicate terms are still showing up. AHHHHHH!!!!!

I'll figure out why and get back to you on that, just not today.

This ticket was mentioned in Slack in #core-customize by sam. View the logs.


5 years ago

#5 @samuelsidler
5 years ago

  • Milestone changed from 4.3.1 to 4.3.2

This can wait until 4.3.2 or later.

@valendesigns
5 years ago

#6 @valendesigns
5 years ago

  • Keywords has-patch needs-testing added; needs-patch needs-unit-tests removed

This is fixed with 33404.2.diff. The issue was the combination of number and offset when using get_terms that when the offset was higher than the number of terms would return all of the terms. I fixed this by passing an array of term IDs to exclude from the query and removing offset.

As well, if the content height is taller than the first page of search results the JS initiates a scroll event to get the second page and fill up the empty space. This is needed because you can't initiate scroll events when the content area is taller than the query items.

#7 @westonruter
5 years ago

I'm noticing self.excludeTerms.filter. It looks like this assumes the existence of Array.prototype.filter but this is not available in IE8. So I think the Underscore _.filter() needs to be used instead here.

#8 follow-up: @valendesigns
5 years ago

@westonruter Does the Customizer even work in IE8?

@valendesigns
5 years ago

#9 @valendesigns
5 years ago

I've updated the patch to use Underscore _.filter().

#10 in reply to: ↑ 8 @adamsilverstein
5 years ago

Replying to valendesigns:

@westonruter Does the Customizer even work in IE8?

Yes, amazingly it does. I do see some warnings in trunk.

#11 @westonruter
5 years ago

@valendesigns I can't seem to be able to reproduce the issue in trunk. What posts and terms do you have in your install which allow the issue to be reproduced?

#12 @westonruter
5 years ago

Oh, one tiny thing: I think array_map() should be used instead of array_filter() since the latter will not sanitize and could let through things unexpectedly. Compare:

<?php
array_filter( array( '1<script>evil</script>' ), 'absint' ) === array( "1<script>evil</script>" );

vs

<?php
array_map( 'absint', array( '1<script>evil</script>' ) ) === array( 1 );

#13 @valendesigns
5 years ago

@westonruter I'll update the patch to use array_map(). To reproduce the issue you need to have the theme unit testing data installed at a minimum, I also have the demo data for WooCommerce, and then do a search that produces less than 20 taxonomies and more than 20 posts so the second page will have the taxonomies repeat with the additional posts. It's difficult to reproduce, but once you do you will see the pattern jump out.

Note: In my case I used as which was common enough to grab a lot of posts but specific enough to limit the taxonomies to mostly aside related stuff for post formats.

Last edited 5 years ago by valendesigns (previous) (diff)

@valendesigns
5 years ago

#14 @westonruter
5 years ago

  • Milestone changed from 4.3.2 to 4.4

#15 @wonderboymusic
5 years ago

  • Owner changed from valendesigns to westonruter

#16 @westonruter
5 years ago

  • Keywords needs-patch added; has-patch needs-testing removed
  • Milestone changed from 4.4 to Future Release
  • Owner westonruter deleted

I'm concerned about using exclude here when there are many terms in the database. If you paginate to high numbers, then the SQL query is going to get huge, and each get_terms() call will require a DB hit and wont be able to be pulled from terms cache like calls using just offsets can be.

The issue was the combination of number and offset when using get_terms that when the offset was higher than the number of terms would return all of the terms. I fixed this by passing an array of term IDs to exclude from the query and removing offset.

@valendesigns So please confirm, is the problem in get_terms() is here specifically:

<?php
        if ( $number && is_array( $terms ) && count( $terms ) > $number ) {
                $terms = array_slice( $terms, $offset, $number );
        }

And specifically the count( $terms ) > $number condition here is causing the problem here, where you would expect that if sending a high offset it would actually result in returning an empty $terms list, whereas right now it is returning all of the terms. unexpectedly.

The condition here was added in [10416] as part of #8832. It seems that this patch perhaps is not doing the right thing.

#17 @valendesigns
5 years ago

Yes, setting a higher offset than the number of terms is the root of the issue.

Last edited 5 years ago by valendesigns (previous) (diff)

#18 follow-up: @westonruter
5 years ago

@boonebgorges do you consider this to be a bug in get_terms()?

#19 in reply to: ↑ 18 @boonebgorges
5 years ago

Replying to westonruter:

@boonebgorges do you consider this to be a bug in get_terms()?

If the issue is as you've described it, then yes. But I can't seem to reproduce. Queries containing a number and an offset will create a SQL clause like LIMIT x,y. If the offset is higher than the number of terms, then SELECT * FROM wp_terms ... LIMIT x,y will return an empty array. https://core.trac.wordpress.org/browser/tags/4.3.1/src/wp-includes/taxonomy.php?marks=1921-1930#L1917

Maybe I'm testing incorrectly?

#20 @dlh
4 years ago

I was able to replicate this behavior in 4.4, but not in 4.5 or trunk.

#21 @westonruter
4 years ago

  • Milestone Future Release deleted
  • Resolution set to worksforme
  • Status changed from assigned to closed

@dlh Thanks for testing! OK, let's close. We can re-open if it re-occurs.

Note: See TracTickets for help on using tickets.