Added option to honour the main page revision when using included pages#217
Added option to honour the main page revision when using included pages#217ruud12 wants to merge 10 commits intodokufreaks:masterfrom
Conversation
…e able to retrieve a specific revision
…e revision of the included page revision should equal to or older than the main page revision
… date of main page. This is only done when a revision of the main page is shown. In case the main page is the latest, all included pages will also be the latest version.
Syncing remote plugin with own fork
michitux
left a comment
michitux
left a comment
There was a problem hiding this comment.
Apart from the changes requested in the individual comments, this looks fine. Would you mind having a look at #131 and possibly try integrate this with the changes proposed here? In particular, support for $DATE_AT would be nice. The additional helper class is not necessary anymore. There are also a couple of interesting points in #131, like a) adapting links to the included pages and b) not including a page if it did not exist at the requested time.
I am also wondering about the support for the approval plugin. For full support, I assume, this would also need to possibly get older revisions if the current page is the most recent revision, but the included page's most recent revision hasn't been approved, right? If this is the case, this will interfere with caching and requires changes to the caching logic. Further, for which approval plugin is this?
| $first_revision = ''; | ||
|
|
||
| $m = p_get_metadata($id); // get metadata for current page | ||
| $sum = $m['last_change']['sum']; // get last change summary |
There was a problem hiding this comment.
These two lines seems to be unused.
| if ($this->getConf('honourmainrevision')) { | ||
|
|
||
| // initialize variables with empty string | ||
| $wanted_revision = ''; |
There was a problem hiding this comment.
$wanted_revision should be initialized to null, outside this if statement.
| $revision_before_main_revision = ''; | ||
| $first_revision = ''; |
There was a problem hiding this comment.
These two variables are only used inside the if-statement and should thus be initialized there.
| } | ||
| } | ||
| } else { | ||
| $wanted_revision = null; |
There was a problem hiding this comment.
If $wanted_revision is initialized to null, this line can be removed.
| $first_revision = $rev; | ||
| } | ||
|
|
||
| if ($wanted_revision == '') { // no suitable revision found with approval |
There was a problem hiding this comment.
If $wanted_revision is initialized to null, this needs to be adapted.
| $changelog = new PageChangeLog($id); // initiate changelog | ||
| $chs = $changelog->getRevisions(0, 10000); // load changes list |
There was a problem hiding this comment.
Why this limit to 10000 revisions? Wouldn't it make more sense to load revisions dynamically in batches? I.e., use two nested loops below, where the outer loop gets revisions in batches of, e.g., 100 revisions and the inner loop then iterates over the current batch. Further, these two lines should be inside the if statement below. Otherwise, they are also executed when the current revision is used. Further, $changelog->getLastRevisionAt() could be used unless the approval plugin is installed, and even then there are functions for getting a number of revisions around that date.
| // add configuration option to honour top page revision or not | ||
| // if (not configuration_option_honour_revision) { | ||
| // $wanted_revision = null; | ||
| // } |
There was a problem hiding this comment.
Please remove this comment as it has been addressed above.
| array_push($page_stack, $id); | ||
|
|
||
| // check if the include plugin should honour the main page revision | ||
| if ($this->getConf('honourmainrevision')) { |
There was a problem hiding this comment.
This if statement could also directly check $REV (as all code below should be moved as explained there).
2 participants
I have added the option to honour the main page revision: if a revision of a page is viewed, any included pages will also be shown with the closest (but older) revision with respect to the revision date of the viewed page. The 'closest' revision is the first revision equal to or older than the viewed page revision timestamp.
I hope I have made myself clear. I'm using the dokuwiki software for manuals and documentation, and whilst the include plugin features great functionality, I needed the option to go back in time to view an older version of a manual and the include plugin did not feature this option yet.
Please comment on my work, as I'm only a hobby programmer and my code can probably be improved. Thanks for the great include plugin anyway!