Customizer Menus: Search results can have duplicate items

[valendesigns]

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.

[adamsilverstein]

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.

[valendesigns]

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.

[samuelsidler]

This can wait until 4.3.2 or later.

[valendesigns]

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.

[westonruter]

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.

[valendesigns]

@westonruter Does the Customizer even work in IE8?

[valendesigns]

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

[adamsilverstein]

Replying to valendesigns:

@westonruter Does the Customizer even work in IE8?

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

[westonruter]

@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?

[westonruter]

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

[valendesigns]

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

[westonruter]

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.

[valendesigns]

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

[westonruter]

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

[boonebgorges]

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?

[dlh]

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

[westonruter]

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