The afterEditHandler is called inconsistently, which makes a beforeEditHandler / afterEditHandler pair asymmetric, e.g. content can't be changed and reverted in a consistent way.
To reproduce:
- Create a test plugin
- in
beforeEditHandler
add this: $_[0] =~ s/_FOO_/_BAR_/go;
- in
afterEditHandler
add this: $_[0] =~ s/_BAR_/_FOO_/go;
- Create a test topic with
_FOO_
in it
- Edit topic
- You see
_BAR_
instead of _FOO_
(as expected)
- Hit preview
- You see
_FOO_
again (as expected)
- Hit save
- Topic text contains
_FOO_
(as expected)
- Edit topic again
- You see
_BAR_
instead of _FOO_
(as expected)
- Hit save
- Topic text contains
_BAR_
(this is a bug)
--
PTh
This is a partial fix that calls
afterEditHandler
also in save:
===================================================================
--- TWiki/UI/Save.pm (revision 10838)
+++ TWiki/UI/Save.pm (working copy)
@@ -457,6 +457,8 @@
my( $newMeta, $newText, $saveOpts, $merged ) =
TWiki::UI::Save::buildNewTopic($session, 'save');
+ $session->{plugins}->afterEditHandler( $newText, $topic, $web );
+
try {
$store->saveTopic( $user, $web, $topic,
$newText, $newMeta, $saveOpts );
This is checked in TWiki 4 (10846) and DEVELOP (10847)
This is a partial fix because a edit / preview / save cycle does now a
beforeEditHandler
/
afterEditHandler
/
afterEditHandler
instead of just a
beforeEditHandler
/
afterEditHandler
. The save after a preview needs to omit the
afterEditHandler
call since it has already been done in preview.
Any idea how to detect that save is called from preview not edit, so that the
afterEditHandler
call can be made conditionally?
--
PTh
i don't think there is any existing way to do that.
Are you sure this approach is the right solution though? I left the function of the
afterEditHandler
where it was (in preview), because by moving it to save, you are assuming a
save
is a result of an edit, which it may not be. Simply duplicating the callpoint of the
beforeSaveHandler
is effectively what you have done, and probably not what you wanted to do. You have to consider all the following scenarios:
- Checkpoint saves
- Save invoked from preview
- REST saves
- Save after edit
You only actually want to invoke an
afterEditHandler
on 1. and 4.
CC
I already checked 1 and 4, they work just fine. Problem is 2, it needs to be treated specially to avoid a double call. I carefully read the code, it is solid except for the one case.
If there is no easy eay to detect if save is called via preview we need to add an extra hidden parameter to preview template for save to identify the case. I'd like to avoid that since it is fragile and depends on skins.
--
PTh
I agree there is currently (after checkins) a problem with
- 3) - only one "last half" is applied there
- 2) - "last half" is applied twice.
I suppose
afterEditHandler
need to be aware of "state" in some sense, but instead of adding a state variable perhaps it is enough to look at current/referring binary and act on the combination (i.e. something like this):
Current binary |
Referring binary |
Action |
preview |
edit |
Apply handler logic (preview) |
save |
preview |
No action (save after preview) |
save |
edit |
Apply handler logic (checkpoint save, standard save) |
save |
other or n/a |
No action (REST?) |
--
SP
I investigated the query parameters. Based on those I fixed the code so that
afterEditHandler
is called exactly once for preview/save, checkpoint, and direct save action. TWiki does now a proper
beforeEditHandler
/
afterEditHandler
roundtrip.
cmd=repRev and delRev do not call the
beforeEditHandler
/
afterEditHandler
as expected.
Also tested with classic skin.
SVN 10882.
--
PTh
Closed in 4.0.5
KJL