X-Git-Url: http://git.vanrenterghem.biz/git.ikiwiki.info.git/blobdiff_plain/dd87d82fbe2bf09abe634caef044474d5b24502e..61a36de432c58a8d7703b1ecc5d5704981e10e2d:/doc/todo/Moving_Pages.mdwn?ds=sidebyside diff --git a/doc/todo/Moving_Pages.mdwn b/doc/todo/Moving_Pages.mdwn index e1447bfb7..387e4fb82 100644 --- a/doc/todo/Moving_Pages.mdwn +++ b/doc/todo/Moving_Pages.mdwn @@ -10,6 +10,8 @@ Thanks again to [Joey](http://kitenet.net/~joey) for putting ikiwiki together. *[Kyle](http://kitenet.net/~kyle/)=* +> Took a bit too long, but [[done]] now. --[[Joey]] + ---- The MediaWiki moving/renaming mechanism is pretty nice. It's easy to get a list of pages that point to the current page. When renaming a page it sticks a forwarding page in the original place. The larger the size of the wiki the more important organization tools become. @@ -37,285 +39,6 @@ Brad ----- -[[!tag patch]] - -This is my second cut at a feature like that requested here. -It can also be found [here](http://ikidev.betacantrips.com/patches/move.patch). - -A few shortcomings exist: - -* No precautions whatsoever are made to protect against race conditions or failures - in the rcs\_move function. I didn't even do the `cgi_editpage` thing where I hold - the lock and render afterwards (mostly because the copy I was editing was not - up-to-date enough to have that code). Although FAILED_SAVE is in movepage.tmpl, - no code activates it yet. -* Some code is duplicated between cgi\_movepage and cgi\_editpage, as well - as rcs\_commit and rcs\_move. -* The user interface is pretty lame. I couldn't figure out a good way to let - the user specify which directory to move things to without implementing a - FileChooser thing. -* No redirect pages like those mentioned on [[todo/Moving_Pages]] exist yet, - so none are created. -* I added a Move link to page.tmpl but it may belong better someplace else -- - maybe editpage.tmpl? Not sure. -* from is redundant with page so far -- but since the Move links could someday - come from someplace other than the page itself I kept it around. -* If I move foo.mdwn to bar.mdwn, foo/* should move too, probably. - -> Looks like a good start, although I agree about many of the points above, -> and also feel that something needs to be done about rcses that don't -> implement a move operation -- falling back to an add and delete. -> --[[Joey]] - -Hmm. Shouldn't that be done on a by-RCS basis, though? (i.e. implemented -by backends in the `rcs_move` function) - -> Probably, yes, but maybe there's a way to avoid duplicating code for that -> in several of them. - -Also, how should ikiwiki react if a page is edited (say, by another user) -before it is moved? Bail, or shrug and proceed? - -> The important thing is to keep in mind that the page could be edited, -> moved, deleted, etc in between the user starting the move and the move -> happening. So, the code really needs to deal with all of these cases in -> some way. It seems fine to me to go ahead with the move even if the page -> was edited. If the page was deleted or moved, it seems reasonable to exit -> with an error. - - diff -urNX ignorepats ikiwiki/IkiWiki/CGI.pm ikidev/IkiWiki/CGI.pm - --- ikiwiki/IkiWiki/CGI.pm 2007-02-14 18:17:12.000000000 -0800 - +++ ikidev/IkiWiki/CGI.pm 2007-02-22 18:54:23.194982000 -0800 - @@ -561,6 +561,106 @@ - } - } #}}} - - +sub cgi_movepage($$) { - + my $q = shift; - + my $session = shift; - + eval q{use CGI::FormBuilder}; - + error($@) if $@; - + my @fields=qw(do from rcsinfo page newdir newname comments); - + my @buttons=("Rename Page", "Cancel"); - + - + my $form = CGI::FormBuilder->new( - + fields => \@fields, - + header => 1, - + charset => "utf-8", - + method => 'POST', - + action => $config{cgiurl}, - + template => (-e "$config{templatedir}/movepage.tmpl" ? - + {template_params("movepage.tmpl")} : ""), - + ); - + run_hooks(formbuilder_setup => sub { - + shift->(form => $form, cgi => $q, session => $session); - + }); - + - + decode_form_utf8($form); - + - + # This untaint is safe because if the page doesn't exist, bail. - + my $page = $form->field('page'); - + $page = possibly_foolish_untaint($page); - + if (! exists $pagesources{$page}) { - + error("page does not exist"); - + } - + my $file=$pagesources{$page}; - + my $type=pagetype($file); - + - + my $from; - + if (defined $form->field('from')) { - + ($from)=$form->field('from')=~/$config{wiki_file_regexp}/; - + } - + - + $form->field(name => "do", type => 'hidden'); - + $form->field(name => "from", type => 'hidden'); - + $form->field(name => "rcsinfo", type => 'hidden'); - + $form->field(name => "newdir", type => 'text', size => 80); - + $form->field(name => "page", value => $page, force => 1); - + $form->field(name => "newname", type => "text", size => 80); - + $form->field(name => "comments", type => "text", size => 80); - + $form->tmpl_param("can_commit", $config{rcs}); - + $form->tmpl_param("indexlink", indexlink()); - + $form->tmpl_param("baseurl", baseurl()); - + - + if (! $form->submitted) { - + $form->field(name => "rcsinfo", value => rcs_prepedit($file), - + force => 1); - + } - + - + if ($form->submitted eq "Cancel") { - + redirect($q, "$config{url}/".htmlpage($page)); - + return; - + } - + - + if (! $form->submitted || ! $form->validate) { - + check_canedit($page, $q, $session); - + $form->tmpl_param("page_select", 0); - + $form->field(name => "page", type => 'hidden'); - + $form->field(name => "type", type => 'hidden'); - + $form->title(sprintf(gettext("moving %s"), pagetitle($page))); - + my $pname = basename($page); - + my $dname = dirname($page); - + if (! defined $form->field('newname') || - + ! length $form->field('newname')) { - + $form->field(name => "newname", - + value => pagetitle($pname, 1), force => 1); - + } - + if (! defined $form->field('newdir') || - + ! length $form->field('newdir')) { - + $form->field(name => "newdir", - + value => pagetitle($dname, 1), force => 1); - + } - + print $form->render(submit => \@buttons); - + } - + else{ - + # This untaint is safe because titlepage removes any problematic - + # characters. - + my ($newname)=$form->field('newname'); - + $newname=titlepage(possibly_foolish_untaint($newname)); - + my ($newdir)=$form->field('newdir'); - + $newdir=titlepage(possibly_foolish_untaint($newdir)); - + if (! defined $newname || ! length $newname || file_pruned($newname, $config{srcdir}) || $newname=~/^\//) { - + error("bad page name"); - + } - + check_canedit($page, $q, $session); - + - + my $newpage = ($newdir?"$newdir/":"") . $newname; - + my $newfile = $newpage . ".$type"; - + my $message = $form->field('comments'); - + unlockwiki(); - + rcs_move($file, $newfile, $message, $form->field("rcsinfo"), - + $session->param("name"), $ENV{REMOTE_ADDR}); - + redirect($q, "$config{url}/".htmlpage($newpage)); - + } - +} - + - sub cgi_getsession ($) { #{{{ - my $q=shift; - - @@ -656,6 +756,9 @@ - elsif (defined $session->param("postsignin")) { - cgi_postsignin($q, $session); - } - + elsif ($do eq 'move') { - + cgi_movepage($q, $session); - + } - elsif ($do eq 'prefs') { - cgi_prefs($q, $session); - } - diff -urNX ignorepats ikiwiki/IkiWiki/Rcs/svn.pm ikidev/IkiWiki/Rcs/svn.pm - --- ikiwiki/IkiWiki/Rcs/svn.pm 2007-01-27 16:04:48.000000000 -0800 - +++ ikidev/IkiWiki/Rcs/svn.pm 2007-02-22 01:51:29.923626000 -0800 - @@ -60,6 +60,34 @@ - } - } #}}} - - +sub rcs_move ($$$$;$$) { - + my $file=shift; - + my $newname=shift; - + my $message=shift; - + my $rcstoken=shift; - + my $user=shift; - + my $ipaddr=shift; - + if (defined $user) { - + $message="web commit by $user".(length $message ? ": $message" : ""); - + } - + elsif (defined $ipaddr) { - + $message="web commit from $ipaddr".(length $message ? ": $message" : ""); - + } - + - + chdir($config{srcdir}); # svn merge wants to be here - + - + if (system("svn", "move", "--quiet", - + "$file", "$newname") != 0) { - + return 1; - + } - + if (system("svn", "commit", "--quiet", - + "--encoding", "UTF-8", "-m", - + possibly_foolish_untaint($message)) != 0) { - + return 1; - + } - + return undef # success - +} - + - sub rcs_commit ($$$;$$) { #{{{ - # Tries to commit the page; returns undef on _success_ and - # a version of the page with the rcs's conflict markers on failure. - diff -urNX ignorepats ikiwiki/IkiWiki/Render.pm ikidev/IkiWiki/Render.pm - --- ikiwiki/IkiWiki/Render.pm 2007-02-14 17:00:05.000000000 -0800 - +++ ikidev/IkiWiki/Render.pm 2007-02-22 18:30:00.451755000 -0800 - @@ -80,6 +80,7 @@ - - if (length $config{cgiurl}) { - $template->param(editurl => cgiurl(do => "edit", page => $page)); - + $template->param(moveurl => cgiurl(do => "move", page => $page)); - $template->param(prefsurl => cgiurl(do => "prefs")); - if ($config{rcs}) { - $template->param(recentchangesurl => cgiurl(do => "recentchanges")); - diff -urNX ignorepats ikiwiki/templates/movepage.tmpl ikidev/templates/movepage.tmpl - --- ikiwiki/templates/movepage.tmpl 1969-12-31 16:00:00.000000000 -0800 - +++ ikidev/templates/movepage.tmpl 2007-02-22 18:40:39.751763000 -0800 - @@ -0,0 +1,44 @@ - + - + - + - + - + - +<TMPL_VAR FORM-TITLE> - + - + - + - + - + - + - + - + - +

