Closed (fixed)
Project:
Diff
Version:
5.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
2 Feb 2007 at 17:25 UTC
Updated:
26 Apr 2008 at 09:14 UTC
Jump to comment: Most recent file
For some special fields in a node the preview changes doesn't work correctly:
upload: the attachements are always shown as removed
cck: nodereference field behaves strange
other cck fields I tested seem to work fine.
| Comment | File | Size | Author |
|---|---|---|---|
| #18 | diff_125494_preview_changes_2.patch | 2.09 KB | dww |
| #17 | diff_125494_preview_changes_1.patch | 1.68 KB | moonray |
| #3 | diff_upload_taxonomy_0.patch | 3.91 KB | rötzi |
| #2 | diff_upload_taxonomy.patch | 3.92 KB | rötzi |
Comments
Comment #1
rötzi commentedit doesnt work for revisioned taxonomy
Comment #2
rötzi commentedThe attached patch solves the issue for upload and taxonomy module. The problem was that the data is stored in a different layout during editing.
The patch is against the HEAD branch.
Comment #3
rötzi commentedThis patch is against the HEAD branch. The one before was against DRUPAL-5 branch.
Comment #4
toemaz commentedIts quite possible this preview changes would not work for other (not core) node types too. In my case, I have my own defined node and 'preview changes' doesn't make sence because the body data is being rendered after submit.
In any case, wouldn't it be better to have an option under admin/settings/node where you can select the nodes where the 'preview changes' should appear?
This follow up is a duplicate from http://drupal.org/node/115226#comment-192611
I can create a patch for this if you think this would be a good idea.
Comment #5
BioALIEN commentedThe webform module should be included in the group of modules listed above. +1 to toemaz's suggestion. He's even offering to contribute the patch, please reconsider your stance on this.
Comment #6
toemaz commentedAs proposed by Moshe (http://drupal.org/node/115226#comment-192669), I made a contrib module implementing the form_alter hook. I can still create a patch, but only if the module devs will consider to commit it.
Comment #7
BioALIEN commentedWe need to hear from the mod authors how they wish to tackle this issue. Any ideas?
Comment #8
dwwpersonally, i wouldn't mind a checkbox on the configure node type forms to indicate if you want the "preview changes" button. seems like making it always work, all the time, is a lot of work, and it'd be easier to just provide an admin UI to turn this on/off on the site.
however, i'm guessing moshe will consider that bloat for diff.module itself.
most of the work for this would be shared if it was a separate module using form alter, or code native to the diff.module. the bulk of the code would be the admin UI, and some DB stuff to store the results (probably via variable_set(), possibly via another table, instead). the actual code to hide the button is basically 3 lines, no matter what (either an if() clause around the $form declaration in diff.module itself, or an if() and an unset() in hook_form_alter())...
so, i say, just write something, however you want to do it, and we can see how big it is. if it's huge, it should probably become a separate module (which i'd have no problem including in the same directory/package as diff.module). if it's tiny, i'd probably vote to just put it in diff.module directly.
thanks,
-derek
Comment #9
BioALIEN commenteddww, this is exactly what has already been discussed and suggested by myself and toemaz. In fact, the introduction of the "Preview changes" button seems unnecessary as someone with the correct permissions can already generate a diff through the revisions tab. Why would someone edit a node, add/remove/modify content and then wants to see a diff before submit?
Seems like not enough thought went into it before it was committed from the patch posted.
Comment #10
dwwi'm not claiming i have a new idea, i was (as requested) giving my thoughts on this as one of the mod maintainers with commit rights... ;) i was just outlining exactly how little code this would involve, as justification for my vote to include it in diff.module proper.
re: the merits of the "Preview changes" button in the first place, this isn't the right place to discuss that. but, briefly: just because i *could* make an edit to an important page on a live site, *then* go look at the revisions tab to see if my change was really only touching what i wanted, and then repeat (all while 100s of people are seeing my live edit before i had a chance to verify it), doesn't mean that's a good idea. it's nice to see exactly what you've changed with a given edit, to make sure something you had no intention of touching wasn't changed too. just seeing a preview of your new copy (especially on a very large post) makes it extremely difficult, time consuming, and error prone, to verify you're only changing what you think you wanted to change. that's the whole reason diff exists in general, and diff.module exists in particular.
think of CVS: you should *ALWAYS* run "cvs diff" in a workspace before you commit it, to make sure no debugging code, other modifications, etc, snunk into a workspace before you commit a given patch. likewise, it's nice to review the diff on your text modifications to a piece of content before you press "submit", and "commit" it to a live site.
anyway, digression over. ;)
cheers,
-derek
Comment #11
BioALIEN commentedDerek, that's why life revolves around diff.module and revision_moderation.module - in case an admin needs to approve of changes before they go live ;)
Comment #12
dwwwhat if you're the admin? ;) you still want to (be able to) see the diff of your change before you submit it. again, just becuase i maintain a bunch of modules (am the admin) doesn't mean i'm *always* 100% right and never screw anything up. "cvs diff" before commit goes a long way to avoiding problems. likewise "Preview changes" before "submit"...
anyway, back to the issue at hand. where's this code? ;) once we have a patch to start from, we can make a better decision on where it goes.
thanks,
-derek
Comment #13
rötzi commentedSee here: http://drupal.org/node/125494
Comment #14
moonray commentedMarking as duplicate of: http://drupal.org/node/198917
Comment #15
dwwThis is the earliest issue about the topic, we should discuss it here.
To review: http://drupal.org/files/issues/diff_125494_preview_changes.patch
Comment #16
dwwNo longer applies after http://drupal.org/node/153234 and http://drupal.org/node/180069
Should be an easy re-roll... anyone?
Comment #17
moonray commentedpatch re-rolled.
Comment #18
dwwThere were a few remaining code style problems with the patch, but I fixed those, did some final review/testing, and committed the attached patch to HEAD and DRUPAL-5.
This would require some larger backporting to 4.7.x, since the content editing form is handled very differently back then. I'm not sure anyone cares, so we can just set this back to 5.x and fixed, but I figured I'd leave it like this for a little while to see if anyone steps up to backport this. Thanks.
Comment #19
moshe weitzman commentedset it back if someone cares to do a 4.7 port.
Comment #20
dwwComment #21
Anonymous (not verified) commentedAutomatically closed -- issue fixed for two weeks with no activity.
Comment #22
scottrigbyHi dww, moshe & everyone...
I'm using Diff 5.x-2.0 on a clean D5.7 install, and had problems with the Preview Changes feature. It shows 'Changes to Taxonomy' when there are none.
I tried to apply the patch in #18 (not clear to me if this was for the first part of this issue, or the taxonomy part).
In any case, I got the following warning when trying to apply the patch:
Which makes me wonder if this isn't already added to 5.x-2.0 .
If so, I'm still having the taxonomy problem.
If not, can you please advise which patch I should use?
Thanks in advance!
Scott
Comment #23
dww@scottrigby: You don't seem to understand the point of the patch that was committed (or seem to have read closely the comments in this issue). You found the path in #18, but didn't realize that I said I committed that. Anyway, the point of this change was to let you disable the "Preview changes" button on certain node types. What you're talking about isn't a bug, it's completely by design. There are little headers for the different kinds of things that diff can tell you about the differences between 2 revisions, and it'll happily write "No visible changes" for headers where there was no change. Please leave this issue closed. Thanks.