Protect against side effect term updates

[mboynes]

Resolves #831

[mboynes]

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.

[mboynes]

Spitballing: On pre_insert_term, at priority 0, this could check if the term being inserted matches the postdata and if so, hook into wp_insert_term_data at a very late priority to see if the data changed. With this, it would also be possible to check against the slug, which this currently doesn't do since the slug might be suffixed by core before insert (-2, -3, etc.). This feels sloppy to me, but I believe it would be pretty safe.

[mboynes]

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.

[renatonascalves]

🗡️

[mboynes]

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.

[johnbillion]

cc @alwaysblank, this is the root cause of WC-1084.

[dlh01]

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()).

[dlh01]

Looks like verify_new_term_data_didnt_change() would catch that, too?

[mboynes]

Good thinking. It does catch it, yes (as does the refactored version). For good measure, I added a test. Thanks!