Protect against side effect term updates#832
Conversation
Bonus: tests uncovered a bug! Hooray for tests!
|
I'm not crazy about this implementation as-is; there are two problems I have with it. One thing that's a bit complicated about this is that there's no way to know for absolute certain if a term being created is the same or not as a side effect term. What I did in the PR is check the term name and description. Consider this scenario which bypasses the new check: a new term is created, and there is a process which runs where every time a new term is created as a child of term A, a second identical term is added as a child of term B (with the same name and description). In this scenario, Fieldmanager would save meta to both terms. I can get past this because the outcome of this edge case is already happening and this guard is better than nothing. The other, more significant issue is if a theme/plugin filters the name or description of a term being created. In that scenario, Fieldmanager will never save meta to it; Fieldmanager will always assume it's a side effect. |
|
Spitballing: On |
|
TIL: Core expects unique combinations of taxonomy-name-parent. Updated the logic to depend on that, which feels much better to me. Still have the scenario of a plugin or theme manipulating the name or description prior to insert. |
|
Updated this to include the proposed workaround for term data changing mid-insert. There still exists edge-of-the-edge-case scenarios where data could be lost, but risk is infinitesimally small and I don't see a way around it. For instance, if the user is creating term A with Fieldmanager metadata, and during insert, some code both (!) manipulates the term's name AND creates term B, AND THEN the creation of term B fails silently, the metadata intended for term A will never be saved (because this code sees that a secondary/side effect term is being inserted, and it expects a successful return, so it never records term A's name change). Of course, it's also entirely possible a more likely edge-case exists and I'm just not seeing it. |
|
cc @alwaysblank, this is the root cause of WC-1084. |
|
|
||
| // Core expects terms to have a unique combination of [taxonomy, name, parent]. | ||
| $term = get_term( $term_id ); | ||
| if ( $term->name !== $this->inserting_term_data['name'] ) { |
There was a problem hiding this comment.
Will this condition match terms whose names contain characters that are automatically converted to HTML entities on save? For example, a term named This & That will be saved as This & That (via sanitize_term() -> 'pre_term_name' -> _wp_specialchars()).
There was a problem hiding this comment.
Looks like verify_new_term_data_didnt_change() would catch that, too?
There was a problem hiding this comment.
Good thinking. It does catch it, yes (as does the refactored version). For good measure, I added a test. Thanks!
Resolves #831