Hierarchical class Ui\(Revisions,Diff), new class File\PageFile

[ssahara]

This PR is follow-up work of #3198, to improve Ui\Revisions and Ui\Diff classes by making them hierarchical subclasses for page and media. This approach is similar to ChangeLog and likely to name resolver in #3272.

[Klap-in]

Date is missing, just today’s date is fine.

[Klap-in]

Thanks for the further improvements. and thanks for adding the extra unit tests.

You delivered a big refactor here. Thank you very much!

[ssahara]

@Klap-in , I added routine for "save on top of external edit" assertions.

[ssahara]

Now, I could implemented what I wanted to do. I hope the new concept of File\PageFile class is accepted by core team members.

[Klap-in]

For me PageFile in current shape is fine. MediaFile looks a bit different, but I think that can wait until later more attention is given to the media part of (external) diff/revs.

[Klap-in]

@ssahara Thanks for the work!
@splitbrain for me this fine to merge. Or do you have anything about the PageFile?

[Klap-in]

My intention is to merge this, but because it is a bit big, I announced it first before doing.

[Klap-in]

At ?do=recent also a diff button is shown for just created pages. That scenario is an corner case for the ?do=diff, because we have no older page than the just created page. The diff does not handled this well before and now, and an error occurs.

(btw, maybe that diff-button should not be shown, but at least (also for a manual visit) to ?do=diff page for a just created pages no error should occur)

old
The page of the current wiki does not load completely for this url: https://www.dokuwiki.org/tips:good_content?do=diff (this is still the code before this pull request).
At line 402 of inc/Ui/Diff.php, $l_rev was null, so val() acts as getter and returns an empty string, so the page fails.
inc/Ui/Diff.php(402) Error: Call to a member function addClass() on string

$input = $form->addDropdown('rev2[0]', $l_revisions)->val($l_rev)->addClass('quickselect');

In the merged pull request this problem is solved, because now 0 is feeded to val().

new
However, since this pull request, another error occurs /inc/Ui/PageDiff.php(141) Error: Unsupported operand types
Here my first approach for handling this a bit nicer:
Twice I introduce the missing info manually.... maybe there are nicer manners?

git diff
diff --git a/inc/ChangeLog/ChangeLog.php b/inc/ChangeLog/ChangeLog.php
index 84a87e5e2..67b6f4b0d 100644
--- a/inc/ChangeLog/ChangeLog.php
+++ b/inc/ChangeLog/ChangeLog.php
@@ -395,7 +395,12 @@ abstract class ChangeLog
         list($revs2, $allrevs, $fp, $lines, $head, $tail) = $this->retrieveRevisionsAround($rev2, $max);
 
         if (empty($revs2)) return array(array(), array());
-
+        if ($rev1 === 0) {
+            //rev1 is 0 if just created
+            $revs2[] = 0;
+            sort($revs2);
+            return array(array_reverse($revs2), array_reverse($revs2));
+        }
         //collect revisions around rev1
         $index = array_search($rev1, $allrevs);
         if ($index === false) {
diff --git a/inc/Ui/PageDiff.php b/inc/Ui/PageDiff.php
index f4099579c..0aa390ef6 100644
--- a/inc/Ui/PageDiff.php
+++ b/inc/Ui/PageDiff.php
@@ -134,6 +134,16 @@ class PageDiff extends Diff
         $this->newRevInfo = $changelog->getRevisionInfo($this->newRev);
 
         foreach ([&$this->oldRevInfo, &$this->newRevInfo] as &$revInfo) {
+            if($revInfo === false) {
+                //if just created there is no oldRev
+                $revInfo = [
+                    'current' => false,
+                    'navTitle' => '—',
+                    'text' => '',
+                    'timestamp' => false
+                ];
+                continue;
+            }
             // use timestamp and '' properly as $rev for the current file
             $isCurrent = $changelog->isCurrentRevision($revInfo['date']);
             $revInfo += [
@@ -420,7 +430,6 @@ class PageDiff extends Diff
      */
     protected function buildRevisionOptions($side, $revs)
     {
-        global $lang;
         $changelog =& $this->changelog;
         $revisions = array();
 
@@ -429,11 +438,18 @@ class PageDiff extends Diff
 
         foreach ($revs as $rev) {
             $info = $changelog->getRevisionInfo($rev);
+            if($info === false) {
+                $info = [
+                    'timestamp' => false,
+                    'user' => '',
+                    'sum' => ''
+                ];
+            }
             // revision info may have timestamp key when external edits occurred
             $info['timestamp'] = $info['timestamp'] ?? true;
             $date = dformat($info['date']);
             if ($info['timestamp'] === false) {
-                // exteranlly deleted or older file restored
+                // externally deleted or older file restored
                 $date = preg_replace('/[0-9a-zA-Z]/','_', $date);
             }
             $revisions[$rev] = array(

[ssahara]

@Klap-in , PageDiff::preProcess() might be move to Action\Diff::preProcess(). it's just idea.

[Klap-in]

Moving to Action\Diff does not solve the not-existing timestamp at left side for a diff of a just created page?

@ssahara the rev2[] parameter, which is used for navigation via dropdowns at do=diff page get lost of an external deletion revision, because the external deletion revision timestamp is not an actual timestamp in the changelog. So it cannot be found, and there is I think now also no mechanism to catch them and link back to external revision (which used, due to time between the page loads, now a different timestamp… I guess this is the challenge of using current as identifying timestamp. Do you have already a proposal for linking the outdated timestamps to the external deletion revision?

In earlier stage I have tested this scenario, I now realize not for the later ones.

[ssahara]

@Klap-in , as you pointed out, It is not possible to catch up non-actual timestamp, i.e. external revision. I understand the both timestamp newRev and oldRev must be checked its validity. I have developed further to avoid Error: Unsupported operand types in /inc/Ui/PageDiff.php file. please see
ssahara@dabd9a2

[Klap-in]

@ssahara the rev2[] is used for the dropdowns on the diff page and the checkboxes on the revisions page.
My guess now is that the proposal does not yet address this yet, I guess(not yet tested) that it will give a message now in case the right side is an external deletion revision.

Maybe it is for this case a lot simpler to use revision timestamp 9999999999? There exists always max 1 external deletion revision per page, it is always more recent than latest revision. If no external deletion revision exists it can be ignored. Of course it looks as a bit strange number, but if we can simplify the code needed for tracking an external deletion on page reload, I think using 9999999999 is quite practical.

[ssahara]

@Klap-in , sorry for late response during in my stay in labo experiments during this week.
I am now trying to improve ChangeLog::getRelativeRevision() can treat non-actual external revision when it exist. In Diff::handle(), $newRev = getRelativeRevision($rev2[1]-1, +1) will get correct revision number even if it is external revision and moving timestamp.