Also, see what can be done with integrating with Diff and Revisioning contrib modules.

CommentFileSizeAuthor
#14 files_ought_to_have_a-2097975-14.patch14.26 KBmalcomio
#11 files_ought_to_have_a-2097975-11.patch6.81 KBmalcomio
#8 file_revision_tab-2097975-7.patch12.94 KBmalcomio
#7 files_ought_to_have_a-2097975-6.patch6.81 KBmalcomio
#6 interdiff-2097975-3-6.txt2.26 KBmalcomio
#3 file_entity_revisions-revisions_tab-2097975-3.patch12.98 KBdas-peter
#3 interdiff-2097975-2-3-do-not-test.diff8.49 KBdas-peter
#2 file_entity_revisions-revisions_tab-2097975-2.patch12.31 KBreubenavery
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ericduran’s picture

ericduran CreditAttribution: ericduran commented

Diff support seems good to me.

We don't use Revisioning but we do plan on adding SPS support for previewing diff on the site.

With the SPS work we're going to add State Machine (3.x) support which is very different then the Revisioning module. That being said I'm ok adding support for both.

reubenavery’s picture

reubenavery CreditAttribution: reubenavery commented
Status: Active » Needs review
FileSize
file_entity_revisions-revisions_tab-2097975-2.patch12.31 KB

Here's a first go at it.

Still to-do, would like to incorporate Diff to see differences between revisions.

das-peter’s picture

das-peter CreditAttribution: das-peter commented
Issue summary: View changes
FileSize
interdiff-2097975-2-3-do-not-test.diff8.49 KB
file_entity_revisions-revisions_tab-2097975-3.patch12.98 KB

First tests with patch are promising.
I assume vast parts originate from node, correct?
Re-rolled & I've found some coding standard issues as well as missing registration of paths as admin paths.
Attached patch contains the cleanup as well as the registration as admin paths.

seanB’s picture

seanB
Primary language Dutch
Location Netherlands
CreditAttribution: seanB commented
Status: Needs review » Reviewed & tested by the community

Patch looks good! Applies perfect and adds the tab as expected.

bircher’s picture

bircher
Location 🇨🇿
CreditAttribution: bircher commented

+1 RTBC for #3

malcomio’s picture

malcomio CreditAttribution: malcomio commented
Status: Reviewed & tested by the community » Needs review
FileSize
interdiff-2097975-3-6.txt2.26 KB

As I noted on #2390565: Revisions overview should show published state the revisions tab should display the published state - here's an updated patch that does it.

I've also removed some of the duplication in the date generation.

malcomio’s picture

malcomio CreditAttribution: malcomio commented
FileSize
files_ought_to_have_a-2097975-6.patch6.81 KB
malcomio’s picture

malcomio CreditAttribution: malcomio commented
FileSize
file_revision_tab-2097975-7.patch12.94 KB

Looks like something went wrong with the previous patch generation - here's a revised patch.

malcomio’s picture

malcomio CreditAttribution: malcomio commented
Status: Needs review » Needs work

In the previous patch, the published column is missing a header.

malcomio’s picture

malcomio CreditAttribution: malcomio commented

Another issue is that reverting to a specific revision does not take effect until there is an explicit save action via the edit form.

malcomio’s picture

malcomio CreditAttribution: malcomio commented
FileSize
files_ought_to_have_a-2097975-11.patch6.81 KB

New patch with table headers correct, and revision ID added - given that the scheduler asks for the revision ID, it helps editors to know what that ID is.

Not sure about the best way to handle my comment in #10 about requiring an explicit save action for the revision to take effect.

malcomio’s picture

malcomio CreditAttribution: malcomio commented
Status: Needs work » Needs review
ivansf’s picture

ivansf CreditAttribution: ivansf commented

Last patch fails because pages.inc wasn't included

malcomio’s picture

malcomio CreditAttribution: malcomio commented
FileSize
files_ought_to_have_a-2097975-14.patch14.26 KB

Patches on top of patches, yuk...

Here's another attempt.

ivansf’s picture

ivansf CreditAttribution: ivansf commented
Status: Needs review » Reviewed & tested by the community

Tested and it works just fine.

I used a clean installation and Media 2.0-dev too for selecting files.

skorzh’s picture

skorzh
Primary language Russian
Location Belarus
CreditAttribution: skorzh commented

hmm.. the patch works good, but if I remove revision, file added in revision didn't remove from the file system. So even If I've removed file with all revisions, all files added in revisions not removed from file system.

Can anyone also check it?

malcomio’s picture

malcomio CreditAttribution: malcomio commented

Good point - I hadn't tested deleting revisions, because the site we're using this on has a policy of wanting to store all old revisions for audit trail purposes.

I doubt I'll have time to work on this at the moment...

Kenneth Lancaster’s picture

Kenneth Lancaster CreditAttribution: Kenneth Lancaster commented

Great patch! It works and may become very useful to a big project we are working on. If we look to implementing this more we may try and extend this more and submit some more features for it. I am also interested in incorporating the Diff module as well, and on the revisions page, it might be cool to have thumbnails if the file is an image. Anyway, right now I am getting this error:

Notice: Undefined index: new_revision in file_entity_revisions_form_file_entity_edit_alter()

which is this line of the file_entity_revisions.module:

'#default_value' => ($file_options['new_revision'] === 'new_revision'),

I'll look at this some more and make sure I did not enter something wrong. Thanks.

damianrobinson’s picture

damianrobinson CreditAttribution: damianrobinson commented

This patch is also rolled into the patch here: https://www.drupal.org/node/2267943 which now includes workflow functionality.