Integrate diff into core

I have committed a nearly core worthy version of diff to diff module's HEAD branch. I need a little feedback before making a final patch.

This version keeps a separate diff.module with its diff engine and implements hook_diff() on behalf of node.module, upload.module, and taxonomy.module.

1. Should we move those node.inc, upload.inc, taxonomy.inc into core modules? I say yes. They are very small.
2. Should we let diff.module be switched off so one can get the current the UI? The alternative is to simply integrate the diff powered revisions tab into node module and be done with the less capable UI we have today. In this case, we would move the diff engine to a node.diff.inc that only gets loaded when viewing a diff.
3. Should the Preview changes button be default enabled for all content types? What about upgraded sites?

In general, I'd support full integration of diff so the Revisions tab is always useful. I'd like some confirmation that this makes sense to others before proceeding. If folks are feeling cautious, the diff that is in HEAD is completely committable. It takes over the Revisions tab using hook_menu_alter() which is quite reasonable.

As for webchick's list:
a) neither is jquery, phpass, etc. not a concern anymore. the diff engine code has not change in years.
b) now it does.
c) now it is.
d) we no longer fear classes. See PDO and simpletest.

So my original idea for this was to #120967: Separate revisions from node.module so that it lived in node_revisions.module and Diff could be incorporated into that. I still think that's a good idea, since many sites don't use revisions and it clutters up the UI to have them.

But. I think it's more important to make revisions actually useful, so I don't want to hold up this effort for that one. Unless we could do both at once, in which case I would pretty much pee my pants with excitement. ;)

1. If you mean take the code from those and integrate them into their respective modules, yes.
2. No separate module (unless you take up the "node_revisions.module" torch). This should be built into the revisions system. Separate inc though, definitely.
3. I actually hate the preview changes button. So no, definitely not enabled for all content types. Possibly for any content types with revisions enabled by default. Let's ping some usability folks about that.

OK, Here is the patch and 2 new files which belongs in the node.module directory. I know of 1 small bug in taxonomy_diff() and I have not yet run the simpletests. Meanwhile, feel free to start reviewing, folks.

I fixed the taxonomy_diff bug and cleaned up a touch. The patch now includes the 2 new files.

subscribing.

I'd like to see this get in, and be part of node module so it owns the revisions tab.

I also dislike the "preview changes" button.

I like some of webchick's comments in #3 but yes maybe for another issue.

What will happen to "Preview" and "Preview changes" buttons?

subscribing for future review. I'm all for making revisions better.

I'm in favor of the proposal in #3 as well. I'd rather see revision split off and joined with diff. I have no need for revisions or diff on most of my sites.

@webchick, Dave Reid: Same here. I propose splitting it off first and merging diff in second. See my comment there.

Sorry, it is silly to make this issue dependant on vaporware. Just because a given order is "more logical" does not mean it shakes down that way.

We are in "Patch needs Review". Please.

Oh sorry Moshe, I certainly don't wanna hold back this patch! Go forward, please!
In the meanwhile I'm gonna see if I can review your patch in #5.

+1
I gave the patch in #5 some testing, and found out that diffing title, body, tags and attachments works like a charm!
Some more testing with CCK fields and possibly some edge cases would be great.

One glitch that comes to my mind: the view revision pages have no proper menu entry, so parent title, tabs and breadcrumb get lost and navigating back is only possible via the browser button. But this has also been the case before, so it's a separate issue.

It would also be awesome to include menu, path and other data in the revisioning system, but this is also beyond the scope of this issue.

So overall this patch seems to work exactly as advertised! Some more reviews would be good, so we can move forward.

Subscribing.

Applied fine with a few offsets, so I remade it.

1. I like the idea of diff in core.

2. On node add/edit pages I find having 'preview' and 'preview changes' confusing. Maybe something like 'preview published version' and 'preview changes' would be better? The text is getting far too long though.

3. The IU on the revision page is confusing too:

* As far as I can tell 2 radio buttons aren't needed on the first and last revision. When you only have 2 revisions, is isn't clear what they do.

* The 'Show diff' button is at the top of the form. Is this a good idea? I know the IU has to be complex, but we don't want to emulate the horrid form mediawiki uses -- I still get confused by that.

I know some of these points apply to the diff module even if it doesn't get in to core.

Very good suggestions from sym. With regards to the button names, it got me the very first time I used the module too.

How about we remove "Pre" to give us: "Preview" and "View Changes"?

I like 'Preview' and 'View Changes', you're not really 'Previewing changes' anyway. sym's point #3 is also valid

I also like 'Preview' and 'View Changes'. My users get confussed with the 'Preview changes" button.

I agree. 'Preview' and 'Changes' look like the best way to add some clarity.

OK, I get the point about text on the button. Will change that ... I would love a more substantial review here.

The patch in #16 doesn't contain the new files, but #5 still applies (with some offset). Attached is a reroll without offset, but with the new files.

