Opened 11 years ago
Last modified 5 years ago
#26475 new defect (bug)
Hierarchical meta box display issues when messing around with new terms
Reported by: | ericlewis | Owned by: | |
---|---|---|---|
Milestone: | 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:
- Create a new post
- In the taxonomy meta box, create an
old testament
term. - Create a
Job
term as a child ofold testament
. - Uncheck
old testament
. - Add a new term
genesis
as a child ofold testament
. - Add a new term
numbers
as a child ofold testament
.
Attachments (4)
Change History (12)
#1
@
11 years ago
- Summary changed from Breaking the hierarchical meta box to Hierarchical meta box display issues when messing around with new terms
#2
@
11 years 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
#3
@
10 years 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
:
- We should get the ancestor (or the parent from level 0) of each selected category.
- 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.
- 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.
- 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.
@
10 years ago
Display and preserve the full hierarchy of any selected item in the category selection list in the Edit Post screen.
#5
@
10 years 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.
#6
@
10 years 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.
#7
@
10 years 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).
#8
@
10 years 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.
screen recording of the bug