Edgewall Software
Modify

Opened 14 years ago

Closed 13 years ago

#7731 closed defect (fixed)

IWikiPageManipulator.validate_wiki_page not called with the correct data

Reported by: benjamin@… Owned by: Remy Blank
Priority: normal Milestone: 0.12
Component: wiki system Version:
Severity: normal Keywords: IWikiPageManipulator
Cc: Branch:
Release Notes:
API Changes:
Internal Changes:

Description

I am trying to write a small piece of software that contains some special (parseable) wiki pages. To force the correct format, I wanted to use the IWikiPageManipulator but noticed that the page contend has not been updated.

The problem is that the page object that is passed to the IWikiPageManipulator contains the old text, instead of the new text that has just been posted. page.text should be set to req.args.get('text') right away, but the change should only written to the datastore later on.

As a workaround I can of course get the text that has been posted, but that seems totally wrong.

Attachments (0)

Change History (11)

in reply to:  description comment:1 by Remy Blank, 14 years ago

Milestone: 0.11.3
Owner: set to Remy Blank

Replying to benjamin@…:

The problem is that the page object that is passed to the IWikiPageManipulator contains the old text, instead of the new text that has just been posted.

True. And the documentation for IWikiPageManipulator.validate_wiki_page() says:

Validate a wiki page after it's been populated from user input.

page.text should be set to req.args.get('text') right away, but the change should only written to the datastore later on.

That sounds sensible indeed. Does anybody know if this was intentional?

comment:2 by Christian Boos, 14 years ago

At least the current behavior is useful because otherwise you wouldn't have a way to access to the old content. Okay, you could look up the previously stored wiki page again from the database, but that wouldn't be that clean either.

What we should really do is to have an API more uniform across resources, e.g. having the possibility to check against "_old" values like we do for tickets. There, the ITicketManipulators are indeed given tickets containing the "new" value.

comment:3 by benjamin@…, 14 years ago

The WikiPage object already has an old_text variable, which is initilized when the page is loaded from the database. So this is already the case.

comment:4 by Christian Boos, 14 years ago

Oh, good to know ;-)

Nevertheless, I don't think we can "fix" this without breaking all the plugins that were written according to the current behavior.

A kind of upgrade of the API itself (deprecating the old method, introducing a new method) would be needed.

in reply to:  4 comment:5 by Remy Blank, 14 years ago

Milestone: 0.11.30.13

Replying to cboos:

Nevertheless, I don't think we can "fix" this without breaking all the plugins that were written according to the current behavior.

Just to add some data: there are currently only 2 plugins on trac-hacks that implement the IWikiPageManipulator interface: th:HierWikiPlugin and th:TagsPlugin. The former doesn't use the page text at all. The latter gets the new text from req.args.get('text') and gets the old text from page.old_text (see wiki.py), so it wouldn't break if we did the change.

A kind of upgrade of the API itself (deprecating the old method, introducing a new method) would be needed.

Then this is certainly not for 0.11.3.

comment:6 by osimons, 14 years ago

No doubt the SpamFilter plugin also uses the interface to check/veto changes.

in reply to:  6 ; comment:7 by Christian Boos, 13 years ago

Keywords: IWikiPageManipulator added
Milestone: 0.130.12

Replying to osimons:

No doubt the SpamFilter plugin also uses the interface to check/veto changes.

And #8257 precisely raised this issue for the SpamFilter.

I just applied the patch proposed there to the spam-filter branches, but I think cleaning up the API would be in order for 0.12.

I think we have two options:

  • passing the new page which has the old_text property instead of the old page (as discussed above)
  • changing the method signature validate_wiki_page(self, context, old_page, new_page). This has the advantage of making the semantic change impossible to miss (and even leaves the door opened for supporting the legacy signature validate_wiki_page(self, req, old_page) using arity)

in reply to:  7 ; comment:8 by Remy Blank, 13 years ago

Replying to cboos:

I just applied the patch proposed there to the spam-filter branches, but I think cleaning up the API would be in order for 0.12.

The first option seems more in line with ITicketManipulator, so I'll go with that and add a note in the API changes document.

in reply to:  8 ; comment:9 by Christian Boos, 13 years ago

Replying to rblank:

… so I'll go with that and add a note in the API changes document.

Would be nice to illustrate the API change in a 0.12 branch of the SpamFilter.

[OT] In there, we could also get rid of the .cs files.

in reply to:  9 comment:10 by Remy Blank, 13 years ago

Fixed in [8429].

Replying to cboos:

Would be nice to illustrate the API change in a 0.12 branch of the SpamFilter.

Err, is it worth creating a new branch for the plugin only for this small change?

comment:11 by Remy Blank, 13 years ago

Resolution: fixed
Status: newclosed

API change documented in TracDev/ApiChanges/0.12@4.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Remy Blank.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Remy Blank to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.