IWikiPageManipulator.validate_wiki_page not called with the correct data

[benjamin@…]

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.

[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?

[Christian Boos]

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.

[benjamin@…]

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.

[Christian Boos]

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.

[Remy Blank]

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.

[osimons]

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

[Christian Boos]

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)

[Remy Blank]

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.

[Christian Boos]

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.

[Remy Blank]

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?

[Remy Blank]

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