WordPress.org

Make WordPress Core

Opened 16 months ago

Last modified 11 days ago

#26475 new defect (bug)

Hierarchical meta box display issues when messing around with new terms

Reported by: ericlewis Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.8
Component: Taxonomy Keywords: has-patch
Focuses: Cc:

Description

There are some bugs with the hierarchical taxonomy box, especially when the post is related to only some terms in a hierarchical chain.

Attaching video of the bug.

Steps to reproduce:

  1. Create a new post
  2. In the taxonomy meta box, create an old testament term.
  3. Create a Job term as a child of old testament.
  4. Uncheck old testament.
  5. Add a new term genesis as a child of old testament.
  6. Add a new term numbers as a child of old testament.

Attachments (4)

26475.mov (3.9 MB) - added by ericlewis 16 months ago.
screen recording of the bug
26475.patch (1.9 KB) - added by tyxla 11 days ago.
Display and preserve the full hierarchy of any selected item in the category selection list in the Edit Post screen.
26475.tests.patch (3.5 KB) - added by tyxla 11 days ago.
Clumsy tests for wp_terms_checklist().
26475.tests.2.patch (2.6 KB) - added by tyxla 11 days ago.
Less fragile unit tests - using assertRegExp() instead of assertContains()

Change History (12)

@ericlewis16 months ago

screen recording of the bug

comment:1 @ericlewis16 months ago

  • Summary changed from Breaking the hierarchical meta box to Hierarchical meta box display issues when messing around with new terms

comment:2 @wonderboymusic11 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

So, that bug is weird and terrible - needs a patch, would be good to fix this for 4.0

comment:3 @tyxla11 days ago

I just encountered this issue on a site I was working on, it appears it still keeps occuring. Really weird behavior indeed.

After investigating in-depth, I discovered that this is actually related with the checked_ontop argument of wp_terms_checklist(), which is used for the category selection box in the Edit Post screen.

The problem is that when the checked_ontop argument is enabled (which is true by default), the checked categories are pushed to the top without considering any unselected categories from the hierarchy, which visually breaks the related category hierarchy.

In order to fix this issue, the following should be done when checked_ontop is true:

  1. We should get the ancestor (or the parent from level 0) of each selected category.
  2. The found ancestor should be added to the list of checked categories. Note: this should not make the ancestor itself "checked" (unless it already is), it should only add it to the top of the list, together with the other selected categories.
  3. We should get and add the entire hierarchy of the ancestor to the top of the list as well, preserving the category hierarchy in the process.
  4. We should unset the categories that we've added to the top from the list of categories at the bottom to avoid duplication.

Patch coming up in a minute.

@tyxla11 days ago

Display and preserve the full hierarchy of any selected item in the category selection list in the Edit Post screen.

comment:4 @tyxla11 days ago

  • Keywords has-patch added; needs-patch removed

comment:5 @boonebgorges11 days ago

  • Keywords needs-unit-tests added

Thanks, tyxla. It'd be nice to get some rudimentary test coverage for wp_terms_checklist() (including and especially a test showing the current bug) before we go messing with the hierarchy algorithm.

comment:6 @tyxla11 days ago

  • Keywords needs-unit-tests removed

@boonebgorges, yeah, unit tests would be great for this case indeed. I actually intended to create some tests that would cover that use case, but I quickly got dissuaded.

Basically, what wp_terms_checklist() does is to output the term checkbox list HTML - the <li>s with the <input>s / <label>s and <ul>s for going into hierarchy depth. Also, each checkbox cah be checked, and there can be additional classes and there are many attributes.

This would make any unit tests of the wp_terms_checklist() function very fragile and unstable, unreliable when any changes are done to the output. For example even if a single space is added somewhere in the Walker_Category_Checklist class (it is used for creating the wp_terms_checklist() hierarchy), this would cause these tests to fail.

Therefore you can guess that any tests of that function will be very ugly themselves. That's why I didn't post any test here.

So I'll add the test that I've written for that functionality, but IMHO such tests should not be committed - they're too ugly, clumsy and fragile.

In my opinion, the better way to go with this would be to completely rewrite wp_terms_checklist() to use another proxy function that would only return the term hierarchy. Then we would add tests to that proxy function without bothering with the output HTML. But this would be the subject of another ticket, I think. Let me know if you feel I'm correct about that and I'll open that ticket and gladly take care of it, as well as of its tests.

@tyxla11 days ago

Clumsy tests for wp_terms_checklist().

comment:7 @boonebgorges11 days ago

Yeah, writing tests for this kind of thing is pretty terrible.

Instead of testing the entire HTML output, we can have more focused tests using assertContains() and assertRegExp(). Still not great, but at least not quite as fragile. See eg https://core.trac.wordpress.org/browser/trunk/tests/phpunit/tests/comment/commentForm.php.

I'm not opposed to separating the term hierarchy logic here (and in fact, we have _get_term_hierarchy(), which could be used to do most of the work, I think), but we'll still have to do some tree-walking when building the HTML. So there won't be a great deal of code simplification, though certainly we'll make up for it in DRYness and probably even in performance (since _get_term_hierarchy() grabs a cached array from the options table).

comment:8 @tyxla11 days ago

@boonebgorges you're right, assertRegExp() seems to be a better way to do that, thanks. I'll update the patch in a minute. I'm sure the regexes wont be very pretty as well, but at least the test will be much less fragile.

And concerning separation of the term hierarchy logic - I guess this is not high priority and can be addressed in a separate ticket, if you feel that's necessary.

@tyxla11 days ago

Less fragile unit tests - using assertRegExp() instead of assertContains()

Note: See TracTickets for help on using tickets.