TimerService: make TimerResponse support redirect()

This allows executing a script doing REQUEST.RESPONSE.redirect() without
error.

Before this patch it's an AttributeError similar to:

    ------
    2022-04-12 03:52:49,083 WARNING ActivityTool Could not call method ...
    Traceback (most recent call last):
      ...
      File "Script (Python)", line 34, in Base_redirect
        return request.RESPONSE.redirect(redirect_url, status=status_code)
    AttributeError: TimerResponse instance has no attribute 'redirect'

product/TimerService/timerserver/TimerServer.py

@@ -125,6 +125,9 @@ class TimerResponse(BaseResponse):
     def _finish(self):
         pass
 
+    def redirect(self, *args, **kw):
+        pass
+
     def unauthorized(self):
         pass
 

[Vincent Pelletier]

Is it really good to silently ignore a redirection ? Wouldn't it make more sense to just not redirect in activities to begin with ?

[Jérome Perrin]

You think explicitly raising an exception explaining redirecting does not make sense in this context would be a better behavior ? I am still not sure what would be the best behavior. Many of the scripts used as action scripts of dialogs redirect to the dialog with a status message when the input is not correct, this would be an example of silently ignoring incorrect input, so maybe it's better to raise.

[Vincent Pelletier]

I mean that not implementing a redirect method should be enough to mean "there is no such feature", so I do not see much value in implementing it only to then make it say that it is a mistake to call it by raising some exception.

At most, I would document in the code that redirection is intentionally not supported, and not just an omission.

[Jérome Perrin]

What I had in mind with this method raising would be mostly for "documentation" purposes. Not having the method is unclear whether it's not implemented because it's something not supported or just something not implemented - which is what lead me to making this change

[Jérome Perrin]

!1618 (closed) ?

[Vincent Pelletier]

Not having the method is unclear whether it's not implemented because it's something not supported or just something not implemented

This is exactly what I meant by:

At most, I would document in the code that redirection is intentionally not supported, and not just an omission.

IOW, is there anything wrong with:

# redirect: not implemented, it is meaningless for activities and activity code should be fixed.

Because I do not like implementing interfaces just to visibly not actually implement them.

To push it to its caricature, should we also implement __add__, __div__ etc to make it clear that we did not just forget that it is meaningless to try to add/divide/... response objects ? I think not.

[Jérome Perrin]

IOW, is there anything wrong with:

# redirect: not implemented, it is meaningless for activities and activity code should be fixed.

It's not "wrong", but I did not know where to put this comment so that it is easily found, but that class is really small, so we could put a comment at class level explaining that this class only implements the subset of HTTPResponse that makes sense in this context. Comparing https://github.com/zopefoundation/Zope/blob/4dfe4360931d986427171db661285f74c4e010f7/src/ZPublisher/BaseResponse.py and https://github.com/zopefoundation/Zope/blob/4dfe4360931d986427171db661285f74c4e010f7/src/ZPublisher/HTTPResponse.py the differences seems to be redirect and some methods related to cookies handling (but not all, the difference between BaseResponse and HTTPResponse is not really clear - as we can see in this "what should be here ?" comment).

BTW, do you know why unauthorized is implemented as a pass here ?

[Vincent Pelletier]

I also do not quite understand the distinction between these classes. This feels like a case of trying to be generic in a design while still having essentially only a single use-case, which leads to such unclear role separation.

BTW, do you know why unauthorized is implemented as a pass here ?

(just to prepare for my use of the plural below: I add _unauthorized in the same basket)

My guess is these get called by the publisher during exception handling, so we cannot easily change it to behave nicely in the execution context of an activity. But I am not 100% sure: maybe we could instead catch such exceptions somewhere in the process_timer codepath ?

[Jérome Perrin]

About the distinction between the classes, what's also interesting is that FTPResponse inherits from HTTPResponse and not BaseResponse, but maybe it's because there is no ZServer's BaseResponse.

I tried to write some words to describe why there would not be a redirect method on this class, but I can not find a good explanation why we have a "silently do nothing unauthorized" but no redirect method.

I'm thinking I will just revert this commit, to go back to the "it's not clear why there is no redirect method in this class".

[Vincent Pelletier]

I'm thinking I will just revert this commit, to go back to the "it's not clear why there is no redirect method in this class".

I would rather say "it's not clear why there are {,_}unauthorized methods on this class", but the absence of redirect is pretty clear to me: redirections during an activity are totally meaningless and do not need any support.

[Jérome Perrin]

By "it's not clear" I was talking about the fact that we don't know if TimerResponse does not have a redirect because it was intentionally not implemented in the initial timerserver code. Anyway, that's true that redirecting does not make sense in the usages we make of timer server, so I'm reverting back to the state where it's an error. Thanks !