This is exactly what I expected, great improvement of the revision system! Most things work as they should, and behavior of the functions also agrees with my expectations. I haven't tested the workings and code of the diff engine too much, but it seems to work and look fine. About the rest of the module changes I have the following remarks:
- an improvement would be if the diff also mentions changes in the user. If this is possible of course (do revisions have authors?)
- There is s a bug when you've added or changed the taxonomy terms when creating a new revision. If you then want to watch the diff at the revisions overview of that node, you'll get a Fatal error: Call to undefined function taxonomy_get_term() in /var/www/home/d7/modules/taxonomy/taxonomy.module on line 1391
- All simpletests run without fails. :-) That means we need more tests! Testing during and after editing the diffs of taxonomy, title, body and uploads, and testing the output of the diff engine. Do we want to create them in this issue or first put this part in and bother with tests later?

Changed the button to read 'View changes' and fixed the taxonomy_get_term() error. This patch again includes the 2 new files.

It would indeed be nice to show changes in url alias and menu settings but we don't version those items yet. We do version the author and I'd like to see that be exposed in these diffs. I'm hoping that can go in with a subsequent patch.

The revisions listing page does indeed have a few radio button combinations which don't make sense. See #114308: add jQuery for hiding radios that shouldn't show diffs. I think that kind of UI optimization can go into a subsequent patch. This one is already substantial.

Thanks for the reviews, folks. Lets keep pushing this one toward core.

Synched with HEAD. Let us please review this soon. I suggest you not spend too many cycles reviewing node.diff.inc. Thats the diff engine itself and has not changed in many years (except for code formatting).

The last submitted patch failed testing.

Test failure : #335122: Test clean HEAD after every commit

Rerolled removing some fuzz.

The last submitted patch failed testing.

thanks. note that your patch has eclipse file in it

Well, now here's another reroll. Let's see if it passes the testbed this time?

When u view an old revision, there a only 3 options shown on message field: edit, revert and delete.
My suggestion is to add one more option "view all revisions" to go back to previous page or make the "tabs" visible ^^.

thx

@kelvinc Please file a new issue for functional changes to the diff module.

The last submitted patch failed testing.

@kelvinc:
I'm not convinced of the approach you suggest - this would linearize or denormalize a hierarchical relation 1∈n.
Getting back from a child page to a parent page is what breadcrumbs are for.

Subscribing

Personally, I'm not convinced that diff.module belongs into core ...

The reason I'd be supportive of this in core is that node revision functionality is, quite literally, absolutely useless without this functionality. Unless of course you're *really* good at staring at two pages in two browser windows and spotting the differences and have a notebook and quill pen handy where you can jot down notes as you go through. ;)

I'd be fine with simplifying it more if that's what your hesitation is, but I really do feel that most people would consider this "core" functionality in a CMS that offers content revisioning.

As previously discussed, maybe content revisioning doesn't belong in core either

I think we need support for revisioned content in core - the underlying schema and on/off variables for content types if nothing else. It looks like it's going to be built into Field API as well, and the 'body' field removed from special casing, so that end of it is here to stay most likely if expanded and unified a bit.

But as webchick says the UI in core is pretty useless - I do have a site which uses revisions without diff, but it's 1. only as a safeguard against vandalism 2. very clunky and I keep meaning to install diff ;) I still think it'd be good to put some effort into #120967: Separate revisions from node.module - probably into a revisions_ui module since node_revisions might get refactored by fields anyway. Then we have three choices - a very limited and clunky revisions_ui module in core and diff in contrib, diff in core, or no UI for revisions in core and everything is handled by diff in contrib.

Content revision is such an important part of content creation because when a user makes a change to a node in Drupal that change is "forever". I don't believe users should be required to wade threw SQL dumps to retrieve content changes. This becomes even more important on sites where multiple users are editing the same node. This seems like such a core feature to me because Drupal is all about content creation, and as a Content Managment System users need a way to revise edits on that content, but without Diff content revision is a usability nightmare.

Maybe the solution is to leave revisions in core, but to move the revisions UI out of core, and to create a revisions_ui project that comes with diff.module. I'm not necessarily advocating this solution, but it might be worthy of exploration.

Revisions absolutely need to be in core. I continue to advocate making revisions required and removing the current ability to directly overwrite existing revisions with new revisions.

The work on fields in core has made me skeptical of how effectively we can embed most APIs in core without corresponding UIs. I agree that Diff itself does not necessarily have to be in core; it's currently a nice example of how flexible Drupal allows decorators to extend core capabilities.

Yes, I don't think anyone's arguing that Diff can't live in contrib. It has for several releases now.

What myself and others are arguing is that as a user, I would find it absolutely bizarre to use a CMS that touted this as one of its features, where I had to go download an *add-on* to actually see differences in revisions. Especially when "finding the add-ons for Drupal that you need to accomplish X" is the toughest task that both new and experienced users alike face.

Let's look at how other projects that way more people use than Drupal offer revisioned content:

Wikipedia:

Google Docs:

WordPress:

Now, let's look at Drupal:

We keep talking about how we want the usability in D7 to be better than any other release before it. Jakob Nielsen's Law of the Web User Experience is, "users spend most of their time on other sites, so that's where they form their expectations for how the Web works." So core matching user expectations is a critical piece of Drupal being more usable, IMO.

