WordPress.org

Make WordPress Core

Opened 11 years ago

Closed 11 years ago

Last modified 10 years ago

#11575 closed defect (bug) (wontfix)

Category search bug

Reported by: arena Owned by: dd32
Milestone: Priority: normal
Severity: major Version: 2.9
Component: Taxonomy Keywords: 2nd-opinion dev-feedback
Focuses: Cc:

Description

with 2.9

OnTheFly013.jpg : normal list

OnTheFly014.jpg : when searching string 't'

+ the code needs a strong review (echo's beeing trapped in a buffer that is then echoed ...)

Attachments (11)

OnTheFly013.jpg (125.5 KB) - added by arena 11 years ago.
list ok
OnTheFly014.jpg (152.1 KB) - added by arena 11 years ago.
search ko
OnTheFly015.jpg (130.7 KB) - added by arena 11 years ago.
new search for 't'
OnTheFly016.jpg (127.2 KB) - added by arena 11 years ago.
new search for '02'
#11575.patch (120.6 KB) - added by arena 11 years ago.
patch
11575.patch (5.7 KB) - added by arena 11 years ago.
my#11838.patch (4.6 KB) - added by arena 11 years ago.
code shorter than long speeches
my#11838.2.patch (4.4 KB) - added by arena 11 years ago.
my#11838.3.patch (4.3 KB) - added by arena 11 years ago.
last code optimization
11575.diff (5.8 KB) - added by dd32 11 years ago.
prepend-example.png (22.2 KB) - added by dd32 11 years ago.

Download all attachments as: .zip

Change History (38)

@arena
11 years ago

list ok

@arena
11 years ago

search ko

#1 follow-up: @arena
11 years ago

  • Keywords has-patch added

#2 in reply to: ↑ 1 @arena
11 years ago

  • Keywords has-patch removed

Replying to arena:

#4 @ryan
11 years ago

  • Milestone changed from 2.9.1 to 3.0

This is too big for 2.9.1, moving to 3.0.

#5 @arena
11 years ago

@ryan

anyway found a new test that breaks the code patched

imagine searching 'ss' in the following list (stars are for levels)

  • categ 01

ss categ 02
* categ 03
ss categ 04

search will retrieve (ss categ 02, ss categ 04)

code patched will display

  • categ 01

ss categ 02

and ss categ 04 will never appear.

working on it !!!

#6 @arena
11 years ago

definitive patch !

solve all cases

#7 @arena
11 years ago

  • Keywords has-patch added

#8 @arena
11 years ago

last version of patch contains some code and db access optimization !

@arena
11 years ago

new search for 't'

@arena
11 years ago

new search for '02'

@arena
11 years ago

patch

#9 @arena
11 years ago

no big changes in patch, just code optimization.

#10 @arena
11 years ago

  • Keywords tested added

#11 @arena
11 years ago

  • Milestone changed from 3.0 to 2.9.2

#12 @arena
11 years ago

Apparently someone did a code review as mentionned in the original bug description.
However this was not fixing the search bug.

File #11575.patch must be discarded

File 11575.patch is the last patch from wordpress trunk asof 2010/01/12

#13 @nacin
11 years ago

Shouldn't we be worrying about backwards compat with the rearranging of arguments in cat_rows() and _cat_rows()?

A core dev has already moved this from a 2.9.x point release to 3.0 previously. Unless this is a 2.9 regression this should be returned to 3.0.

Also, in general it safe and arguably better to omit compressed styles and scripts from patches, as it is easier to review the patches and since the committer will re-compress anyway.

@arena
11 years ago

#14 @arena
11 years ago

@nacin

  • 11575.patch omitting compressed styles.
  • yes this is a 2.9 regression
  • cat_rows() : no worry about arguments changed. This function is called once /wp-admin/categories.php.
  • _cat_rows() : no worry about arguments changed. This function is called twice, once in cat_rows() (see above) and once by itself for recursivity.
  • _cat_row arguments unchanged : retrieves the category table row in the category list and for ajax

#15 @arena
11 years ago

  • Owner changed from filosofo to ryan
  • Status changed from new to assigned

After a short talk with filosofo (owner of this ticket), i change the owner to ryan to get a chance to have it commited.

thank you

#16 @dd32
11 years ago

  • Keywords has-patch tested removed
  • Milestone changed from 2.9.2 to 3.0
  • Owner changed from ryan to dd32
  • Priority changed from high to normal
  • Status changed from assigned to accepted

Just taking ownership of this as it ties into #11838.

The attached patches have a issue with pagination and a few other edge cases that i'd have to try before confirming.

Setting to 3.0 as any cleanups that remain from this will not be 2.9.x material.

Can you explain what is the regression exactly? I've not been able to work out what previously worked, and now doesnt. Is it simply the weird ordering of the table (and children)?

#17 @arena
11 years ago

@dd32

read ticket #11838

hard to show you screenshots on nightly build wordpress because categories admin page displays a left column with tags and a right column with categories. Waiting for the admin page to be more stable.

However i noticed the search form does not works properly as hierarchy is not respected !

#18 @dd32
11 years ago

  • Keywords 2nd-opinion dev-feedback added

hard to show you screenshots on nightly build wordpress because categories admin page displays a left column with tags and a right column with categories.

The strings may say Tags/Categories but the terms listed will all be categories or whatever else you're expecting. Its just the strings havnt been updated.

However i noticed the search form does not works properly as hierarchy is not respected !

No, I specifically disabled the hierachy with the latest changes there, In that case, simply as it was completely broken, which it seems is what you're saying here :) - My reasoning being that you cant display half of a hierachy, My opinion was that you should only see a list of matching nodes, not their parents.

From what i can gather, http://core.trac.wordpress.org/attachment/ticket/11575/OnTheFly013.jpg is the original listing, http://core.trac.wordpress.org/attachment/ticket/11575/OnTheFly014.jpg is after the search?

http://core.trac.wordpress.org/attachment/ticket/11575/OnTheFly015.jpg and http://core.trac.wordpress.org/attachment/ticket/11575/OnTheFly016.jpg are how you'd expect it to be shown? (With parents dulled to show they're not part of the results)

2nd-opinion: how does everyone else expect the category searching to work? just show matching elements? show matching elements with their parents?

#19 @nacin
11 years ago

May be space prohibitive, but why not show parents inline in search results?

i.e. if the search will return "Hawk," then it shows:

Animals — Birds — Hawk

We could possibly show that with line breaks, just keeping them in the same table row:

Animals
— Birds
— — Hawk

#20 @arena
11 years ago

@dd32

First, do you want this code to work for any taxonomy (with hierarchy or not) ?

Current rule :
in wp 2.9.1 in _cat_rows there is a comment :
If the page starts in a subtree, print the parents.

Reminder :
A sub category can have the same name with different parents (not the same slug !!)

Your approach :
Searching this sub category would retrieve two rows without the ability to identifed them (unless i memorized the slug, who does!)

Other points on 2.9.1 code :
1) The search is performed correctly and retrieves the categories matching the search criterias. The point is that the recursive code is a mess, and parent and childs are not displayed properly (see OnTheFly014.jpg in that ticket).
2) Not sure current code handles correctly a hierarchy of more than 20 parents (current list limit), as all categories displayed are tallied (counted).

