Previewing content removes the taxonomy selection

I didn't know somebody was writing a patch. I wrote my own. I also wrote a lenthy explanation, but I guess that it's better to wait for the FAPI guys to first clear all the obvious bugs...

There are two kinds of $node: the pristine one, and the $_POST one (aka "fixed up"). The taxonomy module breaks because it doesn't handle the "fixed up" kind. But I suggest we wait with our patches till the FAPI guys decide on a common approach for this problem. IMHO, it's sad to have two kinds of $node.

Related issues:

http://drupal.org/node/167333 (Error when previewing a forum topic)
http://drupal.org/node/166882 (Preview not displaying correct date or taxonomy)

Preview is currently only working if ‘multiple select’ is disabled. The comment ‘On preview, we get tids’ in function node_link() is incorrect when dealing with multiple select of free tagging. Because then they are arrays of tids.

The main problem is that term should be objects but after preview a node they become arrays. Also, free tagging is not working now because there is no way the typed values can be filled in the form after the preview.

The patch from #1 fixes only a small part of the problem. Attached a patch that fixes hopefully all:

• a new helper function, taxonomy_terms_to_objects() to convert term-arrays to term-objects when previewing and ‘re-filling’ the form.
• in the helper function the free tagging info will be saved for re-filling the form in function taxonomy_form_alter().
• fixes all notices

Preview works correctly now; except that free-tagged term will not be links. This is on purpose because there might be ‘unsaved’ terms added. I think it’s overkill to check for them as it will become more or less a copy of the taxonomy_node_safe function.

The problem also exist when editing a node; not just with creating new ones. Without ‘preview’ (directly save) everything works as expected and all the term (selected and free) are saved into the database.

i tested and this fixes various problems as described. might be nice to get a fapi expert to chime in but i think this is committable as is.

I always found it annoying that the array structure switches out completely from previews to normal views. Could this be fixed by keeping everything consistent no matter how it's being viewed instead of manipulating it back and forth.

dvessel,

In my opinion it's worse that you are loosing content after a preview than that it's annoying that we have to go 'back and fort'. Yes it's annoying but I see no other way to fix this problem....

Review the code please, make a better proposal or else agree with it. I spend a lot, lot, lot of time to fix this issue and would be very happy if somebody sets this RTBC or guide me in a better direction.

I never submit something without preview and I told everybody who I made a site for to -always preview-, so this is critical and has to be solved. Loosing content is no-go.

Updated the patch as by Chx suggestions on Irc:

• wrapped  $node = taxonomy_terms_to_objects($node); into proper preview checks
• removed the 'is_object' check in the taxonomy_terms_to_objects() function; this is not necessary anymore because of the proper preview check above

Updated the patch again:

• I should have used NODE_BUILD_PREVIEW instead of '1'
• Documented the use of an is_object() in function taxonomy_link.

I am not sure I like the approach of this specific preview handling either. Anyway, first started to understand what happens and found some whitespace and comment issues, which made it harder to understand:

- "the other terms who are objects" should be "which are objects" I guess
- $links['taxonomy_term_'. $typed_term] line and friends are indented too much
- "Typed string can be changes by the user, so we just insert the form-field." I don't understand, maybe you "use the form input"?
- taxonomy_terms_to_objects()'s phpdoc is misplaced
- this function does not really convert to objects, but objects and strings depending on free tagging options, docs are misleading
- "but not as object but as string." sounds wrong... could be "as strings instead of objects"

Thanks for the review Gábor!

About the approach I took.... yes, not too nice but I really see no better way to solve this problem.

I updated the patch:

- Updated the incorrect English as best as I could.
- Corrected the indented lines.
- Updated the docs to be more clear.
- Renamed 'taxonomy_terms_to_objects' to 'taxonomy_preview_terms' to be less misleading.

Rerolled because the patch had an offset.

I can confirm that #14 fixes the bug.

Stylewise, the following issues:

1) taxonomy_preview_terms(), there's an extra line between the docblock and the function that shouldn't be there.
2) Said docblock has rather clumsy language. "objects with exception for 'free tagging' because new tags" feels very clunky.
3) "A 'Single select' field returns numeric." - Also clumsy language.
4) "If previewing; the terms must be converted to objects first." - That should be a comma, not semi-colon.

The attached patch changes no functionality but tries to clean the docs up a bit.

A fine patch with polished comments FTW

Cell,

You don't like good oll' Dutchifide English.... Don't forget we ruled the world in the 18th century!

Thanks for the corrections and review, let's forget the past, I'm just joking :-)

Cross-posted... CTBC :-)

Small change in function taxonomy_link() to use 'taxonomy_preview_term_' instead of 'taxonomy_term_' for previewing 'free-tagging-term'.

This will avoid an altering of the (non-functional) term during preview like forum.module is doing for example and so avoiding notices.

Applies with offset, works as advertised.

I tested this with single, multiple and free tagging vocabs and all could be previewed and then saved.

This misses one case: if oyu have a multiple-select taxonomy, and you have the first element selected, this term will not be held on preview. For instance I have a multiple-select with terms foo, bar, baz and lur. "bar" is at the top of that list, and it won't "stick" on a preview.

Turns out this is a problem in how taxonomy_preview_terms() functions:

function taxonomy_preview_terms($node) {
  foreach ($node->taxonomy as $key => $term) {
    unset($node->taxonomy[$key]);
    // A 'Multiple select' and a 'Free tagging'field returns an array.
    if (is_array($term)) {
      foreach ($term as $tid) {
        if ($key == 'tags') {
          // Free tagging; the values will be saved for later as strings instead of objects to fill the form again .
          $node->taxonomy['tags'] = $term;
        }
        else {
          $node->taxonomy[$tid] = taxonomy_get_term($tid);
        }
      }
    }
    // A 'Single select' field returns the term id.
    elseif ($term) {
      $node->taxonomy[$term] = taxonomy_get_term($term);
    }
  }
  return $node;
}

Basically if you pass through more than one vocabulary's worth of terms, and there is a key overlap between a term id and a later-passed vocabulary ID, the term will be lost because of unset($node->taxonomy[$key]);

One solution is to actually build a separate taxonomy array and stick it onto the node at the end. Beyond that, some more fundamental shifts in data-structure seem necessary to prevent vid/tid key collisions from creating lost data here. I can also see an instance where a term that's processed first could wipe out an entire vocabulary.

Ok, here's a patch which tweaks the new function to build and return a new $taxonomy array specifically, rather than working on and altering a whole $node object. This prevents any data-loss.

Nice joshk; better indeed. But now taxonomy_link didn't output anything because it was still expecting a whole node from taxonomy_preview_terms. Corrected in attached patch.

Tried this with fixed, freetagging and forum vocabularies. Looks great.

Thanks. Committed the patch with some wrapping and white space issues fixed in taxonomy_preview_terms(). Also committed a follow up patch when I noticed there were some other overly long lines in the patch and in the original code around the patch. Thanks again.

I'm using Drupal 6.4; the above patch is applied to taxonomy.module, but I have the same problem. Checkboxes for a taxonomy field lose their values during preview. Any suggestions?