Dries, is there any chance you could elaborate on why you think this wouldn't be a good thing for core revisions to offer out of the box?

I absolutely agree that Diff should be in core. If we don't add Diff, then we can remove Poll. As webchick pointed out in #43, almost every other competing CMS provides revisions and revision differences out of the box.

Hence, subscribing, and marking for later review.

Awesome post webchick, that can be Drupal's new usability slogan "Where's my button!"

Your spot on, users expect this ability to compare revisions and the ability to compare content in a content management system.

This talk of removing revisions from core scares me. For me the main reason to keep revisions is another missing Drupal feature: node moderation. While Diff may exist comfortably in contrib, the Revision Moderation module is far to limited by its contrib status. As I see it, that is the feature that makes revisions necessary and needs to be brought into core (#218755: support revisions in different states), which will be difficult to do if revisions themselves are not in core.

That being said, if you're going to include revisions it does seem odd not to provide a way to compare them. So, show me the button!

+1 to diff in core. webchick's screenshots and attached argument are exactly what my expectation of revisions in core was and is.

+1 vote for core.

Would making revisions a standalone, optional core module, which integrates diff, and serves as an API for contrib to extend the abilities of revisions (allowing #218755: support revisions in different states) be the best solution? Then if someone doesn't need revisions, they can be turned off and removed completely, making the UI simpler for those who have no need for, and currently don't use revisions, and then having a more fully featured and capable starting point for revisions to build from.

And then we could "addz muh button!!11!"

---Edit---
Duh, I've been beaten to this suggestion by webchick in #3! Ah well, that was a few comments ago - worth the reminder I guess.

I downloaded this big patch expecting much difficulty getting it to apply - but since this patch merely adds code, the three failures were only caused by trivial changes in the surrounding context. :) Here's a fresh patch.

(Naturally, this upload only fixes the obvious problem of applying the changes to the code. I'll leave it to testbot to decide whether the changes still work.)

Revision Moderation and Diff are both requirements for a "serious" CMS I think. Would programmers in a large project use a source code control system that didn't have that kind of functionality? There must be a way for an 'amateur' on a project to edit existing material, and a way for the 'professional' on the project to check the edits and then optionally approve them before they become public.

The last submitted patch failed testing.

Ceterum censeo, we need more detailed failure output on the testing site. We have such lovely automated testing, and all it accomplishes in this case is that I know which test I have to run locally to see what broke.

I wasn't able to replicate the testbot's results (my test site runs SQLite by the way).

I haven't got the time to examine the error right now, but here is a full dump of the failed test case (Node revisions, naturally).

It's interesting to note that the deletion and reverting asserts failed, but their actions seem to have taken effect.

Also some stuff about the log messages.

So, looks like node/1/revisions runs into an infinite recursion.

This is because the node_revisions_overview form is set to be rendered by theme_node_revisions_overview, which passes the form array to drupal_render, which dispatches it to the designated renderer theme_node_revisions_overview, which... and so on.

I don't know why theme_node_revisions_overview tries to render the entire form anyway - it already renders a perfectly functional table from the form's elements. I have removed the recursive call in this patch. The revision overview is now displayed with no problems.

All tests pass on my site. I still get the notice, but since testbot didn't get it I rather suspect it's caused by one of my contrib addons.

Revisions need to be in core, but diff doesn't have to be (although it could). I'm not 100% convinced yet, it should.

What is usefull about revisions without diff, for a user?

@Bojhan - I've said it before, but I'll say it again: In my humble opinion, the most important reason for revisions to be in core is the (potential) ability to support content moderation and advanced workflows. (#218755: support revisions in different states)

That being said, I support the inclusion of Diff in core 100%.

@Bojhan: it can be useful for 'backups' of content if you have a few different editors but don't need to actually revert very often - that's more a site policy thing than actually useful though.

It'd make sense to tie revisions and diff to fieldable objects (or entities if that patch lands), and have it work for more than just nodes, but I don't think whether diff in core is a good idea depends on doing something like that.

Exactly, it is like Ctrl-Z (Undo). My undo feature doesn't sport a diff-feature either.

Maybe not a great analogy, because the idea of Undo or ctrl-z exists in the world of one person working on one project. On a CMS, various people (with various levels of competence and authority) may be be editing the same page. Same as a SCCS?

But, the more I think about it, I agree that DIFF doesn't need to be in core, because as long as revisions / revision moderation is in core, it should be easy to implement diff as a contributed module. Plus, diff is exactly the sort of thing where people may disagree about the appearance etc, so there might be a good reason to have more than one contributed DIFF module?

I agree diff could be done as a contributed module, but I don't think that's a good idea. Coming purely from a usability stand point, I think it's something that a lot of people expect to be present when you say there's a revisions feature in core, and are left scratching their heads not realising they need a contrib module. So +1 for adding it to core.

Though if we could make it work for fields other than the body, then I think it'd be a really fantastic feature.

The last submitted patch failed testing.