My approach :
a) based on the retrieved list of categories (with or without search => get_categories function),
b) the retrieved array is sliced (for pagination)
c) all found categories have an attribute added : _found
d) parent categories not in the sliced array are added.
e) then the recursive process does its job.
f) so the retrieved list can have more than 20 rows but that is necessary to allow full identification of the hierarchy.

@ nacin

Your proposal would only apply when searching categories ! not sure Jane would appreciate this !

@arena
11 years ago

code shorter than long speeches

#21 @dd32
11 years ago

All that strange logic that you're removing is to do with paging, not searching.

// If the page starts in a subtree, print the parents.

Thats related to paging, If the page starts with a subchild, Print the parents (Even if they were on the previous page)

How do you deal with paging in that code? I dont see any at all.

Setting the offset + number is beneficial for non-hierachial taxonomies, as it reduces memory load.

I agree that it could be made much more efficient by lazy-loading the required terms, I'm not sure how that'll work with paging though, infact, i really cant see how that'll work with paging well.

Not sure current code handles correctly a hierarchy of more than 20 parents (current list limit)

The same happens if you have the limit set to 5. or 50.

I agree with Nacin here actually, Skip the rows, and simply display the hierachy inline with the result. It may take a bit of UI work to get something that displays it whilst not removing the context of it, but its doable. And a lot cleaner than displaying all the parent nodes.

