Deprecate viewrev action

[tmortagne]

Hi everyone,

I think 13.0 is a good occasion to fix https://jira.xwiki.org/browse/XWIKI-13794.

Most code in pages is designed to expect the action to be == ‘view’ when viewing a page, but it’s not the case when viewing a page revision (it’s “viewrev”) so it always comes as a surprise when it’s badly displayed.

ViewAction already support the rev URL parameter but currently we redirect to viewrev action, so I would like to propose that we simply deprecated /viewrev/ action and always use /view/. In practice, it means doing the opposite of the current behavior and redirect /viewrev/ to /view/.

The only thing that will change is the redirect since the execution of the revision will work exactly the same way only with “view” as context action instead of “viewrev” so I don’t think this can really cause much retro compatibility issues.

WDYT ?

[surli]

@tmortagne

The only thing that will change is the redirect since the execution of the revision will work exactly the same way only with “view” as context action instead of “viewrev” so I don’t think this can really cause much retro compatibility issues.

What about possible custom scripts that does perform different output depending on “viewrev” context action? We might not have any in XS but they could exist in some extensions. Would that be possible to provide a new context key/value to check that case? Or maybe it exists already?

@tmortagne

ViewAction already support the rev URL parameter but currently we redirect to viewrev action, so I would like to propose that we simply deprecated /viewrev/ action and always use /view/.

In term of implem, does it mean also removing the ViewrevAction, in favor of handling everything in ViewAction? Those are not internal I think.

[tmortagne]

@surli

What about possible custom scripts that does perform different output depending on “viewrev” context action?

I honestly don’t think this is a use case. Keep in mind that we are talking about displaying older versions of a document, and I’m sure this change will fix a lot more displays that it will break.

@surli

does it mean also removing the ViewrevAction

No because /viewrev/ URLs have to keep leading somewhere even if it’s then redirected.

[vmassol]

@tmortagne

No because /viewrev/ URLs have to keep leading somewhere even if it’s then redirected.

We should still move ViewrevAction to legacy though.

@tmortagne

WDYT ?

Sounds good in general.

I’m still unsure about the backward compatibility breakage level: All URLs created with “viewrev” action will continue to work (thanks to legacy) but checks for == "viewrev" might fail. We could check this by reviewing the 28 occurrences of “viewrev” in xwiki-contrib for a start: https://github.com/search?q=org%3Axwiki-contrib+viewrev&type=code

Thanks

[tmortagne]

@vmassol

We should still move ViewrevAction to legacy though.

Yes. Note that we have various deprecated actions which haven’t been moved because it was not possible, but it’s easy now that actions are XWiki components.

[tmortagne]

I applied the modificaion with a slight change of plan: I did not make /viewrev/ action redirect to /view/ just yet, we can always do that later.