- +Failed to save your changes. - +

- +

- +Your changes were not able to be saved to disk. The system gave the error: - +

- + - +
- +Your changes are preserved below, and you can try again to save them. - +

- +
- + - +
- +/ - +
- + - + - + - + - +New location: / - +
- + - +Optional comment about this change:
- +
- +
- + - + - + - + - diff -urNX ignorepats ikiwiki/templates/page.tmpl ikidev/templates/page.tmpl - --- ikiwiki/templates/page.tmpl 2006-12-28 12:27:01.000000000 -0800 - +++ ikidev/templates/page.tmpl 2007-02-22 01:52:33.078464000 -0800 - @@ -32,6 +32,9 @@ - -
  • Edit
  • -
    - + - +
  • Move
  • - +
    - -
  • RecentChanges
  • -
    - ----- - I'm going to try to run through a full analysis and design for moving and deleting pages here. I want to make sure all cases are covered. --[[Joey]] @@ -361,14 +84,25 @@ It won't be possible to put it on the action bar or somewhere else. (It would be possible to code up a different rename button that doesn't do that, and use it elsewhere.) +Hmm, unless it saves the edit state and reloads it later, while using a separate +form. Which seems to solve other problems, so I think is the way to go. + ## SubPages When renaming `foo`, it probably makes sense to also rename `foo/Discussion`. Should other SubPages in `foo/` also be renamed? I think it's probably simplest to rename all of its SubPages too. +(For values of "simplest" that don't include the pain of dealing with all +the changed links on subpages.. as well as issues like pagespecs that +continue to match the old subpages, and cannot reasonably be auto-converted +to use the new, etc, etc... So still undecided about this.) + When deleting `foo`, I don't think SubPages should be deleted. The -potential for mistakes and abuse is too large. +potential for mistakes and abuse is too large. Deleting Discussion page +might be a useful exception. + +TODO: Currently, subpages are not addressed. ## link fixups @@ -376,6 +110,18 @@ When renaming a page, it's desirable to keep links that point to it working. Rather than use redirection pages, I think that all pages that link to it should be modified to fix their links. +The rename plugin can add a "rename" hook, which other plugins can use to +update links &etc. The hook would be passed page content, the old and new +link names, and would modify the content and return it. At least the link +plugin should have such a hook. + +After calling the "rename" hook, and rendering the wiki, the rename plugin +can check to see what links remain pointing to the old page. There could +still be some, for example, CamelCase links probably won't be changed; img +plugins and others contain logical links to the file, etc. The user can be +presented with a list of all the pages that still have links to the old +page, and can manually deal with them. + In some cases, a redirection page will be wanted, to keep long-lived urls working. Since the meta plugin supports creating such pages, and since they won't always be needed, I think it will be simplest to just leave it up to @@ -387,60 +133,90 @@ The source page must be editable by the user to be deleted/renamed. When renaming, the dest page must not already exist, and must be creatable by the user, too. -When deleting/renaming attachments, the `allowed_attachments` PageSpec +lWhen deleting/renaming attachments, the `allowed_attachments` PageSpec is checked too. ## RCS -Two new optional functions are added to the RCS interface: - -* `rcs_delete(file, message, rcstoken, user, ipaddr)` -* `rcs_rename(old, new, message, rcstoken, user, ipaddr)` +Three new functions are added to the RCS interface: -The page move/rename code will check if these are not available, and error -out. +* `rcs_remove(file)` +* `rcs_rename(old, new)` +* `rcs_commit_staged(message, user, ip)` -Similar to `rcs_commit` both of these take a rcstoken, which is generated -by an earlier `rcs_prepedit`. +See [[rcs_updates_needed_for_rename_and_remove]]. ## conflicts -Cases that have to be dealt with: +Cases to consider: * Alice clicks "delete" button for a page; Bob makes a modification; Alice confirms deletion. Ideally in this case, Alice should get an error message that there's a conflict. + Update: In my current code, alice's deletion will fail if the file was + moved or deleted in the meantime; if the file was modified since alice + clicked on the delete button, the modifications will be deleted too. I + think this is acceptable. * Alice opens edit UI for a page; Bob makes a modification; Alice clicks delete button and confirms deletion. Again here, Alice should get a conflict error. Note that this means that the rcstoken should be recorded when the edit UI is first opened, not when the delete button is hit. + Update: Again here, there's no conflict, but the delete succeeds. Again, + basically acceptible. * Alice and Bob both try to delete a page at the same time. It's fine for the second one to get a message that it no longer exists. Or just to silently fail to delete the deleted page.. + Update: It will display an error to the second one that the page doesn't + exist. * Alice deletes a page; Bob had edit window open for it, and saves it afterwards. I think that Bob should win in this case; Alice can always notice the page has been added back, and delete it again. + Update: Bob wins. * Alice clicks "rename" button for a page; Bob makes a modification; Alice confirms rename. This case seems easy, it should just rename the modified page. + Update: it does * Alice opens edit UI for a page; Bob makes a modification; Alice clicks rename button and confirms rename. Seems same as previous case. + Update: check * Alice and Bob both try to rename a page at the same time (to probably different names). Or one tries to delete, and the other to rename. I think it's acceptible for the second one to get an error message that the page no longer exists. + Update: check, that happens * Alice renames a page; Bob had edit window open for it, and saves it afterwards, under old name. I think it's acceptible for Bob to succeed in saving it under the old name in this case, though not ideal. -* Alice renames (or deletes) a page. In the meantime, Bob is uploading an - attachment to it, and finishes after the rename finishes. Is it - acceptible for the attachment to be saved under the old name? + Update: Behavior is the same as if Alice renamed the page and Bob created + a new page with the old name. Seems acceptable, though could be mildly + confusing to Bob (or Alice). * Alice starts creating a new page. In the meantime, Bob renames a different page to that name. Alice should get an error message when committing; and it should have conflict markers. Ie, this should work the same as if Bob had edited the new page at the same time as Alice did. + Update: That should happen. Haven't tested this case yet to make sure. * Bob starts renaming a page. In the meantime, Alice creates a new page with the name he's renaming it to. Here Bob should get a error message that he can't rename the page to an existing name. (A conflict resolution edit would also be ok.) + Update: Bob gets an error message. +* Alice renames (or deletes) a page. In the meantime, Bob is uploading an + attachment to it, and finishes after the rename finishes. Is it + acceptible for the attachment to be saved under the old name? + Update: Meh. It's certianly not ideal; if Bob tries to save the page he + uploaded the attachment to, he'll get a message about it having been + deleted/renamed, and he can try to figure out what to do... :-/ +* I don't know if this is a conflict, but it is an important case to consider; + you need to make sure that there are no security holes. You dont want + someone to be able to rename something to /etc/passwd. + I think it would be enough that you cannot rename to a location outside + of srcdir, you cannot rename to a location that you wouldn't be able + to edit because it is locked, and you cannot rename to an existing page. + + > Well, there are a few more cases (like not renaming to a pruned + > filename, and not renaming _from_ a file that is not a known source + > file or is locked), but yes, that's essentially it. + > + > PS, the first thing I do to any + > web form is type /etc/passwd and ../../../../etc/passwd into it. ;-) --[[Joey]]