You're not loosing any information about the search result, mearly adding context to it. It'll work with any taxonomy.

Your proposal would only apply when searching categories !

Not sure that matters, Its only categories (hierachial taxonomies) which are affected..

#22 @arena
11 years ago

All that strange logic that you're removing is to do with paging, not searching.

// If the page starts in a subtree, print the parents.

Thats related to paging, If the page starts with a subchild, Print the parents (Even if they were on the previous page)

How do you deal with paging in that code? I dont see any at all.

dealing with paging with this line :

$terms = array_slice($_terms, $start, $pagesize, true);

Setting the offset + number is beneficial for non-hierachial taxonomies, as it reduces memory load.

OK ! i will update my patch.

I agree that it could be made much more efficient by lazy-loading the required terms, I'm not sure how that'll work with paging though, infact, i really cant see how that'll work with paging well.

see above !

Not sure current code handles correctly a hierarchy of more than 20 parents (current list limit)

The same happens if you have the limit set to 5. or 50.

True

I agree with Nacin here actually, Skip the rows, and simply display the hierachy inline with the result. It may take a bit of UI work to get something that displays it whilst not removing the context of it, but its doable. And a lot cleaner than displaying all the parent nodes.

You're not loosing any information about the search result, mearly adding context to it. It'll work with any taxonomy.

I personnally don't agree on this point. Having two different ways to display hierarchical taxonomies whether you are searching or not looks weird to me. I'd like to know what are Jane thoughts on this topic.

Your proposal would only apply when searching categories !

Not sure that matters, Its only categories (hierachial taxonomies) which are affected..

And any other hierarchical taxonomies ...

currently updating my patch.

@arena
11 years ago

@arena
11 years ago

last code optimization

@dd32
11 years ago

#23 @dd32
11 years ago

attachment 11575.diff added

  • Implements nacin's idea of prepended parent names.

Having two different ways to display hierarchical taxonomies whether you are searching or not looks weird to me

I guess i'm looking at it from a different perspective.

I look at a hierarchy as-is to see an overall expectation of what it looks like. If I'm searching for something, I want that specific item; I don't need to see Its Childs/Siblings - But the parents are useful information.

Now, Whilst the parent is useful information - its providing context, nothing more. I'm not interested in the hierarchy but i do need to know where it stands in the context of it all, Thus the prepending of the parent names.

Now thats the way I think, I am 100% open to displaying it in a better way, Its just that my opinion says to me mine is 'better' (I do concede that anything is better than the current arrangement!), and would love to get some UI perspective on it.

@dd32
11 years ago

#24 @arena
11 years ago

I do concede that anything is better than the current arrangement!

???

and would love to get some UI perspective on it.

ask jane

#25 @dd32
11 years ago

I do concede that anything is better than the current arrangement!

???

By that i was refering to the mis-matched parent-childs.

ask jane

Have already passed it up the chain.

#26 @arena
11 years ago

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

So closing this ticket as wontfix

  • conflicting with #11838
  • your diff ( 11575.diff ) has nothing to do with this ticket.

#27 @nacin
10 years ago

  • Milestone 3.0 deleted
Note: See TracTickets for help on using tickets.